Clear a stale Codex connectivity failure when publication is withheld - #3214
Clear a stale Codex connectivity failure when publication is withheld#3214olddonkey wants to merge 2 commits into
Conversation
A withheld weekly-reset reading returns before the publication path that clears `errors`, so the message from an earlier outage outlived the successful fetch that replaced it. That message is also persisted beside the preserved account snapshot and rehydrated at launch, so a relaunch could not clear it either: the card kept reporting that the network was offline while every refresh cycle was in fact succeeding. Clear it when the withheld reading's own fetch succeeded. Only a connectivity message is eligible — a successful fetch is evidence against exactly that claim, and leaving every other kind untouched keeps a withheld publication otherwise inert with respect to published state. Eligibility reuses the classification that allowed the message to be stored beside preserved usage in the first place. The persisted copy is amended from the store's own contents rather than by writing `codexAccountSnapshots`, which a single-account refresh empties for its duration; writing that array here would erase the snapshot the withheld cycle is preserving. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Changelog entry for the release flow (not added to Related but out of scope, from the same investigation: on the same account the withhold itself never resolves, because the immediate confirmation lands in the same second as the initial reading and Account, plan, advanced boundary and the |
![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: 97fc3b458c
ℹ️ 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".
| if let recorded = self.errors[.codex], Self.codexErrorDisprovedBySuccessfulFetch(recorded) { | ||
| self.errors[.codex] = nil |
There was a problem hiding this comment.
When an outage has already reached the UI, failureGates[.codex] has a streak of at least two. Clearing only errors[.codex] leaves that streak intact, unlike the normal successful-publication path, which calls recordSuccess(). Consequently, after one or more withheld successful refreshes, the next single transient failure is surfaced immediately instead of being suppressed while the preserved snapshot exists. Reset or record success on the Codex failure gate when this successful fetch clears the connectivity error.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed August 26, 2026, 4:17 PM ET / 20:17 UTC. ClawSweeper reviewWhat this changesThis PR clears stale Codex connectivity messages and resets refresh-failure suppression when a successfully fetched weekly-reset reading is withheld from publication. Merge readiness⛔ Blocked until stronger real behavior proof is added - 3 items remain The implementation is focused and addresses the prior failure-gate finding, but this external PR still needs after-fix real-behavior proof before merge. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCodexBar fetches Codex usage, validates suspicious weekly-reset readings, and then publishes trusted account state to the menu bar card and snapshot store. Withheld readings preserve the trusted usage while this change removes a disproven connectivity error. flowchart LR
A[Codex usage fetch] --> B[Weekly reset admission]
B -->|Publish| C[Update card and account snapshot]
B -->|Withhold successful reading| D[Clear stale connectivity error]
D --> E[Preserve trusted usage]
E --> F[Menu bar card]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the existing weekly-reset admission policy and land this narrow error-state repair once redacted after-fix evidence demonstrates the withheld-success path. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: current main returns before error cleanup on withheld admission, and the PR's focused tests construct the persisted-error and withheld-success sequence. Is this the best way to solve the issue? Yes, the patch reuses the existing connectivity classification and account-scoped persistence path without changing weekly-reset publication policy. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against cf79d1310493. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Review follow-up. The ordinary publication path records the fetch success on the consecutive-failure gate; the withheld path returned without doing so, leaving the streak from the outage that just ended. The next transient failure would then be surfaced immediately instead of receiving the normal first-failure suppression. The streak counts fetch outcomes rather than publications, so record the success alongside clearing the connectivity message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both items addressed in [P2] Reset the failure gate after a withheld successful fetch — fixed. Correct catch, and the reasoning generalises: the streak counts fetch outcomes, not publications, so the withheld path had no business leaving the outage's streak standing. Two things that test surfaced, worth recording because they are easy to get wrong:
Real behavior proof — added, with one honest gap. The "Real behavior on the affected install" section has the live capture from the 0.55.1 install this was found on: the persisted record carrying the message while returned 200 in 0.33 s, the bundled CLI succeeded, and the app's debug log showed six refresh cycles with zero network errors — plus the app's own It also has the live recovery: once weekly usage crossed the 1% threshold the reading published normally and the message disappeared on its own. That is the coupling this PR breaks, observed end to end on a real account — nothing but a publication clears the message, so while readings are withheld it cannot go away. What I could not produce is an after-fix capture of a withheld cycle. That needs the account inside the withhold band (weekly at or below 1%, advanced boundary, reset credit available); this one left the band at 19:41 UTC and Gate on this head: @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
steipete
left a comment
steipete
left a comment
There was a problem hiding this comment.
The stale-connectivity diagnosis is sound, and resetting the failure gate alongside clearing the message addresses the earlier feedback. I found one remaining correctness boundary in head 70a37b9.
P2 — Validate refresh freshness and account ownership before clearing state. clearCodexFetchErrorAfterWithheldPublication records success and clears errors[.codex] before checking the current account, and it never checks the refresh generation. Its caller reaches this branch after awaiting publication admission. A nil admission can also result from cancellation or a failed confirmation, so a superseded refresh can clear the selected account's error/failure gate. Please apply the existing generation, cancellation, and account/workspace ownership guards before any cleanup, with regressions for a superseded refresh and a same-email account/workspace switch.
The stacked-account branch also retains the prior record unchanged when result.outcome is nil. Please cover the same successful-but-withheld recovery there, or explicitly narrow the PR's claim to single-account refreshes. Preserve the published snapshot, credits, pending reset evidence, sibling account errors, and persisted records; this should not change weekly-reset admission policy.
The current CI is green, but the added tests do not exercise these ownership races or the stacked path. The new test file extends CodexAccountScopedRefreshTests, so that is the focused suite to run. A persistence/relaunch assertion after successful cleanup would also pin down the original user-visible failure.
2 participants
Problem
The Codex card can get stuck showing
Network error: <the system's offline message>on a machine whose network is fine, and stay that way across relaunches, while the app is in fact fetching successfully every refresh cycle.Seen on my own install (0.55.1, single Codex account).
~/Library/Application Support/CodexBar/codex-account-snapshots.json:Meanwhile the same machine, seconds apart:
curl https://chatgpt.com/backend-api/wham/usagewith the auth-file token → HTTP 200 in 0.33 s/Applications/CodexBar.app/Contents/Helpers/CodexBarCLI usage --provider codex→ succeeds, freshupdatedAtRoot cause
Two behaviours combine, both in the Codex weekly-reset publication path.
A withheld publication returns before the code that clears
errors.resolvedCodexRefreshOutcomebails out as soon as the admission withholds a reading:Withholding is a judgement about that reading, not about connectivity — the fetch behind it succeeded. But because the publication path is skipped, the previous failure's message survives it.
The message is persisted and rehydrated. A network failure that keeps prior usage takes
resolvedCodexAccountOutcome's preserve branch, which stores the message next to the preserved snapshot; launch restores it (UsageStore+Refresh,self.errors[.codex] = hydratedPrior.error), so relaunching cannot clear it either. The consecutive-failure gate means it is only recorded once an outage survives more than one cycle, so what sticks is a real outage's message — it just never stops being displayed.Put together: one transient outage records a message; the weekly-reset guard then withholds every later reading for as long as the account stays at or below the reset threshold; no cycle ever clears the message; and the card keeps blaming the network for a staleness that has a completely different cause. That misdirection is the real damage here — the frozen value is a separate, deliberate behaviour, but the label sends people to check their Wi-Fi.
Why this surfaces now
Not a recent code change.
git log -Sputs all three ingredients — the withholding guard, the earlyreturn nil, and the error rehydration — in b2b9c53, "fix: confirm Codex weekly reset snapshots" (#2064, 2026-07-11, first released in v0.42.1). #3177 reshaped that branch into anadmissionstruct last week but did not introduce the skip: its parent commit'selsebranch returnsnilthe same way.What changed is how often the branch is reached. It only engages on an early backend weekly reset — a reading at or below the reset threshold while the stored one is above it and the reset credit is still available. Those started arriving in August: #2790 (08-08), then #3168 and #3179 (08-24), #3193 (08-25). Before that the guard essentially never fired in the wild, so the missing clear never showed.
A second condition decides whether it heals: usually the next reading climbs past the threshold within hours and publishes normally, clearing the message with it. An account that stays in the 0–1% band for a long stretch (mine routes most traffic elsewhere) keeps every reading inside the withhold band, so the stale message survives indefinitely.
Fix
When the fetch behind a withheld reading succeeded, clear the recorded connectivity failure — in memory and in the persisted record — before returning.
Only that class is eligible, and the scope matters:
matching weekly lows before the prior reset remain privatepins down a broader invariant — a withheld publication leaves published state alone, down tolastSourceLabelsandlastFetchAttempts. Clearing every error would have broken it. A successful fetch is direct evidence against an offline claim and nothing else, so auth, workspace and parse messages are left untouched and that invariant still holds. Eligibility reusesshouldPreserveCodexAccountSnapshotOnFailure, the same classification that allowed the message to be stored beside preserved usage in the first place, so there is one definition of "connectivity-shaped failure" read in both directions. Account scoping reuses the matching the weekly-reset candidate persistence already applies, so one account's success never clears another account's recorded failure. Nothing about which snapshot gets published changes.The persisted copy is amended from the store's own contents rather than by writing
codexAccountSnapshots: a single-account Codex refresh empties that array for its duration (UsageStore+Refresh), so writing it during a withheld cycle would erase the very snapshot the cycle is preserving. My first draft did exactly that and the regression test caught it, which is why the test also asserts the preserved snapshot survives on disk.Real behavior on the affected install
Everything below is from the live 0.55.1 install this was found on. Timestamps are UTC.
While stuck —
~/Library/Application Support/CodexBar/codex-account-snapshots.json, and the same machine at the same time:The app's own decision log for those cycles, which is what withholds the reading:
After it left the withhold band — the same file once weekly usage crossed the 1% threshold and the reading published normally:
That is the coupling this PR breaks, observed live: nothing but a publication clears the message, so while readings are withheld it cannot go away. The card was reporting an outage for 27 hours on a machine that was online the whole time.
I could not stage an after-fix capture of a withheld cycle: that needs the account inside the withhold band (weekly at or below 1%, advanced boundary, reset credit still available) and this account left it at 19:41 UTC. The threshold is a fixed constant with no override, so producing that output on demand would mean feeding a fake reading, which is not live proof. I drove a real
UsageStoreagainst a copy of the real snapshot file and the real~/.codexauth over the network to confirm the harness exercises the real pipeline end to end; it published the current 2% reading, as expected outside the band. Happy to post the withheld capture at the next early reset, and the harness is available if you would rather run it on an account that is in the band now.Tests
withheld weekly reset clears the failure recorded before it— walks the real sequence: publish → a failing fetch records and persists the message → relaunch rehydrates it → a successful fetch is withheld by the weekly guard. Asserts the published snapshot is still preserved and that the message is gone from both memory and disk.withheld publication keeps a failure recorded by the same refresh— guards the other direction: a fetch that actually failed keeps its message, because that message is what explains the stale card.a withheld publication clears only a connectivity claim— pins the eligibility boundary, including the"prior error"string the existing invariant test relies on.a withheld success restores first-failure suppression— two failures surface a message, a withheld success clears it and records the success on the consecutive-failure gate, and one later transient failure is suppressed again exactly as it would be after an ordinary published success.make checkclean; fullmake testgreen.🤖 Generated with Claude Code