Skip to content

fix(rivetkit): stop lost actor generations immediately - #5824

Open
abcxff wants to merge 1 commit into
stack/fix-envoy-client-signal-lost-actor-generations-synchronously-ykpvpvyofrom
stack/fix-rivetkit-stop-lost-actor-generations-immediately-opswolsp
Open

abcxff wants to merge 1 commit into
stack/fix-envoy-client-signal-lost-actor-generations-synchronously-ykpvpvyofrom
stack/fix-rivetkit-stop-lost-actor-generations-immediately-opswolsp

Conversation

@abcxff

@abcxff abcxff commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

No description provided.

@abcxff

abcxff commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor Author

@claude

claude Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review: fix(rivetkit): stop lost actor generations immediately

I read the first part of the diff closely (CLAUDE.md, docs, context.rs, kv.rs, sleep.rs, sqlite/*, start of task.rs). I only skimmed the registry, NAPI, wasm and TS changes. The design is sound: the lost token is revoked at the storage layer, the actor task takes an abort path, and a generation hold stops the next generation from overlapping the old one. Docs are updated alongside the code.

Possible bugs

  • Stale token in ActorTask. ActorTask::new copies ctx.lost_signal() into self.lost, but configure_lost_signal later replaces the token inside the context. If it runs after the task is constructed, the task watches a token nothing cancels, while is_lost() and the SQLite/KV fences use the new one. Confirm the registry adopts the envoy token before ActorTask::new, read the token at run time, or add a test for this ordering.
  • Replaced token loses state. If mark_lost() fires before configure_lost_signal, the replacement token starts un-cancelled. Cancel the new token if the old one is already cancelled.
  • Wasm is not cancelled on loss. sleep.rs documents that wasm work keeps only the shutdown-deadline policy. User JS can still run on wasm after Lost, which conflicts with the new CLAUDE.md line "no user sleep hooks ... after the lost signal fires". Scope that line or note the gap.
  • is_rollback_statement is exact-match only. ROLLBACK TO sp, ROLLBACK TRANSACTION and /* c */ ROLLBACK are rejected after loss. That fails closed, but check that cleanup paths only send a bare ROLLBACK.
  • Wrong error for KV. LegacyActorKv::ensure_not_lost returns SqliteRuntimeError::Closed for a KV write. Use a dedicated error or a different context message.
  • In-flight fencing. Confirm kv_put_fenced drops a write when the token fires after queueing but before send, and that a test covers it.

Style and nits

  • context.rs: the // Test shim keeps moved tests... comment now sits above the new GenerationHold doc and no longer describes the mod tests below it. Move GenerationHold above the shim comment.
  • sqlite/tx.rs: the set_lost_signal doc comment is attached to is_lost, with a second doc line stacked on top. Split them so each function has its own doc.
  • sqlite/mod.rs: the "recheck after open" block and comment is copy-pasted four times. A small helper would remove the duplication.
  • ensure_not_lost and is_lost lock a parking_lot::Mutex on every statement. This is cheap, but an ArcSwap or a token cloned once avoids the hot-path lock.
  • The PR description is empty. Please add a short bullet summary.

Tests
New tests exist in the core tests/ files (registry, sqlite, task) and the TS lost-generation tests. Please make sure they cover:

  • Lost fires mid-transaction, and only ROLLBACK succeeds.
  • A lost generation stuck in cleanup past 10s makes the next start fail.
  • Lost escalating a pending graceful stop reports StopCode::Error.
  • The token-adoption ordering above.

Any new vi.waitFor in the TS tests needs the adjacent justification comment (pnpm run check:wait-for-comments).

Security
No new trust-boundary concerns. Fencing writes from a superseded generation is a clear improvement for the single-writer invariant.

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No issues found

Reviewed commit 2b3bd85.

@abcxff
abcxff force-pushed the stack/fix-rivetkit-stop-lost-actor-generations-immediately-opswolsp branch from 2b3bd85 to 8d4a713 Compare October 3, 2026 02:05
@abcxff
abcxff force-pushed the stack/fix-envoy-client-signal-lost-actor-generations-synchronously-ykpvpvyo branch from bb0f509 to 14f8833 Compare October 3, 2026 02:05

This branch has not been deployed

No deployments
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.

1 participant