fix(update): stop a slow progress write from erasing the install outcome

The state file is a temp file plus rename, so the last rename wins. Progress
writes were fire-and-forget, and the installer's terminal state=done line
bypasses the write throttle just as the child exits — so on the ordinary
success path that write could land after the outcome, restoring active and
dropping lastSuccess. The next launch read that as an abandoned install and
recorded a failure for a version that had installed cleanly, which parks the
version at two attempts.

Progress writes now run on one chain, new ones stop once the outcome is
settled, and the finalizer drains the chain before writing. Removing the
drain fails the new test.

Also from review: the CDN reject cases named the field they expect instead of
matching any non-empty message, the lock-ordering test now reaches the prompt
before asserting no lock was taken (it previously asserted before the first
await, so it held wherever the acquisition sat), and two lint nits this branch
introduced.

Author: elkaix

apps/pythinker-code/src/cli/update/preflight.ts

@@ -841,6 +841,15 @@ async function startBackgroundInstall(
     let ready = false;
     let settled = false;
     let pendingOutcome: { succeeded: boolean; reason: string } | undefined;
+    // Progress writes are fire-and-forget, and the state file is written as a
+    // temp file plus rename — so the last rename wins. The installer's terminal
+    // `state=done` line bypasses the throttle and writes just as the child
+    // exits, so without ordering that write can land *after* the outcome write
+    // and restore `active` while dropping `lastSuccess`. The next launch reads
+    // that as an abandoned install and records a failure for a version that
+    // installed cleanly. One chain keeps the writes ordered and gives `finish`
+    // something to drain.
+    let progressWrites: Promise<void> = Promise.resolve();
 
     const finish = async (succeeded: boolean, reason: string): Promise<void> => {
       if (!ready) {
@@ -849,6 +858,9 @@ async function startBackgroundInstall(
       }
       if (settled) return;
       settled = true;
+      // `settled` already stops new progress writes; drain the ones in flight so
+      // none of them renames over the outcome below.
+      await progressWrites;
       const attempts = failureAttemptsFor(startedState, target, 'install') + 1;
       const stderrTail = readStderrTail();
       const message = stderrTail === undefined ? reason : `${reason}: ${stderrTail}`;
@@ -917,6 +929,9 @@ async function startBackgroundInstall(
     });
     let lastProgressWriteAt = 0;
     const recordInstallerProgress = (update: UpdateInstallProgress): void => {
+      // Once the outcome is being written, progress is history: writing it would
+      // undo the terminal record.
+      if (settled) return;
       // Terminal states always persist; intermediate ones at most every 2s.
       const terminal = update.state === 'done' || update.state === 'failed';
       if (
@@ -936,14 +951,17 @@ async function startBackgroundInstall(
           progress: update,
         },
       };
-      writeUpdateInstallState(nextState).catch((error) => {
-        // A progress write is best-effort; it must never reject the spawn path.
-        logUpdateWarn(logger, 'could not record installer progress', {
-          targetVersion: target.version,
-          source,
-          error: formatErrorMessage(error),
+      progressWrites = progressWrites
+        .then(() => writeUpdateInstallState(nextState))
+        .catch((error) => {
+          // A progress write is best-effort; it must never reject the spawn path
+          // and must not break the chain for the writes queued behind it.
+          logUpdateWarn(logger, 'could not record installer progress', {
+            targetVersion: target.version,
+            source,
+            error: formatErrorMessage(error),
+          });
         });
-      });
     };
     const readStderrTail = captureStderrTail(child, recordInstallerProgress);
     child.once('error', (error) => { void finish(false, formatErrorMessage(error)); });

apps/pythinker-code/test/cli/update/cdn.test.ts

@@ -173,18 +173,18 @@ describe('fetchUpdateManifest', () => {
   // so reading it after a bad manifest would report an unverifiable target as
   // verified. Every one of these must reject and leave the cached answer alone.
   const rejectCases: ReadonlyArray<readonly [string, Route, RegExp]> = [
-    ['latest.json is missing (HTTP 404)', { status: 404 }, /HTTP 404/],
-    ['latest.json fetch throws', new Error('network down'), /network down/],
-    ['body is not valid JSON', { body: 'not json {' }, /JSON/i],
+    ['latest.json is missing (HTTP 404)', { status: 404 }, /HTTP 404/u],
+    ['latest.json fetch throws', new Error('network down'), /network down/u],
+    ['body is not valid JSON', { body: 'not json {' }, /JSON/iu],
     [
       'version is not semver',
       { body: JSON.stringify({ version: 'nope', publishedAt: '2026-06-12T00:00:00.000Z' }) },
-      /invalid semver/,
+      /invalid semver/u,
     ],
     [
       'publishedAt is unparseable',
       { body: JSON.stringify({ version: '2.0.0', publishedAt: 'garbage' }) },
-      /invalid timestamp/,
+      /invalid timestamp/u,
     ],
     [
       'a batch percent is out of range',
@@ -195,7 +195,9 @@ describe('fetchUpdateManifest', () => {
           rollout: [{ percent: 150, delaySeconds: 0 }],
         }),
       },
-      /./,
+      // Name the field: `/./` matched any non-empty message, so a JSON.parse
+      // failure would have satisfied it just as well as the schema rejection.
+      /percent/u,
     ],
     [
       'a batch delay is negative',
@@ -206,7 +208,7 @@ describe('fetchUpdateManifest', () => {
           rollout: [{ percent: 100, delaySeconds: -1 }],
         }),
       },
-      /./,
+      /delaySeconds/u,
     ],
   ];
 
@@ -229,7 +231,7 @@ describe('fetchUpdateManifest', () => {
       }) as unknown as typeof fetch;
 
       const result = fetchUpdateManifest(f);
-      const expectation = expect(result).rejects.toThrow(/aborted/);
+      const expectation = expect(result).rejects.toThrow(/aborted/u);
       await vi.advanceTimersByTimeAsync(3_000);
 
       await expectation;

apps/pythinker-code/test/cli/update/preflight.test.ts

@@ -388,6 +388,14 @@ function progressActiveStates(): unknown[] {
     .filter((state) => state !== undefined && state !== null && state.active?.progress !== undefined);
 }
 
+/** The terminal success records written by the finalizer, in order. */
+function successOutcomeStates(): unknown[] {
+  return mocks.writeUpdateInstallState.mock.calls
+    .map((call) => call[0])
+    .filter((state) => state !== undefined && state !== null
+      && state.active === null && state.lastSuccess !== undefined && state.lastSuccess !== null);
+}
+
 async function flushBackgroundInstall(): Promise<void> {
   await new Promise<void>((resolve) => {
     setImmediate(resolve);
@@ -961,6 +969,13 @@ describe('runUpdatePreflight', () => {
 
     const running = runUpdatePreflight('0.4.0', options);
 
+    // Let the flow actually reach the prompt first. Asserting straight after the
+    // call was vacuous: nothing had run past the first await, so "no lock yet"
+    // held wherever the acquisition sat, and the ordering claim in the test name
+    // went unchecked.
+    await vi.waitFor(() => {
+      expect(mocks.promptForInstallChoice).toHaveBeenCalled();
+    });
     expect(mocks.tryAcquireUpdateInstallLock).not.toHaveBeenCalled();
     resolvePrompt?.('install');
     await expect(running).resolves.toBe('exit');
@@ -1178,7 +1193,7 @@ describe('runUpdatePreflight', () => {
     await expect(runUpdatePreflight('0.5.0', options)).resolves.toBe('continue');
 
     expect(mocks.spawn).toHaveBeenCalledWith(
-      expect.stringMatching(/^npm(\.cmd)?$/),
+      expect.stringMatching(/^npm(\.cmd)?$/u),
       ['install', '-g', '@pythoughts/pythinker-code@0.6.0'],
       { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] },
     );
@@ -2099,6 +2114,50 @@ describe('runUpdatePreflight', () => {
       }));
     });
 
+    /**
+     * The state file is written as a temp file plus rename, so the last rename
+     * wins. The installer's terminal `state=done` line writes just as the child
+     * exits, so an unawaited progress write can rename over the outcome —
+     * restoring `active` and dropping `lastSuccess`. The next launch reads that
+     * as an abandoned install and records a failure for a version that
+     * installed cleanly, which at two attempts parks it for good.
+     */
+    it('never lets a slow progress write rename over the install outcome', async () => {
+      mocks.readUpdateCache.mockResolvedValue(cacheWith('0.5.0'));
+      mocks.readUpdateInstallState.mockResolvedValue(installState());
+      mocks.refreshUpdateCache.mockResolvedValue(cacheWith('0.5.0'));
+      mocks.detectInstallSource.mockResolvedValue('npm-global');
+      // Hold the progress write open; every other write settles at once.
+      let releaseProgressWrite: (() => void) | undefined;
+      mocks.writeUpdateInstallState.mockImplementation(
+        (state: { active?: { progress?: unknown } | null }) => (
+          state.active?.progress === undefined
+            ? Promise.resolve()
+            : new Promise<void>((resolve) => { releaseProgressWrite = resolve; })
+        ),
+      );
+      mockSpawnExitWithStderr(0, 'progress: state=done transferred=55795679\n');
+      const { options } = captureOutput();
+
+      await expect(runUpdatePreflight('0.4.0', options)).resolves.toBe('continue');
+      await flushBackgroundInstall();
+
+      // The progress write is still in flight, so the outcome must not be out yet.
+      expect(progressActiveStates()).toHaveLength(1);
+      expect(successOutcomeStates()).toEqual([]);
+
+      releaseProgressWrite?.();
+      await flushBackgroundInstall();
+      await flushBackgroundInstall();
+
+      expect(successOutcomeStates()).toHaveLength(1);
+      // The outcome is the last thing written, so it survives on disk.
+      expect(mocks.writeUpdateInstallState.mock.calls.at(-1)?.[0]).toMatchObject({
+        active: null,
+        lastSuccess: expect.objectContaining({ version: '0.5.0' }),
+      });
+    });
+
     it('keeps progress lines out of the failure tail and ordinary stderr lines in it', async () => {
       mocks.readUpdateCache.mockResolvedValue(cacheWith('0.5.0'));
       mocks.readUpdateInstallState.mockResolvedValue(installState());

apps/pythinker-code/test/tui/pythinker-tui-startup.test.ts

@@ -2022,8 +2022,7 @@ describe('footer update status poll', () => {
    */
   it('dispatches availability and then live progress into the status row', async () => {
     const home = mkdtempSync(join(tmpdir(), 'pk-footer-update-'));
-    const previousHome = process.env['PYTHINKER_CODE_HOME'];
-    process.env['PYTHINKER_CODE_HOME'] = home;
+    vi.stubEnv('PYTHINKER_CODE_HOME', home);
     const updates = join(home, 'updates');
     mkdirSync(updates, { recursive: true });
     const manifest = {
@@ -2089,8 +2088,7 @@ describe('footer update status poll', () => {
     } finally {
       driver.stopUpdateStatusPolling();
       driver.state.footer.dispose();
-      if (previousHome === undefined) delete process.env['PYTHINKER_CODE_HOME'];
-      else process.env['PYTHINKER_CODE_HOME'] = previousHome;
+      vi.unstubAllEnvs();
       rmSync(home, { recursive: true, force: true });
     }
   }, 30_000);