Fix Codex RPC timeout classification

[Chipagosfinest]

What changed

Classify a Codex app-server request as timed out before terminating the subprocess that owns stdout.

Why

The previous timeout task terminated the process and then threw. Closing stdout could wake the request body first, causing the same timeout to escape as a misleading EOF/malformed-response error. Selecting the timer result before process termination makes the reported failure deterministic.

Scope

• one production file
• no settings, UI, spend-model, or provider behavior changes
• extracted from Track multi-provider spend and model usage #2759 so the runtime fix can be reviewed independently

Verification

• swift test --filter CodexUsageFetcherFallbackTests — 13 tests passed, including the real spawned-process hung RPC cases
• make check — passed with zero SwiftFormat or SwiftLint violations

Supersedes the timeout portion of #2759.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed August 17, 2026, 6:34 PM ET / 22:34 UTC.

ClawSweeper review

What this changes

The PR makes the Codex app-server timeout win before subprocess termination can turn the response stream closure into a malformed-response error.

Merge readiness

⛔ Blocked until real behavior proof from a real setup is added - 3 items remain

Keep open: the focused patch is source-plausible and current main still has the pre-termination timeout race, but this external PR still needs inspectable after-fix runtime proof before merge.

Priority: P2
Reviewed head: e855b34b8c7d967632fb964d6bdf03ea3aa3b3d8

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The patch is focused and source-plausible, but real after-fix behavior proof remains the merge gate.
Proof confidence	🦪 silver shellfish (2/6)	Needs real behavior proof before merge: The PR reports focused tests but provides no inspectable after-fix terminal output, recording, or redacted runtime log; the prepared images are unrelated UI proof from the older closed PR. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs real behavior proof before merge: The PR reports focused tests but provides no inspectable after-fix terminal output, recording, or redacted runtime log; the prepared images are unrelated UI proof from the older closed PR. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	5 items	Current-main behavior remains unfixed: Current main's timeout task terminates the subprocess before it throws the timeout; closing stdout can let the response task surface a malformed-response error first. Patch selects the deadline before teardown: The branch returns a typed timeout race result, cancels the sibling task, then terminates the process only after the timeout result has been selected. Existing spawned-process coverage: Current tests create a real shell subprocess that hangs on the rate-limits RPC and assert the public fetcher reports RPCWireError.timeout within its time budget; the branch does not add a deterministic regression assertion for the specific ordering race.
Findings	None	None.
Security	None	None.

How this fits together

CodexBar launches the Codex app-server subprocess and reads JSON-RPC replies from stdout to refresh provider usage. Each request races a response reader against a deadline, then returns usage data or a classified provider error.

flowchart LR
A[Usage refresh] --> B[Codex app-server subprocess]
B --> C[JSON-RPC stdout reply]
C --> D{Reply or deadline}
D -->|Reply| E[Decode usage]
D -->|Deadline| F[Terminate subprocess]
E --> G[Provider result]
F --> G

Loading

Before merge

• Add real behavior proof - Needs real behavior proof before merge: The PR reports focused tests but provides no inspectable after-fix terminal output, recording, or redacted runtime log; the prepared images are unrelated UI proof from the older closed PR. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Resolve merge risk (P1) - The changed task-race and subprocess-termination path can still affect provider failure latency or classification; the supplied evidence does not show the after-fix runtime result.
• Complete next step (P2) - Await contributor-supplied real behavior proof and the exact-head CI result; no automated code repair is indicated.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Changed runtime surface	1 production file; production +23/-13, tests unchanged	The patch is tightly scoped to the request-versus-timeout coordinator, so proof should target that exact failure path.

Merge-risk options

Maintainer options:

1. Capture timeout-path proof before merge (recommended)
   Add a redacted terminal transcript or runtime log showing a hung spawned RPC is returned as the timeout error after this change.

Technical review

Best possible solution:

Keep the timer-first classification, add a deterministic ordering regression if feasible, and retain a redacted spawned-process runtime trace as merge evidence.

Do we have a high-confidence way to reproduce the issue?

No live reproduction was supplied, but source inspection and the existing spawned-process hanging-RPC test define a high-confidence source-level reproduction path.

Is this the best way to solve the issue?

Yes: choosing the timer result before stdout-owning process teardown is the narrowest maintainable repair for the stated race, subject to after-fix runtime proof.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against f7723d33a169.

Labels

Label justifications:

• P2: This is a bounded reliability correction for Codex usage-refresh error handling.
• merge-risk: 🚨 availability: The PR changes timeout and subprocess termination sequencing, which can affect failure completion and provider refresh availability.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR reports focused tests but provides no inspectable after-fix terminal output, recording, or redacted runtime log; the prepared images are unrelated UI proof from the older closed PR. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Current-main behavior remains unfixed: Current main's timeout task terminates the subprocess before it throws the timeout; closing stdout can let the response task surface a malformed-response error first. (Sources/CodexBarCore/UsageFetcher.swift:1010, f7723d33a169)
• Patch selects the deadline before teardown: The branch returns a typed timeout race result, cancels the sibling task, then terminates the process only after the timeout result has been selected. (Sources/CodexBarCore/UsageFetcher.swift:1015, e855b34b8c7d)
• Existing spawned-process coverage: Current tests create a real shell subprocess that hangs on the rate-limits RPC and assert the public fetcher reports RPCWireError.timeout within its time budget; the branch does not add a deterministic regression assertion for the specific ordering race. (Tests/CodexBarTests/CodexUsageFetcherFallbackTests.swift:153, f7723d33a169)
• Feature-history routing: Local history includes Peter Steinberger's commits titled “test: stabilize Codex RPC stub startup” and “test: stabilize hung codex rpc timeout test,” making him the strongest current-history routing candidate. (Tests/CodexBarTests/CodexUsageFetcherFallbackTests.swift:153, f8d8a0ef7add)
• Proof media is unrelated: All four prepared images show usage-and-spend UI from the closed broader PR, not an after-fix Codex RPC timeout result.

Likely related people:

• steipete: Current history attributes the existing timeout code and prior Codex RPC timeout-test stabilization to Peter Steinberger. (role: recent area contributor; confidence: medium; commits: dc3ea3206c70, f8d8a0ef7add, bffa0faac7d2; files: Sources/CodexBarCore/UsageFetcher.swift, Tests/CodexBarTests/CodexUsageFetcherFallbackTests.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Add a redacted terminal transcript or runtime log for the spawned hung-RPC case, excluding account, path, and credential data.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)

• reviewed 2026-08-17T22:18:48.501Z sha e855b34 :: needs real behavior proof before merge. :: none

[steipete]

Thanks @Chipagosfinest! This landed via #3025, which carried your commits onto current main and fixed the stale architecture-gatekeeper anchors that were failing CI (your diff was fine — the base predated recent churn in UsageFetcher.swift). The changelog credits you.