Repository navigation
Fix Windows CI after #2: metrics test, single-writer commit path - #3
Closed
ChromaBeast wants to merge 21 commits into
Closed
ChromaBeast wants to merge 21 commits into
ChromaBeast wants to merge 21 commits into
Conversation
…eckpoint, harden WAL/replication - free old tuple on overwrite/delete; compact and reuse pages before growing file - idempotent per-key WAL replay over already-flushed pages - halt writes after a post-WAL apply failure until restart - auto-checkpoint when WAL exceeds -wal-checkpoint-mb (default 64) - sync data file and WAL dir around checkpoint rotation - leader replays archived WAL generation for follower catch-up - constant-time gRPC token compare; 0600 data/WAL files - README claims, CI macOS Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… plan Add ABSENT/PRESENT/VALUE_EQUALS preconditions to Put, Delete and BatchWrite, evaluated atomically under the engine lock (gRPC ABORTED on failure). Add ScanRequest.limit for paged scans. Only effects are logged to the WAL, so recovery and replication are unchanged. Document semantics and add the roadmap plan. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Values were capped at 4,000 bytes, one slotted-page tuple. Larger values (up to MaxValueSize, 2 MiB) are now stored as a CRC-checked chain of chunk tuples headed by a manifest; the B+ tree flags the manifest RID with bit 15 of the slot ID. Small values take the existing path unchanged, existing files need no migration, and the WAL, recovery and replication still log whole values. Batches may carry up to 8 MiB of values. See docs/adr/0001-large-values.md. - Reads verify chunk magic, chain length and CRC (ErrCorruptValue); overwrite and delete free a chain only after it validates. - gRPC: MessageSizeServerOptions / MessageSizeDialOption raise the 4 MiB default; the server, followers and beastctl use them. Write-error status mapping is shared in grpc_status.go. - WAL: frames over 32 MiB are refused on write, and a torn header claiming a huge length now reads as a torn tail instead of allocating up to 4 GiB. - Storage: InsertTuple reuses tombstoned slots. Previously a recycled page's slot directory grew until it could not host a full tuple, and pages too full for one request were dropped from the reuse list; 300 overwrites of a 512 KB value grew the file to 2,120 pages, now 266, and 600 small overwrites added 264 pages, now 3. Tests: boundary sizes, overwrite/delete space reuse, crash and clean restart, CAS and batches on large values, scans, corruption detection, a mixed-size oracle across restarts, gRPC 6 MiB batches, and catch-up plus live replication of 2 MiB values. Full suite passes with -race. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017mWMfRGkKdRgKc5mtSsWEG
…g in batch limit - freeValue checked chunk magic and chain termination but not total length or CRC, so a mis-linked chain that still ended cleanly after the right number of chunks could free tuples owned by another key. It now verifies length and CRC first and leaves a failing chain in place. - MaxBatchBytes counted only value bytes. Each WAL batch operation adds 15 bytes of framing, so many tiny puts could commit a WAL frame larger than the replication message limit, and followers could never catch up on it. The limit now covers the encoded frame. Both reported by Codex review on #1; regression tests reproduce each. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017mWMfRGkKdRgKc5mtSsWEG
Values up to 2 MiB, page-slot reuse, and WAL frame cap
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…g guide Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Hand-written text exposition (no new deps). Authenticated by admin session or bearer token (--api-token / BEASTDB_METRICS_TOKEN); no loopback bypass. Adds buffer-pool hit/miss counters and checkpoint timestamps, gRPC metrics interceptors, runbook docs with scrape config and alert rules. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
WAL gains Append (assign LSN, no fsync), WaitDurable/Sync with a shared syncedLSN so one fsync covers all appended records, and a sticky fsync error. Write keeps its signature and durability but now group-commits too. Engine writes (Put/PutIfAbsent/PutIf/Delete/DeleteIf/BatchWrite[If] and replicated apply) go through a commit queue: a committer goroutine runs a group of writes under the engine lock (append + apply), issues one fsync, notifies the observer in LSN order, then releases the lock and acks writers. Reads and replication never see a write before it is durable. A failed fsync halts the engine and fails every write in the group. Concurrent Put benchmark: 7.82 ms/op -> 0.51 ms/op (~15x). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Group commit applies writes to pages before the group's fsync, so the buffer pool could flush a page ahead of its WAL records. Add a WAL barrier (WAL.EnsureDurable) called before eviction, FlushPage and FlushAll; a failing barrier blocks the page write. Adds regression tests and the upgrade spec. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Resolved conflicts in engine.go and buffer_pool.go by keeping both the group-commit/WAL-barrier fields and the metrics counters. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ench and Studio design Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… the 200-line limit No behaviour change: functions moved verbatim into wal_sync.go, wal_lifecycle.go, buffer_pool_wal.go, buffer_pool_stats.go, runner_load.go and runner_run.go. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…guideline About 400 lines is a prompt to ask whether a file does more than one thing, not a gate. Generated code, tests and data are exempt. Updates GEMINI.md (Antigravity), CONTRIBUTING.md and demo.ps1, and adds AGENTS.md (Codex) and CLAUDE.md (imports AGENTS.md). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…erformance-3d83f3 Group commit, gRPC roles, /metrics, beastbench and security policy
…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. |
Owner
Author
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4667194d0
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if e.wal.FileSize() < maxWALBytes { | ||
| continue | ||
| } | ||
| if err := e.Checkpoint(); err != nil { |
There was a problem hiding this comment.
When the WAL exceeds the threshold, this new background path invokes Checkpoint, which writes the current LSN to the metadata page before FlushAll has persisted every dirty page. If a crash or page-write error occurs after the metadata page is written but before the remaining pages are durable, recovery skips WAL records up to that LSN and can lose acknowledged writes. Persist and sync the data pages before publishing the checkpoint LSN and rotating the WAL.
Useful? React with 👍 / 👎.
Comment on lines
+14
to
+15
| if op.Type != BatchOpPut { | ||
| continue |
There was a problem hiding this comment.
When a batch contains a DELETE operation with a non-empty Value, this skips its bytes even though wal.EncodeBatchPayload serializes Value for every operation. The batch can therefore exceed MaxBatchBytes, bypassing the encoded-frame limit and the gRPC message-size headroom it is meant to protect. Reject values on deletes or include their lengths in the total.
Useful? React with 👍 / 👎.
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.mainhas moved since Group commit, gRPC roles, /metrics, beastbench and security policy #2 (UI overhaul); this branch is based on the Group commit, gRPC roles, /metrics, beastbench and security policy #2 merge commit.🤖 Generated with Claude Code