P1 bundle: flusher retry, fail-closed manifest, strict SQL, full pruning, [from,to) ranges, shutdown drain - #10
Merged
Merged
Conversation
…ing, half-open ranges, shutdown drain Closes the six P1 items from external review. #1 — Flusher failure recovery. attempt_flush returns the events on any pre-commit failure path (segment create / write / finish); handle_message re-inserts them into the memtable so the next size/age trigger retries. Post-commit (manifest save) failures still surface as a retry but with an empty event list — the events are durable in the still-sealed WAL and recovery replays them on next start; returning them here would risk a double-flush. #2 — Corrupt manifest fails closed. recovery.rs no longer silently falls back to Manifest::default() — it returns InvalidData and the process refuses to start. Fresh DBs (no manifest at all) still start normally. The error message points the operator at the .tmp sibling for manual recovery. #3 — Strict SQL parser. Rewritten to reject silently-mapped queries: - SUM only accepts the `quantity` column (was: any column → quantity) - COUNT only accepts COUNT(*) (was: any expression → 1) - <, <=, >, >= each have distinct effect on the half-open range - OR / HAVING / aliases / SELECT * / unknown tables fail with a specific error - AND-only WHERE; ranges only on timestamp_ms / hour_start_ms #4 — Full segment pruning. collect_raw_events now uses every relevant SegmentMeta field before opening a file: time range (half-open), bucket derived from account_id, account_id min/max, and per-segment product/meter/model ID sets via filter_intersects. Same pruning applied to rollup segments in the rollup query path. #5 — Half-open [from, to) range semantics. matches_plan rejects events at exactly to_ms; segment overlap check uses `s.min < to && s.max >= from`; rollup_upper is the exclusive boundary `min(to, watermark)`. Adjacent monthly queries no longer double-count boundary events. #6 — Shutdown drain. main.rs flushes the memtable + rotates the WAL on ctrl+c before letting the flusher loop exit. The age-based flush trigger from the rollup worker (added earlier) covers steady-state; the shutdown drain catches the final outstanding batch. Tests (tests/p1_fixes.rs, 15 tests): - Corrupt manifest aborts recovery with a clear error - Fresh-DB recovery still works - SQL parser rejects each silent-mapping case + accepts canonical form - `<` vs `<=` and `>` vs `>=` produce different ranges - Half-open [from, to): event at boundary lives in exactly one adjacent range; sum across both equals event count exactly - Bucket pruning correctness: account query result matches the true account-event sum across 10+ unrelated-bucket segments - product_id filter prunes segments without that product - build_segment_meta sanity check post-refactor All 44 prior tests still pass; total now 59. Builds clean under RUSTFLAGS=-D warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
Closes the six P1 items from external review in a single PR. Each is a path to either silently-wrong query answers or data getting stranded between subsystems.
What's in
#1 — Flusher failure recovery.
attempt_flushreturns the unflushed events tohandle_messageon any pre-commit failure (segment create / write / finish). The handler re-inserts them into the memtable so the next size or age trigger retries. Without this they sat in the sealed WAL file, invisible to queries until restart. Post-commit (manifest save) failures still surface as a retry but with an empty event list — the events are durable in the still-sealed WAL and recovery replays them on next start; returning them here would risk a double-flush.#2 — Fail-closed corrupt manifest.
recovery.rsno longer silently falls back toManifest::default()on a parse error — for a billing DB, orphaning every committed segment is the worst-case outcome. ReturnsInvalidDataand the process refuses to start. Fresh DBs (no manifest file at all) still start cleanly.#3 — Strict SQL parser. Rewritten end-to-end. Rejects:
SUM(anything but quantity)(was: silently mapped toSUM(quantity))COUNT(col)(was: silently treated as1)OR/HAVING/SELECT */ aliases / unknown tablesDistinguishes
<from<=and>from>=— each adjusts the half-open[from_ms, to_ms)correctly. Range comparisons only ontimestamp_ms/hour_start_ms. WHERE is AND-only.#4 — Full segment pruning.
collect_raw_events(and the rollup-then-tail path) now uses every relevantSegmentMetafield before opening a segment file:bucket = blake3(account_id) % bucket_countwhen an account is specifiedmin_account_id/max_account_idfor further account-range narrowingproduct_ids/meter_ids/model_idsintersected against query filters#5 — Half-open
[from, to)semantics.matches_planexcludes events at exactlyto_ms; segment overlap iss.min < to && s.max >= from; rollup_upper is the exclusive boundarymin(to, watermark). Adjacent monthly queries no longer double-count boundary events.#6 — Shutdown drain.
main.rsdrains the memtable + rotates the WAL onctrl+cbefore letting the background workers exit. Combined with the age-based force-flush from the rollup worker (already shipped), the memtable can no longer hold events indefinitely.Tests
tests/p1_fixes.rs— 15 new tests, each shaped to verify the fix:recovery_refuses_to_start_on_corrupt_manifestrecovery_starts_fresh_when_no_manifest_existssql_tests::rejects_or_in_wheresql_tests::rejects_sum_on_non_quantitysql_tests::rejects_count_with_columnsql_tests::rejects_select_starsql_tests::rejects_unknown_tablesql_tests::rejects_aliassql_tests::distinguishes_lt_from_ltesql_tests::distinguishes_gt_from_gtesql_tests::accepts_canonical_formadjacent_queries_dont_double_count_boundary_eventaccount_query_results_are_correct_with_bucket_pruningproduct_filter_prunes_segments_without_that_productbuild_segment_meta_unchanged_by_failure_refactorTest plan
cargo build --all-targetsclean with-D warningscargo test --all-targets— 59 tests pass (was 44; +15)Still on the backlog
DurabilityMode::Balanced(group commit)After this lands the codebase has no known correctness gaps from either review. Remaining work is purely performance + testing infrastructure.
🤖 Generated with Claude Code