Skip to content

fix(gateway): keep timeout messaging accurate when an approval send is in flight#805

Merged
marcusrbrown merged 5 commits into
mainfrom
fix/approval-visible-output-race
Jun 7, 2026
Merged

fix(gateway): keep timeout messaging accurate when an approval send is in flight#805
marcusrbrown merged 5 commits into
mainfrom
fix/approval-visible-output-race

Conversation

@marcusrbrown

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to the v0.55.3 timeout-messaging fix. When a Discord mention run times out while an approval or status message is still in flight to Discord, the timeout copy could fall back to the "no output" wording even though the approval marker lands moments later — and in the worst case the user could see both the "no output" line and the "posted updates above" line together.

What changed

  • The Discord stream sink now tracks visibility that has been initiated but not yet delivered. An approval/status send marks visibility pending synchronously before it fires, then settles as delivered on success or retracts on failure.
  • Timeout classification treats an in-flight send as visible context, so the message no longer claims "no output" when an approval prompt is on its way.
  • A failed send retracts its claim, so the "no output" wording is still correct when nothing was actually delivered.
  • The empty-output fallback is suppressed while a send is pending, so a timeout in that window can't post a contradictory "no output" + "updates above" pair.

A timeout is still treated as a failure — this only makes the messaging accurate.

Tests

  • Pending send unresolved at timeout → visible-output copy, and no "no output" message posted.
  • Send rejects → no-output copy (a failed send never claims output).
  • Send resolves before timeout → visible-output copy (unchanged).
  • No approval + empty output + timeout → no-output copy (unchanged).
  • Sink-level coverage for the pending counter: pending-as-visible, settle-true persistence, settle-false retraction, concurrent handles, one-shot settle (no negative count), and empty-buffer flush skipping the fallback while pending.

@fro-bot fro-bot left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Verdict: PASS

The fix is well-reasoned and correct. The pending-visibility counter pattern solves the race cleanly: mark in-flight synchronously, settle on resolution, retract on failure. The one-shot guard on the settle handle prevents double-decrement. The classification ordering (flush first, then read hasVisibleOutput()) is preserved correctly.

Blocking issues

None.

Non-blocking concerns

  1. onDeadlineSettled marks visibleOutputSent only when the send succeeds. In run.ts:402-407, onDeadlineSettled calls safeSend then sink?.markVisibleOutputSent(). If safeSend rejects, markVisibleOutputSent is never reached — which is the correct behavior (the comment "markVisibleOutputSent AFTER the send succeeds" is accurate). No action required; just noting that the comment is the spec.

  2. Two separate pending claims per onPending event. The waiting-status send and the embed send each call markVisibleOutputPending() independently (correct — two sends, two claims). The one-shot settle guard keeps this safe, but a future third fire-and-forget send in onPending without a matching pending claim would undercount. Low risk given the narrow call site.

  3. sink captured as nullable in onDeadlineSettled closure (sink?.markVisibleOutputSent()). By the time onDeadlineSettled fires, sink is guaranteed non-null. The optional chain is defensive but harmless.

Missing tests

None. The test matrix is comprehensive: pending-unresolved, send-rejects, send-resolves, no-approval baseline, flush suppression while pending, concurrent handles, one-shot settle, double-settle first-wins, and the onDeadlineSettled path. All 846 tests pass.

Risk assessment (LOW): Change is narrowly scoped to one counter and two call sites in onPending. The one-shot settle guard protects counter arithmetic. The only observable behavioral change is in the timeout-messaging path during the specific race window. No new async coordination surfaces added; existing releaseLock/concurrency finally-block invariants are untouched.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 27081751827
Cache hit
Session ses_15fd38f0fffe9AXBlnKxIQTmVg

@marcusrbrown marcusrbrown merged commit 1755c85 into main Jun 7, 2026
12 checks passed
@marcusrbrown marcusrbrown deleted the fix/approval-visible-output-race branch June 7, 2026 03:47
@fro-bot fro-bot mentioned this pull request Jun 7, 2026
49 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants