From 4ca4ff7cdac5469d4d138dbdeae7681b9742d326 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Sat, 23 May 2026 22:27:06 +0200 Subject: [PATCH 01/26] WIP: begin working on Manifest feature --- cmd/manifest_dump/README.md | 19 +++++++++++++++++ cmd/manifest_dump/main.go | 2 ++ internal/manifest/current.go | 1 + internal/manifest/current_test.go | 1 + internal/manifest/edit.go | 34 +++++++++++++++++++++++++++++++ internal/manifest/edit_test.go | 1 + internal/manifest/reader.go | 1 + internal/manifest/reader_test.go | 0 internal/manifest/version.go | 25 +++++++++++++++++++++++ internal/manifest/version_test.go | 1 + internal/manifest/writer.go | 1 + internal/manifest/writer_test.go | 1 + 12 files changed, 87 insertions(+) create mode 100644 cmd/manifest_dump/README.md create mode 100644 cmd/manifest_dump/main.go create mode 100644 internal/manifest/current.go create mode 100644 internal/manifest/current_test.go create mode 100644 internal/manifest/edit.go create mode 100644 internal/manifest/edit_test.go create mode 100644 internal/manifest/reader.go create mode 100644 internal/manifest/reader_test.go create mode 100644 internal/manifest/version.go create mode 100644 internal/manifest/version_test.go create mode 100644 internal/manifest/writer.go create mode 100644 internal/manifest/writer_test.go diff --git a/cmd/manifest_dump/README.md b/cmd/manifest_dump/README.md new file mode 100644 index 0000000..9792f6f --- /dev/null +++ b/cmd/manifest_dump/README.md @@ -0,0 +1,19 @@ +# manifest_dump + +CLI tool for inspecting BeachDB's MANIFEST files without running the database. + +## Build + +```sh +make build +# or +go build -o bin/manifest_dump ./cmd/manifest_dump +``` + +## Usage + +TODO: Add more details once implemented. + +## Examples + +TODO: Add more details once implemented. diff --git a/cmd/manifest_dump/main.go b/cmd/manifest_dump/main.go new file mode 100644 index 0000000..8aec60e --- /dev/null +++ b/cmd/manifest_dump/main.go @@ -0,0 +1,2 @@ +// Package main provides the manifest_dump CLI tool for inspecting MANIFEST files. +package main diff --git a/internal/manifest/current.go b/internal/manifest/current.go new file mode 100644 index 0000000..88367b0 --- /dev/null +++ b/internal/manifest/current.go @@ -0,0 +1 @@ +package manifest diff --git a/internal/manifest/current_test.go b/internal/manifest/current_test.go new file mode 100644 index 0000000..88367b0 --- /dev/null +++ b/internal/manifest/current_test.go @@ -0,0 +1 @@ +package manifest diff --git a/internal/manifest/edit.go b/internal/manifest/edit.go new file mode 100644 index 0000000..b0d3235 --- /dev/null +++ b/internal/manifest/edit.go @@ -0,0 +1,34 @@ +package manifest + +import "github.com/aalhour/beachdb/internal/keys" + +// FileMetadata represents metadata about an SSTable file. +type FileMetadata struct { + Level int + FileID uint64 + Size uint64 + SmallestKey keys.InternalKey + LargestKey keys.InternalKey +} + +type VersionEdit struct { + AddedFiles []FileMetadata + DeletedFiles []struct { + Level int + FileId uint64 + } + HasNextFileID bool + NextFileID uint64 // Next SSTable file number + HasLastSeqNo bool + LastSeqNo uint64 // DB's seqno + HasLogNo bool + LogNo uint64 // WAL file's log number +} + +func (e *VersionEdit) Encode() []byte { + return nil +} + +func DecodeVersionEdit(data []byte) (*VersionEdit, error) { + return &VersionEdit{}, nil +} diff --git a/internal/manifest/edit_test.go b/internal/manifest/edit_test.go new file mode 100644 index 0000000..88367b0 --- /dev/null +++ b/internal/manifest/edit_test.go @@ -0,0 +1 @@ +package manifest diff --git a/internal/manifest/reader.go b/internal/manifest/reader.go new file mode 100644 index 0000000..88367b0 --- /dev/null +++ b/internal/manifest/reader.go @@ -0,0 +1 @@ +package manifest diff --git a/internal/manifest/reader_test.go b/internal/manifest/reader_test.go new file mode 100644 index 0000000..e69de29 diff --git a/internal/manifest/version.go b/internal/manifest/version.go new file mode 100644 index 0000000..3a6ee3e --- /dev/null +++ b/internal/manifest/version.go @@ -0,0 +1,25 @@ +package manifest + +type Version struct { + files [][]FileMetadata // files[level] = sorted list of files at that level +} + +func NewVersion() *Version { + return &Version{} +} + +func (v *Version) Apply(edit *VersionEdit) *Version { + return &Version{} +} + +func (v *Version) Files(level int) []FileMetadata { + return nil +} + +func (v *Version) AllFiles() []FileMetadata { + return nil +} + +func (v *Version) NumLevels() int { + return 0 +} diff --git a/internal/manifest/version_test.go b/internal/manifest/version_test.go new file mode 100644 index 0000000..88367b0 --- /dev/null +++ b/internal/manifest/version_test.go @@ -0,0 +1 @@ +package manifest diff --git a/internal/manifest/writer.go b/internal/manifest/writer.go new file mode 100644 index 0000000..88367b0 --- /dev/null +++ b/internal/manifest/writer.go @@ -0,0 +1 @@ +package manifest diff --git a/internal/manifest/writer_test.go b/internal/manifest/writer_test.go new file mode 100644 index 0000000..88367b0 --- /dev/null +++ b/internal/manifest/writer_test.go @@ -0,0 +1 @@ +package manifest From 7c1a8eed5ac1cc02c62707818e47d194b20efa60 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Sat, 23 May 2026 22:37:02 +0200 Subject: [PATCH 02/26] WIP: address linter comments --- internal/manifest/doc.go | 5 +++++ internal/manifest/edit.go | 5 +++++ internal/manifest/reader_test.go | 1 + internal/manifest/version.go | 6 ++++++ 4 files changed, 17 insertions(+) create mode 100644 internal/manifest/doc.go diff --git a/internal/manifest/doc.go b/internal/manifest/doc.go new file mode 100644 index 0000000..31156c1 --- /dev/null +++ b/internal/manifest/doc.go @@ -0,0 +1,5 @@ +// Package manifest implements the MANIFEST file format for +// BeachDB's LSM storage engine. +// +// TODO: Explain MANIFEST files and their format. +package manifest diff --git a/internal/manifest/edit.go b/internal/manifest/edit.go index b0d3235..fbfae84 100644 --- a/internal/manifest/edit.go +++ b/internal/manifest/edit.go @@ -11,6 +11,7 @@ type FileMetadata struct { LargestKey keys.InternalKey } +// VersionEdit represents one atomic change to the database's file set. type VersionEdit struct { AddedFiles []FileMetadata DeletedFiles []struct { @@ -25,10 +26,14 @@ type VersionEdit struct { LogNo uint64 // WAL file's log number } +// Encode serializes the batch operations to a byte slice. +// The encoding is deterministic: the same batch always produces the same bytes. func (e *VersionEdit) Encode() []byte { return nil } +// DecodeVersionEdit decodes a byte slice into a VersionEdit. +// Returns an error if the data is malformed, truncated, or contains invalid operations. func DecodeVersionEdit(data []byte) (*VersionEdit, error) { return &VersionEdit{}, nil } diff --git a/internal/manifest/reader_test.go b/internal/manifest/reader_test.go index e69de29..88367b0 100644 --- a/internal/manifest/reader_test.go +++ b/internal/manifest/reader_test.go @@ -0,0 +1 @@ +package manifest diff --git a/internal/manifest/version.go b/internal/manifest/version.go index 3a6ee3e..07f79c1 100644 --- a/internal/manifest/version.go +++ b/internal/manifest/version.go @@ -1,25 +1,31 @@ package manifest +// Version represents an in-memory snapshot of which SSTables exist. type Version struct { files [][]FileMetadata // files[level] = sorted list of files at that level } +// NewVersion returns a new Version. func NewVersion() *Version { return &Version{} } +// Apply applies a single atomic version edit to the current Version. func (v *Version) Apply(edit *VersionEdit) *Version { return &Version{} } +// Files returns the list of files at a given level. func (v *Version) Files(level int) []FileMetadata { return nil } +// AllFiles returns the list of all files in the version. func (v *Version) AllFiles() []FileMetadata { return nil } +// NumLevels returns the number of SSTable levels in the version. func (v *Version) NumLevels() int { return 0 } From f91840343dbb2c22bd080ad006a0b401e13f44d2 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Sun, 24 May 2026 18:41:26 +0200 Subject: [PATCH 03/26] Wite the manifest file format document --- docs/formats/manifest.md | 319 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 305 insertions(+), 14 deletions(-) diff --git a/docs/formats/manifest.md b/docs/formats/manifest.md index c9a884e..912ec1e 100644 --- a/docs/formats/manifest.md +++ b/docs/formats/manifest.md @@ -1,25 +1,316 @@ -# Manifest / versioning (v0.1) +# Manifest Format (v1) -> The manifest is metadata durability. +> The manifest is the official record of which SSTables exist. If a file isn't in the manifest, it doesn't count. + +Introduced in BeachDB [v0.0.5](https://github.com/aalhour/beachdb/releases/tag/v0.0.5). ## Goals -- Crash-safe tracking of live SSTables -- Replayable edits (append-only) -- Deterministic startup reconstruction +- **Atomic state transitions**: each edit applies whole or not at all. No half-applied state on disk. +- **Crash-safe bootstrap**: I always know which manifest file is live, even if a crash happens during rotation. +- **Deterministic replay**: replaying N edits in order always rebuilds the same `Version`. +- **Forward-compatible**: room for compaction stats, snapshot metadata, and other future fields without breaking old readers. +- **Inspectable**: `manifest_dump` decodes the file and prints the reconstructed file set. + +--- + +## File Layout + +A BeachDB data directory contains two manifest-related files: + +``` +data/ +├── CURRENT ← 1-line pointer to the active manifest +├── MANIFEST-000001 ← append-only log of VersionEdits +├── 000007.sst ← SSTables (named by file ID) +├── 000008.sst +└── ... +``` + +- `CURRENT` always contains exactly one line: the filename of the live manifest, followed by `\n`. +- `MANIFEST-NNNNNN` is the active log. File IDs are 6-digit zero-padded so they sort lexicographically. +- v1 never rotates the manifest. One MANIFEST file per database lifetime. + +The indirection through `CURRENT` exists because manifests can be rotated (rewritten compactly, then atomically swapped in). v1 doesn't rotate yet, but the indirection is there from day one. Adding it later would break recovery. + +--- + +## Record Format + +A MANIFEST file is a sequence of records. The framing is the same as the [WAL record format](wal.md#record-format), with one field changed: + +| Field | WAL value | Manifest value | +|-------|-----------|----------------| +| `magic` | `0xBEAC` | `0x4D46` (ASCII `"MF"`) | +| `version` | `0x01` | `0x01` | +| `type` | `0x01` (Full) | `0x01` (Full) | +| `length` | uint32 | uint32 | +| `checksum` | CRC32C of payload | CRC32C of payload | +| `payload` | encoded `Batch` | encoded `VersionEdit` | + +Different magic byte means `wal_dump` cleanly rejects a manifest file and vice versa. + +The header is 12 bytes. The payload is the encoded `VersionEdit` described below. + +--- + +## VersionEdit Encoding + +A `VersionEdit` is one atomic change to the database's metadata: files added, files deleted, counter updates. Each edit is encoded as a sequence of tag-value pairs. + +``` +VersionEdit Encoding (v1) +========================= + +Each field in a VersionEdit is optional. Encode only fields that are set. +For each set field, write: + [tag: 1 byte][field payload] + +Tag values: + 1 = AddFile: [level: uint32][fileID: uint64][size: uint64] + [smallestKey: uint32 length + bytes] + [largestKey: uint32 length + bytes] + 2 = DeleteFile: [level: uint32][fileID: uint64] + 3 = NextFileID: [value: uint64] + 4 = LastSequence: [value: uint64] + 5 = LogNumber: [value: uint64] +``` + +All integers are big-endian. No padding between fields. + +### Byte layout example + +An edit with `NextFileID = 42`, `LastSequence = 100`, and one `AddFile{level=0, fileID=7, size=1024, smallestKey="apple", largestKey="zebra"}`: + +``` +Offset Hex Meaning +------ --- ------- +0 03 tag = NextFileID +1..8 00 00 00 00 00 00 00 2A uint64 BE = 42 +9 04 tag = LastSequence +10..17 00 00 00 00 00 00 00 64 uint64 BE = 100 +18 01 tag = AddFile +19..22 00 00 00 00 level (uint32) = 0 +23..30 00 00 00 00 00 00 00 07 fileID (uint64) = 7 +31..38 00 00 00 00 00 00 04 00 size (uint64) = 1024 +39..42 00 00 00 05 smallestKey length (uint32) = 5 +43..47 61 70 70 6C 65 smallestKey bytes = "apple" +48..51 00 00 00 05 largestKey length (uint32) = 5 +52..56 7A 65 62 72 61 largestKey bytes = "zebra" +``` + +Total body: 57 bytes. Wrapped by the 12-byte record header, the on-disk record is 69 bytes. + +### Deterministic emit order + +`Encode()` must produce the same bytes for the same input on every call. Tags are written in this order: + +1. `NextFileID` (if set) +2. `LastSequence` (if set) +3. `LogNumber` (if set) +4. `DeleteFile` entries, in input order (do not sort) +5. `AddFile` entries, in input order (do not sort) + +Counters first lets a reader see the new watermarks before processing file deltas. Same convention LevelDB uses. + +### Decoder skeleton + +``` +i := 0 +for i < len(data): + tag := data[i]; i++ + switch tag: + case 3: read 8 bytes → NextFileID; HasNextFileID = true + case 4: read 8 bytes → LastSequence; HasLastSequence = true + case 5: read 8 bytes → LogNumber; HasLogNumber = true + case 2: read 4 bytes level, 8 bytes fileID → append DeletedFiles + case 1: read level, fileID, size, then two length-prefixed key blobs → append AddedFiles + default: return ErrUnknownTag + bounds-check every read: short buffer → ErrTruncated +``` + +Unknown tags are a hard error in v1. See [Versioning Strategy](#versioning-strategy). + +--- + +## CURRENT File + +``` +CURRENT Format +============== +"MANIFEST-NNNNNN\n" +``` + +One line. The filename of the live manifest, no directory prefix, terminated with a single `\n`. Nothing else. + +### Atomic update protocol + +To install a new CURRENT, used only during rotation in a future version, use this protocol: + +``` +1. Write "MANIFEST-NNNNNN\n" to "CURRENT.tmp" in the same directory +2. fsync("CURRENT.tmp") +3. rename("CURRENT.tmp", "CURRENT") ← atomic on POSIX +4. fsync(parent directory) ← so the rename is durable +``` + +A crash at any step leaves either the old CURRENT intact or the new CURRENT fully written. Never a partial CURRENT. + +--- + +## Recovery Semantics + +### On Startup + +1. Read `CURRENT`. If missing → fresh database, skip to step 5. +2. Read the manifest file named in `CURRENT`. If missing → corruption, fail loudly. +3. Replay records sequentially. For each `VersionEdit`: + - Apply to in-memory `Version`. + - Update counters (`NextFileID`, `LastSequence`, `LogNumber`) if present. +4. Open SSTable readers for every file in the final `Version`. If a referenced file doesn't exist on disk → corruption, fail loudly. +5. Replay the WAL on top of the recovered state. +6. If fresh database: create `MANIFEST-000001`, write one initial `VersionEdit` carrying zero counters, fsync, then write `CURRENT` atomically. + +### Handling Truncation + +A truncated last record means the process crashed mid-write to the manifest: + +- **Truncated header** (< 12 bytes): ignore, treat as EOF, truncate the file back to the last valid offset. +- **Truncated payload** (header valid, payload incomplete): ignore, treat as EOF, truncate. -## Model (draft) +The incomplete edit was never `fsync`'d, so it never took effect from the database's perspective. Discarding it is correct. -- Manifest is a log of version edits: - - AddFile(level, file_id, smallest_key, largest_key, size_bytes, ...) - - DeleteFile(level, file_id) +### Handling Corruption -## Startup +- **Bad magic on a record**: stop. The manifest is not what it claims to be. +- **Checksum mismatch on a complete record**: stop. Fail loudly. Do not continue replay. +- **Unknown tag in a payload**: stop. v1 readers do not skip unknown tags. +- **Missing SSTable referenced by manifest**: stop. This is different from WAL truncation. A truncated WAL tail means "crash after write, before sync" and is benign. A missing SSTable the manifest promises exists means data was deleted outside the manifest contract. That's a bug or corruption. -- Load the latest manifest state -- Open current file set -- Replay WAL to reach latest state (per scope) +### Orphan SSTables + +An orphan is an `.sst` file on disk that the manifest doesn't reference. Most common cause: a crash between writing the SSTable and appending the matching `AddFile` edit. + +After manifest replay completes, before WAL replay: + +1. Collect all `fileID`s in the recovered `Version`. +2. List all `.sst` files in the data directory. +3. For each `.sst` file not in the manifest: delete it. Log every deletion. If a deletion fails, log the failure and continue. Orphan cleanup is best-effort and must not block startup. + +The strict invariant is that the database does not *use* an orphan: only files referenced by the manifest are opened as SSTables. Deletion is the secondary concern, and a failed deletion leaves the file on disk to be cleaned up next time. + +--- + +## Durability Contract + +The manifest and the SSTables it references must be durable in the right order. The rule: + +``` +1. Write the SSTable file +2. fsync the SSTable file +3. fsync the data directory (so the SSTable filename is durable) +4. Append the AddFile edit to the manifest +5. fsync the manifest file +``` + +Never reverse SSTable sync and manifest sync. If the manifest is synced first, it promises a file that may not be durable. If the order above is followed and a crash happens between step 3 and step 5, the SSTable exists on disk but the manifest doesn't know about it. It's an orphan, and orphan handling takes care of it. + +Unlike the WAL (where `SyncOnWrite` is configurable), the manifest always syncs after each append. Metadata durability is not optional. + +--- + +## Design Decisions + +### Why a log of edits, not a snapshot? + +I considered writing the full file set to disk on every change: `current state → JSON or TLV → atomic rename`. Simpler in some ways, but it has two problems: + +1. **Every change rewrites everything.** A compaction that adds one file and removes one file shouldn't have to re-serialize the entire database state. With a log, the cost is proportional to the change. +2. **No audit trail.** A log lets `manifest_dump` show *how* the database arrived at its current state, not just where it is. + +LevelDB, RocksDB, and Pebble all use a log of edits. Badger writes change sets to a manifest that compacts in place. TidesDB serializes the whole state. I went with the log model for the same reasons LevelDB did. + +### Why a CURRENT file? + +A future rotation step (write a fresh MANIFEST containing the compacted state, then atomically swap) needs a single atomic operation that flips from old to new. Renaming a small text file is that operation. Without CURRENT, the only way to swap manifests would be to rename the manifest file itself. But then readers that opened it just before the rename are stuck holding a stale handle, and recovery has to guess which manifest is current. + +v1 doesn't rotate. But CURRENT is in the format from day one so I don't have to retrofit it later. + +### Why reuse the WAL record framing? + +The framing problem is identical for the WAL and the manifest: variable-length payloads with magic, length, checksum, and version. Solving it twice would mean two readers, two writers, two sets of truncation tests. The only thing that differs is the magic byte and the payload contents. Both are parameters. + +LevelDB and RocksDB use the same `log::Writer` / `log::Reader` for both files. Pebble uses the same package, with two writer variants (one optimized for concurrent WAL appends). The on-disk format is identical across all three engines for both files. I followed the same path. + +### Why TLV instead of a fixed struct? + +The fields in a `VersionEdit` are sparse: most edits set only a few of them. A fixed struct would write zero bytes for every unused field. TLV writes only what's present, and old readers can detect unknown tags cleanly. + +The trade-off is that the decoder is a `switch` on tag bytes instead of a straight read. That cost is paid once at recovery, not on every read. + +### Why fsync on every append? + +Unlike the WAL (where group commit is a future optimization), the manifest always syncs immediately. The reasoning: a manifest entry is metadata. If the SSTable it references exists on disk but the manifest entry hasn't been synced, a crash leaves the file invisible to the database. The next startup either ignores the orphan (safe) or wastes work recreating equivalent state (wasteful but correct). + +Either way, the manifest is small and writes are infrequent. Sync latency is not a hot-path concern. + +### Why hard-error on missing SSTables but tolerate WAL truncation? + +WAL truncation is a benign crash signature: the write was in flight, never acknowledged, and discarding it is correct. The user never saw success. + +A missing SSTable the manifest promises is a different signature entirely. The manifest entry was synced, which means the SSTable's write and sync completed before the manifest was synced (per the ordering rule above). For the file to be gone, something deleted it outside the database's control. That's either a bug in BeachDB or someone touching the data directory. Either way, silent recovery would hide the problem. Failing loudly is the right answer. + +### Why no rotation in v1? + +A single growing manifest is simpler. For a v1 database that hasn't even shipped a server yet, rotation is premature optimization. The infrastructure (CURRENT file, atomic install) is in place for when rotation matters. That likely starts when manifest replay time starts dominating startup. + +--- + +## Versioning Strategy + +v1 ships the minimum tags needed for one database without a table (just one global table): `AddFile`, `DeleteFile`, `NextFileID`, `LastSequence`, `LogNumber`. Unknown tags are a hard error, and there's no skip-unknown machinery. Forward compatibility lives in the format version byte. When new tags need to exist, I will bump the version byte and then introduce a newer format. + +Tags I expect to need later include compaction pointers, Raft snapshot metadata, table catalogs, and per-table file tracking. They are deliberately not reserved. Each ships in v2, v3, v4 when the feature itself ships, and the version byte bumps with it. + +The principle is the same one I follow in the WAL: reserved slots either don't fit the eventual feature or carry dead bytes forever. The version byte is the escape hatch. v1 stays small. + +--- ## Tooling -- `tools/manifest_dump`: print edits and reconstructed file set. +- **`cmd/manifest_dump`**: decode and print the manifest. Shows each `VersionEdit` in order, then the reconstructed `Version` at the end. + +Example output: +``` +$ manifest_dump data/ +CURRENT → MANIFEST-000001 +Edit 0: NextFileID=1, LastSequence=0, LogNumber=1 +Edit 1: AddFile L0/7 size=1024 [apple..zebra]; NextFileID=8; LastSequence=42 +Edit 2: AddFile L0/8 size=2048 [alpha..yankee]; NextFileID=9; LastSequence=87 +End of MANIFEST (3 edits) + +Reconstructed Version: + L0: [7, 8] +``` + +On corruption: +``` +$ manifest_dump data/ +CURRENT → MANIFEST-000001 +Edit 0: NextFileID=1, LastSequence=0 +Edit 1: checksum mismatch (expected 0xABCD1234, got 0xDEADBEEF) +Stopped at edit 1 +``` + +--- + +## References + +- [LevelDB `VersionEdit` header](https://github.com/google/leveldb/blob/main/db/version_edit.h) +- [LevelDB `VersionEdit::EncodeTo` / `DecodeFrom`](https://github.com/google/leveldb/blob/main/db/version_edit.cc): TLV encoding, hard errors on unknown tags +- [LevelDB `VersionSet` (manifest read/write logic)](https://github.com/google/leveldb/blob/main/db/version_set.cc): reuses `log::Writer` / `log::Reader` from the WAL framing layer +- [RocksDB MANIFEST wiki](https://github.com/facebook/rocksdb/wiki/MANIFEST) +- [RocksDB `VersionEdit::DecodeFrom`](https://github.com/facebook/rocksdb/blob/main/db/version_edit.cc): same TLV approach, also hard errors on unknown tags +- [Pebble `internal/manifest/version_edit.go`](https://github.com/cockroachdb/pebble/blob/master/internal/manifest/version_edit.go): Go port of the same encoding +- [Pebble `record` package](https://github.com/cockroachdb/pebble/tree/master/record): shared framing used by both WAL and manifest +- [WAL Record Format](wal.md): manifest reuses the framing layer with a different magic byte From 3e77c37ad5ec191899b25da25b0297ce8bace925 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Sun, 24 May 2026 18:41:55 +0200 Subject: [PATCH 04/26] Clarify docs/formats about future compatibility --- docs/formats/batch.md | 6 ++++++ docs/formats/sstable.md | 2 ++ docs/formats/wal.md | 21 +++++++++++++++++++++ 3 files changed, 29 insertions(+) diff --git a/docs/formats/batch.md b/docs/formats/batch.md index 17ebfb0..f1253be 100644 --- a/docs/formats/batch.md +++ b/docs/formats/batch.md @@ -2,6 +2,8 @@ > The Batch is BeachDB's unit of atomicity. It's the payload that goes into the WAL, and later becomes a Raft log entry. +Introduced in BeachDB [v0.0.1](https://github.com/aalhour/beachdb/releases/tag/v0.0.1). + ## Purpose A `Batch` is a sequence of Put and Delete operations that are applied atomically. The encoding serializes this in-memory structure into a flat byte array that can be: @@ -84,6 +86,10 @@ Delete operations have no value fields — they only need to identify the key be | Put | `0x01` | Store a key-value pair | | Delete | `0x02` | Remove a key (tombstone) | +The `op_type` byte has 256 possible values. v1 uses two. The other 254 are unallocated — v1 readers reject any unknown value as a hard error, so silent misinterpretation isn't possible. + +New op variants: table-aware writes, merge operators, or range deletes will belong in a v2 batch format, signaled by the version byte in the header. v1 won't be retrofitted with new op types. + --- ## Example diff --git a/docs/formats/sstable.md b/docs/formats/sstable.md index 234bc54..9f7ef28 100644 --- a/docs/formats/sstable.md +++ b/docs/formats/sstable.md @@ -4,6 +4,8 @@ BeachDB does **not** have user-facing "tables" yet. In this document, "SSTable" means **Sorted String Table** in the LevelDB/RocksDB sense: a sorted key-value file for the storage engine itself, not an HBase-style table abstraction. +Introduced in BeachDB [v0.0.3](https://github.com/aalhour/beachdb/releases/tag/v0.0.3). + ## Goals - **Immutable sorted file**: Written once, read many times. diff --git a/docs/formats/wal.md b/docs/formats/wal.md index e00a49c..47105d8 100644 --- a/docs/formats/wal.md +++ b/docs/formats/wal.md @@ -2,6 +2,8 @@ > The Write-Ahead Log is BeachDB's durability spine. Every committed batch lands here before it's acknowledged. +Introduced in BeachDB [v0.0.1](https://github.com/aalhour/beachdb/releases/tag/v0.0.1). + ## Goals - **Deterministic recovery**: Replay the same WAL twice, get the same state. @@ -172,6 +174,25 @@ I'm reserving the record type field for future fragmentation support. If a batch For v1, every record is `Full`. But having the field costs 1 byte and saves a format version bump later. +### Why the payload is opaque to the framing layer + +The header carries length and checksum. Nothing else about the payload. What's inside, which is a `Batch` in v1, is the payload's problem. + +This is why the manifest log can reuse the same framing with a different magic byte: the framing layer doesn't know or care that a `VersionEdit` is not a `Batch`. Same property holds when batches eventually become Raft log entries, and the WAL will still just store opaque bytes. + +The rule I follow: framing concerns live in the header (length, checksum, magic, fragmentation). Semantic concerns live in the payload, behind the payload's own version byte. The framing layer never peeks inside. + +### Why v1 doesn't reserve space for future features + +The header has a version byte and nothing else held aside for "later." No flags field, no payload-type discriminator, no padding slots. + +I considered reserving room for table IDs, encryption flags, and a few other things I expect to want eventually. I didn't, for two reasons: + +1. **Reserved bytes age badly.** Half the time the future feature doesn't fit the slot. The other half, every record on disk carries dead bytes forever for a feature that never shipped. +2. **The version byte is the escape hatch.** When a new format version needs to exist, I will just bump the version flag (v1 -> v2) and write the fields that actually need to exist. v1 readers reject the new version cleanly instead of silently misreading. + +v1 ships small. v2 ships when there's a real reason. + --- ## Tooling From d28606333f655aafa50d14b4eead8a9ce89501f2 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Sun, 24 May 2026 22:49:08 +0200 Subject: [PATCH 05/26] Add manifest record wrapper and clean WIP scaffolding lint --- cmd/manifest_dump/main.go | 4 ++++ internal/manifest/edit.go | 4 ++-- internal/manifest/record.go | 41 ++++++++++++++++++++++++++++++++++++ internal/manifest/version.go | 6 +++--- 4 files changed, 50 insertions(+), 5 deletions(-) create mode 100644 internal/manifest/record.go diff --git a/cmd/manifest_dump/main.go b/cmd/manifest_dump/main.go index 8aec60e..a041e47 100644 --- a/cmd/manifest_dump/main.go +++ b/cmd/manifest_dump/main.go @@ -1,2 +1,6 @@ // Package main provides the manifest_dump CLI tool for inspecting MANIFEST files. package main + +// main is the entry point. The manifest_dump tool is not yet implemented. +func main() { +} diff --git a/internal/manifest/edit.go b/internal/manifest/edit.go index fbfae84..d45bdfc 100644 --- a/internal/manifest/edit.go +++ b/internal/manifest/edit.go @@ -16,7 +16,7 @@ type VersionEdit struct { AddedFiles []FileMetadata DeletedFiles []struct { Level int - FileId uint64 + FileID uint64 } HasNextFileID bool NextFileID uint64 // Next SSTable file number @@ -34,6 +34,6 @@ func (e *VersionEdit) Encode() []byte { // DecodeVersionEdit decodes a byte slice into a VersionEdit. // Returns an error if the data is malformed, truncated, or contains invalid operations. -func DecodeVersionEdit(data []byte) (*VersionEdit, error) { +func DecodeVersionEdit(_ []byte) (*VersionEdit, error) { return &VersionEdit{}, nil } diff --git a/internal/manifest/record.go b/internal/manifest/record.go new file mode 100644 index 0000000..533a2f0 --- /dev/null +++ b/internal/manifest/record.go @@ -0,0 +1,41 @@ +package manifest + +import ( + "github.com/aalhour/beachdb/internal/record" +) + +// manifestMagic is the 8-byte ASCII magic identifying BeachDB MANIFEST records. +const manifestMagic string = "BEACHMAN" + +// manifestRecordFormat is the manifest-specific record framing format. +var manifestRecordFormat = mustNewManifestFormat() + +// mustNewManifestFormat builds the manifest record format and panics on misconfiguration. +// The magic and size are compile-time constants so this can never fail in practice. +func mustNewManifestFormat() *record.Format { + f, err := record.NewFormat(manifestMagic, record.DefaultMaxPayloadSize) + if err != nil { + panic(err) + } + return f +} + +// EncodeRecord encodes a payload into a manifest record. +// Returns ErrRecordTooLarge when payload exceeds the format's MaxPayloadSize. +func EncodeRecord(payload []byte) ([]byte, error) { + return manifestRecordFormat.Encode(payload) +} + +// DecodeRecordHeader verifies a manifest record header and returns the payload length and checksum. +func DecodeRecordHeader(header []byte) (payloadLen uint32, crc uint32, err error) { + hdr, err := manifestRecordFormat.DecodeHeader(header) + if err != nil { + return 0, 0, err + } + return hdr.Length, hdr.Checksum, nil +} + +// ValidateRecord verifies that the payload's checksum matches the expected checksum. +func ValidateRecord(payload []byte, expectedChecksum uint32) error { + return record.ValidatePayload(payload, expectedChecksum) +} diff --git a/internal/manifest/version.go b/internal/manifest/version.go index 07f79c1..1c998df 100644 --- a/internal/manifest/version.go +++ b/internal/manifest/version.go @@ -2,7 +2,7 @@ package manifest // Version represents an in-memory snapshot of which SSTables exist. type Version struct { - files [][]FileMetadata // files[level] = sorted list of files at that level + _ [][]FileMetadata // files[level] = sorted list of files at that level (TODO: scaffolding for upcoming work) } // NewVersion returns a new Version. @@ -11,12 +11,12 @@ func NewVersion() *Version { } // Apply applies a single atomic version edit to the current Version. -func (v *Version) Apply(edit *VersionEdit) *Version { +func (v *Version) Apply(_ *VersionEdit) *Version { return &Version{} } // Files returns the list of files at a given level. -func (v *Version) Files(level int) []FileMetadata { +func (v *Version) Files(_ int) []FileMetadata { return nil } From 571bfe2b9448db8b8dfe6be07a1a4d0a10fe81c6 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 25 May 2026 00:41:47 +0200 Subject: [PATCH 06/26] implement manifest's VersionEdit --- README.md | 2 +- docs/formats/manifest.md | 2 +- internal/manifest/edit.go | 261 +++++++++++++++++++++++++++++++++--- internal/manifest/errors.go | 8 ++ 4 files changed, 252 insertions(+), 21 deletions(-) create mode 100644 internal/manifest/errors.go diff --git a/README.md b/README.md index cfb41c7..d1c49ec 100644 --- a/README.md +++ b/README.md @@ -32,8 +32,8 @@ BeachDB is my attempt to re-learn the fundamentals by building them from scratch - [x] **Reference-model randomized tests** (model vs implementation) - [x] **SSTables v1**: immutable sorted files + `sst_dump`, see: [sstables blog post](https://aalhour.com/posts/beachdb-sstables-v1/) - [x] **Crash-loop harness**: kill mid-write, reopen, validate invariants, see: [crash-testing, part 1](https://aalhour.com/posts/beachdb-crash-testing-part1/) blog post -- [ ] **Merge iterators** (memtable + SSTs) + **snapshot reads** (seqno-based) - [ ] **Manifest/versioning** + `manifest_dump` (startup reconstruction) +- [ ] **Merge iterators** (memtable + SSTs) + **snapshot reads** (seqno-based) - [ ] **Read path acceleration**: block index + bloom filters + benchmark evidence - [ ] **Compaction v1**: one strategy, minimal knobs + amplification measurements - [ ] **Adversarial testing**: fault injection + fuzzing (WAL/SST decode paths) diff --git a/docs/formats/manifest.md b/docs/formats/manifest.md index 912ec1e..dd39b50 100644 --- a/docs/formats/manifest.md +++ b/docs/formats/manifest.md @@ -41,7 +41,7 @@ A MANIFEST file is a sequence of records. The framing is the same as the [WAL re | Field | WAL value | Manifest value | |-------|-----------|----------------| -| `magic` | `0xBEAC` | `0x4D46` (ASCII `"MF"`) | +| `magic` | 8-byte ASCII `BEACHWAL` | 8-byte ASCII `BEACHMAN` | | `version` | `0x01` | `0x01` | | `type` | `0x01` (Full) | `0x01` (Full) | | `length` | uint32 | uint32 | diff --git a/internal/manifest/edit.go b/internal/manifest/edit.go index d45bdfc..17d8314 100644 --- a/internal/manifest/edit.go +++ b/internal/manifest/edit.go @@ -1,39 +1,262 @@ package manifest -import "github.com/aalhour/beachdb/internal/keys" +import ( + "github.com/aalhour/beachdb/internal/keys" + "github.com/aalhour/beachdb/internal/record" + "github.com/aalhour/beachdb/internal/util/coding" +) -// FileMetadata represents metadata about an SSTable file. +// VersionEdit field tags for the manifest TLV encoding (v1). +// +// v1 hard-errors on unknown tags. No reserved slots: future fields +// (compaction pointers, snapshots, table catalogs, per-table tracking) +// will ship with a manifest format version bump, not a v1 tag insertion. +// See docs/formats/manifest.md for the full wire format and rationale. +const ( + tagAddFile byte = 0x01 // [level u32][fileID u64][size u64][smallestKey uint32 len+bytes][largestKey uint32 len+bytes] + tagDeleteFile byte = 0x02 // [level u32][fileID u64] + tagNextFileID byte = 0x03 // [value u64] + tagLastSequence byte = 0x04 // [value u64] + tagLogNumber byte = 0x05 // [value u64] +) + +// FileMetadata represents metadata about an SSTable file referenced by the manifest. type FileMetadata struct { - Level int - FileID uint64 - Size uint64 + // Level is the LSM level the file lives at. + Level uint32 + + // FileID is the unique identifier for the SSTable file (matches the on-disk filename). + FileID uint64 + + // Size is the size of the SSTable file in bytes. + Size uint64 + + // SmallestKey is the smallest internal key contained in the file (inclusive). SmallestKey keys.InternalKey - LargestKey keys.InternalKey + + // LargestKey is the largest internal key contained in the file (inclusive). + LargestKey keys.InternalKey +} + +// DeletedFile identifies an SSTable file that is being removed from the file set. +type DeletedFile struct { + // Level is the LSM level the file lived at before deletion. + Level uint32 + + // FileID is the unique identifier of the SSTable file being deleted. + FileID uint64 } // VersionEdit represents one atomic change to the database's file set. +// Each field is optional: only set fields are encoded on disk, and a decoder +// rebuilds the Version by applying a sequence of edits in order. type VersionEdit struct { - AddedFiles []FileMetadata - DeletedFiles []struct { - Level int - FileID uint64 - } + // AddedFiles are SSTables that this edit introduces into the file set. + AddedFiles []FileMetadata + + // DeletedFiles are SSTables that this edit removes from the file set. + DeletedFiles []DeletedFile + + // HasNextFileID indicates whether NextFileID carries a meaningful value + // in this edit. Required because uint64 has no sentinel for "unset". HasNextFileID bool - NextFileID uint64 // Next SSTable file number - HasLastSeqNo bool - LastSeqNo uint64 // DB's seqno - HasLogNo bool - LogNo uint64 // WAL file's log number + + // NextFileID is the next SSTable file number the database will allocate. + NextFileID uint64 + + // HasLastSeqNo indicates whether LastSeqNo carries a meaningful value in this edit. + HasLastSeqNo bool + + // LastSeqNo is the highest sequence number written by the database so far. + LastSeqNo uint64 + + // HasLogNo indicates whether LogNo carries a meaningful value in this edit. + HasLogNo bool + + // LogNo is the WAL file's log number that this edit is associated with. + LogNo uint64 } // Encode serializes the batch operations to a byte slice. // The encoding is deterministic: the same batch always produces the same bytes. func (e *VersionEdit) Encode() []byte { - return nil + buf := make([]byte, 0, 10) + + offset := 0 + + // Next file ID tag + if e.HasNextFileID { + buf[offset] = tagNextFileID + offset++ + coding.PutUint64(buf[offset:], e.NextFileID) + offset += 8 + } + + // Last sequence number tag + if e.HasLastSeqNo { + buf[offset] = tagLastSequence + offset++ + coding.PutUint64(buf[offset:], e.LastSeqNo) + offset += 8 + } + + // Log number tag + if e.HasLogNo { + buf[offset] = tagLogNumber + offset++ + coding.PutUint64(buf[offset:], e.LogNo) + offset += 8 + } + + // Deleted files tags + // Frame: [tag byte][level u32][fileID u64] + for _, deletedFile := range e.DeletedFiles { + buf[offset] = tagDeleteFile + offset++ + coding.PutUint32(buf[offset:], deletedFile.Level) + offset += 4 + coding.PutUint64(buf[offset:], deletedFile.FileID) + offset += 8 + } + + // Added files tags + // Frame: [tag byte][level u32][fileID u64][size u64][smallestKey uint32 len+bytes][largestKey uint32 len+bytes] + for _, addedFile := range e.AddedFiles { + buf[offset] = tagAddFile + offset++ + coding.PutUint32(buf[offset:], addedFile.Level) + offset += 4 + coding.PutUint64(buf[offset:], addedFile.FileID) + offset += 8 + coding.PutUint64(buf[offset:], addedFile.Size) + offset += 8 + + // Encode the smallest key + get it's length + smallestKeyBytes := addedFile.SmallestKey.Encode() + offset = writeLengthPrefixedBytes(buf, offset, smallestKeyBytes) + + // Encode the largest key + get it's length + largestKeyBytes := addedFile.LargestKey.Encode() + offset = writeLengthPrefixedBytes(buf, offset, largestKeyBytes) + } + + return buf } // DecodeVersionEdit decodes a byte slice into a VersionEdit. // Returns an error if the data is malformed, truncated, or contains invalid operations. -func DecodeVersionEdit(_ []byte) (*VersionEdit, error) { - return &VersionEdit{}, nil +func DecodeVersionEdit(data []byte) (*VersionEdit, error) { + r := coding.NewByteReader(data) + + edit := &VersionEdit{} + + for r.Remaining() > 0 { + tag, err := r.ReadByte() + if err != nil { + return nil, record.ErrTruncated + } + switch tag { + case tagNextFileID: + nextFileID, err := r.ReadUint64() + if err != nil { + return &VersionEdit{}, record.ErrCorruptRecord + } + edit.NextFileID = nextFileID + edit.HasNextFileID = true + case tagLastSequence: + lastSeqNo, err := r.ReadUint64() + if err != nil { + return &VersionEdit{}, record.ErrCorruptRecord + } + edit.LastSeqNo = lastSeqNo + edit.HasLastSeqNo = true + case tagLogNumber: + logNo, err := r.ReadUint64() + if err != nil { + return &VersionEdit{}, record.ErrCorruptRecord + } + edit.LogNo = logNo + edit.HasLogNo = true + case tagDeleteFile: + level, err := r.ReadUint32() + if err != nil { + return &VersionEdit{}, record.ErrCorruptRecord + } + fileID, err := r.ReadUint64() + if err != nil { + return &VersionEdit{}, record.ErrCorruptRecord + } + edit.DeletedFiles = append(edit.DeletedFiles, DeletedFile{ + Level: level, + FileID: fileID, + }) + case tagAddFile: + // read level + fileID + size + two length-prefixed keys → append + level, err := r.ReadUint32() + if err != nil { + return &VersionEdit{}, record.ErrCorruptRecord + } + fileID, err := r.ReadUint64() + if err != nil { + return &VersionEdit{}, record.ErrCorruptRecord + } + fileSize, err := r.ReadUint64() + if err != nil { + return &VersionEdit{}, record.ErrCorruptRecord + } + smallestKeyBytes, err := readLengthPrefixedBytes(r) + if err != nil { + return &VersionEdit{}, record.ErrCorruptRecord + } + largestKeyBytes, err := readLengthPrefixedBytes(r) + if err != nil { + return &VersionEdit{}, record.ErrCorruptRecord + } + smallestKey, err := keys.DecodeInternalKey(smallestKeyBytes) + if err != nil { + return &VersionEdit{}, err + } + largestKey, err := keys.DecodeInternalKey(largestKeyBytes) + if err != nil { + return &VersionEdit{}, err + } + edit.AddedFiles = append(edit.AddedFiles, FileMetadata{ + Level: level, + FileID: fileID, + Size: fileSize, + SmallestKey: smallestKey, + LargestKey: largestKey, + }) + default: + return &VersionEdit{}, ErrUnknownTag + } + } + return edit, nil +} + +// writeLengthPrefixedBytes writes b as a length-prefixed byte slice into buf +// starting at offset. Layout: [uint32 length big-endian][b...]. Returns the +// new offset positioned immediately after the written bytes. +func writeLengthPrefixedBytes(buf []byte, offset int, b []byte) int { + coding.PutUint32(buf[offset:], uint32(len(b))) + offset += 4 + copy(buf[offset:], b) + return offset + len(b) +} + +// readLengthPrefixedBytes reads a length-prefixed byte slice from r and returns +// an independent copy of the payload. Returns ErrTruncated if either the length +// prefix or the payload bytes are unavailable. +func readLengthPrefixedBytes(r *coding.ByteReader) ([]byte, error) { + n, err := r.ReadUint32() + if err != nil { + return nil, record.ErrTruncated + } + b, err := r.ReadBytes(int(n)) + if err != nil { + return nil, record.ErrTruncated + } + out := make([]byte, n) + copy(out, b) + return out, nil } diff --git a/internal/manifest/errors.go b/internal/manifest/errors.go new file mode 100644 index 0000000..36f886a --- /dev/null +++ b/internal/manifest/errors.go @@ -0,0 +1,8 @@ +package manifest + +import "errors" + +var ( + // ErrUnknownTag denotes that a tag in the version edit is not supported. + ErrUnknownTag = errors.New("beachdb/manifest: unknown version edit tag") +) From 2f9682679dc071acb0255c112c6a15175339ed11 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 25 May 2026 01:06:04 +0200 Subject: [PATCH 07/26] finish VersionEdit with tests --- Makefile | 29 ++- internal/manifest/edit.go | 374 ++++++++++++++++++++------------- internal/manifest/edit_test.go | 353 +++++++++++++++++++++++++++++++ internal/manifest/errors.go | 11 + 4 files changed, 616 insertions(+), 151 deletions(-) diff --git a/Makefile b/Makefile index 4293874..a575daa 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,19 @@ -.PHONY: all build test coverage lint fmt-check fmt clean check examples crash-check help +.PHONY: all build test coverage lint fmt-check fmt clean check examples crash-check bench fuzz help CYCLES ?= 100 +# Bench knobs. Override on the CLI: `make bench PKG=./internal/wal BENCH=BenchmarkX BENCHTIME=3s` +PKG ?= ./... +BENCH ?= . +BENCHTIME ?= 1s + +# Fuzz knobs. By default `make fuzz` discovers every Fuzz* function across the +# tree and runs each for FUZZTIME. -fuzz only accepts one target at a time +# (and one package), so the target loops rather than passes `./...`. +FUZZPKG ?= ./... +FUZZ ?= ^Fuzz +FUZZTIME ?= 30s + # Default target all: build @@ -48,6 +60,21 @@ fmt: ## check: Runs fmt-check, lint and test check: fmt-check lint test +## bench: Run benchmarks project-wide. Override with `make bench PKG=./internal/wal BENCH=BenchmarkX BENCHTIME=3s` +bench: + go test -run=^$$ -bench=$(BENCH) -benchmem -benchtime=$(BENCHTIME) $(PKG) + +## fuzz: Run every fuzz target FUZZTIME each across FUZZPKG (default ./..., 30s each). `make fuzz FUZZTIME=1m` +fuzz: + @set -eu; \ + for pkg in $$(go list $(FUZZPKG)); do \ + targets=$$(go test -list '$(FUZZ)' $$pkg 2>/dev/null | grep -E '^Fuzz' || true); \ + for t in $$targets; do \ + echo "==> $$pkg :: $$t ($(FUZZTIME))"; \ + go test -run=^$$ -fuzz="^$$t$$" -fuzztime=$(FUZZTIME) $$pkg; \ + done; \ + done + ## crash-check: Run the controller/worker crash harness ($(CYCLES) cycles) with a temporary workspace crash-check: @set -eu; \ diff --git a/internal/manifest/edit.go b/internal/manifest/edit.go index 17d8314..63ac248 100644 --- a/internal/manifest/edit.go +++ b/internal/manifest/edit.go @@ -1,8 +1,9 @@ package manifest import ( + "fmt" + "github.com/aalhour/beachdb/internal/keys" - "github.com/aalhour/beachdb/internal/record" "github.com/aalhour/beachdb/internal/util/coding" ) @@ -12,14 +13,29 @@ import ( // (compaction pointers, snapshots, table catalogs, per-table tracking) // will ship with a manifest format version bump, not a v1 tag insertion. // See docs/formats/manifest.md for the full wire format and rationale. +// Wire format per tag — see docs/formats/manifest.md. +// +// tagAddFile [level u32][fileID u64][size u64][smallestKey len+bytes][largestKey len+bytes] +// tagDeleteFile [level u32][fileID u64] +// tagNextFileID [value u64] +// tagLastSequence [value u64] +// tagLogNumber [value u64] const ( - tagAddFile byte = 0x01 // [level u32][fileID u64][size u64][smallestKey uint32 len+bytes][largestKey uint32 len+bytes] - tagDeleteFile byte = 0x02 // [level u32][fileID u64] - tagNextFileID byte = 0x03 // [value u64] - tagLastSequence byte = 0x04 // [value u64] - tagLogNumber byte = 0x05 // [value u64] + tagAddFile byte = 0x01 + tagDeleteFile byte = 0x02 + tagNextFileID byte = 0x03 + tagLastSequence byte = 0x04 + tagLogNumber byte = 0x05 ) +// maxLengthPrefixedField caps the size of any length-prefixed byte field +// accepted by DecodeVersionEdit (in v1, the smallest and largest internal +// keys in an AddFile record). 64 KiB is generous for an internal key +// (user keys are typically well under 1 KiB plus 9 bytes for seqno + kind) +// and blocks corruption-driven multi-GiB allocations from a bogus uint32 +// length prefix. +const maxLengthPrefixedField uint32 = 1 << 16 + // FileMetadata represents metadata about an SSTable file referenced by the manifest. type FileMetadata struct { // Level is the LSM level the file lives at. @@ -64,197 +80,255 @@ type VersionEdit struct { // NextFileID is the next SSTable file number the database will allocate. NextFileID uint64 - // HasLastSeqNo indicates whether LastSeqNo carries a meaningful value in this edit. - HasLastSeqNo bool + // HasLastSequence indicates whether LastSequence carries a meaningful value in this edit. + HasLastSequence bool + + // LastSequence is the highest sequence number written by the database so far. + LastSequence uint64 - // LastSeqNo is the highest sequence number written by the database so far. - LastSeqNo uint64 + // HasLogNumber indicates whether LogNumber carries a meaningful value in this edit. + HasLogNumber bool - // HasLogNo indicates whether LogNo carries a meaningful value in this edit. - HasLogNo bool + // LogNumber is the WAL file's log number that this edit is associated with. + LogNumber uint64 +} - // LogNo is the WAL file's log number that this edit is associated with. - LogNo uint64 +// addedFileBytes caches the pre-encoded internal-key bytes for one AddedFile +// so the size pass and write pass don't both call InternalKey.Encode(). +type addedFileBytes struct { + smallest []byte + largest []byte } -// Encode serializes the batch operations to a byte slice. -// The encoding is deterministic: the same batch always produces the same bytes. +// Encode serializes the VersionEdit fields to a byte slice in deterministic +// TLV order: counters (NextFileID, LastSequence, LogNumber), then DeletedFiles +// in input order, then AddedFiles in input order. Same convention as LevelDB. +// The same VersionEdit value always encodes to the same byte sequence. func (e *VersionEdit) Encode() []byte { - buf := make([]byte, 0, 10) + addedBytes := make([]addedFileBytes, len(e.AddedFiles)) + for i := range e.AddedFiles { + addedBytes[i].smallest = e.AddedFiles[i].SmallestKey.Encode() + addedBytes[i].largest = e.AddedFiles[i].LargestKey.Encode() + } + buf := make([]byte, e.encodedSize(addedBytes)) offset := 0 - - // Next file ID tag - if e.HasNextFileID { - buf[offset] = tagNextFileID - offset++ - coding.PutUint64(buf[offset:], e.NextFileID) - offset += 8 + offset = writeCounterTag(buf, offset, tagNextFileID, e.HasNextFileID, e.NextFileID) + offset = writeCounterTag(buf, offset, tagLastSequence, e.HasLastSequence, e.LastSequence) + offset = writeCounterTag(buf, offset, tagLogNumber, e.HasLogNumber, e.LogNumber) + for _, d := range e.DeletedFiles { + offset = writeDeletedFile(buf, offset, d) } - - // Last sequence number tag - if e.HasLastSeqNo { - buf[offset] = tagLastSequence - offset++ - coding.PutUint64(buf[offset:], e.LastSeqNo) - offset += 8 + for i, a := range e.AddedFiles { + offset = writeAddedFile(buf, offset, a, addedBytes[i]) } + return buf +} - // Log number tag - if e.HasLogNo { - buf[offset] = tagLogNumber - offset++ - coding.PutUint64(buf[offset:], e.LogNo) - offset += 8 +// encodedSize returns the exact byte size Encode will produce for this edit +// given the pre-encoded internal-key bytes. +func (e *VersionEdit) encodedSize(addedBytes []addedFileBytes) int { + size := 0 + if e.HasNextFileID { + size += 1 + 8 } - - // Deleted files tags - // Frame: [tag byte][level u32][fileID u64] - for _, deletedFile := range e.DeletedFiles { - buf[offset] = tagDeleteFile - offset++ - coding.PutUint32(buf[offset:], deletedFile.Level) - offset += 4 - coding.PutUint64(buf[offset:], deletedFile.FileID) - offset += 8 + if e.HasLastSequence { + size += 1 + 8 } - - // Added files tags - // Frame: [tag byte][level u32][fileID u64][size u64][smallestKey uint32 len+bytes][largestKey uint32 len+bytes] - for _, addedFile := range e.AddedFiles { - buf[offset] = tagAddFile - offset++ - coding.PutUint32(buf[offset:], addedFile.Level) - offset += 4 - coding.PutUint64(buf[offset:], addedFile.FileID) - offset += 8 - coding.PutUint64(buf[offset:], addedFile.Size) - offset += 8 - - // Encode the smallest key + get it's length - smallestKeyBytes := addedFile.SmallestKey.Encode() - offset = writeLengthPrefixedBytes(buf, offset, smallestKeyBytes) - - // Encode the largest key + get it's length - largestKeyBytes := addedFile.LargestKey.Encode() - offset = writeLengthPrefixedBytes(buf, offset, largestKeyBytes) + if e.HasLogNumber { + size += 1 + 8 } - - return buf + size += len(e.DeletedFiles) * (1 + 4 + 8) + for i := range e.AddedFiles { + size += 1 + 4 + 8 + 8 + size += 4 + len(addedBytes[i].smallest) + 4 + len(addedBytes[i].largest) + } + return size } // DecodeVersionEdit decodes a byte slice into a VersionEdit. -// Returns an error if the data is malformed, truncated, or contains invalid operations. +// The input must be the validated payload of a manifest record — the framing +// layer has already checked checksum and length, so any short read here means +// the body itself is malformed (returns ErrTruncatedEdit). An unknown tag +// returns ErrUnknownTag (v1 hard-error policy). func DecodeVersionEdit(data []byte) (*VersionEdit, error) { r := coding.NewByteReader(data) - edit := &VersionEdit{} for r.Remaining() > 0 { tag, err := r.ReadByte() if err != nil { - return nil, record.ErrTruncated + return nil, ErrTruncatedEdit } - switch tag { - case tagNextFileID: - nextFileID, err := r.ReadUint64() - if err != nil { - return &VersionEdit{}, record.ErrCorruptRecord - } - edit.NextFileID = nextFileID - edit.HasNextFileID = true - case tagLastSequence: - lastSeqNo, err := r.ReadUint64() - if err != nil { - return &VersionEdit{}, record.ErrCorruptRecord - } - edit.LastSeqNo = lastSeqNo - edit.HasLastSeqNo = true - case tagLogNumber: - logNo, err := r.ReadUint64() - if err != nil { - return &VersionEdit{}, record.ErrCorruptRecord - } - edit.LogNo = logNo - edit.HasLogNo = true - case tagDeleteFile: - level, err := r.ReadUint32() - if err != nil { - return &VersionEdit{}, record.ErrCorruptRecord - } - fileID, err := r.ReadUint64() - if err != nil { - return &VersionEdit{}, record.ErrCorruptRecord - } - edit.DeletedFiles = append(edit.DeletedFiles, DeletedFile{ - Level: level, - FileID: fileID, - }) - case tagAddFile: - // read level + fileID + size + two length-prefixed keys → append - level, err := r.ReadUint32() - if err != nil { - return &VersionEdit{}, record.ErrCorruptRecord - } - fileID, err := r.ReadUint64() - if err != nil { - return &VersionEdit{}, record.ErrCorruptRecord - } - fileSize, err := r.ReadUint64() - if err != nil { - return &VersionEdit{}, record.ErrCorruptRecord - } - smallestKeyBytes, err := readLengthPrefixedBytes(r) - if err != nil { - return &VersionEdit{}, record.ErrCorruptRecord - } - largestKeyBytes, err := readLengthPrefixedBytes(r) - if err != nil { - return &VersionEdit{}, record.ErrCorruptRecord - } - smallestKey, err := keys.DecodeInternalKey(smallestKeyBytes) - if err != nil { - return &VersionEdit{}, err - } - largestKey, err := keys.DecodeInternalKey(largestKeyBytes) - if err != nil { - return &VersionEdit{}, err - } - edit.AddedFiles = append(edit.AddedFiles, FileMetadata{ - Level: level, - FileID: fileID, - Size: fileSize, - SmallestKey: smallestKey, - LargestKey: largestKey, - }) - default: - return &VersionEdit{}, ErrUnknownTag + if err := decodeTag(r, edit, tag); err != nil { + return nil, err } } + return edit, nil } +// writeCounterTag writes [tag][uint64] for one of the counter tags +// (NextFileID, LastSequence, LogNumber) when has is true. Returns the new offset. +func writeCounterTag(buf []byte, offset int, tag byte, has bool, value uint64) int { + if !has { + return offset + } + buf[offset] = tag + offset++ + coding.PutUint64(buf[offset:], value) + return offset + 8 +} + +// writeDeletedFile writes [tagDeleteFile][level u32][fileID u64] and returns the new offset. +func writeDeletedFile(buf []byte, offset int, d DeletedFile) int { + buf[offset] = tagDeleteFile + offset++ + coding.PutUint32(buf[offset:], d.Level) + offset += 4 + coding.PutUint64(buf[offset:], d.FileID) + return offset + 8 +} + +// writeAddedFile writes the full AddedFile TLV record and returns the new offset. +// Layout: [tagAddFile][level u32][fileID u64][size u64][smallestKey len+bytes][largestKey len+bytes]. +func writeAddedFile(buf []byte, offset int, a FileMetadata, keys addedFileBytes) int { + buf[offset] = tagAddFile + offset++ + coding.PutUint32(buf[offset:], a.Level) + offset += 4 + coding.PutUint64(buf[offset:], a.FileID) + offset += 8 + coding.PutUint64(buf[offset:], a.Size) + offset += 8 + offset = writeLengthPrefixedBytes(buf, offset, keys.smallest) + offset = writeLengthPrefixedBytes(buf, offset, keys.largest) + return offset +} + // writeLengthPrefixedBytes writes b as a length-prefixed byte slice into buf // starting at offset. Layout: [uint32 length big-endian][b...]. Returns the // new offset positioned immediately after the written bytes. func writeLengthPrefixedBytes(buf []byte, offset int, b []byte) int { + //nolint:gosec // G115: byte-slice length is bounded by the surrounding encode size budget. coding.PutUint32(buf[offset:], uint32(len(b))) offset += 4 copy(buf[offset:], b) return offset + len(b) } +// decodeTag reads the payload for one tag and applies it to edit. +func decodeTag(r *coding.ByteReader, edit *VersionEdit, tag byte) error { + switch tag { + case tagNextFileID: + v, err := r.ReadUint64() + if err != nil { + return ErrTruncatedEdit + } + edit.NextFileID = v + edit.HasNextFileID = true + return nil + case tagLastSequence: + v, err := r.ReadUint64() + if err != nil { + return ErrTruncatedEdit + } + edit.LastSequence = v + edit.HasLastSequence = true + return nil + case tagLogNumber: + v, err := r.ReadUint64() + if err != nil { + return ErrTruncatedEdit + } + edit.LogNumber = v + edit.HasLogNumber = true + return nil + case tagDeleteFile: + d, err := decodeDeletedFile(r) + if err != nil { + return err + } + edit.DeletedFiles = append(edit.DeletedFiles, d) + return nil + case tagAddFile: + f, err := decodeAddedFile(r) + if err != nil { + return err + } + edit.AddedFiles = append(edit.AddedFiles, f) + return nil + default: + return ErrUnknownTag + } +} + +// decodeDeletedFile reads [level u32][fileID u64]. +func decodeDeletedFile(r *coding.ByteReader) (DeletedFile, error) { + level, err := r.ReadUint32() + if err != nil { + return DeletedFile{}, ErrTruncatedEdit + } + fileID, err := r.ReadUint64() + if err != nil { + return DeletedFile{}, ErrTruncatedEdit + } + return DeletedFile{Level: level, FileID: fileID}, nil +} + +// decodeAddedFile reads [level u32][fileID u64][size u64][sKey len+bytes][lKey len+bytes]. +func decodeAddedFile(r *coding.ByteReader) (FileMetadata, error) { + level, err := r.ReadUint32() + if err != nil { + return FileMetadata{}, ErrTruncatedEdit + } + fileID, err := r.ReadUint64() + if err != nil { + return FileMetadata{}, ErrTruncatedEdit + } + fileSize, err := r.ReadUint64() + if err != nil { + return FileMetadata{}, ErrTruncatedEdit + } + smallestBytes, err := readLengthPrefixedBytes(r) + if err != nil { + return FileMetadata{}, err + } + largestBytes, err := readLengthPrefixedBytes(r) + if err != nil { + return FileMetadata{}, err + } + smallestKey, err := keys.DecodeInternalKey(smallestBytes) + if err != nil { + return FileMetadata{}, fmt.Errorf("beachdb/manifest: decode AddFile smallest key: %w", err) + } + largestKey, err := keys.DecodeInternalKey(largestBytes) + if err != nil { + return FileMetadata{}, fmt.Errorf("beachdb/manifest: decode AddFile largest key: %w", err) + } + return FileMetadata{ + Level: level, + FileID: fileID, + Size: fileSize, + SmallestKey: smallestKey, + LargestKey: largestKey, + }, nil +} + // readLengthPrefixedBytes reads a length-prefixed byte slice from r and returns -// an independent copy of the payload. Returns ErrTruncated if either the length -// prefix or the payload bytes are unavailable. +// an independent copy of the payload. Returns ErrTruncatedEdit on short read, +// ErrEditFieldTooLarge when the declared length exceeds maxLengthPrefixedField. func readLengthPrefixedBytes(r *coding.ByteReader) ([]byte, error) { n, err := r.ReadUint32() if err != nil { - return nil, record.ErrTruncated + return nil, ErrTruncatedEdit + } + if n > maxLengthPrefixedField { + return nil, ErrEditFieldTooLarge } b, err := r.ReadBytes(int(n)) if err != nil { - return nil, record.ErrTruncated + return nil, ErrTruncatedEdit } out := make([]byte, n) copy(out, b) diff --git a/internal/manifest/edit_test.go b/internal/manifest/edit_test.go index 88367b0..333e4b9 100644 --- a/internal/manifest/edit_test.go +++ b/internal/manifest/edit_test.go @@ -1 +1,354 @@ package manifest + +import ( + "bytes" + "errors" + "reflect" + "testing" + + "github.com/aalhour/beachdb/internal/keys" +) + +// putKey returns an InternalKey of kind Put with the given user key and seqno. +func putKey(userKey string, seqno uint64) keys.InternalKey { + return keys.InternalKey{ + UserKey: []byte(userKey), + Seqno: seqno, + Kind: keys.InternalKeyKindPut, + } +} + +// fileMeta is a convenience constructor for FileMetadata fixtures. +func fileMeta(level uint32, fileID uint64, size uint64, smallest, largest keys.InternalKey) FileMetadata { + return FileMetadata{ + Level: level, + FileID: fileID, + Size: size, + SmallestKey: smallest, + LargestKey: largest, + } +} + +// assertEditsEqual compares two VersionEdits field-by-field and fails the test +// on any mismatch. Used because reflect.DeepEqual fails on InternalKey when +// UserKey is nil vs an empty slice. +func assertEditsEqual(t *testing.T, got, want *VersionEdit) { + t.Helper() + if got.HasNextFileID != want.HasNextFileID || got.NextFileID != want.NextFileID { + t.Errorf("NextFileID: got (has=%v, val=%d), want (has=%v, val=%d)", + got.HasNextFileID, got.NextFileID, want.HasNextFileID, want.NextFileID) + } + if got.HasLastSequence != want.HasLastSequence || got.LastSequence != want.LastSequence { + t.Errorf("LastSequence: got (has=%v, val=%d), want (has=%v, val=%d)", + got.HasLastSequence, got.LastSequence, want.HasLastSequence, want.LastSequence) + } + if got.HasLogNumber != want.HasLogNumber || got.LogNumber != want.LogNumber { + t.Errorf("LogNumber: got (has=%v, val=%d), want (has=%v, val=%d)", + got.HasLogNumber, got.LogNumber, want.HasLogNumber, want.LogNumber) + } + if len(got.DeletedFiles) != len(want.DeletedFiles) { + t.Fatalf("DeletedFiles length: got %d, want %d", len(got.DeletedFiles), len(want.DeletedFiles)) + } + for i := range want.DeletedFiles { + if got.DeletedFiles[i] != want.DeletedFiles[i] { + t.Errorf("DeletedFiles[%d]: got %+v, want %+v", i, got.DeletedFiles[i], want.DeletedFiles[i]) + } + } + if len(got.AddedFiles) != len(want.AddedFiles) { + t.Fatalf("AddedFiles length: got %d, want %d", len(got.AddedFiles), len(want.AddedFiles)) + } + for i := range want.AddedFiles { + g, w := got.AddedFiles[i], want.AddedFiles[i] + if g.Level != w.Level || g.FileID != w.FileID || g.Size != w.Size { + t.Errorf("AddedFiles[%d] scalars: got {L=%d, ID=%d, Sz=%d}, want {L=%d, ID=%d, Sz=%d}", + i, g.Level, g.FileID, g.Size, w.Level, w.FileID, w.Size) + } + if !bytes.Equal(g.SmallestKey.UserKey, w.SmallestKey.UserKey) || + g.SmallestKey.Seqno != w.SmallestKey.Seqno || + g.SmallestKey.Kind != w.SmallestKey.Kind { + t.Errorf("AddedFiles[%d].SmallestKey: got %+v, want %+v", i, g.SmallestKey, w.SmallestKey) + } + if !bytes.Equal(g.LargestKey.UserKey, w.LargestKey.UserKey) || + g.LargestKey.Seqno != w.LargestKey.Seqno || + g.LargestKey.Kind != w.LargestKey.Kind { + t.Errorf("AddedFiles[%d].LargestKey: got %+v, want %+v", i, g.LargestKey, w.LargestKey) + } + } +} + +func TestVersionEdit_RoundTrip(t *testing.T) { + edit := &VersionEdit{ + HasNextFileID: true, + NextFileID: 42, + HasLastSequence: true, + LastSequence: 100, + HasLogNumber: true, + LogNumber: 7, + DeletedFiles: []DeletedFile{ + {Level: 1, FileID: 11}, + }, + AddedFiles: []FileMetadata{ + fileMeta(0, 7, 1024, putKey("apple", 1), putKey("zebra", 2)), + }, + } + + encoded := edit.Encode() + if len(encoded) == 0 { + t.Fatal("expected non-empty encoding for non-empty edit") + } + + decoded, err := DecodeVersionEdit(encoded) + if err != nil { + t.Fatalf("DecodeVersionEdit failed: %v", err) + } + + assertEditsEqual(t, decoded, edit) +} + +func TestVersionEdit_RoundTrip_Partial(t *testing.T) { + // Only AddedFiles set; all counters and DeletedFiles must decode as zero values. + edit := &VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(2, 99, 2048, putKey("aaa", 5), putKey("zzz", 6)), + }, + } + + encoded := edit.Encode() + decoded, err := DecodeVersionEdit(encoded) + if err != nil { + t.Fatalf("DecodeVersionEdit failed: %v", err) + } + + if decoded.HasNextFileID || decoded.HasLastSequence || decoded.HasLogNumber { + t.Errorf("unset counters should decode with Has* == false; got %+v", decoded) + } + if decoded.NextFileID != 0 || decoded.LastSequence != 0 || decoded.LogNumber != 0 { + t.Errorf("unset counter values should be zero; got %+v", decoded) + } + if len(decoded.DeletedFiles) != 0 { + t.Errorf("expected zero DeletedFiles, got %d", len(decoded.DeletedFiles)) + } + + assertEditsEqual(t, decoded, edit) +} + +func TestVersionEdit_RoundTrip_Empty(t *testing.T) { + edit := &VersionEdit{} + + encoded := edit.Encode() + if len(encoded) != 0 { + t.Errorf("empty edit should encode to zero-byte body, got %d bytes", len(encoded)) + } + + decoded, err := DecodeVersionEdit(encoded) + if err != nil { + t.Fatalf("DecodeVersionEdit on empty body failed: %v", err) + } + + if !reflect.DeepEqual(decoded, &VersionEdit{}) { + t.Errorf("decoded empty edit should equal zero VersionEdit, got %+v", decoded) + } +} + +func TestVersionEdit_MultipleFiles(t *testing.T) { + edit := &VersionEdit{ + HasNextFileID: true, + NextFileID: 100, + DeletedFiles: []DeletedFile{ + {Level: 0, FileID: 1}, + {Level: 1, FileID: 5}, + }, + AddedFiles: []FileMetadata{ + fileMeta(0, 10, 512, putKey("a", 1), putKey("c", 2)), + fileMeta(0, 11, 768, putKey("d", 3), putKey("f", 4)), + fileMeta(1, 12, 4096, putKey("g", 5), putKey("z", 6)), + }, + } + + encoded := edit.Encode() + decoded, err := DecodeVersionEdit(encoded) + if err != nil { + t.Fatalf("DecodeVersionEdit failed: %v", err) + } + + assertEditsEqual(t, decoded, edit) +} + +func TestVersionEdit_Encode_Deterministic(t *testing.T) { + edit := &VersionEdit{ + HasNextFileID: true, + NextFileID: 42, + HasLastSequence: true, + LastSequence: 100, + AddedFiles: []FileMetadata{ + fileMeta(0, 7, 1024, putKey("apple", 1), putKey("zebra", 2)), + fileMeta(0, 8, 2048, putKey("alpha", 3), putKey("yankee", 4)), + }, + } + + first := edit.Encode() + second := edit.Encode() + if !bytes.Equal(first, second) { + t.Errorf("Encode should be deterministic; first=%x second=%x", first, second) + } +} + +func TestVersionEdit_Encode_EmitOrder(t *testing.T) { + // Counter tags must precede file deltas. Same convention as LevelDB. + edit := &VersionEdit{ + HasNextFileID: true, + NextFileID: 1, + HasLastSequence: true, + LastSequence: 2, + HasLogNumber: true, + LogNumber: 3, + DeletedFiles: []DeletedFile{{Level: 0, FileID: 99}}, + AddedFiles: []FileMetadata{ + fileMeta(0, 100, 1, putKey("a", 1), putKey("b", 2)), + }, + } + + encoded := edit.Encode() + if len(encoded) < 28 { + t.Fatalf("encoded too short: %d bytes", len(encoded)) + } + + // First three tags should be the counter tags in fixed order. + if encoded[0] != tagNextFileID { + t.Errorf("byte[0] = 0x%X, want tagNextFileID (0x%X)", encoded[0], tagNextFileID) + } + if encoded[9] != tagLastSequence { + t.Errorf("byte[9] = 0x%X, want tagLastSequence (0x%X)", encoded[9], tagLastSequence) + } + if encoded[18] != tagLogNumber { + t.Errorf("byte[18] = 0x%X, want tagLogNumber (0x%X)", encoded[18], tagLogNumber) + } + // Then DeleteFile before AddFile. + if encoded[27] != tagDeleteFile { + t.Errorf("byte[27] = 0x%X, want tagDeleteFile (0x%X)", encoded[27], tagDeleteFile) + } +} + +func TestVersionEdit_Decode_UnknownTag(t *testing.T) { + // Inject an unknown tag byte (0x63 = 99). + bad := []byte{0x63} + _, err := DecodeVersionEdit(bad) + if !errors.Is(err, ErrUnknownTag) { + t.Errorf("expected ErrUnknownTag for unknown tag, got %v", err) + } +} + +func TestVersionEdit_Decode_UnknownTag_AfterValidTags(t *testing.T) { + // Valid prefix (NextFileID=42) followed by unknown tag must still fail. + good := (&VersionEdit{HasNextFileID: true, NextFileID: 42}).Encode() + bad := append(append([]byte(nil), good...), 0xFF) + + _, err := DecodeVersionEdit(bad) + if !errors.Is(err, ErrUnknownTag) { + t.Errorf("expected ErrUnknownTag, got %v", err) + } +} + +func TestVersionEdit_Decode_Truncated_CounterTag(t *testing.T) { + // Tag byte present but uint64 payload missing. + bad := []byte{tagNextFileID, 0x00, 0x00, 0x00} + _, err := DecodeVersionEdit(bad) + if !errors.Is(err, ErrTruncatedEdit) { + t.Errorf("expected ErrTruncatedEdit, got %v", err) + } +} + +func TestVersionEdit_Decode_Truncated_DeleteFile(t *testing.T) { + // Tag + level u32 only; fileID missing. + bad := []byte{tagDeleteFile, 0x00, 0x00, 0x00, 0x01} + _, err := DecodeVersionEdit(bad) + if !errors.Is(err, ErrTruncatedEdit) { + t.Errorf("expected ErrTruncatedEdit, got %v", err) + } +} + +func TestVersionEdit_Decode_Truncated_AddFile_Header(t *testing.T) { + // Tag + partial level only. + bad := []byte{tagAddFile, 0x00, 0x00} + _, err := DecodeVersionEdit(bad) + if !errors.Is(err, ErrTruncatedEdit) { + t.Errorf("expected ErrTruncatedEdit, got %v", err) + } +} + +func TestVersionEdit_Decode_Truncated_AddFile_Keys(t *testing.T) { + // Build a valid AddFile then chop the body inside the smallestKey payload. + full := (&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(0, 1, 100, putKey("hello", 1), putKey("world", 2)), + }, + }).Encode() + + // Truncate to a point inside the smallestKey bytes (last few bytes lopped off). + truncated := full[:len(full)-3] + + _, err := DecodeVersionEdit(truncated) + if !errors.Is(err, ErrTruncatedEdit) { + t.Errorf("expected ErrTruncatedEdit, got %v", err) + } +} + +func TestVersionEdit_Decode_OversizedLengthPrefix(t *testing.T) { + // AddFile with a smallestKey length prefix beyond the v1 cap. Without + // the cap, this would allocate 4 GiB. With the cap, decode rejects fast. + bad := []byte{ + tagAddFile, + 0x00, 0x00, 0x00, 0x00, // level = 0 + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, // fileID = 1 + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, // size = 16 + 0xFF, 0xFF, 0xFF, 0xFF, // smallestKey length = 4 GiB + } + + _, err := DecodeVersionEdit(bad) + if !errors.Is(err, ErrEditFieldTooLarge) { + t.Errorf("expected ErrEditFieldTooLarge, got %v", err) + } +} + +func TestVersionEdit_Decode_InvalidInternalKey(t *testing.T) { + // AddFile with a smallestKey payload that fails keys.DecodeInternalKey. + // The key bytes are too short (< 9 bytes), so keys returns ErrCorruptInternalKey. + // DecodeVersionEdit must wrap that error with manifest context. + bad := []byte{ + tagAddFile, + 0x00, 0x00, 0x00, 0x00, // level = 0 + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, // fileID = 1 + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, // size = 16 + 0x00, 0x00, 0x00, 0x02, // smallestKey length = 2 (too short for InternalKey) + 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, // largestKey length = 0 + } + + _, err := DecodeVersionEdit(bad) + if err == nil { + t.Fatal("expected error for invalid InternalKey") + } + // Should propagate keys.ErrCorruptInternalKey under the wrap. + if !errors.Is(err, keys.ErrCorruptInternalKey) { + t.Errorf("expected wrapped keys.ErrCorruptInternalKey, got %v", err) + } +} + +func FuzzVersionEditDecode(f *testing.F) { + // Seed with a few valid encodings and some malformed inputs. + f.Add([]byte{}) + f.Add((&VersionEdit{HasNextFileID: true, NextFileID: 1}).Encode()) + f.Add((&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(0, 1, 10, putKey("a", 1), putKey("b", 2)), + }, + }).Encode()) + f.Add([]byte{0xFF}) // unknown tag + f.Add([]byte{tagAddFile}) // truncated AddFile + f.Add([]byte{tagNextFileID}) // truncated counter + + f.Fuzz(func(_ *testing.T, data []byte) { + // Only requirement: never panic, always return error or success. + _, _ = DecodeVersionEdit(data) + }) +} diff --git a/internal/manifest/errors.go b/internal/manifest/errors.go index 36f886a..58f28de 100644 --- a/internal/manifest/errors.go +++ b/internal/manifest/errors.go @@ -5,4 +5,15 @@ import "errors" var ( // ErrUnknownTag denotes that a tag in the version edit is not supported. ErrUnknownTag = errors.New("beachdb/manifest: unknown version edit tag") + + // ErrTruncatedEdit denotes that a version edit body ran out of bytes + // mid-field during decode. The framing layer (record) already verified + // the payload length and checksum, so a short read here means the body + // itself is malformed — not the file. Parallels engine.ErrTruncatedBatch. + ErrTruncatedEdit = errors.New("beachdb/manifest: truncated version edit body") + + // ErrEditFieldTooLarge denotes that a length-prefixed field in a version + // edit declared a size beyond what v1 accepts. Caps untrusted length + // prefixes so corruption can't trigger a multi-GiB allocation. + ErrEditFieldTooLarge = errors.New("beachdb/manifest: version edit field exceeds maximum size") ) From e769d81541367feabdc4af7e0da2666bd9b4a081 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 25 May 2026 19:25:35 +0200 Subject: [PATCH 08/26] Strengthen VersionEdit decode coverage with largestKey-twin tests, encodedSize-vs-Encode parity, tag-order independence, wider fuzz seeds, and a table-driven truncation suite. --- internal/manifest/edit_test.go | 272 ++++++++++++++++++++++++++++----- 1 file changed, 235 insertions(+), 37 deletions(-) diff --git a/internal/manifest/edit_test.go b/internal/manifest/edit_test.go index 333e4b9..4b55489 100644 --- a/internal/manifest/edit_test.go +++ b/internal/manifest/edit_test.go @@ -193,7 +193,12 @@ func TestVersionEdit_Encode_Deterministic(t *testing.T) { } } -func TestVersionEdit_Encode_EmitOrder(t *testing.T) { +// TestVersionEdit_Encode_EmitOrder_Canary is intentionally white-box: it +// inspects byte offsets to lock in the wire-format emit order documented in +// docs/formats/manifest.md (counters first, then deletes, then adds). If a +// refactor changes the emit order, update the format spec deliberately, then +// update this canary — do not just adjust the offsets. +func TestVersionEdit_Encode_EmitOrder_Canary(t *testing.T) { // Counter tags must precede file deltas. Same convention as LevelDB. edit := &VersionEdit{ HasNextFileID: true, @@ -249,47 +254,48 @@ func TestVersionEdit_Decode_UnknownTag_AfterValidTags(t *testing.T) { } } -func TestVersionEdit_Decode_Truncated_CounterTag(t *testing.T) { - // Tag byte present but uint64 payload missing. - bad := []byte{tagNextFileID, 0x00, 0x00, 0x00} - _, err := DecodeVersionEdit(bad) - if !errors.Is(err, ErrTruncatedEdit) { - t.Errorf("expected ErrTruncatedEdit, got %v", err) - } -} - -func TestVersionEdit_Decode_Truncated_DeleteFile(t *testing.T) { - // Tag + level u32 only; fileID missing. - bad := []byte{tagDeleteFile, 0x00, 0x00, 0x00, 0x01} - _, err := DecodeVersionEdit(bad) - if !errors.Is(err, ErrTruncatedEdit) { - t.Errorf("expected ErrTruncatedEdit, got %v", err) - } -} - -func TestVersionEdit_Decode_Truncated_AddFile_Header(t *testing.T) { - // Tag + partial level only. - bad := []byte{tagAddFile, 0x00, 0x00} - _, err := DecodeVersionEdit(bad) - if !errors.Is(err, ErrTruncatedEdit) { - t.Errorf("expected ErrTruncatedEdit, got %v", err) - } -} - -func TestVersionEdit_Decode_Truncated_AddFile_Keys(t *testing.T) { - // Build a valid AddFile then chop the body inside the smallestKey payload. - full := (&VersionEdit{ +func TestVersionEdit_Decode_Truncated(t *testing.T) { + // Build a valid AddFile body once, for the keys-truncation case. + validAddFile := (&VersionEdit{ AddedFiles: []FileMetadata{ fileMeta(0, 1, 100, putKey("hello", 1), putKey("world", 2)), }, }).Encode() - // Truncate to a point inside the smallestKey bytes (last few bytes lopped off). - truncated := full[:len(full)-3] + cases := []struct { + name string + body []byte + wantErr error + }{ + { + name: "counter tag missing uint64 payload", + body: []byte{tagNextFileID, 0x00, 0x00, 0x00}, + wantErr: ErrTruncatedEdit, + }, + { + name: "delete file missing fileID after level", + body: []byte{tagDeleteFile, 0x00, 0x00, 0x00, 0x01}, + wantErr: ErrTruncatedEdit, + }, + { + name: "add file partial level u32", + body: []byte{tagAddFile, 0x00, 0x00}, + wantErr: ErrTruncatedEdit, + }, + { + name: "add file mid smallest key bytes", + body: validAddFile[:len(validAddFile)-3], + wantErr: ErrTruncatedEdit, + }, + } - _, err := DecodeVersionEdit(truncated) - if !errors.Is(err, ErrTruncatedEdit) { - t.Errorf("expected ErrTruncatedEdit, got %v", err) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := DecodeVersionEdit(tc.body) + if !errors.Is(err, tc.wantErr) { + t.Errorf("got %v, want %v", err, tc.wantErr) + } + }) } } @@ -334,8 +340,164 @@ func TestVersionEdit_Decode_InvalidInternalKey(t *testing.T) { } } +// addFileHeader builds the constant tag + level + fileID + size prefix of an +// AddFile TLV record (everything before the smallestKey length prefix). +func addFileHeader() []byte { + return []byte{ + tagAddFile, + 0x00, 0x00, 0x00, 0x00, // level = 0 + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, // fileID = 1 + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, // size = 16 + } +} + +// appendLengthPrefixed appends a length-prefixed byte slice to buf using the +// production encoder helper, so test fixtures stay aligned with the wire format. +func appendLengthPrefixed(buf, b []byte) []byte { + lp := make([]byte, 4+len(b)) + writeLengthPrefixedBytes(lp, 0, b) + return append(buf, lp...) +} + +func TestVersionEdit_Decode_OversizedLengthPrefix_LargestKey(t *testing.T) { + // AddFile where smallestKey is valid but largestKey length prefix is oversized. + // Defends against an asymmetric bug in the second readLengthPrefixedBytes call. + smallest := putKey("a", 1).Encode() + bad := make([]byte, 0, len(addFileHeader())+4+len(smallest)+4) + bad = append(bad, addFileHeader()...) + bad = appendLengthPrefixed(bad, smallest) + bad = append(bad, 0xFF, 0xFF, 0xFF, 0xFF) // largestKey length = 4 GiB + + _, err := DecodeVersionEdit(bad) + if !errors.Is(err, ErrEditFieldTooLarge) { + t.Errorf("expected ErrEditFieldTooLarge, got %v", err) + } +} + +func TestVersionEdit_Decode_InvalidInternalKey_LargestKey(t *testing.T) { + // AddFile where smallestKey is valid but largestKey payload is too short + // to be a valid InternalKey. Confirms the wrap site for largest key at + // decodeAddedFile returns errors.Is(_, keys.ErrCorruptInternalKey). + smallest := putKey("a", 1).Encode() + bad := make([]byte, 0, len(addFileHeader())+4+len(smallest)+6) + bad = append(bad, addFileHeader()...) + bad = appendLengthPrefixed(bad, smallest) + bad = append(bad, + 0x00, 0x00, 0x00, 0x02, // largestKey length = 2 (too short) + 0x00, 0x00, + ) + + _, err := DecodeVersionEdit(bad) + if err == nil { + t.Fatal("expected error for invalid largest InternalKey") + } + if !errors.Is(err, keys.ErrCorruptInternalKey) { + t.Errorf("expected wrapped keys.ErrCorruptInternalKey, got %v", err) + } +} + +func TestVersionEdit_RoundTrip_ZeroLengthUserKey(t *testing.T) { + // Empty user key is valid: encoded InternalKey is just the 9-byte trailer. + // Exercises the len=0 length-prefix path on both encode and decode. + edit := &VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(0, 1, 64, + keys.InternalKey{UserKey: nil, Seqno: 1, Kind: keys.InternalKeyKindPut}, + keys.InternalKey{UserKey: []byte{}, Seqno: 2, Kind: keys.InternalKeyKindPut}, + ), + }, + } + + encoded := edit.Encode() + decoded, err := DecodeVersionEdit(encoded) + if err != nil { + t.Fatalf("DecodeVersionEdit failed: %v", err) + } + assertEditsEqual(t, decoded, edit) +} + +func TestVersionEdit_Encode_Length_Matches_EncodedSize(t *testing.T) { + // Silent-divergence canary: if encodedSize and Encode disagree on byte count, + // Encode would either panic on out-of-bounds write or leave trailing zeros. + cases := []struct { + name string + edit *VersionEdit + }{ + {"empty", &VersionEdit{}}, + {"counters only", &VersionEdit{ + HasNextFileID: true, NextFileID: 1, + HasLastSequence: true, LastSequence: 2, + HasLogNumber: true, LogNumber: 3, + }}, + {"deletes only", &VersionEdit{ + DeletedFiles: []DeletedFile{{Level: 0, FileID: 1}, {Level: 1, FileID: 2}}, + }}, + {"adds only", &VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(0, 1, 100, putKey("a", 1), putKey("b", 2)), + fileMeta(2, 3, 200, putKey("c", 3), putKey("d", 4)), + }, + }}, + {"mixed", &VersionEdit{ + HasNextFileID: true, NextFileID: 7, + DeletedFiles: []DeletedFile{{Level: 0, FileID: 1}}, + AddedFiles: []FileMetadata{ + fileMeta(1, 2, 50, putKey("x", 1), putKey("y", 2)), + }, + }}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + addedBytes := make([]addedFileBytes, len(tc.edit.AddedFiles)) + for i := range tc.edit.AddedFiles { + addedBytes[i].smallest = tc.edit.AddedFiles[i].SmallestKey.Encode() + addedBytes[i].largest = tc.edit.AddedFiles[i].LargestKey.Encode() + } + want := tc.edit.encodedSize(addedBytes) + got := len(tc.edit.Encode()) + if got != want { + t.Errorf("len(Encode()) = %d, encodedSize() = %d", got, want) + } + }) + } +} + +func TestVersionEdit_Decode_TagOrderIndependent(t *testing.T) { + // The decoder loops over whatever tag appears next, with no order + // assumption. Hand-craft a stream where adds precede counters and deletes, + // and verify decode produces the same VersionEdit as the canonical order. + canonical := &VersionEdit{ + HasNextFileID: true, NextFileID: 42, + DeletedFiles: []DeletedFile{{Level: 1, FileID: 99}}, + AddedFiles: []FileMetadata{ + fileMeta(0, 7, 1024, putKey("a", 1), putKey("z", 2)), + }, + } + + // Build a non-canonical stream manually: AddFile first, then DeleteFile, then NextFileID. + addedBytes := addedFileBytes{ + smallest: canonical.AddedFiles[0].SmallestKey.Encode(), + largest: canonical.AddedFiles[0].LargestKey.Encode(), + } + bufSize := 1 + 4 + 8 + 8 + 4 + len(addedBytes.smallest) + 4 + len(addedBytes.largest) + + 1 + 4 + 8 + + 1 + 8 + buf := make([]byte, bufSize) + off := 0 + off = writeAddedFile(buf, off, canonical.AddedFiles[0], addedBytes) + off = writeDeletedFile(buf, off, canonical.DeletedFiles[0]) + _ = writeCounterTag(buf, off, tagNextFileID, true, canonical.NextFileID) + + decoded, err := DecodeVersionEdit(buf) + if err != nil { + t.Fatalf("DecodeVersionEdit failed: %v", err) + } + assertEditsEqual(t, decoded, canonical) +} + func FuzzVersionEditDecode(f *testing.F) { - // Seed with a few valid encodings and some malformed inputs. + // Minimal seeds: happy path + error paths. f.Add([]byte{}) f.Add((&VersionEdit{HasNextFileID: true, NextFileID: 1}).Encode()) f.Add((&VersionEdit{ @@ -347,6 +509,42 @@ func FuzzVersionEditDecode(f *testing.F) { f.Add([]byte{tagAddFile}) // truncated AddFile f.Add([]byte{tagNextFileID}) // truncated counter + // Wider seeds: bootstrap corpus for multi-file, large-key, and counter variants. + manyFiles := make([]FileMetadata, 12) + for i := range manyFiles { + manyFiles[i] = fileMeta(uint32(i%3), uint64(i+100), uint64(1024*(i+1)), + putKey(string([]byte{'a' + byte(i)}), uint64(i+1)), + putKey(string([]byte{'b' + byte(i)}), uint64(i+2)), + ) + } + f.Add((&VersionEdit{AddedFiles: manyFiles}).Encode()) + + bigUserKey := make([]byte, 4*1024) + for i := range bigUserKey { + bigUserKey[i] = byte(i % 251) + } + f.Add((&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(0, 1, 1, + keys.InternalKey{UserKey: bigUserKey, Seqno: 1, Kind: keys.InternalKeyKindPut}, + keys.InternalKey{UserKey: bigUserKey, Seqno: 2, Kind: keys.InternalKeyKindPut}, + ), + }, + }).Encode()) + + f.Add((&VersionEdit{HasLastSequence: true, LastSequence: 1234567}).Encode()) + + f.Add((&VersionEdit{ + HasNextFileID: true, NextFileID: 10, + HasLastSequence: true, LastSequence: 20, + HasLogNumber: true, LogNumber: 30, + DeletedFiles: []DeletedFile{{Level: 0, FileID: 1}}, + AddedFiles: []FileMetadata{ + fileMeta(0, 2, 100, putKey("a", 1), putKey("b", 2)), + fileMeta(1, 3, 200, putKey("c", 3), putKey("d", 4)), + }, + }).Encode()) + f.Fuzz(func(_ *testing.T, data []byte) { // Only requirement: never panic, always return error or success. _, _ = DecodeVersionEdit(data) From a980adbc4bc72033f34e65a3e4d087929e0dc96a Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 25 May 2026 19:25:55 +0200 Subject: [PATCH 09/26] Finish Version with immutable Apply (clone, gap-fill, L1+ sort by SmallestKey, idempotent deletes) and accessors, plus full tests for clone isolation, sort invariant, and defensive copies. --- internal/manifest/version.go | 115 ++++++++-- internal/manifest/version_test.go | 355 ++++++++++++++++++++++++++++++ 2 files changed, 457 insertions(+), 13 deletions(-) diff --git a/internal/manifest/version.go b/internal/manifest/version.go index 1c998df..5ecc9de 100644 --- a/internal/manifest/version.go +++ b/internal/manifest/version.go @@ -1,31 +1,120 @@ package manifest +import ( + "slices" +) + // Version represents an in-memory snapshot of which SSTables exist. type Version struct { - _ [][]FileMetadata // files[level] = sorted list of files at that level (TODO: scaffolding for upcoming work) + // files[level] holds the SSTables at that level. For level >= 1 the slice + // is sorted by SmallestKey ascending. L0 ordering is deferred until + // iterators land. + files [][]FileMetadata +} + +// NewVersion returns a new Version with the specified capacity. +func NewVersion(capacity uint32) *Version { + return &Version{ + files: make([][]FileMetadata, 0, capacity), + } } -// NewVersion returns a new Version. -func NewVersion() *Version { - return &Version{} +// Clone returns a deep copy of v: a new outer slice plus a fresh inner slice +// per level. FileMetadata values are copied by value. +func (v *Version) Clone() *Version { + clone := &Version{ + files: make([][]FileMetadata, len(v.files)), + } + for level, filesList := range v.files { + clone.files[level] = slices.Clone(filesList) + } + return clone } -// Apply applies a single atomic version edit to the current Version. -func (v *Version) Apply(_ *VersionEdit) *Version { - return &Version{} +// Apply returns a new Version with edit applied. v is not modified. +// Levels touched by the edit (excluding L0) are sorted by SmallestKey +// before return. +func (v *Version) Apply(edit *VersionEdit) *Version { + newVersion := v.Clone() + touched := make(map[uint32]struct{}) + + for _, deletedFile := range edit.DeletedFiles { + level := deletedFile.Level + fileID := deletedFile.FileID + touched[level] = struct{}{} + + if int(level) >= len(newVersion.files) { + continue // idempotent: level never had files + } + + files := newVersion.files[level] + for i, fm := range files { + if fm.FileID == fileID { + newVersion.files[level] = slices.Delete(files, i, i+1) + break // at most one file per ID per level + } + } + } + + for _, addedFile := range edit.AddedFiles { + level := addedFile.Level + touched[level] = struct{}{} + + for int(level) >= len(newVersion.files) { + newVersion.files = append(newVersion.files, nil) + } + + newVersion.files[level] = append(newVersion.files[level], addedFile) + } + + for level := range touched { + // L0 overlap; ordering deferred until iterators land. + if level == 0 { + continue + } + newVersion.sortFilesAt(level) + } + + return newVersion } -// Files returns the list of files at a given level. -func (v *Version) Files(_ int) []FileMetadata { - return nil +// Files returns a copy of the files at the given level. Returns nil for +// levels above NumLevels so callers can safely range over levels without +// bounds-checking. +func (v *Version) Files(level uint32) []FileMetadata { + if int(level) >= len(v.files) { + return nil + } + return slices.Clone(v.files[level]) } -// AllFiles returns the list of all files in the version. +// AllFiles returns a flat slice of every file in the version, ordered by +// level ascending. func (v *Version) AllFiles() []FileMetadata { - return nil + totalFiles := 0 + for i := range len(v.files) { + totalFiles += len(v.files[i]) + } + + allFiles := make([]FileMetadata, 0, totalFiles) + for _, filesAtLevel := range v.files { + allFiles = append(allFiles, filesAtLevel...) + } + return allFiles } // NumLevels returns the number of SSTable levels in the version. func (v *Version) NumLevels() int { - return 0 + return len(v.files) +} + +// sortFilesAt sorts files at the given level in-place by SmallestKey ascending. +// No-op if level is out of range. +func (v *Version) sortFilesAt(level uint32) { + if int(level) >= len(v.files) { + return + } + slices.SortFunc(v.files[level], func(a, b FileMetadata) int { + return a.SmallestKey.Compare(b.SmallestKey) + }) } diff --git a/internal/manifest/version_test.go b/internal/manifest/version_test.go index 88367b0..202ba4b 100644 --- a/internal/manifest/version_test.go +++ b/internal/manifest/version_test.go @@ -1 +1,356 @@ package manifest + +import ( + "testing" + + "github.com/aalhour/beachdb/internal/keys" +) + +// assertFileIDsAt fails the test if the files at level do not match the +// expected fileID sequence (order matters). Reads the version through Files() +// to exercise the defensive-copy path. +func assertFileIDsAt(t *testing.T, v *Version, level uint32, want ...uint64) { + t.Helper() + got := v.Files(level) + if len(got) != len(want) { + t.Fatalf("level %d: got %d files, want %d", level, len(got), len(want)) + } + for i := range want { + if got[i].FileID != want[i] { + t.Errorf("level %d, files[%d].FileID: got %d, want %d", + level, i, got[i].FileID, want[i]) + } + } +} + +func TestNewVersion(t *testing.T) { + t.Run("zero capacity is valid and starts empty", func(t *testing.T) { + v := NewVersion(0) + if v.NumLevels() != 0 { + t.Errorf("NumLevels = %d, want 0", v.NumLevels()) + } + if len(v.AllFiles()) != 0 { + t.Errorf("AllFiles = %v, want empty", v.AllFiles()) + } + }) + + t.Run("positive capacity hint does not pre-fill levels", func(t *testing.T) { + v := NewVersion(7) + if v.NumLevels() != 0 { + t.Errorf("NumLevels = %d, want 0 (capacity is a hint, not length)", v.NumLevels()) + } + }) +} + +func TestVersion_Clone(t *testing.T) { + build := func() *Version { + v := NewVersion(0) + return v.Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(0, 1, 100, putKey("a", 1), putKey("b", 2)), + fileMeta(1, 2, 200, putKey("c", 3), putKey("d", 4)), + }, + }) + } + + t.Run("empty version clones to empty", func(t *testing.T) { + v := NewVersion(0) + c := v.Clone() + if c.NumLevels() != 0 { + t.Errorf("clone.NumLevels = %d, want 0", c.NumLevels()) + } + }) + + t.Run("clone has equal AllFiles", func(t *testing.T) { + v := build() + c := v.Clone() + vAll := v.AllFiles() + cAll := c.AllFiles() + if len(vAll) != len(cAll) { + t.Fatalf("AllFiles len: original=%d clone=%d", len(vAll), len(cAll)) + } + for i := range vAll { + if vAll[i].FileID != cAll[i].FileID { + t.Errorf("AllFiles[%d].FileID: original=%d clone=%d", + i, vAll[i].FileID, cAll[i].FileID) + } + } + }) + + t.Run("mutating clone does not affect original", func(t *testing.T) { + v := build() + c := v.Clone() + c.files[0][0].FileID = 999 + if v.files[0][0].FileID == 999 { + t.Error("mutating clone bled into original") + } + }) + + t.Run("mutating original does not affect clone", func(t *testing.T) { + v := build() + c := v.Clone() + v.files[0][0].FileID = 888 + if c.files[0][0].FileID == 888 { + t.Error("mutating original bled into clone") + } + }) +} + +func TestVersion_Apply_Empty(t *testing.T) { + v := NewVersion(0).Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(0, 1, 100, putKey("a", 1), putKey("b", 2)), + }, + }) + + v2 := v.Apply(&VersionEdit{}) + if v2.NumLevels() != v.NumLevels() { + t.Errorf("NumLevels diverged after empty Apply: got %d, want %d", + v2.NumLevels(), v.NumLevels()) + } + assertFileIDsAt(t, v2, 0, 1) +} + +func TestVersion_Apply_AddedFiles(t *testing.T) { + t.Run("adds at L0 preserve insertion order (no sort)", func(t *testing.T) { + v := NewVersion(0).Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(0, 3, 100, putKey("c", 1), putKey("cz", 2)), + fileMeta(0, 1, 100, putKey("a", 3), putKey("az", 4)), + fileMeta(0, 2, 100, putKey("b", 5), putKey("bz", 6)), + }, + }) + assertFileIDsAt(t, v, 0, 3, 1, 2) // insertion order, not key order + }) + + t.Run("adds at L2 gap-fill empty L0 and L1", func(t *testing.T) { + v := NewVersion(0).Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(2, 1, 100, putKey("a", 1), putKey("z", 2)), + }, + }) + if v.NumLevels() != 3 { + t.Errorf("NumLevels = %d, want 3 (L0, L1 nil; L2 has the file)", v.NumLevels()) + } + if len(v.Files(0)) != 0 || len(v.Files(1)) != 0 { + t.Error("gap-fill levels should be empty") + } + assertFileIDsAt(t, v, 2, 1) + }) + + t.Run("adds at L1 sort by SmallestKey ascending", func(t *testing.T) { + v := NewVersion(0).Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(1, 30, 100, putKey("c", 1), putKey("cz", 2)), + fileMeta(1, 10, 100, putKey("a", 3), putKey("az", 4)), + fileMeta(1, 20, 100, putKey("b", 5), putKey("bz", 6)), + }, + }) + assertFileIDsAt(t, v, 1, 10, 20, 30) // sorted by SmallestKey: a, b, c + }) +} + +func TestVersion_Apply_DeletedFiles(t *testing.T) { + seed := &VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(1, 1, 100, putKey("a", 1), putKey("az", 2)), + fileMeta(1, 2, 100, putKey("b", 3), putKey("bz", 4)), + fileMeta(1, 3, 100, putKey("c", 5), putKey("cz", 6)), + }, + } + + t.Run("delete existing file", func(t *testing.T) { + v := NewVersion(0).Apply(seed) + v2 := v.Apply(&VersionEdit{ + DeletedFiles: []DeletedFile{{Level: 1, FileID: 2}}, + }) + assertFileIDsAt(t, v2, 1, 1, 3) + }) + + t.Run("delete non-existent fileID is idempotent", func(t *testing.T) { + v := NewVersion(0).Apply(seed) + v2 := v.Apply(&VersionEdit{ + DeletedFiles: []DeletedFile{{Level: 1, FileID: 999}}, + }) + assertFileIDsAt(t, v2, 1, 1, 2, 3) + }) + + t.Run("delete from level beyond NumLevels is idempotent", func(t *testing.T) { + v := NewVersion(0).Apply(seed) + v2 := v.Apply(&VersionEdit{ + DeletedFiles: []DeletedFile{{Level: 99, FileID: 1}}, + }) + assertFileIDsAt(t, v2, 1, 1, 2, 3) + }) +} + +func TestVersion_Apply_Combined(t *testing.T) { + v := NewVersion(0).Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(1, 1, 100, putKey("a", 1), putKey("az", 2)), + fileMeta(1, 2, 100, putKey("b", 3), putKey("bz", 4)), + }, + }) + + v2 := v.Apply(&VersionEdit{ + DeletedFiles: []DeletedFile{{Level: 1, FileID: 1}}, + AddedFiles: []FileMetadata{ + fileMeta(1, 3, 100, putKey("ab", 5), putKey("abz", 6)), + }, + }) + + // L1 should now hold {2, 3} sorted by SmallestKey: "ab" < "b" → fileIDs [3, 2]. + assertFileIDsAt(t, v2, 1, 3, 2) +} + +func TestVersion_Apply_Immutability(t *testing.T) { + v := NewVersion(0).Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(0, 1, 100, putKey("a", 1), putKey("b", 2)), + fileMeta(1, 2, 100, putKey("c", 3), putKey("d", 4)), + }, + }) + + before := v.AllFiles() + _ = v.Apply(&VersionEdit{ + DeletedFiles: []DeletedFile{{Level: 0, FileID: 1}}, + AddedFiles: []FileMetadata{ + fileMeta(1, 9, 100, putKey("e", 5), putKey("f", 6)), + }, + }) + after := v.AllFiles() + + if len(before) != len(after) { + t.Fatalf("Apply mutated original: before=%d files, after=%d files", + len(before), len(after)) + } + for i := range before { + if before[i].FileID != after[i].FileID { + t.Errorf("AllFiles[%d].FileID mutated: before=%d, after=%d", + i, before[i].FileID, after[i].FileID) + } + } +} + +func TestVersion_Apply_L0NotSortedByKey(t *testing.T) { + // L0 files overlap; ordering by key is meaningless. Verify the sort step + // is skipped for L0 by adding three files in descending key order and + // asserting insertion order is preserved. + v := NewVersion(0).Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(0, 1, 100, putKey("z", 1), putKey("zz", 2)), + fileMeta(0, 2, 100, putKey("m", 3), putKey("mz", 4)), + fileMeta(0, 3, 100, putKey("a", 5), putKey("az", 6)), + }, + }) + assertFileIDsAt(t, v, 0, 1, 2, 3) // insertion order, not sorted +} + +func TestVersion_Apply_SortInvariant_AfterDelete(t *testing.T) { + v := NewVersion(0).Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(1, 10, 100, putKey("a", 1), putKey("az", 2)), + fileMeta(1, 20, 100, putKey("b", 3), putKey("bz", 4)), + fileMeta(1, 30, 100, putKey("c", 5), putKey("cz", 6)), + }, + }) + + v2 := v.Apply(&VersionEdit{ + DeletedFiles: []DeletedFile{{Level: 1, FileID: 20}}, + }) + + // Remaining files were already sorted; sort-on-touched is a no-op. + got := v2.Files(1) + for i := 1; i < len(got); i++ { + if got[i-1].SmallestKey.Compare(got[i].SmallestKey) > 0 { + t.Errorf("sort invariant broken at L1: %s > %s", + got[i-1].SmallestKey.UserKey, got[i].SmallestKey.UserKey) + } + } +} + +func TestVersion_Files(t *testing.T) { + v := NewVersion(0).Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(0, 1, 100, putKey("a", 1), putKey("b", 2)), + }, + }) + + t.Run("existing level returns files", func(t *testing.T) { + got := v.Files(0) + if len(got) != 1 || got[0].FileID != 1 { + t.Errorf("Files(0) = %+v, want one file with FileID=1", got) + } + }) + + t.Run("level beyond NumLevels returns nil", func(t *testing.T) { + got := v.Files(99) + if got != nil { + t.Errorf("Files(99) = %v, want nil", got) + } + }) + + t.Run("returned slice is a defensive copy", func(t *testing.T) { + got := v.Files(0) + got[0].FileID = 12345 + again := v.Files(0) + if again[0].FileID == 12345 { + t.Error("mutating returned slice leaked into Version internals") + } + }) +} + +func TestVersion_AllFiles(t *testing.T) { + t.Run("empty Version returns empty slice", func(t *testing.T) { + v := NewVersion(0) + got := v.AllFiles() + if len(got) != 0 { + t.Errorf("AllFiles = %v, want empty", got) + } + }) + + t.Run("multi-level concatenates ordered by level ascending", func(t *testing.T) { + v := NewVersion(0).Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(2, 200, 100, putKey("a", 1), putKey("z", 2)), + fileMeta(0, 1, 100, putKey("a", 3), putKey("z", 4)), + fileMeta(0, 2, 100, putKey("a", 5), putKey("z", 6)), + fileMeta(1, 100, 100, putKey("a", 7), putKey("z", 8)), + }, + }) + got := v.AllFiles() + wantOrder := []uint64{1, 2, 100, 200} + if len(got) != len(wantOrder) { + t.Fatalf("AllFiles len = %d, want %d", len(got), len(wantOrder)) + } + for i, fid := range wantOrder { + if got[i].FileID != fid { + t.Errorf("AllFiles[%d].FileID = %d, want %d (level-ascending order)", + i, got[i].FileID, fid) + } + } + }) +} + +func TestVersion_NumLevels(t *testing.T) { + t.Run("empty Version", func(t *testing.T) { + v := NewVersion(0) + if v.NumLevels() != 0 { + t.Errorf("NumLevels = %d, want 0", v.NumLevels()) + } + }) + + t.Run("after Apply adding at L3 reports 4 levels", func(t *testing.T) { + v := NewVersion(0).Apply(&VersionEdit{ + AddedFiles: []FileMetadata{ + fileMeta(3, 1, 100, + keys.InternalKey{UserKey: []byte("a"), Seqno: 1, Kind: keys.InternalKeyKindPut}, + keys.InternalKey{UserKey: []byte("b"), Seqno: 2, Kind: keys.InternalKeyKindPut}, + ), + }, + }) + if v.NumLevels() != 4 { + t.Errorf("NumLevels = %d, want 4", v.NumLevels()) + } + }) +} From 12488da1fb14dd0444df4f87e40563361360fd0b Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 25 May 2026 20:54:30 +0200 Subject: [PATCH 10/26] add manifest Writer with parent-dir fsync on create, per-Append durability and Close() func. --- internal/manifest/errors.go | 6 + internal/manifest/writer.go | 110 +++++++ internal/manifest/writer_test.go | 476 +++++++++++++++++++++++++++++++ 3 files changed, 592 insertions(+) diff --git a/internal/manifest/errors.go b/internal/manifest/errors.go index 58f28de..b0bd709 100644 --- a/internal/manifest/errors.go +++ b/internal/manifest/errors.go @@ -16,4 +16,10 @@ var ( // edit declared a size beyond what v1 accepts. Caps untrusted length // prefixes so corruption can't trigger a multi-GiB allocation. ErrEditFieldTooLarge = errors.New("beachdb/manifest: version edit field exceeds maximum size") + + // ErrWriterClosed indicates when the manifest writer is closed. + ErrWriterClosed = errors.New("beachdb/manifest: writer is closed") + + // ErrReaderClosed indicates when the manifest reader is closed. + ErrReaderClosed = errors.New("beachdb/manifest: reader is closed") ) diff --git a/internal/manifest/writer.go b/internal/manifest/writer.go index 88367b0..67d0b67 100644 --- a/internal/manifest/writer.go +++ b/internal/manifest/writer.go @@ -1 +1,111 @@ package manifest + +import ( + "fmt" + "os" + "path/filepath" + "sync" +) + +// Writer provides sequential write access to a MANIFEST record stream. +type Writer struct { + // Single-writer per DB. Mutex defends against accidental concurrent use. + mu sync.Mutex + + // Open file handle for the MANIFEST on disk. + file *os.File + + // Path of the MANIFEST file on disk. + path string +} + +// NewWriter creates a new Writer for the MANIFEST file at the given path. +// If the file does not exist, it is created. The file is always opened in +// append mode. +func NewWriter(path string) (*Writer, error) { + //nolint:gosec // G302: 0644 is acceptable for MANIFEST files + file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644) + if err != nil { + return nil, fmt.Errorf("beachdb/manifest: failed to open file: %w", err) + } + + // Fsync the parent directory so the new file's directory entry is + // durable on disk. Without this, a crash after the file was created + // above can leave the file contents on disk but the dirent lost, + // making the MANIFEST invisible after recovery. + dir, err := os.Open(filepath.Dir(path)) + if err != nil { + _ = file.Close() + return nil, fmt.Errorf("beachdb/manifest: failed to open parent dir: %w", err) + } + defer dir.Close() + if err := dir.Sync(); err != nil { + _ = file.Close() + return nil, fmt.Errorf("beachdb/manifest: failed to sync parent dir: %w", err) + } + + writer := &Writer{ + file: file, + path: path, + } + + return writer, nil +} + +// Append writes a new MANIFEST record containing the given payload, +// and syncs the file to disk. +func (w *Writer) Append(payload []byte) error { + // Encode outside the lock: CPU work and the MaxPayloadSize check happen + // before any contention with concurrent callers. + rec, err := EncodeRecord(payload) + if err != nil { + return err + } + + // Serialize concurrent Append callers. + w.mu.Lock() + defer w.mu.Unlock() + + if w.file == nil { + return ErrWriterClosed + } + + if _, err := w.file.Write(rec); err != nil { + return fmt.Errorf("beachdb/manifest: failed to write record to %s: %w", w.path, err) + } + + if err := w.file.Sync(); err != nil { + return fmt.Errorf("beachdb/manifest: failed to sync file to %s: %w", w.path, err) + } + + return nil +} + +// Close syncs the file and closes the writer. +func (w *Writer) Close() error { + if w == nil { + return ErrWriterClosed + } + + w.mu.Lock() + defer w.mu.Unlock() + + if w.file == nil { + return ErrWriterClosed + } + + var firstErr error + + if err := w.file.Sync(); err != nil { + firstErr = fmt.Errorf("beachdb/manifest: sync file failed during close: %w", err) + } + + if err := w.file.Close(); err != nil && firstErr == nil { + firstErr = fmt.Errorf("beachdb/manifest: close file failed: %w", err) + } + + // Mark as closed even if there was an error + w.file = nil + + return firstErr +} diff --git a/internal/manifest/writer_test.go b/internal/manifest/writer_test.go index 88367b0..60a5509 100644 --- a/internal/manifest/writer_test.go +++ b/internal/manifest/writer_test.go @@ -1 +1,477 @@ package manifest + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/aalhour/beachdb/internal/record" +) + +// tempManifestPath returns a fresh path under t.TempDir() suitable for a +// MANIFEST file. The directory exists but the file does not. +func tempManifestPath(t *testing.T) string { + t.Helper() + return filepath.Join(t.TempDir(), "MANIFEST-000001") +} + +// readAllRecords walks the file at path and returns the decoded payload of +// every record in order. Fails the test on any header decode or checksum +// mismatch. Validates the durability + framing claims of Append end-to-end +// without depending on the (still stubbed) Reader. +func readAllRecords(t *testing.T, path string) [][]byte { + t.Helper() + //nolint:gosec // G304: path is from t.TempDir() which is safe. + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read file: %v", err) + } + + var payloads [][]byte + off := 0 + for off < len(data) { + if off+record.HeaderSize > len(data) { + t.Fatalf("trailing bytes at offset %d: %d bytes left, < HeaderSize=%d", + off, len(data)-off, record.HeaderSize) + } + payloadLen, crc, err := DecodeRecordHeader(data[off : off+record.HeaderSize]) + if err != nil { + t.Fatalf("decode header at offset %d: %v", off, err) + } + off += record.HeaderSize + end := off + int(payloadLen) + if end > len(data) { + t.Fatalf("payload past EOF: header at %d wants %d bytes, file has %d", + off-record.HeaderSize, payloadLen, len(data)-off+record.HeaderSize) + } + payload := data[off:end] + if err := ValidateRecord(payload, crc); err != nil { + t.Fatalf("validate record at offset %d: %v", off-record.HeaderSize, err) + } + payloads = append(payloads, append([]byte(nil), payload...)) + off = end + } + return payloads +} + +func TestNewWriter(t *testing.T) { + t.Run("creates new file at path", func(t *testing.T) { + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + defer w.Close() + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if info.Size() != 0 { + t.Errorf("new file size = %d, want 0", info.Size()) + } + }) + + t.Run("reopens existing file in append mode preserving content", func(t *testing.T) { + path := tempManifestPath(t) + + w1, err := NewWriter(path) + if err != nil { + t.Fatalf("first NewWriter: %v", err) + } + if err := w1.Append([]byte("first")); err != nil { + t.Fatalf("Append: %v", err) + } + if err := w1.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + w2, err := NewWriter(path) + if err != nil { + t.Fatalf("reopen NewWriter: %v", err) + } + defer w2.Close() + if err := w2.Append([]byte("second")); err != nil { + t.Fatalf("Append after reopen: %v", err) + } + + got := readAllRecords(t, path) + if len(got) != 2 { + t.Fatalf("record count = %d, want 2", len(got)) + } + if string(got[0]) != "first" || string(got[1]) != "second" { + t.Errorf("payloads = %q, %q, want %q, %q", got[0], got[1], "first", "second") + } + }) + + t.Run("populates struct fields", func(t *testing.T) { + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + defer w.Close() + + if w.file == nil { + t.Error("file field is nil") + } + if w.path != path { + t.Errorf("path field = %q, want %q", w.path, path) + } + }) +} + +func TestNewWriter_InvalidPath(t *testing.T) { + t.Run("path is an existing directory", func(t *testing.T) { + // OpenFile with O_WRONLY against a directory must fail. + dir := t.TempDir() + _, err := NewWriter(dir) + if err == nil { + t.Fatal("expected error when path is a directory") + } + }) + + t.Run("path under non-existent parent directory", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "missing-subdir", "MANIFEST-000001") + _, err := NewWriter(path) + if err == nil { + t.Fatal("expected error when parent directory does not exist") + } + }) +} + +func TestWriter_Append(t *testing.T) { + t.Run("single record on disk equals EncodeRecord output", func(t *testing.T) { + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + defer w.Close() + + payload := []byte("hello manifest") + if err := w.Append(payload); err != nil { + t.Fatalf("Append: %v", err) + } + + want, err := EncodeRecord(payload) + if err != nil { + t.Fatalf("EncodeRecord: %v", err) + } + //nolint:gosec // G304: path is from t.TempDir() which is safe. + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !bytes.Equal(got, want) { + t.Errorf("on-disk bytes != EncodeRecord output") + } + }) + + t.Run("multi-record stream is concatenation", func(t *testing.T) { + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + defer w.Close() + + payloads := [][]byte{ + []byte("alpha"), + []byte("bravo"), + []byte("charlie"), + } + for _, p := range payloads { + if err := w.Append(p); err != nil { + t.Fatalf("Append(%q): %v", p, err) + } + } + + got := readAllRecords(t, path) + if len(got) != len(payloads) { + t.Fatalf("record count = %d, want %d", len(got), len(payloads)) + } + for i := range payloads { + if !bytes.Equal(got[i], payloads[i]) { + t.Errorf("record[%d] = %q, want %q", i, got[i], payloads[i]) + } + } + }) + + t.Run("records are durable before Close", func(t *testing.T) { + // Append returns only after fsync. Read the file mid-stream (without + // calling Close) and confirm the record is already on disk. + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + defer w.Close() + + payload := []byte("durable-on-return") + if err := w.Append(payload); err != nil { + t.Fatalf("Append: %v", err) + } + + got := readAllRecords(t, path) + if len(got) != 1 || !bytes.Equal(got[0], payload) { + t.Errorf("mid-stream read: got %v, want one record %q", got, payload) + } + }) + + t.Run("empty payload roundtrips", func(t *testing.T) { + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + defer w.Close() + + if err := w.Append([]byte{}); err != nil { + t.Fatalf("Append empty: %v", err) + } + + got := readAllRecords(t, path) + if len(got) != 1 { + t.Fatalf("record count = %d, want 1", len(got)) + } + if len(got[0]) != 0 { + t.Errorf("empty payload decoded to %d bytes, want 0", len(got[0])) + } + }) + + t.Run("Append after Close returns ErrWriterClosed", func(t *testing.T) { + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + err = w.Append([]byte("post-close")) + if !errors.Is(err, ErrWriterClosed) { + t.Errorf("Append after Close: got %v, want ErrWriterClosed", err) + } + }) + + t.Run("Append fails when underlying file closed externally", func(t *testing.T) { + // Sabotage path: close the *os.File directly so Write/Sync fail. + // Confirms the wrapped error path in Append returns a non-nil error + // rather than panicking or silently succeeding. + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + _ = w.file.Close() // bypass Writer.Close + + err = w.Append([]byte("dead-file")) + if err == nil { + t.Error("expected error from Append against closed underlying file") + } + // Don't expect a particular sentinel — Write returns os.ErrClosed, + // wrapped under "failed to write record". + }) +} + +func TestWriter_Append_Concurrent(t *testing.T) { + // N goroutines each Append a unique payload. After all complete, every + // record must be present exactly once and the stream must be parseable. + // Run under `go test -race` to catch unprotected-state corruption. + const N = 100 + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + defer w.Close() + + var wg sync.WaitGroup + wg.Add(N) + for i := range N { + payload := []byte{byte(i / 256), byte(i % 256)} + go func() { + defer wg.Done() + if err := w.Append(payload); err != nil { + t.Errorf("Append: %v", err) + } + }() + } + wg.Wait() + + got := readAllRecords(t, path) + if len(got) != N { + t.Fatalf("record count = %d, want %d", len(got), N) + } + seen := make(map[[2]byte]bool, N) + for _, p := range got { + if len(p) != 2 { + t.Errorf("record payload length = %d, want 2", len(p)) + continue + } + key := [2]byte{p[0], p[1]} + if seen[key] { + t.Errorf("duplicate payload %v", key) + } + seen[key] = true + } + if len(seen) != N { + t.Errorf("unique payloads = %d, want %d", len(seen), N) + } +} + +func TestWriter_Close(t *testing.T) { + t.Run("happy path after Append", func(t *testing.T) { + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + if err := w.Append([]byte("payload")); err != nil { + t.Fatalf("Append: %v", err) + } + if err := w.Close(); err != nil { + t.Errorf("Close: %v", err) + } + }) + + t.Run("double Close returns ErrWriterClosed", func(t *testing.T) { + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + + if err := w.Close(); err != nil { + t.Fatalf("first Close: %v", err) + } + err = w.Close() + if !errors.Is(err, ErrWriterClosed) { + t.Errorf("second Close: got %v, want ErrWriterClosed", err) + } + }) + + t.Run("nil receiver returns ErrWriterClosed without panic", func(t *testing.T) { + var w *Writer + err := w.Close() + if !errors.Is(err, ErrWriterClosed) { + t.Errorf("got %v, want ErrWriterClosed", err) + } + }) + + t.Run("Close fails when underlying file closed externally", func(t *testing.T) { + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + _ = w.file.Close() // sabotage + + // Close's Sync + Close on an already-closed file should return + // a non-nil error but still mark the writer as closed. + err = w.Close() + if err == nil { + t.Error("expected error from Close against closed underlying file") + } + + err = w.Close() + if !errors.Is(err, ErrWriterClosed) { + t.Errorf("subsequent Close: got %v, want ErrWriterClosed", err) + } + }) + + t.Run("data from before Close is preserved on disk", func(t *testing.T) { + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + payload := []byte("survives close") + if err := w.Append(payload); err != nil { + t.Fatalf("Append: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + got := readAllRecords(t, path) + if len(got) != 1 || !bytes.Equal(got[0], payload) { + t.Errorf("got %v, want one record %q", got, payload) + } + }) +} + +// --- Benchmarks --- + +// BenchmarkWriter_Append measures per-Append cost across payload sizes. +// Each iteration encodes + writes + fsyncs one record, so wall time is +// dominated by Sync. b.ReportAllocs() surfaces allocator churn in the +// encoder path; the fsync cost is unrelated to allocations. +func BenchmarkWriter_Append(b *testing.B) { + sizes := []struct { + name string + size int + }{ + {"empty", 0}, + {"small-64B", 64}, + {"medium-1KB", 1024}, + {"large-64KB", 64 * 1024}, + } + for _, s := range sizes { + payload := make([]byte, s.size) + b.Run(s.name, func(b *testing.B) { + path := filepath.Join(b.TempDir(), "bench.manifest") + w, err := NewWriter(path) + if err != nil { + b.Fatal(err) + } + defer w.Close() + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if err := w.Append(payload); err != nil { + b.Fatalf("Append: %v", err) + } + } + }) + } +} + +// BenchmarkNewWriter measures the cost of file creation + parent-dir fsync. +// This path runs once per DB open / manifest rotation, so the wall time +// matters less than the allocation profile. +func BenchmarkNewWriter(b *testing.B) { + dir := b.TempDir() + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + path := filepath.Join(dir, "bench-"+itoa(i)+".manifest") + w, err := NewWriter(path) + if err != nil { + b.Fatalf("NewWriter: %v", err) + } + _ = w.Close() + } +} + +// itoa avoids strconv in the benchmark hot path to keep allocation counts +// honest. Benchmarks should not import strconv just to build a path. +func itoa(n int) string { + if n == 0 { + return "0" + } + var buf [20]byte + i := len(buf) + for n > 0 { + i-- + buf[i] = byte('0' + n%10) + n /= 10 + } + return string(buf[i:]) +} From 4f26a0eb1e850db56e02895593f2f3c1efac5e9c Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 25 May 2026 21:14:02 +0200 Subject: [PATCH 11/26] refactor(manifest): switch parent-dir fsync to fs.SyncDir --- internal/manifest/writer.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/internal/manifest/writer.go b/internal/manifest/writer.go index 67d0b67..fe13c02 100644 --- a/internal/manifest/writer.go +++ b/internal/manifest/writer.go @@ -5,6 +5,8 @@ import ( "os" "path/filepath" "sync" + + "github.com/aalhour/beachdb/internal/fs" ) // Writer provides sequential write access to a MANIFEST record stream. @@ -33,13 +35,7 @@ func NewWriter(path string) (*Writer, error) { // durable on disk. Without this, a crash after the file was created // above can leave the file contents on disk but the dirent lost, // making the MANIFEST invisible after recovery. - dir, err := os.Open(filepath.Dir(path)) - if err != nil { - _ = file.Close() - return nil, fmt.Errorf("beachdb/manifest: failed to open parent dir: %w", err) - } - defer dir.Close() - if err := dir.Sync(); err != nil { + if err := fs.SyncDir(filepath.Dir(path)); err != nil { _ = file.Close() return nil, fmt.Errorf("beachdb/manifest: failed to sync parent dir: %w", err) } From 2eebad2bd1fd9a0605a259ba382487be3ba29645 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 25 May 2026 22:19:06 +0200 Subject: [PATCH 12/26] Implement Reader for MANIFEST record stream replay --- internal/manifest/errors.go | 10 + internal/manifest/reader.go | 145 +++++++++ internal/manifest/reader_test.go | 522 +++++++++++++++++++++++++++++++ 3 files changed, 677 insertions(+) diff --git a/internal/manifest/errors.go b/internal/manifest/errors.go index b0bd709..e66d99a 100644 --- a/internal/manifest/errors.go +++ b/internal/manifest/errors.go @@ -22,4 +22,14 @@ var ( // ErrReaderClosed indicates when the manifest reader is closed. ErrReaderClosed = errors.New("beachdb/manifest: reader is closed") + + // ErrNoCurrentFile indicates that the CURRENT pointer file does not + // exist in the database directory. Callers (e.g. DB.Open) treat this + // as a signal that the database is fresh and proceed with bootstrap. + ErrNoCurrentFile = errors.New("beachdb/manifest: CURRENT file does not exist") + + // ErrInvalidManifestName indicates that the manifest filename passed + // to WriteCurrent is empty or contains a path separator. CURRENT must + // hold a bare filename, not a path. + ErrInvalidManifestName = errors.New("beachdb/manifest: invalid manifest filename") ) diff --git a/internal/manifest/reader.go b/internal/manifest/reader.go index 88367b0..792872e 100644 --- a/internal/manifest/reader.go +++ b/internal/manifest/reader.go @@ -1 +1,146 @@ package manifest + +import ( + "bufio" + "errors" + "fmt" + "io" + "os" + "sync" + + "github.com/aalhour/beachdb/internal/record" +) + +// Reader provides sequential read access to a MANIFEST record stream. +type Reader struct { + // Single-reader per replay session. Mutex defends against accidental + // concurrent use. + mu sync.Mutex + + // Open file handle for the MANIFEST on disk. + file *os.File + + // Buffered reader sitting on file. + buf *bufio.Reader + + // Byte offset immediately after the last fully validated record. + // On a clean stream, equals total bytes consumed by Next calls. + // On a truncated tail, points at the last good boundary — useful for + // recovery (e.g. to truncate the file back to the last valid record). + pos int64 +} + +// NewReader creates a new Reader for the MANIFEST file at the given path. +func NewReader(path string) (*Reader, error) { + //nolint:gosec // G304: path is controlled by the engine, not user input + file, err := os.OpenFile(path, os.O_RDONLY, 0644) + if err != nil { + return nil, fmt.Errorf("beachdb/manifest: failed to open file: %w", err) + } + + reader := &Reader{ + file: file, + buf: bufio.NewReader(file), + } + + return reader, nil +} + +// Next reads and returns the next MANIFEST record payload (the encoded +// VersionEdit body). Returns io.EOF cleanly at end of stream. A header or +// payload short-read returns record.ErrTruncated; callers may treat a +// trailing truncation as expected (crash during write) or as corruption +// depending on policy. +func (r *Reader) Next() ([]byte, error) { + r.mu.Lock() + defer r.mu.Unlock() + + if r.file == nil { + return nil, ErrReaderClosed + } + + header := make([]byte, record.HeaderSize) + n, err := io.ReadFull(r.buf, header) + + if errors.Is(err, io.EOF) { + // Clean EOF at a record boundary. + return nil, io.EOF + } + if errors.Is(err, io.ErrUnexpectedEOF) || n < record.HeaderSize { + return nil, record.ErrTruncated + } + if err != nil { + return nil, fmt.Errorf("beachdb/manifest: failed to read header: %w", err) + } + + payloadLen, checksum, err := DecodeRecordHeader(header) + if err != nil { + return nil, err + } + + payload := make([]byte, int(payloadLen)) + n, err = io.ReadFull(r.buf, payload) + if errors.Is(err, io.ErrUnexpectedEOF) || n < int(payloadLen) { + return nil, record.ErrTruncated + } + if err != nil { + return nil, fmt.Errorf("beachdb/manifest: failed to read payload: %w", err) + } + + if err := ValidateRecord(payload, checksum); err != nil { + return nil, err + } + + r.pos += int64(record.HeaderSize) + int64(payloadLen) + + return payload, nil +} + +// NextEdit reads the next record and decodes it as a VersionEdit. Convenience +// wrapper over Next + DecodeVersionEdit. Returns io.EOF at end of stream. +func (r *Reader) NextEdit() (*VersionEdit, error) { + payload, err := r.Next() + if err != nil { + return nil, err + } + return DecodeVersionEdit(payload) +} + +// ValidOffset returns the byte offset immediately after the last fully +// validated record. Replay code can truncate the file to this offset to +// drop a partial trailing record after a crash. +func (r *Reader) ValidOffset() int64 { + if r == nil { + return 0 + } + + r.mu.Lock() + defer r.mu.Unlock() + + return r.pos +} + +// Close closes the reader and releases associated resources. +func (r *Reader) Close() error { + if r == nil { + return ErrReaderClosed + } + + r.mu.Lock() + defer r.mu.Unlock() + + if r.file == nil { + return ErrReaderClosed + } + + err := r.file.Close() + + // Mark as closed even if Close returned an error. + r.buf = nil + r.file = nil + + if err != nil { + return fmt.Errorf("beachdb/manifest: failed to close file: %w", err) + } + return nil +} diff --git a/internal/manifest/reader_test.go b/internal/manifest/reader_test.go index 88367b0..d9f82f3 100644 --- a/internal/manifest/reader_test.go +++ b/internal/manifest/reader_test.go @@ -1 +1,523 @@ package manifest + +import ( + "bytes" + "errors" + "io" + "os" + "path/filepath" + "testing" + + "github.com/aalhour/beachdb/internal/keys" + "github.com/aalhour/beachdb/internal/record" +) + +// writeFile is a small helper that writes the given bytes to path. Used to +// build hand-crafted MANIFEST files for error-path tests where the Writer +// won't help (e.g. corrupted checksum, bad magic). +func writeFile(t *testing.T, path string, data []byte) { + t.Helper() + if err := os.WriteFile(path, data, 0600); err != nil { + t.Fatalf("write file: %v", err) + } +} + +// writeRecords drives a real Writer to produce a MANIFEST containing the +// given payloads, then closes it. Returns the file path. Reusing the Writer +// for the happy-path fixtures keeps reader+writer aligned on the framing. +func writeRecords(t *testing.T, payloads ...[]byte) string { + t.Helper() + path := tempManifestPath(t) + w, err := NewWriter(path) + if err != nil { + t.Fatalf("NewWriter: %v", err) + } + for _, p := range payloads { + if err := w.Append(p); err != nil { + t.Fatalf("Append: %v", err) + } + } + if err := w.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + return path +} + +func TestNewReader(t *testing.T) { + t.Run("opens existing file", func(t *testing.T) { + path := writeRecords(t, []byte("hello")) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + }) + + t.Run("errors on non-existent file", func(t *testing.T) { + _, err := NewReader(filepath.Join(t.TempDir(), "does-not-exist")) + if err == nil { + t.Fatal("expected error for non-existent file") + } + }) +} + +func TestReader_Next(t *testing.T) { + t.Run("single record roundtrip", func(t *testing.T) { + path := writeRecords(t, []byte("hello manifest")) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + got, err := r.Next() + if err != nil { + t.Fatalf("Next: %v", err) + } + if !bytes.Equal(got, []byte("hello manifest")) { + t.Errorf("payload = %q, want %q", got, "hello manifest") + } + + _, err = r.Next() + if !errors.Is(err, io.EOF) { + t.Errorf("second Next: got %v, want io.EOF", err) + } + }) + + t.Run("multi-record stream returns each payload in order", func(t *testing.T) { + want := [][]byte{ + []byte("alpha"), + []byte("bravo"), + []byte("charlie"), + } + path := writeRecords(t, want...) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + for i, w := range want { + got, err := r.Next() + if err != nil { + t.Fatalf("Next[%d]: %v", i, err) + } + if !bytes.Equal(got, w) { + t.Errorf("Next[%d] = %q, want %q", i, got, w) + } + } + + _, err = r.Next() + if !errors.Is(err, io.EOF) { + t.Errorf("trailing Next: got %v, want io.EOF", err) + } + }) + + t.Run("empty file returns io.EOF immediately", func(t *testing.T) { + path := writeRecords(t) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + _, err = r.Next() + if !errors.Is(err, io.EOF) { + t.Errorf("got %v, want io.EOF", err) + } + }) + + t.Run("empty payload roundtrips", func(t *testing.T) { + path := writeRecords(t, []byte{}) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + got, err := r.Next() + if err != nil { + t.Fatalf("Next: %v", err) + } + if len(got) != 0 { + t.Errorf("payload len = %d, want 0", len(got)) + } + }) + + t.Run("truncated header returns record.ErrTruncated", func(t *testing.T) { + full, err := EncodeRecord([]byte("hello")) + if err != nil { + t.Fatalf("EncodeRecord: %v", err) + } + // Truncate to mid-header. + path := tempManifestPath(t) + writeFile(t, path, full[:record.HeaderSize-1]) + + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + _, err = r.Next() + if !errors.Is(err, record.ErrTruncated) { + t.Errorf("got %v, want record.ErrTruncated", err) + } + }) + + t.Run("truncated payload returns record.ErrTruncated", func(t *testing.T) { + full, err := EncodeRecord([]byte("hello manifest")) + if err != nil { + t.Fatalf("EncodeRecord: %v", err) + } + // Keep full header, drop last few payload bytes. + path := tempManifestPath(t) + writeFile(t, path, full[:len(full)-3]) + + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + _, err = r.Next() + if !errors.Is(err, record.ErrTruncated) { + t.Errorf("got %v, want record.ErrTruncated", err) + } + }) + + t.Run("corrupted payload returns record.ErrChecksum", func(t *testing.T) { + full, err := EncodeRecord([]byte("hello manifest")) + if err != nil { + t.Fatalf("EncodeRecord: %v", err) + } + // Flip a byte in the payload (after the header). + corrupted := append([]byte(nil), full...) + corrupted[record.HeaderSize] ^= 0xFF + + path := tempManifestPath(t) + writeFile(t, path, corrupted) + + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + _, err = r.Next() + if !errors.Is(err, record.ErrChecksum) { + t.Errorf("got %v, want record.ErrChecksum", err) + } + }) + + t.Run("bad magic returns record.ErrBadMagic", func(t *testing.T) { + full, err := EncodeRecord([]byte("hello manifest")) + if err != nil { + t.Fatalf("EncodeRecord: %v", err) + } + // Stomp the first magic byte. + corrupted := append([]byte(nil), full...) + corrupted[0] ^= 0xFF + + path := tempManifestPath(t) + writeFile(t, path, corrupted) + + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + _, err = r.Next() + if !errors.Is(err, record.ErrBadMagic) { + t.Errorf("got %v, want record.ErrBadMagic", err) + } + }) + + t.Run("Next after Close returns ErrReaderClosed", func(t *testing.T) { + path := writeRecords(t, []byte("hello")) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + if err := r.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + _, err = r.Next() + if !errors.Is(err, ErrReaderClosed) { + t.Errorf("got %v, want ErrReaderClosed", err) + } + }) +} + +func TestReader_NextEdit(t *testing.T) { + t.Run("decodes a valid VersionEdit", func(t *testing.T) { + want := &VersionEdit{ + HasNextFileID: true, NextFileID: 42, + HasLastSequence: true, LastSequence: 100, + AddedFiles: []FileMetadata{ + fileMeta(0, 7, 1024, putKey("a", 1), putKey("z", 2)), + }, + } + path := writeRecords(t, want.Encode()) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + got, err := r.NextEdit() + if err != nil { + t.Fatalf("NextEdit: %v", err) + } + assertEditsEqual(t, got, want) + }) + + t.Run("propagates VersionEdit decode errors", func(t *testing.T) { + // Encode a record whose payload is a single unknown tag byte. The + // framing layer passes it through (valid record), but + // DecodeVersionEdit rejects with ErrUnknownTag. + path := writeRecords(t, []byte{0xFF}) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + _, err = r.NextEdit() + if !errors.Is(err, ErrUnknownTag) { + t.Errorf("got %v, want ErrUnknownTag", err) + } + }) + + t.Run("returns io.EOF at end of stream", func(t *testing.T) { + path := writeRecords(t) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + _, err = r.NextEdit() + if !errors.Is(err, io.EOF) { + t.Errorf("got %v, want io.EOF", err) + } + }) +} + +func TestReader_ValidOffset(t *testing.T) { + t.Run("advances by header+payload per successful Next", func(t *testing.T) { + payloads := [][]byte{ + []byte("alpha"), + []byte("bravo"), + } + path := writeRecords(t, payloads...) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + if got := r.ValidOffset(); got != 0 { + t.Errorf("initial ValidOffset = %d, want 0", got) + } + + expected := int64(0) + for i, p := range payloads { + if _, err := r.Next(); err != nil { + t.Fatalf("Next[%d]: %v", i, err) + } + expected += int64(record.HeaderSize) + int64(len(p)) + if got := r.ValidOffset(); got != expected { + t.Errorf("ValidOffset after Next[%d] = %d, want %d", i, got, expected) + } + } + }) + + t.Run("does not advance on truncated read", func(t *testing.T) { + // Build [valid record][truncated record]. + valid, err := EncodeRecord([]byte("alpha")) + if err != nil { + t.Fatalf("EncodeRecord: %v", err) + } + truncated, err := EncodeRecord([]byte("bravo-payload-bytes")) + if err != nil { + t.Fatalf("EncodeRecord: %v", err) + } + stream := append([]byte(nil), valid...) + stream = append(stream, truncated[:len(truncated)-2]...) + + path := tempManifestPath(t) + writeFile(t, path, stream) + + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + if _, err := r.Next(); err != nil { + t.Fatalf("first Next: %v", err) + } + afterFirst := r.ValidOffset() + if afterFirst != int64(len(valid)) { + t.Errorf("ValidOffset after valid record = %d, want %d", afterFirst, len(valid)) + } + + _, err = r.Next() + if !errors.Is(err, record.ErrTruncated) { + t.Fatalf("second Next: got %v, want record.ErrTruncated", err) + } + if got := r.ValidOffset(); got != afterFirst { + t.Errorf("ValidOffset after truncated read = %d, want unchanged %d", got, afterFirst) + } + }) + + t.Run("nil receiver returns zero without panic", func(t *testing.T) { + var r *Reader + if got := r.ValidOffset(); got != 0 { + t.Errorf("got %d, want 0", got) + } + }) +} + +func TestReader_Close(t *testing.T) { + t.Run("happy path", func(t *testing.T) { + path := writeRecords(t, []byte("x")) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + if err := r.Close(); err != nil { + t.Errorf("Close: %v", err) + } + }) + + t.Run("double Close returns ErrReaderClosed", func(t *testing.T) { + path := writeRecords(t, []byte("x")) + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + if err := r.Close(); err != nil { + t.Fatalf("first Close: %v", err) + } + err = r.Close() + if !errors.Is(err, ErrReaderClosed) { + t.Errorf("got %v, want ErrReaderClosed", err) + } + }) + + t.Run("nil receiver returns ErrReaderClosed without panic", func(t *testing.T) { + var r *Reader + err := r.Close() + if !errors.Is(err, ErrReaderClosed) { + t.Errorf("got %v, want ErrReaderClosed", err) + } + }) +} + +// End-to-end: write a stream of varied VersionEdits via the Writer, read +// each back via the Reader+NextEdit, and assert byte-for-byte equality. +// Locks down the Writer/Reader framing contract. +func TestWriterReader_RoundTrip(t *testing.T) { + edits := []*VersionEdit{ + {HasNextFileID: true, NextFileID: 1}, + { + HasLastSequence: true, LastSequence: 42, + DeletedFiles: []DeletedFile{{Level: 1, FileID: 9}}, + }, + { + AddedFiles: []FileMetadata{ + fileMeta(0, 7, 1024, putKey("a", 1), putKey("z", 2)), + fileMeta(1, 8, 2048, putKey("aa", 3), putKey("zz", 4)), + }, + }, + { + HasNextFileID: true, NextFileID: 100, + HasLastSequence: true, LastSequence: 200, + HasLogNumber: true, LogNumber: 5, + DeletedFiles: []DeletedFile{{Level: 0, FileID: 1}}, + AddedFiles: []FileMetadata{ + fileMeta(2, 99, 4096, + keys.InternalKey{UserKey: []byte("k"), Seqno: 1, Kind: keys.InternalKeyKindPut}, + keys.InternalKey{UserKey: []byte("z"), Seqno: 2, Kind: keys.InternalKeyKindPut}, + ), + }, + }, + } + + // Encode each via the Writer. + payloads := make([][]byte, len(edits)) + for i, e := range edits { + payloads[i] = e.Encode() + } + path := writeRecords(t, payloads...) + + r, err := NewReader(path) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + for i, want := range edits { + got, err := r.NextEdit() + if err != nil { + t.Fatalf("NextEdit[%d]: %v", i, err) + } + assertEditsEqual(t, got, want) + } + + if _, err := r.NextEdit(); !errors.Is(err, io.EOF) { + t.Errorf("trailing NextEdit: got %v, want io.EOF", err) + } +} + +// --- Benchmarks --- + +// BenchmarkReader_Next measures per-record read cost across payload sizes. +// Each iteration reads one record's header + payload and validates the +// checksum. b.ReportAllocs() exposes allocator churn in the read path — +// expect 2 allocs per Next (header slice + payload slice). +// +// The fixture is built once with raw EncodeRecord + os.WriteFile (skipping +// the Writer's per-record fsync, which would dominate setup cost) and reused +// across all iterations by re-opening a fresh Reader per iteration. Reader +// construction is included in the timer; it's a fixed cost that real callers +// pay once per replay session. +func BenchmarkReader_Next(b *testing.B) { + sizes := []struct { + name string + size int + }{ + {"small-64B", 64}, + {"medium-1KB", 1024}, + {"large-64KB", 64 * 1024}, + } + for _, s := range sizes { + b.Run(s.name, func(b *testing.B) { + payload := make([]byte, s.size) + rec, err := EncodeRecord(payload) + if err != nil { + b.Fatal(err) + } + path := filepath.Join(b.TempDir(), "bench.manifest") + if err := os.WriteFile(path, rec, 0600); err != nil { + b.Fatal(err) + } + + b.ReportAllocs() + b.ResetTimer() + for range b.N { + r, err := NewReader(path) + if err != nil { + b.Fatalf("NewReader: %v", err) + } + if _, err := r.Next(); err != nil { + b.Fatalf("Next: %v", err) + } + _ = r.Close() + } + }) + } +} From 87cab23e37b8b20b0c01f6d48715d515e3a55b54 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Mon, 25 May 2026 22:19:30 +0200 Subject: [PATCH 13/26] Implement implement CURRENT pointer file for manifest --- internal/manifest/current.go | 119 +++++++++++++++++++ internal/manifest/current_test.go | 183 ++++++++++++++++++++++++++++++ 2 files changed, 302 insertions(+) diff --git a/internal/manifest/current.go b/internal/manifest/current.go index 88367b0..fddcdae 100644 --- a/internal/manifest/current.go +++ b/internal/manifest/current.go @@ -1 +1,120 @@ package manifest + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/aalhour/beachdb/internal/fs" +) + +const ( + // currentFileName is the bare filename of the CURRENT pointer file + // inside the database directory. + currentFileName = "CURRENT" + + // currentTmpFileName is the staging filename used by WriteCurrent. The + // final installation step renames it to currentFileName atomically. + currentTmpFileName = "CURRENT.tmp" +) + +// ReadCurrent reads the CURRENT pointer file in dir and returns the live +// manifest filename (no directory prefix). Returns ErrNoCurrentFile when +// the CURRENT file does not exist — callers should treat that as a fresh +// database signal, not a hard error. +func ReadCurrent(dir string) (string, error) { + path := filepath.Join(dir, currentFileName) + //nolint:gosec // G304: path is constructed from the trusted DB directory + data, err := os.ReadFile(path) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return "", ErrNoCurrentFile + } + return "", fmt.Errorf("beachdb/manifest: failed to read CURRENT: %w", err) + } + + name := strings.TrimSpace(string(data)) + if name == "" { + return "", fmt.Errorf("beachdb/manifest: CURRENT is empty: %w", ErrInvalidManifestName) + } + return name, nil +} + +// WriteCurrent installs manifestName as the active manifest pointer in dir +// atomically: write to a temp file, fsync the temp file, rename it onto +// CURRENT, then fsync the parent directory so the rename is durable. +// +// manifestName must be a bare filename (no path separators); WriteCurrent +// validates this and returns ErrInvalidManifestName on misuse. +// +// A crash at any step leaves either the old CURRENT intact or the new +// CURRENT fully written — never a partial CURRENT. This is the same +// rename-into-place pattern used for SSTable installation. +func WriteCurrent(dir, manifestName string) error { + if err := validateManifestName(manifestName); err != nil { + return err + } + + tmpPath := filepath.Join(dir, currentTmpFileName) + finalPath := filepath.Join(dir, currentFileName) + + // Write "MANIFEST-NNNNNN\n" to CURRENT.tmp. + //nolint:gosec // G304: tmpPath is constructed from the trusted DB directory + tmp, err := os.OpenFile(tmpPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0644) + if err != nil { + return fmt.Errorf("beachdb/manifest: failed to open CURRENT.tmp: %w", err) + } + + if _, err := tmp.WriteString(manifestName + "\n"); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpPath) + return fmt.Errorf("beachdb/manifest: failed to write CURRENT.tmp: %w", err) + } + + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpPath) + return fmt.Errorf("beachdb/manifest: failed to sync CURRENT.tmp: %w", err) + } + + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("beachdb/manifest: failed to close CURRENT.tmp: %w", err) + } + + // Atomic rename — POSIX guarantees the dirent flips from old to new + // in a single step. + if err := os.Rename(tmpPath, finalPath); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("beachdb/manifest: failed to rename CURRENT.tmp -> CURRENT: %w", err) + } + + // Fsync the parent directory so the rename's dirent change is durable + // on disk. Without this, a crash can leave the rename visible to the + // running process but invisible after recovery. + if err := fs.SyncDir(dir); err != nil { + return fmt.Errorf("beachdb/manifest: failed to sync parent dir after CURRENT rename: %w", err) + } + + return nil +} + +// validateManifestName rejects empty names and any name containing a path +// separator. CURRENT must hold a bare filename so its interpretation is +// unambiguous and confined to the database directory. +func validateManifestName(name string) error { + if name == "" { + return fmt.Errorf("%w: empty", ErrInvalidManifestName) + } + if strings.ContainsRune(name, os.PathSeparator) { + return fmt.Errorf("%w: contains path separator %q", ErrInvalidManifestName, name) + } + // Also reject literal '/' on platforms where PathSeparator is something + // else (e.g. Windows uses '\\'); CURRENT should never contain either. + if strings.ContainsRune(name, '/') { + return fmt.Errorf("%w: contains '/' %q", ErrInvalidManifestName, name) + } + return nil +} diff --git a/internal/manifest/current_test.go b/internal/manifest/current_test.go index 88367b0..d03d591 100644 --- a/internal/manifest/current_test.go +++ b/internal/manifest/current_test.go @@ -1 +1,184 @@ package manifest + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +func TestCurrent_WriteRead(t *testing.T) { + dir := t.TempDir() + const name = "MANIFEST-000001" + + if err := WriteCurrent(dir, name); err != nil { + t.Fatalf("WriteCurrent: %v", err) + } + + got, err := ReadCurrent(dir) + if err != nil { + t.Fatalf("ReadCurrent: %v", err) + } + if got != name { + t.Errorf("ReadCurrent = %q, want %q", got, name) + } +} + +func TestCurrent_Overwrite(t *testing.T) { + dir := t.TempDir() + + if err := WriteCurrent(dir, "MANIFEST-000001"); err != nil { + t.Fatalf("first WriteCurrent: %v", err) + } + if err := WriteCurrent(dir, "MANIFEST-000002"); err != nil { + t.Fatalf("second WriteCurrent: %v", err) + } + + got, err := ReadCurrent(dir) + if err != nil { + t.Fatalf("ReadCurrent: %v", err) + } + if got != "MANIFEST-000002" { + t.Errorf("ReadCurrent = %q, want second value", got) + } +} + +func TestCurrent_Missing(t *testing.T) { + dir := t.TempDir() + + _, err := ReadCurrent(dir) + if !errors.Is(err, ErrNoCurrentFile) { + t.Errorf("ReadCurrent on empty dir: got %v, want ErrNoCurrentFile", err) + } +} + +func TestCurrent_OnDiskContents(t *testing.T) { + // The file is human-readable: exactly "MANIFEST-NNNNNN\n", nothing else. + // Locks down the wire format defined in docs/formats/manifest.md. + dir := t.TempDir() + const name = "MANIFEST-000042" + + if err := WriteCurrent(dir, name); err != nil { + t.Fatalf("WriteCurrent: %v", err) + } + + //nolint:gosec // G304: path is t.TempDir() + got, err := os.ReadFile(filepath.Join(dir, currentFileName)) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + want := name + "\n" + if string(got) != want { + t.Errorf("CURRENT contents = %q, want %q", got, want) + } +} + +func TestCurrent_NoTmpFileLeftBehind(t *testing.T) { + // After a successful WriteCurrent, CURRENT.tmp must not linger in the + // directory — the rename step consumes it. + dir := t.TempDir() + if err := WriteCurrent(dir, "MANIFEST-000001"); err != nil { + t.Fatalf("WriteCurrent: %v", err) + } + + tmpPath := filepath.Join(dir, currentTmpFileName) + if _, err := os.Stat(tmpPath); !errors.Is(err, os.ErrNotExist) { + t.Errorf("CURRENT.tmp still exists after WriteCurrent: %v", err) + } +} + +func TestCurrent_WriteCurrent_InvalidName(t *testing.T) { + dir := t.TempDir() + cases := []struct { + name string + input string + }{ + {"empty", ""}, + {"contains forward slash", "subdir/MANIFEST-000001"}, + {"absolute path", "/etc/passwd"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := WriteCurrent(dir, tc.input) + if !errors.Is(err, ErrInvalidManifestName) { + t.Errorf("got %v, want ErrInvalidManifestName", err) + } + }) + } +} + +func TestCurrent_ReadCurrent_TrimsWhitespace(t *testing.T) { + // Spec says exactly "\n" terminated, but recovery is lenient: trim any + // surrounding whitespace. Catches sloppy hand-written CURRENT files + // from debugging tools without misreading the manifest name. + dir := t.TempDir() + //nolint:gosec // G306: 0600 fine for test fixture + if err := os.WriteFile(filepath.Join(dir, currentFileName), + []byte(" MANIFEST-000007\n\n"), 0600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + got, err := ReadCurrent(dir) + if err != nil { + t.Fatalf("ReadCurrent: %v", err) + } + if got != "MANIFEST-000007" { + t.Errorf("ReadCurrent = %q, want %q", got, "MANIFEST-000007") + } +} + +func TestCurrent_ReadCurrent_Empty(t *testing.T) { + // CURRENT exists but is empty (corruption / mid-write race that + // shouldn't be reachable via WriteCurrent but worth defending). + dir := t.TempDir() + //nolint:gosec // G306: 0600 fine for test fixture + if err := os.WriteFile(filepath.Join(dir, currentFileName), []byte(""), 0600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + _, err := ReadCurrent(dir) + if !errors.Is(err, ErrInvalidManifestName) { + t.Errorf("got %v, want ErrInvalidManifestName", err) + } +} + +func TestCurrent_WriteCurrent_BadDir(t *testing.T) { + // Writing into a non-existent directory should fail at OpenFile. + _, err := os.Stat("/nonexistent-beachdb-dir") + if err == nil { + t.Skip("/nonexistent-beachdb-dir unexpectedly exists") + } + if err := WriteCurrent("/nonexistent-beachdb-dir", "MANIFEST-000001"); err == nil { + t.Error("expected error writing CURRENT into non-existent dir") + } +} + +// --- Benchmarks --- + +// BenchmarkWriteCurrent measures the cost of a single CURRENT install: +// open temp, write, fsync, rename, sync parent dir. Dominated by the two +// fsyncs. Allocation count is the focus; wall time is fsync latency. +func BenchmarkWriteCurrent(b *testing.B) { + dir := b.TempDir() + b.ReportAllocs() + b.ResetTimer() + for range b.N { + if err := WriteCurrent(dir, "MANIFEST-000001"); err != nil { + b.Fatalf("WriteCurrent: %v", err) + } + } +} + +func BenchmarkReadCurrent(b *testing.B) { + dir := b.TempDir() + if err := WriteCurrent(dir, "MANIFEST-000001"); err != nil { + b.Fatal(err) + } + b.ReportAllocs() + b.ResetTimer() + for range b.N { + if _, err := ReadCurrent(dir); err != nil { + b.Fatalf("ReadCurrent: %v", err) + } + } +} From 473720d9da7226159f0bbce7f5743dc776124606 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Tue, 26 May 2026 00:10:55 +0200 Subject: [PATCH 14/26] fix(engine,manifest): close manifest correctness gaps surfaced by replay tests --- engine/db.go | 456 +++++++++++++++++++++++++---------- internal/manifest/current.go | 8 + internal/manifest/doc.go | 133 +++++++++- 3 files changed, 473 insertions(+), 124 deletions(-) diff --git a/engine/db.go b/engine/db.go index edecb04..6e788d3 100644 --- a/engine/db.go +++ b/engine/db.go @@ -15,6 +15,7 @@ import ( "github.com/aalhour/beachdb/internal/crashhook" "github.com/aalhour/beachdb/internal/fs" "github.com/aalhour/beachdb/internal/keys" + "github.com/aalhour/beachdb/internal/manifest" "github.com/aalhour/beachdb/internal/memtable" "github.com/aalhour/beachdb/internal/record" "github.com/aalhour/beachdb/internal/sstable" @@ -30,6 +31,12 @@ const ( // sstableFileIDWidth keeps lexicographic and numeric order aligned for all uint64 IDs. sstableFileIDWidth = 20 + + // manifestFilePrefix specifies the file name prefix for MANIFEST files. + manifestFilePrefix = "MANIFEST-" + + // manifestFileIDWidth keeps lexicographic and numeric order aligned for all uint64 IDs. + manifestFileIDWidth = 6 ) var ( @@ -57,12 +64,15 @@ type DB struct { cond *sync.Cond // cond.L = &db.mu (write side only) // Mutable state (guarded by `mu`) - closed bool // Flag indicating whether db is closed or not - seqno uint64 // Monotonic sequence counter - mem memtable.Memtable // Memory table with recent writes - immMem memtable.Memtable // frozen memtable being flushed; nil when idle - ssts []*sstable.Reader // Open SSTable readers, newest-last in slice - wal *wal.Writer // Writer for the Write-Ahead Log (WAL) file + closed bool // Flag indicating whether db is closed or not + seqno uint64 // Monotonic sequence counter + logno uint64 // TODO: new field + mem memtable.Memtable // Memory table with recent writes + immMem memtable.Memtable // frozen memtable being flushed; nil when idle + ssts []*sstable.Reader // Open SSTable readers, newest-last in slice + wal *wal.Writer // Writer for the Write-Ahead Log (WAL) file + manifest *manifest.Writer // Active manifest writer + version *manifest.Version // Current in-memory version data (ssts metadata) // SSTable flush goroutine state nextSSTID uint64 // Counter for SST file naming (new files) @@ -70,93 +80,54 @@ type DB struct { flushErr error // Last flush error } -// Open initializes a DB struct and replays the WAL, if present. +// Open initializes a DB struct, restoring state from MANIFEST + CURRENT +// and replaying any WAL records written after the last flush. A directory +// with no CURRENT is treated as a fresh database and bootstrapped from +// scratch (see openManifest for the full decision tree). func Open(dir string, opts ...Option) (*DB, error) { - // Process configuration options cfg := applyOptions(opts) - // Create directory for the database and fsync created directory entries. if err := fs.MkdirAllAndSync(dir); err != nil { return nil, err } - - // Validate the memtable flush threshold if cfg.memtableFlushSize < 0 { return nil, ErrInvalidMemtableFlushSize } - - // Validate the SSTable block size if cfg.sstBlockSize < 0 { return nil, ErrInvalidSSTBlockSize } - // Initialize the DB struct db := &DB{ dir: dir, syncOnWrite: cfg.syncOnWrite, memtableFlushSize: cfg.memtableFlushSize, sstBlockSize: cfg.sstBlockSize, - closed: false, mem: memtable.NewSkipList(), - seqno: 0, } - - // Init the concurrency `cond` field for write workloads db.cond = sync.NewCond(&db.mu) - // Construct the WAL file path - walFilePath := filepath.Join(dir, walFileName) - - // Replay the WAL if it exists - err := replayWAL(db, walFilePath) - if err != nil { + // Read CURRENT, replay manifest (or bootstrap fresh). On return, + // db.version, db.manifest, db.ssts, db.nextSSTID, and db.seqno + // are all installed from on-disk state. + if err := openManifest(db); err != nil { return nil, err } - // Create the WAL writer - writer, err := wal.NewWriter(walFilePath) - if err != nil { - return nil, fmt.Errorf("beachdb: creating WAL writer: %w", err) - } - db.wal = writer + walFilePath := filepath.Join(dir, walFileName) - // Sync the directory so the WAL file's directory entry reaches disk. - // Without this, a crash could leave the WAL data on disk but the - // directory unaware the file exists. - if err := fs.SyncDir(dir); err != nil { - // Best effort: close the writer before returning - _ = writer.Close() + // WAL replay continues from db.seqno (the manifest's LastSequence). + if err := replayWAL(db, walFilePath); err != nil { + _ = db.Close() return nil, err } - // Discover SSTables and create readers for them - sortedFileNames, nextSSTID, err := discoverSSTables(dir) + walWriter, err := wal.NewWriter(walFilePath) if err != nil { _ = db.Close() - return nil, fmt.Errorf("beachdb: error discovering SSTables, %w", err) - } - - // Iterate over discovered sstable files and open readers for them - for _, fileName := range sortedFileNames { - fullPath := filepath.Join(dir, fileName) - sstableFile, err := os.Open(fullPath) //nolint:gosec // trusted dir + discovered filename - if err != nil { - _ = db.Close() - return nil, fmt.Errorf("beachdb: opening SSTable %s: %w", fileName, err) - } - sstReader, err := sstable.OpenReader(sstableFile) - if err != nil { - _ = sstableFile.Close() - _ = db.Close() - return nil, fmt.Errorf("beachdb: reading SSTable %s: %w", fileName, err) - } - db.ssts = append(db.ssts, sstReader) + return nil, fmt.Errorf("beachdb: creating WAL writer: %w", err) } + db.wal = walWriter - // Set the nextSSTID to point to the next free ID number - db.nextSSTID = nextSSTID - - // Start the SSTable flushing goroutine only after Open succeeds. if db.memtableFlushSize > 0 { doneCh := make(chan struct{}) db.flushDoneCh = doneCh @@ -337,11 +308,20 @@ func (db *DB) Close() error { firstError = err } + // Close the manifest file writer + if db.manifest != nil { + if err := db.manifest.Close(); err != nil && firstError == nil { + firstError = err + } + db.manifest = nil + } + // Mark it as closed db.wal = nil db.ssts = nil db.mem = nil db.immMem = nil + db.manifest = nil if firstError != nil { return fmt.Errorf("beachdb: error closing DB: %w", firstError) @@ -676,6 +656,272 @@ func (db *DB) nextSSTPath() string { return filepath.Join(db.dir, buildSSTFileName(db.nextSSTID)) } +// openManifest reads the CURRENT pointer and dispatches to either the +// fresh-database bootstrap or the existing-manifest replay path. After +// it returns, db.version, db.manifest, db.nextSSTID, and db.seqno are +// installed and ready for use. +// +// Dispatch: +// +// CURRENT missing → bootstrapFreshManifest +// CURRENT exists but is empty → hard error +// CURRENT points at missing file → hard error +// manifest is corrupt mid-stream → hard error +// manifest tail truncated by crash → benign, replay tolerates and trims +// CURRENT + manifest both intact → replayExistingManifest +// +// "CURRENT missing" covers both the truly fresh directory and the case +// where a prior bootstrap crashed before installing CURRENT (leaving an +// orphan MANIFEST). CURRENT install is the final commit step of every +// manifest creation, so an orphan MANIFEST without a CURRENT pointing at +// it was never committed and the directory is effectively fresh. +func openManifest(db *DB) error { + name, err := manifest.ReadCurrent(db.dir) + switch { + case errors.Is(err, manifest.ErrNoCurrentFile): + return bootstrapFreshManifest(db) + case err != nil: + // CURRENT exists but is empty or otherwise unreadable. + return fmt.Errorf("beachdb: reading CURRENT: %w", err) + } + + return replayExistingManifest(db, name) +} + +// bootstrapFreshManifest initializes the on-disk manifest state for a +// brand-new database directory. It creates MANIFEST-000001, appends a +// single VersionEdit carrying the initial counters (nextFileID=1, +// lastSequence=0, logNumber=0), and atomically installs CURRENT so the +// database is officially committed before Open returns. +// +// The CURRENT install is the commit barrier: until WriteCurrent succeeds, +// nothing on disk points at the new manifest, so a crash anywhere before +// that step leaves the directory looking fresh again on the next Open. +// A crash *after* CURRENT install leaves a valid, replayable database. +func bootstrapFreshManifest(db *DB) error { + version := manifest.NewVersion(0) + + nextFileID := uint64(1) + seqno := uint64(0) + logno := uint64(0) + + // Pick the next-available MANIFEST ID rather than hardcoding 1. A prior + // bootstrap may have crashed between manifest-create and CURRENT-install, + // leaving an orphan MANIFEST-NNNNNN. Reusing that name with O_APPEND + // would write our initial edit on top of the orphan's garbage bytes and + // corrupt the manifest stream on the next Open. + manifestID, err := nextAvailableManifestID(db.dir) + if err != nil { + return err + } + newManifestFileName := buildManifestFileName(manifestID) + newManifestFilePath := filepath.Join(db.dir, newManifestFileName) + manifestWriter, err := manifest.NewWriter(newManifestFilePath) + if err != nil { + return fmt.Errorf("beachdb: creating initial manifest: %w", err) + } + + // Encode the initial counters. Has* flags must be true or Encode will + // silently drop the fields and the on-disk record will be header-only. + versionEdit := manifest.VersionEdit{ + HasNextFileID: true, + NextFileID: nextFileID, + HasLastSequence: true, + LastSequence: seqno, + HasLogNumber: true, + LogNumber: logno, + } + + // Write the version edit to disk. On failure, close the writer and + // remove the partial manifest so the directory still looks fresh on + // the next Open. + if err := manifestWriter.Append(versionEdit.Encode()); err != nil { + _ = manifestWriter.Close() + _ = os.Remove(newManifestFilePath) + return fmt.Errorf("beachdb: writing initial manifest edit: %w", err) + } + + // Apply to in-memory Version. Apply returns a new Version — assigning + // the result is required, otherwise the change is dropped. + version = version.Apply(&versionEdit) + + // Install CURRENT — the commit barrier. Until this succeeds, the + // directory still looks fresh on the next Open and the orphan + // MANIFEST-000001 will be picked up by a future bootstrap. + if err := manifest.WriteCurrent(db.dir, newManifestFileName); err != nil { + _ = manifestWriter.Close() + _ = os.Remove(newManifestFilePath) + return fmt.Errorf("beachdb: installing CURRENT: %w", err) + } + + db.version = version + db.manifest = manifestWriter + db.seqno = seqno + db.nextSSTID = nextFileID + + return nil +} + +// replayExistingManifest opens the manifest named by CURRENT and replays +// every VersionEdit into a fresh Version, accumulating the file-number, +// sequence, and log-number counters along the way. After replay it opens +// readers for every SSTable referenced by the final Version and installs +// everything on db. +// +// Error policy follows the bootstrap decision tree: +// - manifest file does not exist (CURRENT points at a missing file) → +// hard error (corruption — the database promised this file existed) +// - mid-stream corruption (bad checksum, bad magic, unknown tag, +// truncated edit body, invalid InternalKey) → hard error +// - trailing record.ErrTruncated (crash during write) → benign: truncate +// the manifest file to the last validated boundary and continue +// - referenced SSTable missing on disk → hard error +// +// manifestReplayResult holds the accumulated state from a manifest replay. +type manifestReplayResult struct { + version *manifest.Version + nextFileID uint64 + seqno uint64 + logNumber uint64 + validOffset int64 + tailTrimmed bool +} + +// applyEditToReplay folds one edit into the running replay state. Returns +// an error if the edit re-adds a fileID already present in liveFiles +// (corrupt or engine-buggy manifest). +func applyEditToReplay( + edit *manifest.VersionEdit, + res *manifestReplayResult, + liveFiles map[uint64]struct{}, + current string, + sawNextFileID *bool, +) error { + for _, fm := range edit.AddedFiles { + if _, exists := liveFiles[fm.FileID]; exists { + return fmt.Errorf("beachdb: manifest %q: duplicate AddFile for fileID %d", current, fm.FileID) + } + liveFiles[fm.FileID] = struct{}{} + } + for _, d := range edit.DeletedFiles { + delete(liveFiles, d.FileID) + } + + res.version = res.version.Apply(edit) + if edit.HasNextFileID { + res.nextFileID = edit.NextFileID + *sawNextFileID = true + } + if edit.HasLastSequence { + res.seqno = edit.LastSequence + } + if edit.HasLogNumber { + res.logNumber = edit.LogNumber + } + return nil +} + +// replayManifestStream reads every record from reader, applies each +// VersionEdit to a fresh Version, and accumulates counters. Trailing +// record.ErrTruncated is treated as benign (sets tailTrimmed); any other +// non-EOF error is mid-stream corruption and surfaces as an error. +// Duplicate AddFile for the same fileID is corruption. +func replayManifestStream(reader *manifest.Reader, current string) (manifestReplayResult, error) { + res := manifestReplayResult{version: manifest.NewVersion(0)} + // liveFiles tracks fileIDs currently in the Version so we can detect + // duplicate AddFile edits at replay time. + liveFiles := make(map[uint64]struct{}) + var sawNextFileID bool + + for { + edit, err := reader.NextEdit() + if errors.Is(err, io.EOF) || errors.Is(err, record.ErrTruncated) { + res.validOffset = reader.ValidOffset() + res.tailTrimmed = errors.Is(err, record.ErrTruncated) + if !sawNextFileID { + return res, fmt.Errorf("beachdb: manifest %q has no NextFileID counter — corrupt", current) + } + return res, nil + } + if err != nil { + return res, fmt.Errorf("beachdb: replaying manifest %q: %w", current, err) + } + + if err := applyEditToReplay(edit, &res, liveFiles, current, &sawNextFileID); err != nil { + return res, err + } + } +} + +// openSSTReadersForVersion opens an *sstable.Reader for every file in the +// given Version, in level-ascending order. Returns a hard error if any +// referenced SSTable file is missing or unreadable. +func openSSTReadersForVersion(dir string, version *manifest.Version) ([]*sstable.Reader, error) { + readers := make([]*sstable.Reader, 0, len(version.AllFiles())) + for _, fm := range version.AllFiles() { + sstPath := filepath.Join(dir, buildSSTFileName(fm.FileID)) + //nolint:gosec // G304: path is built from the trusted DB directory + manifest-tracked file ID + sstFile, err := os.Open(sstPath) + if err != nil { + return readers, fmt.Errorf("beachdb: opening SSTable %q referenced by manifest: %w", sstPath, err) + } + sstReader, err := sstable.OpenReader(sstFile) + if err != nil { + _ = sstFile.Close() + return readers, fmt.Errorf("beachdb: reading SSTable %q: %w", sstPath, err) + } + readers = append(readers, sstReader) + } + return readers, nil +} + +func replayExistingManifest(db *DB, current string) error { + manifestPath := filepath.Join(db.dir, current) + + reader, err := manifest.NewReader(manifestPath) + if err != nil { + // Includes os.ErrNotExist: CURRENT names a manifest that no longer + // exists on disk — corruption, the database promised this file. + return fmt.Errorf("beachdb: opening manifest %q: %w", current, err) + } + + res, replayErr := replayManifestStream(reader, current) + closeErr := reader.Close() + if replayErr != nil { + return replayErr + } + if closeErr != nil { + return fmt.Errorf("beachdb: closing manifest reader: %w", closeErr) + } + + if res.tailTrimmed { + if err := os.Truncate(manifestPath, res.validOffset); err != nil { + return fmt.Errorf("beachdb: truncating manifest tail at %d: %w", res.validOffset, err) + } + } + + ssts, err := openSSTReadersForVersion(db.dir, res.version) + if err != nil { + return err + } + + // Open a Writer on the same manifest file. NewWriter uses O_APPEND, + // so subsequent edits land at the (possibly truncated) EOF. + writer, err := manifest.NewWriter(manifestPath) + if err != nil { + return fmt.Errorf("beachdb: opening manifest writer: %w", err) + } + + db.version = res.version + db.manifest = writer + db.ssts = ssts + db.nextSSTID = res.nextFileID + db.seqno = res.seqno + db.logno = res.logNumber + + return nil +} + // Helper function for writing a memtable to an SSTable file on disk. // blockSize controls the target data block size; 0 means use the sstable default. func writeSSTable(path string, mem memtable.Memtable, blockSize int) (*sstable.Reader, error) { @@ -747,73 +993,41 @@ func writeSSTable(path string, mem memtable.Memtable, blockSize int) (*sstable.R return sstReader, nil } -// Helper function for discovering SSTable files on disk -func discoverSSTables(dir string) ([]string, uint64, error) { - dirEntries, err := os.ReadDir(dir) - if err != nil { - return nil, 0, fmt.Errorf("beachdb: reading directory: %w", err) - } - - type sstableMeta struct { - id uint64 - name string - } - - sstableFiles := make([]sstableMeta, 0, len(dirEntries)) - seenIDs := make(map[uint64]string, len(dirEntries)) +// buildSSTFileName builds an SSTable file name from a file ID, +// e.g.: 1 --> 000001.sst +func buildSSTFileName(id uint64) string { + return fmt.Sprintf("%0*d%s", sstableFileIDWidth, id, sstableFileExt) +} - for _, entry := range dirEntries { - if !entry.Type().IsRegular() { - continue - } +// buildManifestFileName builds a MANIFEST filename from a file ID, +// e.g. 1 → "MANIFEST-000001". +func buildManifestFileName(id uint64) string { + return fmt.Sprintf("%s%0*d", manifestFilePrefix, manifestFileIDWidth, id) +} - fileName := entry.Name() - if filepath.Ext(fileName) != sstableFileExt { +// nextAvailableManifestID returns the smallest MANIFEST file ID not yet +// present in dir. Used by bootstrap to avoid colliding with an orphan +// MANIFEST file left by a prior crashed bootstrap. Returns 1 if no +// MANIFEST files exist. +func nextAvailableManifestID(dir string) (uint64, error) { + entries, err := os.ReadDir(dir) + if err != nil { + return 0, fmt.Errorf("beachdb: scanning dir for MANIFEST files: %w", err) + } + var maxID uint64 + for _, e := range entries { + name := e.Name() + if !strings.HasPrefix(name, manifestFilePrefix) { continue } - - strID := strings.TrimSuffix(fileName, filepath.Ext(fileName)) - parsedID, err := strconv.ParseUint(strID, 10, 64) + idStr := strings.TrimPrefix(name, manifestFilePrefix) + id, err := strconv.ParseUint(idStr, 10, 64) if err != nil { - return nil, 0, fmt.Errorf("beachdb: parsing SSTable ID %q: %w", fileName, err) - } - if existingName, exists := seenIDs[parsedID]; exists { - return nil, 0, fmt.Errorf("beachdb: duplicate SSTable ID %d in %q and %q", parsedID, existingName, fileName) + continue } - - seenIDs[parsedID] = fileName - sstableFiles = append(sstableFiles, sstableMeta{ - id: parsedID, - name: fileName, - }) - } - - slices.SortFunc(sstableFiles, func(left, right sstableMeta) int { - switch { - case left.id < right.id: - return -1 - case left.id > right.id: - return 1 - default: - return strings.Compare(left.name, right.name) + if id > maxID { + maxID = id } - }) - - names := make([]string, len(sstableFiles)) - for i, file := range sstableFiles { - names[i] = file.name - } - - var nextID uint64 - if len(sstableFiles) > 0 { - nextID = sstableFiles[len(sstableFiles)-1].id + 1 } - - return names, nextID, nil -} - -// Helper function for building an SSTable file name from a -// file ID number, e.g.: 1 --> 000001.sst -func buildSSTFileName(id uint64) string { - return fmt.Sprintf("%0*d%s", sstableFileIDWidth, id, sstableFileExt) + return maxID + 1, nil } diff --git a/internal/manifest/current.go b/internal/manifest/current.go index fddcdae..1cd4aef 100644 --- a/internal/manifest/current.go +++ b/internal/manifest/current.go @@ -26,6 +26,7 @@ const ( // database signal, not a hard error. func ReadCurrent(dir string) (string, error) { path := filepath.Join(dir, currentFileName) + //nolint:gosec // G304: path is constructed from the trusted DB directory data, err := os.ReadFile(path) if err != nil { @@ -39,6 +40,13 @@ func ReadCurrent(dir string) (string, error) { if name == "" { return "", fmt.Errorf("beachdb/manifest: CURRENT is empty: %w", ErrInvalidManifestName) } + // Symmetric validation with WriteCurrent: CURRENT must hold a bare + // filename, not a path. Without this, a malformed CURRENT lets the + // caller escape the database directory via path traversal. + if err := validateManifestName(name); err != nil { + return "", err + } + return name, nil } diff --git a/internal/manifest/doc.go b/internal/manifest/doc.go index 31156c1..9deb05f 100644 --- a/internal/manifest/doc.go +++ b/internal/manifest/doc.go @@ -1,5 +1,132 @@ -// Package manifest implements the MANIFEST file format for -// BeachDB's LSM storage engine. +// Package manifest implements the MANIFEST log and CURRENT pointer that +// track BeachDB's on-disk state across restarts. // -// TODO: Explain MANIFEST files and their format. +// # What is a manifest? +// +// An LSM-tree database is a soup of files: a WAL, a memtable, and a growing +// pile of SSTables that flushes and compactions keep producing. Someone has +// to remember which files exist, which level each SSTable belongs to, what +// key range it covers, and where the next file ID starts. That memory is the +// manifest. +// +// The manifest is an append-only log of [VersionEdit] records. Each edit +// describes one atomic change to the file set: "add this SSTable at level 0", +// "delete that SSTable at level 1", "next file ID is N", "last sequence +// number written was S". On startup, the database replays every edit in +// order to reconstruct the current [Version] — the immutable snapshot of +// "which SSTables exist right now." +// +// LevelDB and Pebble use the same idea. The append-only design means +// recording a state change is one synchronous record append plus an fsync, +// not a rewrite of the whole inventory. +// +// # File layout +// +// On disk, a BeachDB directory contains: +// +// beachdb.wal ← the write-ahead log +// CURRENT ← one-line pointer to the live MANIFEST +// MANIFEST-000001 ← the live append-only log of version edits +// 000001.sst, 000002.sst ← the SSTables referenced by the manifest +// +// CURRENT exists because manifests can be rotated (rewritten compactly, +// then atomically swapped in) in a future version. v1 does not rotate, but +// the indirection ships from day one so recovery code never depends on a +// fixed manifest filename. +// +// See docs/formats/manifest.md for the byte-level wire format. +// +// # VersionEdit +// +// [VersionEdit] is the unit of change. Each edit is a set of optional fields: +// +// - AddedFiles, DeletedFiles — SSTables entering or leaving the file set +// - NextFileID — bump the file-number allocator +// - LastSequence — advance the global sequence counter +// - LogNumber — the WAL number this edit is associated with +// +// Fields are encoded with a TLV (tag-length-value) scheme. Unknown tags are +// a hard error in v1 — there is no skip-unknown machinery. Future format +// changes ship via a manifest format-version bump rather than tag insertion. +// +// # Version +// +// [Version] is the in-memory snapshot the database operates on. Applying a +// VersionEdit to a Version produces a new Version (immutable, copy-on-write). +// Callers atomically swap the active pointer once an edit is durable on +// disk. v1 sorts L1+ by SmallestKey for efficient lookups; L0 is left in +// insertion order because L0 files overlap and need a merging iterator. +// +// # Writer and Reader +// +// [Writer] appends records to the manifest. Each Append encodes a record +// (magic + length + checksum + payload), writes it to the file, and fsyncs. +// The fsync happens inside Append, not as a separate call — manifest writes +// are infrequent (per flush, per compaction) and each one gates other engine +// actions, so there is no batching benefit to a separate Sync call. This is +// the deliberate asymmetry with the WAL writer, where Append and Sync are +// split so the WAL can group-commit many writes per fsync. +// +// [Reader] reads records sequentially from a manifest file. [Reader.Next] +// returns the raw record payload; [Reader.NextEdit] is the convenience +// wrapper that decodes it as a VersionEdit. Returns io.EOF cleanly at end +// of stream. Short reads surface as record.ErrTruncated; callers (replay +// code) can treat a trailing truncation as expected (crash during write) +// or as corruption depending on policy. [Reader.ValidOffset] reports the +// byte boundary after the last validated record so recovery can truncate +// a partial tail. +// +// # CURRENT file +// +// [ReadCurrent] reads the single-line CURRENT file and returns the live +// manifest's filename. Missing CURRENT returns [ErrNoCurrentFile] so the +// caller can treat a missing pointer as "fresh database, bootstrap from +// scratch" rather than a hard error. +// +// [WriteCurrent] installs a new pointer atomically: +// +// 1. Write "MANIFEST-NNNNNN\n" to CURRENT.tmp +// 2. fsync(CURRENT.tmp) +// 3. rename(CURRENT.tmp, CURRENT) — atomic on POSIX +// 4. fsync(parent directory) — so the rename is durable +// +// A crash at any step leaves either the old CURRENT intact or the new +// CURRENT fully written. Never a partial CURRENT. Same rename-into-place +// pattern as SSTable installation. +// +// # Recovery semantics +// +// On startup, the engine: +// +// 1. Reads CURRENT. If missing → fresh database. +// 2. Opens the manifest named in CURRENT. +// 3. Replays every VersionEdit in order, applying each to an initially +// empty Version. Counter tags advance the file-number / sequence / +// log-number watermarks. +// 4. Opens SSTable readers for every file in the final Version. A +// referenced file missing on disk is corruption — fail loudly. +// +// A truncated trailing record (crash during write) is recoverable: replay +// stops at the last valid record, and the engine can truncate the manifest +// file to [Reader.ValidOffset] before continuing. +// +// # Concurrency model +// +// Both [Writer] and [Reader] are single-instance per database. Their +// mutexes defend against accidental concurrent use, not designed +// concurrency. The engine wires the writer behind its own serialization +// (flush / compaction completion). +// +// # Current simplifications +// +// The following features are intentionally omitted from v1 and will land +// in later milestones: +// +// - No manifest rotation (the file grows unbounded for now) +// - No skip-unknown tag support (unknown tags hard-error) +// - No compaction pointers, snapshots, or per-table catalogs +// - No checksum on the manifest file as a whole (per-record checksum only) +// +// See docs/formats/manifest.md for the full format specification and +// versioning strategy. package manifest From 9748b67dd03084a8036421cccca0841d89325719 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Tue, 26 May 2026 00:11:33 +0200 Subject: [PATCH 15/26] test(engine): exercise the manifest bootstrap and replay paths --- engine/db_manifest_test.go | 1027 ++++++++++++++++++++++++++++++++++++ 1 file changed, 1027 insertions(+) create mode 100644 engine/db_manifest_test.go diff --git a/engine/db_manifest_test.go b/engine/db_manifest_test.go new file mode 100644 index 0000000..809976a --- /dev/null +++ b/engine/db_manifest_test.go @@ -0,0 +1,1027 @@ +package engine + +import ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + "testing" + + "github.com/aalhour/beachdb/internal/keys" + "github.com/aalhour/beachdb/internal/manifest" + "github.com/aalhour/beachdb/internal/memtable" + "github.com/aalhour/beachdb/internal/record" +) + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +// readCurrentName reads the CURRENT pointer file in dir and returns the +// manifest filename it contains. Fails the test if CURRENT can't be read +// or the contents don't parse as a single line. +func readCurrentName(t *testing.T, dir string) string { + t.Helper() + name, err := manifest.ReadCurrent(dir) + if err != nil { + t.Fatalf("ReadCurrent(%q): %v", dir, err) + } + return name +} + +// statManifest returns the size of the live manifest file in dir. +func statManifestSize(t *testing.T, dir string) int64 { + t.Helper() + name := readCurrentName(t, dir) + info, err := os.Stat(filepath.Join(dir, name)) + if err != nil { + t.Fatalf("stat manifest %q: %v", name, err) + } + return info.Size() +} + +// writeFile writes raw bytes to path. Used to plant hand-crafted fixtures +// (corrupt manifest, empty CURRENT, etc.). +func writeFile(t *testing.T, path string, data []byte) { + t.Helper() + if err := os.WriteFile(path, data, 0600); err != nil { + t.Fatalf("WriteFile(%q): %v", path, err) + } +} + +// readFile reads a whole file. Wrapper that fails the test on error. +func readFile(t *testing.T, path string) []byte { + t.Helper() + //nolint:gosec // path is from t.TempDir() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile(%q): %v", path, err) + } + return data +} + +// dirContains returns true if dir contains a file named name (non-recursive). +func dirContains(t *testing.T, dir, name string) bool { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + for _, e := range entries { + if e.Name() == name { + return true + } + } + return false +} + +// openFresh opens a brand-new DB in t.TempDir() and returns it. Caller is +// responsible for Close. +func openFresh(t *testing.T) (*DB, string) { + t.Helper() + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + return db, dir +} + +// putKey constructs an InternalKey with kind=Put. Mirrors the manifest pkg +// helper; redeclared here because cross-package test helpers don't carry. +func putInternalKey(userKey string, seqno uint64) keys.InternalKey { + return keys.InternalKey{ + UserKey: []byte(userKey), + Seqno: seqno, + Kind: keys.InternalKeyKindPut, + } +} + +// fileMeta builds a FileMetadata for fixture VersionEdits. +func fileMetaFixture(level uint32, fileID, size uint64, sk, lk string) manifest.FileMetadata { + return manifest.FileMetadata{ + Level: level, + FileID: fileID, + Size: size, + SmallestKey: putInternalKey(sk, 1), + LargestKey: putInternalKey(lk, 2), + } +} + +// createRealSST writes a tiny but well-formed SSTable at dir/.sst. +// Returns the size of the resulting file. Used to satisfy +// replayExistingManifest's "open SST readers from Version" step in tests +// that plant manifest AddFile edits. +func createRealSST(t *testing.T, dir string, fileID uint64) uint64 { + t.Helper() + sstPath := filepath.Join(dir, buildSSTFileName(fileID)) + + mem := memtable.NewSkipList() + mem.Put(keys.InternalKey{ + UserKey: []byte("a"), + Seqno: 1, + Kind: keys.InternalKeyKindPut, + }, []byte("v")) + + reader, err := writeSSTable(sstPath, mem, 0) + if err != nil { + t.Fatalf("writeSSTable: %v", err) + } + _ = reader.Close() + + info, err := os.Stat(sstPath) + if err != nil { + t.Fatalf("stat new SST: %v", err) + } + //nolint:gosec // G115: SST file size is bounded by test fixture + return uint64(info.Size()) +} + +// appendEditsToManifest opens the existing manifest file via Writer and +// appends each edit in order. Caller is responsible for the file already +// existing. +func appendEditsToManifest(t *testing.T, dir, manifestName string, edits ...*manifest.VersionEdit) { + t.Helper() + w, err := manifest.NewWriter(filepath.Join(dir, manifestName)) + if err != nil { + t.Fatalf("manifest.NewWriter: %v", err) + } + for _, e := range edits { + if err := w.Append(e.Encode()); err != nil { + t.Fatalf("Append: %v", err) + } + } + if err := w.Close(); err != nil { + t.Fatalf("Close: %v", err) + } +} + +// --------------------------------------------------------------------------- +// openManifest dispatch (CURRENT-based bootstrap vs. replay) +// --------------------------------------------------------------------------- + +// Empty dir → bootstrap creates CURRENT + MANIFEST-000001. +func TestOpen_Manifest_FreshDB(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer db.Close() + + if !dirContains(t, dir, "CURRENT") { + t.Error("CURRENT missing after fresh Open") + } + if !dirContains(t, dir, "MANIFEST-000001") { + t.Error("MANIFEST-000001 missing after fresh Open") + } + + name := readCurrentName(t, dir) + if name != "MANIFEST-000001" { + t.Errorf("CURRENT = %q, want MANIFEST-000001", name) + } + + if db.nextSSTID != 1 { + t.Errorf("db.nextSSTID = %d, want 1", db.nextSSTID) + } + if db.seqno != 0 { + t.Errorf("db.seqno = %d, want 0", db.seqno) + } + if db.version == nil { + t.Fatal("db.version is nil") + } + if db.version.NumLevels() != 0 { + t.Errorf("db.version.NumLevels() = %d, want 0", db.version.NumLevels()) + } +} + +// Pre-place MANIFEST without CURRENT → bootstrap as fresh, orphan ignored. +func TestOpen_Manifest_OrphanManifestWithoutCurrent(t *testing.T) { + dir := t.TempDir() + // Plant a bogus manifest file that no CURRENT points at. + orphanPath := filepath.Join(dir, "MANIFEST-000003") + writeFile(t, orphanPath, []byte("orphan-garbage")) + + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer db.Close() + + // Bootstrap path runs. + if !dirContains(t, dir, "CURRENT") { + t.Error("CURRENT missing after bootstrap") + } + // Orphan still present, not opened. + if !dirContains(t, dir, "MANIFEST-000003") { + t.Error("orphan MANIFEST-000003 should remain on disk (no cleanup yet)") + } +} + +// CURRENT exists but is empty → hard error. +func TestOpen_Manifest_EmptyCurrent(t *testing.T) { + dir := t.TempDir() + // Plant an empty CURRENT. + writeFile(t, filepath.Join(dir, "CURRENT"), []byte("")) + + _, err := Open(dir, WithSync(false)) + if err == nil { + t.Fatal("expected error for empty CURRENT") + } + if !errors.Is(err, manifest.ErrInvalidManifestName) { + t.Errorf("got %v, want ErrInvalidManifestName in chain", err) + } +} + +// CURRENT points at a manifest that doesn't exist → hard error. +func TestOpen_Manifest_CurrentPointsAtMissing(t *testing.T) { + dir := t.TempDir() + if err := manifest.WriteCurrent(dir, "MANIFEST-999999"); err != nil { + t.Fatalf("WriteCurrent: %v", err) + } + + _, err := Open(dir, WithSync(false)) + if err == nil { + t.Fatal("expected error when CURRENT names a missing manifest") + } + if !errors.Is(err, os.ErrNotExist) { + t.Errorf("got %v, want os.ErrNotExist in chain", err) + } +} + +// Manifest is corrupt mid-stream → hard error. +func TestOpen_Manifest_MidStreamCorruption(t *testing.T) { + // Bootstrap a real DB, then corrupt the manifest payload, then reopen. + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Flip a byte well past the header into the payload. + manifestPath := filepath.Join(dir, readCurrentName(t, dir)) + data := readFile(t, manifestPath) + if len(data) <= record.HeaderSize { + t.Fatalf("bootstrap manifest unexpectedly short: %d bytes", len(data)) + } + data[record.HeaderSize] ^= 0xFF // payload corruption + writeFile(t, manifestPath, data) + + _, err = Open(dir, WithSync(false)) + if err == nil { + t.Fatal("expected error opening corrupted manifest") + } + if !errors.Is(err, record.ErrChecksum) { + t.Errorf("got %v, want record.ErrChecksum in chain", err) + } +} + +// Trailing record is truncated → benign; file gets resized to last +// validated boundary. +func TestOpen_Manifest_TailTruncation_Benign(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Append one extra record (so we have something to chop), then truncate + // mid-payload. + currentName := readCurrentName(t, dir) + extra := &manifest.VersionEdit{HasLastSequence: true, LastSequence: 42} + appendEditsToManifest(t, dir, currentName, extra) + + manifestPath := filepath.Join(dir, currentName) + full := readFile(t, manifestPath) + // Chop the last 3 bytes — guaranteed mid-payload of the appended record. + writeFile(t, manifestPath, full[:len(full)-3]) + preOpenSize := int64(len(full) - 3) + _ = preOpenSize + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open after tail truncation should succeed: %v", err) + } + defer db2.Close() + + // After replay, file should be truncated to the last valid record boundary. + postSize := statManifestSize(t, dir) + if postSize >= int64(len(full)) { + t.Errorf("manifest not truncated: size=%d, original=%d", postSize, len(full)) + } +} + +// Happy path: pre-populated manifest replays into a fresh DB. +func TestOpen_Manifest_HappyPath(t *testing.T) { + // Bootstrap then append a counter edit to a real manifest, then reopen. + // The replay path should pick up the bumped counters. + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + bumped := &manifest.VersionEdit{ + HasNextFileID: true, NextFileID: 7, + HasLastSequence: true, LastSequence: 123, + } + appendEditsToManifest(t, dir, readCurrentName(t, dir), bumped) + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open replay: %v", err) + } + defer db2.Close() + + if db2.nextSSTID != 7 { + t.Errorf("nextSSTID after replay = %d, want 7", db2.nextSSTID) + } + if db2.seqno != 123 { + t.Errorf("seqno after replay = %d, want 123", db2.seqno) + } +} + +// --------------------------------------------------------------------------- +// bootstrapFreshManifest behaviors +// --------------------------------------------------------------------------- + +func TestBootstrap_CreatesManifestAndCurrent(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer db.Close() + + if !dirContains(t, dir, "CURRENT") { + t.Error("CURRENT not created on fresh Open") + } + if !dirContains(t, dir, "MANIFEST-000001") { + t.Error("MANIFEST-000001 not created on fresh Open") + } +} + +// Bootstrap must set HasNextFileID/HasLastSequence/HasLogNumber so the +// counters actually get encoded in the initial edit. Without Has*=true, +// VersionEdit.Encode silently drops the fields and the initial record is +// header-only (no payload). +func TestBootstrap_InitialEditHasHasFlags(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Read manifest directly and inspect the first record. + manifestPath := filepath.Join(dir, readCurrentName(t, dir)) + r, err := manifest.NewReader(manifestPath) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + defer r.Close() + + edit, err := r.NextEdit() + if err != nil { + t.Fatalf("NextEdit: %v", err) + } + if !edit.HasNextFileID { + t.Error("initial edit missing HasNextFileID — counter will be silently dropped") + } + if !edit.HasLastSequence { + t.Error("initial edit missing HasLastSequence") + } + if !edit.HasLogNumber { + t.Error("initial edit missing HasLogNumber") + } +} + +// Counters set by bootstrap must survive a close + reopen. Locks down the +// end-to-end persistence path: bootstrap encodes Has* flags, the next Open +// reads them back, and the post-reopen db.nextSSTID / db.seqno match the +// values bootstrap chose. +func TestBootstrap_CountersSurviveReopen(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("first Open: %v", err) + } + firstNext := db.nextSSTID + firstSeqno := db.seqno + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("reopen: %v", err) + } + defer db2.Close() + + if db2.nextSSTID != firstNext { + t.Errorf("nextSSTID after reopen = %d, want %d (bootstrap counters not persisted)", db2.nextSSTID, firstNext) + } + if db2.seqno != firstSeqno { + t.Errorf("seqno after reopen = %d, want %d", db2.seqno, firstSeqno) + } + if db2.nextSSTID == 0 { + t.Error("nextSSTID = 0 after reopen → next flush would write 00000000000000000000.sst (file ID collision)") + } +} + +// After fresh Open + Close, a second Open should follow the replay path, +// not bootstrap. We can't directly observe which branch fired, so we use +// a proxy: the second Open must not change the manifest filename. +func TestBootstrap_NextOpenTakesReplayPath(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("first Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + firstName := readCurrentName(t, dir) + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("second Open: %v", err) + } + defer db2.Close() + secondName := readCurrentName(t, dir) + + if firstName != secondName { + t.Errorf("manifest name changed across Opens: %q -> %q (replay path should not rename)", firstName, secondName) + } +} + +// If bootstrap crashes between Append and WriteCurrent, the orphan +// MANIFEST file is present but CURRENT is not. The next Open should +// treat the directory as fresh (scenario 2) and continue. +func TestBootstrap_CurrentInstallIsLast_SimulatedOrphan(t *testing.T) { + dir := t.TempDir() + // Plant an orphan MANIFEST with no CURRENT. + writeFile(t, filepath.Join(dir, "MANIFEST-000001"), []byte("orphan-from-crashed-bootstrap")) + + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open should bootstrap fresh: %v", err) + } + defer db.Close() + + if !dirContains(t, dir, "CURRENT") { + t.Error("CURRENT should be installed by bootstrap") + } +} + +// --------------------------------------------------------------------------- +// replayExistingManifest deep paths (counters, deletes, truncation, recovery) +// --------------------------------------------------------------------------- + +// Counter edits accumulate with last-wins semantics (only Has* fields apply). +func TestReplay_Counters_LastWins(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + currentName := readCurrentName(t, dir) + appendEditsToManifest(t, dir, currentName, + &manifest.VersionEdit{HasNextFileID: true, NextFileID: 1}, + &manifest.VersionEdit{HasNextFileID: true, NextFileID: 5}, + &manifest.VersionEdit{HasNextFileID: true, NextFileID: 3}, + ) + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open replay: %v", err) + } + defer db2.Close() + + if db2.nextSSTID != 3 { + t.Errorf("nextSSTID = %d, want 3 (last-wins)", db2.nextSSTID) + } +} + +// HasNextFileID=false means the field is not present in the encoding; +// the previous counter value must survive. +func TestReplay_Counters_HasFlagsRespected(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + currentName := readCurrentName(t, dir) + appendEditsToManifest(t, dir, currentName, + &manifest.VersionEdit{HasNextFileID: true, NextFileID: 10}, + // Has*=false: NextFileID=5 is in the struct but won't encode. + &manifest.VersionEdit{HasLastSequence: true, LastSequence: 99}, + ) + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open replay: %v", err) + } + defer db2.Close() + + if db2.nextSSTID != 10 { + t.Errorf("nextSSTID = %d, want 10 (second edit must not overwrite without HasNextFileID)", db2.nextSSTID) + } + if db2.seqno != 99 { + t.Errorf("seqno = %d, want 99", db2.seqno) + } +} + +// AddFile then DeleteFile for the same fileID leaves the Version empty. +func TestReplay_DeleteFileEdit_RemovesFromVersion(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + currentName := readCurrentName(t, dir) + appendEditsToManifest(t, dir, currentName, + &manifest.VersionEdit{ + AddedFiles: []manifest.FileMetadata{ + fileMetaFixture(0, 42, 100, "a", "z"), + }, + }, + &manifest.VersionEdit{ + DeletedFiles: []manifest.DeletedFile{{Level: 0, FileID: 42}}, + }, + ) + + // Note: this manifest references fileID=42 transiently. Replay will + // try to open the SST file for any survivors of Version.AllFiles(). + // Since the DeleteFile removes it, no SST open should be attempted. + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open replay (delete cancels add): %v", err) + } + defer db2.Close() + + if db2.version == nil || len(db2.version.AllFiles()) != 0 { + t.Errorf("expected empty Version after Add+Delete cancellation") + } + if len(db2.ssts) != 0 { + t.Errorf("expected zero SST readers, got %d", len(db2.ssts)) + } +} + +// Empty manifest (zero bytes) — replay must either fail loudly or +// produce safe defaults; in particular nextSSTID must not be 0, which +// would collide with the "not allocated" sentinel. +func TestReplay_EmptyManifest_NoEdits(t *testing.T) { + dir := t.TempDir() + // Fabricate: install CURRENT pointing at an empty manifest file. + emptyManifest := filepath.Join(dir, "MANIFEST-000001") + writeFile(t, emptyManifest, []byte{}) + if err := manifest.WriteCurrent(dir, "MANIFEST-000001"); err != nil { + t.Fatalf("WriteCurrent: %v", err) + } + + db, err := Open(dir, WithSync(false)) + if err != nil { + // Acceptable outcome: hard error on a manifest with no counter edits. + t.Logf("Open on empty manifest returned error (acceptable): %v", err) + return + } + defer db.Close() + + // Alternative acceptable outcome: bootstrap-like defaults. nextSSTID + // must NOT be 0 (would collide with a non-existent file 0). + if db.nextSSTID == 0 { + t.Error("replay of empty manifest left db.nextSSTID=0 — next flush would write file 0") + } +} + +// Tail truncation must resize the manifest file to the last valid offset. +func TestReplay_TruncatedTail_FileResized(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + currentName := readCurrentName(t, dir) + appendEditsToManifest(t, dir, currentName, + &manifest.VersionEdit{HasLastSequence: true, LastSequence: 7}, + ) + manifestPath := filepath.Join(dir, currentName) + full := readFile(t, manifestPath) + + // Chop last 3 bytes — mid-payload of the appended record. + writeFile(t, manifestPath, full[:len(full)-3]) + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open should tolerate tail truncation: %v", err) + } + defer db2.Close() + + postSize := statManifestSize(t, dir) + if postSize >= int64(len(full)) { + t.Errorf("manifest not truncated: size=%d, original=%d", postSize, len(full)) + } +} + +// After tail truncation, the next append should land at the truncation +// boundary (not over the partial bytes). +func TestReplay_AfterTruncation_AppendsLandCleanly(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + currentName := readCurrentName(t, dir) + appendEditsToManifest(t, dir, currentName, + &manifest.VersionEdit{HasLastSequence: true, LastSequence: 7}, + ) + manifestPath := filepath.Join(dir, currentName) + full := readFile(t, manifestPath) + writeFile(t, manifestPath, full[:len(full)-3]) + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + + // Append a fresh edit via the now-open Writer. + if err := db2.manifest.Append((&manifest.VersionEdit{HasLastSequence: true, LastSequence: 100}).Encode()); err != nil { + t.Fatalf("Append after recovery: %v", err) + } + if err := db2.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Reopen — replay must succeed end-to-end with no corruption. + db3, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open after recovery + append: %v", err) + } + defer db3.Close() + + if db3.seqno != 100 { + t.Errorf("seqno = %d, want 100 (latest counter)", db3.seqno) + } +} + +// --------------------------------------------------------------------------- +// Open consolidation and error-path cleanup +// --------------------------------------------------------------------------- + +// After Open succeeds, db.version / db.manifest / db.nextSSTID / db.seqno +// must all be installed. +func TestOpen_PostStateInstalled(t *testing.T) { + db, _ := openFresh(t) + defer db.Close() + + if db.version == nil { + t.Error("db.version is nil after Open") + } + if db.manifest == nil { + t.Error("db.manifest is nil after Open") + } + // On fresh DB: nextSSTID=1, seqno=0 (assumes bootstrap counters are set). + if db.nextSSTID == 0 { + t.Errorf("db.nextSSTID = %d, want 1 (bootstrap should set NextFileID=1)", db.nextSSTID) + } +} + +// WAL replay continues from the manifest's LastSequence. +// TODO: needs a flush that checkpoints seqno into the manifest before this +// can produce a meaningful baseline. Currently flush does not write a +// VersionEdit, so the seqno-in-manifest path is unreachable. +func TestOpen_OrderingInvariant_ManifestBeforeWAL(t *testing.T) { + t.Skip("TODO: requires flush to checkpoint LastSequence in the manifest") +} + +// --------------------------------------------------------------------------- +// Close path with manifest +// --------------------------------------------------------------------------- + +func TestClose_ReleasesManifestWriter(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Re-opening must succeed (proves the file handle was released). + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen after Close: %v", err) + } + defer db2.Close() +} + +func TestClose_NullsManifestField(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if db.manifest != nil { + t.Error("db.manifest not nil after Close") + } +} + +// Closing twice should return ErrDBClosed the second time, not panic on +// the now-nil manifest. +func TestClose_DoubleClose_WithManifest(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("first Close: %v", err) + } + err = db.Close() + if !errors.Is(err, ErrDBClosed) { + t.Errorf("second Close: got %v, want ErrDBClosed", err) + } +} + +// --------------------------------------------------------------------------- +// Flush + cross-restart durability (skipped pending wiring) +// +// TODO: these tests require publishFlushedSSTLocked to write a VersionEdit +// to the manifest on every successful flush, plus matching crashhook +// points around the SST-sync → manifest-append boundary. Until that +// wiring lands, flush is invisible to the manifest and these scenarios +// can't be exercised. +// --------------------------------------------------------------------------- + +func TestFlush_WritesManifestEdit(t *testing.T) { + t.Skip("TODO: requires flush to write a VersionEdit per flush") +} +func TestFlush_OrderingInvariant_SSTBeforeManifest(t *testing.T) { + t.Skip("TODO: requires crashhook points around SST-sync → manifest-append") +} +func TestFlush_ManifestAppendFailure_SSTOrphaned(t *testing.T) { + t.Skip("TODO: requires a fault-injection point for manifest.Append") +} +func TestFlush_NextSSTIDPersistedViaManifest(t *testing.T) { + t.Skip("TODO: requires flush to record NextFileID in the manifest") +} +func TestFlush_LastSequencePersistedViaManifest(t *testing.T) { + t.Skip("TODO: requires flush to checkpoint LastSequence in the manifest") +} +func TestFlush_Multiple_AllRecoverable(t *testing.T) { + t.Skip("TODO: requires flush to write a VersionEdit per flush") +} +func TestRestart_DataFromSSTOnly(t *testing.T) { + t.Skip("TODO: requires flush wiring so SSTs are recoverable without the WAL") +} +func TestRestart_DataFromSSTAndWAL(t *testing.T) { + t.Skip("TODO: requires flush wiring for the post-checkpoint WAL path") +} +func TestRestart_NextSSTID_NoCollisionsAfterCrash(t *testing.T) { + t.Skip("TODO: requires flush to checkpoint NextFileID across restarts") +} +func TestRestart_SeqnoMonotonic(t *testing.T) { + t.Skip("TODO: requires flush to checkpoint LastSequence across restarts") +} + +// --------------------------------------------------------------------------- +// Adversarial / recovery scenarios +// --------------------------------------------------------------------------- + +// TestBootstrap_RecoversFromOrphanManifestCollision: a prior crashed +// bootstrap may have left a MANIFEST-NNNNNN on disk with no CURRENT +// pointing at it (the install step never ran). The next bootstrap must +// not reuse the same filename with O_APPEND — writing the new initial +// edit on top of the orphan's garbage bytes would corrupt the stream. +// Bootstrap is expected to pick a fresh, unused MANIFEST ID. +func TestBootstrap_RecoversFromOrphanManifestCollision(t *testing.T) { + dir := t.TempDir() + // Plant an orphan MANIFEST-000001 with garbage (no CURRENT). + writeFile(t, filepath.Join(dir, "MANIFEST-000001"), []byte("garbage-from-crashed-bootstrap")) + + db, err := Open(dir, WithSync(false)) + if err != nil { + // Acceptable alternative: detect the collision and refuse. + t.Logf("Open detected orphan and failed (acceptable): %v", err) + return + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Reopen → replay must succeed on the manifest CURRENT points at. + // If bootstrap appended to the orphan, replay will hit corruption. + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Errorf("orphan collision corrupted the manifest stream: %v", err) + return + } + defer db2.Close() +} + +// TestOpen_RejectsCurrentPathTraversal: a malformed CURRENT containing +// path separators must be rejected by ReadCurrent's validation, not +// blindly followed via filepath.Join. The test plants a real file at the +// traversal target so that "any error" is insufficient — only validation +// rejection (ErrInvalidManifestName) is correct. Without symmetric +// validation between ReadCurrent and WriteCurrent, a hand-edited CURRENT +// could escape the database directory. +func TestOpen_RejectsCurrentPathTraversal(t *testing.T) { + // Layout: + // / + // decoy/MANIFEST-evil ← a real, openable file outside the DB dir + // db/CURRENT ← contents: "../decoy/MANIFEST-evil" + root := t.TempDir() + + if err := os.MkdirAll(filepath.Join(root, "decoy"), 0750); err != nil { + t.Fatalf("mkdir decoy: %v", err) + } + // File can be empty — replayExistingManifest will fail later anyway, + // but we want to make sure failure isn't os.ErrNotExist. + writeFile(t, filepath.Join(root, "decoy", "MANIFEST-evil"), []byte("decoy-content")) + + dbDir := filepath.Join(root, "db") + if err := os.MkdirAll(dbDir, 0750); err != nil { + t.Fatalf("mkdir db: %v", err) + } + writeFile(t, filepath.Join(dbDir, "CURRENT"), []byte("../decoy/MANIFEST-evil\n")) + + _, err := Open(dbDir, WithSync(false)) + if err == nil { + t.Fatal("Open followed path traversal without rejecting CURRENT") + } + + // Right rejection: ErrInvalidManifestName from validation. + // Wrong rejection: any other error (e.g. record framing failure on + // the decoy file's contents) means the traversal was followed. + if !errors.Is(err, manifest.ErrInvalidManifestName) { + t.Errorf("got %v, want manifest.ErrInvalidManifestName (path traversal not validated)", err) + } +} + +// TestOpen_OrphanManifestIgnoredWhenCurrentValid: when multiple MANIFEST +// files exist on disk but CURRENT names a specific one, Open must use +// only that one. The other(s) are ignored — they may be artifacts of a +// failed rotation or a crashed bootstrap. +func TestOpen_OrphanManifestIgnoredWhenCurrentValid(t *testing.T) { + dir := t.TempDir() + // Bootstrap a real DB to get a valid MANIFEST-000001. + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Plant a junk MANIFEST-000002 that no CURRENT points at. + writeFile(t, filepath.Join(dir, "MANIFEST-000002"), []byte("orphan-with-junk")) + + // CURRENT still points at MANIFEST-000001 — Open should succeed. + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Errorf("orphan MANIFEST-000002 should not affect Open of MANIFEST-000001: %v", err) + return + } + defer db2.Close() +} + +// TestReplay_RejectsDuplicateAddFile: a manifest that adds the same +// fileID twice without an intervening DeleteFile is corrupt or +// engine-buggy. Replay must reject it rather than silently producing +// duplicate readers / duplicate Version entries. +// +// The test plants a real SST file at fileID=42 so that the post-replay +// "open SST readers" step succeeds — that isolates the duplicate- +// detection question from the SST-existence question. +func TestReplay_RejectsDuplicateAddFile(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Plant a real SST file at fileID=42 so the post-replay open + // succeeds. Without this fixture the test would fail at the wrong + // step (missing SST) and mask the duplicate-detection bug. + const fileID uint64 = 42 + sstSize := createRealSST(t, dir, fileID) + + currentName := readCurrentName(t, dir) + dup := manifest.FileMetadata{ + Level: 0, + FileID: fileID, + Size: sstSize, + SmallestKey: putInternalKey("a", 1), + LargestKey: putInternalKey("a", 1), + } + appendEditsToManifest(t, dir, currentName, + &manifest.VersionEdit{AddedFiles: []manifest.FileMetadata{dup}}, + &manifest.VersionEdit{AddedFiles: []manifest.FileMetadata{dup}}, + ) + + db2, err := Open(dir, WithSync(false)) + if err != nil { + // Acceptable outcome: replay detected the duplicate and rejected. + t.Logf("Open rejected duplicate AddFile (acceptable): %v", err) + return + } + defer db2.Close() + + // Open succeeded — verify it didn't silently produce duplicate + // readers / duplicate Version entries. + if got := len(db2.ssts); got != 1 { + t.Errorf("db.ssts has %d entries for one fileID (want 1) — duplicate not detected", got) + } + if db2.version != nil { + if got := len(db2.version.AllFiles()); got != 1 { + t.Errorf("version has %d files for one fileID (want 1) — duplicate not detected", got) + } + } +} + +// TestReplay_OrphanDeleteFile_Ignored: a DeleteFile for a fileID that +// was never added is silently ignored (idempotent). Locks down that +// behavior so the replay path stays tolerant of partial-replay or +// reorder scenarios that may legitimately produce orphan deletes. +func TestReplay_OrphanDeleteFile_Ignored(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + currentName := readCurrentName(t, dir) + appendEditsToManifest(t, dir, currentName, + &manifest.VersionEdit{DeletedFiles: []manifest.DeletedFile{{Level: 0, FileID: 999}}}, + ) + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open with orphan DeleteFile: %v", err) + } + defer db2.Close() + + if db2.version == nil || len(db2.version.AllFiles()) != 0 { + t.Errorf("Version not empty after orphan DeleteFile") + } +} + +// TODO: the following adversarial scenarios require fault-injection hooks +// that don't exist yet — append failures during bootstrap, rename failures +// in CURRENT install. Re-enable once the relevant crashhook/fault points +// are wired up. +func TestBootstrap_HandlesAppendFailure(t *testing.T) { + t.Skip("TODO: requires a fault-injection point for manifest.Append") +} +func TestBootstrap_HandlesRenameFailure(t *testing.T) { + t.Skip("TODO: requires filesystem-level fault injection for rename") +} + +// Discard unused import warnings if a section is fully skipped. +var _ = io.EOF +var _ = fmt.Sprint +var _ = record.HeaderSize From b5b0ad3a8b68e30e3a832526c9dd93380090bcc4 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Tue, 26 May 2026 00:12:29 +0200 Subject: [PATCH 16/26] chore(engine): remove discoverSSTables tests; align SST ID convention with manifest --- engine/db_flush_test.go | 314 ++++------------------------------------ 1 file changed, 30 insertions(+), 284 deletions(-) diff --git a/engine/db_flush_test.go b/engine/db_flush_test.go index ab641b8..b946558 100644 --- a/engine/db_flush_test.go +++ b/engine/db_flush_test.go @@ -29,125 +29,16 @@ func TestDB_OpenWithNoSSTables(t *testing.T) { if len(db.ssts) != 0 { t.Fatalf("expected 0 SSTable readers on fresh DB, got %d", len(db.ssts)) } - if db.nextSSTID != 0 { - t.Fatalf("expected nextSSTID=0 on fresh DB, got %d", db.nextSSTID) + if db.nextSSTID != 1 { + t.Fatalf("expected nextSSTID=1 on fresh DB (bootstrap reserves ID 0 as sentinel), got %d", db.nextSSTID) } } -// Spec: "create SST files manually, open DB, verify they are loaded" -func TestDB_DiscoverSSTables(t *testing.T) { - dir := t.TempDir() - - // Create a DB, write entries, flush to produce SSTable files - db, err := Open(dir, WithSync(false)) - if err != nil { - t.Fatalf("Open: %v", err) - } - ctx := context.Background() - for i := range 10 { - if err := db.Put(ctx, fmt.Appendf(nil, "key-%04d", i), fmt.Appendf(nil, "val-%04d", i)); err != nil { - t.Fatalf("Put: %v", err) - } - } - if err := db.flushMemtable(); err != nil { - t.Fatalf("flush: %v", err) - } - db.Close() - - // Verify the SST file exists on disk - matches, err := filepath.Glob(filepath.Join(dir, "*.sst")) - if err != nil { - t.Fatalf("Glob: %v", err) - } - if len(matches) != 1 { - t.Fatalf("expected 1 SST file, got %d", len(matches)) - } - - // Re-open the DB — it should discover the SSTable - db2, err := Open(dir, WithSync(false)) - if err != nil { - t.Fatalf("re-Open: %v", err) - } - defer db2.Close() - - if len(db2.ssts) != 1 { - t.Fatalf("expected 1 SSTable reader after re-open, got %d", len(db2.ssts)) - } - if db2.nextSSTID != 1 { - t.Fatalf("expected nextSSTID=1, got %d", db2.nextSSTID) - } -} - -func TestDB_DiscoverMultipleSSTables(t *testing.T) { - dir := t.TempDir() - - db, err := Open(dir, WithSync(false)) - if err != nil { - t.Fatalf("Open: %v", err) - } - ctx := context.Background() - - // Flush 3 times to produce 3 SSTable files - for batch := range 3 { - for i := range 5 { - key := fmt.Appendf(nil, "batch%d-key%d", batch, i) - val := fmt.Appendf(nil, "batch%d-val%d", batch, i) - if err := db.Put(ctx, key, val); err != nil { - t.Fatalf("Put: %v", err) - } - } - if err := db.flushMemtable(); err != nil { - t.Fatalf("flush %d: %v", batch, err) - } - } - db.Close() - - // Verify 3 SST files on disk - matches, err := filepath.Glob(filepath.Join(dir, "*.sst")) - if err != nil { - t.Fatalf("Glob: %v", err) - } - if len(matches) != 3 { - t.Fatalf("expected 3 SST files, got %d", len(matches)) - } - - // Re-open and verify - db2, err := Open(dir, WithSync(false)) - if err != nil { - t.Fatalf("re-Open: %v", err) - } - defer db2.Close() - - if len(db2.ssts) != 3 { - t.Fatalf("expected 3 SSTable readers, got %d", len(db2.ssts)) - } - if db2.nextSSTID != 3 { - t.Fatalf("expected nextSSTID=3, got %d", db2.nextSSTID) - } -} - -// Non-SST files in the directory should be ignored -func TestDB_DiscoverIgnoresNonSSTFiles(t *testing.T) { - dir := t.TempDir() - - // Create some non-SST files - for _, name := range []string{"notes.txt", "backup.bak", "data.log"} { - if err := os.WriteFile(filepath.Join(dir, name), []byte("junk"), 0o600); err != nil { - t.Fatal(err) - } - } - - files, nextID, err := discoverSSTables(dir) - if err != nil { - t.Fatalf("discoverSSTables: %v", err) - } - if len(files) != 0 { - t.Fatalf("expected 0 SST files, got %d: %v", len(files), files) - } - if nextID != 0 { - t.Fatalf("expected nextID=0, got %d", nextID) - } -} +// Filesystem-discovery tests (TestDB_DiscoverSSTables and friends) were +// removed because the manifest is now the source of truth for which +// SSTables exist; the engine no longer walks the directory to find them. +// SST loading behavior is now covered in engine/db_manifest_test.go via +// TestOpen_Manifest_HappyPath and the bootstrap/replay tests there. // buildSSTFileName produces the expected zero-padded format func TestBuildSSTFileName(t *testing.T) { @@ -248,8 +139,9 @@ func TestFlush_ProducesValidSSTable(t *testing.T) { t.Fatalf("flushMemtable: %v", err) } - // SST file should exist on disk - sstPath := filepath.Join(dir, buildSSTFileName(0)) + // SST file should exist on disk. The first allocated file ID is 1 — + // 0 is reserved as the "unallocated" sentinel. + sstPath := filepath.Join(dir, buildSSTFileName(1)) if _, err := os.Stat(sstPath); err != nil { t.Fatalf("SST file not found: %v", err) } @@ -349,16 +241,17 @@ func TestFlush_IncrementsSSTID(t *testing.T) { } } - if db.nextSSTID != 3 { - t.Fatalf("expected nextSSTID=3 after 3 flushes, got %d", db.nextSSTID) + // File IDs start at 1 (bootstrap reserves 0 as sentinel), so after + // 3 flushes db.nextSSTID is 4 and files 1..3 exist on disk. + if db.nextSSTID != 4 { + t.Fatalf("expected nextSSTID=4 after 3 flushes, got %d", db.nextSSTID) } if len(db.ssts) != 3 { t.Fatalf("expected 3 readers, got %d", len(db.ssts)) } - // Verify filenames on disk - for i := range 3 { - sstPath := filepath.Join(dir, buildSSTFileName(uint64(i))) + for i := uint64(1); i <= 3; i++ { + sstPath := filepath.Join(dir, buildSSTFileName(i)) if _, err := os.Stat(sstPath); err != nil { t.Fatalf("expected %s to exist: %v", sstPath, err) } @@ -470,8 +363,8 @@ func TestFlush_SynchronousFailurePreservesReadableData(t *testing.T) { if len(db.ssts) != 0 { t.Fatalf("expected no SSTs to be published on failed flush, got %d", len(db.ssts)) } - if db.nextSSTID != 0 { - t.Fatalf("expected nextSSTID to stay at 0 after failed flush, got %d", db.nextSSTID) + if db.nextSSTID != 1 { + t.Fatalf("expected nextSSTID to stay at 1 after failed flush (no successful publish), got %d", db.nextSSTID) } if db.immMem == nil { t.Fatal("expected failed synchronous flush to keep the frozen memtable readable") @@ -513,9 +406,9 @@ func TestFlush_MultipleFlushes(t *testing.T) { t.Fatalf("expected 5 readers, got %d", len(db.ssts)) } - // Each SST should be independently openable - for i := range 5 { - path := filepath.Join(dir, buildSSTFileName(uint64(i))) + // Each SST should be independently openable. File IDs start at 1. + for i := uint64(1); i <= 5; i++ { + path := filepath.Join(dir, buildSSTFileName(i)) f, err := os.Open(path) //nolint:gosec // test with known temp path if err != nil { t.Fatalf("Open SST %d: %v", i, err) @@ -544,125 +437,9 @@ func TestFlush_OnClosedDB(t *testing.T) { _ = db.flushMemtable() } -// --- discoverSSTables unit tests --- - -func TestDiscoverSSTables_SortOrder(t *testing.T) { - dir := t.TempDir() - - // Create SST files out of order - names := []string{"000003.sst", "000001.sst", "000002.sst"} - for _, name := range names { - path := filepath.Join(dir, name) - // Write a valid (empty) SSTable - f, err := os.Create(path) //nolint:gosec // test with known temp path - if err != nil { - t.Fatal(err) - } - w, err := sstable.NewWriter(f, sstable.WithSync(false)) - if err != nil { - t.Fatal(err) - } - w.Close() - } - - files, nextID, err := discoverSSTables(dir) - if err != nil { - t.Fatalf("discoverSSTables: %v", err) - } - - // Should be sorted ASC - expected := []string{"000001.sst", "000002.sst", "000003.sst"} - if !slices.Equal(files, expected) { - t.Fatalf("expected %v, got %v", expected, files) - } - if nextID != 4 { - t.Fatalf("expected nextID=4, got %d", nextID) - } -} - -func TestDiscoverSSTables_EmptyDir(t *testing.T) { - dir := t.TempDir() - - files, nextID, err := discoverSSTables(dir) - if err != nil { - t.Fatalf("discoverSSTables: %v", err) - } - if len(files) != 0 { - t.Fatalf("expected 0 files, got %d", len(files)) - } - if nextID != 0 { - t.Fatalf("expected nextID=0, got %d", nextID) - } -} - -func TestDiscoverSSTables_RejectsMalformedName(t *testing.T) { - dir := t.TempDir() - - if err := os.WriteFile(filepath.Join(dir, "not-a-number.sst"), []byte("junk"), 0o600); err != nil { - t.Fatal(err) - } - - _, _, err := discoverSSTables(dir) - if err == nil { - t.Fatal("expected malformed SSTable name to fail discovery") - } -} - -func TestDiscoverSSTables_SortsByNumericID(t *testing.T) { - dir := t.TempDir() - - for _, name := range []string{"10.sst", "2.sst", "000001.sst"} { - path := filepath.Join(dir, name) - f, err := os.Create(path) //nolint:gosec // test with known temp path - if err != nil { - t.Fatal(err) - } - w, err := sstable.NewWriter(f, sstable.WithSync(false)) - if err != nil { - t.Fatal(err) - } - if err := w.Close(); err != nil { - t.Fatal(err) - } - } - - files, nextID, err := discoverSSTables(dir) - if err != nil { - t.Fatalf("discoverSSTables: %v", err) - } - - expected := []string{"000001.sst", "2.sst", "10.sst"} - if !slices.Equal(files, expected) { - t.Fatalf("expected %v, got %v", expected, files) - } - if nextID != 11 { - t.Fatalf("expected nextID=11, got %d", nextID) - } -} - -func TestDiscoverSSTables_RejectsDuplicateNumericID(t *testing.T) { - dir := t.TempDir() - - for _, name := range []string{"1.sst", "000001.sst"} { - path := filepath.Join(dir, name) - f, err := os.Create(path) //nolint:gosec // test with known temp path - if err != nil { - t.Fatal(err) - } - w, err := sstable.NewWriter(f, sstable.WithSync(false)) - if err != nil { - t.Fatal(err) - } - if err := w.Close(); err != nil { - t.Fatal(err) - } - } - - _, _, err := discoverSSTables(dir) - if err == nil { - t.Fatal("expected duplicate numeric SSTable IDs to fail discovery") - } -} +// discoverSSTables unit tests were removed alongside the helper itself. +// The manifest's FileID → SST filename mapping replaces filesystem +// discovery; filename validation happens implicitly through that mapping. // --- Get reads from SSTables + flush integration --- @@ -1116,44 +893,13 @@ func TestDB_AutoFlush_MultipleFlushes(t *testing.T) { } } -// 5. ReopenAfterAutoFlush — data recovered from SSTables, not just WAL. +// ReopenAfterAutoFlush — data recovered from SSTables, not just WAL. +// TODO: requires flush to write a VersionEdit to the manifest so the +// SST is rediscovered on reopen. Currently flush touches db.ssts in +// memory only; the manifest doesn't know the SST exists, so replay +// can't restore it. func TestDB_AutoFlush_ReopenAfterAutoFlush(t *testing.T) { - dir := t.TempDir() - db, err := Open(dir, WithSync(false), WithMemtableFlushSize(512)) - if err != nil { - t.Fatal(err) - } - ctx := context.Background() - writeNBytes(ctx, t, db, 1024) - if err := db.Close(); err != nil { - t.Fatalf("Close: %v", err) - } - - // Verify SST files exist before reopen - sstCount := countSSTFiles(t, dir) - if sstCount < 1 { - t.Fatalf("expected SST files after flush, got %d", sstCount) - } - - // Reopen — data should load from SSTables - db2, err := Open(dir, WithSync(false)) - if err != nil { - t.Fatalf("Reopen: %v", err) - } - defer db2.Close() - - if len(db2.ssts) < 1 { - t.Fatalf("expected SSTable readers after reopen, got %d", len(db2.ssts)) - } - - // Spot-check a key - val, err := db2.Get(ctx, []byte("key-000000")) - if err != nil { - t.Fatalf("Get(key-000000): %v", err) - } - if string(val) != "value-000000" { - t.Fatalf("Get(key-000000) = %q, want %q", val, "value-000000") - } + t.Skip("TODO: requires flush to record an AddFile VersionEdit per SST") } // 6. DeletesVisibleAfterFlush — tombstone propagation across flush boundaries. From 8ec9a32dd96733e4bef4a4133bb658f895ae9eba Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Tue, 26 May 2026 00:13:26 +0200 Subject: [PATCH 17/26] remove package comment from batch.go --- engine/batch.go | 1 - 1 file changed, 1 deletion(-) diff --git a/engine/batch.go b/engine/batch.go index f6a8fd1..bfe7c03 100644 --- a/engine/batch.go +++ b/engine/batch.go @@ -1,4 +1,3 @@ -// Package engine implements the public storage engine API package engine /* From 19ec25cba5384f29428eafb18f0131bd06694dac Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 29 May 2026 11:44:33 +0200 Subject: [PATCH 18/26] persist flushed SSTables via manifest VersionEdits --- engine/db.go | 161 ++++++++++-- engine/db_flush_test.go | 226 ++++++++++++++++- engine/db_manifest_test.go | 428 ++++++++++++++++++++++++++++++-- engine/errors.go | 4 + internal/crashhook/crashhook.go | 9 +- internal/keys/keys.go | 4 + internal/manifest/doc.go | 4 +- internal/memtable/doc.go | 6 +- internal/sstable/reader.go | 6 + internal/sstable/reader_test.go | 47 ++++ 10 files changed, 830 insertions(+), 65 deletions(-) diff --git a/engine/db.go b/engine/db.go index 6e788d3..eb25f63 100644 --- a/engine/db.go +++ b/engine/db.go @@ -524,11 +524,23 @@ func (db *DB) flushLoop(doneCh chan struct{}) { // Grab what we need under the lock imm := db.immMem + + // A concurrent rotation can hand off an empty memtable: two writers + // both pass the threshold check and queue on the flush slot, the + // first installs a fresh memtable, and the second freezes it before + // any write lands. Skip it — there is nothing to persist and an empty + // flush would record a zero-range file in the manifest. + if imm.Empty() { + db.immMem = nil + db.cond.Broadcast() + continue + } + sstPath := db.nextSSTPath() // Release the lock for I/O - this is where the work happens db.mu.Unlock() - newSSTableReader, err := writeSSTableFn(sstPath, imm, db.sstBlockSize) + flushResult, err := writeSSTableFn(sstPath, imm, db.sstBlockSize) // Re-acquire the lock to publish the results db.mu.Lock() @@ -540,7 +552,7 @@ func (db *DB) flushLoop(doneCh chan struct{}) { } // Publish the new SSTable reader and clear the immutable memtable. - if err := db.publishFlushedSSTLocked(newSSTableReader); err != nil { + if err := db.publishFlushedSSTLocked(flushResult); err != nil { db.flushErr = err db.cond.Broadcast() return @@ -633,14 +645,51 @@ func (db *DB) flushMemtable() error { } // publishFlushedSSTLocked publishes a successfully flushed SSTable under db.mu. -func (db *DB) publishFlushedSSTLocked(sstReader *sstable.Reader) error { +func (db *DB) publishFlushedSSTLocked(flushResult sstWriteResult) error { // FAILPOINT: sst_publish_error if err := crashhook.MaybeFault(crashhook.FaultSSTPublishError); err != nil { return fmt.Errorf("beachdb: publishing SSTable: %w", err) } + // Create a new version edit and sync manifest with latest changes. + versionEdit := manifest.VersionEdit{ + AddedFiles: []manifest.FileMetadata{ + { + Level: 0, + FileID: db.nextSSTID, + Size: flushResult.size, + SmallestKey: flushResult.smallest, + LargestKey: flushResult.largest, + }, + }, + HasNextFileID: true, + NextFileID: 1 + db.nextSSTID, + HasLastSequence: true, + LastSequence: db.seqno, + } + + // The SSTable is already durable (writeSSTable synced the file and its + // parent directory); the manifest edit has not been appended yet. + // FAILPOINT: manifest_after_sst_sync + crashhook.CrashIfArmed(crashhook.PointManifestAfterSSTSync) + + // Append the edit to the manifest --> this fsyncs the file + dir on disk! + if err := db.manifest.Append(versionEdit.Encode()); err != nil { + // The .sst file itself staying on disk is fine, it will be an orphan and will be + // deleted on the next `Open()`. nextSSTID isn't bumped so the id/path is reused. + _ = flushResult.reader.Close() + return fmt.Errorf("beachdb: appending flush edit to manifest: %w", err) + } + + // The manifest edit is durable; the in-memory version has not been + // updated yet. + // FAILPOINT: manifest_after_append + crashhook.CrashIfArmed(crashhook.PointManifestAfterAppend) + + // Update current version with the edit and append the sst reader to db.ssts + db.version = db.version.Apply(&versionEdit) db.flushErr = nil - db.ssts = append(db.ssts, sstReader) + db.ssts = append(db.ssts, flushResult.reader) db.immMem = nil db.nextSSTID++ @@ -922,18 +971,72 @@ func replayExistingManifest(db *DB, current string) error { return nil } +// sstWriteResult carries the outputs of writing a memtable to an on-disk +// SSTable: the open reader plus the metadata needed to record the new +// file in a manifest VersionEdit. +type sstWriteResult struct { + // reader is an open handle to the freshly written SSTable, ready to + // serve reads. + reader *sstable.Reader + + // smallest is the smallest InternalKey in the SSTable (first key in + // sorted order). + smallest keys.InternalKey + + // largest is the largest InternalKey in the SSTable (last key in + // sorted order). + largest keys.InternalKey + + // size is the SSTable file size in bytes. + size uint64 +} + +// writeMemtableEntries adds every entry of mem to writer in ascending +// InternalKey order, returning the smallest and largest keys written. The +// memtable yields keys in sorted order, so the first key is the smallest and +// the last is the largest. It returns ErrFlushEmptyMemtable when the memtable +// has no entries. +func writeMemtableEntries( + writer *sstable.Writer, + mem memtable.Memtable, +) (smallest, largest keys.InternalKey, err error) { + iter := mem.NewIterator() + iter.SeekToFirst() + + first := true + for iter.Valid() { + key := iter.Key() + if addErr := writer.Add(key, iter.Value()); addErr != nil { + _ = iter.Close() + return smallest, largest, fmt.Errorf("beachdb: writing entry to SSTable: %w", addErr) + } + if first { + smallest = key + first = false + } + largest = key + iter.Next() + } + _ = iter.Close() + + if first { + return smallest, largest, ErrFlushEmptyMemtable + } + return smallest, largest, nil +} + // Helper function for writing a memtable to an SSTable file on disk. // blockSize controls the target data block size; 0 means use the sstable default. -func writeSSTable(path string, mem memtable.Memtable, blockSize int) (*sstable.Reader, error) { +func writeSSTable(path string, mem memtable.Memtable, blockSize int) (sstWriteResult, error) { // FAILPOINT: sst_write_error if err := crashhook.MaybeFault(crashhook.FaultSSTWriteError); err != nil { - return nil, fmt.Errorf("beachdb: writing SSTable: %w", err) + return sstWriteResult{}, fmt.Errorf("beachdb: writing SSTable: %w", err) } // Create the new sstable file sstFile, err := os.Create(path) //nolint:gosec // path constructed from trusted db.dir + formatted ID if err != nil { - return nil, fmt.Errorf("%w: %w", ErrCreatingSSTFile, err) + return sstWriteResult{}, fmt.Errorf("%w: %w", ErrCreatingSSTFile, err) } defer sstFile.Close() @@ -947,31 +1050,27 @@ func writeSSTable(path string, mem memtable.Memtable, blockSize int) (*sstable.R writer, err := sstable.NewWriter(sstFile, writerOpts...) if err != nil { _ = os.Remove(path) //nolint:gosec // path is constructed from the trusted DB directory - return nil, fmt.Errorf("beachdb: creating SSTable writer: %w", err) + return sstWriteResult{}, fmt.Errorf("beachdb: creating SSTable writer: %w", err) } - // Iterate the immutable memtable and write entries to the SSTable - iter := mem.NewIterator() - iter.SeekToFirst() - for iter.Valid() { - if err := writer.Add(iter.Key(), iter.Value()); err != nil { - _ = writer.Close() - _ = iter.Close() - _ = os.Remove(path) //nolint:gosec // path is constructed from the trusted DB directory - return nil, fmt.Errorf("beachdb: writing entry to SSTable: %w", err) - } - iter.Next() + // Write every memtable entry to the SSTable, capturing the key range. + // An empty memtable is rejected (it would record a zero-value key range + // in the manifest). + smallestKey, largestKey, err := writeMemtableEntries(writer, mem) + if err != nil { + _ = writer.Close() + _ = os.Remove(path) //nolint:gosec // path is constructed from the trusted DB directory + return sstWriteResult{}, err } - _ = iter.Close() if err = writer.Close(); err != nil { _ = os.Remove(path) //nolint:gosec // path is constructed from the trusted DB directory - return nil, fmt.Errorf("beachdb: closing SSTable writer: %w", err) + return sstWriteResult{}, fmt.Errorf("beachdb: closing SSTable writer: %w", err) } // Sync parent directory so the new file's directory entry is durable if err = fs.SyncDir(filepath.Dir(path)); err != nil { - return nil, fmt.Errorf("beachdb: syncing directory after flush: %w", err) + return sstWriteResult{}, fmt.Errorf("beachdb: syncing directory after flush: %w", err) } // FAILPOINT: flush_after_file_sync @@ -980,17 +1079,29 @@ func writeSSTable(path string, mem memtable.Memtable, blockSize int) (*sstable.R // Re-open the file for reading sstFileReadMode, err := os.Open(path) //nolint:gosec // path constructed from trusted db.dir + formatted ID if err != nil { - return nil, fmt.Errorf("beachdb: opening SSTable for reading: %w", err) + return sstWriteResult{}, fmt.Errorf("beachdb: opening SSTable for reading: %w", err) } // Create a reader for the newest sstable sstReader, err := sstable.OpenReader(sstFileReadMode) if err != nil { _ = sstFileReadMode.Close() - return nil, fmt.Errorf("beachdb: error reading newly flushed SSTable: %w", err) + return sstWriteResult{}, fmt.Errorf("beachdb: error reading newly flushed SSTable: %w", err) } - return sstReader, nil + fileSize := sstReader.FileSize() + if fileSize < 0 { + _ = sstReader.Close() + return sstWriteResult{}, fmt.Errorf("beachdb: SSTable %q reports negative size %d", path, fileSize) + } + + result := sstWriteResult{ + reader: sstReader, + smallest: smallestKey, + largest: largestKey, + size: uint64(fileSize), + } + return result, nil } // buildSSTFileName builds an SSTable file name from a file ID, diff --git a/engine/db_flush_test.go b/engine/db_flush_test.go index b946558..348e503 100644 --- a/engine/db_flush_test.go +++ b/engine/db_flush_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/aalhour/beachdb/internal/keys" "github.com/aalhour/beachdb/internal/memtable" "github.com/aalhour/beachdb/internal/sstable" ) @@ -340,8 +341,8 @@ func TestFlush_SynchronousFailurePreservesReadableData(t *testing.T) { flushFailure := errors.New("forced flush failure") prevWriteSSTableFn := writeSSTableFn - writeSSTableFn = func(string, memtable.Memtable, int) (*sstable.Reader, error) { - return nil, flushFailure + writeSSTableFn = func(string, memtable.Memtable, int) (sstWriteResult, error) { + return sstWriteResult{}, flushFailure } t.Cleanup(func() { writeSSTableFn = prevWriteSSTableFn @@ -893,13 +894,57 @@ func TestDB_AutoFlush_MultipleFlushes(t *testing.T) { } } -// ReopenAfterAutoFlush — data recovered from SSTables, not just WAL. -// TODO: requires flush to write a VersionEdit to the manifest so the -// SST is rediscovered on reopen. Currently flush touches db.ssts in -// memory only; the manifest doesn't know the SST exists, so replay -// can't restore it. +// ReopenAfterAutoFlush — data recovered from SSTables, not just WAL. After +// auto-flush plus an explicit final flush, the active memtable is empty and +// every key lives in an SST tracked by the manifest. Deleting the WAL before +// reopen proves recovery comes from the manifest+SSTs. func TestDB_AutoFlush_ReopenAfterAutoFlush(t *testing.T) { - t.Skip("TODO: requires flush to record an AddFile VersionEdit per SST") + dir := t.TempDir() + db, err := Open(dir, WithSync(false), WithMemtableFlushSize(256)) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + + known := []string{"alpha", "beta", "gamma"} + for _, k := range known { + if err := db.Put(ctx, []byte(k), []byte("val-"+k)); err != nil { + t.Fatalf("Put(%s): %v", k, err) + } + } + // Drive several auto-flushes, then flush the remainder so nothing is + // left in the active memtable. + writeNBytes(ctx, t, db, 1024) + if err := db.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Remove the WAL so recovery must come from the SSTs via the manifest. + if err := os.Remove(filepath.Join(dir, walFileName)); err != nil { + t.Fatalf("removing WAL: %v", err) + } + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen: %v", err) + } + defer db2.Close() + + if len(db2.version.AllFiles()) == 0 { + t.Fatal("no SSTs in Version after reopen; auto-flush did not record manifest edits") + } + for _, k := range known { + got, err := db2.Get(ctx, []byte(k)) + if err != nil { + t.Fatalf("Get(%s) after WAL deletion: %v", k, err) + } + if string(got) != "val-"+k { + t.Errorf("Get(%s) = %q, want %q", k, got, "val-"+k) + } + } } // 6. DeletesVisibleAfterFlush — tombstone propagation across flush boundaries. @@ -1002,8 +1047,8 @@ func TestDB_AutoFlush_FailureStopsFurtherWrites(t *testing.T) { flushFailure := errors.New("forced auto-flush failure") prevWriteSSTableFn := writeSSTableFn - writeSSTableFn = func(string, memtable.Memtable, int) (*sstable.Reader, error) { - return nil, flushFailure + writeSSTableFn = func(string, memtable.Memtable, int) (sstWriteResult, error) { + return sstWriteResult{}, flushFailure } t.Cleanup(func() { writeSSTableFn = prevWriteSSTableFn @@ -1195,3 +1240,164 @@ func TestDB_AutoFlush_ConcurrentReadWrite(t *testing.T) { } } } + +// --------------------------------------------------------------------------- +// writeSSTable: key-range/size capture and empty-memtable guard +// --------------------------------------------------------------------------- + +// writeSSTable records the smallest/largest InternalKey and the on-disk file +// size in its result, so the flush path can build an accurate manifest edit. +func TestWriteSSTable_CapturesKeyRangeAndSize(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, buildSSTFileName(1)) + + mem := memtable.NewSkipList() + mem.Put(putInternalKey("m", 5), []byte("v-m")) + mem.Put(putInternalKey("a", 3), []byte("v-a")) + mem.Put(putInternalKey("z", 7), []byte("v-z")) + + res, err := writeSSTable(path, mem, 0) + if err != nil { + t.Fatalf("writeSSTable: %v", err) + } + defer res.reader.Close() + + if got := string(res.smallest.UserKey); got != "a" { + t.Errorf("smallest.UserKey = %q, want %q", got, "a") + } + if got := string(res.largest.UserKey); got != "z" { + t.Errorf("largest.UserKey = %q, want %q", got, "z") + } + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + //nolint:gosec // G115: SST file size is bounded by test fixture + if res.size != uint64(info.Size()) { + t.Errorf("size = %d, want %d (on-disk size)", res.size, info.Size()) + } +} + +// writeSSTable refuses an empty memtable: an empty flush would record a +// zero-value key range in the manifest. It must leave no orphan SST behind. +func TestWriteSSTable_RejectsEmptyMemtable(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, buildSSTFileName(1)) + + _, err := writeSSTable(path, memtable.NewSkipList(), 0) + if !errors.Is(err, ErrFlushEmptyMemtable) { + t.Fatalf("writeSSTable(empty) = %v, want ErrFlushEmptyMemtable", err) + } + if dirContains(t, dir, buildSSTFileName(1)) { + t.Errorf("empty flush left an orphan SST file on disk") + } +} + +// A single-entry memtable is the boundary where the first key is also the +// last: smallest and largest must both be that key, full InternalKey (seqno +// and kind) included, not just the user key. +func TestWriteSSTable_SingleKey(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, buildSSTFileName(1)) + + mem := memtable.NewSkipList() + mem.Put(putInternalKey("solo", 9), []byte("only")) + + res, err := writeSSTable(path, mem, 0) + if err != nil { + t.Fatalf("writeSSTable: %v", err) + } + defer res.reader.Close() + + if string(res.smallest.UserKey) != "solo" || string(res.largest.UserKey) != "solo" { + t.Errorf("single-key range: smallest=%q largest=%q, want both %q", + res.smallest.UserKey, res.largest.UserKey, "solo") + } + if res.smallest.Seqno != 9 || res.largest.Seqno != 9 { + t.Errorf("boundary seqno: smallest=%d largest=%d, want 9", res.smallest.Seqno, res.largest.Seqno) + } +} + +// Two entries share a user key but differ by sequence number. InternalKey +// ordering is user key ascending then seqno descending, so the boundary keys +// carry the same user key with different seqnos — the capture must preserve +// the full InternalKey, not collapse to the user key. +func TestWriteSSTable_DuplicateUserKey_DifferentSeqno(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, buildSSTFileName(1)) + + mem := memtable.NewSkipList() + mem.Put(putInternalKey("dup", 5), []byte("newer")) + mem.Put(putInternalKey("dup", 3), []byte("older")) + + res, err := writeSSTable(path, mem, 0) + if err != nil { + t.Fatalf("writeSSTable: %v", err) + } + defer res.reader.Close() + + if string(res.smallest.UserKey) != "dup" || string(res.largest.UserKey) != "dup" { + t.Errorf("user keys: smallest=%q largest=%q, want both %q", + res.smallest.UserKey, res.largest.UserKey, "dup") + } + // Higher seqno sorts first (smallest), lower seqno sorts last (largest). + if res.smallest.Seqno != 5 { + t.Errorf("smallest.Seqno = %d, want 5", res.smallest.Seqno) + } + if res.largest.Seqno != 3 { + t.Errorf("largest.Seqno = %d, want 3", res.largest.Seqno) + } +} + +// A tombstone (Delete-kind entry) can be a range boundary. The captured +// largest key must preserve the Delete kind so the manifest range reflects it. +func TestWriteSSTable_TombstoneBoundary(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, buildSSTFileName(1)) + + mem := memtable.NewSkipList() + mem.Put(putInternalKey("apple", 1), []byte("v")) + mem.Put(keys.InternalKey{UserKey: []byte("zebra"), Seqno: 2, Kind: keys.InternalKeyKindDelete}, nil) + + res, err := writeSSTable(path, mem, 0) + if err != nil { + t.Fatalf("writeSSTable: %v", err) + } + defer res.reader.Close() + + if string(res.smallest.UserKey) != "apple" { + t.Errorf("smallest.UserKey = %q, want %q", res.smallest.UserKey, "apple") + } + if string(res.largest.UserKey) != "zebra" { + t.Errorf("largest.UserKey = %q, want %q", res.largest.UserKey, "zebra") + } + if res.largest.Kind != keys.InternalKeyKindDelete { + t.Errorf("largest.Kind = %d, want Delete (%d) preserved in boundary", + res.largest.Kind, keys.InternalKeyKindDelete) + } +} + +// An empty user key is valid and sorts before any non-empty key, so it must be +// captured as the smallest boundary. +func TestWriteSSTable_EmptyUserKey(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, buildSSTFileName(1)) + + mem := memtable.NewSkipList() + mem.Put(putInternalKey("", 1), []byte("empty-key-val")) + mem.Put(putInternalKey("b", 2), []byte("v")) + + res, err := writeSSTable(path, mem, 0) + if err != nil { + t.Fatalf("writeSSTable: %v", err) + } + defer res.reader.Close() + + if len(res.smallest.UserKey) != 0 { + t.Errorf("smallest.UserKey = %q, want empty", res.smallest.UserKey) + } + if string(res.largest.UserKey) != "b" { + t.Errorf("largest.UserKey = %q, want %q", res.largest.UserKey, "b") + } +} diff --git a/engine/db_manifest_test.go b/engine/db_manifest_test.go index 809976a..b11d2a8 100644 --- a/engine/db_manifest_test.go +++ b/engine/db_manifest_test.go @@ -1,6 +1,7 @@ package engine import ( + "context" "errors" "fmt" "io" @@ -124,11 +125,11 @@ func createRealSST(t *testing.T, dir string, fileID uint64) uint64 { Kind: keys.InternalKeyKindPut, }, []byte("v")) - reader, err := writeSSTable(sstPath, mem, 0) + res, err := writeSSTable(sstPath, mem, 0) if err != nil { t.Fatalf("writeSSTable: %v", err) } - _ = reader.Close() + _ = res.reader.Close() info, err := os.Stat(sstPath) if err != nil { @@ -469,7 +470,7 @@ func TestBootstrap_NextOpenTakesReplayPath(t *testing.T) { // If bootstrap crashes between Append and WriteCurrent, the orphan // MANIFEST file is present but CURRENT is not. The next Open should -// treat the directory as fresh (scenario 2) and continue. +// treat the directory as fresh and continue. func TestBootstrap_CurrentInstallIsLast_SimulatedOrphan(t *testing.T) { dir := t.TempDir() // Plant an orphan MANIFEST with no CURRENT. @@ -719,12 +720,45 @@ func TestOpen_PostStateInstalled(t *testing.T) { } } -// WAL replay continues from the manifest's LastSequence. -// TODO: needs a flush that checkpoints seqno into the manifest before this -// can produce a meaningful baseline. Currently flush does not write a -// VersionEdit, so the seqno-in-manifest path is unreachable. +// Open replays the manifest first (loading flushed SSTs) and then the WAL on +// top, so a newer un-flushed write shadows the older flushed value for the +// same key: the manifest wins on file existence, the WAL wins on recent data. func TestOpen_OrderingInvariant_ManifestBeforeWAL(t *testing.T) { - t.Skip("TODO: requires flush to checkpoint LastSequence in the manifest") + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + ctx := context.Background() + + // Older value flushed into an SST (recorded in the manifest). + if err := db.Put(ctx, []byte("k"), []byte("old-from-sst")); err != nil { + t.Fatalf("Put(old): %v", err) + } + if err := db.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + // Newer value for the same key, left un-flushed in the WAL only. + if err := db.Put(ctx, []byte("k"), []byte("new-from-wal")); err != nil { + t.Fatalf("Put(new): %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen: %v", err) + } + defer db2.Close() + + got, err := db2.Get(ctx, []byte("k")) + if err != nil { + t.Fatalf("Get: %v", err) + } + if string(got) != "new-from-wal" { + t.Errorf("Get = %q, want %q (WAL replay must shadow the older SST value)", got, "new-from-wal") + } } // --------------------------------------------------------------------------- @@ -781,44 +815,390 @@ func TestClose_DoubleClose_WithManifest(t *testing.T) { } // --------------------------------------------------------------------------- -// Flush + cross-restart durability (skipped pending wiring) +// Flush + cross-restart durability // -// TODO: these tests require publishFlushedSSTLocked to write a VersionEdit -// to the manifest on every successful flush, plus matching crashhook -// points around the SST-sync → manifest-append boundary. Until that -// wiring lands, flush is invisible to the manifest and these scenarios -// can't be exercised. +// A successful flush appends a VersionEdit to the manifest (AddFile + +// NextFileID + LastSequence). These tests exercise that the file set, SST id +// counter, and sequence number all survive a close/reopen via the manifest. // --------------------------------------------------------------------------- +// A successful flush appends an AddFile VersionEdit to the manifest. After +// reopen the file is visible in the replayed Version with its captured key +// range and a non-zero size. func TestFlush_WritesManifestEdit(t *testing.T) { - t.Skip("TODO: requires flush to write a VersionEdit per flush") + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + ctx := context.Background() + for _, k := range []string{"banana", "apple", "cherry"} { + if err := db.Put(ctx, []byte(k), []byte("v-"+k)); err != nil { + t.Fatalf("Put(%s): %v", k, err) + } + } + if err := db.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen: %v", err) + } + defer db2.Close() + + files := db2.version.AllFiles() + if len(files) != 1 { + t.Fatalf("Version.AllFiles() = %d files, want 1", len(files)) + } + f := files[0] + if f.Size == 0 { + t.Errorf("FileMetadata.Size = 0, want > 0") + } + if got := string(f.SmallestKey.UserKey); got != "apple" { + t.Errorf("SmallestKey = %q, want %q", got, "apple") + } + if got := string(f.LargestKey.UserKey); got != "cherry" { + t.Errorf("LargestKey = %q, want %q", got, "cherry") + } } + +// The SSTable is synced to disk before the manifest edit is appended, so a +// crash in between leaves an orphan SST the manifest never references. +// Exercising the crash itself needs the out-of-process crash harness; the +// crashhook point (PointManifestAfterSSTSync) is wired in +// publishFlushedSSTLocked for that harness. func TestFlush_OrderingInvariant_SSTBeforeManifest(t *testing.T) { - t.Skip("TODO: requires crashhook points around SST-sync → manifest-append") + t.Skip("TODO: crash point wired; scenario needs the out-of-process crash harness") } + +// When the manifest Append fails, flush returns an error, the open SST reader +// is released, and the file is left as an orphan: the Version does not gain +// the file and nextSSTID is not advanced, so the next flush reuses the id. func TestFlush_ManifestAppendFailure_SSTOrphaned(t *testing.T) { - t.Skip("TODO: requires a fault-injection point for manifest.Append") + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + ctx := context.Background() + if err := db.Put(ctx, []byte("k"), []byte("v")); err != nil { + t.Fatalf("Put: %v", err) + } + + // Force the manifest Append to fail by closing the writer underneath + // the flush path. + if err := db.manifest.Close(); err != nil { + t.Fatalf("closing manifest writer: %v", err) + } + + prevSSTID := db.nextSSTID + if err := db.Flush(); err == nil { + t.Fatal("Flush succeeded, want manifest append error") + } + + if got := len(db.version.AllFiles()); got != 0 { + t.Errorf("Version.AllFiles() = %d, want 0 (edit must not apply on append failure)", got) + } + if db.nextSSTID != prevSSTID { + t.Errorf("nextSSTID advanced to %d, want %d (no advance on append failure)", db.nextSSTID, prevSSTID) + } + if !dirContains(t, dir, buildSSTFileName(prevSSTID)) { + t.Errorf("expected orphan SST %s on disk", buildSSTFileName(prevSSTID)) + } } + +// NextFileID recorded in the flush edit is restored on reopen, so SST ids keep +// climbing across restarts instead of resetting and overwriting files. func TestFlush_NextSSTIDPersistedViaManifest(t *testing.T) { - t.Skip("TODO: requires flush to record NextFileID in the manifest") + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + ctx := context.Background() + if err := db.Put(ctx, []byte("k"), []byte("v")); err != nil { + t.Fatalf("Put: %v", err) + } + if err := db.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen: %v", err) + } + defer db2.Close() + + // One flush starting at id 1 ⇒ next available id is 2. + if db2.nextSSTID != 2 { + t.Errorf("nextSSTID after reopen = %d, want 2", db2.nextSSTID) + } } + +// LastSequence checkpointed by a flush is restored on reopen. Deleting the WAL +// isolates the manifest as the only seqno source. func TestFlush_LastSequencePersistedViaManifest(t *testing.T) { - t.Skip("TODO: requires flush to checkpoint LastSequence in the manifest") + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + ctx := context.Background() + const nPuts = 4 + for i := range nPuts { + k := fmt.Sprintf("key-%d", i) + if err := db.Put(ctx, []byte(k), []byte("v")); err != nil { + t.Fatalf("Put(%s): %v", k, err) + } + } + if err := db.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Remove the WAL so the manifest checkpoint is the sole seqno source. + if err := os.Remove(filepath.Join(dir, walFileName)); err != nil { + t.Fatalf("removing WAL: %v", err) + } + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen: %v", err) + } + defer db2.Close() + + if db2.seqno != nPuts { + t.Errorf("seqno after reopen = %d, want %d (manifest LastSequence)", db2.seqno, nPuts) + } } + +// Several flushes each append an AddFile edit; all files are present in the +// replayed Version and every key is readable after reopen. func TestFlush_Multiple_AllRecoverable(t *testing.T) { - t.Skip("TODO: requires flush to write a VersionEdit per flush") + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + ctx := context.Background() + wantKeys := []string{"alpha", "bravo", "charlie"} + for _, k := range wantKeys { + if err := db.Put(ctx, []byte(k), []byte("v-"+k)); err != nil { + t.Fatalf("Put(%s): %v", k, err) + } + if err := db.Flush(); err != nil { + t.Fatalf("Flush(%s): %v", k, err) + } + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen: %v", err) + } + defer db2.Close() + + if got := len(db2.version.AllFiles()); got != len(wantKeys) { + t.Errorf("Version.AllFiles() = %d, want %d", got, len(wantKeys)) + } + for _, k := range wantKeys { + got, err := db2.Get(ctx, []byte(k)) + if err != nil { + t.Fatalf("Get(%s): %v", k, err) + } + if string(got) != "v-"+k { + t.Errorf("Get(%s) = %q, want %q", k, got, "v-"+k) + } + } } + +// After a flush, the data lives in the SSTable. Deleting the WAL before reopen +// proves recovery comes from the manifest+SST, not the WAL. func TestRestart_DataFromSSTOnly(t *testing.T) { - t.Skip("TODO: requires flush wiring so SSTs are recoverable without the WAL") + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + ctx := context.Background() + if err := db.Put(ctx, []byte("durable"), []byte("survives")); err != nil { + t.Fatalf("Put: %v", err) + } + if err := db.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + if err := os.Remove(filepath.Join(dir, walFileName)); err != nil { + t.Fatalf("removing WAL: %v", err) + } + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen: %v", err) + } + defer db2.Close() + + got, err := db2.Get(ctx, []byte("durable")) + if err != nil { + t.Fatalf("Get after WAL deletion: %v", err) + } + if string(got) != "survives" { + t.Errorf("Get = %q, want %q", got, "survives") + } } + +// On reopen, flushed data comes from the SST (manifest) and un-flushed data +// comes from replaying the WAL on top. func TestRestart_DataFromSSTAndWAL(t *testing.T) { - t.Skip("TODO: requires flush wiring for the post-checkpoint WAL path") + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + ctx := context.Background() + + // Flushed key ⇒ ends up in an SST tracked by the manifest. + if err := db.Put(ctx, []byte("flushed"), []byte("from-sst")); err != nil { + t.Fatalf("Put(flushed): %v", err) + } + if err := db.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + // Un-flushed key ⇒ only in the WAL + active memtable. + if err := db.Put(ctx, []byte("buffered"), []byte("from-wal")); err != nil { + t.Fatalf("Put(buffered): %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen: %v", err) + } + defer db2.Close() + + for k, want := range map[string]string{"flushed": "from-sst", "buffered": "from-wal"} { + got, err := db2.Get(ctx, []byte(k)) + if err != nil { + t.Fatalf("Get(%s): %v", k, err) + } + if string(got) != want { + t.Errorf("Get(%s) = %q, want %q", k, got, want) + } + } } + +// After a flush + reopen, the next flush must use a fresh SST id rather than +// reusing id 1 and overwriting the first file. func TestRestart_NextSSTID_NoCollisionsAfterCrash(t *testing.T) { - t.Skip("TODO: requires flush to checkpoint NextFileID across restarts") + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + ctx := context.Background() + if err := db.Put(ctx, []byte("first"), []byte("1")); err != nil { + t.Fatalf("Put(first): %v", err) + } + if err := db.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen: %v", err) + } + if err := db2.Put(ctx, []byte("second"), []byte("2")); err != nil { + t.Fatalf("Put(second): %v", err) + } + if err := db2.Flush(); err != nil { + t.Fatalf("Flush after reopen: %v", err) + } + if err := db2.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Both SSTs must coexist on disk under distinct ids. + if !dirContains(t, dir, buildSSTFileName(1)) { + t.Errorf("first SST %s missing", buildSSTFileName(1)) + } + if !dirContains(t, dir, buildSSTFileName(2)) { + t.Errorf("second SST %s missing (id collision?)", buildSSTFileName(2)) + } + + db3, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen 2: %v", err) + } + defer db3.Close() + if got := len(db3.version.AllFiles()); got != 2 { + t.Errorf("Version.AllFiles() = %d, want 2", got) + } } + +// Sequence numbers stay monotonic across a flush + restart: a post-restart +// write wins over the pre-restart value for the same key. func TestRestart_SeqnoMonotonic(t *testing.T) { - t.Skip("TODO: requires flush to checkpoint LastSequence across restarts") + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + ctx := context.Background() + if err := db.Put(ctx, []byte("k"), []byte("old")); err != nil { + t.Fatalf("Put(old): %v", err) + } + if err := db.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Drop the WAL so seqno is seeded solely from the manifest checkpoint. + if err := os.Remove(filepath.Join(dir, walFileName)); err != nil { + t.Fatalf("removing WAL: %v", err) + } + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen: %v", err) + } + defer db2.Close() + + seqnoBefore := db2.seqno + if err := db2.Put(ctx, []byte("k"), []byte("new")); err != nil { + t.Fatalf("Put(new): %v", err) + } + if db2.seqno <= seqnoBefore { + t.Errorf("seqno did not advance: before=%d after=%d", seqnoBefore, db2.seqno) + } + + got, err := db2.Get(ctx, []byte("k")) + if err != nil { + t.Fatalf("Get: %v", err) + } + if string(got) != "new" { + t.Errorf("Get = %q, want %q (post-restart write must win)", got, "new") + } } // --------------------------------------------------------------------------- diff --git a/engine/errors.go b/engine/errors.go index 9608a8d..f7e1500 100644 --- a/engine/errors.go +++ b/engine/errors.go @@ -29,4 +29,8 @@ var ( // ErrInvalidSSTBlockSize indicates that the SSTable block size is invalid. ErrInvalidSSTBlockSize = errors.New("beachdb: invalid SSTable block size, must be >= 0") + + // ErrFlushEmptyMemtable indicates an attempt to flush an empty memtable, + // which would record a zero-value key range in the manifest. + ErrFlushEmptyMemtable = errors.New("beachdb: refusing to flush empty memtable") ) diff --git a/internal/crashhook/crashhook.go b/internal/crashhook/crashhook.go index d7e4026..87faa6f 100644 --- a/internal/crashhook/crashhook.go +++ b/internal/crashhook/crashhook.go @@ -22,6 +22,12 @@ const ( PointFlushAfterFileSync = "flush_after_file_sync" // PointFlushAfterPublish crashes after a flushed SSTable is published in memory. PointFlushAfterPublish = "flush_after_publish" + // PointManifestAfterSSTSync crashes after the SSTable is durable but before the manifest + // edit is appended. + PointManifestAfterSSTSync = "manifest_after_sst_sync" + // PointManifestAfterAppend crashes after the manifest edit is appended and synced but before + // the in-memory version is updated. + PointManifestAfterAppend = "manifest_after_append" // FaultWALSyncError injects a deterministic WAL sync failure. FaultWALSyncError = "wal_sync_error" @@ -53,7 +59,8 @@ var ( // IsCrashPoint reports whether point names a supported crash point. func IsCrashPoint(point string) bool { switch point { - case "", PointWALAfterAppend, PointWALAfterSync, PointFlushAfterFileSync, PointFlushAfterPublish: + case "", PointWALAfterAppend, PointWALAfterSync, PointFlushAfterFileSync, PointFlushAfterPublish, + PointManifestAfterSSTSync, PointManifestAfterAppend: return true default: return false diff --git a/internal/keys/keys.go b/internal/keys/keys.go index 66b1f8c..226346a 100644 --- a/internal/keys/keys.go +++ b/internal/keys/keys.go @@ -38,9 +38,13 @@ func (k InternalKey) Compare(other InternalKey) int { if cmp != 0 { return cmp } + // Larger sequence number means that `k` is newer, which means + // "recent/smaller", hence -1 if k.Seqno > other.Seqno { return -1 } + // Smaller sequence number means that `k` is older, which means + // "older/bigger", hence 1 if k.Seqno < other.Seqno { return 1 } diff --git a/internal/manifest/doc.go b/internal/manifest/doc.go index 9deb05f..b95f261 100644 --- a/internal/manifest/doc.go +++ b/internal/manifest/doc.go @@ -119,8 +119,8 @@ // // # Current simplifications // -// The following features are intentionally omitted from v1 and will land -// in later milestones: +// The following features are intentionally omitted from v1 and may land +// in future work: // // - No manifest rotation (the file grows unbounded for now) // - No skip-unknown tag support (unknown tags hard-error) diff --git a/internal/memtable/doc.go b/internal/memtable/doc.go index 702fc09..d01b25c 100644 --- a/internal/memtable/doc.go +++ b/internal/memtable/doc.go @@ -57,9 +57,9 @@ // until Close is called // // This means iterators block writers. It's a deliberate v1 simplicity choice — -// the frozen-memtable pattern (Milestone 4) will route new writes to a fresh -// memtable while the old one drains, so iterator lock contention becomes a -// non-issue in practice. +// the frozen-memtable flush pattern routes new writes to a fresh memtable +// while the old one drains, so iterator lock contention becomes a non-issue +// in practice. // // IMPORTANT: Callers MUST call Iterator.Close() to release the lock. Forgetting // this will deadlock writers indefinitely. The compiler won't save you here. diff --git a/internal/sstable/reader.go b/internal/sstable/reader.go index 13a4ccc..1e1fee1 100644 --- a/internal/sstable/reader.go +++ b/internal/sstable/reader.go @@ -86,6 +86,12 @@ func (r *Reader) Close() error { return nil } +// FileSize returns the SSTable file size in bytes. It returns int64 to +// match the os file-size API (os.FileInfo.Size). +func (r *Reader) FileSize() int64 { + return r.fileSize +} + // EntryCount returns the total number of key-value entries in the SSTable. func (r *Reader) EntryCount() uint64 { return r.footer.entryCount diff --git a/internal/sstable/reader_test.go b/internal/sstable/reader_test.go index de3e979..2d343b9 100644 --- a/internal/sstable/reader_test.go +++ b/internal/sstable/reader_test.go @@ -782,6 +782,53 @@ func TestReader_EntryCount_Empty(t *testing.T) { } } +func TestReader_FileSize(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "filesize.sst") + + entries := []struct { + key keys.InternalKey + value []byte + }{ + {putKey("a", 3), []byte("v1")}, + {putKey("b", 2), []byte("v2")}, + {putKey("c", 1), []byte("v3")}, + } + writeSSTable(t, path, entries) + + r := openReader(t, path) + defer r.Close() + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if got := r.FileSize(); got != info.Size() { + t.Fatalf("FileSize() = %d, want %d (on-disk size)", got, info.Size()) + } +} + +func TestReader_FileSize_Empty(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "filesize-empty.sst") + + writeSSTable(t, path, nil) + + r := openReader(t, path) + defer r.Close() + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if got := r.FileSize(); got != info.Size() { + t.Fatalf("FileSize() = %d, want %d (on-disk size)", got, info.Size()) + } + if r.FileSize() <= 0 { + t.Errorf("FileSize() = %d, want > 0 (footer/index always present)", r.FileSize()) + } +} + func TestReader_DataBlockCount(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "blocks.sst") From b9e5765a4aeb882bd1e5d0ec338cf306fe9a9080 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 29 May 2026 12:12:46 +0200 Subject: [PATCH 19/26] write the manifest_dump tool and doc under cmd/ --- cmd/manifest_dump/README.md | 88 ++++++++++++++++++++- cmd/manifest_dump/main.go | 147 +++++++++++++++++++++++++++++++++++- docs/formats/manifest.md | 41 ++++++---- 3 files changed, 260 insertions(+), 16 deletions(-) diff --git a/cmd/manifest_dump/README.md b/cmd/manifest_dump/README.md index 9792f6f..16fa3a9 100644 --- a/cmd/manifest_dump/README.md +++ b/cmd/manifest_dump/README.md @@ -1,6 +1,8 @@ # manifest_dump CLI tool for inspecting BeachDB's MANIFEST files without running the database. +It reads `CURRENT`, replays the manifest it points at, prints every +`VersionEdit` in order, and then prints the reconstructed `Version`. ## Build @@ -12,8 +14,90 @@ go build -o bin/manifest_dump ./cmd/manifest_dump ## Usage -TODO: Add more details once implemented. +``` +manifest_dump +``` + +The argument is the database directory (the one containing `CURRENT` and the +`MANIFEST-NNNNNN` files), not a manifest file path. ## Examples -TODO: Add more details once implemented. +### Normal database + +```sh +$ manifest_dump /tmp/mydb +``` + +``` +Manifest: MANIFEST-000001 +Path: /tmp/mydb/MANIFEST-000001 + +Edit #0: + next_file_id: 1 + last_sequence: 0 + log_number: 0 +Edit #1: + next_file_id: 2 + last_sequence: 3 + add_file: level=0 id=1 size=170 smallest="apple/2/Put" largest="cherry/3/Put" +Edit #2: + next_file_id: 3 + last_sequence: 5 + add_file: level=0 id=2 size=126 smallest="apple/5/Delete" largest="kiwi/4/Put" +Current Version: + Level 0: 2 files (296 bytes total) + [1] apple..cherry (170 bytes) + [2] apple..kiwi (126 bytes) +``` + +Each edit prints only the fields it sets. `add_file` keys are shown as +`//` (the same convention as `sst_dump`), where kind is +`Put` or `Delete`. + +### Fresh database + +A directory with no `CURRENT` file is a fresh database — there is nothing to +dump, and the tool exits 0: + +```sh +$ manifest_dump /tmp/empty-dir +No CURRENT file in /tmp/empty-dir — fresh database, nothing to dump. +``` + +### Truncated manifest + +A partial trailing record (a crash mid-append) is recoverable. The tool notes +it, prints the `Version` reconstructed from the complete edits, and exits 0: + +``` +Edit #2: incomplete trailing record (crash mid-append, recoverable) + +Current Version: + Level 0: 1 files (170 bytes total) + [1] apple..cherry (170 bytes) +``` + +### Corrupt manifest + +A checksum mismatch or undecodable edit is fatal. The tool prints the `Version` +reconstructed up to the failure, reports the last valid edit, and exits +non-zero: + +```sh +$ manifest_dump /tmp/mydb +... +Current Version: + Level 0: 1 files (170 bytes total) + [1] apple..cherry (170 bytes) + +Last valid edit: #1 +Error: corrupt manifest at edit #2: beachdb/record: checksum mismatch +``` + +## Exit codes + +| Code | Meaning | +|------|----------------------------------------------------------------| +| `0` | Manifest dumped (or fresh database, or recoverable truncation) | +| `1` | Bad arguments, unreadable directory, or a corrupt manifest | diff --git a/cmd/manifest_dump/main.go b/cmd/manifest_dump/main.go index a041e47..9cda340 100644 --- a/cmd/manifest_dump/main.go +++ b/cmd/manifest_dump/main.go @@ -1,6 +1,151 @@ // Package main provides the manifest_dump CLI tool for inspecting MANIFEST files. package main -// main is the entry point. The manifest_dump tool is not yet implemented. +import ( + "errors" + "flag" + "fmt" + "io" + "os" + "path/filepath" + + "github.com/aalhour/beachdb/internal/keys" + "github.com/aalhour/beachdb/internal/manifest" + "github.com/aalhour/beachdb/internal/record" +) + +// main is the entry point. It dumps the live manifest of the database +// directory named by the single positional argument. func main() { + flag.Usage = func() { + fmt.Fprintf(os.Stderr, "Usage: manifest_dump \n") + } + flag.Parse() + + if flag.NArg() != 1 { + flag.Usage() + os.Exit(1) + } + + if err := dumpManifest(flag.Arg(0)); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } +} + +// dumpManifest reads CURRENT in dir, replays the manifest it points at, prints +// each VersionEdit and the final Version summary. A missing CURRENT is treated +// as a fresh database (not an error). A corrupt manifest prints the failing +// edit and returns an error so the process exits non-zero. +func dumpManifest(dir string) error { + name, err := manifest.ReadCurrent(dir) + if errors.Is(err, manifest.ErrNoCurrentFile) { + fmt.Printf("No CURRENT file in %s — fresh database, nothing to dump.\n", dir) + return nil + } + if err != nil { + return fmt.Errorf("reading CURRENT: %w", err) + } + + manifestPath := filepath.Join(dir, name) + fmt.Printf("Manifest: %s\n", name) + fmt.Printf("Path: %s\n\n", manifestPath) + + reader, err := manifest.NewReader(manifestPath) + if err != nil { + return fmt.Errorf("opening manifest: %w", err) + } + defer reader.Close() + + version := manifest.NewVersion(0) + editIdx := 0 + var corruptErr error + + for ; ; editIdx++ { + edit, err := reader.NextEdit() + if errors.Is(err, io.EOF) { + break + } + if errors.Is(err, record.ErrTruncated) { + fmt.Printf("Edit #%d: incomplete trailing record (crash mid-append, recoverable)\n\n", editIdx) + break + } + if err != nil { + corruptErr = fmt.Errorf("corrupt manifest at edit #%d: %w", editIdx, err) + break + } + + printEdit(editIdx, edit) + version = version.Apply(edit) + } + + printVersion(version) + + if corruptErr != nil { + fmt.Printf("\nLast valid edit: #%d\n", editIdx-1) + return corruptErr + } + return nil +} + +// printEdit renders a single VersionEdit in human-readable form, emitting only +// the fields the edit actually sets. +func printEdit(idx int, edit *manifest.VersionEdit) { + fmt.Printf("Edit #%d:\n", idx) + if edit.HasNextFileID { + fmt.Printf(" next_file_id: %d\n", edit.NextFileID) + } + if edit.HasLastSequence { + fmt.Printf(" last_sequence: %d\n", edit.LastSequence) + } + if edit.HasLogNumber { + fmt.Printf(" log_number: %d\n", edit.LogNumber) + } + for _, f := range edit.AddedFiles { + fmt.Printf(" add_file: level=%d id=%d size=%d smallest=%q largest=%q\n", + f.Level, f.FileID, f.Size, formatInternalKey(f.SmallestKey), formatInternalKey(f.LargestKey)) + } + for _, d := range edit.DeletedFiles { + fmt.Printf(" delete_file: level=%d id=%d\n", d.Level, d.FileID) + } +} + +// printVersion renders the final replayed Version: one block per non-empty +// level, listing each file's id, key range, and size. +func printVersion(version *manifest.Version) { + fmt.Println("Current Version:") + + levels := version.NumLevels() + if levels == 0 { + fmt.Println(" (empty — no files)") + return + } + + for level := range levels { + files := version.Files(uint32(level)) //nolint:gosec // level is a small non-negative loop index + if len(files) == 0 { + continue + } + + var total uint64 + for _, f := range files { + total += f.Size + } + + fmt.Printf(" Level %d: %d files (%d bytes total)\n", level, len(files), total) + for _, f := range files { + fmt.Printf(" [%d] %s..%s (%d bytes)\n", + f.FileID, string(f.SmallestKey.UserKey), string(f.LargestKey.UserKey), f.Size) + } + } +} + +// formatInternalKey renders an InternalKey as "//", +// matching the convention used by sst_dump. +func formatInternalKey(k keys.InternalKey) string { + kind := "Put" + if k.Kind == keys.InternalKeyKindDelete { + kind = "Delete" + } + return fmt.Sprintf("%s/%d/%s", string(k.UserKey), k.Seqno, kind) } diff --git a/docs/formats/manifest.md b/docs/formats/manifest.md index dd39b50..315fd24 100644 --- a/docs/formats/manifest.md +++ b/docs/formats/manifest.md @@ -283,23 +283,38 @@ The principle is the same one I follow in the WAL: reserved slots either don't f Example output: ``` $ manifest_dump data/ -CURRENT → MANIFEST-000001 -Edit 0: NextFileID=1, LastSequence=0, LogNumber=1 -Edit 1: AddFile L0/7 size=1024 [apple..zebra]; NextFileID=8; LastSequence=42 -Edit 2: AddFile L0/8 size=2048 [alpha..yankee]; NextFileID=9; LastSequence=87 -End of MANIFEST (3 edits) - -Reconstructed Version: - L0: [7, 8] +Manifest: MANIFEST-000001 +Path: data/MANIFEST-000001 + +Edit #0: + next_file_id: 1 + last_sequence: 0 + log_number: 0 +Edit #1: + next_file_id: 2 + last_sequence: 42 + add_file: level=0 id=1 size=1024 smallest="apple/40/Put" largest="zebra/42/Put" +Edit #2: + next_file_id: 3 + last_sequence: 87 + add_file: level=0 id=2 size=2048 smallest="alpha/80/Put" largest="yankee/87/Put" +Current Version: + Level 0: 2 files (3072 bytes total) + [1] apple..zebra (1024 bytes) + [2] alpha..yankee (2048 bytes) ``` -On corruption: +On corruption, the tool prints the `Version` reconstructed up to the failure, +reports the last valid edit, and exits non-zero: ``` $ manifest_dump data/ -CURRENT → MANIFEST-000001 -Edit 0: NextFileID=1, LastSequence=0 -Edit 1: checksum mismatch (expected 0xABCD1234, got 0xDEADBEEF) -Stopped at edit 1 +... +Current Version: + Level 0: 1 files (1024 bytes total) + [1] apple..zebra (1024 bytes) + +Last valid edit: #1 +Error: corrupt manifest at edit #2: beachdb/record: checksum mismatch ``` --- From 6a3408fd87b82f5f037687b34724cdbfc87e8870 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 29 May 2026 13:05:15 +0200 Subject: [PATCH 20/26] Add manifest engine example under examples/ --- .gitignore | 1 + .golangci.yml | 4 + Makefile | 6 +- examples/engine/manifest/main.go | 136 +++++++++++++++++++++++++++++++ 4 files changed, 146 insertions(+), 1 deletion(-) create mode 100644 examples/engine/manifest/main.go diff --git a/.gitignore b/.gitignore index 0afc896..e6aabad 100644 --- a/.gitignore +++ b/.gitignore @@ -24,3 +24,4 @@ profile.cov # Internal stuff docs-internal/ +scratchpad/ diff --git a/.golangci.yml b/.golangci.yml index aae096e..42092f9 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -164,6 +164,10 @@ linters: # Use some standard exclusion presets presets: - std-error-handling + # Fully skip the scratchpad: it's a throwaway feature playground, not + # production code, and is gitignored. + paths: + - scratchpad/ rules: # Exclude examples from all linters (they're for demonstration, not production) - path: examples/ diff --git a/Makefile b/Makefile index a575daa..1ec1d47 100644 --- a/Makefile +++ b/Makefile @@ -34,7 +34,11 @@ test: examples: @echo "Running examples..." @for file in $$(find examples -name "*.go" -type f | sort); do \ - echo "\n=== Running $$file ==="; \ + header="=== Running $$file ==="; \ + sep=$$(printf '%*s' $${#header} '' | tr ' ' '='); \ + echo "\n$$sep"; \ + echo "$$header"; \ + echo "$$sep"; \ go run $$file || exit 1; \ done @echo "\n✓ All examples completed successfully" diff --git a/examples/engine/manifest/main.go b/examples/engine/manifest/main.go new file mode 100644 index 0000000..2ebdda0 --- /dev/null +++ b/examples/engine/manifest/main.go @@ -0,0 +1,136 @@ +package main + +import ( + "context" + "fmt" + "log" + "os" + "path/filepath" + "sort" + "strings" + + "github.com/aalhour/beachdb/engine" +) + +func main() { + dir := "/tmp/beachdb-example-manifest" + defer os.RemoveAll(dir) + + ctx := context.Background() + + // === Session 1: write data and flush it into SSTables === + // + // Auto-flush is left disabled (the default), so each explicit Flush turns + // the active memtable into exactly one SSTable. Behind the scenes the + // engine records every new SSTable in the MANIFEST — an append-only log of + // "which files exist, at which level, covering which key range". Three + // flushes produce three SSTables and three manifest entries. + fmt.Println("=== Session 1: writing and flushing three SSTables ===") + db, err := engine.Open(dir) + if err != nil { + log.Fatal(err) + } + + batches := []map[string]string{ + {"apple": "red", "apricot": "orange"}, + {"banana": "yellow", "blueberry": "blue"}, + {"cherry": "red", "cranberry": "crimson"}, + } + + for i, batch := range batches { + for _, k := range sortedKeys(batch) { + if err := db.Put(ctx, []byte(k), []byte(batch[k])); err != nil { + log.Fatal(err) + } + } + if err := db.Flush(); err != nil { + log.Fatal(err) + } + fmt.Printf(" Flushed batch %d (%d keys) -> new SSTable, tracked by the manifest\n", i+1, len(batch)) + } + + if err := db.Close(); err != nil { + log.Fatal(err) + } + + // === On-disk layout === + // + // CURRENT is a one-line pointer to the live MANIFEST file. The MANIFEST + // records the SSTable inventory; the .sst files hold the data. Inspect the + // manifest's individual edits with the manifest_dump tool: + // + // go run ./cmd/manifest_dump /tmp/beachdb-example-manifest + fmt.Println("\n=== On-disk layout ===") + printDirLayout(dir) + + // === Session 2: reopen — state is rebuilt from the manifest === + // + // All data was flushed before Close, so the WAL has nothing left to replay. + // On Open the engine reads CURRENT, replays the manifest to learn which + // SSTables exist, and opens exactly those files. Every key below is served + // from an SSTable the manifest pointed at — no directory scan, no WAL. + fmt.Println("\n=== Session 2: reopening (state rebuilt from the manifest) ===") + db, err = engine.Open(dir) + if err != nil { + log.Fatal(err) + } + defer db.Close() + + for _, batch := range batches { + for _, k := range sortedKeys(batch) { + val, err := db.Get(ctx, []byte(k)) + if err != nil { + log.Fatalf("Get(%s): %v", k, err) + } + fmt.Printf(" %s = %s\n", k, val) + } + } + + fmt.Println("\n✓ SSTable inventory survived the restart via the manifest") +} + +// printDirLayout lists the database directory, grouping the manifest machinery +// (CURRENT, MANIFEST-*) apart from the SSTables it tracks and the WAL. +func printDirLayout(dir string) { + entries, err := os.ReadDir(dir) + if err != nil { + log.Fatal(err) + } + + var manifestFiles, sstables, other []string + for _, e := range entries { + name := e.Name() + switch { + case name == "CURRENT" || strings.HasPrefix(name, "MANIFEST-"): + manifestFiles = append(manifestFiles, name) + case filepath.Ext(name) == ".sst": + sstables = append(sstables, name) + default: + other = append(other, name) + } + } + + fmt.Println(" Manifest:") + for _, n := range manifestFiles { + fmt.Printf(" %s\n", n) + } + fmt.Println(" SSTables (tracked by the manifest):") + for _, n := range sstables { + fmt.Printf(" %s\n", n) + } + fmt.Println(" Other:") + for _, n := range other { + fmt.Printf(" %s\n", n) + } +} + +// sortedKeys returns the keys of m in ascending order so the example writes +// and reads them deterministically. +func sortedKeys(m map[string]string) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} From e6cc2b6f57e1f818ede1eb5c3dc097ad43f80bb5 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 29 May 2026 13:25:54 +0200 Subject: [PATCH 21/26] Add reserved blog post link. --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index d1c49ec..1cbace5 100644 --- a/README.md +++ b/README.md @@ -31,8 +31,8 @@ BeachDB is my attempt to re-learn the fundamentals by building them from scratch - [x] **Memtable v1**: sorted structure + tombstones, see: [memtable blog post](https://aalhour.com/posts/beachdb-memtable-v1/) - [x] **Reference-model randomized tests** (model vs implementation) - [x] **SSTables v1**: immutable sorted files + `sst_dump`, see: [sstables blog post](https://aalhour.com/posts/beachdb-sstables-v1/) -- [x] **Crash-loop harness**: kill mid-write, reopen, validate invariants, see: [crash-testing, part 1](https://aalhour.com/posts/beachdb-crash-testing-part1/) blog post -- [ ] **Manifest/versioning** + `manifest_dump` (startup reconstruction) +- [x] **Crash-loop harness**: kill mid-write, reopen, validate invariants, see: [crash-testing, part 1 blog post](https://aalhour.com/posts/beachdb-crash-testing-part1/) +- [x] **Manifest/versioning** + `manifest_dump`: startup reconstruction, see: [manifest blog post](https://aalhour.com/posts/beachdb-manifest-v1) - [ ] **Merge iterators** (memtable + SSTs) + **snapshot reads** (seqno-based) - [ ] **Read path acceleration**: block index + bloom filters + benchmark evidence - [ ] **Compaction v1**: one strategy, minimal knobs + amplification measurements From d55cb362bacc8f50c3130baf97a798ddfd1b5af6 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 29 May 2026 13:25:54 +0200 Subject: [PATCH 22/26] Add reserved blog post link. --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index d1c49ec..f3b61a5 100644 --- a/README.md +++ b/README.md @@ -31,8 +31,8 @@ BeachDB is my attempt to re-learn the fundamentals by building them from scratch - [x] **Memtable v1**: sorted structure + tombstones, see: [memtable blog post](https://aalhour.com/posts/beachdb-memtable-v1/) - [x] **Reference-model randomized tests** (model vs implementation) - [x] **SSTables v1**: immutable sorted files + `sst_dump`, see: [sstables blog post](https://aalhour.com/posts/beachdb-sstables-v1/) -- [x] **Crash-loop harness**: kill mid-write, reopen, validate invariants, see: [crash-testing, part 1](https://aalhour.com/posts/beachdb-crash-testing-part1/) blog post -- [ ] **Manifest/versioning** + `manifest_dump` (startup reconstruction) +- [x] **Crash-loop harness**: kill mid-write, reopen, validate invariants, see: [crash-testing, part 1 blog post](https://aalhour.com/posts/beachdb-crash-testing-part1/) +- [x] **Manifest v1**: durable SSTable catalog (`CURRENT` + `VersionEdit` log) for startup reconstruction + `manifest_dump`, see: [manifest blog post](https://aalhour.com/posts/beachdb-manifest-v1/) - [ ] **Merge iterators** (memtable + SSTs) + **snapshot reads** (seqno-based) - [ ] **Read path acceleration**: block index + bloom filters + benchmark evidence - [ ] **Compaction v1**: one strategy, minimal knobs + amplification measurements From 87819b0520e9bb16f142ba6b2139f6a0a7b53af2 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 29 May 2026 14:09:28 +0200 Subject: [PATCH 23/26] Correct header size in manifest format document: 12 -> 18 bytes. --- docs/formats/manifest.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/formats/manifest.md b/docs/formats/manifest.md index 315fd24..802a832 100644 --- a/docs/formats/manifest.md +++ b/docs/formats/manifest.md @@ -48,9 +48,11 @@ A MANIFEST file is a sequence of records. The framing is the same as the [WAL re | `checksum` | CRC32C of payload | CRC32C of payload | | `payload` | encoded `Batch` | encoded `VersionEdit` | -Different magic byte means `wal_dump` cleanly rejects a manifest file and vice versa. +Different magic bytes mean `wal_dump` cleanly rejects a manifest file and vice versa. -The header is 12 bytes. The payload is the encoded `VersionEdit` described below. +The header is 18 bytes: 8-byte magic, 1-byte version, 1-byte record type, +4-byte payload length, and 4-byte checksum. The payload is the encoded +`VersionEdit` described below. --- @@ -175,7 +177,7 @@ A crash at any step leaves either the old CURRENT intact or the new CURRENT full A truncated last record means the process crashed mid-write to the manifest: -- **Truncated header** (< 12 bytes): ignore, treat as EOF, truncate the file back to the last valid offset. +- **Truncated header** (< 18 bytes): ignore, treat as EOF, truncate the file back to the last valid offset. - **Truncated payload** (header valid, payload incomplete): ignore, treat as EOF, truncate. The incomplete edit was never `fsync`'d, so it never took effect from the database's perspective. Discarding it is correct. From ca201445b80910909a513d2d4c05a7fac87c6657 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 29 May 2026 14:09:51 +0200 Subject: [PATCH 24/26] Implement best-effort orphaned SSTable file cleanup in engine/ --- engine/db.go | 58 ++++++++++++++++ engine/db_flush_test.go | 63 +++++++++++++++++ engine/db_manifest_test.go | 134 +++++++++++++++++++++++++++++++++++++ 3 files changed, 255 insertions(+) diff --git a/engine/db.go b/engine/db.go index eb25f63..87e93d3 100644 --- a/engine/db.go +++ b/engine/db.go @@ -924,6 +924,38 @@ func openSSTReadersForVersion(dir string, version *manifest.Version) ([]*sstable return readers, nil } +// cleanupOrphanSSTables removes canonical SSTable files that exist on disk +// but are not referenced by the manifest Version. Orphan cleanup is +// best-effort: the manifest is still the source of truth, so a deletion +// failure leaves a harmless file behind for a future Open to try again. +func cleanupOrphanSSTables(dir string, version *manifest.Version) { + referenced := make(map[uint64]struct{}) + for _, fm := range version.AllFiles() { + referenced[fm.FileID] = struct{}{} + } + + entries, err := os.ReadDir(dir) + if err != nil { + return + } + + for _, entry := range entries { + if entry.IsDir() { + continue + } + + fileID, ok := parseSSTFileName(entry.Name()) + if !ok { + continue + } + if _, exists := referenced[fileID]; exists { + continue + } + + _ = os.Remove(filepath.Join(dir, entry.Name())) + } +} + func replayExistingManifest(db *DB, current string) error { manifestPath := filepath.Join(db.dir, current) @@ -949,6 +981,8 @@ func replayExistingManifest(db *DB, current string) error { } } + cleanupOrphanSSTables(db.dir, res.version) + ssts, err := openSSTReadersForVersion(db.dir, res.version) if err != nil { return err @@ -1110,6 +1144,30 @@ func buildSSTFileName(id uint64) string { return fmt.Sprintf("%0*d%s", sstableFileIDWidth, id, sstableFileExt) } +// parseSSTFileName extracts the file ID from a canonical BeachDB SSTable +// filename. Non-canonical names are ignored by orphan cleanup. +func parseSSTFileName(name string) (uint64, bool) { + if filepath.Ext(name) != sstableFileExt { + return 0, false + } + + idStr := strings.TrimSuffix(name, sstableFileExt) + if len(idStr) != sstableFileIDWidth { + return 0, false + } + for _, ch := range idStr { + if ch < '0' || ch > '9' { + return 0, false + } + } + + id, err := strconv.ParseUint(idStr, 10, 64) + if err != nil { + return 0, false + } + return id, true +} + // buildManifestFileName builds a MANIFEST filename from a file ID, // e.g. 1 → "MANIFEST-000001". func buildManifestFileName(id uint64) string { diff --git a/engine/db_flush_test.go b/engine/db_flush_test.go index 348e503..5894924 100644 --- a/engine/db_flush_test.go +++ b/engine/db_flush_test.go @@ -60,6 +60,69 @@ func TestBuildSSTFileName(t *testing.T) { } } +func TestParseSSTFileName(t *testing.T) { + tests := []struct { + name string + wantID uint64 + wantOK bool + }{ + { + name: "00000000000000000000.sst", + wantID: 0, + wantOK: true, + }, + { + name: "00000000000000000001.sst", + wantID: 1, + wantOK: true, + }, + { + name: "00000000000000000042.sst", + wantID: 42, + wantOK: true, + }, + { + name: "18446744073709551615.sst", + wantID: ^uint64(0), + wantOK: true, + }, + { + name: "00000000000000000001.wal", + wantOK: false, + }, + { + name: "0000000000000000001.sst", + wantOK: false, + }, + { + name: "000000000000000000001.sst", + wantOK: false, + }, + { + name: "00000000000000000x01.sst", + wantOK: false, + }, + { + name: "99999999999999999999.sst", + wantOK: false, + }, + { + name: "CURRENT", + wantOK: false, + }, + } + + for _, tt := range tests { + gotID, gotOK := parseSSTFileName(tt.name) + if gotOK != tt.wantOK { + t.Errorf("parseSSTFileName(%q) ok = %v, want %v", tt.name, gotOK, tt.wantOK) + } + if gotID != tt.wantID { + t.Errorf("parseSSTFileName(%q) id = %d, want %d", tt.name, gotID, tt.wantID) + } + } +} + // Close should close all SSTable readers func TestDB_CloseClosesSSTReaders(t *testing.T) { dir := t.TempDir() diff --git a/engine/db_manifest_test.go b/engine/db_manifest_test.go index b11d2a8..04429e2 100644 --- a/engine/db_manifest_test.go +++ b/engine/db_manifest_test.go @@ -139,6 +139,71 @@ func createRealSST(t *testing.T, dir string, fileID uint64) uint64 { return uint64(info.Size()) } +func TestCleanupOrphanSSTables_RemovesOnlyUnreferencedCanonicalSSTables(t *testing.T) { + dir := t.TempDir() + + referencedFiles := []string{buildSSTFileName(1), buildSSTFileName(3)} + for _, name := range referencedFiles { + writeFile(t, filepath.Join(dir, name), []byte("referenced")) + } + + orphanName := buildSSTFileName(2) + writeFile(t, filepath.Join(dir, orphanName), []byte("orphan")) + + // Hit the non-removal branches too: directory entries are skipped, + // non-canonical SST-looking names are ignored, and non-SST database files + // are not touched by cleanup. + orphanDirName := buildSSTFileName(4) + if err := os.Mkdir(filepath.Join(dir, orphanDirName), 0700); err != nil { + t.Fatalf("Mkdir(%q): %v", orphanDirName, err) + } + ignoredFiles := []string{ + "0001.sst", + "not-a-number.sst", + "CURRENT", + "MANIFEST-000001", + walFileName, + } + for _, name := range ignoredFiles { + writeFile(t, filepath.Join(dir, name), []byte("ignored")) + } + + version := manifest.NewVersion(0).Apply(&manifest.VersionEdit{ + AddedFiles: []manifest.FileMetadata{ + fileMetaFixture(0, 1, 100, "a", "m"), + fileMetaFixture(0, 3, 100, "n", "z"), + }, + }) + + cleanupOrphanSSTables(dir, version) + + if dirContains(t, dir, orphanName) { + t.Errorf("orphan SST %s still exists", orphanName) + } + for _, name := range referencedFiles { + if !dirContains(t, dir, name) { + t.Errorf("referenced SST %s was removed", name) + } + } + if !dirContains(t, dir, orphanDirName) { + t.Errorf("directory entry %s was removed", orphanDirName) + } + for _, name := range ignoredFiles { + if !dirContains(t, dir, name) { + t.Errorf("ignored file %s was removed", name) + } + } +} + +func TestCleanupOrphanSSTables_MissingDirIsBestEffort(t *testing.T) { + missingDir := filepath.Join(t.TempDir(), "missing") + + // Best-effort cleanup must not turn a transient ReadDir failure into an + // Open failure. The manifest replay path still treats missing referenced + // SSTables as hard errors separately. + cleanupOrphanSSTables(missingDir, manifest.NewVersion(0)) +} + // appendEditsToManifest opens the existing manifest file via Writer and // appends each edit in order. Caller is responsible for the file already // existing. @@ -911,6 +976,75 @@ func TestFlush_ManifestAppendFailure_SSTOrphaned(t *testing.T) { } } +// On Open, the manifest is authoritative: any canonical SSTable file not +// referenced by the replayed Version is an orphan and gets removed before the +// next file allocation can reuse that ID. +func TestOpen_CleansUpOrphanSSTables(t *testing.T) { + dir := t.TempDir() + db, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Open: %v", err) + } + ctx := context.Background() + if err := db.Put(ctx, []byte("stable"), []byte("from-sst")); err != nil { + t.Fatalf("Put(stable): %v", err) + } + if err := db.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + // Remove the WAL so reopen is forced to recover the key from the + // manifest-tracked SSTable, then plant a junk canonical SSTable at the + // next ID. This is the crash-after-SST-sync/before-manifest-append shape. + if err := os.Remove(filepath.Join(dir, walFileName)); err != nil { + t.Fatalf("removing WAL: %v", err) + } + orphanName := buildSSTFileName(2) + writeFile(t, filepath.Join(dir, orphanName), []byte("partial-orphan")) + + db2, err := Open(dir, WithSync(false)) + if err != nil { + t.Fatalf("Reopen: %v", err) + } + defer db2.Close() + + if dirContains(t, dir, orphanName) { + t.Errorf("orphan SST %s still exists after Open", orphanName) + } + if got := len(db2.version.AllFiles()); got != 1 { + t.Fatalf("Version.AllFiles() = %d files, want 1 manifest-referenced file", got) + } + if got := len(db2.ssts); got != 1 { + t.Fatalf("db.ssts = %d readers, want 1 manifest-referenced reader", got) + } + val, err := db2.Get(ctx, []byte("stable")) + if err != nil { + t.Fatalf("Get(stable): %v", err) + } + if string(val) != "from-sst" { + t.Fatalf("Get(stable) = %q, want %q", val, "from-sst") + } + if db2.nextSSTID != 2 { + t.Fatalf("nextSSTID after cleanup = %d, want 2", db2.nextSSTID) + } + + if err := db2.Put(ctx, []byte("after-cleanup"), []byte("new-sst")); err != nil { + t.Fatalf("Put(after-cleanup): %v", err) + } + if err := db2.Flush(); err != nil { + t.Fatalf("Flush after cleanup: %v", err) + } + if !dirContains(t, dir, orphanName) { + t.Errorf("expected new SST %s to reuse the cleaned orphan ID", orphanName) + } + if got := len(db2.version.AllFiles()); got != 2 { + t.Errorf("Version.AllFiles() = %d files, want 2 after second flush", got) + } +} + // NextFileID recorded in the flush edit is restored on reopen, so SST ids keep // climbing across restarts instead of resetting and overwriting files. func TestFlush_NextSSTIDPersistedViaManifest(t *testing.T) { From 6e1a25ab89173831561fbb35f391fd962ee1ff3d Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 29 May 2026 14:16:21 +0200 Subject: [PATCH 25/26] fix CI, remove milestone branches from push and run crash testing step --- .github/workflows/ci.yml | 26 ++++++++++++++++++++++++-- Makefile | 8 ++++++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e7ec1ec..3ff95ea 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,10 +2,14 @@ name: CI on: push: - branches: [main, master, "milestone/*"] + branches: [main, master] pull_request: branches: [main, master] +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + jobs: build-and-test: runs-on: ubuntu-latest @@ -18,7 +22,7 @@ jobs: uses: actions/setup-go@v5 with: go-version: "1.26" - + - name: golangci-lint uses: golangci/golangci-lint-action@v8 with: @@ -33,3 +37,21 @@ jobs: - name: Test run: make test + + - name: Examples + run: make examples + + crash: + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: "1.26" + + - name: Crash harness (ci profile) + run: make crash-check PROFILE=ci diff --git a/Makefile b/Makefile index 1ec1d47..f0494c6 100644 --- a/Makefile +++ b/Makefile @@ -2,6 +2,9 @@ CYCLES ?= 100 +# Crash harness profile: `full` (uses CYCLES) or `ci` (fast deterministic preset). +PROFILE ?= full + # Bench knobs. Override on the CLI: `make bench PKG=./internal/wal BENCH=BenchmarkX BENCHTIME=3s` PKG ?= ./... BENCH ?= . @@ -79,15 +82,16 @@ fuzz: done; \ done -## crash-check: Run the controller/worker crash harness ($(CYCLES) cycles) with a temporary workspace +## crash-check: Run the controller/worker crash harness with a temporary workspace. `make crash-check PROFILE=ci` crash-check: @set -eu; \ tmpdir=$$(mktemp -d /tmp/beachdb-crash.XXXXXX); \ dbdir="$$tmpdir/db"; \ artdir="$$tmpdir/artifacts"; \ - echo "Running crash harness ($(CYCLES) cycles) in $$dbdir"; \ + echo "Running crash harness (profile=$(PROFILE), cycles=$(CYCLES)) in $$dbdir"; \ echo ""; \ go run ./cmd/crash run \ + --profile=$(PROFILE) \ --dbdir="$$dbdir" \ --artifact-dir="$$artdir" \ --cycles=$(CYCLES) \ From 57720d61c6042670926d414d331a1975a31d0f36 Mon Sep 17 00:00:00 2001 From: Ahmad Alhour Date: Fri, 29 May 2026 14:35:08 +0200 Subject: [PATCH 26/26] More format explanation corrections in the doc --- docs/formats/manifest.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/formats/manifest.md b/docs/formats/manifest.md index 802a832..c2076c5 100644 --- a/docs/formats/manifest.md +++ b/docs/formats/manifest.md @@ -101,7 +101,7 @@ Offset Hex Meaning 52..56 7A 65 62 72 61 largestKey bytes = "zebra" ``` -Total body: 57 bytes. Wrapped by the 12-byte record header, the on-disk record is 69 bytes. +Total body: 57 bytes. Wrapped by the 18-byte record header, the on-disk record is 75 bytes. ### Deterministic emit order