fix(envoy-client): ack terminating stop commands so pegboard-envoy stops replaying them

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/actors

• #5565 fix(envoy-client): ack terminating stop commands so pegboard … 👈
• main

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

_{change zrklppqy}

[railway-app]

🚅 Deployed to the actors-pr-5565 environment in rivet-frontend

Service	Status	Web	Updated (UTC)
website	❌ Build Failed (View Logs)	Web	Aug 13, 2026 at 5:09 am
kitchen-sink	😴 Sleeping (View Logs)	Web	Aug 12, 2026 at 7:46 pm
mcp-hub	✅ Success (View Logs)	Web	Aug 12, 2026 at 7:34 pm
frontend-inspector	❌ Build Failed (View Logs)	Web	Aug 12, 2026 at 7:34 pm
ladle	❌ Build Failed (View Logs)	Web	Aug 12, 2026 at 7:34 pm
frontend-cloud	❌ Build Failed (View Logs)	Web	Aug 12, 2026 at 7:34 pm

[claude]

Review

Clean, well-motivated fix. Sourcing acks from processed_command_idx (which survives remove_actor) instead of the live ctx.actors map correctly closes the bug where a fast-stopping actor's checkpoint never made it into an ack before the entry was removed, causing pegboard-envoy to replay the stop forever. The three new tests cover the core scenarios well (immediate ack, retained-on-send-failure + replay retry, unknown-actor stop).

A few things worth a look:

1. The immediate ack sweeps the entire processed_command_idx map, not just the stop's own checkpoint.
batch_has_stop only gates whether to fire send_command_ack_inner(ctx, false), but that function still builds last_command_checkpoints from every entry in ctx.processed_command_idx (commands.rs:108). So any batch containing a stop, even a duplicate/replayed one, now also immediately acks and clears (server-side) the replay safety net for unrelated, just-processed CommandStartActors in the same batch, rather than waiting for the 5-minute periodic tick (ACK_COMMANDS_INTERVAL_MS). The existing TODO above (commands.rs:139-148) already documents a narrow race where an ack can be committed by pegboard-envoy before the local dedup map reflects it; this change increases how often that ack fires for freshly-started actors, shrinking the crash-recovery window (envoy process dies right after a start is acked but before the actor is durably initialized) compared to before. Worth confirming this is intentional/acceptable, since it's a meaningful behavior change beyond "ack stops promptly", or scope the immediate ack to just the stop checkpoints if unintended.

2. ActorEntry::last_command_idx is now dead.
After switching ack sourcing to processed_command_idx, nothing in the crate reads entry.last_command_idx anymore (still written in commands.rs:74 and via insert_actor's last_command_idx param, but never read, confirmed via grep across the crate). Since the field/param are pub, this won't trigger a dead_code warning, but it's vestigial after this refactor. Consider removing it (and the insert_actor parameter) unless it's meant to stay for a future purpose.

3. Minor comment accuracy nit.
The comment at commands.rs:90-92 ("Ack a stop immediately since its actor is removed before the periodic tick") implies removal happens synchronously with the stop command. Looking at events.rs:14-36, remove_actor is actually only called later, when the actor's own Stopped state-update event arrives with received_stop already set, a separate, later event. Not wrong in effect (removal can still race ahead of the 5-minute tick), but slightly imprecise about when/why; might be clearer as "...may be removed before the next periodic tick."

4. Test coverage suggestion.
Given point 1, a test pinning the "a stop in the batch also flushes unrelated pending checkpoints (e.g., a start for a different actor in the same or a prior batch)" behavior would help make that scope-widening explicit and regression-proof, since it's the main behavioral change here beyond the targeted stop-ack fix.

Nothing blocking, the core fix is sound and matches the stated goal (stop pegboard-envoy from replaying acked-but-forgotten stops). Points 1 and 2 are the ones I'd want a second opinion on before merging.

[claude]

Review

Solid, well-targeted fix for the stop-command ack gap. The core insight, detecting a stop on the raw batch before dedup so a replayed/skipped stop still re-triggers an ack, correctly closes the hole where a stopped actor's entry disappears from ctx.actors before the 5-minute periodic tick ever gets a chance to ack it, leaving pegboard-envoy replaying the stop forever.

I traced through the single-threaded envoy_loop tokio::select! to confirm there's no race between handle_commands and the periodic send_command_ack tick (both run to completion sequentially, so the immediate ack always sees the just-stopped actor before any later message can call remove_actor). The four new tests (stop_command_is_acked_immediately, stop_ack_retried_via_replay_after_failed_send, unknown_actor_stop_is_acked, live_actor_is_reacked_on_each_tick) cover the important paths: fresh stop, replayed stop after a failed send, a stop for an actor that was never known locally, and the pre-existing periodic re-ack behavior. Good use of real channels/queues per the repo's no-mocking testing convention.

Minor / non-blocking:

1. Perf: the immediate ack is unscoped to the batch. send_command_ack_inner(ctx, false) rebuilds the entire highest map (every live actor plus every processed_command_idx entry) on every batch that contains a stop, not just the checkpoint(s) for the actor(s) that stopped. On a process hosting many actors with routine start/stop churn (e.g. serverless-style short-lived actors), this turns each stop into an O(total tracked actors) message instead of O(1), and since it fires per-batch rather than per-tick, it could meaningfully increase WS traffic under high churn. Worth considering whether the immediate ack should carry just the newly-orphaned/stopped checkpoints, leaving the periodic tick to do the recover-an-unconfirmed-ack full re-scan it already does. Not a correctness bug, just a scaling consideration if actor density per envoy grows.

2. Nit: the merge loops build highest with entry(...).or_insert(x); *slot = slot.max(x), which always does an entry lookup/clone even when the key is already present and does not need updating. entry(...).and_modify(|v| *v = (*v).max(x)).or_insert(x) avoids the redundant clone+insert on the common already-seen-no-change path. Trivial, skip if not worth the diff.

3. The updated comment above the dedup-clear TODO (about removed actors whose stops are acked here) is accurate and appreciated. It correctly extends the existing documented race-window caveat rather than treating it as newly introduced. No action needed, just noting the documentation is good.

No security or protocol-versioning concerns. This only touches in-memory ack bookkeeping on the trusted engine to envoy-client command channel, does not touch the wire schema, and does not need runner-protocol version bumps.