Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions devlog/_plan/260914_l2_pool_routing_cache/000_unit.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# 260914 L2 — pool routing prompt-cache preservation and honest routing status

Lane R1-L2 of the 260914 delivery round. One pull request against `dev` from
`codex/260914-l2-pool-routing-cache`, closing two issues that both come down to the
same thing: the pool tells the operator one story and does another.

- #4546 — quota-strategy account rotation moves a **bound** conversation mid-thread, so the
account-isolated prompt-cache prefix is discarded on every turn once the pool is hot.
- #4550 — `ocx status` prints `routing=opencodex-local` read from config on disk, which is the
*configured* route, not the route an already-running Codex client actually adopted.

## Write scope

Permitted: `src/codex/routing.ts`, the account-pool / session-affinity code, a new
`src/codex/routing-adoption.ts` leaf, `src/codex/native-profile-processes.ts`,
`src/codex/autostart-health.ts` wiring, their tests, the docs-site configuration reference,
`structure/providers/openai-tiers.md`, and this unit.

`structure/providers/openai-tiers.md` is not optional: `structure/manifest.json` lists it as a
doc for `src/codex/`, and `structure/AGENTS.md` makes changing an owned source area oblige the
same change to update its doc. `bun run structure:check` is wired into the suite by
`tests/ci-workflows/structure-ssot.test.ts`, so ownership here is enforced, not advisory.

Tests EXTEND existing subsystem files rather than adding new ones. A new test file would also
require entries in `scripts/test-layout/layout.json` and
`tests/fixtures/test-layout-expected.json`, and `tests/test-layout.test.ts` enforces that.

Excluded, owned by concurrent lanes: `src/providers/devin*`, `src/providers/antigravity*`,
`src/server/responses/*`, `src/codex/catalog/*`, `src/adapters/cursor/*`, `gui/`.

## Verification posture

Local suite, typecheck, install and GUI build are **not run** for this unit by explicit
instruction. Proof is hosted CI at the exact final head SHA and nothing else. The pull
request states that posture in its Verification section rather than implying a local green.

## Roadmap

| Doc | Work phase | Outcome |
| --- | --- | --- |
| `010_cache_safe_rebind.md` | wp1 | A live binding only moves to an account with real headroom (#4546) |
| `020_routing_adoption.md` | wp2 | Status separates configured routing from adopted routing (#4550) |
| `030_delivery.md` | wp3 | One template-filled PR, hosted CI green at the exact final head |

Implementation is delegated to subagents on `devin/swe-2` and `xai/grok-4.6` at a 2:3 ratio,
each with a disjoint write scope so two writers never hold the same file.

## Review record

An independent reviewer audited this roadmap before implementation and returned FAIL with three
blocking findings, all folded in: the planned Codex-client process set could not see a CLI process
at all, `adopted` was not sound as written, and the write scope omitted the `structure/` doc that
owns `src/codex/`. The `010` diagnosis was confirmed correct on every point.
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# wp1 — a bound thread may only move to an account that has headroom (#4546)

## What the code does today

`resolveCodexAccountForThreadDetailed` reuses a live thread binding and then calls
`reevaluateAffinityQuota`. Under the `quota` strategy that helper computes the bound
account's usage score and asks `mayRebindAffinityForQuota`, whose default answer is
`usage >= autoSwitchThreshold` (80). When that is true it calls `pickLowerUsageAccount`,
which returns whichever eligible account is **strictly cooler** — by any margin at all.

Two consequences, both reported:

1. Because `mayRebind` is also what short-circuits the 60 s re-score interval, a thread in the
80–100 % band is re-scored on **every request**, not once a minute.
2. "Strictly cooler" has no floor. Once every account sits at 95–99 %, the coolest is still
over the threshold, so the thread is handed from account to account on consecutive turns.

Codex prompt caches are tenant-isolated, so each hop starts from a cold prefix. The reporter
measured a 7k-token turn becoming a 150k-token turn, 1.9 B tokens across 15,607 requests in
about 13 hours on five accounts.

## The rule to add

A live binding may only be moved to an account that has **genuine quota headroom** — the same
bar `resetFirstAffinityReplacement` already applies for the `reset-first` strategy via
`hasCodexQuotaHeadroom`. Quota strategy is the outlier, and that asymmetry is the defect.

Consequences of the new rule, which are what the regression test pins:

- Every account over the threshold ⇒ no candidate has headroom ⇒ the thread stays put and keeps
its cache. There is nothing to win by moving: the destination is as hot as the origin.
- A cool account exists ⇒ the thread still moves, exactly once, and lands somewhere it can stay.
Movement is now bounded by the number of accounts rather than by the number of turns.
- Nothing changes for an **unbound** request: cascading fresh single-turn work onto the coolest
account is correct, because there is no warm prefix to lose.
- Release paths are untouched. `hasUnrecoveredCodexQuotaRefusal` (429/402) still outranks every
affinity preference, `shouldFailover` still applies, and an exhausted or unusable account still
loses the binding. The rule narrows a *preference*, never a refusal.
- One correction from review: a known score of 100 with **no** recorded refusal is not by itself a
release path today, and this change does not make it one. Such a thread stays while its account is
still selectable, and surrenders the binding as soon as a sibling with headroom exists. Stickiness
until the account actually refuses is intended, so the regression test asserts that and not the
stronger claim.

## Where it goes

The rule exists in two places that the suite asserts answer identically, so both change together:

- `reevaluateAffinityQuota` — the live resolve path.
- `previewReusableAffinityAccount` — the side-effect-free preview used for subagent fallback.

Headroom alone is not sufficient, because `hasCodexQuotaHeadroom` deliberately answers **true**
for an account whose usage is unknown — unknown-means-selectable is the right default for an
unbound request. It is the wrong bet for a bound one: trading a warm prefix for an unmeasured
account is a guess, not an improvement. So the candidate must clear both bars, headroom **and**
strictly lower usage than the bound account. `CODEX_UNKNOWN_USAGE_SCORE` is 101, so an
unobserved account can never be strictly cooler than a known over-threshold score and the second
bar excludes it without a special case. An unknown-usage *bound* account never rebinds today
either, because `mayRebindAffinityForQuota` requires a known score.

## Regression test

Next to the existing pool-rotation tests in `tests/codex-integration/`. Three cases:

1. All accounts over the threshold: the bound thread's account is unchanged across repeated
resolves — the ping-pong case, which fails before the fix.
The scores must be UNEQUAL (95 / 90 / 97). Equal scores would not move even before the fix, so an
equal-score fixture would pass for the wrong reason and prove nothing.
2. One account below the threshold: the bound thread moves to it once, then stays.
3. Preview agrees with resolve in both situations.

`pickLowerUsageAccount` itself must not change: it is shared with `applyQuotaAutoSwitch` and the
unbound selection path, so the new bar belongs at the two bound-thread call sites only.
120 changes: 120 additions & 0 deletions devlog/_plan/260914_l2_pool_routing_cache/020_routing_adoption.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
# wp2 — configured routing is not adopted routing (#4550)

## What the report establishes

A Codex CLI thread kept returning `usage_limit_exceeded` while a healthy secondary account sat
in the pool. Thread-correlated diagnostics show those turns dialing
`wss://chatgpt.com/backend-api/codex/responses` directly, with no matching proxy usage record,
while `ocx status` reported `routing=opencodex-local`. The reporter's own leading hypothesis is
retained pre-injection configuration: the process started before the route was written and has
been holding the old one ever since.

The report asks for either transport interception or an honest status. Interception is not
available to us — a client process that already resolved its endpoint is beyond the proxy's
reach, and restarting it is the operator's call. So the defect we can actually fix is the
status: it presents a fact about **config on disk** as a fact about **live traffic**.

## What status knows today

`getCodexRoutingKind()` classifies `~/.codex/config.toml` and `deriveStartupHealth` turns
`opencodex-local` into `routingInjected: true`. `formatStartupRoutingDetail` prints
`routing=<kind>, service=…, shim=…`. Every input is a file read. No part of that chain can
distinguish a client that adopted the route from one that predates it.

## The evidence we do have

Both halves already exist in this repository:

- **When the route was written.** `src/codex/journal.ts` records our injection and stamps it.
Review pinned down why the newer of two readings is required: `Journal.timestamp` is the
*native snapshot* time and is not refreshed on re-injection, `writeJournal` no-ops when config is
already injected, and `markJournalInjectedState` rewrites the file — moving its mtime — without
touching `timestamp`. So the bound is `max(journal mtime, recorded timestamp)`. The new leaf
parses `JOURNAL_PATH` itself rather than calling the private `readJournal`, because that helper
can delete a corrupt journal and a status read must never mutate state.
- **When each client started.** `src/codex/app-server-processes.ts` already enumerates
processes cross-platform and reads start times (`readProcessStartMs`,
`readProcessStartMsBatch`, `/proc/<pid>/stat` on Linux, `ps -o lstart` on macOS,
`Win32_Process.CreationDate` on Windows), and its `ProcessSnapshot` already carries an
optional `startedAtMs`.
Correction from review: that field is declared on the type but the enumerators never populate it.
Start times come from `readProcessStartMsBatch`, which is how `collectCodexAppServerCatalogState`
already does it.
- **Which processes are Codex clients.** `src/codex/native-profile-processes.ts` carries the
matching rules — direct `codex` basenames plus interpreter-wrapped `node|bun codex.js`
entrypoints — but they are private and reachable only through a *count*. The count is enough to
answer "is Codex busy" and not enough to name a stale PID, so those rules are extracted into an
exported predicate and the existing counter is rewired through it. Copying them into a second
module is how `#2457` happened; one predicate, two callers.

Review corrected the lister, and this was the roadmap's worst error:
`listCodexAppServerProcesses` must **not** be the client set. It matches `app-server` and
`codex-code-mode-host` command lines only, and #4550 is a **CLI** process, so using it would make
`adopted` vacuously true — the same false reassurance the issue reports.
`probeNativeCodexProcesses` cannot stand in either: it is async and returns a count, while
`collectStartupHealth` is synchronous. So the extracted predicate comes with a **synchronous**
CLI lister returning `{ pid, commandLine }`.

Round 2 narrowed that further: "Windows cannot enumerate" is true only of
`windowsProcessCount`'s `@($items).Count`, not of the platform. All three snapshot listers in
`app-server-processes.ts` are already synchronous and already return `{ pid, commandLine }` —
`listUnixProcSnapshots` reads `/proc`, `listDarwinSnapshots` and `listWindowsSnapshots` use
`execFileSync` — and the Windows pre-filter `WINDOWS_CODEX_BASENAME_CANDIDATE_RE` already admits
CLI `codex.exe`/`codex.cmd` lines, with `isCodexAppServerCommandLine` applied only afterwards.
So the lister filters those snapshots with the extracted predicate instead of shelling out to
`ps` a second time, and Windows yields PIDs like the others.

What must NOT be reused is `listCodexAppServerProcesses` itself: it deliberately maps
enumeration failure to an empty array for the #476 kill contract, and for adoption an empty array
means "no clients running" and resolves to `adopted`. "Could not enumerate" has to stay a
distinct outcome that resolves to `unknown`. Where that distinction cannot be preserved the
conservative answer is `unknown`; a platform that cannot enumerate must never report `adopted`.

A Codex client whose start time precedes the injection cannot have read the injected route.
That is a sound inference, and it is the one the operator needed.

## Shape

A new leaf module `src/codex/routing-adoption.ts`, so the derivation is pure and testable and
the wiring into shared files stays small:

```ts
type RoutingAdoption = "not-applicable" | "adopted" | "pending-client-restart" | "unknown";
deriveRoutingAdoption({ routingKind, injectedAtMs, clients }): RoutingAdoptionEvidence
collectRoutingAdoption(...): RoutingAdoptionEvidence // journal + process enumeration
```

- `not-applicable` — routing is not ours to speak for (native or custom).
- `adopted` — routing is `opencodex-local` and every running Codex client started after the
injection. This is still an inference about *opportunity*, not a traffic observation, and the
wording must not overclaim.
Review named the false-`adopted` sources this design knowingly does not cover, and the doc comment
must name them too: a Codex client the matcher fails to recognise, a restored or resumed thread
that keeps an already-open direct WebSocket even though its process started after injection, and an
`OPENAI_BASE_URL` or profile override in the client's own environment. Anything unverifiable is
`unknown`.
Round 2 added four more the comment must name: a start time in the SAME second as the injection,
which we deliberately treat as not stale; an empty match set, which is vacuously `adopted`; a
client running against a different `CODEX_HOME` or config path than the journal we read; and
Codex surfaces the CLI predicate does not match at all — `codex-code-mode-host`, Electron
helpers, VS Code extension hosts. None of these restores the original "config on disk implies
live traffic" overclaim, but a status line that sounds more certain than its evidence is the whole
defect in #4550, so the limits belong in the code.
- `pending-client-restart` — at least one running Codex client predates the injection.
- `unknown` — no injection time, or process start times unreadable. Enumeration failure reports
`unknown`; it never invents a clean bill of health, matching the `#476` restart contract.

Clock coarseness matters: `ps lstart` is second-granularity, and `app-server-processes.ts`
already documents why its equivalent comparison uses `<=`. A client started in the same second
as the injection is treated as **not** stale, so a rounding artifact cannot produce a false
warning.

`formatStartupRoutingDetail` gains an adoption token, and the summary names the concrete
action — restart the affected client — rather than only the routing kind.

## Regression test

A pure-derivation test: a pre-injection client yields `pending-client-restart` with that PID
listed; a post-injection client yields `adopted`; a missing injection time or an unreadable
start time yields `unknown`; a same-second start is not stale; a non-opencodex routing kind is
`not-applicable`; and the formatted detail string differs between configured and adopted.
13 changes: 13 additions & 0 deletions devlog/_plan/260914_l2_pool_routing_cache/030_delivery.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# wp3 — delivery and proof

One pull request against `dev`, filled to `.github/PULL_REQUEST_TEMPLATE.md`, with
`Closes #4546` and `Closes #4550`. Pushed with `git push --no-verify`. No merge: the parent
session performs the admin squash merge.

Proof is hosted CI at the exact final head SHA. The Verification section states that the local
suite, typecheck, install and GUI build were **NOT RUN** for this unit, names the hosted run id,
and reports its conclusion at that SHA. A green run at an earlier head is not proof for a later
one, so any follow-up commit resets the evidence and the new head's run is what gets reported.

Because `enforce-target` resets the contributor readiness checklist on every push, the head
SHA is captured after the final commit, not before.
Loading
Loading