Fix deferred file budget starvation in cost scanner - #3207
Conversation
When fresh session files continuously consume the per-refresh byte budget, partially-scanned large files can be indefinitely deferred (budget starvation). This happens because newest-first ordering ensures today's files always exhaust the budget before older, larger files get a chance. Fix: After the first-pass scan loop, collect files that were deferred due to budget exhaustion but have existing partial progress (parsedBytes > 0). Run a second "deferred phase" pass that boosts the budget limit by 25%, guaranteeing these files make forward progress each refresh. The boost is additive: fresh files still get the full original budget, and the deferred phase only fires when there are actually starved partial files. This avoids changing behavior for scenarios with no starvation. Closes steipete#3203 Co-authored-by: Cursor <cursoragent@cursor.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![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: aa49594b85
ℹ️ 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".
…uarantee - Collect partially-scanned files skipped by time budget expiration into the deferred list (previously only byte-budget deferrals were collected) - Use shouldStopDeferredFile() in deferred phase to ignore time deadline and only respect byte budget, ensuring stuck files always advance - Raise effective boost floor to max(25% * byteBudget, maxFileBytes) so each deferred file can advance by a full slice even when the main budget is fully consumed - Add integration test covering time-budget-based starvation rescue Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41d877d469
ℹ️ 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".
clawsweeper
Bot
added
merge-risk: 🚨 other
|
Codex review: needs real behavior proof before merge. Reviewed August 27, 2026, 2:05 AM ET / 06:05 UTC. ClawSweeper reviewWhat this changesThe PR changes CodexBar’s bounded local cost scanner to schedule persisted pending session files fairly, rotating unfinished work behind waiting files while preserving existing byte and duration limits. Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 3 items remain Keep this PR open: the current head replaces main’s re-sorting behavior with persisted queue rotation and has focused coverage, but it still needs redacted after-fix evidence from a real local session corpus before merge. Likely related people: steipete (high confidence) and Luís Miguel (medium confidence). Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCodexBar scans local Codex session JSONL files into a SQLite-backed usage cache, then reports the resulting spend totals. The bounded scheduler decides which incomplete or newly discovered files can advance during a refresh. flowchart LR
A[Local session files] --> B[File discovery]
B --> C[Persisted pending queue]
C --> D[Bounded scan scheduler]
D --> E[SQLite usage cache]
E --> F[Spend reports]
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the bounded persisted-queue approach once a real local-corpus run shows older partial files advancing under continued fresh writes without exceeding the configured scan budget. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main reorders the selected pending prefix newest-first on every refresh, while the added deterministic fixtures model continual fresh work and show the required queue-turn behavior. Is this the best way to solve the issue? Yes: persisted FIFO-style rotation is narrower than a second boosted pass because it guarantees waiting work a turn while retaining the existing per-file, per-refresh, and duration limits. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0660761522b1. 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 (3 earlier review cycles)
|
- Share scanState across first pass and deferred phase to preserve contributingSessionIds/seenFileIds deduplication (P1 fix) - Record deferred-phase attempts in attemptedPaths so completed files are removed from the pending lookback queue (P2 fix) - Use 1μs time budget instead of 1ms in the time-expiration test to guarantee deterministic deadline expiration regardless of host speed - Regenerate parser hash Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb1a5d6a22
ℹ️ 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".
- Sort deferred files by remaining work (ascending) so near-complete files finish first instead of one large file monopolizing the boost - Record workRecorder scan attempts in the deferred phase for accurate performance metrics Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35ff9a1865
ℹ️ 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".
Actively-growing files have changed mtime/size, causing the cache freshness check to reject them. They re-enter the first pass through newest-first ordering and never appear in the deferred list. Co-authored-by: Cursor <cursoragent@cursor.com>
clawsweeper
Bot
added
merge-risk: 🚨 availability
steipete
left a comment
steipete
left a comment
There was a problem hiding this comment.
The starvation report is credible, but the current rescue pass still needs changes before landing.
P2 — Preserve the existing budget and deadline while making scheduling fair. The effective boost is at least one full file slice, so default limits rise from 512 MiB to 768 MiB, not by 25%. Sorting rescue work by cached remaining bytes also does not ensure rotation: a growing incomplete file can reach budget rejection with stale cached metadata and repeatedly win that ordering. Please schedule a bounded turn for pending work within the existing ceiling and deadline, preserving fairness across refreshes rather than adding another allowance. The existing persisted pending-file queue appears sufficient; this does not need a new storage format.
P2 — Preserve deployed caches across the scheduler-only hash change. The generated hash changes from the shipped 0.55.1 value c6c46a376ba16304, which is not in the compatible predecessor set. That would rebuild the SQLite cache despite unchanged parsed-row semantics, contrary to the cache-preservation contract in #3051. Please prove adoption preserves rows, resume state, and the correctly scoped retained report. Also check pi/OMP: its pricing fingerprint includes the parser hash, so native SQLite compatibility alone does not prevent unchanged-history reparsing there.
Regression proof needs to fail on the old scheduler. In the fresh-work fixture, the 20 small files leave enough of slice * 3 for the old file's next slice; the progress assertion does not establish starvation. Use explicitly ordered mtimes and fresh work that demonstrably exhausts each refresh's budget, several pending files including a growing file, persisted reloads between passes, and final totals checked against an unbounded fixture scan. The one-microsecond timeout test should use the existing controlled-clock seam to distinguish expiry before admission from expiry after consumed work.
The earlier attempt-recording and shared scan-state problems are fixed in this revision. The JSONL reader still checks elapsed time between chunks, so this review does not claim it necessarily consumes a whole slice after deadline expiry. The remaining issue is consistent admission and real fairness proof. CI is currently awaiting workflow approval, not green; the prior bot run did not execute Swift tests.
Integrate current main and replace the boosted rescue pass with persisted queue rotation inside the existing byte, duration, and selection limits. Preserve deployed cache rows and scoped reports across this scheduler-only transition, including narrowly compatible Pi/OMP pricing fingerprints. Co-authored-by: idevlab <idevlab@outlook.com>
clawsweeper
Bot
added
rating: 🦪 silver shellfish
steipete
left a comment
steipete
left a comment
There was a problem hiding this comment.
Reviewed the complete repaired delta at 25c4a45. This supersedes my earlier changes-requested review. The boosted second pass is gone; pending work uses the ordinary shared scan state, attempt accounting and deadline admission, with persisted fair rotation after original-prefix completion. Discovery and identity reconciliation no longer reorder waiting files. The six inline threads are resolved.
The deployed-cache contract is preserved: no SQLite row/schema/accounting change or rebuild, correctly scoped retained reports and resume state survive, and Pi/OMP recognizes only the reviewed hash transition with identical pricing inputs. Real pricing changes still invalidate. The regression failed against the submitted proposal, then passed through the actual JSONL reader, scheduler, SQLite persistence and report construction; 514 waiting files drain into an exact 1,034-file inventory matching independent unbounded totals. Controlled clocks cover both deadline-admission cases.
Maintainer proof decision: I accept this deterministic real-file production-pipeline proof for the bounded local-scheduler change. The input data is synthetic, but the scheduler, reader, persistence and report paths are not mocked. No natural live-account corpus or GUI recovery is claimed. The fresh public review found no actionable code or security findings; its remaining real-corpus request is explicitly treated as a proof limitation, not an unaddressed implementation finding. Its package setup failure occurred before Swift tests ran.
All 249 focused tests, formatting/lint, parser-hash verification and independent complete-diff review passed. Full make test passed all 938 selections/79 groups first try, zero failures/retries/timeouts, in 1,101.7 seconds. Exact-head CI passed every macOS/Linux, plugin-golden, lint and aggregate check without a CI rerun. The macOS shards took 23m46s and 32m49s. Approved for merge.
|
Landed through #3207 as 2a38c2c, preserving the original contribution and credit to @IchenDEV. Issue #3203's older-partial-file starvation is fixed on main. The scheduler now gives persisted waiting work a turn inside the original byte, duration and selection limits, then rotates unfinished serviced files behind other waiters. It removes the boosted second pass and preserves the existing shared scan state, completion accounting and deadline admission. Compatible deployed caches retain their rows, resume state and scoped reports; Pi/OMP still invalidates when real pricing changes. Verification on 25c4a45 used these isolated commands: suites=(
CostUsageFairSchedulingTests PiSessionCostCompatibilityTests PiSessionCostScannerTests
CostUsageBoundedProgressTests CostUsageCatchUpCompletionTests CostUsageCatchUpProgressTests
CostUsageStoreTests CostUsagePerformanceGateTests CostUsageScannerTests CostUsageCacheWideMigrationTests
CostUsageScannerForkSplitTests CostUsageScannerForkSplitPricingEvidenceTests CodexForkAppendResumeTests
CostUsageCancellationTests ProviderArchitectureGatekeeperTests
)
for suite in "${suites[@]}"; do
env -u CODEXBAR_ALLOW_TEST_KEYCHAIN_ACCESS CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 \
CODEXBAR_TEST_CODEX_FILE_ISOLATION=1 swift test --filter "$suite"
done
env -u CODEXBAR_ALLOW_TEST_KEYCHAIN_ACCESS CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 \
CODEXBAR_TEST_CODEX_FILE_ISOLATION=1 make check
Scripts/regenerate-codex-parser-hash.sh --check
env -u CODEXBAR_ALLOW_TEST_KEYCHAIN_ACCESS CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 \
CODEXBAR_TEST_CODEX_FILE_ISOLATION=1 make testThe 249 focused tests passed; formatting/lint found zero violations in 2,007 files. Full suite: 938 selections/79 groups, all first-pass, zero failures/retries/timeouts, 1,101.7 seconds. Independent review covered the complete final delta and found no actionable findings. The original submitted scheduler failed the new real-file fixture, exceeding a 1,024-byte refresh ceiling and leaving older waiters stalled. The repaired production JSONL reader/scheduler/SQLite/report pipeline advances all 514 waiters and drains to exactly 1,034 files and 570,790 tokens; costs match an independent unbounded scan within $0.000000001. Controlled clocks cover expiry before admission and after consumed work. Additional tests preserve cache rows, resume state and scoped reports with zero rebuild/head parses, and prove unchanged Pi/OMP history does not reparse while genuine pricing changes still invalidate. These are synthetic data in real temporary files and SQLite stores, not live-account or GUI proof. The initial repair exposed completion regressions and style/line-anchor failures; those were corrected without weakening existing assertions, then all focused and full gates passed on the final code. The public bot found no actionable code/security findings, and its own package setup failed before executing Swift tests. The explicit maintainer proof decision accepts the executed production-pipeline regressions, without claiming natural-corpus recovery. Exact-head CI passed every required check and GitGuardian was green. macOS shards took 23m46s and 32m49s; Linux x64/ARM64/musl passed, along with plugin goldens, lint and the aggregate gate. No CI rerun. All six review threads are resolved and the final head was approved. Main was fast-forwarded and verified clean, with the tested commit reachable and tree-equal to the merge. No release was published. |
2 participants
Summary
Fixes #3203. Older partially scanned Codex sessions can stop advancing when newer or growing sessions repeatedly consume each refresh's budget. This repair uses the existing persisted pending-file queue to give waiting work a bounded turn, then rotates unfinished serviced files behind the remaining waiters. Newly discovered work is appended without reordering existing waiters.
The contributor's boosted second pass has been replaced: pending and fresh work share the existing per-file, per-refresh and duration limits. The selection stays bounded, including queues larger than 512 files and deduplicated prefixes. Completion, fork retry, cancellation and exact-inventory behavior remain covered. Rotation alone is not treated as progress.
The scheduler-only parser hash transition preserves the shipped 0.55.1 SQLite rows, resume state and correctly scoped retained report. Pi/OMP recognizes only this transition with identical pricing inputs; changed prices and unrelated parser transitions still invalidate normally. No schema, token accounting, retention or scan-root change.
Thanks @IchenDEV for the report and original contribution; the original PR history is preserved.
Verification
The new real-file regression failed against the submitted scheduler: older waiters did not all advance, and the boosted pass exceeded the original byte ceiling. The repaired fixture exercises explicit modification times, continual fresh work, a growing partial file, persisted SQLite reloads, 514 waiting files and a 1,034-file eventual inventory in both byte-only and duration-limited modes. After writes stop, bounded scans match an independent unbounded scan's token and cost totals. Controlled clocks verify expiry before admission and after consumed work.
Focused coverage also exercises remaining capacity for fresh/appended files, queue-order preservation through discovery and identity reconciliation, zero rebuild/head parsing during compatible SQLite adoption, unchanged Pi/OMP parsing, genuine pricing invalidation, completion, migration, forks, cancellation, performance and architecture gates. All tests use temporary fixtures with Keychain access suppressed and Codex credential-file isolation enabled; this is not a live-account or GUI test.
All 249 tests across 15 focused suites passed.
make checkand normal parser-hash verification passed. Independent complete-diff review found no actionable findings. Fullmake testpassed all 938 selections across 79 groups on the first try, with zero failures, retries or timeouts (1,101.7 seconds). Exact-head CI passed all macOS/Linux, plugin-golden, lint and aggregate checks without a CI rerun. The macOS shards took 23m46s and 32m49s.