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

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/actors

• #5593 fix(rivetkit-core): always signal actor stop handle on teard …
• #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 24, 2026 at 2:44 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

Re-reviewed; the diff is unchanged since the last pass (single commit 203b9d3). Re-traced the dedup/ack interaction independently from scratch and my conclusions match the prior review — findings still stand, no new issues found.

What it fixes: send_command_ack (the periodic tick, every ACK_COMMANDS_INTERVAL_MS = 5 min) previously sourced checkpoints only from ctx.actors. Once an actor's entry is removed (remove_actor, called once the actor reports stopped), its stop's checkpoint could never be acked or have its processed_command_idx entry cleared, so pegboard-envoy would replay the stop command indefinitely. This PR sends an immediate, scoped ack for stopped actors right after handle_commands processes a batch, instead of waiting for the tick.

Correctness — traced and confirmed:

• stopped_actors is collected from the raw batch before the per-command dedup loop (commands.rs:22-27), so a replayed stop that gets continue'd by dedup is still included and re-acked. This is the core of the fix — previously a replayed-and-dropped stop got no ack at all.
• The unconditional ctx.processed_command_idx.insert(...) before the match (runs for every non-replayed command, start or stop) is what lets send_stop_command_acks find and ack stops for unknown/already-removed actors — covered by unknown_actor_stop_is_acked.
• send_stop_command_acks never clears the dedup entry regardless of send success (return value of send_ack_checkpoints is intentionally discarded there). That's consistent with the pre-existing "skip dedup clear on failed send" pattern in send_command_ack, and only the periodic tick clears dedup after a confirmed-sent ack. The updated TODO comment correctly extends the pre-existing ack-bytes-sent-vs-FDB-commit race note to cover resurrection of a just-removed actor — this is an existing, documented, narrow race window, not something newly introduced.
• The send_command_ack merge of ctx.actors and ctx.processed_command_idx (max per key) is safe today because the only two command variants (CommandStartActor/CommandStopActor) keep entry.last_command_idx and the dedup index in lockstep. Worth remembering if a third command type is ever added that doesn't update entry.last_command_idx.

Style/conventions: matches CLAUDE.md — full-sentence comments without dash-fragments, no _ => fallthrough on the Command enum match, hard tabs, structured tracing fields, versioned BARE (vbare::OwnedVersionedData) used in the new test decoder rather than raw serde_bare.

Security/Performance: no security concerns — this is envoy-client-side protocol bookkeeping on data that was already flowing through the (untrusted per Trust Boundaries) envoy↔pegboard-envoy channel before this change; no new unbounded state is introduced since processed_command_idx is still cleared on each successful ack tick. Performance impact is negligible: one small extra HashMap/WS send per batch that contains a stop.

Minor / non-blocking nits:

1. send_stop_command_acks builds an intermediate HashMap<(String,u32), i64> from a typically single-element slice just to funnel through checkpoints_from, which flattens it back to a Vec. Could build the Vec<ActorCheckpoint> directly since stopped_actors keys are already unique within a batch. Not a correctness issue.
2. stop_command_is_acked_immediately doesn't assert ctx.processed_command_idx retains the entry after a successful send (the sibling failed-send test does check retention). Adding that assertion would pin down "only the tick clears dedup" so a future refactor can't silently regress it.
3. No test exercises a batch mixing a stop for one actor with a non-stop command for another actor, to confirm the immediate ack stays scoped to just the stopped actor(s) and doesn't turn into a full-state ack. Low risk given the logic is generic over the stopped_actors list, but would strengthen confidence in the "scoped, not full-state" claim in the comment.
4. The PR description is empty; repo convention is a short bullet list of what changed.

Overall: solid, well-tested fix with correct scoping and no regressions found.