Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@railway-app
Copy link

railway-app bot commented Jan 22, 2026

🚅 Deployed to the rivet-pr-4002 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-cloud ❌ Build Failed (View Logs) Web Jan 30, 2026 at 8:30 am
website ❌ Build Failed (View Logs) Web Jan 30, 2026 at 8:21 am
frontend-inspector ❌ Build Failed (View Logs) Web Jan 30, 2026 at 8:20 am
mcp-hub ✅ Success (View Logs) Web Jan 30, 2026 at 8:19 am

Copy link
Member Author

NathanFlurry commented Jan 22, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Jan 22, 2026

PR Review: Add run lifecycle hook to RivetKit Actors

This PR adds a new run lifecycle hook to RivetKit actors for long-running background tasks. The implementation is well-designed with good test coverage and documentation.


✅ Strengths

1. Clean API Design

The run hook API is intuitive and follows existing patterns:

  • Properly integrates with the lifecycle (called after onWake)
  • Uses c.abortSignal for graceful shutdown (consistent with web standards)
  • Crash-on-exit behavior is appropriate for background tasks

2. Robust Implementation

The core implementation in src/actor/instance/mod.ts:917-966 is solid:

  • Non-blocking startup (#startRunHandler does not await)
  • Proper timeout handling during shutdown
  • Good logging at key lifecycle points
  • Handles both sync and async return values

3. Comprehensive Test Coverage

The test suite (actor-run.ts) covers all critical scenarios:

  • Normal execution with tick loops
  • Queue consumption patterns
  • Graceful shutdown via abort signal
  • Error handling (early exit + thrown errors)
  • Comparison with actors without run handlers

4. Excellent Documentation

The lifecycle docs (website/src/content/docs/actors/lifecycle.mdx:237-316) are clear and provide practical examples.


🔍 Issues & Suggestions

1. Race Condition: State Mutation During Shutdown ⚠️

Issue: The run handler can mutate state during/after the abort signal fires, potentially causing inconsistent state persistence.

Location: rivetkit-typescript/packages/rivetkit/fixtures/driver-test-suite/run.ts:14-38

After the loop exits (when abortSignal.aborted === true), the code sets c.state.runExited = true. However, state persistence might have already occurred or be in progress during shutdown.

Recommendation:

  • Document that state mutations after abort signal fires may not be reliably persisted
  • Consider adding a warning in the docs about state mutation timing during shutdown
  • Alternatively, ensure state is saved after #waitForRunHandler completes in the shutdown flow

Impact: Medium - could cause confusing behavior where final state updates are lost


2. Test Flakiness Risk ⚠️

Issue: Tests rely heavily on timing with hardcoded delays.

Locations:

  • actor-run.ts:14 - waitFor(driverTestConfig, 100)
  • actor-run.ts:27 - waitFor(driverTestConfig, 200)
  • actor-run.ts:55 - waitFor(driverTestConfig, RUN_SLEEP_TIMEOUT + 300)

Problem: In CI environments or under load, these timing assumptions may not hold, leading to flaky tests.

Recommendation:

  • Add retry logic or use polling with timeout instead of fixed delays
  • Document that these tests may need adjustment for different driver implementations

Impact: Low-Medium - tests may become unreliable in some environments


3. Unhandled Edge Case: Synchronous run Handler

Issue: If a user provides a synchronous (non-Promise) run handler, it blocks startup.

Location: src/actor/instance/mod.ts:922-941

If run returns a non-Promise, no warning is issued and crash-on-exit behavior would not apply.

Recommendation:

  • Add a runtime warning if run returns a non-Promise value
  • Or update TypeScript types to enforce Promise return

Impact: Low - edge case, but poor developer experience if encountered


4. Config Schema Cleanup 📝

The removal of provision_users, ClickHouseUser, and ClickHouseUserRole from engine/artifacts/config-schema.json seems unrelated to adding the run lifecycle hook. Consider mentioning this in the PR description or splitting into separate commits.

Impact: None - just organizational


5. Documentation: Abort Signal Pattern Could Be Simplified

The abort signal pattern in the docs is verbose. Consider showing a simpler Promise.race alternative or adding a utility helper to RivetKit.

Impact: Low - quality of life improvement


🔒 Security Considerations

✅ No security concerns identified. The feature properly scopes access through context, uses standard abort signal patterns, and has appropriate timeout protection.


🚀 Performance Considerations

Good performance characteristics:

  • Non-blocking startup
  • Configurable timeout prevents hanging during shutdown
  • No busy-waiting loops

⚠️ Potential concern:

  • If many actors use CPU-intensive run handlers, it could impact overall system performance
  • Consider documenting best practices (e.g., yielding to event loop, avoiding tight loops)

📋 Summary

This is a well-executed feature addition. The implementation is solid, tests are comprehensive, and documentation is clear. The main concerns are:

  1. State mutation race condition during shutdown (medium priority)
  2. Test timing reliability (low-medium priority)
  3. Sync run handler edge case (low priority)

Recommended Actions Before Merge:

  1. Address the state persistence race condition (document or fix)
  2. Consider adding retry/polling to timing-sensitive tests
  3. Add validation or warning for non-Promise run return values

Overall Assessment: ✅ Approve with Minor Revisions

The core functionality is sound and ready for use. The suggested improvements would increase robustness but are not blockers for merge.


Reviewed by Claude Code on behalf of the Rivet team

Comment on lines +917 to +942
#startRunHandler() {
if (!this.#config.run) return;

this.#rLog.debug({ msg: "starting run handler" });

const runResult = this.#config.run(this.actorContext);

if (runResult instanceof Promise) {
this.#runPromise = runResult
.then(() => {
// Run handler exited normally - this should crash the actor
this.#rLog.warn({
msg: "run handler exited unexpectedly, crashing actor to reschedule",
});
this.startDestroy();
})
.catch((error) => {
// Run handler threw an error - crash the actor
this.#rLog.error({
msg: "run handler threw error, crashing actor to reschedule",
error: stringifyError(error),
});
this.startDestroy();
});
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The run handler only crashes the actor if it returns a Promise. If the run handler is synchronous (returns void), it exits immediately without crashing the actor, which violates the documented behavior.

According to the documentation and comments, if the run handler exits (returns), the actor should crash and reschedule. However, the current code only handles the Promise case.

Fix:

#startRunHandler() {
	if (!this.#config.run) return;

	this.#rLog.debug({ msg: "starting run handler" });

	const runResult = this.#config.run(this.actorContext);

	if (runResult instanceof Promise) {
		this.#runPromise = runResult
			.then(() => {
				this.#rLog.warn({
					msg: "run handler exited unexpectedly, crashing actor to reschedule",
				});
				this.startDestroy();
			})
			.catch((error) => {
				this.#rLog.error({
					msg: "run handler threw error, crashing actor to reschedule",
					error: stringifyError(error),
				});
				this.startDestroy();
			});
	} else {
		// Synchronous return - crash immediately
		this.#rLog.warn({
			msg: "run handler exited unexpectedly (synchronous), crashing actor to reschedule",
		});
		this.startDestroy();
	}
}
Suggested change
#startRunHandler() {
if (!this.#config.run) return;
this.#rLog.debug({ msg: "starting run handler" });
const runResult = this.#config.run(this.actorContext);
if (runResult instanceof Promise) {
this.#runPromise = runResult
.then(() => {
// Run handler exited normally - this should crash the actor
this.#rLog.warn({
msg: "run handler exited unexpectedly, crashing actor to reschedule",
});
this.startDestroy();
})
.catch((error) => {
// Run handler threw an error - crash the actor
this.#rLog.error({
msg: "run handler threw error, crashing actor to reschedule",
error: stringifyError(error),
});
this.startDestroy();
});
}
}
#startRunHandler() {
if (!this.#config.run) return;
this.#rLog.debug({ msg: "starting run handler" });
const runResult = this.#config.run(this.actorContext);
if (runResult instanceof Promise) {
this.#runPromise = runResult
.then(() => {
// Run handler exited normally - this should crash the actor
this.#rLog.warn({
msg: "run handler exited unexpectedly, crashing actor to reschedule",
});
this.startDestroy();
})
.catch((error) => {
// Run handler threw an error - crash the actor
this.#rLog.error({
msg: "run handler threw error, crashing actor to reschedule",
error: stringifyError(error),
});
this.startDestroy();
});
} else {
// Synchronous return - crash immediately
this.#rLog.warn({
msg: "run handler exited unexpectedly (synchronous), crashing actor to reschedule",
});
this.startDestroy();
}
}

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants