diff --git a/docs-site/src/content/docs/reference/cli/lifecycle.md b/docs-site/src/content/docs/reference/cli/lifecycle.md index 3473d3b19cc..68752d29f15 100644 --- a/docs-site/src/content/docs/reference/cli/lifecycle.md +++ b/docs-site/src/content/docs/reference/cli/lifecycle.md @@ -334,6 +334,8 @@ and this session ends with the app. Invalidate Codex's local model picker cache so it is rebuilt from the active opencodex catalog. The same stale-`app-server` warning and optional restart flags as `ocx sync` apply. +If the derived cache already has identical bytes, the command succeeds without rewriting it or restarting Codex. With `--json`, this is reported as `ok: true`, `wrote: false`, `skipped: true`, and `skippedReason: "unchanged"`; an invalid catalog or failed cache write still exits nonzero. + ### `ocx catalog pull [--auth-env ] [--json] [--restart-codex] [--restart-app-server-only]` Install a complete catalog served by another OpenCodex instance's `/v1/catalog` endpoint, then diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index cea39a48d23..3600f5cc19d 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -547,21 +547,19 @@ const commandRunners: Record = { const cacheArgs = deps.args.slice(1); const restartScope = readRestartScope(cacheArgs, console); const { withCatalogWriteSerialization } = await import("../codex/catalog-write-serialization"); - const { invalidateCodexModelsCacheWithPermit } = await import("../codex/catalog/sync"); + const { invalidateCodexModelsCacheWithPermitOutcome } = await import("../codex/catalog/sync"); const { getCodexHome } = await import("../codex/paths"); - const { readCodexCatalogPathForHome } = await import("../codex/catalog/parsing"); - const { existsSync } = await import("node:fs"); const owningCodexHome = getCodexHome(); const cacheGateSnapshot = deps.loadConfig(); const desiredDisabled = !shouldSyncCodexOnStart(cacheGateSnapshot); const invalidated = withCatalogWriteSerialization(owningCodexHome, permit => - invalidateCodexModelsCacheWithPermit(permit, owningCodexHome, { allowWhenDesiredDisabled: true })); + invalidateCodexModelsCacheWithPermitOutcome(permit, owningCodexHome, { allowWhenDesiredDisabled: true })); const cacheJson = cacheArgs.includes("--json"); const jsonSafeLog = cacheJson ? { log: (...values: unknown[]) => console.error(...values), error: (...values: unknown[]) => console.error(...values) } : console; // Only warn/restart when models_cache was actually rewritten from a readable catalog. - if (invalidated.kind === "completed" && invalidated.value) { + if (invalidated.kind === "completed" && invalidated.value === "written") { await handleRestartScopeAfterWrite(restartScope, jsonSafeLog); } else if (desiredDisabled && !cacheJson) { // Worth saying in the human path, because it explains why nothing was written. @@ -572,8 +570,8 @@ const commandRunners: Record = { "No catalog or cache write resulted.", )); } - // `completed` with a falsy value means the cache was NOT rewritten. Previously every - // outcome exited 0, so a script could not tell a refreshed cache from a skipped one. + // An identical cache is a successful no-op, not a failed refresh. Only a real write + // should restart Codex; a missing catalog or contended writer is also a benign skip. // // Losing the catalog write lock to another process is a skip, not a failure: // serialization working as designed is the expected outcome under concurrency, and a @@ -588,30 +586,25 @@ const commandRunners: Record = { // means the user asked for it regardless of the toggle. Treating OFF as automatic success // would report exit 0 and `skipped: true` for a refresh that actually failed. // - // But `invalidateCodexModelsCacheWithPermit` returns a bare boolean for four different - // situations -- wrote it, no catalog file exists, the OFF gate fired, or it threw -- so - // `false` alone cannot be read as failure either. `!existsSync(catalogPath)` is a - // legitimate nothing-to-do: with no catalog there is no cache to derive, which is the - // normal state of a fully native home and the case - // `codex-composed-acceptance.test.ts` pins at exit 0. It is checked here rather than by - // widening that function's return type, because its boolean is consumed by a dozen - // management routes that have no use for the distinction. - const wrote = invalidated.kind === "completed" && Boolean(invalidated.value); + // The detailed outcome distinguishes an unchanged cache from a failed rewrite while + // the boolean wrapper remains available to callers that only care whether bytes changed. + const wrote = invalidated.kind === "completed" && invalidated.value === "written"; + const unchanged = invalidated.kind === "completed" && invalidated.value === "unchanged"; const contended = invalidated.kind === "unavailable" && invalidated.reason === "busy"; - const noCatalog = !wrote && !existsSync(readCodexCatalogPathForHome(owningCodexHome)); - const ok = wrote || contended || noCatalog; + const noCatalog = invalidated.kind === "completed" && invalidated.value === "missing_catalog"; + const ok = wrote || unchanged || contended || noCatalog; if (cacheJson) { console.log(JSON.stringify({ schemaVersion: 1, ok, wrote, - skipped: contended || noCatalog, + skipped: unchanged || contended || noCatalog, outcome: invalidated.kind, // `outcome` alone cannot separate a contended lock from a hard serialization // failure -- both are `unavailable`. Carry the reason so a caller can. reason: invalidated.kind === "unavailable" ? invalidated.reason : undefined, // Which of the two benign skips this was, so `skipped: true` is never opaque. - skippedReason: contended ? "contended" : noCatalog ? "no_catalog" : undefined, + skippedReason: unchanged ? "unchanged" : contended ? "contended" : noCatalog ? "no_catalog" : undefined, desiredDisabled, codexHome: owningCodexHome, }, null, 2)); @@ -619,6 +612,8 @@ const commandRunners: Record = { console.log("Another process owns the catalog write; cache sync skipped."); } else if (noCatalog) { console.log("No Codex catalog to derive a cache from; nothing to sync."); + } else if (unchanged) { + console.log("Codex model cache is already current; nothing to sync."); } else if (!ok) { console.error(`Cache refresh did not complete (${invalidated.kind}). The Codex model cache was not rewritten.`); } diff --git a/src/codex/catalog/retained-sync.ts b/src/codex/catalog/retained-sync.ts index efd3a63413e..aab9ebc205d 100644 --- a/src/codex/catalog/retained-sync.ts +++ b/src/codex/catalog/retained-sync.ts @@ -649,11 +649,13 @@ export async function syncCatalogModels( }; } -export function invalidateCodexModelsCacheWithPermit( +export type CodexCacheInvalidationOutcome = "written" | "unchanged" | "missing_catalog" | "desired_disabled" | "failed"; + +export function invalidateCodexModelsCacheWithPermitOutcome( permit: CatalogWritePermit, owningCodexHome: string, options?: CodexCatalogSyncOptions, -): boolean { +): CodexCacheInvalidationOutcome { try { // This permit is a REACQUISITION: refreshCodexModelCatalog's commit released // K before this rewrite runs, so the commit-path desired-state check cannot @@ -661,9 +663,9 @@ export function invalidateCodexModelsCacheWithPermit( // routed cache write — re-read intent under this permit, same as the commit. // The catalog-only sync override applies here too so an explicit refresh // keeps the cache consistent with the catalog it just wrote. - if (!shouldSyncCodexOnStart(loadConfig()) && options?.allowWhenDesiredDisabled !== true) return false; + if (!shouldSyncCodexOnStart(loadConfig()) && options?.allowWhenDesiredDisabled !== true) return "desired_disabled"; const catalogPath = readCodexCatalogPathForHome(owningCodexHome); - if (!existsSync(catalogPath)) return false; + if (!existsSync(catalogPath)) return "missing_catalog"; const catalog = JSON.parse(readFileSync(catalogPath, "utf8")); const models = catalog.models ?? catalog; const cachePath = join(owningCodexHome, "models_cache.json"); @@ -711,14 +713,22 @@ export function invalidateCodexModelsCacheWithPermit( // `cacheSynced` mean what its name and its consumers already assume, and what // `pullRemoteCatalog` and the early returns in `refreshCodexModelCatalog` // already assert: a write happened. - if (!preparedBytesDifferFromDisk(preparedCache)) return false; + if (!preparedBytesDifferFromDisk(preparedCache)) return "unchanged"; replaceCodexModelsCache(permit, owningCodexHome, preparedCache); - return true; + return "written"; } catch { - return false; + return "failed"; } } +export function invalidateCodexModelsCacheWithPermit( + permit: CatalogWritePermit, + owningCodexHome: string, + options?: CodexCatalogSyncOptions, +): boolean { + return invalidateCodexModelsCacheWithPermitOutcome(permit, owningCodexHome, options) === "written"; +} + export function invalidateCodexModelsCache(options?: CodexCatalogSyncOptions): boolean { const owningCodexHome = getCodexHome(); const outcome = withCatalogWriteSerialization( diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 373bcb43779..83c233ef299 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -46,7 +46,7 @@ export { export { syncCatalogModels, invalidateCodexModelsCache, - invalidateCodexModelsCacheWithPermit, + invalidateCodexModelsCacheWithPermit, invalidateCodexModelsCacheWithPermitOutcome, } from "./retained-sync"; -export type { CodexCatalogSyncOptions } from "./retained-sync"; +export type { CodexCacheInvalidationOutcome, CodexCatalogSyncOptions } from "./retained-sync"; export { restoreCodexCatalog, restoreCodexCatalogWithPermit } from "./restore"; diff --git a/structure/catalog.md b/structure/catalog.md index 4379a35a891..16cd6cf782e 100644 --- a/structure/catalog.md +++ b/structure/catalog.md @@ -74,6 +74,8 @@ provider-wide fallback. Exact model output limits precede the provider default o native rows from the output without rewriting the pristine backup or unrelated snapshots; - invalidates `$CODEX_HOME/models_cache.json` when model visibility changes. +Cache invalidation reports an unchanged derived cache separately from a failed rewrite. `ocx sync-cache` treats identical bytes as a successful no-op, preserving the cache mtime and avoiding a needless app-server restart; malformed catalogs and write failures remain errors. + `src/codex/catalog/model-visibility.ts` also excludes models owned by disabled providers, including custom rows. `src/codex/catalog/routed-gather.ts` does not inherit provider configuration into custom rows while that provider is disabled. On the default `opencodex-catalog.json` path, sync deliberately uses two catalog sources: Codex's diff --git a/tests/codex-integration/codex-app-server-processes.test.ts b/tests/codex-integration/codex-app-server-processes.test.ts index 8f170d2f1ea..e36f91c22e0 100644 --- a/tests/codex-integration/codex-app-server-processes.test.ts +++ b/tests/codex-integration/codex-app-server-processes.test.ts @@ -823,8 +823,8 @@ describe("CLI /api sync wiring for stale app-servers (#476)", () => { // write actually landed, never on a refused/failed serialization attempt. expect(syncCacheCase).toContain("withCatalogWriteSerialization"); // #1931: explicit sync-cache refreshes even when injection is OFF (side profiles). - expect(syncCacheCase).toContain("invalidateCodexModelsCacheWithPermit(permit, owningCodexHome, { allowWhenDesiredDisabled: true })"); - const gate = 'if (invalidated.kind === "completed" && invalidated.value)'; + expect(syncCacheCase).toContain("invalidateCodexModelsCacheWithPermitOutcome(permit, owningCodexHome, { allowWhenDesiredDisabled: true })"); + const gate = 'if (invalidated.kind === "completed" && invalidated.value === "written")'; expect(syncCacheCase).toContain(gate); expect(syncCacheCase).toContain("handleRestartScopeAfterWrite"); expect(syncCacheCase.indexOf(gate)) diff --git a/tests/codex-integration/codex-composed-acceptance.test.ts b/tests/codex-integration/codex-composed-acceptance.test.ts index 8274ca9be2f..31b0e9a5fa4 100644 --- a/tests/codex-integration/codex-composed-acceptance.test.ts +++ b/tests/codex-integration/codex-composed-acceptance.test.ts @@ -551,11 +551,15 @@ describe("WP13 composed toggle acceptance", () => { expect(result.exitCode).toBe(0); expect(manifest(fx.codex)).toEqual(before); } - for (const argv of [["sync"], ["sync-cache"]]) { - const result = await fx.runCli(argv); - expect(result.exitCode).toBe(0); - expect(manifestWithoutCatalogArtifacts(manifest(fx.codex))).toEqual(manifestWithoutCatalogArtifacts(before)); - } + const synced = await fx.runCli(["sync"]); + expect(synced.exitCode).toBe(0); + expect(manifestWithoutCatalogArtifacts(manifest(fx.codex))).toEqual(manifestWithoutCatalogArtifacts(before)); + const unchangedCache = await fx.runCli(["sync-cache", "--json"]); + expect(unchangedCache.exitCode).toBe(0); + expect(JSON.parse(unchangedCache.stdout)).toMatchObject({ + ok: true, wrote: false, skipped: true, skippedReason: "unchanged", desiredDisabled: true, + }); + expect(manifestWithoutCatalogArtifacts(manifest(fx.codex))).toEqual(manifestWithoutCatalogArtifacts(before)); const sync = await fx.request(server.runtime, "/api/sync", { method: "POST" }); expect(sync.status).toBe(200); expect(sync.body).toMatchObject({ status: "skipped", skippedReason: "desired_disabled", ok: true }); diff --git a/tests/codex-integration/codex-models-cache-invalidate.test.ts b/tests/codex-integration/codex-models-cache-invalidate.test.ts index b644740e40e..c8264d50d0c 100644 --- a/tests/codex-integration/codex-models-cache-invalidate.test.ts +++ b/tests/codex-integration/codex-models-cache-invalidate.test.ts @@ -3,7 +3,7 @@ import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from import { tmpdir } from "node:os"; import { join } from "node:path"; import { invalidateCodexModelsCache } from "../../src/codex/catalog"; -import { invalidateCodexModelsCacheWithPermit } from "../../src/codex/catalog/sync"; +import { invalidateCodexModelsCacheWithPermit, invalidateCodexModelsCacheWithPermitOutcome } from "../../src/codex/catalog/sync"; import { withCatalogWriteSerialization } from "../../src/codex/catalog-write-serialization"; import { collectCodexAppServerCatalogStateForRequest, @@ -64,6 +64,23 @@ describe("invalidateCodexModelsCache write gate (#476 / #518)", () => { expect(cache.models).toEqual([{ slug: "gpt-5.5" }]); }); + test("distinguishes an unchanged cache from a failed refresh", () => { + writeFileSync(join(codexHome, "opencodex-catalog.json"), JSON.stringify({ + models: [{ slug: "gpt-5.5" }], + }, null, 2) + "\n"); + const invalidate = () => withCatalogWriteSerialization(codexHome, permit => + invalidateCodexModelsCacheWithPermitOutcome(permit, codexHome)); + + expect(invalidate()).toMatchObject({ kind: "completed", value: "written" }); + const cachePath = join(codexHome, "models_cache.json"); + const before = readFileSync(cachePath); + expect(invalidate()).toMatchObject({ kind: "completed", value: "unchanged" }); + expect(readFileSync(cachePath)).toEqual(before); + + writeFileSync(join(codexHome, "opencodex-catalog.json"), "{ not-json"); + expect(invalidate()).toMatchObject({ kind: "completed", value: "failed" }); + }); + test("permit-bound invalidation stays on its owning home after ambient drift", () => { const ambientCodexHome = mkdtempSync(join(tmpdir(), "ocx-invalidate-ambient-")); try {