Skip to content

fix(treedb): seal explicit NoWAL batch writes after bypass - #5190

Merged
snissn merged 8 commits into
mainfrom
codex/nowal-batch-sync-repair-20261020
Oct 20, 2026
Merged

snissn merged 8 commits into
mainfrom
codex/nowal-batch-sync-repair-20261020

Conversation

@snissn

@snissn snissn commented Oct 20, 2026 •

Copy link
Copy Markdown
Owner

NoWALFast cached batches could return from explicit WriteSync after backend admission without sealing a durable root when USER leaves were stored in the index. Complete the existing sync barrier after releasing the cache's direct-write locks, and record committed backend ranges before that barrier so a barrier error preserves visibility of an already-published mutation.

Ordinary Write remains relaxed. Explicit sync now pays the durability work its contract requires; this is a correctness repair, with no speedup claim. Persistent payload dependencies, collection drains, and finite checkpoint boundaries continue through the existing checkpoint path.

Validation at local head 8af8c1a8050a42a9b04eca7fe0574bdf1aa31c93: the public regression fails against main-identical production and passes for both USER placements before Close, reopening a stable image containing the earlier ordinary write and pointer payload. The same 18 focused controls pass normal and race (57 test/subtest passes each; zero skips), covering reuse, write/barrier failures, iterator visibility, conditional ordering, and registered collection drains. Automatic streaming selectors are disabled on this main; retained backend-stream state has controlled/source coverage only. Original red and intermediate fixture failures are retained. Final 1d181d004 includes landed reader #5188, durability-contract/spec clarifications, canonical no_wal_fast documentation wording and one validated CI inventory refresh; the repair production and test postimages remain identical to local 8af8c1a80. The original documentation terminology control passes on exact final head (one test, no skips; matcher unchanged). Latest-head hosted qualification: 54/54 checks pass, including the explicit TreeDB required gate; zero failed or pending checks.

At prior integrated head ecae41db9, all five raw benchmarks passed, while the subsequent strict public replacement-cost comparison failed its unchanged bytes gate: median 222153 → 222154 B/op (+1 B), with timing and allocation count passing. Source review found the changed batch seam inactive in that point workload; the measurement’s exact allocation origin remains unresolved. The original failure is retained without a waiver. On exact current head 1d181d004, the unchanged strict comparison passed in the Linux raw-path job; no old-head rerun was accepted.

Independent GPT-6.1 Sol source review accepted exact head 1d181d0048de5493e42114b57bb4e593a1bb657f, including all three callers, unlocked checkpoint ordering, error visibility, stable-image reopen, and source-equivalent normal/race qualification. This is the independent review path permitted by the pinned-model contract in #4879. Automatic review findings are dispositioned separately.

Fixes #5189. Refs #5139 and #5145.

Summary by CodeRabbit

  • Bug Fixes

    • Improved no-WAL batch synchronization so WriteSync reliably seals pending changes before returning.
    • Ensured queued and bypassed writes are included in synchronization and remain readable after failures.
    • Improved propagation of write and checkpoint errors.
  • Documentation

    • Clarified no-WAL durability, synchronization boundaries, storage behavior, and verification coverage.
  • Tests

    • Added coverage for synchronization, batch reuse, checkpoint boundaries, persistence across reopen, and failure scenarios.

Copilot AI balanced review requested due to automatic review settings October 20, 2026 19:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-20T19:46:41.288645Z ecae41d PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6acd7277-7910-40d6-a106-435fece358d6
📥 Commits

Reviewing files that changed from the base of the PR and between a2a6f2e and 1d181d0.

📒 Files selected for processing: 9
  • .github/ci/ci_impact.json
  • TreeDB/caching/batch_sync_barrier_test.go
  • TreeDB/caching/db.go
  • TreeDB/caching/db_test.go
  • TreeDB/docs/spec/no-wal-fast-audit.md
  • TreeDB/docs/spec/storage-format.md
  • TreeDB/docs/spec/verification.md
  • TreeDB/nowal_batch_sync_test.go
  • docs/contracts/DURABILITY.md

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

📝 Walkthrough

For WAL-disabled cached batches, explicit WriteSync now serializes publication, releases cache leases, and then runs the checkpoint barrier. The barrier covers earlier queued writes and direct or streamed bypass publications. Ordinary Write retains relaxed acknowledgement behavior. Tests cover durability, reuse, errors, and reopen behavior.

Changes: No-WAL batch synchronization

Layer / File(s) Summary
Serialize writes and apply the sync barrier
TreeDB/caching/db.go
Backend writes and publication callbacks remain serialized. WAL-off WriteSync runs the checkpoint barrier after releasing cache leases. Streaming and direct bypass paths update backendRange in the publication callback.
Verify sync, bypass, and failure behavior
TreeDB/nowal_batch_sync_test.go, TreeDB/caching/batch_sync_barrier_test.go, TreeDB/caching/db_test.go, .github/ci/ci_impact.json
Tests cover durability across reopen, sync boundaries, batch reuse, write and checkpoint errors, queued writes, and large-value bypass behavior. The CI discovery digest is updated.
Document the durability boundary
TreeDB/docs/spec/no-wal-fast-audit.md, TreeDB/docs/spec/storage-format.md, TreeDB/docs/spec/verification.md, docs/contracts/DURABILITY.md
The specifications describe the sync boundary, the writes it covers, behavior after a sealing failure, and related verification coverage.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Batch as Batch.WriteSync
  participant Writer as writeBackendBatchSerialized
  participant Backend
  participant Barrier as syncBarrierAfterWrite
  participant Checkpoint
  Batch->>Writer: submit batch with sync requested
  Writer->>Backend: Write and publish batch
  Writer->>Barrier: invoke after releasing cache leases
  Barrier->>Checkpoint: seal visible root
  Checkpoint-->>Batch: return checkpoint result
Loading
Merge Risk: ⚪ Minimal · up to 1d181

The inspected sync path provides the documented durability boundary. No identified issue blocks merging after normal checks.

Pre-merge checks: Passed 4 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Docstring Coverage Inconclusive Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. (6 skipped: 5… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
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: sealing explicit NoWAL batch writes after bypass operations.
Linked Issues check Passed Issue #5189 requires explicit Batch.WriteSync durability for NoWAL direct and retained streamed bypasses, while ordinary Write stays relaxed. TreeDB/caching/db.go now records committed ranges in…
Out of Scope Changes check Passed The changes remain within Issue #5189. Production edits implement the requested explicit NoWAL sync barrier and committed-range ordering. Tests add the required return-boundary, placement, reuse, erro…
Full details: Linked Issues check

Explanation

Issue #5189 requires explicit Batch.WriteSync durability for NoWAL direct and retained streamed bypasses, while ordinary Write stays relaxed. TreeDB/caching/db.go now records committed ranges inside the serialized publication section, releases flushMu and writeMu, and then calls syncBarrierAfterWrite(sync). The code avoids backend WriteSync while cache leases are held. The public regression covers both USER-leaf placements, ordinary Write, explicit WriteSync, forced pointer payloads, a pre-Close stable-image reopen, and reuse. Focused tests cover retained backend state, relaxed and strict modes, write and barrier errors, iterator visibility after a barrier error, and closed-batch handling. The supplied validation reports normal and race success with no skips, and the documentation records the sealed-root contract.

Full details: Out of Scope Changes check

Explanation

The changes remain within Issue #5189. Production edits implement the requested explicit NoWAL sync barrier and committed-range ordering. Tests add the required return-boundary, placement, reuse, error, visibility, and ordinary-write controls. Documentation updates describe the same durability contract. The CI inventory hash refresh is administrative and does not add product behavior. No new publication owner, storage format, decoder, placement policy, or unrelated feature appears in the reviewed change summary.

Full details: Docstring Coverage

Explanation

Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. (6 skipped: 5 unsupported, 1 too large.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1
📝 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.

@snissn
snissn merged commit b03c73f into main Oct 20, 2026
54 checks passed
@snissn
snissn deleted the codex/nowal-batch-sync-repair-20261020 branch October 20, 2026 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve explicit NoWAL batch durability across direct and streamed bypass writes

2 participants