Skip to content

Describe Codex rate-limit RPC timeouts instead of the method name - #13059

Closed
Bartok9 wants to merge 5 commits into
omacom:quattrofrom
Bartok9:bartok9/codex-rpc-timeout-message
Closed

Bartok9 wants to merge 5 commits into
omacom:quattrofrom
Bartok9:bartok9/codex-rpc-timeout-message

Conversation

@Bartok9

@Bartok9 Bartok9 commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Summary

rpc_request raised TimeoutError(method), so a 4s account/rateLimits/read stall stored authHelpText as the bare RPC name. That looked like an auth failure ("Codex limits unavailable" + account/rateLimits/read) instead of a timeout.

Include the timeout duration in the exception text.

Test plan

  • Isolated collector run against a stub codex that sleeps on account/rateLimits/read: usageStatusText is Codex limits unavailable and authHelpText is account/rateLimits/read timed out after 4s.
  • Added the same assertion to test/shell.d/agent-usage-codex-scanner-test.sh.

Closes #12880

rpc_request raised TimeoutError(method), so a 4s account/rateLimits/read
stall stored authHelpText as the bare RPC name and looked like an auth
failure. Include the timeout duration in the exception text.

Closes omacom#12880
@llstrk

llstrk commented Sep 24, 2026

Copy link
Copy Markdown

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Verified: On an account/rateLimits/read stall, the head collector stores usageStatusText "Codex limits unavailable" and authHelpText "account/rateLimits/read timed out after 4s", after about 4.07 s. True stalls of initialize and account/read are described the same way (... timed out after 8s / 4s). The new test case fails against the base collector at the new assertion and passes against the head.

One issue in the new message and one in the new test case are described below.

End of stream is now reported as a timeout

rpc_request also leaves its loop when the app-server closes stdout before replying (readline() returns "", then break), and then reaches the same raise. With this PR, that path claims a timeout that never elapsed:

Stub app-server behavior (synthetic) Base authHelpText Head authHelpText Elapsed
Never answers the rate-limit read account/rateLimits/read account/rateLimits/read timed out after 4s ~4.07 s
Exits before any reply initialize initialize timed out after 8s ~0.05-0.11 s
Exits after the initialize reply account/read account/read timed out after 4s ~0.06 s

No real Codex binary was run; the stubs emulate the app-server's JSON-RPC stream.

Impact: When Codex exits early, for example because it rejects a command-line flag (the situation fixed in #7649), the panel reports a slow 8 s timeout instead of an early exit. That points diagnosis toward load or latency rather than a CLI incompatibility.

Suggested change: Give end of stream its own message, for example:

    line = proc.stdout.readline()
    if not line:
      raise RuntimeError(f"codex app-server exited before answering {method}")

A stub case that exits immediately could assert that text.

New EXIT trap drops six cleanup targets

Each earlier section of test/shell.d/agent-usage-codex-scanner-test.sh extends the EXIT trap cumulatively; the line 548 trap covers eight directories. The new line 608 replaces it with only three:

-trap 'rm -rf "$TEST_HOME" "$PI_HOME" "$OPENCODE_HOME" "$CACHE_HOME" "$FRESH_HOME" "$MALFORMED_HOME" "$UNWRITABLE_HOME" "$INTERRUPTED_HOME"' EXIT   # line 548
+trap 'rm -rf "$TEST_HOME" "$PI_HOME" "$TIMEOUT_HOME"' EXIT                                                                                          # line 608

Impact: Each run of the test file now leaves six mktemp -d directories behind (six observed on head, none on base).

Suggested change: Append "$TIMEOUT_HOME" to the full existing list. Setting XDG_CACHE_HOME="$TIMEOUT_HOME/.cache" on the new collector invocation would also keep its scan cache inside the temporary home, as the later cases in the file do; without it, an inherited XDG_CACHE_HOME receives an extra codex-scan-* file pair.

Optional context on the timeout itself: In Codex rust-v0.155.1, account/rateLimits/read waits for a reset-credit detail lookup with its own 5 s cap, which is longer than the collector's 4 s deadline. That version accepts excludeResetCreditDetails: true for background polls; compatibility with older app-servers was not checked. Whether to change the deadline or add a retry is outside this PR's stated scope.


Review information

Test scope: Source review of the pinned head and base, plus isolated runs of the production collector and the scanner test file against synthetic stub app-servers (stall, early exit, error reply and success cases), with base controls. Codex behavior is from reading the openai/codex rust-v0.155.1 source. No real Codex binary, account or network was used, and the panel was checked from QML source only, not rendered.

AI process: Opus 5.5 Medium coordination and synthesis, independent Opus 5.5 Xhigh and GPT 6 Sol Xhigh technical assessments, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

Raise a dedicated early-exit error when stdout closes before a reply,
restore the cumulative EXIT trap (plus TIMEOUT/EOF homes), and pin
XDG_CACHE_HOME for the timeout/EOF collector runs.
@Bartok9

Bartok9 commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review — both issues look right.

Pushed a follow-up that:

  1. raises RuntimeError("codex app-server exited before answering {method}") on empty readline instead of labeling EOF as a timeout
  2. restores the cumulative EXIT trap (prior homes + TIMEOUT_HOME / EOF_HOME) and pins XDG_CACHE_HOME for the timeout/EOF collector runs
  3. adds an early-exit stub assertion for the new message

@llstrk

llstrk commented Sep 24, 2026

Copy link
Copy Markdown

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Verified: At 4d227e6, both issues from the earlier review are resolved, matching the three changes listed in the author's reply.

Earlier finding Status at 4d227e6
End of stream is now reported as a timeout Resolved
New EXIT trap drops six cleanup targets Resolved

End of stream: resolved

rpc_request now raises codex app-server exited before answering <method> when readline() returns "". Its only caller catches Exception and stores the message text, so switching from TimeoutError to RuntimeError does not change how the failure is handled. Real stalls keep the timeout text.

Stub app-server behavior (synthetic) 98a7ba6 authHelpText 4d227e6 authHelpText Elapsed
Exits before any reply initialize timed out after 8s codex app-server exited before answering initialize ~0.06 s
Exits after the initialize reply account/read timed out after 4s codex app-server exited before answering account/read ~0.06 s
Exits after the rate-limit request account/rateLimits/read timed out after 4s codex app-server exited before answering account/rateLimits/read ~0.11 s
Never answers the rate-limit read account/rateLimits/read timed out after 4s unchanged ~4.07 s

All four cases store usageStatusText "Codex limits unavailable". The new test assertion fails against the 98a7ba6 collector (initialize timed out after 8s) and passes against 4d227e6.

Test cleanup: resolved

Both trap lines in test/shell.d/agent-usage-codex-scanner-test.sh now list all ten temporary homes, and both new collector runs set XDG_CACHE_HOME inside their own home. A run of the test file on 4d227e6 leaves no temporary directories behind (six on 98a7ba6), and the inherited cache receives the same three scan-cache file pairs as on the base.

Optional test improvement: The new early-exit stub exits without reading stdin. If it has already exited when the collector writes initialize, the collector stores [Errno 32] Broken pipe instead of the early-exit text (shown with a synthetic probe that forces this order; not observed in 400 runs without forcing it). A stub that reads one line first (read -r _; exit 0) makes the test exercise the end-of-stream path every time.

Optional: The same write-side case can occur in the collector itself, for example when the app-server answers initialize and exits immediately: the panel then shows the raw [Errno 32] Broken pipe. The base behaves the same way. Catching BrokenPipeError at the request and initialized notification writes and raising the same exited before answering <method> message would cover it.

The earlier optional context on the rate-limit timeout (the 5 s reset-credit lookup in account/rateLimits/read) is unaffected by this commit and still matches Codex rust-v0.155.1 and rust-v0.156.1.


Review information

Test scope: Follow-up on 4d227e6 against 98a7ba6 and the base: source review, plus isolated runs of the production collector and the scanner test file against synthetic stub app-servers (early exit at each request, stall, error reply and success). No real Codex binary, account or network was used, and the panel was checked from QML source only, not rendered.

AI process: Opus 5.5 Medium coordination and synthesis, independent Opus 5.5 Xhigh and GPT 6 Sol Xhigh technical assessments of this revision, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

Catch BrokenPipeError on RPC and initialized writes so early app-server
exit is reported as exited-before-answering rather than raw EPIPE. Make
the EOF test stub read one line first so initialize always hits EOF.
@Bartok9

Bartok9 commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the verification — prior issues look resolved.

Pushed a small follow-up for the optional write-side race:

  1. rpc_request and the initialized notification catch BrokenPipeError and raise the same codex app-server exited before answering … message (instead of raw [Errno 32] Broken pipe).
  2. EOF stub now read -r _ once before exit so the test always exercises the end-of-stream path rather than a racey EPIPE on initialize.

Happy to leave the rate-limit deadline / excludeResetCreditDetails discussion for a separate PR.

@llstrk

llstrk commented Sep 24, 2026

Copy link
Copy Markdown

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Verified: At a25e227, both optional notes from the previous review are addressed, as described in the author's reply. The two findings from the first review remain resolved. No regressions were found in the changed code.

Earlier note Status at a25e227
Early-exit test stub could store [Errno 32] Broken pipe Resolved
Same write-side case in the collector Resolved
End of stream reported as a timeout (first review) Still resolved
EXIT trap drops cleanup targets (first review) Still resolved

Test stub race: resolved

The early-exit stub now reads one line before exiting, so it cannot exit before the collector's initialize write reaches the pipe. The test therefore always takes the end-of-stream path: 200 of 200 direct runs stored codex app-server exited before answering initialize. The scanner test file passes on a25e227 and still fails against the 98a7ba6 collector (initialize timed out after 8s).

Collector write side: resolved

rpc_request and the initialized notification now map BrokenPipeError to the same early-exit text. A failed initialized write names account/read, the next request, which matches what the end-of-stream path reports in the same situation.

Stub app-server exits before the collector writes (synthetic, forced order) 4d227e6 authHelpText a25e227 authHelpText
initialize request [Errno 32] Broken pipe codex app-server exited before answering initialize
initialized notification [Errno 32] Broken pipe codex app-server exited before answering account/read
account/read request [Errno 32] Broken pipe codex app-server exited before answering account/read
account/rateLimits/read request [Errno 32] Broken pipe codex app-server exited before answering account/rateLimits/read

Without forced ordering, a stub that answers initialize and exits at once gave the early-exit text in 50 of 50 runs on a25e227, against 31 of 50 raw [Errno 32] Broken pipe on 4d227e6. Stalls still report account/rateLimits/read timed out after 4s, the success path still parses both limit windows, and the stub process is still terminated and reaped.

Optional: After a broken pipe, the unsent request stays buffered in proc.stdin. When fetch_codex_rpc() returns, Python's file finalizer retries the flush and prints Exception ignored while finalizing file ... BrokenPipeError: [Errno 32] Broken pipe to the collector's stderr. The agents panel logs that stderr through console.warn, so the raw error this PR replaces in the panel text still appears in the Quickshell log. The JSON output is unaffected, and the base behaves the same way. Closing proc.stdin at the start of the finally block, with OSError suppressed, removed the message in every probe of a locally patched copy and changed no other result.

Optional test improvement: After this commit, no test reaches the new write-side mapping, because the end-of-stream stub now always takes the read side. This stub fails the initialized write every time (5 of 5 runs) and separates the revisions: exited before answering account/read on a25e227, raw [Errno 32] Broken pipe on 4d227e6 and the base.

#!/bin/bash
read -r _ || exit 0
exec 0<&-
printf '%s\n' '{"id":1,"result":{}}'

Review information

Test scope: Follow-up on a25e227 against 4d227e6 and the base: source review, plus isolated runs of the production collector and the scanner test file against synthetic stub app-servers (early exit before each write, end of stream, stall and success). Forced-order cases control write timing in the test harness and are labelled as such; natural-order counts come from a single machine. Python finalizer behavior was checked in CPython 3.13 and 3.14 source and observed on Python 3.14 only. No real Codex binary, account or network was used, and the panel was checked from QML source only, not rendered.

AI process: Opus 5.5 Medium coordination and synthesis, independent Opus 5.5 Xhigh and GPT 6 Sol Xhigh technical assessments of this revision, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

Close proc.stdin in fetch_codex_rpc finally so CPython does not emit
ignored BrokenPipeError on finalizer flush after a failed write.
Add a scanner stub that answers initialize then closes stdin so the
initialized notification path is covered.
@Bartok9

Bartok9 commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the re-verify — both optional notes look right.

Pushed a follow-up:

  1. Close proc.stdin (suppressing OSError) at the start of fetch_codex_rpc()'s finally so CPython does not retry a buffered flush and log Exception ignored ... BrokenPipeError after a write-side early exit.
  2. Add a write-side EPIPE scanner stub (read one line → exec 0<&- → print initialize result) that asserts authHelpText is codex app-server exited before answering account/read.

Still leaving rate-limit deadline / excludeResetCreditDetails for a separate PR.

@llstrk

llstrk commented Sep 27, 2026

Copy link
Copy Markdown

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Verified: At c3757d8, both optional notes from the previous review are addressed, as described in the author's reply. The earlier fixes still hold, and no regressions were found in the changed code.

Earlier note Status at c3757d8
Finalizer prints Exception ignored ... BrokenPipeError to collector stderr Resolved
No test reaches the write-side mapping Resolved
End of stream, EXIT trap, early-exit stub race, collector write side (earlier reviews) Still resolved

Finalizer noise: resolved

The finally block in fetch_codex_rpc() now closes proc.stdin first, with OSError suppressed. In CPython 3.14.7, close() still closes the file when its flush raises BrokenPipeError, and the finalizer only acts on files that are still open, so nothing is printed later.

Stub app-server closes its stdin before the collector writes (synthetic) a25e227 stderr c3757d8 stderr
initialized notification (the PR's new stub), 50 runs finalizer message 50/50 empty 50/50
account/rateLimits/read request, 20 runs finalizer message 20/20 empty 20/20
initialize request (forced order), 10 runs finalizer message 10/10 empty 10/10

The JSON output is identical on both revisions in every case. Success, error-reply, stall (still about 4.07 s) and end-of-stream results are unchanged, and the stub process was gone after every run.

Write-side test: resolved

The new EPIPE_HOME case uses the suggested stub unchanged and asserts codex app-server exited before answering account/read. The case passed in 30 of 30 runs on a25e227 and c3757d8 and failed in 30 of 30 on 4d227e6 and the base ([Errno 32] Broken pipe). The stub closes its stdin before it answers initialize, so the collector's next write always finds no reader. The three trap lines this PR adds list all 11 temporary homes, and a run of the test file leaves none behind.

Optional test improvement: The new case reads only stdout, so it still passes if the stdin close is removed (it passes against a25e227, where stderr carries the finalizer message). Capturing stderr and asserting it is empty separated the revisions in 30 of 30 runs each:

epipe_result=$(... "$ROOT/bin/omarchy-agent-usage-codex" 2>"$EPIPE_HOME/stderr")
[[ ! -s $EPIPE_HOME/stderr ]] ||
  fail "Codex collector leaves no finalizer noise on stderr after a write-side exit" "$(cat "$EPIPE_HOME/stderr")"

A narrower ! grep -q 'Exception ignored' "$EPIPE_HOME/stderr" would ignore unrelated warnings; only the empty-stderr form was run.

Optional context, related open PRs: Four other open PRs change the same collector or test file and differ from this PR, so whichever merges second has to reconcile them (read from their diffs; none was run):

PR Overlap
#13106 Returns the login hint before starting the app-server when the default file credential store has no auth.json and CODEX_ACCESS_TOKEN is unset, and adds auth.json to the existing test homes. The three new homes here (TIMEOUT_HOME, EOF_HOME, EPIPE_HOME) would need it too, or their stubs are never started.
#13102 Rewrites rpc_request's read loop; its reader returns None on end of stream, which falls into the timeout message again.
#12979 Its new test asserts the bare account/read text that this PR replaces.
#8073 Detects early exit with different wording (Codex app-server exited before initialize).

Review information

Test scope: Follow-up on c3757d8 against a25e227, 4d227e6 and the base: source review, CPython 3.14.7 _io source for the close and finalizer behavior, and isolated runs of the production collector and the scanner test file against synthetic stub app-servers (write-side failure at initialize, the initialized notification and account/rateLimits/read, end of stream, stall, error reply and success). Forced-order cases control write timing in the test harness. Related PRs were read from their diffs only. No real Codex binary, account or network was used, and the panel was checked from source only, not rendered.

AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

@Bartok9

Bartok9 commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the re-verify — prior notes look resolved.

Pushed a small test follow-up for the optional stderr coverage:

  • EPIPE collector run now redirects stderr to $EPIPE_HOME/stderr and asserts the file is empty, so the proc.stdin.close() cleanup is locked in (not only the authHelpText mapping).

Still leaving rate-limit deadline / excludeResetCreditDetails, and merge reconciliation with #13106 / #13102 / #12979 / #8073, for separate work.

Capture collector stderr in the EPIPE_HOME case and require it empty so
the stdin-close cleanup is covered, not only the authHelpText mapping.
@Bartok9

Bartok9 commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up to the previous note: the stderr assertion is now actually on the branch (EPIPE_HOME redirects collector stderr and requires it empty). Sorry for the earlier comment racing the push.

@dhh

dhh commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Thanks! The same fix is coming in through #14049 (from #8977). Closing in favor of that.

@dhh dhh closed this Oct 2, 2026
@Bartok9

Bartok9 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — closing in favor of #14049 works for me. Glad the timeout wording is landing there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

omarchy-agent-usage-codex: account/rateLimits/read RPC timeout (4s) misreported as unavailable

4 participants