Repository navigation
Fix Windows CI after #2: metrics test, single-writer commit path - #4
Merged
Merged
Conversation
…s it DiskFreeBytes is unix-only, so TestMetricsAuthAndFormat failed on Windows CI with a missing beastdb_disk_free_bytes line. Require the gauge only when DiskFreeBytes reports support. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Instead of waking a dedicated committer goroutine for every write, the writer that finds no commit in progress runs the queued group on its own goroutine; if more writers queued meanwhile, a drain goroutine serves them. Group commit semantics are unchanged (one fsync per group, lock held across it, observer in LSN order). Aims to remove a cross-goroutine wake-up per uncontended write, which can cost up to a timer tick on Windows. Not yet verified on Windows. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ChromaBeast
added a commit
that referenced
this pull request
Nov 13, 2026
Fix Windows CI after #2: metrics test, single-writer commit path
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Follow-up to #2, whose Windows CI job failed after merge. Two commits:
test(metrics): require the disk-free gauge only where it is reported.metrics.DiskFreeBytesis unix-only (the gauge is omitted elsewhere, as documented), butTestMetricsAuthAndFormatrequiredbeastdb_disk_free_bytesunconditionally, so it failed on Windows CI withmissing "beastdb_disk_free_bytes ". The test now checks the gauge only whenDiskFreeBytesreports support. This one is a certain fix.perf(engine): let the first writer lead its own commit group. Instead of waking a dedicated committer goroutine for every write, the writer that finds no commit in progress runs the queued group on its own goroutine; if more writers queued meanwhile, a drain goroutine serves them. Group-commit semantics are unchanged (one fsync per group, engine lock held across it, observer notified in LSN order, one group at a time so queue order is LSN order).Closenow waits for the in-flight group and rejects later writes.Why
TestEngineSystemWorkloadBenchmarkalso failed on Windows CI in #2: 398 ops/s against its fixed 1000 ops/s floor, with p95 latency 16.5 ms. Earlier Windows runs onmainmeasured 1,351 and 1,923 ops/s with p95 about 3.5 ms. My hypothesis is that waking a separate goroutine for every uncontended write costs up to a timer tick (about 15.6 ms) on Windows, which matches the p95.Things a reviewer should know
d8c1280) the benchmark gets about 1,560 to 1,710 ops/s in a full parallelgo test ./... -racerun; on the merged branch about 830 to 930 (fix branch included). Run alone, the two are within about 4 to 7% of each other. I counted fsyncs (one per write, same as before) and ruled out thebeastbenchtests and synthetic fsync load as the cause. The remaining difference under a full parallel run is unexplained.mainwas rewritten and conflicted. This branch is the same two commits rebased onto the currentmain.🤖 Generated with Claude Code