🤖 fix: queue a send behind an earlier send still in preflight - #4053
Conversation
WorkspaceService.sendMessage decided queue-vs-direct from session.isBusy() alone, which only turns true once AgentSession claims PREPARING. Two manual sends inside that window both went direct; StreamManager.ensureStreamSafety then aborted the first live stream (abortReason "system") to start the second, committing an empty assistant row for the first prompt. Track session-invisible preflights as arrival-ordered tickets and queue a later send while an earlier ticket is live. A disposer drains entries queued behind a preflight that settled without ever becoming a turn (refused, rejected, or startup failed); AgentSession.drainQueuedMessagesIfIdle keeps the auto-retry and edit flows' claim on the next dispatch.
This comment has been minimized.
This comment has been minimized.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
![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: d1bffd88b2
ℹ️ 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".
- Drain when the settling preflight was the head of the arrival line, not only when no preflight is live, so an entry queued behind a failed oldest send dispatches before a younger in-preflight send decides. - Mark requireIdle maintenance sends as supersedable tickets: manual input never queues behind them, and they still yield through preflightSendCounts. - drainQueuedMessagesIfIdle defers to hasActiveOrPendingTurnWork so a mid-stream compaction about to dispatch its request keeps the queue.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 126700ef5a
ℹ️ 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".
- The drain decision ignores supersedable tickets, like the queue decision does: a failed manual send drains the entries queued behind it even when a maintenance preflight is physically older. - Queue-mode heartbeats (yieldToQueuedMessages) are supersedable too, carry the live preflight probe, and yield quietly when manual input is in preflight instead of racing it to PREPARING. - drainQueuedMessagesIfIdle no longer defers to a scheduled auto-retry: it is the only drain the queued input gets, and the retry defers to the busy session until stream success cancels it.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b7a29d81f
ℹ️ 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".
- Supersedable tickets never initiate the drain: nothing queues behind them, and a heartbeat settling first must not dispatch entries ahead of a live manual send. - Sends reach their queue-or-direct decision in arrival order: a later send whose preflight awaits finish first waits for earlier blocking tickets to decide, so the queue keeps prompt order. - interruptForCompaction drains on exit when its request never became a turn.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This comment has been minimized.
This comment has been minimized.
## Summary Version bump for the v0.28.4 patch release. The headline change since v0.28.3 is Gemini 3.8 Flash becoming the default Gemini Flash model (coder#4060). The release also carries browser Login with Coder on remote Xum servers (coder#4047), the opt-in project bundle for settings backup (coder#4043), the connection-indicator slow-response surfacing (coder#4059), send-queue and terminal-wake fixes (coder#4053, coder#4052), and the Effect Phase 11 runtime refactors. ## Implementation Bumped with `node ./scripts/set-package-version.js 0.28.4` so the root `package.json` and the legacy `packages/mux-compat` forwarding package stay version-locked (the v0.28.3 bump missed the compat package and broke `Test / Unit` on main, fixed in coder#4048). `src/common/compat/productIdentity.test.ts` passes locally. After this PR merges, the `v0.28.4` tag will be applied to the squash commit and the GitHub Release published to trigger the desktop/npm/docker pipelines. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `medium` • Cost: `$0.00`_ <!-- mux-attribution: model=anthropic:claude-fable-5-1 thinking=medium costs=0.00 -->
1 participant
Summary
Two prompts sent seconds apart could both start a turn: the second one's stream startup aborted the first one's live stream (
abortReason: "system"), committing an empty assistant row for the first prompt.WorkspaceService.sendMessagenow queues a send that arrives while an earlier send is still in its pre-admission window, and drains anything queued behind a send that never became a turn.Background
shouldQueuewas decided fromsession.isBusy()alone, which only turns true onceAgentSession.sendMessageclaimsPREPARING. Everything before that (pricing gate, settings persistence, history appends, goal sync) runs against an idle-looking session. Under load that window was tens of seconds. Observed in a dogfood workspace:continueprompts at 21:53:17 and 21:53:28 both went direct, two[stream-startup]breadcrumb sequences interleaved inmux.log, and when the second reachedStreamManager.startStream,ensureStreamSafetykilled the first stream 44s in with no output (turn.interrupted reason=system,retry.abandoned reason=aborted).The existing
sessionInvisiblePreflightCountsalready modeled exactly this window for the session's idle probes (goal continuations, heartbeats), but manual sends did not consult it.Implementation
workspaceService.ts: the session-invisible preflight counter becomes a set of arrival-ordered tickets. A later send queues while an earlier ticket is still live (hasEarlierPreflight()), so two simultaneous arrivals cannot each defer to the other and the first-arrived send wins.requireIdlecallers keep their existing skip semantics; edits still bypass the queue by design.armQueuedBehindPreflightDrain(used bysendMessageandresumeStream) drains entries queued behind a preflight that settled without a turn (refused, rejected, startup failed). Without it those entries would sit in the queue with no stream end to drain them. It only fires from the last live preflight.agentSession.ts:drainQueuedMessagesIfIdle()yields to a busy session, a pending auto-retry, or the edit flow's claim on the next dispatch, matching the existing stream-end drain contracts.Validation
workspaceService.test.tsfail with the production change reverted and pass with it. The first test issues two sends back to back (both inside preflight awaits) and assertsfirstis dispatched andsecondis queued, which also rejects a naive "count > 1" implementation that inverts order. The second asserts the drain runs only after the earlier send settles with a failure, never while it is still in preflight.Risks
Low to moderate, scoped to send admission. Behavior change: a message arriving during another send's preflight is now queued (visible in the composer queue) instead of racing it. A manual send arriving during a heartbeat's short preflight will wait behind the heartbeat turn rather than kill it; making
requireIdlesends yield to a message queued behind them is a possible follow-up. The drain is guarded byisBusy,hasPendingAutoRetry, and the edit-flow deferral, so it cannot dispatch during a retry backoff or while an edit is truncating.Generated with
xum• Model:anthropic:claude-fable-5-1• Thinking:xhigh• Cost:$17.79