Skip to content

Auto-update: server updates itself when quiet - #5

Merged
scuttletoby merged 1 commit into
mainfrom
fork/auto-update
Nov 14, 2026
Merged

Auto-update: server updates itself when quiet#5
scuttletoby merged 1 commit into
mainfrom
fork/auto-update

Conversation

@scuttletoby

@scuttletoby scuttletoby commented Nov 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Why

A server that runs as the background service only updates when someone presses Update. Left alone it falls behind. This adds an opt-in updater that installs a newer release by itself, but only when nothing is working, and holds any work that arrives during the restart so it starts afterwards instead of being lost.

Upstream has the same idea in review: pingdotgg#17585 adds the automaticUpdates setting and pingdotgg#17586 uses it for server self-update. This PR now uses the same setting name, type, schema shape and place in the file, so when upstream merges, their line replaces ours and saved choices carry over.

What changed in round 4

What

One opt-in server setting, off by default: automaticUpdates (with optional automaticUpdatesQuietCommand).

  • 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.

There is no UI toggle yet. SettingsPanels.tsx changes often upstream, and editing it would conflict on almost every sync; upstream's pingdotgg#17585 adds the toggle under the same name. Users set the option in the server settings file.

Settings rename: old file, empty file, torn file

renameRetiredAutoUpdateKeys runs on the parsed file before PersistedServerSettings decodes it. Unknown keys are ignored by the decode, and writes are sparse (defaults are not saved), so:

Saved state What loads Test (autoUpdateSettings.test.ts)
Old file: autoUpdateServer: true, autoUpdateQuietCommand On, same quiet command; other settings unchanged "loads an old file's choice under the new names, then saves only those"
Next write after an old file loaded Only automaticUpdates / automaticUpdatesQuietCommand saved; old keys gone same test
Both old and new names (hand edit) The new name wins "keeps a new name already in the file over the old one"
Old key with the wrong type ("yes", 7) Dropped; it would otherwise fail the whole decode and reset every setting "drops an old value of the wrong type without resetting the other settings"
No settings file Off "stays off with no settings file"
{} Off "stays off with an empty settings object"
File cut off half-way ({ "autoUpdateServer": tr) Decode fails, defaults apply: off "stays off when the file was cut off half-way through an old key"
Not an object (null, array, number) Passed through unchanged; the decode handles it as before "passes through what is not a settings object…"
Rollback to an older fork build that only knows autoUpdateServer That build ignores automaticUpdates and reads off: fails closed (no update). The launcher does not restore settings.json. by design; not tested
Update interrupted mid-install, or runs again after a failed update The setting is not written by the updater, so it is unchanged; the launcher state file and marker rows below cover the rest state table below

Settings writes are atomic (writeFileStringAtomically), so a half-written write leaves the old file whole.

Default: off here, on upstream. The fork keeps this opt-in, as this PR always said. Upstream pingdotgg#17585 defaults automaticUpdates to true. Because a value equal to the default is not saved, a server that never turned it on has no key in its file. When upstream's line replaces ours, such servers will follow upstream's default and start updating by themselves (provider CLI updates too, which upstream ties to the same setting). Servers that turned it on stay on. Whether the fork should keep "off" after convergence (by overriding the default) is an owner decision for that sync, not this PR.

Rejected alternatives

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 automaticUpdatesQuietCommand 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"

Tests

All runs with GH_TOKEN/GITHUB_TOKEN unset and an empty GH_CONFIG_DIR, on 3ecd968:

  • The test files this PR touches (autoUpdate, autoUpdateSettings, workAdmission, serverRuntimeStartup.updateHold, ProviderRuntimeRecoveryService, ThreadLaunchService, runtimeLayer, selfUpdate): all pass (320/320 after the rebase, then autoUpdateSettings + autoUpdate 50/50 after the rename).
  • Server and desktop typecheck clean; format, lint and knip pass (pre-commit hook).
  • The full server suite was not run locally (the machine was overloaded); fork CI runs it (Test Server 1–8).

What the test files cover:

  • autoUpdateSettings.test.ts (7, new): the settings rename table above.
  • autoUpdate.test.ts: a fake-server loop around the real updater (launcher, thread state, hook, self-update, gate): hook → gate → thread state order; marker written and cleared; hold until exit; release on a rejected handoff; lost answers, grace and the 30-minute cap; in-progress reset; refusal on a pass-through or partly registered gate; terminal commands; round-1 cases.
  • workAdmission.test.ts: hold and release, in-flight count, pass-through default, outbox not claiming while held, marker freshness, consumer registration.
  • ThreadLaunchService.test.ts: a worktree preparation waits at a closed gate, then runs; one held at stop stays preparing and the next process resumes it.
  • ProviderRuntimeRecoveryService.test.ts, "work held for an automatic update": starts only after startHeldWork; request-time independence; user-held queues untouched; rollback and crash give exactly one start; recovery twice; part-way failure and retry; runs narrowing; none/stale/invalid markers.
  • serverRuntimeStartup.updateHold.test.ts: held work starts after prepareTrial, and not when it fails.
  • runtimeLayer.test.ts: simulated restarts through the real runtime layer, and end to end on a file database with commit and with rollback: each held run reaches a provider exactly once.

Guards, mutation-checked. Round 4, each a hand break on the new head with only its own tests run:

Mutation Red
rename call removed from serverSettings.ts 1 (old file loads)
"new name wins" check removed 1
type check on the old value removed 1
ThreadLaunchService admitted guard removed (:953) 1
recovery's queueHeld guard removed (ProviderRuntimeRecoveryService.ts:379) 1
startHeldWork removed from startup 1
setPreparationResumer removed (:1301) 1
startHeldWork moved before prepareTrial 2

Earlier rounds' mutation tables (round 1 and round 3) are in the PR history; the round-3 ones above were re-run after the rebase because their lines moved.

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.
  • 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 (checked on current main: runRecoveryOnce has no production caller, and no ProviderTurnStartService caller bypasses the outbox).
  • Users' own terminals and clones are not gated after the last check. This is intended.
  • The window length is an estimate from a synthetic copy, not a measured update.

General vs project

All general. One server setting and its gate in the public fork; no project names, paths, hosts or ids. The quiet command is the hook for project-specific rules (a project points it at its own script); this PR ships none.

🤖 Generated with Claude Code

@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 scuttletoby mentioned this pull request Nov 12, 2026
Merged
@scuttletoby

scuttletoby commented Nov 12, 2026

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

scuttletoby commented Nov 12, 2026

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)

@scuttletoby scuttletoby mentioned this pull request Nov 12, 2026
Merged
@scuttletoby

scuttletoby commented Nov 12, 2026

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 #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

scuttletoby commented Nov 12, 2026

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

scuttletoby commented Nov 12, 2026

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.

@scuttletoby

scuttletoby commented Nov 12, 2026

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

scuttletoby commented Nov 12, 2026

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

scuttletoby commented Nov 12, 2026

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)

@scuttletoby

scuttletoby commented Nov 12, 2026

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

@scuttletoby
scuttletoby marked this pull request as draft November 14, 2026 05:20
@scuttletoby @claude
Auto-update (server): background service updates itself when quiet
3ecd968 
A boot-service server checks its release track hourly and, when the
automaticUpdates setting is on, runs the existing self-update once no
thread, background task, terminal command or quiet command objects. A
WorkAdmission gate holds new work from the last check to the restart,
and the restarted server starts the held work once the launcher commits.

This squashes the reviewed rounds 1-3 (9df6fa4..350656a) and rebases
them on fork main. Round 4 changes:
- Settings take upstream pingdotgg#17585/pingdotgg#17586's name and place: automaticUpdates
  (was autoUpdateServer), plus the fork-only automaticUpdatesQuietCommand
  (was autoUpdateQuietCommand). A file with the old names is renamed in
  memory on load (cloud/autoUpdateSettings.ts); no schema migration.
- The desktop install-on-quit half moved out of this PR; it waits for
  upstream pingdotgg#17588.
- Rebase fixes for fork main: worker.runOnce(claimLimits),
  launcher.prepareTrial({ baseDir }), the launch layer's event sink.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@scuttletoby scuttletoby mentioned this pull request Nov 14, 2026
Draft
@scuttletoby scuttletoby changed the title Auto-update: server updates itself when quiet, desktop installs on quit Auto-update: server updates itself when quiet Nov 14, 2026
@scuttletoby scuttletoby mentioned this pull request Nov 14, 2026
Open
@scuttletoby

scuttletoby commented Nov 14, 2026

Copy link
Copy Markdown
Owner Author

lens: data-integrity DI-2 r4 · reviewer: opus high (data-integrity DI-2 r4) · read at: 3ecd968

covered: the round-4 change at the head. (1) The rebase conflict merges: EffectWorker.ts (gate around worker.runOnce(claimLimits)), serverRuntimeStartup.ts (startHeldWork right after prepareTrial({ baseDir })), ThreadLaunchService.ts and its test (harness now includes layerOrchestrator), and the test fakes (runOnce: () => …, nextClaimableAt, protocol). (2) cloud/autoUpdateSettings.ts, its test and the single call in serverSettings.ts, read in full together with the load and write paths: loadSettingsFromDisk, updateAndPersistSettings, writeSettingsAtomically, managed withDefaults/keepStored. (3) The contract lines in packages/contracts/src/settings.ts. Instead of git range-diff (one squashed commit on each side) I compared the PR diff old (24e7747..350656a) against new (9cdbb73..3ecd968) line by line. Besides the rebase, only these changed: the rename (pure name swaps in autoUpdate.ts and its test; workAdmission.ts is byte-identical) and the new settings files. I also read main's changes since the old base that touch this path: the recovery "Server restarted" notice, ThreadLaunchService branch checks and rename, and the EffectWorker claim limits. Upstream pingdotgg#17585/pingdotgg#17586 (read only): same key, Schema.Boolean with withDecodingDefault, after enableProviderUpdateChecks; upstream defaults it to true.

checks:

  • Tests (GH_TOKEN/GITHUB_TOKEN unset, empty GH_CONFIG_DIR, capacity OK, only the PR's 8 touched test files, on my own detached worktree at the head): 327/327 pass.
  • The r3 findings are still fixed after the rebase. Each guard was broken by hand on a copy and only its own tests were run. Red each time:
    • held work started before prepareTrial: 2 red;
    • ThreadLaunchService admitted guard removed: 1 red;
    • recovery queueHeld !== true removed: 1 red;
    • 30-min cap checked before the pending record: 1 red;
    • request-time comparison: gone (held work is chosen by marker, runs and started, not by time).
      Also red: started set ignored (2), runs narrowing ignored (1), started not recorded before the starts (2), outbox admit removed from the conflict-merged EffectWorker line (1), and in the settings rename: call removed, "new name wins" removed, type check removed (1 each). Survived: the rename moved after the policy defaults (finding 1).
  • Conflict merges: the outbox daemon is still the only production caller of OrchestrationEffectWorkerV2 (drain/runRecoveryOnce have none), and every claim goes through admit. Main's new "Server restarted" notice loop runs over runs after the held runs are filtered out, so a held run gets no notice and is not cancelled. A usage-limit unsent resume that is held is started by startHeldWork and not refreshed and resent by the limit worker (the requestId stays spent), so it starts once. Main added no new turn-start or setup-script path inside a turn that bypasses the gate (the WorktreeMcpService/GitManager setup scripts run only inside a turn or from the UI, as stated).
  • Old-name copy:
    • old file → loads ON, other keys kept, the next write saves only the new names (tested);
    • both names → the new one wins;
    • wrong type → dropped, other settings kept;
    • {} / no file → off;
    • torn file → decode fails, defaults (off), and updateAndPersistSettings refuses every write while parseFailure is set, so the torn file is never overwritten;
    • a write while old names are present → every write first runs reloadSettings (rename applied), then writes the sparse decoded settings: old keys go, the value is kept under the new name. A write that turns it off saves no key (false = default), so it reads off afterwards;
    • automaticUpdates: null in the file → fails the decode → off.
      No path copies a value that turns updates ON unless the user saved ON under the old name.
  • State question: "What happens when this runs again, stops half-way, or meets existing/old/empty state (rerun, half-applied, legacy data, empty history)?"
    • Rerun: the rename is pure and runs on every load and watcher reload; once a write drops the old keys it is a no-op. Recovery twice, startHeldWork twice and a later startup start only runs minus started (mutation-checked above).
    • Half-applied: the settings write is atomic, so the old file stays whole and the old names still load. A crash mid-update leaves the setting untouched (the updater never writes it). The marker's runs/started order is unchanged by the rebase (mutation M12 red).
    • Legacy data / rollback: an older fork build ignores automaticUpdates and reads off, which fails closed. One addition to the body's rollback row: that older build's next settings write is sparse from its own schema, so it also deletes automaticUpdates from the file. Going forward again then still reads off. The ON choice is lost quietly, but updates never turn on by themselves and no other setting is lost. An auto-rollback right after an update normally leaves settings.json untouched, because startup writes only for other migrations, so the old names still read ON there.
    • Empty history: no file, {} and no marker all take the upstream path, off.
  • Convergence (not this PR's code, noted for the owner decision the body already names): an explicit OFF is never saved (false = default, stripped). After upstream's default-true line lands, a server that turned it on and later off cannot be told apart from one that never chose, and both start updating. If the fork keeps "off", the sync has to override the default; the rename file alone cannot keep it off.

finding: minor apps/server/src/serverSettings.ts:798 The order "rename, then managed.withDefaults" is right but untested. Moving the rename after the policy defaults keeps all 7 settings tests green. With that mutation, and a managed policy that defaults automaticUpdates to true, an old file that says autoUpdateServer: false loads ON: the policy fill comes first, the rename then sees a "new name" and skips. In the other direction, an old true under a policy default of false loads off. Add one test with ManagedPolicy.layerTest giving a default for automaticUpdates plus an old-name file. (medium)

verdict: SAFE

@scuttletoby scuttletoby mentioned this pull request Nov 14, 2026
Open
@scuttletoby

scuttletoby commented Nov 14, 2026

Copy link
Copy Markdown
Owner Author

lens: data-integrity DI-1 r4 · reviewer: opus high (t3lens5-di1-r4) · read at: 3ecd968
covered: what round 4 changed. Old head 350656a (r3 fixes on 5164fea) compared with 3ecd968, per file, PR diff against PR diff, because the squash leaves git range-diff nothing to pair. Read in full at head: cloud/autoUpdateSettings.ts (+ test), the call in serverSettings.ts (load, writeSettingsAtomically, updateSettings, readStoredSettings), policy/managedSettings.ts (withDefaults / keepStored), and the contracts/settings.ts lines. Main's changes under the PR (24e7747..9cdbb73) read where the hold depends on them: EffectWorker daemon loop, serviceLauncherClient.prepareTrial({baseDir}), serviceProtocol (protocol 3|4 state decode), selfUpdate (launcher.protocol), recovery's new "Server restarted" notice, ThreadLaunchService (branch rename / checkWorktreeBase), and the callers of ProviderTurnStartService / startRootRun. Upstream pingdotgg#17585 setting shape compared.
checks:

  • moved besides the rebase: only the renames in autoUpdate.ts / server.ts, and EffectWorker now wraps worker.runOnce(claimLimits). serverRuntimeStartup.ts, ThreadLaunchService.ts, ProviderRuntimeRecoveryService.ts, workAdmission.ts, selfUpdate.ts: the PR diff is the same line for line as at 350656a.
  • tests (GH_TOKEN/GITHUB_TOKEN unset, empty GH_CONFIG_DIR), 8 touched files: 327/327 green at head.
  • mutations, each on a local copy against all 8 files, then reverted. 12 of 13 red: rename call removed (1), type check removed (1), new-name-wins removed (1), outbox admit around runOnce(claimLimits) removed (1), startHeldWork moved before prepareTrial (2), admitted guard (1), setPreparationResumer (1), r3 requestedAt filter put back (4), finishHeldWork after a part-way failure (1), queueHeld guard (1), started not recorded (2), runs narrowing removed (1). NOT caught: rename moved after managed.withDefaults (finding).
  • r3 findings after the rebase: (1) rollback double start: main's prepareTrial still resolves only on the launcher's committed reply. Repointing launchers happens after that, and if it fails, startHeldWork is skipped and the next startup does it once, from runs minus started. Still fixed. (2)/(3) request-time race / "send now": fixed, 4 red tests when the filter returns. (4) untested guards: now red. (5) part-way failure: marker kept, red when broken.
  • outbox gate across main's changes: the claim (outbox.claimNext) happens only inside runOnce, which is inside admit. runRecoveryOnce has no production caller. Turn starts reach a provider only via EffectWorker -> ProviderTurnStartService -> startRootRun. Main's new recovery notice loops over runs after held runs are removed (:449), so held runs get no "Server restarted" item. The launcher state read now accepts protocol 3 and 4. selfUpdate refusing when launcher.protocol is undefined fails before installing, so the gate is never closed for it.
  • old-name copy: old file -> loads; next write (stripDefaultServerSettings of the in-memory settings) saves only new names; both names -> new wins; wrong type / null -> not copied, decode unaffected, other settings kept; {}, no file, truncated file -> off (a truncated file takes upstream's parse-failure path, unchanged); non-object -> passed through. The function copies, never mutates parsed; the legacy provider decode still reads raw. Hand edits picked up by the watcher reload through the same load path. Nothing turns updates ON by itself, except the policy-default edge in the finding below.
  • rollback to an older fork build: no fork release or tag contains this PR's code (only branch fork/auto-update), so old-name files exist only where a branch build ran. Such a build ignores automaticUpdates -> off (fails closed). If it then writes, it drops automaticUpdates -> still off after rolling forward. The launcher does not restore settings.json. A file holding both names with conflicting values after a rollback and a hand edit: new wins, as documented.
  • upstream fit: name, type and position match feat(server): install provider CLI updates in the background pingdotgg/t3code#17585. Upstream defaults it to true, so servers that never chose would turn on at convergence: already owner issue Owner question: automatic updates on or off by default after upstream's version lands? #41.
  • STATE question: "What happens when this runs again, stops half-way, or meets existing/old/empty state (rerun, half-applied, legacy data, empty history)?" Rename: a pure function on every load, so a rerun is the same. A half-written settings write cannot happen (atomic write). Old/empty/torn files as above. Held work: rerun = only runs minus started. Half-way = marker kept, the rest starts later. Rollback = the restored version starts each run once. No marker, or a stale/invalid one = upstream path. All unchanged by the conflict merges (mutations above).
    finding: minor apps/server/src/serverSettings.ts:799 + apps/server/src/policy/managedSettings.ts:474: the rename must run before managed.withDefaults, and it does, but no test has a managed policy, so swapping the order passes all 327 tests. With a policy that defaults automaticUpdates: true and an old file saying autoUpdateServer: false, the swapped order would switch updates ON. A related case, already reachable now: under such a policy default, keepStored reads the un-renamed file. An old-name autoUpdateServer: true that equals the policy default is therefore not restored on the first write, and the choice is lost once the policy is removed (fails closed). Both need a policy naming this key plus an old-name file, so they are rare. Fix idea: one test with ManagedPolicy defaulting automaticUpdates, and apply renameRetiredAutoUpdateKeys in readStoredSettings too. (low)
    verdict: SAFE (0 major, 1 minor)

@scuttletoby
scuttletoby marked this pull request as ready for review November 14, 2026 06:07
@scuttletoby scuttletoby mentioned this pull request Nov 14, 2026
Open
@scuttletoby
scuttletoby merged commit ebcb5cb into main Nov 14, 2026
27 checks passed
@scuttletoby
scuttletoby deleted the fork/auto-update branch November 14, 2026 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@scuttletoby