diff --git a/src/cli/index.ts b/src/cli/index.ts index 335221b7c..fdcec562d 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 085ddcc94..93594d028 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 5999d4873..f75f261b3 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 0601bb04e..69b2d8435 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 3f3544434..c1b421bd7 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 642c06c51..9a45ce5d4 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); }); });