🤖 perf: bind xum server listener before startup recovery; stop per-task config.json reloads - #4058
🤖 perf: bind xum server listener before startup recovery; stop per-task config.json reloads#4058ibetitsmike wants to merge 28 commits into
Conversation
… config reloads (#4055) - ServiceContainer.initialize() split into initializeCore() (fast) and runStartupRecovery() (O(workspaces) task/workspace recovery); `xum server` binds its port after core init and runs recovery in the background. - Recovery snapshots config synchronously before its first await so tasks created by clients after listen are never treated as stale `starting` tasks; running-task resume skips tasks that are already streaming. - Startup patch-generation and reported-task cleanup loops reuse one config snapshot instead of re-parsing config.json per task. - TerminalAttentionStore.listPendingOwnerWorkspaceIds scans session dirs with bounded concurrency and short-circuits on the first pending record. - [startup] completion logs switch to warn above SLOW_STARTUP_WARN_THRESHOLD_MS.
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: 5f89fb0cec
ℹ️ 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".
- Capture the recovery config snapshot before startServer() opens the listener (it accepts requests while awaiting the lockfile/mDNS), and pass it into runStartupRecovery(). - Orphan scratch workdir sweep skips recently touched dirs and re-checks fresh config before deleting, so createScratch during recovery is safe. - Isolate workspace/task recovery failures so the periodic services (idle compaction, heartbeat, agent status) always start.
|
@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: 314818f428
ℹ️ 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".
…anges - Stale-starting recovery compare-and-sets: only entries still `starting` are rewritten, so a task_stop that persisted `interrupted` wins. - Running-task resume re-reads the live status before nudging, so a task stopped after the snapshot is not restarted. - WorkspaceService.initialize re-reads config right before scheduling chat startup recovery, skipping workspaces archived or removed since the metadata read.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6aa18ab264
ℹ️ 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".
…p and unarchive Startup recovery now runs while clients are connected, so every one-shot read it makes before a mutation can go stale: - The pending-guidance replay, restart nudge, and awaiting_report completion prompt carry an admissionStale probe. A task_stop that lands between the live status read and turn admission now refuses the send instead of resurrecting the stopped task through markInterruptedTaskRunning. - reconcileAgentTaskExecutionIds compare-and-sets its mirror write under the per-handle settlement lock, so a stop that settled the handle during the liveness check keeps its terminal mirror and no stale live registration is installed. - cleanupArchivedDevToolsLogs re-checks the live archive state under the task-tree lock before each deletion. DevToolsService.hasWorkspaceData gates that fresh config read so it only happens for actual deletions, not once per archived workspace.
|
@codex review |
1 similar comment
|
@codex review |
Now that the graceful SIGINT handler is installed while the startup DuckDB sync may still be running, exiting the process mid-sync tore the worker down inside native DuckDB and aborted the process (Napi::Error -> SIGABRT, 7/8 reproductions in UAT). AnalyticsService.dispose() now waits (bounded by ANALYTICS_WORKER_SHUTDOWN_TIMEOUT_MS) for the worker to exit after posting the shutdown message.
|
@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: f095888084
ℹ️ 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".
…nstalled execution handle - AnalyticsService.dispose() now waits for the worker's exit event with no local bound. A 2 s timeout let dispose resolve while an ingest still held DuckDB open, so the caller's process.exit() reproduced the SIGABRT the wait exists to prevent. The outer quit budgets in cli/server.ts and desktop/main.ts already race the whole dispose and own the hard-exit decision. A worker that already exited short-circuits the wait. - reconcileAgentTaskExecutionIds also compare-and-sets the workspace mirror pointer against its snapshot. A client that installs a new handle while reconciliation awaits the old handle's liveness check writes the mirror under a different settlement lock and leaves the old record unchanged, so the record-status check alone let the stale handle overwrite 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: 02cfc0a53a
ℹ️ 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".
…and removed tasks - interruptTaskRecoveryForInactiveWorkflowOwner leaves a child alone when a workflow resume is admitted for the run's workspace (checked synchronously inside the config edit via the workflow admission registry), so recovery cannot mark a freshly resumed run's child interrupted. - maybeStartGeneration re-reads config before creating a pending artifact for a snapshot child that has no artifact yet, so a task removed since the startup snapshot does not get a new artifact.
|
@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: b460a3e682
ℹ️ 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".
…ity on live config - Workflow admissions are tracked per run id as well as per workspace; startup recovery skips a stale child only when a resume of its own run is in flight, so unrelated runs in the same workspace no longer shield it. - canCleanupReportedTask treats a caller snapshot as a screen only: any positive verdict is re-evaluated on freshly loaded config before the workspace is removed.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
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: 3a74713dbb
ℹ️ 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".
A startup-recovery chain observes neither the housekeeping abort nor the bounded join, so a recovery that was one await away from dispatching could start a provider stream during the join window. Set the session shutdown latch right after the abort, before waiting on housekeeping to settle.
This comment has been minimized.
This comment has been minimized.
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: 5cebf008cf
ℹ️ 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".
|
@codex review Round 20 (08ca3a8): merged current main into the branch (no code changes of our own since 5cebf00). Main now carries the OpenAI 404 model_not_found classification fix via #4067, which is what made Test / Integration (modelNotFound) fail on the older base. The single Test / Unit failure on 5cebf00 (code_execution "guest Proxy vars swallows writes") was a QuickJS WASM "Unreachable code" crash while constructing a fresh runtime; it passes locally on both this branch and origin/main, and this PR touches no ptc/code_execution files. |
streamWithHistory re-checked only the per-turn abortSignal after its awaits, and internal retries carry none, so a retry that was mid-preparation when shutdown began could still reach the provider. Fold disposed/shuttingDown into the checkpoint predicate so every boundary before streamMessage honors it.
This comment has been minimized.
This comment has been minimized.
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: 1d4d77ecd9
ℹ️ 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".
A reported best-of child that a client reawakened keeps its previous report artifact while its continuation runs, and the grouped builder only checked that every artifact exists. Revalidate each non-reporting sibling's execution and stream state before assembling the grouped output, and treat such a sibling as recoverable for the synthetic fallback so the parent waits for the new report.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 433e2bda4f
ℹ️ 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".
…st-of assembly Removal holds turn admission on the workspace's sessions for its whole duration (released on failure), so startup recovery resuming from a disk read cannot start work against a checkout being deleted. sendMessage refuses at the pre-persist gate once the shutdown latch is set, so no follow-up row can read as a dispatched turn on the next startup. Best-of assembly re-reads sibling state after its artifact reads so a reactivation during those awaits cannot ship the stale report it already collected.
This comment has been minimized.
This comment has been minimized.
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: b795316c1e
ℹ️ 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".
1 participant
Summary
xum servernow binds its port after the fast core init plus agent-task restart recovery, and runs the O(workspaces)/O(reported tasks) startup housekeeping in the background, so time-to-listen depends on the number of active tasks rather than on deployment size. The housekeeping loops also stop re-parsingconfig.jsononce per reported task, the terminal-attention pending scan is bounded-concurrent, slow startup passes are logged atwarn, andAnalyticsService.dispose()waits for its worker so a signal during the startup DuckDB sync no longer aborts the process. Fixes #4055.Background
On a deployment with 1,535 workspaces (343
reportedtasks, 18 GB sessions dir)xum servertook 13 minutes to bind its port becausesrc/cli/server.tsawaitedserviceContainer.initialize()beforestartServer(), andTaskService.initializespent ~776 s in sequential recovery: a sequential scan of every session dir for pending terminal-attention records (306 s), patch-generation recovery with a synchronous 2.47 MB config parse per reported task (205 s), and reported-task cleanup with another parse per task (131 s). Clients got connection refused the whole time and Coder marked the app unhealthy. See #4055 for the measured timeline.The part of startup that actually has to precede clients is small: reconciling execution handles, fixing stale
startingtasks, draining the queue, and prompting the handful ofawaiting_report/runningtasks to continue. Everything that scales with deployment size (patch artifacts and cleanup for every reported task, the session-dir scan, workflow garbage sweeps, chat restart retries, orphan sweeps) is housekeeping that is idempotent and already runs against live state at runtime.Implementation
TaskService.initialize()is split intorecoverInterruptedTasks()(execution-handle reconciliation, stale-startingfixup, queue drain, inactive-workflow-owner prepass,awaiting_report/runningrestart prompts; bounded by the number of active tasks) andrunStartupHousekeeping({ signal })(patch-artifact recovery, best-of delivery, reported-task cleanup, workflow archive sweep, terminal-attention sweeps and drains).initialize()still runs both for desktop startup and tests.ServiceContainer.initialize()is split the same way:initializeCore()(extensionMetadata, telemetry, policy, experiments, thentaskService.recoverInterruptedTasks()) andrunStartupHousekeeping()(workspaceService.initialize(),taskService.runStartupHousekeeping(), then the idle-compaction/heartbeat/agent-status starts and the completion log).server.tsawaitsinitializeCore(), starts the server, then runsrunStartupHousekeeping()in the background with an error handler;dispose()aborts the housekeeping signal and joins the in-flight step (bounded bySTARTUP_HOUSEKEEPING_JOIN_TIMEOUT_MS, 500 ms) so a shutdown mid-housekeeping stops at the next step boundary before the services it uses are torn down, and never starts periodic services against disposed dependencies; the transient chat-recovery sessions that housekeeping scheduled are disposed right after that join (their chains re-checkdisposedbefore every dispatch), withbackgroundProcessManager.beginShutdown()still the first teardown step so none of those disposals can erase persisted armed-monitor records, andworkspaceService.initialize({ signal })schedules none once shutdown began. Desktop startup is unchanged (initialize()).beforeRemoveprecondition thatWorkspaceService.remove()evaluates inside its task-tree lifecycle lock (the lock reactivation, re-parenting, andtask_stopall mutate under). That confirmation also rejects a task whose execution mirror (taskExecutionStatus) is starting or running, which is the state an existing-workspace turn leaves before its stream registers, and the lineage walk continues from the parent the live confirmation saw (a re-parent since the screen).tryFinalizePendingTaskToolCallInPartial, also the runtime child-report path) is now a compare-and-set:HistoryService.updatePartialIfMessageIdMatches()re-reads the partial under the per-workspace file lock and writes only while it is still the same message with the task call still pending and no stream running. A parent turn that starts meanwhile (client send, or a resumed child'stask_send_message) commits that partial and writes its own under a new id, so the finalization is dropped instead of resurrecting or overwriting it.commitPartial()is now one transaction under the same pair of locks the CAS holds (the workspace mutex and the cross-process history write lock): snapshot, history append/update/delete, and partial delete cannot interleave with the CAS or with another backend's commit of the same partial, so a finalization is either included in the commit or declined, never appended pre-update and deleted. The lock-held bodies ofappendToHistory/updateHistory/deleteMessage/getHistoryFromLatestBoundarywere extracted for that (their public wrappers are unchanged).maybeStartGeneration(..., { config }); a snapshot child with no artifact yet, or with a crash-left pending one, is re-checked on live config before generation runs, and skipped when it was removed or reactivated (active workspace-turn execution) or is streaming. Reactivated tasks get their artifact from the existing continuation refresh when that execution settles.archive()disposes a workspace's still-pending transient recovery session oncearchivedAtis durable; a recovery scheduled on a session a client had already created is not disposed by archive, so the two internal dispatch points of startup recovery (pending compaction follow-up, startup auto-retry) re-read the durable archived state right before dispatching, mirroring the guardWorkspaceService.sendMessagealready applies; the orphan scratch-workdir sweep gained the session sweep's grace window and fresh-config recheck; the DevTools-log sweep re-checks live archive state under the task-tree lock before each deletion.TerminalAttentionStore.listPendingOwnerWorkspaceIds()scans session dirs through anAsyncSemaphore(16)and short-circuits per owner on the first pending record.[startup] ... completedlogs uselog.warnwhentotalMsexceedsSLOW_STARTUP_WARN_THRESHOLD_MS(30 s,src/constants/startup.ts).AnalyticsService.dispose()waits for the analytics worker to exit instead of tearing it down mid-native call. With the graceful SIGINT handler installed while the startup DuckDB sync may still be running, exiting the process mid-sync tore the worker down inside native DuckDB and aborted the process (Napi::Error-> SIGABRT); UAT reproduced this 7/8 times before the fix.Validation
Remote dogfood UAT ran on a Coder Agents workspace against an earlier head of this branch (one where all recovery, including the task prompts, ran post-listen) and
mainwith identical synthetic roots (config.jsonwith 1,500 to 8,000 workspaces, 300 to 2,000reportedtasks, session dirs with terminal-attention records), driving the real UI with a real model:/health200 at N=1,500: 2.7 to 3.9 s (branch) vs 12.8 s (main); at N=8,000 with 1,500 reported tasks: 2.2 s, while background housekeeping took ~57 s. The current head additionally awaits the task recovery phase before binding; that phase is bounded by the number of active tasks (0 to 2 in the measured deployments, sub-second) rather than by N./api/docs, and a 150-request hammer stayed healthy during housekeeping; a workspace created during housekeeping persisted with a clean 2-turn chat;kill -9mid-housekeeping restarted cleanly; a second server on the same root was refused by the lockfile.cleanupReportedTasksMsstayed flat as config grew 16x (111 to 173 ms vs 701 to 8,479 ms onmain);terminalAttentionDrainMsover 8,001 session dirs was 209 ms;pendingTerminalAttentionOwnerWorkspaceCountwas exact (3 of 3), including with a corrupt record and a stray file insessions/.initializeCorewaits for task recovery and does not run housekeeping;recoverInterruptedTasksresumes running tasks without touching reported ones whilerunStartupHousekeepingprunes reported tasks without resuming anything; best-of finalization leaves a parent partial that a live turn replaced mid-finalization untouched (red-green); dispose aborts in-flight housekeeping before the periodic services start; the snapshot patch path skips removed and reactivated children (red-green).Risks
server.tsremains a fast-fail nicety, as onmain: task recovery runs beforestartServer()acquires the lock, so two servers racing on one root could both attempt recovery;startServer()still refuses the second one.Cleanup timed out, forcing exitpath does not take effect in that state. Adocker stopwith a 10 s grace period would SIGKILL mid-checkpoint. Bounding or interrupting the checkpoint is a follow-up.pendingby a crash), each completion still triggers a cleanup recheck that reloads config, sopatchGenerationRecoveryMsstill grows with config size in that case. It is off the connect path now.WorkspaceService.initializechat-restart recovery with thousands of non-task workspaces is noisy (MaxListenersExceededWarning); pre-existing and unchanged.cleanupReportedLeafTaskunder the workspace event lock andWorkspaceService.remove()then takes the task-tree lifecycle lock (event -> tree), whiletask_send_messagetakes tree -> event. This PR adds no acquisition on that path.Pains
Eleven Codex rounds found one race after another between background recovery and early clients while the whole of
TaskService.initializeran post-listen (stop vs restart nudge, reactivation vs patch artifact, unarchive vs DevTools sweep, workflow resume vs child interrupt, ...), each fixed with another compare-and-set or lock. Moving the small state-mutating recovery phase back in front of the listener removed that whole class along with the admission probes, idle-only sends, refunds, run-keyed workflow admissions, and settlement-lock compare-and-set they had accumulated (net about 120 production and 360 test lines), and left only idempotent housekeeping behind the listener.Generated with
xum• Model:anthropic:claude-fable-5-1• Thinking:xhigh• Cost:$__COST__