Skip to content

test(rivetkit): cover failed manual startup initialization - #5438

Merged
NathanFlurry merged 1 commit into
mainfrom
oncreate-idempotency
Jul 19, 2026
Merged

test(rivetkit): cover failed manual startup initialization#5438
NathanFlurry merged 1 commit into
mainfrom
oncreate-idempotency

Conversation

@NathanFlurry

Copy link
Copy Markdown
Member
  • Add regression coverage for manual startup failures before initialization is durably marked complete.
  • Verify retry startup still sees the actor as uninitialized and only commits the flag after success.

@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5438 July 19, 2026 17:47 Destroyed
@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Review

This is a test-only PR adding regression coverage for a real correctness invariant: a manual-startup runtime that fails before signaling startup_ready must not leave has_initialized = true durably persisted, so a retry is correctly treated as a fresh (is_new) startup.

Verified against the actual implementation (rivetkit-rust/packages/rivetkit-core/src/actor/task.rs::start_actor/spawn_run_handle):

  • For a new actor with requires_manual_startup_ready() == true, the CoreInit phase intentionally skips set_has_initialized(true) + persist_state(...) (line ~1183), deferring to the runtime preamble.
  • If the manual runtime's startup_ready reply is Err(...), spawn_run_handle propagates the error via .context("runtime startup preamble")? before start_actor ever reaches the PostReady fallback block that would set has_initialized.
  • So the test's assertion that maybe_load_persisted_actor(&ctx).await.is_none() after a failed start, followed by the retry observing has_initialized == false and only persisting true after a successful manual Ok(()) reply, is an accurate exercise of this code path. Good regression coverage for a bug class that would otherwise silently corrupt the "is this actor new" determination on retry.

Style/consistency:

  • The new test closely mirrors the existing sibling test directly above it (manual_startup_does_not_mark_initialized_before_runtime_preamble), reusing the same std::sync::Mutex-wrapped oneshot pattern for observing has_initialized inside the closure, and the same shared-kv fixture trick (kv.clone() then reusing kv) to give both ActorContexts the same underlying in-memory sqlite fixture (confirmed via Kv::test_identity() returning a stable Arc::as_ptr, so the fixture key matches across the two contexts). This is consistent with the file's established conventions.
  • maybe_load_persisted_actor is a small, appropriately-scoped addition alongside the existing load_persisted_actor helper.
  • Correctly follows the repo's "Rust tests live under tests/" convention — this is already in tests/task.rs.

Minor nit (non-blocking): the first (task) run handle from the failing manual-startup attempt is never explicitly .abort()/awaited, unlike retry_task's at the end. This is harmless here since the closure sends its error and returns immediately (no long-lived loop), but for consistency with the rest of the file's tests you could join/abort it too.

No production code is touched, so there's no functional, performance, or security risk from this change. Nice, tightly-scoped regression test.

@NathanFlurry
NathanFlurry merged commit b9b1187 into main Jul 19, 2026
10 of 12 checks passed
@NathanFlurry
NathanFlurry deleted the oncreate-idempotency branch July 19, 2026 17:56
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