fix(broker): persist correlated worker exits - #1750
khaliqgant wants to merge 15 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
263e474 to
8cc940e
Compare
Addresses independent review P2 findings on PR #1750 (#1603): 1. Fleet invocation correlation was looked up from `fleet_inventory` keyed only by worker name at reap time. A same-name replacement worker registering before the old generation was reaped could overwrite that by-name entry, misattributing the new generation's invocation id to the old generation's exit. Correlation now travels on the exited generation's own `WorkerHandle` (captured by `reap_exited` before the handle is removed), with the by-name lookup kept only as a legacy fallback for handles that never carried an invocation id. 2. Hosted `agent_exited` delivery used `hosted_agent_event_tx.try_send`, silently dropping the terminal event on a full or closed channel. Delivery now goes through a bounded backlog: a full channel holds the event for retry (drained at the top of every maintenance tick, preserving order) instead of losing it; a closed channel cannot be retried but is now counted and logged at error level so the miss is observable. The durable crash-insights record saved just before this emission remains the authoritative source of truth either way. Adds deterministic regression coverage: a same-name-replacement test proving old/new invocation ids never cross at both the WorkerRegistry and full maintenance-tick levels, and full/closed-channel backlog tests at both the unit-helper and maintenance-tick levels. Validation: - cargo test -p agent-relay-broker --lib (1099 passed) - cargo test -p relay-pty --lib (251 passed) - cargo clippy -p agent-relay-broker --lib -- -D warnings (clean) - cargo fmt -p agent-relay-broker (clean) Co-authored-by: Cursor <cursoragent@cursor.com>
…e outbox Addresses independent review P2 finding on PR #1750 (#1603): the previous "bounded backlog" for hosted `agent_exited` delivery was purely in-memory. A broker restart lost every still-queued event, a closed hosted-event channel dropped its event permanently with only an invisible internal counter, and there was no way for a hosted/REST client to see or replay what was pending. This replaces that with a durable outbox tied directly to the existing per-exit crash-insights record (`crates/relay-pty/src/crash_insights.rs`): - `CrashRecord` gains a `hosted_delivery: HostedDeliveryState` field (`Pending`/`Delivered`, defaulting to `Pending` so records from older brokers are always replay-eligible) and a `dedupe_key()` of `agent_name::generation` — stable across restarts, and distinct per process generation so a same-name replacement worker's exit can never be conflated with (or accidentally acknowledge) the old generation's. - The maintenance-tick reap path now records+persists the crash record (`Pending`) *before* attempting hosted delivery, so a crash at any point either leaves a `Pending` record (replayed on restart) or, after a successful channel handoff, a `Delivered` one persisted immediately at that handoff — the only "ack boundary" actually available (there is no further downstream ack from the hosted publisher's best-effort HTTP call). - On broker startup, `reload_pending_hosted_agent_exit_backlog` rebuilds the in-memory retry backlog from every still-`Pending` durable record, in original order, bounded by the same `HOSTED_AGENT_EXIT_BACKLOG_CAP` (256) the live path already enforced — so an outage long enough to accumulate a huge pending set still starts with a small, loudly-logged backlog. - A full channel still backlogs and retries every tick (unchanged); a closed channel (permanent for the rest of the process's life — the publisher task is never resupervised) is no longer a silent, invisible loss: the durable record stays `Pending`, discoverable via `GetCrashInsights`'s new `hosted_delivery` block (pending count, in-memory backlog length/cap, drop total) and guaranteed to be replayed on the next restart. - Retention is explicit and unchanged: `CrashInsights::max_records` (500) bounds the outbox file exactly as it already bounded crash history, evicting the oldest pending-or-not record with a loud log rather than growing the file without limit. Deterministic coverage: durable state round-trips through save/load (including UTF-8 agent names), dedupe-by-generation, delivered records are excluded from replay, retention still evicts the oldest pending entries, and a broker-crash-then-restart is exercised end-to-end at the maintenance-tick level (closed channel -> durable `Pending` record -> fresh `CrashInsights::load` -> replay -> redelivery on a live channel) in `restart_before_drain_replays_pending_delivery_from_disk`. Unavoidable limitation: delivery is at-least-once, not exactly-once. A crash that lands between a successful `try_send` and the immediately- following `save()` of the `Delivered` state can cause one redundant replay of an already-delivered event after restart. This is why replay carries `dedupe_key` (`agent_name::generation`) — hosted consumers must treat a repeated key as a no-op, not a new exit. Delivery to Relaycast itself also remains best-effort HTTP inside the publisher task (existing behavior, unchanged): a 5xx/timeout there is logged but not retried by this outbox, which only guarantees the event reaches that task at least once. Validation: - cargo test -p agent-relay-broker --lib (1101 passed) - cargo test -p relay-pty --lib (260 passed) - cargo clippy -p agent-relay-broker --lib -- -D warnings (clean) - cargo clippy -p relay-pty --lib -- -D warnings (clean) - cargo fmt -p agent-relay-broker -p relay-pty (clean) - cargo build --workspace (clean) Co-authored-by: Cursor <cursoragent@cursor.com>
…st success Critical fix from #1750 review: the durable crash record was marked Delivered and persisted right after the local mpsc::try_send handoff to the hosted-event publisher task, before run_hosted_agent_event_publisher completed its real Relaycast HTTP emit. An HTTP timeout or 5xx after that point was permanently lost — the on-disk record already (wrongly) said Delivered, so a broker restart would never replay it. - run_hosted_agent_event_publisher now performs bounded in-process retry/backoff (3 attempts) on timeout or failure, then reports the real outcome back over a new HostedDeliveryOutcome channel. - BrokerRuntime::handle_hosted_delivery_outcome is now the only place that may mark a crash record Delivered, and only does so on a confirmed Relaycast HTTP success. enqueue/drain of the publisher channel is a mere handoff and no longer touches the durable record at all. - On exhausted retries the record stays Pending; broker-restart replay (already-existing outbox reload) remains the fallback, and a new hosted_agent_exit_publish_failures_total counter makes the operator status (GetCrashInsights) truthful about failures instead of silent. - The dedupe_key (agent_name::generation) is now actually carried into the HTTP payload sent to Relaycast (it was computed but never transmitted), so a hosted consumer can itself discard a duplicate replayed delivery. Relaycast's HTTP API has no server-side idempotency key today (external SDK, out of repo) so this remains documented at-least-once, not exactly-once; filed as a companion issue (#1752). - New deterministic tests fake a persistent and a transient Relaycast 5xx/timeout: Pending remains Pending through failure and retries, and only a confirmed success (in-process retry or simulated restart replay) reaches Delivered. - Updated relayflows/1603-raw-spawn-readiness with a note pointing at these new Rust regression tests, since its actions-mocked MCP harness never boots a real local broker/worker/Relaycast loop and so cannot itself exercise this code path. Addresses the critical defect from the fresh review of #1750 (#1603). Co-authored-by: Cursor <cursoragent@cursor.com>
- Crash-insights save failures are no longer log-only: persist() tracks a dirty flag and save_failures_total counter, retried automatically on every event-loop flush (mirrors the existing pending-deliveries/ dead-letters/dedup dirty-flag pattern) until it succeeds. Both are surfaced via GetCrashInsights. Deterministic failure+recovery tests use a blocking-file fixture (no IO mocking, no sleeps). - Fixed the actual off-by-one in retry_pending_delivery: a successful handoff's `attempts` counter was capped at MAX_DELIVERY_RETRIES even though only `failed_attempts` gates the failure budget, causing wait_delivery_successful_handoffs_do_not_exhaust_failure_budget to fail for long-lived Wait-mode deliveries. Cap now only applies on the failure path; updated the transient-blip test's invariant to match (attempts <= calls made, not <= the failure cap). - Updated crash_insights retention tests to reflect the deliberate (not accidental) policy already implemented: the generic cap evicts already-delivered records first and never silently evicts pending hosted-delivery outbox records, even past max_records. Added a retention_pressure_total counter (surfaced via the API) for the case where pressure hits and nothing evictable is found, plus a module-level doc distinguishing bounded/lossy diagnostics from the never-silently-evicted durable pending outbox. - The in-memory hosted-publish backlog (256-entry bound) is now continuously replenished from durable Pending crash records after overflow drops, within the running process (no restart required), via a new maintenance-tick call to replenish_hosted_agent_exit_backlog. A tracked in-flight dedupe-key set (populated on enqueue/backlog-drain, cleared on outcome or on drop) guarantees no duplicate in-flight delivery. Added a deterministic test driving 600 pending records through repeated replenish/deliver passes with no restart, sleeping, loss, or duplication. - Verified PID-disappearance checks (worker::pid_is_gone, PtySession:: has_exited): both only ever kill(pid, 0) an owned, not-yet-reaped child, so ESRCH is an unambiguous "gone" signal (no PID-reuse race is possible before the zombie is reaped) — no bug found, no change made. Full relay-pty and agent-relay-broker --lib suites, clippy -D warnings, and cargo fmt --check are all green; see CHANGELOG.md for the user-visible summary. Co-authored-by: Cursor <cursoragent@cursor.com>
…omment My prior restore used the pre-branch (fa14d5e) version of run.mjs, which dropped the review-follow-up comment an earlier commit on this branch (cbb5a6b) had already added explaining this case's scope limitation. Restore from cbb5a6b instead so that comment is kept. Co-authored-by: Cursor <cursoragent@cursor.com>
…review comment" This reverts commit b28df94.
|
Replaced the seeded outbox simulation with an independently reviewed live fleet-control proof. Exact new head: |
Addresses independent review P2 findings on PR #1750 (#1603): 1. Fleet invocation correlation was looked up from `fleet_inventory` keyed only by worker name at reap time. A same-name replacement worker registering before the old generation was reaped could overwrite that by-name entry, misattributing the new generation's invocation id to the old generation's exit. Correlation now travels on the exited generation's own `WorkerHandle` (captured by `reap_exited` before the handle is removed), with the by-name lookup kept only as a legacy fallback for handles that never carried an invocation id. 2. Hosted `agent_exited` delivery used `hosted_agent_event_tx.try_send`, silently dropping the terminal event on a full or closed channel. Delivery now goes through a bounded backlog: a full channel holds the event for retry (drained at the top of every maintenance tick, preserving order) instead of losing it; a closed channel cannot be retried but is now counted and logged at error level so the miss is observable. The durable crash-insights record saved just before this emission remains the authoritative source of truth either way. Adds deterministic regression coverage: a same-name-replacement test proving old/new invocation ids never cross at both the WorkerRegistry and full maintenance-tick levels, and full/closed-channel backlog tests at both the unit-helper and maintenance-tick levels. Validation: - cargo test -p agent-relay-broker --lib (1099 passed) - cargo test -p relay-pty --lib (251 passed) - cargo clippy -p agent-relay-broker --lib -- -D warnings (clean) - cargo fmt -p agent-relay-broker (clean) Co-authored-by: Cursor <cursoragent@cursor.com>
…e outbox Addresses independent review P2 finding on PR #1750 (#1603): the previous "bounded backlog" for hosted `agent_exited` delivery was purely in-memory. A broker restart lost every still-queued event, a closed hosted-event channel dropped its event permanently with only an invisible internal counter, and there was no way for a hosted/REST client to see or replay what was pending. This replaces that with a durable outbox tied directly to the existing per-exit crash-insights record (`crates/relay-pty/src/crash_insights.rs`): - `CrashRecord` gains a `hosted_delivery: HostedDeliveryState` field (`Pending`/`Delivered`, defaulting to `Pending` so records from older brokers are always replay-eligible) and a `dedupe_key()` of `agent_name::generation` — stable across restarts, and distinct per process generation so a same-name replacement worker's exit can never be conflated with (or accidentally acknowledge) the old generation's. - The maintenance-tick reap path now records+persists the crash record (`Pending`) *before* attempting hosted delivery, so a crash at any point either leaves a `Pending` record (replayed on restart) or, after a successful channel handoff, a `Delivered` one persisted immediately at that handoff — the only "ack boundary" actually available (there is no further downstream ack from the hosted publisher's best-effort HTTP call). - On broker startup, `reload_pending_hosted_agent_exit_backlog` rebuilds the in-memory retry backlog from every still-`Pending` durable record, in original order, bounded by the same `HOSTED_AGENT_EXIT_BACKLOG_CAP` (256) the live path already enforced — so an outage long enough to accumulate a huge pending set still starts with a small, loudly-logged backlog. - A full channel still backlogs and retries every tick (unchanged); a closed channel (permanent for the rest of the process's life — the publisher task is never resupervised) is no longer a silent, invisible loss: the durable record stays `Pending`, discoverable via `GetCrashInsights`'s new `hosted_delivery` block (pending count, in-memory backlog length/cap, drop total) and guaranteed to be replayed on the next restart. - Retention is explicit and unchanged: `CrashInsights::max_records` (500) bounds the outbox file exactly as it already bounded crash history, evicting the oldest pending-or-not record with a loud log rather than growing the file without limit. Deterministic coverage: durable state round-trips through save/load (including UTF-8 agent names), dedupe-by-generation, delivered records are excluded from replay, retention still evicts the oldest pending entries, and a broker-crash-then-restart is exercised end-to-end at the maintenance-tick level (closed channel -> durable `Pending` record -> fresh `CrashInsights::load` -> replay -> redelivery on a live channel) in `restart_before_drain_replays_pending_delivery_from_disk`. Unavoidable limitation: delivery is at-least-once, not exactly-once. A crash that lands between a successful `try_send` and the immediately- following `save()` of the `Delivered` state can cause one redundant replay of an already-delivered event after restart. This is why replay carries `dedupe_key` (`agent_name::generation`) — hosted consumers must treat a repeated key as a no-op, not a new exit. Delivery to Relaycast itself also remains best-effort HTTP inside the publisher task (existing behavior, unchanged): a 5xx/timeout there is logged but not retried by this outbox, which only guarantees the event reaches that task at least once. Validation: - cargo test -p agent-relay-broker --lib (1101 passed) - cargo test -p relay-pty --lib (260 passed) - cargo clippy -p agent-relay-broker --lib -- -D warnings (clean) - cargo clippy -p relay-pty --lib -- -D warnings (clean) - cargo fmt -p agent-relay-broker -p relay-pty (clean) - cargo build --workspace (clean) Co-authored-by: Cursor <cursoragent@cursor.com>
…st success Critical fix from #1750 review: the durable crash record was marked Delivered and persisted right after the local mpsc::try_send handoff to the hosted-event publisher task, before run_hosted_agent_event_publisher completed its real Relaycast HTTP emit. An HTTP timeout or 5xx after that point was permanently lost — the on-disk record already (wrongly) said Delivered, so a broker restart would never replay it. - run_hosted_agent_event_publisher now performs bounded in-process retry/backoff (3 attempts) on timeout or failure, then reports the real outcome back over a new HostedDeliveryOutcome channel. - BrokerRuntime::handle_hosted_delivery_outcome is now the only place that may mark a crash record Delivered, and only does so on a confirmed Relaycast HTTP success. enqueue/drain of the publisher channel is a mere handoff and no longer touches the durable record at all. - On exhausted retries the record stays Pending; broker-restart replay (already-existing outbox reload) remains the fallback, and a new hosted_agent_exit_publish_failures_total counter makes the operator status (GetCrashInsights) truthful about failures instead of silent. - The dedupe_key (agent_name::generation) is now actually carried into the HTTP payload sent to Relaycast (it was computed but never transmitted), so a hosted consumer can itself discard a duplicate replayed delivery. Relaycast's HTTP API has no server-side idempotency key today (external SDK, out of repo) so this remains documented at-least-once, not exactly-once; filed as a companion issue (#1752). - New deterministic tests fake a persistent and a transient Relaycast 5xx/timeout: Pending remains Pending through failure and retries, and only a confirmed success (in-process retry or simulated restart replay) reaches Delivered. - Updated relayflows/1603-raw-spawn-readiness with a note pointing at these new Rust regression tests, since its actions-mocked MCP harness never boots a real local broker/worker/Relaycast loop and so cannot itself exercise this code path. Addresses the critical defect from the fresh review of #1750 (#1603). Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
- Crash-insights save failures are no longer log-only: persist() tracks a dirty flag and save_failures_total counter, retried automatically on every event-loop flush (mirrors the existing pending-deliveries/ dead-letters/dedup dirty-flag pattern) until it succeeds. Both are surfaced via GetCrashInsights. Deterministic failure+recovery tests use a blocking-file fixture (no IO mocking, no sleeps). - Fixed the actual off-by-one in retry_pending_delivery: a successful handoff's `attempts` counter was capped at MAX_DELIVERY_RETRIES even though only `failed_attempts` gates the failure budget, causing wait_delivery_successful_handoffs_do_not_exhaust_failure_budget to fail for long-lived Wait-mode deliveries. Cap now only applies on the failure path; updated the transient-blip test's invariant to match (attempts <= calls made, not <= the failure cap). - Updated crash_insights retention tests to reflect the deliberate (not accidental) policy already implemented: the generic cap evicts already-delivered records first and never silently evicts pending hosted-delivery outbox records, even past max_records. Added a retention_pressure_total counter (surfaced via the API) for the case where pressure hits and nothing evictable is found, plus a module-level doc distinguishing bounded/lossy diagnostics from the never-silently-evicted durable pending outbox. - The in-memory hosted-publish backlog (256-entry bound) is now continuously replenished from durable Pending crash records after overflow drops, within the running process (no restart required), via a new maintenance-tick call to replenish_hosted_agent_exit_backlog. A tracked in-flight dedupe-key set (populated on enqueue/backlog-drain, cleared on outcome or on drop) guarantees no duplicate in-flight delivery. Added a deterministic test driving 600 pending records through repeated replenish/deliver passes with no restart, sleeping, loss, or duplication. - Verified PID-disappearance checks (worker::pid_is_gone, PtySession:: has_exited): both only ever kill(pid, 0) an owned, not-yet-reaped child, so ESRCH is an unambiguous "gone" signal (no PID-reuse race is possible before the zombie is reaped) — no bug found, no change made. Full relay-pty and agent-relay-broker --lib suites, clippy -D warnings, and cargo fmt --check are all green; see CHANGELOG.md for the user-visible summary. Co-authored-by: Cursor <cursoragent@cursor.com>
…leet 429 retry
- Retire tests/relayflows/cases/1603-raw-spawn-readiness (proved a
different, already-fixed raw-spawn-readiness bug and never touched a
real broker/Relaycast boundary). Replace with
1603-hosted-exit-durable-delivery, which spawns the real compiled
agent-relay-broker binary against a fake local Relaycast HTTP server,
seeds the exact on-disk crash-insights.json format with one Pending
hosted-delivery record, and proves at the real binary boundary that
base never replays it on restart (the durable outbox does not exist
there) while head replays it and delivers it through one transient 503
and a successful retry. The Rust regression tests in
crates/broker/src/runtime/{event_loop,tests}.rs remain authoritative for
retry exhaustion, dedupe-on-replay, and >256-record backlog draining.
- Fix two stale doc comments in crates/relay-pty/src/crash_insights.rs:
a try_send channel handoff is not the hosted-delivery acknowledgment
(only confirmed Relaycast HTTP success is, via
BrokerRuntime::handle_hosted_delivery_outcome); the durable pending
outbox is not bounded by max_records (only already-delivered
diagnostic history is). No behavior change.
- Harden the Fleet E2E getAgent test helper (tests/e2e/fleet/harness.ts)
against the free-plan Relaycast 429 rate_limit_exceeded response that
flaked the "Two-node fleet matrix" CI job: bounded, deterministic retry
(honoring Retry-After when present) capped at both attempt count and
wall-clock deadline; every other status/error code still throws
immediately, unchanged. Add tests/e2e/fleet/get-agent-retry.test.ts
covering retry-then-succeed, exhaustion, and non-retry paths.
Co-authored-by: Cursor <cursoragent@cursor.com>
…e it The RelayFlow PR-proof dispatcher requires a PR to touch exactly its one declared case (scripts/pr-proof/prepare.mjs / contract.mjs). Deleting 1603-raw-spawn-readiness (pre-existing on main since #1708) alongside adding 1603-hosted-exit-durable-delivery made the diff touch two case directories and failed dispatch validation. Restore 1603-raw-spawn-readiness unmodified and keep the new case as this PR's sole declared case. Co-authored-by: Cursor <cursoragent@cursor.com>
…omment My prior restore used the pre-branch (fa14d5e) version of run.mjs, which dropped the review-follow-up comment an earlier commit on this branch (cbb5a6b) had already added explaining this case's scope limitation. Restore from cbb5a6b instead so that comment is kept. Co-authored-by: Cursor <cursoragent@cursor.com>
…review comment" This reverts commit b28df94.
e967438 to
b484766
Compare
Addresses #1603.
What changed:
Critical review follow-up (base 355dd24 -> head 6d1baa8)
The initial durable-outbox design marked hosted
agent_exiteddeliveryDeliveredand persisted that right after the localmpsc::try_sendhandoff to the publisher task — before
run_hosted_agent_event_publishercompleted its real Relaycast HTTP emit. An HTTP timeout or 5xx after that
handoff was therefore permanently lost: the durable record already
(wrongly) said
Delivered, so a broker restart would never replay it.Fixed by moving the durable acknowledgment to after the real network
success boundary:
run_hosted_agent_event_publisherperforms bounded in-processretry/backoff (3 attempts) on timeout/5xx and reports the true outcome
back over a new
HostedDeliveryOutcomechannel.BrokerRuntime::handle_hosted_delivery_outcome— the sole owner ofCrashInsights— is now the only place that may mark a recordDelivered, and only on confirmed HTTP success. A mere channel handoffno longer touches the durable record.
Pending; existing broker-restartreplay is the fallback. A new
hosted_agent_exit_publish_failures_totalcounter (exposed via
GetCrashInsights) makes operator status truthfulabout failures instead of silently claiming delivery.
dedupe_key(agent_name::generation) is now actually carried into theHTTP payload sent to Relaycast (previously computed but never
transmitted), so a hosted consumer can discard a duplicate itself.
Relaycast's HTTP API has no server-side idempotency key today (external
SDK, out of repo), so this remains documented at-least-once, not
exactly-once — filed as a companion issue, Relaycast agent-events HTTP endpoint has no server-side idempotency key, so hosted agent_exited delivery is at-least-once not exactly-once #1752.
5xx/timeout:
PendingremainsPendingthrough failure/retries, andonly confirmed success (in-process retry, or simulated restart replay)
reaches
Delivered.tests/relayflows/cases/1603-raw-spawn-readiness: itsactions-mocked MCP harness proved a different, already-fixed bug (raw
CLI spawn readiness) and never booted a real broker/Relaycast boundary,
so it could not stand in as proof of this fix. Replaced it with
tests/relayflows/cases/1603-hosted-exit-durable-delivery, which spawnsthe real compiled
agent-relay-brokerbinary against a fake localRelaycast HTTP server, seeds the exact on-disk
crash-insights.jsonformat with one
Pendinghosted-delivery record, and proves at the realbinary boundary that
basenever replays it (the durable outbox doesnot exist there) while
headreplays it on startup and delivers itthrough one transient 503 and a successful retry. The narrower,
exhaustive Rust regression tests remain authoritative for retry
exhaustion, dedupe-on-replay, and >256-record backlog draining; this
case only proves the real binary wires them together end to end.
crates/relay-pty/src/crash_insights.rsthat described atry_sendchannel handoff as the hosted-delivery acknowledgment (it isn't — only
confirmed Relaycast HTTP success does) and described the durable pending
outbox as bounded by
max_records(it isn't — only already-delivereddiagnostic history is).
getAgenttest helper(
tests/e2e/fleet/harness.ts) against the free-plan Relaycast429 rate_limit_exceededresponse that caused a prior "Two-node fleetmatrix" CI flake: it now retries that specific error with a small,
deterministic, bounded backoff (honoring
Retry-Afterwhen present)before giving up, while every other status or error code still throws
immediately exactly as before. New tests in
tests/e2e/fleet/get-agent-retry.test.tscover the retry, exhaustion,and non-retry paths.
Validation:
The harness-driver TypeScript check remains blocked by the host Node runtime missing libada.3.dylib.
RelayFlow Proof
bugfix1603-hosted-exit-durable-delivery