Skip to content

fix(slack,discord): drain mentions queued behind a continuation dispatch - #237

Merged
clsandoval merged 2 commits into
mainfrom
fix/drain-mentions-after-continuation
Sep 24, 2026
Merged

clsandoval merged 2 commits into
mainfrom
fix/drain-mentions-after-continuation

Conversation

@clsandoval

@clsandoval clsandoval commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

dispatch_continuations_in_thread runs a continuation turn from outside a mention turn. It is called after a private form submission, and again at release when #233 re-runs a skipped dispatch. It adds the thread to _processing for the whole dispatch. A mention that arrives in that window goes down the normal queue path: it is appended to _pending and gets the ⌛ reaction. But the dispatch's finally only called _release_thread, which never drains _pending. So the mention kept its ⌛ and got no reply until someone mentioned the bot in that thread again. On Slack it also never received the "Sorry, something went wrong" notice. #233 sends more dispatches down this path, which makes the bug more likely.

An independent audit of the formal-verification PRs reported this (#236 review, finding (a)). I confirmed it with the failing tests below before changing any code.

The fix sends the end of the dispatch through the same drain the mention turn uses. The drain logic is shared, not copied:

  • Slack: _orchestrate's drain loop moves unchanged into _drain_pending_mentions (one turn per author, per-author failure isolation), and its leftover apology moves into _notify_undrained_mentions. _orchestrate and dispatch_continuations_in_thread both call these two helpers. The dispatch drains before it releases the thread. If the dispatch raises, it apologises to anything still queued, the same way a failed mention turn does. dispatch_continuations_in_thread now takes team_id, which the drained turns need. The credential-form caller passes the workspace it already has, and the deferred re-run stores it with the other routing arguments.
  • Discord: the dispatch calls the existing _drain_pending_mentions before _release_thread, on success and when the dispatch raises (then it re-raises for the spawned task's error log). This follows Discord's own drain-always contract in on_message, where a mention queued behind a failed turn still gets its turn; _drain_pending_mentions never raises. Slack apologises instead on its raise path, as its _orchestrate does. On both adapters a queued mention now gets either a turn or a notice, never a silent ⌛.

A drained turn runs without a per-tenant in-flight slot. This matches queued mentions behind a mention turn, which never take a slot of their own, and continuation turns, which take none either.

Formal model note: the ThreadQueue model in #236 cannot express this today, because FormDispatch there is atomic and never takes processing. Catching it needs a spec change: split the dispatch into a claim step and a release step. A release that does not drain then violates NoStrandedMention. Per the audit's run, that happens at 53 states with WithForm = TRUE: FormRecord, FormDispatch, 3 × Arrive, FormRelease.

Tests (red on main, green with the fix):

  • slack/tests/test_continuation_dispatch_entry.py::test_mention_queued_during_a_continuation_dispatch_gets_its_own_turn: a real pending continuation is claimed by the real dispatch. During its continuation turn, a mention goes through the real _orchestrate, is queued, and gets ⌛. When the dispatch ends, that mention must get its own _run_thread_turn. Before the fix: AssertionError: the queued mention must get its own turn once the dispatch ends, got 0 turns. For this run only the team_id parameter was added, so the test could call the dispatch.

  • discord/tests/test_mention_queue.py::test_mention_queued_during_a_continuation_dispatch_gets_its_own_turn: the same interleaving, with the mention going through the real on_message. Before the fix: AssertionError: the queued mention must get its own turn once the dispatch ends, got []. The ⌛ assertion before it passes.

  • discord/tests/test_mention_queue.py::test_mention_queued_during_a_raising_continuation_dispatch_still_gets_its_turn: the dispatch raises after the mention queues. The mention must still get its own turn, and the error must still propagate. Before the second commit: AssertionError: the queued mention must get its own turn even when the dispatch raises, got [].

Existing Slack dispatch tests now pass team_id.

Checklist

  • Tests added or updated for any behavior change
  • uv run pytest passes locally (Slack adapter 677 passed; Discord adapter 1161 passed)
  • uv run pyright passes locally (strict mode): 0 errors
  • uv run ruff check . is clean
  • uv run lint-imports passes (package boundary contracts)

🤖 Generated with Claude Code

clsandoval and others added 2 commits September 24, 2026 13:07
dispatch_continuations_in_thread takes the thread's _processing slot while
it runs a continuation turn from outside a mention turn (a private form
submission, or a dispatch re-run at release). A mention arriving in that
window was queued with the hourglass reaction, but the dispatch's finally
only released the slot and never drained _pending, so the mention waited
for the next mention in the thread.

The dispatch now drains the queue through the same helper the mention turn
uses before it releases the thread. Slack's drain loop and its leftover
apology move into _drain_pending_mentions and _notify_undrained_mentions,
shared by _orchestrate and the dispatch; dispatch_continuations_in_thread
now takes team_id for the drained turns. Discord reuses its existing
_drain_pending_mentions.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
dispatch_continuations_in_thread drained mentions queued (hourglass) behind
the dispatch only when the dispatch returned. If dispatch_pending_continuations
raised, the finally released the thread without draining, so the mention kept
its hourglass with no reply or notice until a later mention in the thread;
Slack apologises on the same path.

The dispatch now drains on the raise path too, then re-raises for the
spawned task's error log. This is Discord's drain-always contract from
on_message (a mention queued behind a failed turn still gets its turn);
_drain_pending_mentions never raises. Cancellation still skips the drain.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@clsandoval
clsandoval force-pushed the fix/drain-mentions-after-continuation branch from 88ef23b to 418c4b3 Compare September 24, 2026 13:13
@clsandoval

Copy link
Copy Markdown
Collaborator Author

Audit round 4 follow-up (requested change 1: a Discord dispatch that raises strands the queued mention).

Audit item → change

  • Finding: on Discord, if dispatch_pending_continuations raised inside dispatch_continuations_in_thread, the finally released the thread without draining. The mention that queued during the dispatch kept its ⌛ with no reply or notice. Slack apologises on the same path.
  • Change (418c4b3, packages/adapters/discord/daimon/adapters/discord/bot.py): the dispatch now drains _pending when the dispatch raises too, then re-raises so the spawned task still logs the error. Cancellation (shutdown) still skips the drain.
  • Behaviour chosen: drain, not apologise. This is Discord's own drain-always contract. on_message drains a mention queued behind a failed turn (test_queued_followup_drains_even_when_originating_turn_fails), and _drain_pending_mentions never raises, so nothing is left over to apologise for. Slack keeps its apology, matching its _orchestrate. The invariant both adapters now share is that a queued mention gets either a turn or a notice, never a silent ⌛.

Fail before / pass after

  • New test: discord/tests/test_mention_queue.py::test_mention_queued_during_a_raising_continuation_dispatch_still_gets_its_turn. A mention arrives through the real on_message during the dispatch and queues with ⌛, then the dispatch raises. The test asserts that the mention gets its own turn, that the RuntimeError propagates, and that _pending and _processing end up empty.
  • Before the fix (head 88ef23b rebased onto main as a0c8aa8; bot.py is byte-identical between the two): AssertionError: the queued mention must get its own turn even when the dispatch raises, got [] at test_mention_queue.py:1024.
  • After the fix: it passes.

Local checks (rebased onto main with #238; no conflicts): discord 1161 passed, slack 677 passed, ruff check/format --check clean, uv run pyright 0 errors, lint-imports 8 contracts kept. The CHANGELOG entry now says the mention gets its reply even when resuming the task fails.

clsandoval added a commit that referenced this pull request Sep 24, 2026
- thread_queue: split the out-of-turn continuation dispatch into a claim step
  (the thread joins _processing) and a release step. FormDispatchNoDrain
  (main) strands a mention queued behind the dispatch; FormDispatchDrain
  (#237's shape) is clean. The #233 re-run now reclaims the thread after an
  await, as the spawned call does.
- oauth: GrantRetryThreeMirrors shows #230's retry losing the sign-in to three
  concurrent mirrors; GrantLocked is #239's shape and clean with three.
- adapter_recovery: WizardBypassRows is a regression row fixed by #238, and
  recovery adopts the thread's live row as #238's _replace_dead_session does.
  OverlapCASNoStale is documented as clean by construction.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
clsandoval added a commit that referenced this pull request Sep 24, 2026
…pter restarts (#236)

* docs(formal): model oauth callbacks, vault slot, thread queue and adapter restarts

Adds formal/oauth (McpOAuthFlow, VaultSlot, SlackInstall), formal/thread_queue
(ThreadQueue) and adapter_recovery/AdapterOverlap, each calibrated on past fixes
(7a5a74b, 41234e9, 2cbe69e, 39450e8, dc2d866, 997b3d8, 090ecc1, 287b719 ->
0bc1b14/2d3a002 -> c923083) before its new counterexamples were checked
against the code. Updates the coverage table.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* ci(formal): check the oauth, vault, install, thread-queue and overlap models

Adds the 37 configs of these models to formal/expected.tsv, each with its
verdict as documented and its pinned distinct-state count (60 rows in
about a minute), and the code they describe to the workflow's path filter.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix(formal): pin the blind-clear race separately from the cas-only one

OverlapPreCAS and OverlapCASOnly stopped on the same 8-step trace (a
marker written before the sweep's snapshot), so the unconditional clear
that 2d3a002 replaced was never pinned. Add NoStaleClear (the sweep never
clears a live marker written after its snapshot): OverlapPreCAS now
violates it on the 11-step trace 2d3a002 describes (229 states), the same
configuration with compare-and-clear is clean (OverlapCASNoStale, 561),
and OverlapCASOnly keeps the c923083 case under NoStolenClear (88).
Label OverlapGated as c923083 (Slack) / #232 (Discord).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* docs(formal): label bugs fixed on main as regression rows

#228, #230, #231, #232 and #233 are merged, so their pre-fix configs are
regression rows, not current findings. Rename SlackReinstallCurrent to
SlackReinstallPre231, move DiscordReactFirst out of the calibration table,
and update the spec comments that still said current or proposed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* ci(formal): run the models when their remaining source files change

Add slack_bot_tokens.py, tenants.py, sessions.py, ma.py, and the Slack
and Discord credential form handlers to the path filter.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix(formal): pin the bugs #237 and #239 fix and relabel #238's rows

- thread_queue: split the out-of-turn continuation dispatch into a claim step
  (the thread joins _processing) and a release step. FormDispatchNoDrain
  (main) strands a mention queued behind the dispatch; FormDispatchDrain
  (#237's shape) is clean. The #233 re-run now reclaims the thread after an
  await, as the spawned call does.
- oauth: GrantRetryThreeMirrors shows #230's retry losing the sign-in to three
  concurrent mirrors; GrantLocked is #239's shape and clean with three.
- adapter_recovery: WizardBypassRows is a regression row fixed by #238, and
  recovery adopts the thread's live row as #238's _replace_dead_session does.
  OverlapCASNoStale is documented as clean by construction.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* ci(formal): run the models when the slack boot order or deploy changes

Adds the Slack entry point (orphan recovery starts before connect), the
stored agent MCP token, and the deploy and compose files behind the
single-process assumption to the path filter.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@clsandoval
clsandoval merged commit 958c749 into main Sep 24, 2026
12 checks passed
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.

1 participant