Skip to content

Auto-update: server updates itself when quiet, desktop installs on quit - #5

Open
scuttletoby wants to merge 4 commits into
mainfrom
fork/auto-update
Open

scuttletoby wants to merge 4 commits into
mainfrom
fork/auto-update

Conversation

@scuttletoby

@scuttletoby scuttletoby commented Nov 12, 2026 •

Copy link
Copy Markdown
Owner

What

There are two opt-in settings, and both are off by default.

  • Server: autoUpdateServer (in the server settings, with optional autoUpdateQuietCommand)
    • When the server runs as the background service, it checks the release source every hour.
    • If a newer release exists on the same track, it waits until the server is quiet, then runs the existing self-update. The next section defines quiet.
  • Desktop: "autoUpdate": true (in desktop-settings.json)
    • After each background update check, an available update is downloaded.
    • It installs when the app quits.

There are no UI toggles yet. SettingsPanels.tsx changes often upstream, and editing it would cause a rebase conflict on almost every sync. Users set the options in the settings files.

How the server decides it is quiet

Thread state is read through upstream's own helpers: the threads from getRecoveryThreadIds("runtime"), each read with getRuntimeRecoveryProjection. The server is quiet only when all of these hold:

  • no run is preparing, starting, running or waiting (a parent waiting on its children counts as busy);
  • no queued run is waiting to start, unless the queue is held;
  • no approval or question is pending;
  • no background work is pending, counted by upstream's derivePendingBackgroundWork. That covers provider background tasks and live command_execution / dynamic_tool / subagent turn items, so Codex and the other adapters count too, not only Claude;
  • no integrated terminal has a command running (from the terminal metadata stream the UI uses);
  • if autoUpdateQuietCommand is set, it exits 0.

Link requests are not counted. Recovery leaves link cards in place across a restart, and one the user ignores would otherwise block updates forever.

The quiet command is an absolute path. It runs with the server base dir as its cwd and a 30 s timeout, and gets two environment variables: T3CODE_AUTO_UPDATE_CURRENT_VERSION and T3CODE_AUTO_UPDATE_TARGET_VERSION.

  • Exit 0 means "update now".
  • Any other exit means "not now", and the first line of output is logged as the reason.
  • A timeout or a spawn error also means "not now".

The command runs twice per attempt: once at each quiet check, and once more at the handoff.

Work that arrives during the update is held, then started

A server-wide gate, WorkAdmission (apps/server/src/workAdmission.ts), sits in front of the places where new work starts:

  • the outbox daemon's claims (EffectWorker.runDaemonWithOptions): turn starts, continues, steers, approval answers and notifications;
  • worktree preparation (ThreadLaunchService), including setup scripts.

At the self-update's installing stage, just before the launcher request, the updater runs the last check in this order:

  1. Re-read the setting. If it was turned off while staging, stop.
  2. Run the quiet command.
  3. Close the gate and note the time. New work now waits.
  4. Count the work already admitted. Anything still starting means not quiet.
  5. Read thread state, last.
  6. Write the marker <baseDir>/runtime/work-held-for-update.json with that time and the target version. If it cannot be written, the update does not go ahead.

If any step says "not now", the gate opens again (removing the marker) and the updater goes back to waiting. The staged runtime is reused next time.

While the gate is closed, nothing new is claimed. Outbox rows stay pending in the database. A worktree preparation waits at the gate instead of failing its run.

At the restart: decided by save order, started after the commit. Startup recovery (ProviderRuntimeRecoveryService) reads the marker. If it is fresh (under 2 h old), recovery keeps the held work instead of cancelling it. Held work means every start, preparation or queued run that no provider ever claimed.

The marker's time is not used to pick this work. The gate closed before the last quiet check, and anything that check counted stopped the update. So all unclaimed work at the restart was saved after the hold, whatever its request time. That also covers "send now", a resumed queue and a worktree retry, which keep their old request times, and a request stamped just before the hold but saved after it.

  • a run that never started (a user message, a scheduled run, a t3_thread_send message, a delegated child's completion report) gets a fresh provider-turn.start;
  • a run held in preparing has its worktree preparation run again;
  • queued runs on the same thread start behind them as usual. Queued runs on a thread with no such held run take the upstream path, because nothing would start them.

User-held queues and buffered notifications keep upstream's behaviour. Without a marker, recovery is unchanged.

Nothing starts before the launcher commits the update. Recovery only decides what to keep. Startup (serverRuntimeStartup.ts) starts it with startHeldWork right after launcher.prepareTrial succeeds. If the trial fails before the commit, the launcher restores the database and the previous version runs again. That version finds the same held runs and starts them, so each starts once across both processes.

Exactly once across restarts. The marker records two things:

  • the runs the first startup kept (runs), written before the commit;
  • the runs already started (started), written per thread before that thread's starts are written.

Each later startup considers only runs minus started. Once everything has started, the marker is removed. If starting fails part-way, the marker stays, and the next startup starts only what is left. A crash between the started record and the start write loses that start (at most once, not twice). The run then takes the upstream path at the next startup, as a visible held queue or a cancelled agent message.

The updater does not check for updates again until held work has settled. A stale (over 2 h) or unreadable marker is removed and ignored.

Outbox, thread launch and recovery each register on the gate. The updater refuses to run, with a logged error, on the pass-through default (tests and tools) or when one of them did not register. It also refuses unless the server is the background service.

Gate vs launcher

Once the launcher has been asked, the gate stays closed until the process exits, with these exceptions:

  • The launcher rejects (its own rejection error): the gate opens and the updater backs off.
  • The answer is lost (for example the 30 s exchange timeout): the gate stays closed, because the launcher may still restart this server.
  • Every tick, the updater reads the launcher's state file:
    • Unreadable: the gate stays closed and a warning is logged once.
    • A pending record for this version: the hold is confirmed and continues.
    • Anything else: the launcher dropped the handoff. For a handoff whose answer was lost, this only counts after a 5-minute grace period.
  • After 30 minutes (AUTO_UPDATE_MAX_HOLD_MS) the gate opens, unless the launcher's record still shows this update pending. A pending record keeps the hold past the cap, because the restart may still come, and a warning is logged once. An unreadable or different record past the cap opens the gate.

When a handoff is dropped, the gate opens (removing the marker), the self-update's "update in progress" flag is reset so the Update button works again, a warning names the reason, and the updater backs off.

Visible status. The log shows when the hold starts (with the target version and the 30-minute limit) and why it ended. There is no UI or health surface for the auto-updater to show it in, and this PR does not add one.

How long the window is. It runs from the gate closing until the new server's recovery starts the held work. On a filesystem without cloning (ext4, for example) the launcher goes through these steps in order:

  1. the backup-space check (milliseconds);
  2. HANDOFF_DELAY_MS (2 s);
  3. the online backup copies the main database file with cp and fsyncs it, while the old server still runs;
  4. SIGTERM, with up to 5 s grace for the old server to stop;
  5. the WAL is copied and fsynced;
  6. the trial server starts.

A synthetic 1 GiB file copied and fsynced on a SATA ext4 disk took 1.4 s, 5.8 s and 5.1 s in three runs, with the source in the page cache. A cold source would be slower. A 128 MiB WAL took about 5 s, while earlier writes were still flushing. For a 1 GiB database that adds up to roughly 10–20 s plus the trial startup. The copy time grows about linearly with database size. Trial startup was not measured. On a filesystem that can clone (btrfs, XFS with reflink, APFS), the offline backup clones instead and the copy step is close to instant.

Reused code

Nothing new touches the install, handoff or rollback paths.

  • Install. ServerSelfUpdate.update (apps/server/src/cloud/selfUpdate.ts) stages the runtime and hands off to the launcher. The auto-updater shares that one service instance with the Update button, so its inFlight guard covers both. The last check runs inside its installing progress callback. The only addition is resetAfterDroppedHandoff.
  • Listing releases. It uses resolveNewestVersion / resolveMirrorVersion from cli/update.ts, and honours a managed update mirror.
  • Rollback. The launcher's trial start and rollback handle a new build that fails to start. The auto-updater reads runtime/service-state.json. See the failure table.
  • Threads after the restart. RestartContinuation.ts continues threads that were mid-turn, and its chain cap (MAX_CONTINUATION_CHAIN = 3) still applies.
  • Activity. Only upstream's existing projection reads are used, so there is no schema change.

Failure cases

Case What happens
The Update button's update is already running The shared inFlight guard refuses before anything starts. The updater logs that another update is running and tries again a minute later. This is not counted as a failure.
Work starts while the update is staging (staging can take minutes) The last check at the installing stage catches it. The update stops before the launcher handoff, the gate opens and the updater goes back to waiting.
Work arrives after the gate closed It is not claimed. After the restart, recovery starts it (see above).
The marker cannot be written Counted as "not now": the gate opens and the updater waits.
The launcher's answer is lost The gate stays closed until the record shows a pending update (confirmed), or until 5 minutes pass with no restart in the record. Without a pending record it opens after 30 minutes at most.
The launcher hangs with a pending record The gate stays closed past 30 minutes and a warning is logged once. The work stays saved, but nothing new starts until the process exits.
service-state.json unreadable after the handoff The gate stays closed (up to 30 minutes) and a warning is logged once.
The launcher already has an update pending No new release is offered until it is gone.
Never becomes quiet It gives up after 2 h of waiting and tries again at the next hourly check. It never forces a restart.
Stops half-way (crash or kill during the update) The launcher's state file and its #recover path handle this, the same as for a manual update. A marker left by a crashed process is ignored once it is over 2 h old.
New build fails to start, launcher rolls back That version is skipped from then on. A newer release is still offered. The trial started no held work, and the previous version starts it after the restore (see the table below).
Launcher records the attempt as failed (for example a transient start timeout) It backs off 6 h once for that attempt, then offers the release again. It is logged as failed, not as rolled back.
service-state.json cannot be read or parsed before an update No update happens, and a warning is logged once per reason.
Release listing fails (offline, rate limit) A warning is logged and it checks again in an hour.
Install fails after it started A warning is logged and it backs off 6 h.
Setting turned off while waiting It stops waiting at the next tick, with no restart needed.
Setting turned off while staging The last check sees it and stops before the handoff.
Desktop-managed server, or no launcher The server logs once that auto-update needs the background service, then does nothing. A desktop server updates with its app.
A gate that holds nothing, or a consumer that does not use it The updater refuses and logs an error once.

Timings. The first check is 5 minutes after start, then one check every hour. While waiting, quiet is checked once a minute.

State: each write step, run again, stopped half-way, old or empty state

Each row is one write, with what happens if the process crashes right after it, if the step runs a second time, and if it meets old or empty state. Test names are in ProviderRuntimeRecoveryService.test.ts (PRRS), autoUpdate.test.ts (AU), workAdmission.test.ts (WA), serverRuntimeStartup.updateHold.test.ts (SRS) and runtimeLayer.test.ts (RL).

Write step Crash right after it Second run Old or empty state Test
Hold (gate closes, in memory) Nothing on disk. With no marker, recovery takes the upstream path: user messages wait in the visible held queue, agent messages are cancelled. hold twice stays held, and one release opens it. A fresh process starts with the gate open. RL "work that waited at the closed gate … ordinary restart"; WA hold/release
Marker write (heldAt, target) Recovery keeps the work saved after the hold and starts it after the commit. Rewriting the marker keeps a runs list that is already recorded. Stale (over 2 h) or unreadable marker: removed, upstream path. No marker: upstream path. PRRS "none, stale and invalid markers"; WA marker freshness
Recovery start: startup records runs, then keeps the work in memory Nothing has started. The next startup reads the same database and runs, and keeps the same work. Recovery twice in one process starts nothing extra (it only keeps). runs is recorded only once, so a later startup cannot widen it. Without runs (written by the first startup only), everything unclaimed counts. PRRS "recovery twice", "runs-list narrowing"
Commit, then start: prepareTrial succeeds; per thread, started is written, then its starts Before started: the next startup starts it (once). Between started and the start write: that start is lost (at most once) and the run takes the upstream path. After: nothing more starts. A second startHeldWork or a later startup starts only runs minus started. A part-way failure keeps the marker, and the retry finishes the rest. Disk error writing started: the start is not written, the marker stays, and the updater waits (settled stays false) until the next startup. SRS "after the update trial, committed"; PRRS "partial failure then retry", "crash mid-start"; RL "exactly once when the trial commits"
Rollback: the trial fails before the commit, and the launcher restores the database The trial started nothing. The restored previous version reads the same held runs and starts each once. A further restart after that starts nothing new (the marker is gone). An old version without this PR ignores runs and started: it would start held work by its own rule. Only the 2 h freshness limit protects against that. SRS "failing before the commit"; RL "exactly once when the trial fails and the launcher restores the database"

Desktop

autoDownloadAvailableUpdate runs after the startup check and after each poll check.

  • Setting on, update available: it downloads the update. If the download completed and is still offerable, it calls setAutoInstallOnAppQuit(true).
  • At quit: a finalizer on the updater's scope runs as the app shuts down, before electron-updater's own quit handler installs.
    • It reads the setting again. If it is off now, or the download is gone, install-on-quit is switched off and nothing installs, even within the poll interval.
    • Otherwise it writes the same install record the Update button writes. A crash or power cut before a real quit leaves no record, so it is not reported as an install that did not apply, and that version is not held back.
  • Install on quit that did not apply: the app restarts on the old version. The existing "last update did not apply" check at startup reports it, as it does for the Update button. That version is then not downloaded and armed again automatically, which prevents a silent loop. The Update button still offers it.
  • Turning off: if the setting is off at a poll, or the update track changes, install-on-quit is switched off. This follows upstream's rule that a download from a track the app no longer follows must not install.

How quitting interacts with it:

  • The quit path uses app.quit, so electron-updater's install-on-quit runs.
  • The relaunch path uses exit(0), which skips the install.
  • Install-on-quit doesn't write the app's own restart marker. So after a desktop install, threads do not auto-continue. After a server update they do, through RestartContinuation.

Tests

  • apps/server/src/cloud/autoUpdate.test.ts runs a fake-server loop around the real updater: launcher, thread state, hook, self-update and gate. It covers:
    • the order hook → gate → thread state, with the gate closed before the read;
    • the marker written at the hold and cleared at release, and a failed write counting as busy;
    • the hold until exit; release on a rejected handoff; an unreadable record keeps the hold;
    • a lost answer with a pending record keeps the hold; one with no record is dropped only after the grace period; the 30-minute maximum; the in-progress reset after a dropped handoff;
    • clearing the leftover marker after the restart;
    • refusal on the pass-through gate and on a partly registered gate;
    • terminal commands counted through runningTerminalCommands;
    • the round-1 cases: re-checks, launcher records, the Update button first, and each kind of work that counts.
  • apps/server/src/workAdmission.test.ts: hold and release, the in-flight count, the pass-through default, the outbox daemon not claiming while held, the marker file and its freshness, and consumer registration. Together with autoUpdate.test.ts: 47 tests.
  • ThreadLaunchService.test.ts: a worktree preparation waits at a closed gate, then runs. A preparation still held when the server stops stays preparing, and the next process's resumePreparation prepares it.
  • ProviderRuntimeRecoveryService.test.ts, block "work held for an automatic update":
    • each kind of held work starts only after startHeldWork (the commit), and none starts during recovery;
    • work is picked whatever its request time ("send now", resume, retry, a request stamped before the hold);
    • a user-held queue is not started;
    • rollback and crash after the commit each give exactly one start;
    • recovery twice in one process; a part-way failure and its retry; a crash mid-start (at most once, then the upstream path);
    • the runs list narrows later startups;
    • none, stale and invalid markers.
  • serverRuntimeStartup.updateHold.test.ts (fork-owned): held work starts after prepareTrial, and not at all when it fails.
  • runtimeLayer.test.ts:
    • a DB-backed simulated restart through the real runtime layer, with and without a marker;
    • end to end on a file database: the old version holds and writes the marker; the trial process runs; the launcher's copy is restored; the old version runs again; then a later restart. Each held run reaches a provider exactly once, whether the trial commits or is rolled back. One message is stamped before the marker's time.
  • apps/desktop/src/updates/DesktopAutoInstall.test.ts (7): the record is written at quit and not before; a stop without a quit leaves no record; turning the setting off just before quitting installs nothing; plus the round-1 cases.

Red on the old code. The new tests run against 5280dbc's sources: 35 server tests and 3 desktop tests fail. Some of the server failures are API changes. The hand mutations below are the precise check. Each was made on the new head, with only its own tests run, and every one turned at least one test red:

Mutation Failing tests
thread state read before gate.hold 1
admit removed from ThreadLaunchService 1
terminal command count forced to 0 1
recovery ignores the marker 6
recovery does not start held work 6
missing-consumer refusal removed 1
unreadable launcher record releases the gate 1
no maximum hold 1
no in-progress reset after a dropped handoff 3
admit removed from the outbox daemon 1
recovery does not register on the gate 2
desktop: no quit step 2
desktop: record written at arm time 5
desktop: setting not re-read at quit 1

Round 3 (fix round on 5164fea), each hand break on 350656a, with the tests it turns red:

Mutation Red
held work started during recovery (before the commit) 10 recovery tests; RL rollback end to end
held work picked by request time again recovery request-time tests; RL both end-to-end cases
user-held queue guard removed recovery "user-held queue"
started not recorded recovery "crash after commit", "recovery twice"
runs list ignored recovery "runs-list narrowing"
marker cleared when starting fails recovery "partial failure then retry"
startHeldWork moved before prepareTrial SRS both cases
startHeldWork removed from startup SRS "committed"
updater does not wait for held work to settle AU "no update checks until held work is settled"
30-minute cap checked before the launcher record AU "pending record past the cap keeps holding"
ThreadLaunchService: preparation interrupted at stop marked failed (:829 guard) "keeps a preparation held when the server stops"
ThreadLaunchService: setPreparationResumer removed (:1177) the same test (fails at the 120 s test timeout)

Local runs on 350656a (fork CI does not run tests):

  • Typecheck is clean for server and desktop. Lint, format and knip pass (pre-commit hook).
  • Server: src/orchestration-v2 2512/2513 pass; everything else 4726 pass (366 files). The one failure is ProjectionStore.test.ts, which this PR does not touch: it uses StatementSync.stat, which this machine's Node lacks.
  • Desktop (no changes this round): 1045 pass. Two files fail here: browser-secret-native needs the libsecret-1 dev package, which this machine lacks, and DesktopBackendManager timed out under load during the full run but passes alone (12/12).

Not yet proven:

  • No end-to-end run against a real launcher. The rollback is simulated: a file database copied at stop and restored after the trial.
  • No real electron-updater download and install-on-quit. Both need a published fork release first.
  • An old version without this PR ignores runs and started. If a rollback goes back to such a version, it can start held work by its own rule; only the 2 h marker limit protects against that.
  • A launcher record that stays pending forever keeps the gate closed until the process exits.
  • A disk failure while writing started leaves the updater waiting (held work not settled) until the next restart.
  • A crash after the gate closed but before the marker write takes the upstream path: user messages wait in the visible held queue, agent messages are cancelled.
  • A crash between the started record and the start write loses that start (at most once), and the run then takes the upstream path.
  • How recoverDelegatedTasks treats a child that is still preparing at the restart was not checked.
  • Other workers that drain work outside the outbox and thread launch are not gated.
  • Users' own terminals and clones are not gated after the last check. This is intended.
  • A reboot that kills the app during the quit step, after the record was written but before the install ran, would still report "did not apply".
  • The window length is an estimate from a synthetic copy, not a measured update.

🤖 Generated with Claude Code

Opt-in settings, both off by default:
- Server `autoUpdateServer`: in boot-service mode, check the release
  source hourly; when a newer release on the same track exists, wait
  until no turn, approval, link request or background task is live (and
  an optional `autoUpdateQuietCommand` exits 0), then run the existing
  self-update. Skips the version the launcher rolled back; gives up a
  wait after 2 hours and never forces.
- Desktop `autoUpdate`: download an available update after each check
  and install it when the app quits.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L labels Nov 12, 2026
@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: correctness+concurrency DI-1 · reviewer: opus high (t3lens-di1-opus) · read at: 9df6fa4
covered: apps/server/src/cloud/autoUpdate.ts (all), autoUpdate.test.ts (all), cloud/selfUpdate.ts (all), serviceLauncher.ts Launcher class (#recover, #handleUpdateRequest, #beginTrial, startOnlineDatabaseBackup, #handleExit, #returnToPrevious), cloud/serviceProtocol.ts (state decode, version compare), ProjectionStore.listActiveShellIndex, processRunner timeout/env handling, RestartContinuation.ts head, the DesktopUpdates/DesktopAppSettings/settings.ts/server.ts hunks.
checks:

  • Tests: pnpm install + vp test run src/cloud/autoUpdate.test.ts in my clone: 16/16 pass.
  • Mutation runs (each reverted after): (M1) the installing-stage re-check turned off (stage !== "installing" -> true): 16/16 still pass. (M2) state file path changed to runtimeX/: 16/16 pass. (M3) || update?.status === "failed" removed: 16/16 pass. (M4) rolled-back compare removed from offerableVersion: 1 fails (good). (M5) deleted_at filter on runs removed: 1 fails (good). (M6) 'waiting' removed from the run statuses: 16/16 pass.
  • Race trace from the last quiet check to the server stopping: see the first finding. Nothing in the server holds back new runs during a handoff (grep for a drain/admission gate: none). The Update button has the same window, but there a person chose to restart. Auto-update promises it never restarts during a turn.
  • Every kind of activity, compared with listActiveShellIndex (ProjectionStore.ts:7391-7424): runs, pending requests, waiting link requests and background tasks are read. Non-terminal command_execution/dynamic_tool/subagent turn items and PR watches are not (minor below). Counting 'waiting' runs means a parent that waits on children blocks the update. That is the right, conservative choice, but no test covers it (M6).
  • Rollback skip: the path matches the launcher (autoUpdate.ts:222 vs serviceLauncher.ts:558). decodeServiceState accepts rolled-back/failed only when fromVersion == activeVersion, which holds after #returnToPrevious. The skip is logged (autoUpdate.ts:356). A newer release is still offered. Correct today, but it has no test (M2/M3). If it broke, the loop would be: rollback restarts the old server, state resets, the first check comes 5 min later (autoUpdate.ts:34), the rolled-back version is offered again, the staged runtime is reused, the live server restarts again. That is a restart every few minutes whenever the server is quiet.
  • 6 h backoff / 2 h give-up / Update button: the shared inFlight Ref (selfUpdate.ts:198) rejects the second of two triggers. If the button is first, auto-update logs "failed" and backs off 6 h (logged). The "busy" path (work started while staging) is not counted as a failure, goes back to waiting and stays bounded by the original since. The 2 h give-up resets since on the next hourly check, so it is really a 3 h cycle with a 1 h gap. That is fine and logged.
  • State question: rerun/two triggers: the inFlight guard. Crash mid-download: inFlight is in memory, so a restart clears it. Staging reuse is upstream's. Crash mid-swap: the launcher's pending record and #recover resume or roll back, the same as for the button. Rolled-back version: skipped and logged. Install started while a turn begins: the window below. Empty or old DB: all four COUNTs give 0 and the SUM gives NULL, read as 0. A missing pendingBackgroundTasks key gives NULL, so 0. A malformed payload_json row makes the query fail, which gives "thread state could not be read", so the server is never quiet. That is logged when the reason changes, so it is not silent. Hook missing: spawn error gives "could not run", so not now (logged). Hook hanging: the 30 s timeout, scoped, so the process is killed. Hook path not absolute: logged as not now. env is extended, not replaced (processRunner.ts:274). Setting off while waiting: reset at the next tick (logged).
  • Owner requirements: no personal data in the diff (grep for names/hosts/paths: none). No schema change (one SELECT; two settings JSON fields with defaults). Release lookup goes through resolveNewestVersion/resolveMirrorVersion, so it follows the fork's repository constant. 0.1.9-fork.N passes isExactServiceVersion and compares numerically on N. Hot upstream files: server.ts +3, settings.ts +6, DesktopUpdates.ts +19. The rest is new files, so there is little rebase risk.
    finding: major apps/server/src/cloud/autoUpdate.ts:276-282 + 293-301 (window from serviceLauncher.ts:726-807) - the last quiet check is not the handover. notQuietReason reads thread activity FIRST and only then runs the quiet hook, which can take up to 30 s (HOOK_TIMEOUT_MS). After that, selfUpdate sends request-update. The launcher runs its backup-space check, replies, waits HANDOFF_DELAY_MS (2 s, serviceLauncher.ts:32) and then, in #beginTrial, copies the main DB file while the old server still runs (startOnlineDatabaseBackup, serviceLauncher.ts:302-313, when the filesystem cannot clone; minutes on a large DB). Only then does it SIGTERM the server. Nothing stops new work in that window. Trace: T0 readActivity = all zero. T0+5 s the user sends a message, or a scheduled task, delegate_task child, usage-limit resume or queued follow-up starts a run. T0+20 s the hook exits 0. Accepted, then 2 s plus the copy, then terminateChild. The new turn is killed mid-tool-call, and so is any approval it raised. RestartContinuation then continues it with its "may or may not have run" prompt. The result is the outcome the PR says it avoids ("If something is now running, the update fails before the launcher handoff"). Fix, smallest first: (a) read activity after the hook, not before, so the hook's 30 s is not in the window; (b) hold new run starts from the final check until handoff accepted or failed: a shared "handoff pending" Ref that the run-start path leaves 'queued' (the PR already relies on queued runs surviving a restart). Or have the launcher re-ask the child "still quiet?" right before terminateChild and abort the pending update if not. (c) Until then, the PR text and the code comment should name the window instead of claiming none. (high confidence on the window; its length depends on the hook's run time and on whether the DB filesystem supports clone)
    finding: major apps/server/src/cloud/autoUpdate.test.ts:1-194 - the update loop itself (make/tick/install) has no test. Three guards that keep a live server from restarting can be deleted with all 16 tests green (mutations M1, M2, M3 above): the installing-stage re-check (autoUpdate.ts:290-301, the PR's "work starts while staging" row), the state file read for the rollback skip (autoUpdate.ts:222-238), and the "failed" status skip (autoUpdate.ts:233). The pure helpers are covered; the wiring is not. Needed: a test with a fake ServerSelfUpdate/launcher and a temp service-state.json showing (1) a run inserted between the probe and "installing" stops the handoff and returns to waiting, and (2) a rolled-back or failed record makes the tick log the skip and not call update. (high)
    finding: minor apps/server/src/cloud/autoUpdate.ts:168 - the comment says it reads "the live sources ProjectionStore.listActiveShellIndex reads". It leaves out non-terminal command_execution/dynamic_tool/subagent turn items (ProjectionStore.ts:7404-7409) and PR watches. Most of these sit inside a counted run. A subagent item for a child on a linked environment, or a tool item left non-terminal after its run ended, is not counted. Either add them or say why they are safe to restart through. (medium)
    optional nits:
  • DesktopUpdates.ts:902 sets autoInstallOnAppQuit(true) and nothing sets it back. Turning autoUpdate off after a download still installs on quit, until the app restarts (the reset is at :1184).
  • Turning autoUpdateServer off during staging does not stop that attempt: the installing-stage check (autoUpdate.ts:293) re-reads activity but not the setting.
  • The "give-up" outcome name and log hide that the next check starts a fresh 2 h wait. "pausing for an hour" would describe what happens.
  • No test for a 'waiting' run blocking (M6).

@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: correctness+concurrency DI-2 · reviewer: opus high (t3lens-di2) · read at: 9df6fa4
covered: apps/server/src/cloud/autoUpdate.ts (all), autoUpdate.test.ts (all), cloud/selfUpdate.ts (all), serviceLauncherClient.ts requestUpdate, serviceLauncher.ts #handleUpdateRequest/#beginTrial/terminateChild, pinnedRuntime.ts installPinnedRuntime, serviceProtocol.ts parseServiceState, ProjectionStore.ts listActiveShellIndex, packages/shared/src/orchestrationV2PendingBackgroundWork.ts, processRunner.ts env/timeout, docs/internals/server-updates.md, apps/desktop/src/updates/DesktopUpdates.ts (download, install, install record, setUpdateChannel, configure), DesktopAppSettings.ts diff, contracts settings.ts diff, server.ts diff.
checks:

  • Ran the PR's tests in my own clone: autoUpdate.test.ts 16/16 pass.
  • Removed guards one at a time, re-ran: M1 the installing-stage quiet re-check (autoUpdate.ts:291-302 -> always Effect.void) 16/16 PASS; M2 the rolled-back/failed state-file read (autoUpdate.ts:233 -> false) 16/16 PASS; M3 'waiting' dropped from the run statuses (autoUpdate.ts:180) 16/16 PASS; M4 the bootService guard (autoUpdate.ts:331) 16/16 PASS. Caught: dropping deleted_at on runs, the rolledBack equality in offerableVersion, the hook exit-code rule.
  • Wrote a new test (not pushed): thread with a completed run plus a running 'subagent' item and a running 'command_execution' item, empty provider roster. Upstream derivePendingBackgroundWork returns 2 tasks for it; activityReason(readActivity(sql)) returns null (quiet). Test FAILS. See the second finding.
  • Race traced: final DB read (autoUpdate.ts:276) -> settings read -> hook up to 30 s (autoUpdate.ts:41,252-269) -> requestUpdate (selfUpdate.ts:309-321) -> launcher backup-space check (serviceLauncher.ts:759-773) -> accepted -> HANDOFF_DELAY_MS 2 s (serviceLauncher.ts:32,786) -> online backup start -> SIGTERM (serviceLauncher.ts:803). Nothing stops new runs anywhere in that window.
  • State question. Rerun: tick runs on one fiber (autoUpdate.ts:424), so it never overlaps itself; a staged runtime is kept by its sentinel and validated again (pinnedRuntime.ts:285-307). Crash mid-download: staging goes to a temp dir and is renamed into place (pinnedRuntime.ts:332-365), and a half-written version dir is removed (310). Crash mid-install: the launcher writes the pending record durably before it acks (serviceLauncher.ts:775-785) and recovers like a manual update. Two triggers: the inFlight Ref (selfUpdate.ts:198) rejects the second one and is reset only on error (329); the launcher also rejects a second pending update (serviceLauncher.ts:740). Rolled-back version: only the newest candidate is compared, and a newer one is still offered (tested). Empty DB: quiet (tested). Old DB with missing tables, or malformed payload_json: the SQL error becomes "thread state could not be read" and it keeps waiting; this is logged when the reason changes and at give-up every 2 h. Hook: missing -> spawn error -> not now; hangs -> 30 s timedOutResult -> not now; exit 0 with no output -> update; non-zero with no output -> "said not now"; env is merged with the server's (processRunner.ts:274).
  • Owner requirements: no schema change (a SELECT plus two JSON settings fields); no personal content (grepped the diff); hot upstream files are touched lightly (server.ts +3, settings.ts +6, DesktopUpdates.ts +19). CI: every Test/Lint/Typecheck job on this head is CANCELLED, so there is no green run.
    finding: major apps/server/src/cloud/autoUpdate.ts:273-283 (with selfUpdate.ts:308-321, serviceLauncher.ts:32,786) The last quiet check is not atomic with the restart. readActivity runs BEFORE the quiet hook, so the gap between the DB read and SIGTERM is hook time (up to 30 s) + launcher backup check + 2 s, and nothing blocks new run starts in it. Trace: hook takes 10 s; at +5 s the user sends a message, a scheduled task fires, or a delegate_task child starts; the hook exits 0; handoff; the turn is SIGTERMed about 2 s later. RestartContinuation then continues it with "may or may not have run". Harm is bounded, but the PR's promise (work that started stops the handoff) does not hold. Fix: run the hook first and the DB read last, then hold new run dispatch (keep them queued, which the PR says survive a restart) from the final check until exit, or release the hold if the handoff is rejected. (high on the window, medium on how often it is hit)
    finding: major apps/server/src/cloud/autoUpdate.ts:165-199 The activity query misses live background turn items: 'command_execution' / 'dynamic_tool' / 'subagent' items in pending/running/waiting status on a settled run. Upstream counts these as pending background work (orchestrationV2PendingBackgroundWork.ts:15-19,228-236,296-302), and so does listActiveShellIndex, which the code comment says it mirrors (ProjectionStore.ts:7404-7409). Only ClaudeAdapterV2 writes provider_threads.pendingBackgroundTasks, so for Codex and the other adapters this work exists only as turn items. Inputs: completed run + running subagent item + running command item -> readActivity reports 0 -> the server restarts and kills them. My failing test is described under checks. (high on the query gap, medium on how often adapters leave such items running)
    finding: major apps/desktop/src/updates/DesktopUpdates.ts:901-906 Install-on-quit skips writeInstallRecord, which the manual install writes (DesktopUpdates.ts:817). The "last update install did not apply" check at startup (DesktopUpdates.ts:689-701,1148-1153) therefore never fires for an auto install. Inputs: autoUpdate on; the installer fails on quit (Squirrel/NSIS error, no rights) -> the app restarts on the old version with no warning; the next poll downloads again and re-arms, a silent loop. That is a silent fallback. Fix: write the install record when arming, and remove it if the flag is turned off. (medium-high)
    finding: major apps/desktop/src/updates/DesktopUpdates.ts:902 autoInstallOnAppQuit is set to true and never reset. Upstream sets it to false only once, at configure (DesktopUpdates.ts:1184). setUpdateChannel (DesktopUpdates.ts:1247-1270) resets the update state, and turning the autoUpdate setting off changes nothing either, while electron-updater keeps the downloaded file. Inputs: auto-download of track A; the user switches to track B (or raises the release floor), or turns autoUpdate off; then quits -> the old download installs anyway. Upstream's own invariant at DesktopUpdates.ts:1251 says "a retained offer below the new floor, which must not install". (medium: electron-updater's quit handler traced from its docs, not run)
    finding: major apps/server/src/cloud/autoUpdate.test.ts (whole file) The PR's main safety guards are untested. M1-M4 above (the installing-stage re-check, the rolled-back read from service-state.json, the 'waiting' status, the bootService guard) can each be removed with 16/16 passing. install() and tick() have no test at all, so the busy -> wait, failed -> 6 h backoff and handed-off paths are unproven. (high, mutation runs)
    finding: minor apps/server/src/cloud/autoUpdate.ts:233,356-359 status "failed" (any failed trial, including a transient prepared-deadline timeout on a loaded host) is skipped until a newer release appears, and the log says "a release that was rolled back". (high)
    finding: minor apps/server/src/cloud/autoUpdate.ts:399-416 with selfUpdate.ts:198 When the Update button already holds inFlight, the auto attempt logs the warning "Automatic server update failed; it retries later" and backs off 6 h. Nothing failed: the user's own update is running. (high)
    finding: minor apps/server/src/cloud/autoUpdate.ts:237 An unreadable or unparsable service-state.json silently makes rolledBackVersion undefined, so a rolled-back release is offered again with no log line. The launcher refuses to boot on invalid state, so this is mostly a read race. (medium)
    finding: minor apps/server/src/cloud/autoUpdate.ts:165 Work outside the DB is not counted: integrated terminals with a running subprocess (terminal/Manager.ts:241 hasRunningSubprocess) and project setup scripts. Whether a PTY child outlives the server (survivesRestart slice, Manager.ts:2378) is not verified. (low)

…ty, desktop record

Server:
- WorkAdmission gate in front of outbox claims and worktree preparation.
  The last check at the self-update's installing stage reads the setting,
  runs the quiet hook, closes the gate, counts admitted work, then reads
  thread state. The gate stays closed until exit; it opens again on a
  rejected handoff or when the launcher drops the pending record.
- Activity comes from upstream's runtime recovery projections and
  derivePendingBackgroundWork, so every provider's background work counts,
  plus unheld queued runs and running terminal commands.
- A failed launcher record backs off once instead of skipping forever; an
  unreadable state file stops the update with a warning; the Update
  button's update running first retries after a minute.
- makeAutoUpdater takes its dependencies, and the loop is tested against
  a fake server.

Desktop:
- Arming install-on-quit writes the install record, so an install that
  did not apply is reported; that version is not armed again
  automatically.
- Install-on-quit is switched off when the setting is off or the track
  changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scuttletoby

Copy link
Copy Markdown
Owner Author

fix round 1: every major from DI-1 and DI-2 is fixed at 5280dbc (was 9df6fa4). Each line below is finding → fix → test. Each named test fails when its guard is removed; the evidence is at the end.

Majors

  1. Restart gap. The last check was not the handoff (DI-1 fork: the server and CLI update from the desktop app's repository #1, DI-2 fork: the server and CLI update from the desktop app's repository #1).
    • Fix: a new server-wide gate, WorkAdmission (apps/server/src/workAdmission.ts). It wraps the outbox daemon's claims (EffectWorker.runDaemonWithOptions) and worktree preparation (ThreadLaunchService.schedulePreparation).
    • At the self-update's installing stage, the last check runs in this order: setting → quiet hook → close the gate → count the work already admitted → thread state.
    • Not quiet → the gate opens and the updater goes back to waiting. Accepted → the gate stays closed until exit.
    • Every tick the updater reads the launcher state file. If the pending record is gone while this process lives, the gate opens and the updater backs off. A rejected or failed handoff also opens the gate (onError).
    • Tests:
      • updates when quiet, asking the hook first and thread state last, behind the gate
      • holds new work from the last check until exit
      • re-checks thread state right before the handoff and waits if work started
      • re-asks the hook right before the handoff
      • counts work that was admitted before the gate closed
      • opens the gate again when the launcher rejects the handoff
      • opens the gate again when the launcher drops the handoff and the server lives on
      • WorkAdmission > stops the outbox daemon from claiming effects while held
  2. Activity check missed work that upstream counts (DI-2 fork: release workflow for v<upstream>-fork.N on GitHub-hosted runners #2, DI-1 minor).
    • Fix: the SQL query is gone. Activity now comes from upstream's own reads: getRecoveryThreadIds("runtime") and getRuntimeRecoveryProjection, and background work comes from upstream's derivePendingBackgroundWork + latestUnheldRun. So live command_execution / dynamic_tool / subagent turn items count for every provider.
    • Unheld queued runs now count as busy. Held queues do not.
    • Tests:
      • threadActivity > counts background work the way upstream does, for every provider (a Codex-style turn item with no roster)
      • counts unheld queued runs, not held ones
      • counts waiting, preparing, starting and running runs as busy
  3. Main guards had no tests (DI-1 fork: release workflow for v<upstream>-fork.N on GitHub-hosted runners #2, DI-2 Auto-update: server updates itself when quiet, desktop installs on quit #5).
    • Fix: makeAutoUpdater(deps) takes its dependencies as input, and autoUpdate.test.ts now runs the real loop (tick / install) against a fake server.
    • Tests:
      • re-check: see 1
      • rolled-back and failed records: skips a rolled-back release and backs off from a failed one, plus the decideRelease cases
      • waiting runs: see 2
      • background-service-only: runs only for a background service with an enforcing gate
      • the main loop: every auto-update loop test
  4. Desktop: a failed install on quit was silent and looped (DI-2 fork: CI check that refuses database schema and migration changes #3).
    • Fix: arming install-on-quit now writes the same install record as the Update button (writeInstallRecord). The existing startup check reports an install that did not apply.
    • A version that did not apply is not downloaded and armed again automatically; the Update button still offers it.
    • Tests:
      • DesktopAutoInstall.test.ts > records an install on quit like the Update button does
      • leaves a version that did not apply to the Update button
  5. Desktop: install-on-quit was never switched off (DI-2 fork: follow each stable upstream release with the patch series #4, DI-1 nit).
    • Fix: disarmAutoInstall (setAutoInstallOnAppQuit(false) + removeInstallRecord) runs when the setting is off at a poll, and in setChannel before the track is saved.
    • Tests:
      • stops installing on quit when the setting is turned off
      • stops installing on quit when the track changes

Minors

  • Update button first was logged as failed, then a 6 h wait. The refusal comes before any stage, so it now counts as not-started: the updater logs that another update is running and retries a minute later. Test: waits a minute, not six hours, when the Update button's update is running.
  • Transient failed was skipped forever and logged as "rolled back". It now backs off once for that update id, then offers the release again, and the log says failed. Rolled-back stays skipped. Test: decideRelease > backs off once from a failed attempt, then offers the release again.
  • Unreadable launcher state was dropped silently. It now stops the update, with a warning once per reason. Tests: does not update when the launcher state cannot be read and reads rolled-back and failed records, and reports a file it cannot use.
  • Terminal commands were not counted. Integrated terminals with a running command now count, from TerminalManager.subscribeMetadata. This is covered by the activityReason test only, not end-to-end.
  • DI-1 nit: setting turned off during staging. The last check now re-reads the setting. Test: stops at the handoff when the setting was turned off while staging.
  • Launcher already has an update pending. No offer until it is gone. Test: decideRelease > does not offer while the launcher has an update pending.

Still open, with reasons

  • Held work at the restart. Held outbox effects stay in the database, and upstream startup recovery handles them. That recovery cancels held scheduled-task, notification and delegated-completion runs, and a run held in preparing fails and can be retried. None of these is killed part-way, but they are not run late either. Changing that means editing upstream recovery, a hot file.
  • PR watches are not counted. They are polling, not work a restart interrupts.
  • Link requests are no longer counted. Recovery leaves link cards in place, and an ignored one would block updates forever.
  • The quiet hook runs twice per attempt, once at the quiet check and once at the handoff.
  • DI-1 nit: the "give-up" naming. Not changed. The log already says it retries at the next check.
  • Desktop: an Update-button install that fails while install-on-quit is armed. Upstream's recovery removes the install record, so a later failed quit install would go unreported. Fixing it means editing upstream's install-recovery path.
  • Desktop crash while armed. If the app crashes instead of quitting, the next start reports an install that did not apply, although none was tried.

Guard removal (each removed alone, then restored)

At least one test goes red for every guard:

Guard removed Red tests
re-check of thread state at the handoff 2
gate.hold 4
hook re-ask 1
setting re-read 1
rolled-back skip 2
failed back-off 2
launcher-busy 1
waiting status 1
unheld queued 1
derivePendingBackgroundWork 1
bootService guard 1
enforced-gate guard 1
onError release 1
handoff-watch release 1
not-started vs failed 1
unreadable-state stop 1
main-loop probe 6
daemon admit 1
gate in-flight count 2

Red on 9df6fa4:

  • The new server tests fail: 29 fail, because decideRelease / makeAutoUpdater do not exist there.
  • With 9df6fa4's DesktopUpdates.ts, 4 of the 5 new desktop tests fail. The 5th, "never arms without the setting", is a baseline and passes on both.

Local runs at 5280dbc (fork CI does not run tests):

  • Typecheck: server, desktop and contracts pass.
  • Lint and format: pass on every changed file.
  • Tests:
    • autoUpdate.test.ts + workAdmission.test.ts: 38/38.
    • DesktopAutoInstall.test.ts + DesktopUpdates.test.ts: 48/48.
    • Full desktop suite: 1055 passed, 10 skipped. One suite fails here for lack of the system libsecret-1 dev package.
    • Server, affected areas (all of orchestration-v2/ and cloud/, plus serverSettings, serverRuntimeStartup, serviceLauncher, ws and terminal/Manager): 150 files, 2955 passed, 1 failed. The failure is ProjectionStore.test.ts "reads a fixed SQL turn-item window": it calls StatementSync.stat, which this machine's Node v24.21.0 does not have. That file is untouched.
    • The whole server suite did not finish within 25 minutes here, so there is no full-suite result.

Unproven:

  • No end-to-end run against a real launcher.
  • No real electron-updater install on quit.
  • ThreadLaunchService's use of the gate has no unit test of its own.

@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: correctness+concurrency DI-1 r2 · reviewer: opus high (t3lens-5-di1-r2) · read at: 5280dbc

covered: fix diff 9df6fa4..5280dbc, all 10 files; non-test code line by line: workAdmission.ts, autoUpdate.ts (whole file), EffectWorker/ThreadLaunchService/server.ts hunks, DesktopUpdates.ts. Read what it calls into: EffectWorker.runOnce/drain/runRecoveryOnce and their callers, ThreadLaunchService.schedulePreparation/prepareRun, selfUpdate.update (stage callbacks, inFlight), serviceLauncherClient.exchange (30 s timeout), serviceLauncher #handleUpdateRequest/#beginTrial/startOnlineDatabaseBackup/writeServiceState, launcherWatch, ProviderRuntimeRecoveryService.reconcileProjection, RestartContinuation.restartContinuationRun, EffectOutbox process-loss sets, ProjectionStore getRecoveryThreadIds("runtime"), IdleCompactionService, UsageLimitRecoveryWorker, ScheduledTaskService.runTask, the desktop install-record path (updateMachine.resolveNotAppliedInstallVersion). Tests: workAdmission.test.ts, autoUpdate.test.ts, DesktopAutoInstall.test.ts, read for what they prove.

checks:

  • Round-1 majors. Restart gap: fixed. holdIfQuiet (autoUpdate.ts:347-361) runs setting, then hook, then gate.hold, then in-flight count, then thread state, inside selfUpdate's "installing" callback, right before launcher.requestUpdate (selfUpdate.ts:308-311). A callback failure propagates (reportProgress is yielded directly), so "not quiet" really stops the handoff. Non-Claude background work: fixed (derivePendingBackgroundWork + latestUnheldRun over getRuntimeRecoveryProjection). Guards untested: mostly fixed, with one gap (finding 1). Desktop silent loop: fixed (install record at arm time, and a notApplied version is not re-armed). Install-on-quit never switched off: fixed (disarm at the poll and in setChannel). Minors: Update-button race, transient failure, unreadable state file and terminal commands are each fixed as described.
  • The gate itself (workAdmission.ts). The held check and the inFlight++ happen in one Ref.modify, and the latch loop re-checks after every wake. So nothing admitted after hold() goes uncounted, and nothing starts after hold(). hold, release and watchHandoff all run on the single updater tick fiber, so a release->hold interleaving (latch open while held, which would spin) cannot happen. Wiring: server.ts:865-867 builds one gate and provides it at server.ts:1225, above layerRuntimeServices, which holds RuntimeLayer (ThreadLaunchService), the effect-worker startup phase and ServerAutoUpdate.layer. All three get the same enforced gate today.
  • Entry points that start work. The outbox daemon is the only consumer of OrchestrationEffectWorkerV2 (runOnce). drain and runRecoveryOnce have no production callers; the other ".drain" calls belong to separate PR/relay/settlement workers. Every provider-turn.start, steer, restart and respond goes through the outbox, so it is gated. These all dispatch into the outbox: user messages, idle compaction, usage-limit auto-resume, MCP t3_thread_send, delegated completions. ThreadLaunchService.launch (scheduled tasks, delegate_task, remote delegation) gates its worktree/setup preparation, and its turn start goes through the outbox. Not gated: integrated-terminal commands (counted at the check, but new ones can start in the window), git/preview/project-action RPCs, PR watches. All of these are interactive or excluded on purpose, so I accept them. Claude/Codex sessions starting a turn by themselves need live background work, which the check counts.
  • Gate stuck closed? Launcher rejects -> onError release (tested). requestUpdate times out -> release (see finding 3). Server crashes or restarts without updating -> the gate lives in memory, so a new process starts open. Launcher drops the pending record -> watchHandoff releases on the next tick (tested). Launcher dies -> launcherWatch shuts the server down (not on Windows). Exceptions in tick -> caught; the gate state does not change. A pending record that never resolves (launcher hung) -> the gate stays closed with no upper bound (finding 3).
  • State question (rerun, half-applied, legacy, empty). Rerun after a failure: backedOffFailureId is per process. After a restart a "failed" record gets one more 6 h back-off, then a retry. Rolled-back stays skipped. Half-applied, with the gate closed and the server killed: the held work never started, and upstream recovery then holds or cancels it (finding 2). Update applied but process not restarted: watchHandoff keeps holding while the record is pending, and releases otherwise. After that release, selfUpdate.inFlight is never reset (finding 3). Old/legacy state file: parseServiceState failure -> "unreadable" -> no update (tested). During the handed-off phase, though, "unreadable" counts as "dropped" (finding 3). The launcher's state writes are atomic (tmp + fsync + rename). Empty history: no record -> offer. No threads -> activity null -> update. Two servers on one DB: blocked upstream by the state-dir ownership lock. The gate is per process, which is correct there.
  • Hardest case: work that arrives between the final check and SIGTERM. On this host's ext4 (no reflink) the launcher copies the whole DB file online before SIGTERM (serviceLauncher.ts:304-312), so that window is 2 s + a full DB copy + fsync, not a moment. Nothing in it starts (good), but see finding 2 for what happens to it.
  • Red->green: new server tests on 9df6fa4 source -> 29 red, including "stops the outbox daemon from claiming effects while held". New desktop tests on 9df6fa4's DesktopUpdates.ts -> 4/5 red. At 5280dbc: server 38/38, desktop src/updates 98/98.
  • Guards broken by hand (each restored after): EffectWorker admit removed -> 1 red. Ignore the in-flight count -> 1 red. onError release removed -> 1 red. watchHandoff release removed -> 1 red. Read thread state BEFORE gate.hold -> 0 red (finding 1). Drop the held check inside admit -> 0 red (only a race guard, which the latch covers in tests; acceptable). Drop the release-when-busy -> 0 red (redundant with onError; fine).
  • Author's known gaps: read after I formed this view. "Held work at the restart" is worse than stated (finding 2). "Desktop crash while armed" is somewhat worse than stated (finding 5).

finding: minor apps/server/src/cloud/autoUpdate.test.ts:406 (guard: autoUpdate.ts:354-358). This ordering is the restart-gap fix: thread state is read after gate.hold. Moving the deps.activity read before deps.gate.hold leaves all 38 tests green. The call log has no "hold" entry, and the fake's activity cannot change between the two reads. The fix comment's "gate.hold removed -> 4 red" covers removing the hold, not reordering it. Add a test where work is admitted, and thread state turns busy, between a pre-hold read and the hold (e.g. note("hold") in a wrapped gate, plus an activity fake that flips once the gate is held). (high)

finding: minor apps/server/src/workAdmission.ts:1-3 + autoUpdate.ts:3-4 vs ProviderRuntimeRecoveryService.ts:331-443. "Held work waits in the database for startup recovery" is not what happens. A run held at the gate sits at status starting with startedAt null. At restart, user messages become a held queue that the user has to release. Every createdBy "agent" message is cancelled: MCP t3_thread_send, delegate follow-ups, project/worktree MCP sends, scheduled tasks, delegated completions, notifications. Only usage-limit resumes are re-sent. This is worse than "still open" states in two ways: (a) it covers every agent-created message, not just the scheduled-task/notification/delegated-completion kinds; (b) on a non-reflink filesystem (ext4 here) the window lasts as long as the launcher's online DB copy (serviceLauncher.ts:304-312), which can be minutes for a large DB. Still no kill part-way, and better than upstream's Update button (which kills mid-turn), hence minor. At least fix the two comments, and say in the PR that messages sent during an update are held or cancelled. (high)

finding: minor apps/server/src/cloud/autoUpdate.ts:386 + 462-471. Gate releases do not follow what the launcher actually does. (a) watchHandoff treats an "unreadable" record (a transient read error) as "dropped", reopens the gate and backs off 6 h, while the launcher's pending update still SIGTERMs the server. (b) requestUpdate's 30 s IPC timeout (serviceLauncherClient.ts:191) -> onError release + "failed" 6 h back-off, but the launcher can still write pending and restart after a late accept. Admitted work is then killed. (c) After a real drop, selfUpdate's inFlight is reset only onError (selfUpdate.ts:333). So every later auto attempt, and the Update button, gets "already in progress" until the process restarts. (d) A pending record that never resolves (launcher hung in the copy) keeps every message waiting, with no limit and nothing in the UI. Suggest: keep holding on "unreadable" or a timeout until the record shows a terminal status, cap the hold with a logged release, and reset inFlight when watchHandoff releases. Each case is unlikely. (medium)

finding: minor apps/server/src/orchestration-v2/ThreadLaunchService.ts:823. No test covers the gate on worktree/setup preparation (the author acknowledges this; I did not re-run that mutation). Nothing tests that the production layer graph gives EffectWorker and ThreadLaunchService the same enforced gate as autoUpdate. WorkAdmission is a Context.Reference that defaults to pass-through, and autoUpdate checks enforced only on its own instance, so a wiring slip would update over running work without any error. The wiring is correct today (server.ts:1225). (high that the gap exists)

finding: minor apps/desktop/src/updates/DesktopUpdates.ts:930 + 62. The install record is written when install-on-quit is armed, not when an install starts. So any restart of the old version while armed counts as "did not apply": crash, force-quit or kill, power loss, an OS reboot that skips electron-updater's quit hook. The UI then reports a failed install that was never tried, and that release is left to the Update button until a newer one ships. That is broader than "crash" in "still open". Also, disarm runs only at the 4-minute poll, so turning the setting off and quitting within 4 minutes still installs. (high)

@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: correctness+concurrency DI-2 r2 · reviewer: opus high (t3lens-5-di2-r2) · read at: 5280dbc

covered: the whole fix diff 9df6fa4..5280dbc. I read every non-test line in workAdmission.ts, cloud/autoUpdate.ts, cloud/selfUpdate.ts, the server.ts wiring and desktop updates/DesktopUpdates.ts. I also read the code they call into: serviceLauncherClient.ts (30 s exchange timeout), serviceLauncher.ts (HANDOFF_DELAY, #recover, pending record), ProviderRuntimeRecoveryService (startup and shutdown reconcile), ThreadLaunchService, EffectWorker.runDaemon and serverRuntimeStartup. Tests: at head, server autoUpdate + workAdmission 38/38 pass and desktop updates 98/98 pass. At base 9df6fa4 with the new tests, 30 server and 4 desktop tests fail, so they go red to green. Hand mutations killed by the tests: gate.hold swapped for inFlight (3 red); the onError release removed; the watchHandoff release removed; the daemon admit removed; the background-work count removed; desktop guards D1-D5 each broken. Mutations that survived: M4 (read thread state before gate.hold), M7 (ThreadLaunchService admit removed, 84 tests still pass), G1 (terminal count forced to 0), G2 (no latch close in hold: correct but spins), M5 (no release-when-busy: harmless, because onError releases anyway). Wiring: Layer.provide(Layer.merge(layerActivation, layerWorkAdmission)) in server.ts covers the routes (ServerAutoUpdate), the runtime services (orchestration, ThreadLaunchService, the EffectWorker daemon forked from serverRuntimeStartup) and McpSessionRegistry. Nothing that admits work sits in the layers below it. I checked this by reading only, not at runtime. WorkAdmission is a Context.Reference with a pass-through default, so a layer that misses the provide fails open silently. Round-1 majors: restart gap fixed (hold, then last activity read, then handoff; the gate stays closed until exit); non-Claude background work now counted; guards tested; desktop failed install-on-quit now reported, not looped; install-on-quit now disarmed. Round-1 minors: the Update-button race is fixed by the inFlight getAndSet; a transient failure now leads to afterFailure and a retry; an unreadable state file stops with one warning; terminal commands are counted.

checks: state question. Rerun: decideRelease plus the launcher record make a second check idempotent; a pending record for the same version keeps the hold, anything else releases. Stops half-way: if the process crashes after the gate closes, the new process starts with a fresh, open gate; the launcher's #recover resumes or finishes the pending update; held work is still in the DB, but see finding 1 for what recovery does with it. Legacy or unreadable state file: stops and warns once, with no loop. Empty history: launcher record NotFound is read as none, which leads to release and afterFailure. Update applied but process not restarted: watchHandoff sees a record that is no longer pending and releases, with a warning. Two servers on one DB: out of scope for this PR (each process has its own gate, so the gate does not protect the other one). Gate stuck closed forever: launcher rejection, client error and hook/activity failure all release through onError; a launcher drop is released by watchHandoff. The one unbounded case is a launcher that wedges with the record left pending (finding 4). Hardest case: work that arrives during the hold window. It cannot start, which is correct. What happens to it afterwards is worse than the comments say (finding 1).
finding: minor apps/server/src/workAdmission.ts:1-3 (and autoUpdate.ts:354 holdIfQuiet) work that arrives between gate.hold and SIGTERM -> it does not "wait in the database for startup recovery". The startup/shutdown reconcile in ProviderRuntimeRecoveryService cancels unstarted scheduled-task, delegated and notification runs. User messages become a held queue that the user must release by hand. Worktree preparations get failPreparedRun "Worktree setup cancelled." when preparationScope is interrupted at shutdown. The window is also longer than a moment: it runs from the hold through the launcher's 2 s HANDOFF_DELAY and the online DB backup (startOnlineDatabaseBackup copies the whole DB file while the server runs) until SIGTERM. The fix-round-1 comment names the cancellation but not how long the window is. Fix the comment, or re-admit held work after restart (medium)
finding: minor apps/server/src/cloud/autoUpdate.ts:386 the launcher accepts the handoff, but the client then hits its 30 s exchange timeout or loses the connection (serviceLauncherClient) -> onError opens the gate and sets afterFailure while the launcher still writes the pending record and kills the child about 2 s later, so work starts and is then killed: the restart gap reopens. Fix: treat a client error that is not an explicit rejection as "unknown", stay held, and go to watchHandoff so the record decides (low likelihood; mechanism medium)
finding: minor apps/server/src/cloud/selfUpdate.ts:198 (inFlight set at 198, reset only onError at 329) the handoff succeeds, then watchHandoff releases because the launcher dropped it -> inFlight stays true, so every later automatic and manual update is refused with "already in progress"; the auto loop then waits about 2 h of every 3 h, runs the hook every minute and logs a misleading "another update is running", until the process restarts. Reset inFlight in watchHandoff's release path (low)
finding: minor apps/server/src/cloud/autoUpdate.ts:462 watchHandoff: the launcher wedges or never restarts the child while its record stays "pending" for this version -> the gate stays closed forever with no time limit and no visible state: every outbox effect and worktree preparation is held silently. Add a deadline (for example HANDOFF_DELAY plus backup time plus a margin) after which it releases and warns, and show "held for update" in the UI/status (low)
finding: minor apps/server/src/cloud/autoUpdate.ts:354 + tests: M4 (thread/activity state read before gate.hold, the exact round-1 restart-gap order) survives, because the test checks only the call order and not that the read happens behind the hold. M7 (ThreadLaunchService admit removed) and G1 (terminal count forced to 0) also survive: the make() activity wiring and terminal counting have no tests. A regression could reintroduce the restart gap with CI green (high)
finding: minor apps/desktop/src/updates/DesktopUpdates.ts:897/956 disarmAutoInstall runs only from the 4-minute poll -> the user turns the setting off and quits within 4 minutes, and the update still installs on quit. Disarm when the setting changes. A crash while armed also gives a false "did not apply" on the next start; the author already states this (medium)
note: terminals, project clones and other work the user starts directly do not go through the gate after the final check. That is intentional for user-driven work, but it is not counted after the hold either.

…desktop record at quit

Held work:
- The WorkAdmission gate writes a marker (runtime/work-held-for-update.json) when an
  update holds new work. Startup recovery reads a fresh marker and starts runs and worktree
  preparations requested since then, with queued runs behind them, instead of cancelling
  them or leaving them in the held queue. Without a marker upstream recovery is unchanged.
- Outbox, thread launch and recovery register on the gate; the updater refuses loudly on the
  pass-through gate or when a consumer is missing. A launch waiting at a closed gate no
  longer fails the run.

Gate vs launcher:
- The gate closes before the last thread-state read. After the launcher was asked, a lost
  answer or an unreadable record keeps the hold; it opens only when the record shows no
  restart (after a grace period for an unconfirmed handoff) or after 30 minutes, with a
  warning, and the self-update's in-progress flag is reset.

Desktop:
- The install record is written when the app quits, not when install-on-quit is armed, and
  the autoUpdate setting is read again at quit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scuttletoby

Copy link
Copy Markdown
Owner Author

fix round 2: 5280dbc..5164fea answers DI-1 (6823167624) and DI-2 (6823190929). The PR body is updated; it now includes the window estimate.

Held work (DI-1 finding 2, DI-2 finding 1). Work held at the gate now starts after the restart. It is not cancelled and does not land in a held queue.

  • The gate writes a marker, <baseDir>/runtime/work-held-for-update.json, holding the time the hold began. A failed write counts as "not now". Release removes the marker.
  • While the gate is closed, the outbox daemon claims nothing, so the rows stay pending. A worktree preparation now waits at the gate instead of failing its run (ThreadLaunchService).
  • At startup, ProviderRuntimeRecoveryService reads the marker if it is under 2 h old. It keeps unstarted starting runs and preparing runs requested at or after the marker's time out of the cancel and hold paths:
    • each start gets a fresh provider-turn.start;
    • each preparation runs again;
    • queued runs behind them on the same thread start as usual.
  • Without a marker, and for older work, held queues and buffered notifications, upstream's behaviour is unchanged.
  • The marker is cleared after the restart once recovery has read it and the launcher's record is no longer pending. While a trial is pending it stays, so a rollback's recovery sees it too.
  • The code comments in workAdmission.ts and autoUpdate.ts now say this.
  • Window, from the code (startOnlineDatabaseBackup copies the main file and fsyncs it before SIGTERM): 2 s + main copy + up to 5 s stop + WAL copy + trial startup. A synthetic 1 GiB copy + fsync on SATA ext4 took 1.4 / 5.8 / 5.1 s, so roughly 10–20 s plus startup for a 1 GiB database. Details are in the PR body.

Gate vs launcher (DI-1 finding 3; DI-2 findings 2, 3, 4).

  • (a) An unreadable record during the handoff keeps the hold and warns once.
  • (b) Only a ServiceLauncherRejectedError releases the gate. A lost answer (timeout, broken connection) becomes an unconfirmed handoff. A pending record for this version confirms it. No pending record releases it, but only after a 5-minute grace period.
  • (c) ServerSelfUpdate.resetAfterDroppedHandoff clears inFlight whenever a handoff is dropped.
  • (d) AUTO_UPDATE_MAX_HOLD_MS is 30 minutes. After it, the gate opens with a warning naming the reason, and the updater backs off.
  • Visible status is the log only: the hold start is logged with the target version and the limit, and the release with its reason. There is no auto-update UI or health surface to put it in, and I did not add one.

Tests (DI-1 findings 1 and 4, DI-2 finding 5). These were the surviving mutations:

  • M4, the read before the hold: the test now flips activity once the gate is held, and fails when the read moves before gate.hold.
  • M7, ThreadLaunchService admit: there is a new test for a preparation waiting at a closed gate.
  • G1, the terminal count: runningTerminalCommands is exported and tested.

Wiring:

  • Outbox, thread launch and recovery each register on the gate.
  • The updater refuses, with logError, on the pass-through gate or when any consumer is missing, so a wiring slip fails loudly.
  • runtimeLayer.test.ts runs a DB-backed simulated restart through the real runtime layer, with and without a marker, and checks that recovery registered on the shared gate.

Held-work tests in ProviderRuntimeRecoveryService.test.ts: a user message, a scheduled run, a t3_thread_send, a delegated completion and a worktree preparation that arrive while the gate is closed are each started after the restart.

Desktop (DI-1 finding 5, DI-2 finding 6).

  • The install record is no longer written when install-on-quit is armed. A finalizer on the updater's scope writes it as the app quits, before electron-updater's quit handler installs. A crash or kill before a real quit leaves no record, so there is no false "did not apply" and that version is not held back.
  • The same quit step reads autoUpdate again. If it is off, install-on-quit is switched off and nothing installs, closing the window up to the next poll.
  • Disarming no longer removes the record, since there is none to remove.

Red / green.

  • On 5280dbc's sources, the new tests fail: 35 server and 3 desktop. Some of the server failures are API changes.
  • 14 hand mutations on the new head each turn at least one test red:
Mutation Red
read before hold 1
ThreadLaunch admit 1
terminal count 0 1
recovery ignores marker 6
recovery doesn't start held work 6
missing-consumer refusal 1
unreadable releases 1
no max hold 1
no inFlight reset 3
outbox admit 1
recovery not registered 2
desktop: no quit step 2
desktop: record at arm 5
desktop: setting not re-read 1
  • Green at 5164fea:
    • typecheck is clean for server and desktop;
    • fmt, lint and knip pass (pre-commit);
    • desktop src/updates + src/app: 230/230;
    • server src/orchestration-v2: 2182 + 317;
    • src/cloud plus every other test file importing a changed module: 596;
    • one failure: ProjectionStore.test.ts, which this PR does not touch, needs StatementSync.stat and this machine's Node is 24.21.

Still unproven.

  • No run against a real launcher, and no real electron-updater install.
  • The restart is simulated. The tests show the state after recovery, not what a live daemon's reactors do with the cancelled old start effect.
  • After a rollback, the previous version starts the held work again, so a turn the trial already started could start twice. Not tested.
  • How recoverDelegatedTasks treats a child still preparing was not checked.
  • Workers that drain work outside the outbox and thread launch are not gated.
  • Users' own terminals and clones are not gated after the last check. This is intended.
  • A reboot that kills the app during the quit step, after the record but before the install, would still report "did not apply".
  • The window is an estimate from a synthetic copy.

@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: data-integrity DI-1 r3 · reviewer: opus high (t3lens-5-di1-r3) · read at: 5164fea
covered: fix diff 5280dbc..5164fea in full: workAdmission.ts, autoUpdate.ts (hold, marker, handoff watch, settleMarker, max hold), ProviderRuntimeRecoveryService.ts (marker read, heldSinceUpdate, startHeldWork, reconcileAfterProcessLoss), ThreadLaunchService.ts (admit / admitted guard / preparation resumer), outbox daemon, serverRuntimeStartup.ts phase order, ServerSelfUpdate handoff + resetAfterDroppedHandoff, desktop DesktopAutoInstall finalizer. Read into: Orchestrator message.dispatch / queue.resume / queued-run.start / startNextQueuedRun / prepared-run.retry, EffectWorker -> ProviderTurnStartService, PROCESS_BOUND_EFFECT_TYPES, scheduled tasks via ThreadLaunchService.launch, other drain callers.
checks:

  • red/green: the new tests at 5280dbc's code: 35 server + 3 desktop red; at head the 5 touched server files (248) + DesktopAutoInstall (7) are green.
  • hand breaks (reverted each): caught: requestedAt filter, outbox register, thread-launch register, recovery register, settleMarker pending check, marker age check, lost answer treated as reject, 30-min max hold, release clearing the marker. NOT caught: (B1) dropping the admitted guard in ThreadLaunchService.ts:829 (a preparation held at shutdown is failed "Worktree setup cancelled" instead of left preparing; this is the round-2 fix itself); (B4) dropping ThreadLaunchService.ts:1177 setPreparationResumer(prepareRun) (held preparations are only logged as an error after the restart); (B11) running startHeldWork on the shutdown trigger too (harmless: the restart cancels the process-bound effects).
  • double start, normal update: no. reconcileAfterProcessLoss cancels pending provider-turn.start (process-bound) before startHeldWork writes fresh ones; the outbox daemon is the only claimer of the orchestration outbox, and turn starts only go EffectWorker -> ProviderTurnStartService, so the old server cannot also start them. Recovery running twice: startHeldWork runs only on the startup trigger. A second startup re-starts only runs still in starting/preparing, after cancelling the first start effect; a crash in the gap between a provider accepting the turn and the run leaving "starting" could repeat it, but that is the same as upstream (not checked further).
  • gate entry points: scheduled tasks go through ThreadLaunchService.launch; delegated tasks / t3_thread_send / resume after limit end in message.dispatch -> outbox, so they are gated. The other drain callers use their own workers and do not start turns. Terminals not gated after the last check (intended, as stated).
  • gate never stuck: release on explicit reject, 5-min grace on no pending record, 30-min cap reachable because selfUpdate.update returns after the launcher accepts, and watchHandoff runs. Restart without update: the marker is read only if under 2 h old, and settleMarker clears it once the launcher's record is not pending.
  • desktop: finishAutoInstallAtQuit is added after the markComplete finalizer, so it runs first and before quit is allowed. autoUpdate is re-read at quit. OK.
  • STATE question (runs again / stops half-way / old or empty state): rerun of recovery = idempotent (above). Empty marker / no marker / marker > 2 h = upstream path, unchanged. Old version's state file: an old server ignores the marker, so the upstream path applies. Half-way: (a) a crash after hold but before the marker write = "not now", no held work; (b) startup recovery failing after it read the marker still lets settleMarker clear it (finding 5); (c) the TRIAL being rolled back after it already started held work = the hardest case, finding 1.
  • hardest case, rollback: serverRuntimeStartup.ts runs recover (startHeldWork) -> recoverDelegatedTasks -> startEffectWorker (:545-552), then http.wait / auxiliary-roots.parked (:637-638), and only then launcher.prepareTrial (:640), the commit. The trial therefore sends held prompts to providers before it is committed. If it then fails (exit, prepare timeout, later phase error), the launcher restores the DB backup taken at the stop and starts the old version. autoUpdate.ts:586-598 keeps the marker on purpose while the trial is pending, and it is under 2 h old, so the old version's recovery (if it carries this PR) starts the same runs again.
    finding: major apps/server/src/cloud/autoUpdate.ts:595 + apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.ts:1046 + apps/server/src/serverRuntimeStartup.ts:640: a trial that started held work and then rolls back -> the restored DB still shows those runs unstarted, the marker is kept for the rollback, and the previous version starts them again: the provider gets the same prompt twice, and the first attempt's worktree / file changes stay on disk under the second. This happens whenever a rollback follows a trial that got past startEffectWorker, not by chance. Fix idea: on a trial, defer startHeldWork until prepareTrial reports committed (or have the trial record which held runs it started and drop the marker for them). Upstream restart continuations share this class of problem. (medium-high)
    finding: minor apps/server/src/orchestration-v2/Orchestrator.ts:6640/7023 vs apps/server/src/cloud/autoUpdate.ts:368: requestedAt is DateTime.now at the start of dispatch, and the commit comes later (EventSink transaction, writer lane). A message stamped just before heldAt and committed after the activity read is not counted by the quiet check and has requestedAt < heldAt, so after the restart it takes the upstream path (held queue / cancelled), and the user must act again, against the round-2 requirement. Fix idea: write heldAt with a margin (e.g. heldAt minus a few seconds). This is safe, because any run committed before the activity read already blocks the quiet check. (medium)
    finding: minor apps/server/src/orchestration-v2/Orchestrator.ts:2420-2426 (startNextQueuedRun) + ProviderRuntimeRecoveryService.ts:374-379: queued-run.start / queue.resume during the hold turn an old queued run into "starting" with its old requestedAt, and a gated provider-turn.start. After the restart that start is cancelled, and heldSinceUpdate rejects the run (requestedAt < heldAt), so the user's "send now" / resume is undone and needs repeating. prepared-run.retry keeps the run's requestedAt the same way. Fix idea: treat a run whose status changed to starting/preparing after heldAt as held (e.g. a startRequestedAt or the event time), not its first requestedAt. (medium)
    finding: minor apps/server/src/orchestration-v2/ThreadLaunchService.ts:829 and :1177: no test fails without the admitted guard (the round-2 "preparation waits, is not failed" fix) or without setPreparationResumer(prepareRun). Both regressions would ship silently. (high)
    finding: minor apps/server/src/cloud/autoUpdate.ts:588-598: settleMarker clears the marker after any startup that read it, even when recovery failed part-way after the read. Runs left in starting/preparing are then cancelled visibly on the next restart. Also, a runId-null worktree preparation held at shutdown is not resumed (same as upstream). (low)
    author's known gaps (6824890689), compared after forming the above: "After a rollback ... could start twice. Not tested" is worse than stated. It is not a possible edge: settleMarker keeps the marker for the rollback by design, and the trial starts held work before prepareTrial, so every rollback after startEffectWorker starts it twice. The other listed gaps (no real launcher / electron-updater, restart simulated, recoverDelegatedTasks with a preparing child, ungated drains, terminals, reboot during quit) match what I saw. The drains I checked do not start turns.
    safe to merge: no, not until the rollback double start is fixed or its risk is accepted by the owner.

@scuttletoby

Copy link
Copy Markdown
Owner Author

lens: data-integrity DI-2 r3 · reviewer: opus high (t3lens-5-di2-r3) · read at: 5164fea

covered: fix diff 5280dbc..5164fea in full (workAdmission.ts, cloud/autoUpdate.ts, cloud/selfUpdate.ts, ProviderRuntimeRecoveryService.ts, ThreadLaunchService.ts, EffectWorker.ts outbox gate, server.ts / serverRuntimeStartup.ts wiring, desktop install-on-quit finalizer) plus what it calls: launcher prepareTrial / handoff record, shutdown reconcile, restartContinuationRun, recoverDelegatedTasks, Orchestrator dispatch (requestedAt), electron-updater quit path. Every round-2 point from 6823167624 and 6823190929 is fixed at the root: held work is no longer cancelled and needs no manual release (marker file + start after restart, outbox claims nothing while held); only an explicit launcher reject releases the gate, a lost answer becomes handed-off with a 5-minute grace, 30-minute cap; outbox, thread launch and recovery register on the gate and the updater refuses on a missing consumer; desktop record is written at actual quit and autoUpdate is re-read there.

checks:

  • Red/green: new tests at 5280dbc sources: 35 fail (mostly missing APIs). At head: workAdmission, autoUpdate, ProviderRuntimeRecoveryService, DesktopAutoInstall 79/79; ThreadLaunchService + runtimeLayer gate cases 6/6. Hand mutations caught: requestedAt >= heldAt removed, no activity read after hold, release keeps marker, no 30-min cap, settleMarker guards removed (x2), lost answer releases, no 5-min grace, marker age unchecked, queued runs behind a held start not started. Survived: M10 (ThreadLaunchService.ts:829 !admitted guard removed), M2 (ProviderRuntimeRecoveryService.ts:376 queueHeld !== true removed), M3 (startHeldWork also on shutdown; harmless).
  • State question ("runs again / stops half-way / existing, old or empty state"):
    • Runs again: recovery twice in the new process is fine (the first start effect is cancelled by the process-loss pass and recreated once). restartContinuationRun does not touch held runs (they are neverStarted); recoverDelegatedTasks does not start them a second time.
    • Stops half-way: crash after gate.hold but before handoff: marker stays, next start (same version) starts held work once and clears the marker; no work lost. Crash between marker write and outbox stop: the outbox only claims through admit, so nothing claimed after the hold; rows stay pending. Marker write fails -> "not now", gate does not close.
    • Old/empty state: no marker (older version, first boot, non-update restart after release) -> upstream behaviour unchanged. Marker > 2 h -> ignored as stale and upstream path; marker < 2 h after a non-update restart -> held work started, which is the intended outcome. Unreadable marker = no marker (held work then follows upstream cancel/hold: not lost, but needs a manual resend). Backwards clock jump makes the marker look stale: same outcome.
    • Two servers on one DB: not made worse by this PR; the marker is per baseDir.
    • Hardest case: a failed trial. Startup recovery starts held work (ProviderRuntimeRecoveryService.ts:1046) BEFORE launcher.prepareTrial (serverRuntimeStartup.ts:640), so the new version sends the held turns to providers before the trial is committed. If the trial then fails, the DB is restored to the stop state and the marker is kept on purpose, so the old version starts the same turns again. See finding 1.
  • Gate closed forever: no path found. Explicit reject releases; lost answer -> grace -> release; unreadable record keeps the hold but the 30-min cap applies; a restart without update clears the marker after recovery; errors, defects and interrupts in the install step release the gate (autoUpdate.ts:459); a failing tick while handed-off is caught and retried, and the 30-min cap still applies.
  • Work starting after the final check: outbox (EffectWorker), thread launch and startup recovery are gated; scheduled tasks, t3_thread_send and delegated completions dispatch through the orchestrator and land in the outbox, so they are covered. Not gated: integrated-terminal input, git/UI operations (intended, as the author says).
  • Author's known gaps (read after forming this view): all match. One is worse than stated: "after a rollback ... could start twice" is not only a deliberate rollback; because held work is started before prepareTrial, EVERY failed trial (crash, prepared-exchange timeout, PREPARED_TIMEOUT) after recovery double-starts the held turns, and provider-side work and file edits are not rolled back with the DB.

finding: minor apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.ts:1046 (with serverRuntimeStartup.ts:640) held turns are started during startup recovery before the trial is committed; trial fails after that -> launcher restores the DB, marker kept, old version starts the same turns again -> each held turn runs twice and the first run's file edits stay. Suggested fix: start held work only after prepareTrial commits (or only when no trial is pending). (high)
finding: minor apps/server/src/orchestration-v2/ThreadLaunchService.ts:829 the !admitted guard is right but untested: removing it keeps every test green; without it a worktree preparation waiting at a closed gate that is interrupted at shutdown runs failPreparedRun -> the run is marked failed and the held preparation is lost after the restart. Add a test that interrupts a held preparation. (high)
finding: minor apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.ts:376 the queueHeld !== true exclusion is untested (mutation survives): without it a run in a user-held queue would be started after an update restart. Add a test. (high)
finding: minor apps/server/src/orchestration-v2/Orchestrator.ts:7023 (also :6640) requestedAt is DateTime.now taken before the command commits; a command whose now < heldAt but that commits after the activity read is neither counted as busy nor treated as held -> after the restart a user message goes to the held queue and a scheduled/agent run is cancelled. Narrow window (ms). Fix: compare against heldAt minus a small margin (safe, since anything committed before the activity read already counts as busy). (medium)
finding: minor apps/server/src/cloud/autoUpdate.ts:552 the 30-minute cap is checked before the launcher record; while the record still shows the update pending (e.g. a long online backup on a large DB), the gate opens anyway and work that starts is then killed by the restart; the marker is already cleared at that point, so that work is not restarted either. Check the record first, cap only when it is not pending (or keep the marker). (medium)

- Startup recovery only keeps work held for an update; startup starts it
  after launcher.prepareTrial succeeds, so a rolled-back trial starts nothing.
- Held work is chosen by save order, not request time: everything unclaimed
  at the restart was saved after the hold ("send now", resume and retry keep
  their old request time).
- The marker records the runs kept by the first startup and the runs started,
  so later startups (after a rollback, a crash or a part-way failure) start
  only what is left. A failure keeps the marker.
- The updater waits for held work to settle before checking again, and keeps
  the hold past the 30-minute cap while the launcher's record is pending.
- Tests: end-to-end rollback on a file database, startup order, each guard.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scuttletoby

Copy link
Copy Markdown
Owner Author

Fix round 3

Fixes the round-3 findings (DI-1 #issuecomment-6825354751, DI-2 #issuecomment-6825370028). Range: 5164fea70..350656aa3.

  1. Held work started twice on a failed update (major). Recovery now only keeps held work. Startup starts it with startHeldWork right after launcher.prepareTrial succeeds (serverRuntimeStartup.ts). A trial that fails before the commit starts nothing, and the restored previous version starts each held run once. The marker records which runs the first startup kept (runs) and which have started (started, written before each thread's starts). So a crash after the commit, recovery running twice, or a part-way failure never starts a run twice. Tested end to end in runtimeLayer.test.ts: a file database, copied at stop and restored after the trial, with one start per run over all processes, both committed and rolled back.
  2. Hold-moment race. Held work is chosen by save order, not by request time. The gate closes before the last quiet check, and anything that check counts stops the update, so all unclaimed work at the restart was saved after the hold. The end-to-end test includes a message stamped before the marker's time.
  3. "Send now", resume and worktree retry during the hold. Covered by item 2: request time is no longer used. Recovery tests cover each.
  4. 30-minute cap during a pending handoff. The updater reads the launcher's record first. A pending record for this version keeps the hold past the cap (with one warning). An unreadable or different record past the cap releases it.
  5. Untested guards. Each now has a test that fails when the guard is removed: ThreadLaunchService.ts:829, :1177, and the user-held queue guard in recovery.
  6. A part-way recovery failure clears the marker. It now keeps the marker, and the next run starts only what is left.

The PR body has the state table (one row per write step: crash right after it, second run, old or empty state, with a test each), the hand-break table and the local runs. Server: 2512/2513 + 4726 pass; the one failure is ProjectionStore (StatementSync.stat missing in this Node). Typecheck, lint, format and knip are clean.

Not proven:

  • no real launcher;
  • an old version without this PR, after a rollback, is protected only by the 2 h marker limit;
  • a launcher record that stays pending keeps the gate closed until exit;
  • a disk failure while writing started leaves the updater waiting until restart;
  • a crash between the hold and the marker write takes the upstream path.

🤖 Generated with Claude Code

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

Labels

size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants