fix(codex): warn when a startup write leaves an app-server stale (#1046) - #1072
fix(codex): warn when a startup write leaves an app-server stale (#1046)#1072lidge-jun wants to merge 1 commit into
Conversation
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.
📝 WalkthroughWalkthroughStartup synchronization now reports catalog and cache writes. Server startup records cache rewrites. The CLI invokes a failure-safe stale Codex app-server check after startup writes and logs one warning for stale processes. ChangesStartup stale app-server handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Startup
participant CodexSync
participant CacheState
participant StaleCheck
participant AppServers
Startup->>CodexSync: run startup synchronization
CodexSync-->>Startup: return catalog/cache write outcomes
Startup->>CacheState: consume cache rewrite status
Startup->>StaleCheck: check after a startup write
StaleCheck->>AppServers: collect process catalog state
AppServers-->>StaleCheck: return stale or non-stale state
StaleCheck-->>Startup: return warned status
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/codex-desired-state.test.ts`:
- Around line 195-204: Add a focused test near the existing
syncCodexOnStartIfEnabled tests where the callback returns { catalogWritten:
true, cacheSynced: true }, then assert the function’s result preserves both
flags. Keep the existing undefined and thrown-callback coverage unchanged and
verify the positive outcome directly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 537753c7-5e2e-4150-aeef-0f637f0a2bc8
📒 Files selected for processing (6)
src/cli/index.tssrc/codex/app-server-processes.tssrc/codex/desired-state.tssrc/server/index.tstests/codex-app-server-processes.test.tstests/codex-desired-state.test.ts
| 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; }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add positive write-outcome coverage.
These tests only cover undefined and thrown synchronization callbacks. Add a callback that returns { catalogWritten: true, cacheSynced: true }. Assert that syncCodexOnStartIfEnabled preserves both flags.
Without this test, a false-valued mapping can disable the startup warning gate while the changed tests still pass.
As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-desired-state.test.ts` around lines 195 - 204, Add a focused test
near the existing syncCodexOnStartIfEnabled tests where the callback returns {
catalogWritten: true, cacheSynced: true }, then assert the function’s result
preserves both flags. Keep the existing undefined and thrown-callback coverage
unchanged and verify the positive outcome directly.
Source: Path instructions
|
CI on this PR just reproduced #1061 — and it is not caused by this change. The macOS leg failed on one test: That line is the exact defect #1061 reports and #1071 fixes: the test waits for the settled file to exist, then immediately parses it, so a partially written document satisfies the wait and throws. #1071 replaces that with a wait for parseable content and makes the child publish atomically. Run: 9018 pass / 8 skip / 1 fail. The single failure is the flake, on the macOS leg, which is where #1061 says it shows up. So this is load-dependent CI noise against an unrelated pre-existing defect. Two ways forward, whichever the maintainers prefer:
I have not rebased this branch onto #1071, because chaining an app-server fix behind a test-harness fix would create a dependency that does not exist in the code. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 254db138cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const first = collectCodexAppServerCatalogState(); | ||
| const second = collectCodexAppServerCatalogState(); |
There was a problem hiding this comment.
On slower Windows/macOS CI this new test can become flaky because it calls the real default collector twice and asserts object identity. The collector records the cache timestamp before the cold process scan and only reuses the memo for 5s; its own default path can spend up to ~8s+5s on Windows or ~5s+3s on macOS, so by the time second is collected the cache may already be expired and this recomputes a different object. Use injected time/fake IO or otherwise control the TTL boundary instead of depending on wall-clock duration.
AGENTS.md reference: AGENTS.md:L136-L137
Useful? React with 👍 / 👎.
| if (consumeStartupCacheInvalidationWrite() || startupSync.catalogWritten || startupSync.cacheSynced) { | ||
| const { warnIfStaleCodexAppServersAfterStartupWrite } = await import("../codex/app-server-processes"); | ||
| warnIfStaleCodexAppServersAfterStartupWrite({ log: console }); |
There was a problem hiding this comment.
When startup is driven through ocx ensure, this warning only runs inside the detached ocx start child, which is spawned with stdio: "ignore"; the parent then returns as soon as /healthz responds and runs its own syncModelsToCodex(port) without any stale-app-server handler. In that shim/autostart path the warning is either discarded or can run before the parent writes the final catalog, so users can still be left with a stale Codex picker and no actionable message. Mirror this post-write warning around the parent-side ensure sync, or avoid the duplicate parent write.
Useful? React with 👍 / 👎.
1 participant
Summary
Service startup rewrites the Codex catalog and the models cache, but an app-server that booted earlier keeps its own in-memory model list — Codex builds a static models 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 with a two-host comparison: their catalog was rewritten 4m27s after the app-server started, and killing the app-server made the routed models reappear.
afterCatalogWriteHandleAppServers()already exists, but only runs from explicitocx sync/sync-cache. Startup never called it.The obvious fix is unsafe, and that shaped this one. That handler's
restart: truebranch SIGTERMs matching app-servers and says in its own log line that active turns may be interrupted. Wiring it into an unattended boot would kill a user's in-flight turn on every login, repair, or update. A human typing--restart-codexis consenting to that; a boot is not.So this adds a warning-only counterpart that cannot reach the signal path — asserted by an injected
killthat fails the test if it is ever called. It also asks the existing mtime classifier whether an app-server is genuinely stale, rather than warning about any running one, so a boot with Codex open and a current catalog stays quiet.Two startup sites write, and neither warns on its own:
startServer()invalidates the models cache during boot.handleStartruns the catalog sync moments later.handleStartORs their results and warns once, after both. Warning after the first would read a catalog mtime the second is about to move, and two independent calls would warn twice. Making that observable required typing the startup sync seam, which had been discardingsyncModelsToCodex()'s result and returning a bare boolean meaning "the integration was enabled" — so "a write happened" was not observable at the startup boundary at all.The memoized catalog state is dropped before the check. It caches for 5s on the default path, so a
freshreading taken before the write would otherwise be replayed after it and this would stay silent about the staleness it just created.Failure is swallowed throughout: startup synchronization is best-effort and must not stop the proxy from coming up.
Closes #1046.
Verification
Red-green, per branch rather than per suite:
stalefails the quiet-path test.catchfails the swallowed-failure test.killassertion passes only because no startup path reachesrestartCodexAppServers().One honest limit recorded rather than papered over: the 5s memo is only consulted when every io field is defaulted, so an io-injected test bypasses it and cannot reproduce the masking. Rather than write a test that looks like proof and cannot fail, the memo/invalidation mechanism the fix depends on is asserted directly.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests