From 254db138cb435c9cdef78a155602cd90b3b4a920 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 00:57:19 +0900 Subject: [PATCH] fix(codex): warn when a startup write leaves an app-server stale (#1046) Service startup rewrites the catalog and the models cache, but an app-server that booted earlier keeps its own in-memory model list - Codex builds a static manager from the catalog once and never rereads the file. Every check a user runs reads the file; the picker renders memory. The reporter pinned it exactly: their catalog was rewritten four minutes after the app-server started. afterCatalogWriteHandleAppServers already exists but only runs from explicit sync and sync-cache, and its restart branch SIGTERMs matching processes while saying active turns may be interrupted. That is fine when a human typed --restart-codex. It is not fine on a login. So this adds a warning-only counterpart that can never reach the signal path, asserted by an injected kill that fails the test if called. It also asks the mtime classifier whether an app-server is actually stale rather than warning about any running one, so a boot with Codex open and a current catalog stays quiet. Two startup sites write: the server's cache invalidation and the later catalog sync. Neither warns on its own - handleStart ORs their results and warns once, after both, because warning after the first would read an mtime the second is about to move. Making that observable meant typing the startup sync seam, which had been discarding its result and returning a bare boolean. --- src/cli/index.ts | 11 ++- src/codex/app-server-processes.ts | 45 ++++++++- src/codex/desired-state.ts | 34 +++++-- src/server/index.ts | 28 +++++- tests/codex-app-server-processes.test.ts | 118 +++++++++++++++++++++++ tests/codex-desired-state.test.ts | 15 +-- 6 files changed, 235 insertions(+), 16 deletions(-) diff --git a/src/cli/index.ts b/src/cli/index.ts index 335221b7c5..fdcec562d2 100755 --- a/src/cli/index.ts +++ b/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(); } diff --git a/src/codex/app-server-processes.ts b/src/codex/app-server-processes.ts index 085ddcc949..93594d0284 100644 --- a/src/codex/app-server-processes.ts +++ b/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; 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 }; + } +} diff --git a/src/codex/desired-state.ts b/src/codex/desired-state.ts index 5999d48738..f75f261b31 100644 --- a/src/codex/desired-state.ts +++ b/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; +/** + * 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; 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, sync: CodexStartupSync = defaultStartupSync, -): Promise { - 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 { +async function defaultStartupSync(port: number): Promise { const { syncModelsToCodex } = await import("./sync"); return syncModelsToCodex(port); } diff --git a/src/server/index.ts b/src/server/index.ts index 0601bb04e6..69b2d84353 100644 --- a/src/server/index.ts +++ b/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 diff --git a/tests/codex-app-server-processes.test.ts b/tests/codex-app-server-processes.test.ts index 3f35444340..c1b421bd79 100644 --- a/tests/codex-app-server-processes.test.ts +++ b/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([]); + }); +}); diff --git a/tests/codex-desired-state.test.ts b/tests/codex-desired-state.test.ts index 642c06c511..9a45ce5d4a 100644 --- a/tests/codex-desired-state.test.ts +++ b/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); }); });