Repository navigation
🤖 tests: cover maybeStartQueuedTasksFromReservations paths before the split - #7744
Conversation
… split Adds characterization tests for the queue drain paths that no suite reached, before that method is split into steps. The tests drive the public maybeStartQueuedTasks and assert outcomes only: which plans the drain handed to the launch, which rows it marked failed (interrupted with a taskLaunchError), which it left queued, and slot use. Paths covered: the slots-full break, a queued task that is already streaming, a missing parentWorkspaceId, a parent without a workspace name, a failed parent metadata read (fallback metadata, both projectName sources), a failed skip_init_hook read (with a positive control), a missing task workspace name, and a workflow-owned task whose run ends between the prepass and the per-task loop. - Line coverage of the method goes from 283/316 to 317/317. - Each test failed under a scratch edit of the branch it covers. - Setup installs call-through tracking of drains, launches, launch failure marks and background inits for every test, and teardown settles it before removing the root. No production change.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
🟢 e2e stories (Haiku): 14 passed22 agent steps · 7 replayed from cache · 15 model calls · 46.3k tokens (28% cached) All 14 tests in 9 files
e2e 0.20.0 · 1m 14s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 7.9k tokens (33% cached) All 3 tests in 3 files
e2e 0.20.0 · 18.8s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 5.3s · 2 targets (web, phone) · run artifacts |
Adds queue drain tests for branches whose hand mutants survived the queue-drain suites during the split of maybeStartQueuedTasksFromReservations. They pass on the unsplit method and on the split. Each one fails under a scratch edit of the branch it covers: - Queued tasks are reserved oldest first when the config lists them newest first (the createdAt sort). - A multi-project task with a secondary project the config does not list is marked failed, and the next task still launches. - A failed parent metadata read whose projects list names only other projects falls back to the project directory's name. - Once shutdown began, the drain stops at the first launch CAS: no lease hold for the next task, and both tasks stay queued. - A reservation this process owns keeps its attempt id and its cancellation across the queue. A task without one gets a new id. - When the drain hands the plan over, the row is `starting` with the plan's attempt id and the brief's send id. No production change. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=xhigh -->
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
No test checked that the queue drain releases the launch lease of a task it does not launch. A leaked lease makes removal of that task refuse until the process exits. - The shutdown test now checks that no launch lease stays held. - New: a launch CAS lost to another writer releases the lease and plans nothing. - New: a config write that throws out of the loop after an earlier task's plan leaves no lease held (the CAS-throw release and the `_unlaunchedLeases` disposer). - The describe and header no longer say "before the split", and the streaming test names #7743 (the double slot). Each new assertion fails when the release it covers is removed from taskService.ts (scratch edit, reverted).
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: 7f8e7c95d3
ℹ️ 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".
Addresses Codex review on #7744: the unowned case now asserts plan.abortSignal is undefined, so a plan that carries any other signal fails.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…ch CAS, bookkeeping and plans (coder#7745) ## Summary Part 1 of 3 of the `maybeStartQueuedTasksFromReservations` split (stack 9, tail first). It moves the tail of the per-task body into steps: the launch lease hold, the queued → starting CAS, the bookkeeping after it, the plan build and the launch of the plans after the mutex is released. The method's CC goes from 76 to 56. Changed lines: 317. No behaviour change: same awaits, same calls to other methods in the same order (checked with a script that inlines every step), same `markTaskLaunchFailed` messages. Each new step has CC 8 or less, with 100 % line coverage. The method keeps the `_unlaunchedLeases` disposer, the mutex block and the per-task loop. No step takes the mutex or starts a drain, and every step call inside the mutex block is awaited. ## Implementation - `leaseQueuedTaskLaunch` holds the launch lease before the CAS. On a refusal it still calls `armQueuedLaunchLeaseRetry()` before it skips the task. - `claimQueuedTaskLaunch` runs the CAS. The updater keeps the `isShuttingDown()` check, and `moveQueuedRowToStarting` picks the attempt from the fresh row and writes `taskPromptSendId` in the same updater call. The step reads `launch` and `shuttingDown` only after the edit, as before, because `editConfig` can run the updater twice. - Every exit without a plan releases the lease. The shutdown exit still breaks the loop, and the plans collected before it still launch. - `recordQueuedLaunch` pushes into the same `plans` array that the disposer reads. ## Risks Queued task launches. A wrong lease release would leave a task's lease held until the process exits. The tests in coder#7744 cover the shutdown, lost-CAS and lease-retry exits. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$313.07`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=313.07 -->
…n preamble (coder#7747) ## Summary Part 3 of 3. It moves the drain's preamble under the mutex into `openQueuedDrain` (config, settings, queued list and the inactive-workflow prepass) and `countQueuedDrainSlots` (the stale-turn sweep bookkeeping and the capacity gates). `maybeStartQueuedTasksFromReservations` ends at CC 8 (from 76 before the stack). Changed lines: 172. No behaviour change: same awaits, same calls to other methods in the same order (checked with a script that inlines every step), same `markTaskLaunchFailed` messages. Each new step has CC 8 or less, with 100 % line coverage. The method keeps the `_unlaunchedLeases` disposer, the mutex block and the per-task loop. No step takes the mutex or starts a drain, and every step call inside the mutex block is awaited. ## Implementation - `countQueuedDrainSlots` reads the sweep generation before the turn count and does the sweep bookkeeping only when the scan saw every owner. - The turn count still runs after the prepass. A test in coder#7744 depends on that order. ## Risks Queue drains run after every task state change. The split adds microtasks between tasks inside the mutex block (up to 7 before the next task's first call), as the merged `recoverInterruptedTasks` split did. All 5019 targeted tests pass at every part. A mutation pass over the new code left 33 survivors: equivalent mutants, assertions for impossible conditions, empty-string edge cases, one debug log, and two test gaps (an extra coalesced drain, and a worktree parent's runtime project path). --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$313.07`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=313.07 -->
Summary
Adds 19 characterization tests for
TaskService.maybeStartQueuedTasksFromReservations(the queue drain), with no production change. Line coverage of the method goes from 90 % (283 of 316 lines) to 100 %. This is the test PR at the start of stack 9 of the TaskService recovery and launch path split. The split PRs on top of it split the method (CC 76) into named steps.What the tests cover
The new file is
taskService.queuedDrain.test.ts. The tests start the drain through the publicmaybeStartQueuedTasks(). They check the plans that the drain hands toenqueueReservedTaskLaunch, the rows it marks failed, the rows it leavesqueued, and the slots it uses. No test checks log or error text.Slots: with one free slot, only the first queued task is reserved. A queued task that already streams becomes
runningand takes a slot.Bad rows: a task without
parentWorkspaceId, a parent without a workspace name, and a task without a workspace name are each marked failed. The next queued task still launches in the slot.Fallbacks: when the parent metadata read fails, the plan carries fallback metadata, named by the folder or by the parent's project entry. When the
skip_init_hookread throws, the launch goes on withskipInitHookfalse.Workflows: a workflow-owned queued task whose run ends between the first owner check and the per-task loop is interrupted, not failed, and its slot goes to the next task.
Found by a mutation pass during the split (second commit): queued tasks launch oldest first; a task with an unlisted secondary project is marked failed; the metadata fallback uses the directory name when the parent's projects list names only other projects; once shutdown began, the drain stops at the first CAS, holds no lease for the next task and leaves both tasks queued; a reservation this process owns keeps its attempt id and abort signal; the handed-over row is
startingwith the plan's attempt id and the brief's send id.Launch leases (third commit, from the stack's advisor): the shutdown exit, a lost CAS, and a throw out of the per-task loop each leave no launch lease held. Before this commit, deleting all three releases and the
_unlaunchedLeasesdisposer failed no test. The tests readworkspaceUseLeasesFor(config).heldCount(taskId, "launch").Validation
--rerun-each 10), and it passes withXUM_DISABLE_PROJECT_AUTOMATIONset to 1 and to 0.Found while testing
A queued task that already streams uses two slots in one drain:
countActiveAgentTaskscounts it, and the drain adds one more. The test pins the current behaviour. #7743 tracks the fix.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$307.02