Skip to content

test(history): corrupt the health index through a cold connection - #8000

Merged
lidge-jun merged 1 commit into
devfrom
fix/request-history-health-cold-cache-20261030
Oct 30, 2026
Merged

lidge-jun merged 1 commit into
devfrom
fix/request-history-health-cold-cache-20261030

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Oct 30, 2026 •

Copy link
Copy Markdown
Owner

Summary

Fixes the macOS failure that remained after #7995: request-history integrity lifecycle (#7966) > routing health marks query corruption for repair and recovers current historical evidence still failed on macOS 2/4 of lane=all run 41056253802 (dev d9e0c0578, which includes #7995), at tests/usage/request-history-integrity.test.ts:429. As before, it expected {} and received live evidence.

This corrects #7995's diagnosis. Planner choice was not what kept macOS from seeing the corruption. The difference is that this case is the only corruption case that reads through the same connection before corrupting. The pre-check healthEvidenceForCandidate(input).sampleCount === 1 pulls the index pages into that connection's page cache. The helper then overwrites the index root page on disk and relies on PRAGMA shrink_memory to evict those cached pages. SQLite doesn't detect a raw file write, so if the build keeps the pages cached, the next read is served from the cache. That happened on macOS, where Bun uses the system SQLite. Every corruption case that passes on macOS corrupts a connection that has not yet read the index.

The test now closes and reopens the index after the pre-check, so it corrupts through a fresh connection, the same way the other cases do. The assertions are unchanged. The EXPLAIN-based index selection from #7995 stays: it is harmless and still guards against a planner difference.

Verification

  • Cache simulation (scratch copies, not committed): with PRAGMA shrink_memory replaced by a no-op to mimic a build that keeps the pages cached, the previous body fails with the CI signature (- Expected -1 / + Received +6) and the new body passes.
  • bun test tests/usage/request-history-integrity.test.ts on Windows: 20 pass / 0 fail.
  • NOT RUN: native macOS (no Mac host); the batched lane=all dispatch on dev after merge confirms it. Full local suite not run: the host is shared with other lanes.

Checklist

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

Summary by CodeRabbit

  • Tests
    • Updated automated checks to verify request history integrity after reopening the connection, improving coverage of index corruption scenarios.

The routing-health case read the index before corrupting it and relied on PRAGMA shrink_memory to evict the cached pages. macOS's system SQLite kept them, so the read after corruption was served from cache (macOS 2/4 of lane=all run 41056253802, after #7995). Reopen the index after the pre-check so the corruption lands on a connection that has not cached it, like the other corruption cases.
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner October 30, 2026 12:58
@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Oct 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@coderabbitai

coderabbitai Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 227811f and 53ff350.

Files selected for processing: 1
  • tests/usage/request-history-integrity.test.ts

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

Walkthrough

The routing-health corruption test closes and reopens the request-history index before corruption setup. Comments explain that cached pages may remain after shrink_memory on some SQLite builds.

Changes: Request-history integrity test

Layer / File(s) Summary
Reopen the index before corruption setup
tests/usage/request-history-integrity.test.ts:426-430
The test closes and reopens the request-history index before corruption setup. Comments note that some SQLite builds may retain cached pages after shrink_memory.

Priority: Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Bug fix

Merge Risk: Minimal · up to 53ff3

The test now exercises corruption through a fresh connection without disrupting its recovery checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: updating the history integrity test to corrupt the health index through a cold connection.
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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.
Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • 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

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner Author

Maintainer integration (lane L1, cross-platform CI). Squash-merging into dev under the maintainer-integration exception in MAINTAINERS.md. This records the decision; it is not a self-approval.

Exact head: 53ff3503f505cd134c87f9feee66e91d5ccdff87
Exact-head CI: CI run 41059399337 (headSha 53ff350…, conclusion success): gates, test changed, storage policy, api usage and ci success. enforce-target, hygiene, label and CodeRabbit pass.
Review: a fresh independent reviewer, not the author, returned PASS at this head. It confirmed that the macOS-passing corruption cases corrupt before reading through the connection and that the health case is the only one that reads first. It also confirmed that close and reopen build a new Database without reading the index, that health reads through requestHistoryDb(), and that the assertions are unchanged. It reproduced the cache simulation itself: the old body gave 0 pass / 1 fail and the new body 1 pass / 0 fail. The real file passed 20/0.
Union check vs current dev: one existing test file is modified, with no layout or baseline entry. git merge-tree against current dev is clean.

@lidge-jun
lidge-jun merged commit 9922f81 into dev Oct 30, 2026
30 checks passed
@lidge-jun
lidge-jun deleted the fix/request-history-health-cold-cache-20261030 branch October 30, 2026 13:04
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).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant