Repository navigation
fix(runtime): retry network, timeout and 5xx failures of the codex login and model check - #2971
Conversation
…n and model check
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
…solvable-host output; widen login backoff Review follow-up: the class names the most specific text, so a 401 beside network text or a proxy 5xx naming a certificate could still be retried. A no-retry marker in stderr or the check's error events now vetoes every retry. Login gets up to five attempts (1, 2, 4, 8 s waits plus jitter) inside the same 90 s, refuses process errors, and never starts an attempt with under a second left after a late wake. The model check's second attempt also needs a second after its delay.
|
Codex review: needs maintainer review before merge. What this changesThis PR adds bounded retries for Codex login and startup model-check reliability failures, with sanitized login-attempt logs. Example: Codex access-token login exits with
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open: the prior findings are addressed, no actionable defect remains in the reviewed diff, and the explicit maintainer review hold still applies. Priority: P2 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherThe Kubernetes Compute Driver generates the Codex startup wrapper, which consumes Agent credentials and model selection, validates login and model access, and either starts the app server or holds readiness with sanitized diagnostics. flowchart TB
A[Agent deployment] --> B[Kubernetes Compute Driver]
B --> C[Generated startup wrapper]
C --> D[Codex login]
D --> E[Startup model check]
E --> F[App server or held failure]
C --> G[Sanitized runtime logs]
Technical reviewBest possible solution: Retain the bounded retries in the existing startup owner, shared security veto, and closed diagnostic vocabulary. Do we have a high-confidence way to reproduce the issue? Current-main source holds a fast access-token network failure immediately and does not retry an eventless network-class model-check exit; the reported OpenShell incident itself was not reproduced during this read-only review. Is this the best way to solve the issue? The patch extends the existing startup retry owner and reuses its classifier while preserving bounded budgets, recognized security vetoes, and fail-closed readiness. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against ee1320c9d265. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (2 earlier review cycles)
Reviewed October 19, 2026, 9:27 PM ET / October 20, 2026, 01:27 UTC (Revision 3). |
Codex names the request URL in a transport error, so a configured endpoint whose host or path carries a word like quota or credential must not turn its network retries off.
|
@freeqaz please hold for my clanker's review on this side. also review clawsweeper feedback. this needs work |
…try; drop the class-list test ClawSweeper review on ce873a2: the harness execution page said credential and TLS failures never retry, but Codex prints some TLS and DNS failures only as a failed connection, which keeps its retry. Qualify the statement and keep the page under the word cap. Remove the test that restated the private class lists; the login and model-check scenarios cover the behavior.
|
Free's clanker here. Holding this for your review; it will not land before you weigh in. The ClawSweeper items from ce873a2 are addressed on the current head (e237321): the certificate and quoted-401 vetoes were already in f16a946, and e237321 qualifies the TLS/credential wording in harness-execution and drops the low-value class-list test. Details are in the PR body under "ClawSweeper review on ce873a2". OpenShell's 500 credential_unavailable stays held at once (no auto-retry of credential failures). Requesting a fresh ClawSweeper pass next. |
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
ClawSweeper review on e237321: a proxy 5xx whose output names a TLS handshake failure or an unknown issuer classes as http_5xx, and the no-retry markers only knew "certificate", so the new login and model-check retries repeated a recognized TLS failure that main held at once. The veto now reuses CODEX_TLS_FAILURE_MARKERS. Login and model-check scenarios cover a proxy 5xx naming a TLS handshake, a handshake failure and an unknown issuer; they fail without the fix. Docs say TLS error instead of certificate.
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
… the sync Main's squashes of #2971, #2909, #2910 and #2921 equal the PR heads this branch already merged (checked per file: e8185da + PR head == main's blob, and no other main commit since e8185da touches these files). The seven conflicted files therefore take this branch's side, which is those same PR heads plus resolutions R1-R7. Everything else merged cleanly.
Brings in main bca5b19, which landed #2971, #2909, #2910 and #2921. Resolutions: - runtime-entrypoints.ts: keep #2971's single model-check retry decision and jittered delay behind the providerTools !== "none" guard, so a protected profile still retries nothing (finding 1599). - codex-native-startup fixture: keep both option sets and both sandbox additions (protected inference/verifier token; retry jitter/late wake). - codex-native-startup test: return the now-async checkCodexStartup in the retry block, and add a protected scenario proving a network-class Codex exit is held at once. - jobs.yaml: preprovisionedNamespace wrapper outermost; the install-time initialization Job lookup sits inside it. - postgres-migration-compatibility test: runHistoryMigration(db, mode, flag, refusal) combines the command flag with the refusal pattern. - postgres.json: keep the first-install and both canonical test names. - first-agent-smoke.mjs: credentialDigests is a routeModelProvider option beside journey and cleanup; only the smoke passes digests.
Problem
Finding 2017: after an OpenShell gateway restart, a redeployed Codex Harness held
RUNTIME_LOGIN_FAILED/RUNTIME_MODEL_PROBE_FAILED(probeexit-1, no events) on its first try, and the deployment ended permanently. Today access-token login retries only a process timeout, and the model check retries only a timeout or a transient event; a fast transport failure is held at once.Owner rule (10-19): retry network and timeout errors, which are reliability concerns; never automatically retry security errors (invalid or missing credentials and the like).
Change
One rule for both startup checks, driven by the
failureClassfrom #2959/#2962 (no second classifier):network,timeout, andhttp_5xx. Owner, please vetohttp_5xxif you disagree: a provider or proxy 5xx is treated as transient, matching the probe's existing transient rule.http_401_403,http_other(other 4xx, including 429) andtls; any credential rejection (AUTHENTICATION_FAILED, or stderr namingstatus 401/403 Unauthorized/Forbidden); and output with a no-retry marker anywhere in stderr or the check's error events, whatever its class. The markers are: unauthorized, forbidden, credential, API key, a bare 401/403/429, too many requests, rate/usage limit, quota, every TLS marker the classifier knows (certificate, TLS handshake, handshake failure, unknown issuer), and the two unresolvable-host forms the probe's event rules already hold. URLs are stripped before matching, so a configured endpoint whose host or path contains one of these words keeps its network retries. This matters because the class names only the most specific text, so a 401 beside network text, or a proxy 5xx naming a certificate, would otherwise class as reliability. The veto also applies to the existing timeout and transient retries.emptyandunknownkeep the old rule.codex_pat, provider-managed PAT; API-key login only saves the key and still never retries): up to 5 attempts. Each attempt's timeout is capped by one 90 s deadline, the same 90 s that three 30 s attempts always had, so at most three timeouts still fit. A fast failure waits 1, 2, 4, then 8 s, plus up to 50% jitter, so a restarting gateway gets about 15 to 22 s (Atomics.wait, synchronous like the login itself). A timed-out attempt has already waited, so the next one starts at once, as today. A wrapper-side process error (ENOBUFS,ENOENT) is never retried. If a wait wakes late, no attempt starts with less than 1 s left. Each failed attempt logs{event: "codex.login", attempt, elapsedMs, exitCode, failureClass}(enum only, never output). The final held failure and itscodex-loginstartup-phase line are unchanged.-attempt2and the closed vocabulary in OCC and the console stays the same.codexProbeRetryable()is now the single retry decision. It covers a timeout, a transient event (existing), or a Codexexit-Nwith no conclusive event whose class isnetwork/timeout/http_5xx(new: the 2017 shape). Event-level rules that hold on purpose (DNS "Name does not resolve", certificate, 4xx, rate limit, findings 1601/1602/1621) are unchanged. Spawn errors such aserror-ENOBUFSare not retried. The delay is 1 to 1.5 s (jitter), and the second attempt must still have at least 1 s left after it.codex.login(attempt, elapsedMs, exitCode, closed-listfailureClass) atwarn. The line shows inocc agent logs, the console Logs tab and pod logs; the bundled Collector does not export it, just as it does not exportfailureClass.Worst-case added time: none to either budget. Login is still at most 90 s, and the model check still at most 61 s including delays. A fast reliability failure that used to be held in about 1 s can now take up to 90 s (login: five attempts plus at most about 22 s of waits) or 61 s (check) before it is held. A security failure is still held at once.
Merging with #2487 (Credential Proxy)
protected-no-inferencesetslogin = { status: 0 }before the retry loop, and its probe result{}is never retryable. This PR does not touch Credential Proxy Implementation #2487'sif (!noInference) logStartupPhase("codex-login", ...)hunk; that conflict comes from fix(runtime): classify failed codex login and model check output without logging it #2959 and is the same as before.pluginRuntime?.manifest.providerTools !== "none". Resolve it by keeping this PR's single block and adding that guard to it:if (pluginRuntime?.manifest.providerTools !== "none" && attempt === 1 && codexProbeRetryable(result) && performance.now() + delay < deadline).That one guard then covers the timeout, transient and new exit retries, which also fixes board finding 1599 (the transient retry was missing the guard). Add an opt-in Strands history policy backend for Credential Proxy #2543, stacked on Credential Proxy Implementation #2487, gets the same hunk.
codex_patlogin also gets the network/5xx login retries. Those calls go to the login endpoint, not model requests. If Credential Proxy Implementation #2487 wants no login retries either, the same guard can go on the login loop's retry condition.Open-PR conflict check (3-way merge-tree, conflict hunks in shared files, main → this head)
Internal review (separate reviewer, focused on "no security failure retried" and bounded time) asked for the no-retry marker veto, a longer login backoff, the late-wake guard, refusing process errors on login, the 1 s floor for the check's second attempt, and narrower doc wording. All of these are applied in f16a946. Its re-review approved with nits: URL stripping (da42306) and the owner decision below.
OpenShell
credential_unavailable: held at once (owner call)When the pinned OpenShell (5a7728c3) cannot resolve a credential placeholder, it answers
500with{"error":"credential_unavailable",...}. The "credential" marker holds that at once, following the owner rule never to auto-retry credential or auth failures. This is deliberately conservative: if 2017 on OpenShell turns out to be a placeholder that the supervisor has not reloaded yet after a gateway restart, this PR will not fix that case, and a follow-up would need an explicit owner decision.ClawSweeper review on ce873a2
"status 401 Unauthorized"on an eventless check exit: the check now carries the credential-rejection result (codexAuthenticationRejected) and the markers into its retry decision (noRetry). Covered by "a quoted credential rejection beside network text" for the model check.On e237321 ClawSweeper found one more gap: a proxy 5xx naming a TLS handshake failure or an unknown issuer still retried, because the veto knew only "certificate". 78839e7 reuses the classifier's
CODEX_TLS_FAILURE_MARKERSin the veto, with login and model-check scenarios for those three shapes (they fail without the fix).Verification
tests/conformance/codex-native-startup.test.mjsuses the scripted native fakes (fixture clock, captured timers, a recordedAtomics.wait, fixedMath.random, no wall-clock windows). The new block "retries a reliability failure of its login or model check, never a security failure" covers:exit-1-attempt2); jitter bound and budget.codex.loginlines); with this change, 210/210 pass.tests/conformance/runtime-logs-content.test.mjs: acodex.loginline keeps its closed-list class at warn and drops an off-list value. This fails on main.tsc --build, eslint and prettier on the touched files.Risks
http_5xxretry is a judgement call (see above).codex.model_probelines now show the class for the next decision.spawnSyncduring login, readiness is already unready then, and SIGTERM still ends the process at once (no handler is installed yet).