[codex] preserve failed local development#1
Closed
Cmochance wants to merge 1 commit into
Closed
Conversation
Owner
Author
|
Closing per maintainer request to prevent accidental merge into main. |
4 tasks
Cmochance
added a commit
that referenced
this pull request
May 10, 2026
Three review agents flagged a mix of MUST / HIGH / SHOULD items on the perf bundle. This commit addresses every blocker and the test gaps; nice-to-have items (B8 in-flight dedupe via Notify, corrupt- profile.json logging) are left for the broader follow-up backlog. Errors no longer silently fall through: - `try_refresh_via_chatgpt_api` (mac + win) now matches on the inner `AppError`. Re-login signatures (`token_invalidated`, `refresh_token_reused`, etc.) propagate as `AUTH_REFRESH_RELOGIN_REQUIRED` immediately instead of falling through to the app-server RPC fallback that would hit the same dead refresh_token. Other errors `eprintln!` and fall through. Predicate is hoisted to `chatgpt_api::looks_like_relogin_required` so the existing matcher in `codex_app_server::map_rpc_error` can share it next time it's touched. - Bulk plan refresh's four `let _ =` swallows on the per-profile API call, both `sync_profile_*` writes, and the post-loop index reload all gain `eprintln!` lines so a systematic failure across every profile is visible in stderr instead of silently leaving the dashboard stale. - Front-end `applySnapshot` failure after a per-card Refresh logs via `console.warn`. Toast suppression matches pre-PR-3 `refreshAllData(false)` UX (the user already saw the success toast and a follow-up failure isn't actionable), but the warning makes a systematic failure debuggable. Active-card live-quota regression fix (code-reviewer Should #1): - B4 used to skip `getCurrentLiveQuota` unconditionally after a per-card Refresh. That worked for non-active cards but lost the "JSONL session count newer than API value" merge for the active card. Now we re-fetch live quota only when the refreshed profile is the active one — saves the JSONL pass on non-active refreshes while preserving the pre-PR active-panel behavior. Threshold deduplication: - The `6 * 60 * 60 * 1000` literal lived in three places (mac refresh_runtime, win refresh_runtime, dashboard.rs bulk gate). Hoist to `chatgpt_api::PLAN_FRESHNESS_TTL_MS` so a future tuning change moves all three sites in lock-step. OnceLock no longer poisons the HTTP client: - B6's cache shape changed from `OnceLock<Result<Client, String>>` to `OnceLock<Client>`. A first-call build failure no longer permanently caches the error; subsequent calls re-attempt the build. New tests: - `should_force_oauth_rotation` mac + win each get four cases: missing `last_plan_check_ms`, recent (rotation skipped), stale (rotation forced), and future-dated (clock-skew defense: rotation skipped, documenting the deliberate "trust local cache" stance). 96 tests passing on macOS (was 92, +4). Cache invariant comment added to `profiles_index::store_cached_index` documenting the rule every per-profile mutation site already follows: any caller mutating per-profile metadata must call `load_profiles_index` afterwards so the 250 ms cache repopulates. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Cmochance
added a commit
that referenced
this pull request
May 10, 2026
* perf(chatgpt_api): share a single reqwest::Client across calls Every `refresh_profile_via_api*` invocation used to call `build_http_client()` which spun up a fresh blocking reqwest Client, and therefore a fresh TLS connection pool, a fresh handshake state, and a fresh resolver cache. Bulk refresh paid that cost N times in a row; single-card Refresh paid it once per click. `reqwest::blocking::Client` already wraps an `Arc<Inner>`, so handing clones from a process-wide `OnceLock<Result<Client, _>>` is cheap and lets the underlying connection pool / TLS state stick around between calls. Build error is captured into the cell once and surfaced as `HTTP_CLIENT_BUILD_FAILED` to every caller, matching the prior behavior. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * perf(refresh): drop force_token_rotation on Refresh button repeat clicks `try_refresh_via_chatgpt_api` was hardcoded to `force_token_rotation: true` so the id_token claims (plan tier, subscription expiry) would move within a single click. Reasonable when the user clicked Refresh once after a plan change, but every repeat click in the same session paid the full OAuth POST + usage GET round-trip even though the cached plan info was certainly fresh. Gate the forced rotation on `last_plan_check_ms`: rotate when the cached check is older than 6 hours (or absent); otherwise reuse the cached `id_token` claims and let `chatgpt_api`'s inner `access_token_expired` predicate drive rotation only when the access_token itself is about to expire. The "user clicked Refresh and expects fresh plan info" guarantee still holds — once per day per profile the heavier path runs. What changes is that a frantic-click session now costs 1 × OAuth POST across N clicks instead of N × OAuth POST. Mac and Windows mirrors get the same change pending the planned mac/win refresh_runtime merge. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * perf(dashboard): gate bulk plan refresh on last_plan_check_ms `refresh_all_oauth_profile_plans_silent` runs at app launch and on each local-day rollover. It used to walk every OAuth profile unconditionally with `force_token_rotation: true`, which on a 5-account workspace meant 10–25 s of background OAuth + usage round- trips trickling cards updating one-by-one — visible to the user as plan / quota fields changing under their cursor for tens of seconds after each launch. Skip any profile whose `last_plan_check_ms` is younger than 6 hours. Sized to be longer than the 5-min silent dashboard ticker (active profile keeps moving) but shorter than the day-rollover trigger so a paid-tier change on a parked profile still surfaces by the next day. Repeat launches inside the same working day now cost zero round- trips. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * perf(profiles_index): 250ms in-process result cache for back-to-back IPC pairs The front-end's `refreshAllData` issues `get_profiles_snapshot` and `get_current_live_quota` concurrently via `Promise.all`. Each Tauri command independently calls `load_profiles_index`, so the disk-side reconcile + atomic save of `profiles.json` happened twice in near-lockstep — wasted I/O, wasted index-rewrite, occasional needless mtime-bumping on every dashboard refresh. Cache the rebuilt index in process for 250 ms keyed by `codex_home`. Sized to dedupe the back-to-back pair (which fire microseconds apart) without holding stale data across user-visible state changes — every action handler that mutates profiles already calls `load_profiles_index` at the end of its operation and refreshes the cache as a side-effect, so the next IPC sees the latest data. Tests bypass the cache via `cfg(test)` so per-test fs setup remains observable on the next call without explicit invalidation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * perf(refresh): drop redundant refreshAllData after per-card Refresh `drainRefreshQueue` used to call `refreshAllData(false)` after every successful per-card Refresh, which fans out to two IPCs: `getProfilesSnapshot` (rebuilds + saves the profiles index again even though `refresh_profile` just did) and `getCurrentLiveQuota` (walks the JSONL parse cache for data that's already live in the index we just refreshed). The backend's `refresh_profile` already wrote the new quota + plan into `profiles.json` before returning. The front-end just needs to re-read the snapshot to pick those up — `getCurrentLiveQuota`'s only job here is hitting the JSONL cache for a value the backend already authoritatively persisted. Replace the call with a single `getProfilesSnapshot` + `applySnapshot`. Failures are best-effort (transient snapshot fetch failures leave the cards on their pre-refresh state, no worse than swallowing the failure entirely). Saves one IPC round-trip and one JSONL cache pass per Refresh click; combined with the `profiles_index` 250 ms cache this also dedupes the index reconcile on the snapshot side. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * review: address PR-3 audit findings Three review agents flagged a mix of MUST / HIGH / SHOULD items on the perf bundle. This commit addresses every blocker and the test gaps; nice-to-have items (B8 in-flight dedupe via Notify, corrupt- profile.json logging) are left for the broader follow-up backlog. Errors no longer silently fall through: - `try_refresh_via_chatgpt_api` (mac + win) now matches on the inner `AppError`. Re-login signatures (`token_invalidated`, `refresh_token_reused`, etc.) propagate as `AUTH_REFRESH_RELOGIN_REQUIRED` immediately instead of falling through to the app-server RPC fallback that would hit the same dead refresh_token. Other errors `eprintln!` and fall through. Predicate is hoisted to `chatgpt_api::looks_like_relogin_required` so the existing matcher in `codex_app_server::map_rpc_error` can share it next time it's touched. - Bulk plan refresh's four `let _ =` swallows on the per-profile API call, both `sync_profile_*` writes, and the post-loop index reload all gain `eprintln!` lines so a systematic failure across every profile is visible in stderr instead of silently leaving the dashboard stale. - Front-end `applySnapshot` failure after a per-card Refresh logs via `console.warn`. Toast suppression matches pre-PR-3 `refreshAllData(false)` UX (the user already saw the success toast and a follow-up failure isn't actionable), but the warning makes a systematic failure debuggable. Active-card live-quota regression fix (code-reviewer Should #1): - B4 used to skip `getCurrentLiveQuota` unconditionally after a per-card Refresh. That worked for non-active cards but lost the "JSONL session count newer than API value" merge for the active card. Now we re-fetch live quota only when the refreshed profile is the active one — saves the JSONL pass on non-active refreshes while preserving the pre-PR active-panel behavior. Threshold deduplication: - The `6 * 60 * 60 * 1000` literal lived in three places (mac refresh_runtime, win refresh_runtime, dashboard.rs bulk gate). Hoist to `chatgpt_api::PLAN_FRESHNESS_TTL_MS` so a future tuning change moves all three sites in lock-step. OnceLock no longer poisons the HTTP client: - B6's cache shape changed from `OnceLock<Result<Client, String>>` to `OnceLock<Client>`. A first-call build failure no longer permanently caches the error; subsequent calls re-attempt the build. New tests: - `should_force_oauth_rotation` mac + win each get four cases: missing `last_plan_check_ms`, recent (rotation skipped), stale (rotation forced), and future-dated (clock-skew defense: rotation skipped, documenting the deliberate "trust local cache" stance). 96 tests passing on macOS (was 92, +4). Cache invariant comment added to `profiles_index::store_cached_index` documenting the rule every per-profile mutation site already follows: any caller mutating per-profile metadata must call `load_profiles_index` afterwards so the 250 ms cache repopulates. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This draft PR preserves the current local working state from a failed development pass so the main branch is not polluted.
It is intentionally not ready for merge. The follow-up work should inspect, split, or discard parts of this diff before any normal review.
Current contents captured in this branch:
Validation status: