Skip to content

fix(server): a held queue no longer strands later queued messages - #18792

Closed
leojkwan wants to merge 1 commit into
pingdotgg:mainfrom
leojkwan:fix/held-queue-starvation
Closed

leojkwan wants to merge 1 commit into
pingdotgg:mainfrom
leojkwan:fix/held-queue-starvation

Conversation

@leojkwan

@leojkwan leojkwan commented Oct 12, 2026 •

Copy link
Copy Markdown

After a restart or Stop holds the queue, a new message starts on the idle thread, and messages queued behind it inherit the older hold and never start. One user's messages waited 21 to 31 hours.

A queued message now joins a hold only when the held messages wait behind the same run, and the queue starts the first unheld message. Stop, failure and restart holds still wait for Resume.

Small fix for an obvious starvation bug, so no prior issue.

Test plan: the new runtimeLayer case failed before the fix; 14 hold-related test files (324 tests) and server typecheck pass on macOS arm64, Node 26.8.1; installed app untested.

Model: Claude Opus 5.5, Claude Code harness in T3 Code.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-12T21:47:26.820787Z ae75bf0 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 12, 2026
@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 12, 2026 — with ChatGPT Codex Connector
@macroscopeapp

macroscopeapp Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at ae75bf0

Macroscope's review found this PR approvable — This is a narrowly scoped server bug fix that prevents held queues from starving later eligible messages while preserving holds owned by the correct run. Production logic is limited to queue selection/readiness, and targeted runtime tests cover both affected ordering cases.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1801018f-16ed-4286-a1f8-2add80fd3837

📥 Commits

Reviewing files that changed from the base of the PR and between 511d20c and ae75bf0.


📒 Files selected for processing (3)
  • apps/server/src/orchestration-v2/Orchestrator.ts
  • apps/server/src/orchestration-v2/ProjectionStore.ts
  • apps/server/src/orchestration-v2/runtimeLayer.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.



📝 Walkthrough

Walkthrough

Queued-run selection and start eligibility now skip held runs, while new queued runs inherit a hold under narrower conditions. Runtime tests cover recovery and follow-up behavior.

Changes

Queued run hold behavior

Layer / File(s) Summary
Held-run selection and start eligibility
apps/server/src/orchestration-v2/Orchestrator.ts, apps/server/src/orchestration-v2/ProjectionStore.ts, apps/server/src/orchestration-v2/runtimeLayer.test.ts
The orchestrator selects the first unheld run in delivery order and applies hold inheritance based on missing worktrees or the ordinal of held runs. Both projection stores exclude held runs from startable work. Tests cover recovery, follow-up queueing, and held-run states.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge, t3dotgg


Merge Risk: ⚪ Minimal · up to ae75b

The change allows later unheld messages to proceed without releasing messages held for Resume. No merge-blocking issue remains after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to ae75b

The change allows later messages to proceed without releasing older held messages. Existing execution guards remain in place, and no new security issue was identified. Incomplete partial-failure and recovery validation leaves limited residual uncertainty.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed scheduling unit is a thread's queue. An actor already able to dispatch messages can cause later eligible work to execute while older work remains held. Execution still uses the selected run's provider selection and associated workspace scope; this inspection does not establish broader transport or tenant-isolation guarantees.

Trust Boundaries and Controls

  • observed — The newly eligible run still passes the selected provider's project-enable check and, when a provider context transfer is needed, the existing context-handoff capability check. The PR does not replace these checks with hold eligibility.

Resilience and Maintainability Implications

  • observed — Stop continues to hold queued work, and explicit Resume clears holds. Preparation retry releases holds only when the retried failure is the latest executed run, preserving the existing failure-ownership check.
  • observed — Command dispatch and terminal-event queue startup share per-thread locking. Receipt replay checks thread identity; repeated Resume rechecks current queue eligibility rather than recreating the command's runs. Queued-start failure handling uses the same unheld-run selector as normal startup.

Pre-merge checks · Passed 4
Passed Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main fix: preventing held queues from stranding later queued messages. It also follows the repository's conventional commit format.
Description check Passed The description covers the problem, the fix, scope rationale, focused verification results, environment details, and the untested installed app. It does not use the template's exact section headings, …

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@AdEx-Partners-DE

Copy link
Copy Markdown
Contributor

Adopted into our fork (Windows 11, Node 24), on top of a fork that has its own queue hold paths (an environment-wide queue pause, and restart holds that a "continue after update" setting can skip). The commit applied without conflicts.

  • The new runtimeLayer case passes. Across the 14 test files that touch queueHeld or canStartQueuedRun, 323 tests pass and 3 fail (OrchestratorMcpToolkit.integration.test.ts, ProjectionRecovery.test.ts, ThreadLaunchService.test.ts); those 3 fail on this machine without this commit too, so they are unrelated.
  • Server typecheck clean.
  • End-to-end on a dev server with real Codex turns: a message queued and held by a pause starts after release, a queued message survives a hard server kill and runs after it, and a message queued behind a long turn completes on its own after a kill mid-turn. All three pass with this commit.

We have not yet seen the starvation case itself on a real installation with this commit.

Claude posted this on behalf of @AdEx-Partners-DE; no human reviewed it.

@juliusmarminge

Copy link
Copy Markdown
Member

Note

Grok responding on behalf of Julius.

Superseded by #21473, which just merged: a message the user sends now releases a held queue, so later queued messages no longer inherit the old hold and get stranded. Closing; thanks @leojkwan.

@juliusmarminge juliusmarminge self-assigned this Oct 19, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one as maintainer. I will preserve your commits and credit, verify newer queued work can progress past older held messages without releasing those holds, and push the finished change here without merging.

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

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants