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

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/actors

• #5585 feat(container-runner): self-sleep on repeated actor start
• #5586 feat(container-runner): sleep on startup idle timeout
• #5589 chore(container-runner): tighten comments
• #5587 feat(container-runner): exit process when the last child sto …
• #5588 feat(container-runner): drain child before SIGTERM on engine …
• #5584 feat(container-runner): drain children and engine concurrent …
• #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 21, 2026 at 4:27 pm
kitchen-sink	😴 Sleeping (View Logs)	Web	Aug 13, 2026 at 5:56 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

I reviewed the diff in engine/sdks/rust/envoy-client/src/commands.rs and its test file. The change adds an immediate ack for CommandStopActor right after handle_commands processes a batch, instead of waiting up to ACK_COMMANDS_INTERVAL_MS (5 min) for the periodic tick, since a stopped actor's entry is removed from ctx.actors before the next tick fires. Without this, the tick would have nothing to re-ack and pegboard-envoy would keep replaying the stop.

Correctness

I traced the logic carefully, including the interaction with dedup (ctx.processed_command_idx) and the existing documented race (the ws_send-vs-FDB-commit gap noted in the pre-existing TODO). The change is a straightforward, correctly-scoped widening of that same known race to also cover removed/stopped actors (the comment at commands.rs:161-163 acknowledges this explicitly), not a new regression. Key invariants hold up:

• stopped_actors is collected from the raw batch before the dedup loop (commands.rs:23-27), so a replayed stop that gets continue'd by dedup (line 46) is still included and re-acked. This is the actual bug being fixed: replays of already-processed stops were previously silently dropped with no ack, causing pegboard-envoy to replay forever.
• send_stop_command_acks deliberately does not clear the dedup entry, matching the doc comment and the existing race-avoidance pattern. Clearing is left to the tick, consistent with the existing "skip dedup clear on failed send" logic.
• The unconditional ctx.processed_command_idx.insert(...) before the match (line 49-50) means even stops for unknown/already-removed actors get an index recorded, which is what lets send_stop_command_acks find and ack them. Covered by the unknown_actor_stop_is_acked test.

No bugs found here.

Minor / nit-level observations

1. engine/sdks/rust/envoy-client/src/commands.rs:105-118 — send_stop_command_acks builds an intermediate HashMap<(String,u32), i64> from a small slice (typically one stopped actor per batch) purely to funnel it through checkpoints_from, which then immediately flattens it back to a Vec. Since stopped_actors already has unique (actor_id, generation) keys per stop command, this could be a direct filter_map over actors into a Vec<ActorCheckpoint> without the hashing/dedup machinery. Not a correctness issue, just avoidable overhead: reusing checkpoints_from/HashMap here trades a few lines of dedup logic for an allocation and hash that is not needed on this path.

2. Test coverage gap — stop_command_is_acked_immediately (command_dedup.rs:289) asserts the ack is sent but does not assert that ctx.processed_command_idx still retains the entry afterward, unlike stop_ack_retried_via_replay_after_failed_send, which does check retention on the failed-send path. Adding that assertion on the success path would pin down the "do not clear dedup here, only the tick clears it" invariant that the code comments call out as important, so a future refactor that accidentally clears dedup in send_stop_command_acks would not slip through green.

3. Multiple simultaneous stops in one batch (stopped_actors with more than one entry) are not explicitly exercised by a test, though the logic is generic over the Vec. Low priority given the single-actor tests already cover the code paths involved.

Style / conventions

Matches CLAUDE.md conventions well: comments are full sentences without dashes, no _ => fallthrough needed since only one enum variant is matched, hard-tab formatting preserved, structured tracing fields used correctly (actor_id = %..., generation = ...).

Security / Performance

No security concerns. This is internal envoy-client protocol handling for ack bookkeeping, not parsing untrusted input differently than before. Performance impact is negligible: one extra small HashMap per batch that contains a stop, at most.

Overall: solid, well-tested fix with a clear, correct rationale. Only the two minor nits above, neither blocking.