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

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/actors

• #5594 feat(container-runner): drain engine sleeps unless the idle …
• #5595 feat(container-runner): jitter idle timeout to avoid teardow …
• #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 28, 2026 at 7:21 pm
kitchen-sink	😴 Sleeping (View Logs)	Web	Aug 28, 2026 at 5:26 am
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 on the current merge commit (346d4c72, same single commit as previously reviewed). Re-traced the dedup/ack interaction independently from scratch; conclusions match the prior review, 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 in events.rs), 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:23-27), so a replayed stop that gets skipped 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 or already-removed actors, covered by unknown_actor_stop_is_acked.
• send_stop_command_acks never clears the dedup entry regardless of send success. That is 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. This correctly defers to the existing TODO-documented ack-bytes-sent-vs-FDB-commit race window rather than introducing a new one.
• Verified server-side handling in pegboard-envoy/src/ws_to_tunnel_task.rs::ack_commands: it does a clear_range up to and including the checkpoint index, which is idempotent. So the redundant re-ack every periodic tick until the dedup entry is finally cleared (documented behavior) is harmless, not a correctness or resource issue.
• 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, start and stop, 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 and conventions: matches CLAUDE.md, full-sentence comments without dash-fragments, no wildcard 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 and performance: no security concerns, this is envoy-client-side protocol bookkeeping on data already flowing through the (untrusted per Trust Boundaries) envoy to pegboard-envoy channel; no new unbounded state, since processed_command_idx is still cleared on each successful ack tick. Performance impact is negligible: one small extra map lookup and websocket send per batch that contains a stop.

Test coverage: 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 main branches well: immediate ack, failed-send retry via dedup retention, unknown-actor stop, and periodic re-ack of live actors.

Minor, non-blocking nits:

1. send_stop_command_acks builds an intermediate map from a typically single-element slice just to funnel through checkpoints_from, which flattens it back to a vector. Could build the checkpoint vector 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 that 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.