fix(broker): keep respawned agents and held initial tasks receiving - #1367
Merged
Conversation
An agent released and respawned under the same name stopped receiving relay messages. Release drops the agent's delivery cursor, and the engine reuses the agent record without reporting a cumulative position, so the next message arrived past the start of the sequence, was classified as a gap, and was acknowledged without being surfaced — discarding it and stopping the engine from retrying. An identity confirmed by `agent.register` now adopts the observed sequence as its cursor instead of treating a missing position as a gap. A provisional binding keeps the previous response so an unconfirmed second identity cannot claim a live name mid-sequence. A PTY worker that becomes ready while its inbound delivery is held also now releases the initial task from its spawn through the hold, instead of leaving it parked in the injection queue until the hold lifts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe broker now preserves delivery continuity for authoritative agent respawns and distinguishes seq:0 fan-out from established sequence positions. PTY workers support event-targeted injection flushing, allowing initial tasks to start while interactive holds remain active. ChangesBroker delivery and worker startup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BrokerRuntime
participant PTYWorker
participant InjectionQueue
BrokerRuntime->>PTYWorker: worker_ready with replayed manual hold
BrokerRuntime->>PTYWorker: flush_injections(event_id)
PTYWorker->>InjectionQueue: find matching queued delivery
InjectionQueue-->>PTYWorker: start initial task
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efb9309ac4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Both sides added entries under `### Fixed` in CHANGELOG.md; keep main's telemetry/identity fixes alongside this branch's respawn-delivery and held initial-task fixes.
Two P1s from review on the respawn fix.
Restart adoption survived only while the respawned identity had no
cursor at all. A `seq:0` fan-out frame — a reaction, a read receipt, an
`action.completed` result — creates a cursor without establishing any
position, so one landing before the agent's next real message put the
cursor at zero and the resumed frame (seq 43) read as a gap again:
acked, never surfaced, agent deaf. Adoption is now keyed on an explicit
`has_sequenced_position`, set by a seeded cursor or the first sequenced
delivery, instead of on the cursor's existence.
The `flush_injections` that releases a (re)spawn's initial task through
a replayed interactive hold granted an allowance for the worker's whole
queue. A supervised restart retains unacknowledged deliveries, so a
relay message retried into the new worker before `worker_ready` could
spend that allowance and splice into the human's session. The frame now
carries the initial task's `event_id` and the worker releases only that
delivery — popping it past anything queued in front, which stays parked.
A blanket flush (`POST /api/spawned/{name}/flush`) is unchanged.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/broker/src/pty_worker.rs`:
- Around line 968-982: In the blanket flush branch around active_injection,
preserve an existing targeted exemption instead of unconditionally clearing
inj.targeted_hold_exemption. Only mark the injection as blanket-exempt when it
is not already targeted-exempt, so restore_hold_exemption can reinsert the
delivery event_id into hold_exempt_event_ids if its write is requeued.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5eca119e-bb7d-4acd-8036-c8bf39e4ce49
📒 Files selected for processing (6)
CHANGELOG.mdcrates/broker/src/node_control.rscrates/broker/src/protocol.rscrates/broker/src/pty_worker.rscrates/broker/src/runtime/worker_events.rspackages/harness-driver/src/protocol.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- crates/broker/src/runtime/worker_events.rs
… flush A blanket `flush_injections` arriving while the initial task was in flight downgraded its targeted exemption. If that injection's write then failed, `restore_hold_exemption` returned the credit to the shared counter instead of to the delivery's own event id, so it could be spent on whatever else was queued — the failure mode the targeted exemption exists to prevent. The blanket branch now only grants `hold_exempt` and leaves an existing targeted exemption alone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Behavior
An agent released and respawned under the same name receives relay messages. A PTY worker that becomes ready while its inbound delivery is held runs the initial task from its spawn.
Respawn delivery
Release drops the agent's delivery cursor. The engine reuses the agent record across a respawn and does not report a cumulative position (it does not negotiate
relay:delivery-cursor-v1), so nothing re-seeds the cursor while the engine's per-agent sequence keeps counting. The next message therefore arrived past the start of the sequence,FleetDeliveryBook::observeclassified it as a gap, andplan_fleet_deliverymaps a gap toAcknowledge— acked without being surfaced. The message was discarded and the engine never retried, so the agent stayed deaf for the rest of its life. Only theseq == 1case was accepted without a cursor, which is why first spawns worked and every respawn did not.An identity confirmed by
agent.registernow adopts the observed sequence as its starting position, andcommit_receivedseeds the cursor to match so it advances from there. A provisional binding keeps the previous gap response: an unconfirmed second identity still cannot claim a live name mid-sequence.Adoption is keyed on an explicit
AgentDeliveryCursor::has_sequenced_position, not on whether a cursor exists.seq:0fan-out (reactions, read receipts, action results) creates a cursor without establishing any position, so keying on existence would let one fan-out frame landing before the respawned agent's next real message pin the cursor at zero — and the resumed frame would read as a gap again. The flag is set byseed_cursoror by the first sequenced delivery; fan-out never sets it, and stays deduped bymsg_idin the meantime.Held initial task
The initial task bypasses the delivery-mode pending queue but still lands in the PTY worker's injection queue, where a replayed interactive hold freezes pops. A worker that restarts while explicitly held reaches
worker_readyin that state, so its spawn's task sat in the queue until the hold lifted. The task is now released through the hold with aflush_injectionsexemption scoped to that one delivery; later relay messages keep parking as before.The exemption has to be task-specific rather than a blanket flush. A supervised restart retains unacknowledged deliveries, so a relay message retried into the new worker before
worker_readywould otherwise spend the same allowance and splice into the human's session.flush_injectionsnow carries an optionalevent_id: with it set the worker records only that id andnext_injection_indexreaches past anything queued in front of it, which stays parked (FIFO order among the parked entries is preserved once the hold lifts). A requeue after a failed write returns a targeted exemption to the event-id set rather than the counter, a hold boundary clears both, andPOST /api/spawned/{name}/flushsends noevent_idand keeps the blanket behavior unchanged.Verification
cargo test -p agent-relay-broker --lib— 843 passedcargo clippy -- -D warnings— clean;cargo fmt --all --check— cleannpm run typecheck,npm run lint(0 errors),npm run format:check— cleanNote:
runtime::tests::delivery_retry_transient_blip_emits_failed_event_for_present_workeris timing-sensitive under a loaded machine and flaked once during this work. It passes onmainand on this branch; it is unrelated to this change.Review in cubic