fix(discord): release the agent when clarify buttons expire - #72742
hyuseinleshov wants to merge 2 commits into
Conversation
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the separate Discord-view and agent-side deadlines; current main still has the reported expiry path (plugins/platforms/discord/adapter.py:9131-9145), while the gateway blocks in gateway/run.py:4717-4722.
Problems
plugins/platforms/discord/adapter.py:8987treats a still-registered entry as unanswered. A typed reply setsentry.event, butwait_for_responseremoves the entry only after that wait returns (tools/clarify_gateway.py:143-157). Becauseresolve_gateway_clarifyoverwritesentry.responsewithout checkingevent.is_set()(tools/clarify_gateway.py:164-176), the expiry task can replace a real reply with""during that window.- The proposed regression test manually removes the entry before draining the task (
tests/gateway/test_discord_clarify_buttons.py:729-735), so it does not exercise that production race.
Suggested changes
- Make
resolve_gateway_clarifysingle-winner under_lock(reject already-set entries), then rely on its return value in the expiry task. - Test an event-set entry that remains registered and verify the expiry task preserves its typed response. Document the new user-facing
approvals.discord_clarify_text_gracesetting alongside the Discord clarify timeout behavior.
Automated hermes-sweeper review.
| try: | ||
| if grace > 0: | ||
| await asyncio.sleep(grace) | ||
| if not _clarify_entry_pending(self.clarify_id): |
There was a problem hiding this comment.
A present entry is not necessarily unanswered: resolve_gateway_clarify() sets its event before wait_for_response() removes it. A typed reply can therefore remain in _entries here and then be overwritten by the empty expiry resolution. Make resolution single-winner atomically in tools/clarify_gateway.py (reject an already-set event) and use that result rather than entry presence.
teknium1
added
sweeper:risk-message-delivery
A native multi-choice clarify renders a Discord button view whose timeout
(`approvals.discord_prompt_timeout`, 300s by default) is independent of the
agent-side wait (`agent.clarify_timeout`, 3600s by default, unlimited at 0).
On expiry the view only greyed out the embed and disabled the buttons — the
clarify entry stayed armed, so the agent thread stayed parked in
`clarify_gateway.wait_for_response` for the remaining ~55 minutes.
The prompt is unanswerable in that window. The buttons are dead, and
`_coerce_text_response` rejects prose for an `awaiting_text=False` entry —
correct per the strict-prose behavior, but it means every follow-up message
queues behind the very turn it was meant to unblock. Observed on a Discord
gateway sitting at "Working — 54 min — iteration 7/150, clarify" under an
embed reading "Prompt expired — no action taken"; three user messages, one of
them "Cancel the current task", produced no `inbound message` log line at all.
`on_timeout` now:
* flips a still-pending entry into text-capture mode, so a typed reply
answers the prompt once the buttons are gone, and says so in the footer
instead of claiming that no action was taken;
* schedules a release task that resolves the entry with an empty response
after `approvals.discord_clarify_text_grace` seconds (300 by default, 0
to release as soon as the view expires), so an abandoned prompt can never
pin the session — including under `clarify_timeout: 0`;
* keeps the old "no action taken" footer when the entry is already gone.
Related: #71946 (Telegram native-choice clarify deadlock — same
`awaiting_text=False` coercion boundary, different trigger).
Mirrors the discord_prompt_timeout reader tests: defaults, numeric strings, malformed values, clamping, and a crashing read_raw_config. Adds one cross-default invariant — view timeout plus grace must fit inside the default agent.clarify_timeout, which is exactly what drifted apart and left sessions pinned behind an expired prompt.
60f9e5b to
e16e0b2
Compare
|
Rebased onto current The only conflict was in Verified all three imported symbols ( Tests, using per-file isolation as Full Happy to split the test commit out or adjust anything if that helps review. |
GottZ
left a comment
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Three PRs address or reference #71946: #71997 breaks the unmatched-prose redirect-to-steer deadlock, #75732 carries that fix forward with atomic first-writer resolution and retry-aware rejection classification, and #72742 addresses the distinct Discord button-expiry path that can leave the clarify waiter blocked.
Related pull requests
- #71997
best fix— (+14/-3) — superseded deadlock fix: releases a rejected native-choice clarify before ordinary busy-message routing, preserving strict prose rejection, but its unguarded empty resolution can overwrite a button result. Despite the keep_open review and recorded best-fix verdict on #71997, #75732 preserves this core change while adding first-writer-wins resolution and distinguishing retryable invalid selections from free prose. - #72742
related— (+401/-11) — distinct Discord expiry salvage path: disables expired buttons, enables a configurable typed-answer grace period, and eventually releases the waiter, covering a separate cause from the unmatched-prose deadlock. The keep_open review remains blocking because the diff tests entry presence rather than atomic unresolved state, allowing expiry to overwrite an answer before the waiter removes the entry. - #75732
best fix— (+378/-37) — strongest current unmatched-prose fix: releases only free-prose rejects before redirect-to-steer routing, retains retryable out-of-range and malformed selection attempts, and makes clarify resolution first-writer-wins under the lock. This directly addresses the keep_open review's selection-classification concern in commit 09129e4f5 and adds regressions for both retry and button-then-prose races.
Duplicates
#71997 and #75732 substantially implement the same unmatched-prose deadlock fix; #75732 is the successor and subsumes #71997. #72742 overlaps in clarify release and race handling but targets the distinct Discord button-expiry path.
Suggested consolidation
Keep #75732 open with its salvage path as the consolidated implementation for the unmatched-prose deadlock, and close #71997 as a duplicate of #75732; this closure is justified despite #71997's recorded best-fix verdict and keep_open review because #75732 carries its core fix forward while correcting the documented button-result overwrite and invalid-selection retry regressions. Keep #72742 open with a salvage path for the distinct Discord expiry behavior, but require it to use the atomic first-writer-wins result from #75732 instead of entry presence and to add the event-set-but-still-registered race test requested by its keep_open review.
Cross-PR triage: Reviewed 3 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 44 kB of PR diffs, 29 kB of issue/PR text, 9 kB of discussion (12 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
|
We reproduced the split-timeout failure on Discord with a production gateway: This PR is therefore directionally right, but the post-expiry 5-minute free-text grace still ends by abandoning the original waiter rather than giving a later-message recovery contract. A conflict-minimizing split that looks safe:
For the clarify path, the two regression cases that mattered in our reproduction were: invalid selection visibly re-presents the numbered choices; non-selection prose releases the parked waiter exactly once and continues as ordinary user text. A first-writer-wins guard is important because a late component tap can race the prose handoff. |
5 participants
What does this PR do?
A native multi-choice
clarifyon Discord is rendered byClarifyChoiceView, whose lifetime comes fromapprovals.discord_prompt_timeout(300 s by default). The agent thread that asked the question is parked inclarify_gateway.wait_for_response, governed by a completely separate deadline —agent.clarify_timeout(3600 s by default, unlimited when set to0).When the view timed out,
on_timeoutdisabled the buttons and repainted the embed with "⏱ Prompt expired — no action taken" — and that was all. The clarify entry stayed armed, so the agent stayed blocked for the remaining ~55 minutes.The prompt is genuinely unanswerable during that window:
_coerce_text_responserejects prose for anawaiting_text=Falseentry, so typing an answer does nothing (correct in itself — strict multi-choice coercion is deliberate);busy_input_mode: interruptcallsAIAgent.interrupt(). That flag is only observed at loop checkpoints, and the thread is asleep insideEvent.wait. So the message is queued behind the very turn it was meant to unblock.Net effect: the session is pinned, the UI claims the prompt is dead, and every follow-up message vanishes silently — no
inbound messagelog line is ever emitted for it.Evidence from a live gateway
Discord showed
⏳ Working — 54 min — iteration 7/150, clarifyunder an embed reading "Prompt expired — no action taken". Three user messages (including "Cancel the current task") were accepted by the adapter but never reached the agent:kill -USR2on the gateway confirmed where the turn was stuck:Related Issue
No existing issue covers the Discord view-expiry path — I searched open and closed issues and PRs for
clarify timeout,clarify prompt expired,discord clarify buttonandprompt expired no action takenbefore writing this.Related but distinct: #71946 (Telegram native-choice clarify deadlock). Same
awaiting_text=Falsecoercion boundary, different trigger — that one is the redirect/steer path during tool execution, this one is UI expiry racing an independent agent-side deadline. This PR does not fix #71946.Related history: #45903 made the view timeout configurable, which is what allowed the two deadlines to drift apart in the first place.
Type of Change
Changes Made
plugins/platforms/discord/adapter.pyClarifyChoiceView.on_timeoutnow flips a still-pending entry into text-capture mode viamark_awaiting_text(), so a typed reply answers the prompt once the buttons are gone. The footer says so — "⏱ Buttons expired — reply with a message to answer" — instead of claiming no action was taken._resolve_after_grace(), which resolves the entry with an empty response afterapprovals.discord_clarify_text_graceseconds (default 300,0= release immediately). An abandoned prompt can no longer pin a session, including underclarify_timeout: 0, where the old code hung forever._read_clarify_text_grace()reader (same shape as_read_discord_prompt_timeout, clamped to[0, 3600]) and_clarify_entry_pending()helper. Expiry tasks are held in_CLARIFY_EXPIRY_TASKS— asyncio keeps only a weak reference to a running task, so without a strong ref the GC can collect one mid-sleep and the agent never gets released.tests/gateway/test_discord_clarify_buttons.py— newTestClarifyChoiceViewTimeout(6 tests): entry flipped to text-capture; prose answers the prompt after expiry (the regression); agent released when grace is disabled; an answer during the grace window is not overwritten; expiry with no entry stays a plain no-op; expiry with no stored message reference doesn't raise.tests/gateway/test_discord_prompt_timeout_config.py— 11 tests for the new reader, mirroring the existing ones, plus a cross-default invariant: view timeout + grace must fit inside the defaultagent.clarify_timeout. That invariant is precisely what had drifted.How to Test
Reproduce (before this PR)
approvals.discord_prompt_timeout: 30inconfig.yamlto make the window short.clarifywith native choices.cancel that). Nothing happens — no reply, noinbound messageline in~/.hermes/logs/gateway.log, and the session stays busy untilagent.clarify_timeoutelapses.After this PR
discord_clarify_text_graceseconds later, withDiscord clarify expired unanswered (id=…, grace=…s, ok=True)in the log.Automated
scripts/run_tests.sh tests/gateway/test_discord_clarify_buttons.py tests/gateway/test_discord_prompt_timeout_config.py -q # 38 passedThe 6 behavioural tests fail on
main(assert entry.awaiting_text is True→False;resolve_text_response_for_session(...) is True→False) and pass with the fix.Wider sweep —
scripts/run_tests.sh tests/gateway/ tests/tools/ -q→ 901 files, 20056 passed, 3 failed. All three failures reproduce with the three touched files reverted toorigin/maincontent, so none of them come from this PR:tests/gateway/test_agent_cache.py::TestExtractCacheBustingConfig::test_honcho_cache_busting_config_memoized_by_mtimetests/tools/test_managed_browserbase_and_modal.py::test_browser_use_explicit_local_mode_stays_local_even_when_managed_gateway_is_readytests/tools/test_managed_browserbase_and_modal.py::test_browser_use_availability_skips_refresh_for_expired_cached_gateway_tokenChecklist
Code
fix(scope):,feat(scope):, etc.)tests/gateway/andtests/tools/are green apart from the pre-existingtest_agent_cachefailure noted aboveDocumentation & Housekeeping
docs/, docstrings) — the new reader and both timeout interactions are documented in docstrings; there is no prose doc coveringapprovals.*todaycli-config.yaml.examplehas noapprovals:section (its siblingdiscord_prompt_timeoutisn't listed there either), so adding one key alone would be inconsistent; happy to add the whole block in a follow-up if you'd like it documentedasyncioonly, no platform-specific calls; the touched path is adapter-level and identical on Windows, macOS and Linux