fix(rivetkit-core): scope actor stops to their generation

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/actors

• #5683 fix(rivetkit-core): tolerate workflow not_configured in insp …
• #5684 feat(rivetkit-rust): select serverless vs envoy runner mode …
• #5682 fix(rivetkit-core): scope actor stops to their generation 👈
• #5680 fix(rivetkit-core): ship inspector UI bundle inside the publ …
• #5681 fix(rivetkit): wire abort signal through client get/getOrCre …
• main

Get stack: forklift get 5682
Push local edits: forklift submit
Merge when ready: forklift merge 5682

_{change ymnorrzk}

[claude]

Review: fix(rivetkit-core): scope actor stops to their generation

The core fix is sound: stop_actor now threads the target generation through the whole path (starting-check → pending_stops → transition_actor_to_stopping), and a stop whose generation doesn't match the currently registered/starting instance is treated as stale and its ActorStopHandle is completed instead of silently dropped or (worse) applied to the wrong instance. The exhaustive TransitionResult enum keeps this readable and matches the repo's "no _ => on enums" convention. The three new tests directly exercise the regression (parked-before-start, parked-during-start, and the "still applies to the right generation" guard-rail), which is good coverage for the scenario described.

A few things worth a look before merging:

1. pending_stops can still silently drop a stop's completion handle

stop_actor's two parking sites (the starting_instances check, and the TransitionResult::Vacant branch) both do:

let _ = self.pending_stops.insert_async(actor_id.to_owned(), PendingStop { generation, reason, stop_handle }).await;

scc::HashMap::insert_async does not overwrite an existing key — it returns Err((key, value)) if one is already present (this is also why set_actor_instance_state has to go through entry_async + explicit entry.insert(...) to actually overwrite). So if a second stop_actor call for the same actor_id arrives while a first stop is already parked (e.g. a retried/duplicate CommandStopActor, or stops for two different not-yet-started generations racing), the second call's insert_async fails, the local PendingStop (including the caller's ActorStopHandle) is dropped immediately, and only the first parked stop's reason/generation survives to be evaluated at startup. This isn't a hang — finalize_stop on the envoy-client side treats a dropped completion channel as Err(RecvError) and just logs a warning + finalizes with the precomputed stop code — but the second stop's reason is silently discarded and produces a spurious "completion handle dropped" warning. Given this PR is specifically about handling races around parked/generation-scoped stops, this collision case seems worth handling explicitly (e.g. via entry_async to overwrite, or by completing the older one first) rather than relying on insert_async's not-overwriting default. None of the three new tests park two stops for the same actor_id back-to-back, so this path isn't covered.

2. No log line when a stale generation-mismatched stop is discarded

Both TransitionResult::Stale in stop_actor and the stale-pending-stop branch in start_actor's success arm complete the handle silently with no tracing call. Since this is a fix for a real production incident (per the test doc comment), a tracing::warn!/info! with actor_id, expected generation, and actual generation when a stop is suppressed this way would help — otherwise a recurrence (or a case where the generation mismatch itself is a bug) is invisible in logs.

3. Test-only startup gate in mod.rs looks avoidable

test_hooks::{arm_startup_gate, release_startup_gate, wait_for_startup_gate} adds a #[cfg(test)] await point directly inside production start_actor, backed by a crate-wide static OnceLock<SccHashMap<String, Arc<Semaphore>>>. This looks replaceable without touching mod.rs at all: ActorFactory::new_with_manual_startup_ready + ActorStart::startup_ready already exists precisely to let a test's entry closure block ActorTask::start_actor()'s completion (and therefore the LifecycleCommand::Start reply that RegistryDispatcher::start_actor awaits) until the test releases it — which happens after starting_instances registration and before the instance is set Active, i.e. the exact window these tests need. That would avoid adding a synchronization seam to src/ purely for test purposes.

Separately, STARTUP_GATES is keyed only by a literal actor-id string ("actor-gated", "actor-preparked", "actor-current") and is global across the whole test binary, not scoped to this test file. rivetkit-core's own CLAUDE.md already calls out this exact class of hazard for tests/modules/task.rs (needing test_hook_lock() to avoid parallel-test interference on shared global state) — if any other test in the crate ever calls start_actor with one of these same literal ids, it could unintentionally hang waiting on (or be released by) an unrelated test's gate. Per-test unique ids (e.g. a UUID suffix) would remove that risk cheaply if the gate mechanism is kept.

Minor

• ActorStopHandle::detached() is a reasonable, appropriately #[doc(hidden)] cross-crate test seam (can't use #[cfg(test)] since the consumer is a different crate) — no issue there.
• The TransitionResult/generation-check logic itself is race-safe: the generation comparison happens inside the same entry_async lock acquisition used to mutate the state, so there's no TOCTOU window between checking starting_instances/actor_instances and applying the stop.

Good regression coverage overall; the points above are about tightening an edge case in the new parking logic and reducing the src/ footprint of the test harness, not blockers to the core fix.

🤖 Generated with Claude Code