Skip to content

test(usage): pin checkpoint fixture clock after scan yields - #8571

Open
atinseau wants to merge 2 commits into
lidge-jun:devfrom
atinseau:codex/test-usage-checkpoint-clock
Open

atinseau wants to merge 2 commits into
lidge-jun:devfrom
atinseau:codex/test-usage-checkpoint-clock

Conversation

@atinseau

@atinseau atinseau commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The checkpoint-digest test can fail with content_changed on a loaded worker after #8455. Its fourth cancellation check deliberately rewrites the ledger after verification, but an 8 ms row yield adds an earlier check and triggers the rewrite before verification. Pin the monotonic clock only in this fixture and restore it in finally; keep the scanner's elapsed-time and cancellation tests on their existing controlled clocks.

Addresses the post-merge CodeRabbit finding on #8455 and the Codex finding at #8455 (comment). Runtime scheduling, cancellation checks, the 750 ms health probe and lock-acquisition budget are unchanged. No user documentation change is needed for this test-only fix.

Verification

Current-head validation at c3992e31412f59d2bb1a5ca8784dc32f638c539f after merging dev a159cdde84032324f03cbce7dbf467037d0c5412:

  • bun test tests/usage/usage-ledger-scanner.test.ts: 23 pass, 0 fail in the scanner file, including elapsed-time yields and post-yield cancellation.
  • bun run typecheck: no errors in 2,435 files. bun run privacy:scan, bun run structure:check and git diff --check --cached origin/dev (before the merge commit): pass.
  • bun test tests/ci-workflows/file-size-ratchet.test.ts: 9 pass, 0 fail.
  • The personal patch has the same stable patch ID before and after the base refresh. Historical red/green at 114698a7: advancing the original checkpoint fixture clock by 9 ms makes it fail with content_changed; the fixed fixture pins only that clock. That reproduction was not repeated as part of this base refresh.
  • Resource exception: focused validation covers this test-only patch on the new base. The shared Mac runs the live proxy; previous broad runs were costly. The full suite and cross-platform CI were not run here; required CI remains for upstream. No manual full-CI workflow was dispatched.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed (no user behavior change).
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults (test clock only).

Review readiness checklist

  • Required local validation passed; commands, results, and any full-suite exception are documented.
  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

Ready for review based on the verified local checks above. This does not attest completion of a fresh automated review or required CI.

Summary by CodeRabbit

  • Tests
    • Improved the reliability of a ledger-scanning test by isolating rewrite timing and ensuring the timing spy is restored, while retaining checks for detected content changes.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Nov 11, 2026
@coderabbitai

coderabbitai Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The checkpoint-digest test now freezes performance.now() during the ledger rewrite scenario and restores the spy in finally. It still checks that the scan returns the original checkpoint and that a later incremental scan detects changed content.

Changes: Checkpoint digest test

Layer / File(s) Summary
Control timing in the rewrite scenario
tests/usage/usage-ledger-scanner.test.ts
The test freezes performance.now() around the rewrite-after-verification scenario and restores the spy in finally. It retains the assertions for the original checkpoint and the content_changed result.

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Other

Possibly related PRs

  • lidge-jun/opencodex#8455: Adds the 8 ms yield that allows the checkpoint-digest test timer to run before verification.

Suggested labels: chore, review-ready

Merge Risk: Minimal · up to 11469

This change only stabilizes a test fixture's timing and does not alter production behavior. No actionable merge risk remains beyond normal CI.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: pinning the checkpoint fixture clock after scan yields in the usage test.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between de0914a and 114698a.

Files selected for processing: 1

  • tests/usage/usage-ledger-scanner.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ✅ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

✅ 4/4 boxes ticked.

This pull request has been marked Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

Hygiene

✅ Deterministic PR hygiene checks passed.

@github-actions
github-actions Bot marked this pull request as draft November 12, 2026 10:07
@github-actions
github-actions Bot marked this pull request as ready for review November 12, 2026 10:08

This branch has not been deployed

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

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants