Skip to content

fix(codex): exclude queue waiting from catch-up delays - #3566

Merged
steipete merged 2 commits into
mainfrom
oss-triage/20260911-b6-scheduler
Sep 12, 2026
Merged

steipete merged 2 commits into
mainfrom
oss-triage/20260911-b6-scheduler

Conversation

@steipete

@steipete steipete commented Sep 11, 2026 •

Copy link
Copy Markdown
Owner

Automatic Codex history catch-up currently counts time waiting behind another account or provider on the shared serial scan queue as active work. Its duty-cycle policy then multiplies that wait into additional sleep. Measure the scan inside the executor and use its own active duration in both usage and Spend Dashboard workers.

Preserve the existing power/thermal policy, initial delay, scan budgets, cancellation, and complete-history publication boundary. Refs #3508 and #3411; these broader reports remain open. This does not change held PR #3509 or enable its dormant accelerated path. The changelog entry will be centralized in the final batch notes PR.

Validation so far: 48 focused regression tests passed, independent P0–P2 review clean, and a real integration executable using the production executor and policy parsed 10,000 synthetic JSON records. A 460 ms total await contained only 32 ms of active work: the old delay was 459 seconds, while the corrected delay was 32 seconds. The final full local suite passed 1,072 selections in 90 groups with zero failures or retries. Signed native menu smoke, make check, 41 architecture-gate tests, and final branch review pass. Exact-head CI is green: https://github.com/steipete/CodexBar/actions/runs/34658785497.

@clawsweeper

clawsweeper Bot commented Sep 11, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 11, 2026
@clawsweeper

clawsweeper Bot commented Sep 11, 2026 •

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 11, 2026, 8:22 PM ET / September 12, 2026, 00:22 UTC (Revision 3).

ClawSweeper review

What this changes

Measures Codex history scans inside the shared executor and uses their active duration to schedule subsequent usage and Spend Dashboard catch-up passes, with regression coverage and documentation.

Merge readiness

✅ Ready for maintainer review

This remains a useful, focused fix: current main and the latest release still count queue waiting toward catch-up delays. No blocking defect was found, and the supplied production-executor trace supports the correction.

Priority: P2
Reviewed head: cdfefc954488afff6ed9b16eddf839dee652935f

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused correction with relevant production-path timing evidence, regression coverage, and no identified blocking defect.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The supplied executable trace exercises the production serial executor and scheduling policy under injected queue contention, showing 460 ms awaited versus 32 ms active and a corresponding reduction in calculated sleep; worker regression tests support the integration.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The supplied executable trace exercises the production serial executor and scheduling policy under injected queue contention, showing 460 ms awaited versus 32 ms active and a corresponding reduction in calculated sleep; worker regression tests support the integration.
Evidence reviewed 8 items Policy and review scope: Read the full root AGENTS.md and found no nested AGENTS.md or maintainer notes under the inspected source, test, docs, and .agents trees. Applied concurrency, provider isolation, focused coverage, and isolated-defaults guidance. Builds and tests were not run during this read-only review.
Current main still contains the defect: The usage worker starts its clock before awaiting the queued scan and calculates previousActiveDuration after that await. The Spend Dashboard worker uses the same outside-queue timing pattern.
Latest release retains outside-queue timing: The supplied v0.59.0 release revision also starts timing before the queued await and uses the entire elapsed duration; this correction is not already present there.
Findings None None.
Security None None.

How this fits together

CodexBar scans local session history on a shared serial queue to calculate usage and spend. Background workers use scan duration and power conditions to schedule further work before publishing completed history.

flowchart TD
  A[Local Codex history] --> B[Shared serial scan queue]
  B --> C[Measure executing scan]
  C --> D[Cache and progress status]
  C --> E[Active duration]
  E --> F[Power and thermal scheduling policy]
  F --> B
  D --> G[Usage and Spend Dashboard publication]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +54/-30, tests +96/-21, docs +4 The modest production growth provides shared timing measurement, supported by three focused new regression tests.

Technical review

Best possible solution:

Keep scan-duration accounting inside the executor while retaining existing resource safeguards and complete-history publication, with broader discovery and freshness work tracked separately.

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

Yes: current-main workers time the queued await, and the unchanged AC policy multiplies that duration by 999. Queue contention therefore creates excess sleep; this review established the mechanism from source without executing tests.

Is this the best way to solve the issue?

Yes: measuring inside the existing executor is the narrowest shared correction, and both consumers use it without altering cancellation, settings, scan budgets, or publication rules.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 830cdd86c4b6.

Labels

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The supplied executable trace exercises the production serial executor and scheduling policy under injected queue contention, showing 460 ms awaited versus 32 ms active and a corresponding reduction in calculated sleep; worker regression tests support the integration.

Label justifications:

  • P2: Queue contention can unnecessarily delay local Codex history catch-up, with a bounded effect on usage and spend freshness.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The supplied executable trace exercises the production serial executor and scheduling policy under injected queue contention, showing 460 ms awaited versus 32 ms active and a corresponding reduction in calculated sleep; worker regression tests support the integration.
  • proof: sufficient: Contributor real behavior proof is sufficient. The supplied executable trace exercises the production serial executor and scheduling policy under injected queue contention, showing 460 ms awaited versus 32 ms active and a corresponding reduction in calculated sleep; worker regression tests support the integration.

Evidence

What I checked:

  • Policy and review scope: Read the full root AGENTS.md and found no nested AGENTS.md or maintainer notes under the inspected source, test, docs, and .agents trees. Applied concurrency, provider isolation, focused coverage, and isolated-defaults guidance. Builds and tests were not run during this read-only review. (AGENTS.md:1, cdfefc954488)
  • Current main still contains the defect: The usage worker starts its clock before awaiting the queued scan and calculates previousActiveDuration after that await. The Spend Dashboard worker uses the same outside-queue timing pattern. (Sources/CodexBar/UsageStore+CodexCostCatchUp.swift:175, 830cdd86c4b6)
  • Latest release retains outside-queue timing: The supplied v0.59.0 release revision also starts timing before the queued await and uses the entire elapsed duration; this correction is not already present there. (Sources/CodexBar/UsageStore+CodexCostCatchUp.swift:175, dca9c5f4b7a2)
  • Timing and cancellation boundary: runTimed starts its monotonic clock inside the existing queued work closure and delegates cancellation to the unchanged executor. The fetcher includes scanning and status reading in that measurement; both workers consume activeDuration. The changed interfaces are package-scoped, with no persisted-format or public plugin API change. (Sources/CodexBarCore/CostUsageScanExecutor.swift:36, cdfefc954488)
  • Production-path timing evidence: The supplied owner comment at fix(codex): exclude queue waiting from catch-up delays #3566 (comment) identifies the reviewed head and reports a separately built executable using the production serial executor and unchanged policy behind another queued task: 10,000 parsed records, 0.459539541 seconds awaited, 0.031833417 seconds active, and calculated delay reduced from 459.080001459 to 31.801583583 seconds. This directly exercises the changed timing boundary; the native menu smoke is supplemental. (cdfefc954488)
  • Regression coverage and review continuity: The patch adds one queue-contention timing test and one duration-consumption test for each worker. The current head differs from the earlier reviewed head only by architecture-test line reanchoring; provider memberships and anchors are unchanged. Prior review projections contain no outstanding findings. The supplied owner comment reports 1,072 passing selections, make check, and passing head-specific CI. (Tests/CodexBarTests/CostUsageScanExecutorTests.swift:16, cdfefc954488)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Yuxin-Qiao: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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 (2 earlier review cycles)
  • reviewed 2026-09-11T23:10:51.104Z sha f0161b1 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-11T23:41:53.200Z sha cdfefc9 :: needs maintainer review before merge. :: none

@steipete

Copy link
Copy Markdown
Owner Author

Verified head cdfefc954488afff6ed9b16eddf839dee652935f: exact-head CI passed at https://github.com/steipete/CodexBar/actions/runs/34658785497.

The final full local run passed all 1,072 selections in 90 groups, with zero failures, retries, or timeouts. The targeted scheduler/executor tests and 41 architecture-gate tests pass; existing provider memberships and fingerprints are unchanged. Make check and final independent P0–P2 branch review pass.

A separately built integration executable used the production serial executor and unchanged scheduling policy while parsing 10,000 synthetic JSON records behind another queued task. Its output was:

parsed_records=10000
awaited_seconds=0.459539541 active_seconds=0.031833417
old_delay=runAfter(459.080001459) fixed_delay=runAfter(31.801583583000003)

The old-delay value feeds the measured total await into the existing policy, matching the old workers' input; the fixed value uses the executor's own measured work. A freshly built Developer ID–signed app also passed native menu interaction in the isolated synthetic smoke mode.

Ready for squash. This corrects queue-wait debt only: retain the initial automatic delay, resource safeguards, and complete-history publication. Leave #3508 and #3411 open, and keep #3509 held. The release note is in #3567.

@clawsweeper clawsweeper Bot added the proof: sufficient Contributor real behavior proof is sufficient. label Sep 12, 2026
@steipete
steipete merged commit 9376229 into main Sep 12, 2026
9 checks passed
steipete added a commit that referenced this pull request Sep 12, 2026
Collect the ordered 0.59.1 Unreleased notes and contributor credits after #3539, #3566, #3547, #3525 and #3544. Preserve all released sections.

Separate process-fixture startup from measured cleanup deadlines and use the existing task-local authentication hook in Claude retry fixtures. Retain retry, timeout, ownership, and dedicated authentication tests. Dependency pins remain current within their existing policies; larger migrations remain separate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant