This repository was archived by the owner on Sep 8, 2026. It is now read-only.
Repository navigation
Fix core robustness and security issues (#2–#7) - #20
Merged
Merged
Conversation
Every publish now prunes manifest generations beyond the newest file plus two retained fallbacks, drops inactive rollup descriptors from the new generation, and best-effort unlinks .rseg files that no retained generation references. Store::open runs the same sweep, reclaiming segments orphaned by a maintain interrupted between segment creation and manifest publication. Safety: the just-published generation is never a deletion candidate, the fallback window keeps corruption recovery working, and the segment sweep aborts whenever any retained generation cannot be read back, so a file a recovery fallback may still need is never removed. All cleanup is non-fatal because the preceding publish is already durable. Fixes #2 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCz23P2PXpQ7QAiVtCM5so
A crafted index entry or block header could claim up to u32::MAX points per block, and decode_columns reserved Vec capacity from that untrusted count before parsing could fail, letting a small file force multi-GB allocations on open or query. validate_index now rejects entries whose claimed point count cannot fit in the block's stored payload even at the LZ4 format's maximum 255x expansion, and decode_columns rejects counts whose minimum encoded size (eight varint bytes per point plus the fixed first timestamp and value) exceeds the decoded payload before reserving any capacity. Blocks the writer produces always satisfy both bounds because every encoded point occupies at least the minimum bytes and LZ4 cannot compress beyond the ratio the format allows. Fixes #3 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCz23P2PXpQ7QAiVtCM5so
decompress_size_prepended allocated whatever four-byte size the compressed payload claimed before decompressing, so a crafted segment or rollup file of a few hundred bytes could force a ~4 GiB allocation from Store::open, check_integrity, and the check-store and backup commands. Both readers now call lz4_flex::block::decompress with the segment's own header length after validating it: rollup segments bound it by the LZ4 format's maximum 255x expansion of the bytes actually stored, and point blocks pin it between the minimum and maximum encoded size of the block's validated point count. The four-byte prefix the writer embeds via compress_prepend_size must equal that header length — the writer derives the header field from the prefix, so legitimate files always agree — and is skipped explicitly before decompression. Fixes #4 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCz23P2PXpQ7QAiVtCM5so
Database::open_read_only opens without write access or creation, takes a shared advisory lock instead of the exclusive one, and simulates torn-tail recovery in memory: the tail is skipped and accounted in Stats exactly as if truncated, but the inspected file is never rewritten or created. Writer APIs on a read-only handle fail with the new structured Error::ReadOnly. Store::open_read_only builds on it for directory stores: no manifest publication, pruning, or orphan sweeping. A rollup whose provenance trails the raw log is reported through IntegrityReport::stale_rollup_files instead of being invalidated on open. backup_to works from a read-only source because flushing was its only source-side mutation, so `ftw backup` can no longer alter or accidentally create the store it copies. The CLI wires inspect to the read-only database open and check-store and backup to the read-only store open; generate and bench-ftwdb keep their write behavior. Fixes #5 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCz23P2PXpQ7QAiVtCM5so
Database creation wrote the header and synced the file's contents but never fsynced the directory that names it, and Store::open_with synced the root before the active log existed, so a power loss after any number of acknowledged durable commits could make a freshly created store or standalone database vanish wholesale. Segment and manifest publication already sync their parent directories; creation now gets the same treatment. A writable open that finds a zero-length file — brand new or left by a crash between creation and header write — writes the header, syncs the file, then syncs its parent directory entry, reusing the directory-sync helper that moved from store.rs into storage.rs so both layers share it. Store::open_with keeps its root sync for the manifests/ and rollups/ entries it creates and additionally syncs a freshly created root's own entry in its parent, in the same contents-then-entry order publication uses. Read-only opens are untouched and still sync nothing. Directory syncing remains open-and-sync_all, with Windows tracked by issue #15. Fixes #6 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCz23P2PXpQ7QAiVtCM5so
write_frame and append already poison the database when their write-or-sync closure fails, but the public flush() propagated a sync_data error while leaving poisoned unset and bytes_since_sync untouched. On Linux a failed fsync can mark the still-unwritten dirty pages clean, so a caller that retried flush() after the error could get Ok(()) — a false durability claim over data that never reached media (fsyncgate). Store::flush, Store::close, and the durable-commit upgrade path in Store::commit/append all inherited the same hole. flush() now sets poisoned before returning the sync error, matching the frame writers, so every later flush/commit/append fails with Error::Poisoned until the database is reopened and recovery re-reads what is actually on disk. The remaining log-file syncs run during open (header creation and tail truncation), where a failure aborts the open and no writer handle survives to poison. Directory-entry syncs keep their existing publish error paths. The regression test swaps the writer's file handle for a pipe, whose fsync fails with EINVAL, and asserts the first flush reports the I/O error while every subsequent flush, append, and commit fails poisoned. Fixes #7 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCz23P2PXpQ7QAiVtCM5so
![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: 504e9ff4f4
ℹ️ 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".
read_block bounded a block's claimed uncompressed length only through its claimed point count, and validate_index admits point counts up to payload_bytes * MAX_LZ4_RATIO / MIN_ENCODED_POINT_BYTES. Combining the two, a crafted block with a ~1 MiB payload could claim a point count that passes the index bound and an uncompressed length near maximum_encoded_bytes(points) — roughly 2.8 GiB — pass every header, checksum, and prefix check, and reach lz4_flex decompress, which sizes the output buffer from that length before failing. The rollup segment reader already rejects such headers with its stored-length ratio check; the raw segment reader did not. read_block now also rejects an uncompressed length larger than the LZ4 stream bytes (the payload minus its four-byte size prefix) times MAX_LZ4_RATIO before any decompression buffer is sized. The format cannot expand one stored byte into more than 255 output bytes, so the writer's own compress_prepend_size output always satisfies the bound and only corrupt or hostile headers are rejected. The regression test rewrites a real block's point count to pass the index bound and its uncompressed length and prefix to sit inside the per-point window while far exceeding the stream's capacity, then asserts the query fails with the new corruption reason. Hardens the fix for #4 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCz23P2PXpQ7QAiVtCM5so
prune_generations retained the newest 1 + RETAINED_FALLBACK_GENERATIONS manifest files by filename generation number alone. Manifest::load, however, falls back arbitrarily far past corrupt candidates, so if more corrupt newer files existed than the window holds — repeated torn publishes, or junk files with manifest names — open would load an older valid generation and then, via remove_unreferenced_files, delete the very file it had just read. The next open would find only corrupt manifests: a recoverable store became unrecoverable. Pruning now takes the loaded generation and never unlinks it, retaining it alongside the filename window and returning it in the retained set, so referenced_rollup_files keeps protecting the rollup segments that generation references. Store::remove_unreferenced_files passes the generation of the manifest it is running on; everything else below the window is still pruned. The store-level regression test plants three corrupt generations above the newest valid one, asserts open falls back and the loaded manifest file survives the prune, and reopens the store to prove it stays recoverable. A manifest unit test covers the retained-set shape directly. Hardens the fix for #2 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCz23P2PXpQ7QAiVtCM5so
This was referenced Jul 21, 2026
frahlg
changed the title
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Roadmap
This closes core hardening gaps in M0, enforces M2's bounded-memory goal, and advances M3's physical rollup cleanup. The read-only tools also support M4 operations work.
Checks
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-targets --all-features(63 library tests plus property and bench targets)Closes #2
Closes #3
Closes #4
Closes #5
Closes #6
Closes #7