Skip to content

fix(acp): notify the channel when a panicked batch is dead-lettered - #5709

Open
EmminiX wants to merge 3 commits into
block:mainfrom
EmminiX:fix/acp-panic-dead-letter-notice
Open

fix(acp): notify the channel when a panicked batch is dead-lettered#5709
EmminiX wants to merge 3 commits into
block:mainfrom
EmminiX:fix/acp-panic-dead-letter-notice

Conversation

@EmminiX

@EmminiX EmminiX commented Aug 12, 2026

Copy link
Copy Markdown

Summary

A panicking agent whose batch has already burned its retry budget now tells the channel, the same
way the non-panic path does. Before this the batch was discarded with only a log line, so the
channel went silent with no explanation.

EventQueue::requeue already returns the dead-lettered batch and recover_panicked_agent was
throwing that return value away. This threads the existing RestClient through
recover_panicked_agent and drain_ready_join_results and reuses spawn_failure_notice, so there
is no new mechanism and no new dependency. The notice text follows the wording of the three
existing ones.

Closes #5708

Duplicate search

No open PR touches recover_panicked_agent or spawn_failure_notice, and none matches panic
together with dead-letter.

This is deliberately not another dead-session recovery fix. #5598 and others already work that
lane and I did not want to add noise to it. This is only the missing notification on the panic
path, which is a separate and much smaller hole.

Testing

New unit test panic_dead_letter_posts_a_failure_notice:

  • spawns a task that genuinely panics and asserts join_error.is_panic(), so it exercises the real
    panic ingress rather than a cancellation
  • exhausts the retry budget first, so requeue dead-letters
  • stands a TcpListener in for the relay, then reads the request and asserts it is
    POST /events, that it carries the dead-lettered channel id, and that it contains the reason
    text, before returning a 200

Verification:

  • confirmed the test fails with the production change reverted, so it is a real gate
  • ran it 25 times consecutively with no failures, to check it is not flaky
  • cargo test -p buzz-acp: 766 passed, 0 failed (plus 9 in pool_lifecycle_state)
  • cargo clippy -p buzz-acp --all-targets -- -D warnings: clean
  • cargo fmt --check: clean

Rebased on current main and re-verified after #5682 landed in this file.

Logging

The dead-letter case now logs at WARN with channel_id and the event count, matching the detail
level of the requeue case beside it.

@EmminiX
EmminiX requested a review from a team as a code owner August 12, 2026 22:15
@Chessing234

Copy link
Copy Markdown
Contributor

the fix is right — let _ = queue.requeue(batch) was throwing away the dead-letter signal, and the test covers it.

the notice string is worth not duplicating though. lib.rs:3824 already has this exact sentence as a template with a {reason} hole: "⚠️ I couldn't process the last request after multiple retries ({reason}). Please re-send if it's still needed." — the new one is that string with reason = "the agent crashed", written out longhand. there are already four sibling notices around 3772-3824 and they will drift the first time anyone reworks the wording. passing "the agent crashed" through the existing template keeps them in step.

@Chessing234 Chessing234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked the scope question: after this, lib.rs:3788 and this new branch are the only two production requeue() call sites — every other hit in the tree is a queue.rs test — so it closes the last path where a dead-lettered batch vanished without telling the channel.

wording lines up too: the existing template at :3823 is "⚠️ I couldn't process the last request after multiple retries ({reason}). Please re-send if it's still needed.", and yours is that with reason = "the agent crashed". spawn_failure_notice already took Option<&RestClient>, so nothing widened to make this fit.

@ravarora2 ravarora2 added the triage-ready Appropriate for agentic review label Aug 14, 2026
EmminiX and others added 2 commits August 14, 2026 15:57
When an agent task panics, recover_panicked_agent requeued the batch and
discarded the result. If that requeue exhausted the retry budget the batch
was dropped with only a log line, so the channel went silent with no
explanation. The non-panic path already posts a notice via
spawn_failure_notice; this makes the panic path do the same.

Threads the existing RestClient through recover_panicked_agent and
drain_ready_join_results, and captures the dead-lettered batch that
EventQueue::requeue already returns.

Adds a test that fails without the change: it burns the retry budget, panics
a task holding a recoverable batch, and asserts a request reaches a local
listener standing in for the relay.

Signed-off-by: Emanuel Covasa <e.covasa@gmail.com>
Exercise the real join-drain handoff, parse and verify the signed channel event, and pin retry and removed-channel no-notice behavior.

Co-authored-by: Ravneet Arora <rarora@squareup.com>
Signed-off-by: Ravneet Arora <rarora@squareup.com>
@ravarora2
ravarora2 force-pushed the fix/acp-panic-dead-letter-notice branch from c9d8b37 to 876dab7 Compare August 14, 2026 19:58
@ravarora2

Copy link
Copy Markdown
Contributor

🤖 Follow-up implementation completed and pushed.

I rebased the PR onto current main and added commit 876dab78df40b1e23dbf12d5dbb9c371291914a4 to close the review’s test gaps without changing production behavior.

The regression coverage now:

  • enters through the real drain_ready_join_results panic handoff rather than calling recover_panicked_agent directly;
  • reads the complete POST /events body and deserializes it as a Nostr event;
  • verifies the event signature, signing pubkey, exact kind 9, exact generic failure text, one h channel tag, and the expected NIP-10 root/reply tags;
  • asserts exactly one submission;
  • proves retryable panics and removed channels do not post terminal notices.

Mutation evidence:

  • changing the production drain handoff from the REST client to None fails on the missing notice;
  • changing the failure event from kind 9 to kind 1 fails on the exact-kind assertion;
  • both mutations were restored, and the focused panic matrix passed 10 consecutive runs.

Verification at exact head 876dab78df40b1e23dbf12d5dbb9c371291914a4:

  • cargo test -p buzz-acp: 779 library tests plus 9 integration tests passed;
  • cargo clippy -p buzz-acp --all-targets -- -D warnings: passed;
  • cargo fmt --check: passed;
  • git diff --check and clean worktree: passed.

The existing best-effort tokio::spawn notice can still be lost during immediate runtime shutdown. That behavior predates this PR and is shared by the other failure-notice paths, so I am leaving it as a non-blocking reliability follow-up rather than widening this fix.

Signed-off-by: Emanuel Covasa <e.covasa@gmail.com>
@EmminiX

EmminiX commented Aug 15, 2026

Copy link
Copy Markdown
Author

Good catch on the duplication, thank you. Fixed in d7e16053, rebased onto 876dab78.

Added a retry_exhausted_notice(reason) helper next to spawn_failure_notice and routed both matching call sites through it: the pre-existing one in handle_prompt_result and the one this PR added in recover_panicked_agent. The sentence now exists once. A test pins the helper's output against the original literal so the user-facing copy provably did not shift.

One I deliberately left, so you do not have to ask: the hard-timeout notice still carries the same sentence with its reason inlined as (the turn exceeded the maximum duration ({}s)). Routing it through the helper would emit identical bytes, so it is a legitimate candidate. I left it because this PR never touched that line and widening a review response into untouched code felt like the wrong call on your repo. Happy to fold it in if you would rather it went in one go.

The other two siblings are genuinely different sentences (one has no "after multiple retries" clause, the auth-failure one is a different remedy), so they stay as they are.

cargo fmt --check, clippy -D warnings and the full error_outcome_emission_tests module are clean: 30 passed, 0 failed, including the three panic-path tests from 876dab78.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

triage-ready Appropriate for agentic review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

buzz-acp: a panicked agent's dead-lettered batch is discarded with no notice to the channel

3 participants