diff --git a/src/codex/catalog-auto-refresh.ts b/src/codex/catalog-auto-refresh.ts index 6486b1fb1ff..5a8fe833f35 100644 --- a/src/codex/catalog-auto-refresh.ts +++ b/src/codex/catalog-auto-refresh.ts @@ -65,11 +65,17 @@ async function tick(): Promise { const entryGeneration = generation; try { const { + armDetachedConfigBaseline, loadConfig, isCatalogAutoRefreshEnabled, resolveCatalogAutoRefreshIntervalMs, } = await import("../config"); const config = loadConfig(); + // Convergence can persist model-discovery fields after awaiting provider /models. + // Arm this independently loaded snapshot as detached so the save rebases every + // field — listener binding and disk-only keys included — against what is on disk + // by then, and concurrent hand edits survive the tick. + armDetachedConfigBaseline(config); if (!isCatalogAutoRefreshEnabled(config)) return; const configured = resolveCatalogAutoRefreshIntervalMs(config); // 0 is dormant: the section stays configured but this tick must not converge, diff --git a/src/config.ts b/src/config.ts index 0c63a168bad..e12283aeb1d 100644 --- a/src/config.ts +++ b/src/config.ts @@ -133,7 +133,7 @@ export { withExpectedConfigGenerationSync, } from "./config/mutation-lock"; export { - armClaudeCodeBaseline, + armClaudeCodeBaseline, armDetachedConfigBaseline, adoptPersistedProviderIntoLiveConfig, claudeCodeBaselineArmed, reconcileLiveConfigFromDisk, diff --git a/src/config/live-reconcile.ts b/src/config/live-reconcile.ts index 8715b1f146b..b5e2d38721b 100644 --- a/src/config/live-reconcile.ts +++ b/src/config/live-reconcile.ts @@ -47,6 +47,16 @@ type PersistedServerBinding = Pick; const persistedLiveServerBinding = new WeakMap(); +/** + * Config instances nobody holds long-term — the catalog auto-refresh tick's + * per-tick `loadConfig()` snapshot. A detached snapshot cannot express a + * deliberate deletion and owns no live listener socket, so the live policy's + * `hostname`/`port` and disk-only-key skips would only discard concurrent hand + * edits wholesale. Keyed on the instance so the mode cannot leak into the + * long-lived server config. + */ +const detachedConfigSnapshots = new WeakSet(); + /** * Arm the baseline for a long-lived config. MANDATORY at `startServer`, not lazy on * first save — arming lazily would lose exactly the hand edit made before that first @@ -57,6 +67,17 @@ export function armClaudeCodeBaseline(config: OcxConfig): void { claudeCodeBaseline.set(config, structuredClone(config.claudeCode)); } +/** + * Arm a freshly loaded config instance no long-lived server owns. The save path + * then reconciles every field — the listener binding and keys that exist only + * on disk included — against this arming baseline, so a concurrent hand edit is + * adopted rather than overwritten by the snapshot's stale values. + */ +export function armDetachedConfigBaseline(config: OcxConfig): void { + armClaudeCodeBaseline(config); + detachedConfigSnapshots.add(config); +} + /** * Adopt one schema-validated provider that was read from the authoritative disk * config into a long-lived server config without rebasing any unrelated field. @@ -124,6 +145,42 @@ type IndexedCustomModels = { byId: Map>; }; +function indexDisabledModels(value: ConfigMergeValue): { order: string[]; members: Set } | null { + if (value === MISSING_CONFIG_VALUE) return { order: [], members: new Set() }; + if (!Array.isArray(value)) return null; + const members = new Set(); + for (const item of value) { + if (typeof item !== "string" || members.has(item)) return null; + members.add(item); + } + return { order: value as string[], members }; +} + +/** + * Merge disabled-model lists by membership instead of treating the array as one + * opaque leaf: a slug either side deleted stays deleted (deletion wins over the + * other side's unchanged retention) while slugs added on either side are kept. + * Discovery only appends, so live additions are its arrivals; a hand edit that + * hid or un-hid a model mid-refresh survives the discovery save. + */ +function reconcileDisabledModels( + baseline: ConfigMergeValue, + live: ConfigMergeValue, + persisted: ConfigMergeValue, +): ConfigMergeValue | null { + const baselineSet = indexDisabledModels(baseline); + const liveSet = indexDisabledModels(live); + const persistedSet = indexDisabledModels(persisted); + if (!baselineSet || !liveSet || !persistedSet) return null; + const order = [...liveSet.order, ...persistedSet.order.filter(id => !liveSet.members.has(id))]; + const merged: string[] = []; + for (const id of order) { + if (baselineSet.members.has(id) && (!liveSet.members.has(id) || !persistedSet.members.has(id))) continue; + merged.push(id); + } + return merged; +} + function indexCustomModels(value: ConfigMergeValue): IndexedCustomModels | null { if (!Array.isArray(value)) return null; const order: string[] = []; @@ -192,7 +249,10 @@ function reconcileConfigRecord( : key === "customModels" ? reconcileCustomModels(baselineValue, liveValue, persistedValue) ?? reconcileConfigValue(baselineValue, liveValue, persistedValue) - : reconcileConfigValue(baselineValue, liveValue, persistedValue, key === "providers"); + : key === "disabledModels" + ? reconcileDisabledModels(baselineValue, liveValue, persistedValue) + ?? reconcileConfigValue(baselineValue, liveValue, persistedValue) + : reconcileConfigValue(baselineValue, liveValue, persistedValue, key === "providers"); if (merged === MISSING_CONFIG_VALUE) delete live[key]; else live[key] = merged; } @@ -302,7 +362,11 @@ function readPersistedServerBinding( } /** - * The save entry point for every writer holding a LIVE server config. + * The save entry point for every writer holding a LIVE server config, and for a + * detached snapshot armed through {@link armDetachedConfigBaseline}. The live + * policy keeps `hostname`/`port` and disk-only keys out of the merge; a detached + * snapshot has neither hazard, so it rebases every field against its arming + * baseline instead. * * Conflict policy, chosen deliberately: * - disk changed, we did not → their hand edit wins; @@ -312,8 +376,9 @@ function readPersistedServerBinding( * live state edited that same row; * - file missing/unreadable → save what we have, no throw. * - * Custom-model rows are merged by their stable `id`, preserving independent - * edits and deletions across stale whole-config saves. + * Custom-model rows are merged by their stable `id`, and `disabledModels` by + * member, preserving independent edits and deletions across stale whole-config + * saves. */ export function saveConfigPreservingClaudeCode(config: OcxConfig): void { const pinError = configReasoningPinsConfigError(config); @@ -327,28 +392,41 @@ export function saveConfigPreservingClaudeCode(config: OcxConfig): void { if (baseline && onDisk !== undefined) { const persistedDiagnostics = configDiagnosticsFromRaw(JSON.stringify(onDisk)); if (persistedDiagnostics.source === "file") { - const deletedKeys = configRebaseDeletionKeys(config); - const provenanceExists = configHasRebaseProvenance(config); - // Only keys this live config is actually known to have diverged on may be - // rebased. The baseline is captured once when the server arms it, so any key - // that appeared on disk afterwards — through saveConfig(), a hand edit, or - // another process — is absent from the baseline as well as from the live - // config. Reconciling those keys reads "live never changed this" and adopts - // the disk value, which resurrects a field the live writer had deliberately - // deleted (#1462 regression: PUT /api/grok/selection with an empty list). - // Restrict the merge to keys the baseline knew about, plus keys the live - // config still carries; a key that exists only on disk is left to the - // ordinary whole-config write below. - const rebaseableKeys = new Set([ - ...Object.keys(baseline as unknown as Record), - ...Object.keys(config as unknown as Record), - ...(provenanceExists - ? Object.keys(persistedDiagnostics.config as unknown as Record) - : []), - ]); - const skipped = new Set(["hostname", "port", "claudeCode", CONFIG_REBASE_PROVENANCE_KEY]); - for (const key of Object.keys(persistedDiagnostics.config as unknown as Record)) { - if (!rebaseableKeys.has(key)) skipped.add(key); + // A detached snapshot diverged only where this pipeline mutated it (model + // discovery fields for the auto-refresh tick): it cannot express a + // deliberate deletion, so the disk-only-key and listener-binding skips + // below would only discard concurrent hand edits. It merges every top-level + // key — including configRebaseProvenance, so a cooperating writer's + // deletion marker adopted from disk is honored instead of silently dropped — + // and reads deletion intent AFTER the merge, from the marker disk actually + // carries now rather than a stale one the snapshot loaded. + const detached = detachedConfigSnapshots.has(config); + const deletedKeys = detached ? null : configRebaseDeletionKeys(config); + const skipped = detached + ? new Set(["claudeCode"]) + : new Set(["hostname", "port", "claudeCode", CONFIG_REBASE_PROVENANCE_KEY]); + if (!detached) { + const provenanceExists = configHasRebaseProvenance(config); + // Only keys this live config is actually known to have diverged on may be + // rebased. The baseline is captured once when the server arms it, so any key + // that appeared on disk afterwards — through saveConfig(), a hand edit, or + // another process — is absent from the baseline as well as from the live + // config. Reconciling those keys reads "live never changed this" and adopts + // the disk value, which resurrects a field the live writer had deliberately + // deleted (#1462 regression: PUT /api/grok/selection with an empty list). + // Restrict the merge to keys the baseline knew about, plus keys the live + // config still carries; a key that exists only on disk is left to the + // ordinary whole-config write below. + const rebaseableKeys = new Set([ + ...Object.keys(baseline as unknown as Record), + ...Object.keys(config as unknown as Record), + ...(provenanceExists + ? Object.keys(persistedDiagnostics.config as unknown as Record) + : []), + ]); + for (const key of Object.keys(persistedDiagnostics.config as unknown as Record)) { + if (!rebaseableKeys.has(key)) skipped.add(key); + } } reconcileConfigRecord( config as unknown as Record, @@ -356,7 +434,9 @@ export function saveConfigPreservingClaudeCode(config: OcxConfig): void { persistedDiagnostics.config as unknown as Record, skipped, ); - for (const key of deletedKeys) delete (config as unknown as Record)[key]; + for (const key of deletedKeys ?? configRebaseDeletionKeys(config)) { + delete (config as unknown as Record)[key]; + } } } if (claudeCodeBaseline.has(config)) { diff --git a/structure/config.md b/structure/config.md index 12cf9acbd10..a1c9c1c849f 100644 --- a/structure/config.md +++ b/structure/config.md @@ -521,7 +521,7 @@ being treated as a text model by one and an image target by the other. ## Catalog auto-refresh -`catalogAutoRefresh` on `src/types/config.ts` stores an optional `enabled` / `intervalMinutes` section that defaults off: an absent key, an explicit false, and a malformed value all leave the scheduler dormant. `src/config/feature-flags.ts` resolves the cadence; an explicit `intervalMinutes: 0` keeps the unref'd timer idle, and any other value is clamped up to 15 minutes because upstream `/models` caches have not moved below that and a shorter tick only multiplies rate-limit exposure. `src/codex/catalog-auto-refresh.ts` is the module-singleton interval `src/server/background-lifecycle.ts` starts beside the quota reset poller; a tick that is enabled and non-dormant drives the same catalog-only converge funnel management mutations drive. The last-outcome record lives in `src/codex/catalog-refresh-status.ts` (when the tick finished, the normalized `CatalogDisposition`, whether the served model set changed, consecutive failures) and carries no provider or account detail. +`catalogAutoRefresh` on `src/types/config.ts` stores an optional `enabled` / `intervalMinutes` section that defaults off: an absent key, an explicit false, and a malformed value all leave the scheduler dormant. `src/config/feature-flags.ts` resolves the cadence; an explicit `intervalMinutes: 0` keeps the unref'd timer idle, and any other value is clamped up to 15 minutes because upstream `/models` caches have not moved below that and a shorter tick only multiplies rate-limit exposure. `src/codex/catalog-auto-refresh.ts` is the module-singleton interval `src/server/background-lifecycle.ts` starts beside the quota reset poller; a tick that is enabled and non-dormant drives the same catalog-only converge funnel management mutations drive. Each tick arms its independently loaded config snapshot as a detached baseline before provider work, so the discovery save rebases every field — the listener binding and sections absent from the snapshot included — against the disk state at save time and concurrent hand edits survive the tick — `disabledModels` merges by member, so an overlapping visibility edit survives alongside the discovery additions. The last-outcome record lives in `src/codex/catalog-refresh-status.ts` (when the tick finished, the normalized `CatalogDisposition`, whether the served model set changed, consecutive failures) and carries no provider or account detail. ## Aggregate request metrics export diff --git a/tests/codex-integration/catalog-auto-refresh-scheduler.test.ts b/tests/codex-integration/catalog-auto-refresh-scheduler.test.ts index 04508442da7..ffee9de891a 100644 --- a/tests/codex-integration/catalog-auto-refresh-scheduler.test.ts +++ b/tests/codex-integration/catalog-auto-refresh-scheduler.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; -import { mkdtempSync, writeFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -18,7 +18,9 @@ import { CATALOG_AUTO_REFRESH_MIN_INTERVAL_MS, getConfigPath, getDefaultConfig, + saveConfigPreservingClaudeCode, } from "../../src/config"; +import type { OcxConfig } from "../../src/types"; import { installIsolatedCodexHome, type IsolatedCodexHome, @@ -35,7 +37,7 @@ let previousOpenCodexHome: string | undefined; let openCodexHome = ""; let isolatedCodexHome: IsolatedCodexHome | null = null; let convergeFactoryCalls = 0; -let convergeImpl: () => Promise = async () => COMMITTED_CATALOG_ONLY; +let convergeImpl: (config: OcxConfig) => Promise = async () => COMMITTED_CATALOG_ONLY; let convergeSpy: { mockRestore(): void } | null = null; let releaseHanging: ((outcome: CatalogOnlyOutcome) => void) | null = null; let pendingTick: Promise | null = null; @@ -68,9 +70,9 @@ beforeEach(() => { pendingTick = null; // The tick's only converge seam is a dynamic import of management-convergence. // Stub it so an enabled fixture cannot spend a live /models call or rewrite the catalog. - convergeSpy = spyOn(managementConvergence, "createManagementConvergeCodex").mockImplementation(() => { + convergeSpy = spyOn(managementConvergence, "createManagementConvergeCodex").mockImplementation((config) => { convergeFactoryCalls += 1; - return convergeImpl; + return () => convergeImpl(config); }); }); @@ -168,6 +170,48 @@ describe("catalog auto-refresh scheduler", () => { expect(lastCatalogAutoRefreshOutcome()).toBeNull(); }); + test("a tick preserves a config hand edit made while convergence is in flight", async () => { + writeCatalogAutoRefreshConfig({ enabled: true, intervalMinutes: 60 }); + convergeImpl = async (config) => { + const onDisk = JSON.parse(readFileSync(getConfigPath(), "utf8")) as OcxConfig; + onDisk.catalogAutoRefresh = { enabled: false, intervalMinutes: 60 }; + writeFileSync(getConfigPath(), JSON.stringify(onDisk), "utf8"); + saveConfigPreservingClaudeCode(config); + return COMMITTED_CATALOG_ONLY; + }; + + await runCatalogAutoRefreshTickForTests(); + + const persisted = JSON.parse(readFileSync(getConfigPath(), "utf8")) as OcxConfig; + expect(persisted.catalogAutoRefresh?.enabled).toBe(false); + }); + + test("a tick preserves listener and newly added fields edited while convergence is in flight", async () => { + // The general live-save policy leaves hostname/port and disk-only keys out of + // the rebase. For the tick's detached snapshot those skips would discard the + // hand edit wholesale, so arming it detached reconciles every field instead. + writeCatalogAutoRefreshConfig({ enabled: true, intervalMinutes: 60 }); + convergeImpl = async (config) => { + const onDisk = JSON.parse(readFileSync(getConfigPath(), "utf8")) as OcxConfig; + onDisk.port = 10101; + onDisk.hostname = "127.0.0.2"; + onDisk.metricsExport = { enabled: true }; + writeFileSync(getConfigPath(), JSON.stringify(onDisk), "utf8"); + // The same save convergeCodexCatalog performs after mutating discovery fields. + config.disabledModels = ["xai:grok-0"]; + saveConfigPreservingClaudeCode(config); + return COMMITTED_CATALOG_ONLY; + }; + + await runCatalogAutoRefreshTickForTests(); + + const persisted = JSON.parse(readFileSync(getConfigPath(), "utf8")) as OcxConfig; + expect(persisted.port).toBe(10101); + expect(persisted.hostname).toBe("127.0.0.2"); + expect(persisted.metricsExport?.enabled).toBe(true); + expect(persisted.disabledModels).toEqual(["xai:grok-0"]); + }); + test("an overlapping tick returns immediately without a second converge", async () => { writeCatalogAutoRefreshConfig({ enabled: true, intervalMinutes: 60 }); diff --git a/tests/config/config-user-edits.test.ts b/tests/config/config-user-edits.test.ts index 38ac41aa378..92b59bb71e9 100644 --- a/tests/config/config-user-edits.test.ts +++ b/tests/config/config-user-edits.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { armClaudeCodeBaseline, + armDetachedConfigBaseline, adoptPersistedProviderIntoLiveConfig, deleteConfigTopLevelKey, getConfigPath, @@ -908,3 +909,55 @@ test("a malformed upstreamHostCircuitThreshold hand edit disables only the circu ); expect(diagnostics.config.providers.test).toBeDefined(); }); + +// A detached snapshot — the catalog auto-refresh tick's per-tick loadConfig() — +// owns no live listener and cannot express a deletion of its own, so every field +// rebases: a concurrent hand edit to the binding or to a key the snapshot never +// held is adopted rather than overwritten by the snapshot's stale values. +test("a detached snapshot save adopts concurrent listener and disk-only hand edits", () => { + const snapshot = loadConfig(); + armDetachedConfigBaseline(snapshot); + // Discovery mutates only its own surfaces on the snapshot. + snapshot.disabledModels = ["test/retired"]; + writeDiskConfig({ + port: 10101, + hostname: "127.0.0.2", + metricsExport: { enabled: true }, + claudeCode: { authMode: "proxy" }, + }); + + saveConfigPreservingClaudeCode(snapshot); + + const disk = diskConfig(); + expect(disk.port).toBe(10101); + expect(disk.hostname).toBe("127.0.0.2"); + expect(disk.metricsExport).toEqual({ enabled: true }); + expect((disk.claudeCode as Record).authMode).toBe("proxy"); + expect(disk.disabledModels).toEqual(["test/retired"]); +}); + +test("a detached snapshot save merges concurrent disabledModels edits by member", () => { + writeDiskConfig({ disabledModels: ["test/seeded"] }); + const snapshot = loadConfig(); + armDetachedConfigBaseline(snapshot); + // Discovery only appends, so the snapshot's extra slug is its arrival. + snapshot.disabledModels = ["test/seeded", "test/discovered"]; + // The operator's mid-flight edit both hides a new slug and un-hides the seeded one. + writeDiskConfig({ disabledModels: ["test/hand-hidden"] }); + + saveConfigPreservingClaudeCode(snapshot); + + expect(diskConfig().disabledModels).toEqual(["test/discovered", "test/hand-hidden"]); +}); + +test("a live save merges concurrent disabledModels edits by member", () => { + writeDiskConfig({ disabledModels: ["test/seeded"] }); + const live = loadConfig(); + armClaudeCodeBaseline(live); + live.disabledModels = ["test/seeded", "test/live-hidden"]; + writeDiskConfig({ disabledModels: ["test/seeded", "test/hand-hidden"] }); + + saveConfigPreservingClaudeCode(live); + + expect(diskConfig().disabledModels).toEqual(["test/seeded", "test/live-hidden", "test/hand-hidden"]); +});