fix(codex): warn when a startup write leaves an app-server stale (#1046)

src/cli/index.ts

@@ -317,7 +317,16 @@ async function handleStart(options: { block?: boolean } = {}) {
   installShellHook();
 
   await maybeShowStarPrompt(); // once-only Yes/No GitHub-star prompt on first interactive start
-  await syncCodexOnStartIfEnabled(port, config);
+  const startupSync = await syncCodexOnStartIfEnabled(port, config);
+  // #1046: one warning per startup, after BOTH writes. The server's cache
+  // invalidation happens first and the catalog sync second, so the mtime is only
+  // final here — and neither write site warns on its own, or a boot that hits
+  // both would warn twice.
+  const { consumeStartupCacheInvalidationWrite } = await import("../server");
+  if (consumeStartupCacheInvalidationWrite() || startupSync.catalogWritten || startupSync.cacheSynced) {
+    const { warnIfStaleCodexAppServersAfterStartupWrite } = await import("../codex/app-server-processes");
+    warnIfStaleCodexAppServersAfterStartupWrite({ log: console });
+  }
   if (!currentExternalCodexModelProvider() && !shouldInjectApiAuthHeader(config) && config.syncResumeHistory !== false) {
     historyGuardian = startHistoryMigrationGuardian();
   }

src/codex/app-server-processes.ts

@@ -407,7 +407,9 @@ export function listCodexAppServerProcesses(io: CodexAppServerProcessIo = {}): C
   return matched;
 }
 
-export function formatStaleCodexAppServerWarning(processes: readonly CodexAppServerProcess[]): string {
+export function formatStaleCodexAppServerWarning(
+  processes: readonly { pid: number }[],
+): string {
   const pids = processes.map(process => process.pid).join(", ");
   return (
     `WARNING: ${processes.length} Codex app-server process(es) still running (PID${processes.length === 1 ? "" : "s"}: ${pids}). `
@@ -754,3 +756,44 @@ export function afterCatalogWriteHandleAppServers(
   }
   return { processes, warned: false, restart, hint };
 }
+
+/**
+ * Startup-safe counterpart to {@link afterCatalogWriteHandleAppServers} (#1046).
+ *
+ * Service startup rewrites the catalog and the models cache, but an app-server
+ * that booted earlier keeps an in-memory model list — Codex builds a static
+ * manager from the catalog once and never rereads the file — so the picker shows
+ * a roster that no longer exists on disk. Every check a user runs reads the file;
+ * the picker renders memory.
+ *
+ * Two things this deliberately does NOT do, both of which the `--restart-codex`
+ * path does:
+ *
+ * - It never signals anything. Killing an app-server on an unattended boot would
+ *   interrupt whatever turn the user has in flight. A human typing
+ *   `ocx sync --restart-codex` is consenting to that; a login is not.
+ * - It never warns about a merely-running app-server. It asks the mtime
+ *   classifier whether one is actually stale, so a boot with Codex open and a
+ *   current catalog stays quiet.
+ *
+ * Failure is swallowed: startup synchronization is best-effort and must not stop
+ * the proxy from coming up.
+ *
+ * The memoized state is dropped first. {@link collectCodexAppServerCatalogState}
+ * caches for 5s when every io field is defaulted, so a `fresh` reading taken
+ * before the write would otherwise be replayed after it and this would stay
+ * silent about the very staleness it exists to report.
+ */
+export function warnIfStaleCodexAppServersAfterStartupWrite(
+  options: { log?: Pick<Console, "error">; io?: CodexAppServerProcessIo } = {},
+): { warned: boolean } {
+  try {
+    resetCodexAppServerCatalogStateCache();
+    const status = collectCodexAppServerCatalogState(options.io ?? {});
+    if (status.state !== "stale") return { warned: false };
+    options.log?.error(formatStaleCodexAppServerWarning(status.processes));
+    return { warned: true };
+  } catch {
+    return { warned: false };
+  }
+}

src/codex/desired-state.ts

@@ -27,7 +27,17 @@ import type { OcxClientIntegrationsConfig, OcxConfig } from "../types";
 export type DurableIntentClientId = keyof OcxClientIntegrationsConfig;
 
 /** Injectable for tests; production passes the real sync. */
-export type CodexStartupSync = (port: number) => Promise<unknown>;
+/**
+ * The startup sync result the caller needs to decide whether anything was
+ * actually written (#1046). It used to be `unknown`, so "a write happened" was
+ * not observable at the startup boundary and no post-write action could be
+ * gated on it.
+ */
+export interface CodexStartupSyncOutcome {
+  catalogWritten: boolean;
+  cacheSynced: boolean;
+}
+export type CodexStartupSync = (port: number) => Promise<CodexStartupSyncOutcome | undefined>;
 
 export type CodexDesiredStateResult =
   | { readonly ok: true; readonly status: "committed" | "unchanged"; readonly enabled: boolean }
@@ -143,19 +153,29 @@ export function setGrokIntegrationEnabled(enabled: boolean): CodexDesiredStateRe
  * Swallowing the user's decision was not.
  *
  * Returns whether the sync ran, so a caller — or a test — can tell "skipped
- * because the user turned it off" from "ran and quietly failed".
+ * because the user turned it off" from "ran and quietly failed", plus what it
+ * wrote when it did run (#1046 — the caller warns about stale app-servers only
+ * after a real write).
  */
 export async function syncCodexOnStartIfEnabled(
   port: number,
   config: Pick<OcxConfig, "clientIntegrations">,
   sync: CodexStartupSync = defaultStartupSync,
-): Promise<boolean> {
-  if (!codexIntegrationEnabled(config)) return false;
-  await sync(port).catch(() => {});
-  return true;
+): Promise<{ ran: boolean; catalogWritten: boolean; cacheSynced: boolean }> {
+  if (!codexIntegrationEnabled(config)) {
+    return { ran: false, catalogWritten: false, cacheSynced: false };
+  }
+  // The `.catch` is deliberate and stays: a failure to APPLY must not stop the
+  // proxy from coming up. A failed sync simply reports no writes.
+  const outcome = await sync(port).catch(() => undefined);
+  return {
+    ran: true,
+    catalogWritten: outcome?.catalogWritten === true,
+    cacheSynced: outcome?.cacheSynced === true,
+  };
 }
 
-async function defaultStartupSync(port: number): Promise<unknown> {
+async function defaultStartupSync(port: number): Promise<CodexStartupSyncOutcome> {
   const { syncModelsToCodex } = await import("./sync");
   return syncModelsToCodex(port);
 }

src/server/index.ts

@@ -359,6 +359,27 @@ export interface StartServerDeps {
   liveSidebandWebSocketFactory?: LiveSidebandWebSocketFactory;
 }
 
+/*
+ * #1046. `startServer` rewrites the Codex models cache during boot, and an
+ * app-server that started earlier keeps its own in-memory model list. The stale
+ * warning is not emitted here: `handleStart` runs a catalog sync moments later,
+ * so warning now would read an mtime that write is about to move, and both sites
+ * calling the helper independently would warn twice. This records the fact; the
+ * CLI start path owns the single decision.
+ *
+ * A caller that starts a server without `handleStart` (tests, embedded use)
+ * deliberately gets no warning — lifecycle diagnostics belong to whoever owns
+ * the lifecycle.
+ */
+let startupCacheInvalidationWrote = false;
+
+/** #1046: did this process's startup cache invalidation actually write? */
+export function consumeStartupCacheInvalidationWrite(): boolean {
+  const wrote = startupCacheInvalidationWrote;
+  startupCacheInvalidationWrote = false;
+  return wrote;
+}
+
 export function startServer(port?: number, deps: StartServerDeps = {}) {
   const config = runAlibabaRegionStartupMigration(runOpenAiTierStartupMigration(loadConfig()));
   setLiveStateStoreConfig(config);
@@ -407,8 +428,13 @@ export function startServer(port?: number, deps: StartServerDeps = {}) {
   // otherwise turn "no Codex installed" into "proxy will not start".
   try {
     const startupCodexHome = getCodexHome();
-    withCatalogWriteSerialization(startupCodexHome, permit =>
+    // #1046: record whether this actually rewrote the cache. `handleStart` ORs this
+    // with the later startup sync and warns ONCE about stale app-servers; warning
+    // here instead would read a catalog mtime the sync is about to move.
+    const outcome = withCatalogWriteSerialization(startupCodexHome, permit =>
       invalidateCodexModelsCacheWithPermit(permit, startupCodexHome));
+    // A refused permit is not a write; only a completed run that returned true is.
+    startupCacheInvalidationWrote = outcome.kind === "completed" && outcome.value === true;
   } catch { /* no readable Codex home: nothing to invalidate */ }
   // Arm the `claudeCode` hand-edit guard (devlog 260726_claude_auth_auto/040 H1) BEFORE
   // the server can serve a request, and AFTER the startup migrations above — those run

tests/codex-app-server-processes.test.ts

@@ -11,8 +11,10 @@ import {
   isWindowsCodexCandidateCommandLine,
   listCodexAppServerProcesses,
   listWindowsSnapshots,
+  resetCodexAppServerCatalogStateCache,
   restartCodexAppServers,
   STALE_CODEX_APP_SERVER_HINT,
+  warnIfStaleCodexAppServersAfterStartupWrite,
   WINDOWS_CODEX_BASENAME_CANDIDATE_RE,
 } from "../src/codex/app-server-processes";
 
@@ -500,3 +502,119 @@ describe("Windows Win32_Process owner enumeration (#476)", () => {
     { timeout: 35_000 },
   );
 });
+
+/*
+ * #1046. Service startup rewrites the catalog and the models cache while an
+ * app-server that booted earlier keeps its own in-memory model list, so the
+ * picker shows a roster that no longer exists on disk. The startup path warns;
+ * it must never signal, because a boot is not a user consenting to have an
+ * in-flight turn interrupted.
+ */
+describe("warnIfStaleCodexAppServersAfterStartupWrite (#1046)", () => {
+  const APP_SERVER_CMD = "/usr/local/bin/codex app-server";
+  const collectErrors = () => {
+    const errors: string[] = [];
+    return { log: { error: (m?: unknown) => { errors.push(String(m)); } }, errors };
+  };
+
+  test("warns when an app-server predates the catalog write", () => {
+    const { log, errors } = collectErrors();
+    const result = warnIfStaleCodexAppServersAfterStartupWrite({
+      log,
+      io: {
+        listSnapshots: () => [{ pid: 4242, commandLine: APP_SERVER_CMD }],
+        readStartMs: () => 1_000,
+        catalogMtimeMs: () => 2_000,
+      },
+    });
+    expect(result.warned).toBe(true);
+    expect(errors).toHaveLength(1);
+    expect(errors[0]).toContain("4242");
+  });
+
+  test("stays quiet for fresh, not_running, and unknown", () => {
+    const fresh = collectErrors();
+    expect(warnIfStaleCodexAppServersAfterStartupWrite({
+      log: fresh.log,
+      io: {
+        listSnapshots: () => [{ pid: 1, commandLine: APP_SERVER_CMD }],
+        readStartMs: () => 3_000,
+        catalogMtimeMs: () => 1_000,
+      },
+    }).warned).toBe(false);
+
+    const none = collectErrors();
+    expect(warnIfStaleCodexAppServersAfterStartupWrite({
+      log: none.log,
+      io: { listSnapshots: () => [], catalogMtimeMs: () => 1_000 },
+    }).warned).toBe(false);
+
+    const unknown = collectErrors();
+    expect(warnIfStaleCodexAppServersAfterStartupWrite({
+      log: unknown.log,
+      io: {
+        listSnapshots: () => [{ pid: 2, commandLine: APP_SERVER_CMD }],
+        readStartMs: () => null,
+        catalogMtimeMs: () => 1_000,
+      },
+    }).warned).toBe(false);
+
+    expect([...fresh.errors, ...none.errors, ...unknown.errors]).toEqual([]);
+  });
+
+  /*
+   * The masking risk this layer has to defend against, stated honestly.
+   *
+   * `collectCodexAppServerCatalogState` memoizes for 5s, but ONLY when every io
+   * field is defaulted (`fullyDefault`). That has two consequences:
+   *
+   * - Production startup runs on the default path, so a `fresh` reading taken
+   *   before the catalog write CAN be replayed after it, and the helper drops the
+   *   memo first for exactly that reason.
+   * - Any test that injects io bypasses the cache, so it cannot reproduce the
+   *   masking and would pass with or without the reset. Writing one anyway would
+   *   be a test that looks like proof and is not.
+   *
+   * So this asserts the mechanism the fix depends on — that a defaulted read is
+   * memoized and an explicit invalidation clears it — rather than pretending to
+   * exercise a path the seam makes unreachable. The helper's own call to the
+   * invalidation is verified by reading it, not by a test that cannot fail.
+   */
+  test("a defaulted read is memoized, and invalidation is what clears it", () => {
+    resetCodexAppServerCatalogStateCache();
+    const first = collectCodexAppServerCatalogState();
+    const second = collectCodexAppServerCatalogState();
+    // Same object identity: the second call served the memo rather than recomputing.
+    expect(second).toBe(first);
+
+    resetCodexAppServerCatalogStateCache();
+    expect(collectCodexAppServerCatalogState()).not.toBe(first);
+  });
+
+  /*
+   * The assertion that would catch a future refactor pointing startup at
+   * `afterCatalogWriteHandleAppServers({ restart: true })`, which SIGTERMs matching
+   * processes and says so in its own log line.
+   */
+  test("never signals a process", () => {
+    const killed: number[] = [];
+    warnIfStaleCodexAppServersAfterStartupWrite({
+      io: {
+        listSnapshots: () => [{ pid: 999, commandLine: APP_SERVER_CMD }],
+        readStartMs: () => 1_000,
+        catalogMtimeMs: () => 2_000,
+        kill: pid => { killed.push(pid); },
+      },
+    });
+    expect(killed).toEqual([]);
+  });
+
+  test("a discovery failure is swallowed so startup still comes up", () => {
+    const { log, errors } = collectErrors();
+    expect(warnIfStaleCodexAppServersAfterStartupWrite({
+      log,
+      io: { listSnapshots: () => { throw new Error("ps unavailable"); } },
+    }).warned).toBe(false);
+    expect(errors).toEqual([]);
+  });
+});

tests/codex-desired-state.test.ts

@@ -180,9 +180,9 @@ describe("the startup gate", () => {
     const ran = await syncCodexOnStartIfEnabled(
       10100,
       { clientIntegrations: { codex: false } },
-      async () => { calls += 1; },
+      async () => { calls += 1; return undefined; },
     );
-    expect(ran).toBe(false);
+    expect(ran.ran).toBe(false);
     expect(calls).toBe(0);
   });
 
@@ -192,16 +192,16 @@ describe("the startup gate", () => {
       const ran = await syncCodexOnStartIfEnabled(
         10100,
         { clientIntegrations },
-        async () => { calls += 1; },
+        async () => { calls += 1; return undefined; },
       );
-      expect(ran).toBe(true);
+      expect(ran.ran).toBe(true);
       expect(calls).toBe(1);
     }
   });
 
   test("the port reaches the sync", async () => {
     const ports: number[] = [];
-    await syncCodexOnStartIfEnabled(43210, {}, async port => { ports.push(port); });
+    await syncCodexOnStartIfEnabled(43210, {}, async port => { ports.push(port); return undefined; });
     expect(ports).toEqual([43210]);
   });
 
@@ -216,7 +216,10 @@ describe("the startup gate", () => {
       {},
       async () => { throw new Error("provider unreachable"); },
     );
-    expect(ran).toBe(true);
+    expect(ran.ran).toBe(true);
+    // #1046: a failed sync reports no writes, so the caller does not warn.
+    expect(ran.catalogWritten).toBe(false);
+    expect(ran.cacheSynced).toBe(false);
   });
 });