Recovery tail-truncation had two compounding bugs in wal/recover.go:
C5: truncateSegment only called os.Truncate. Missing per design §3.2
line 787-794:
- Step 2: fsync the truncated segment
- Step 3: delete empty trailing segments
- Step 4: fsync WAL directory
And all errors were swallowed into result.TruncateError with recovery
still returning success, violating design line 799: "若 ftruncate、
segment fsync、空 segment 删除或 WAL directory fsync 任一步失败，
recovery 必须报错，DB 不得进入可写状态".
H8: findValidOffset only checked physical record CRCs, ignoring the
FragmentCollector state machine. For a tail of First + Middle*
without Last, it returned the offset AFTER the last Middle fragment
instead of the last COMPLETE batch end. Result: residual half-batch
fragments caused repeated tail-corruption reports on every restart.
Changes:
- wal/recover.go:
- Add findLastCompleteBatchEnd: batch-aware offset finder using
FragmentCollector state machine. Handles block-boundary padding
correctly (continue across full-block padding, return on short-block).
- Add truncateAndPersist: 4-step protocol (ftruncate + fsync segment +
delete empty trailing + fsync dir). Any step failure is fatal.
- Add segmentFsyncFn (package-level var for test injection, same
pattern as C6's dirFsyncFn).
- Refactor Recover failure path: use new functions, hard-error on
truncation persist failure (was: swallow to TruncateError).
- TruncateError field semantics: informational only ("tail corruption
was detected and repair attempted"). Persist failures return error.
- Delete findValidOffset and truncateSegment (replaced).
- wal/recover_offset_test.go (new): 8 unit tests for
findLastCompleteBatchEnd covering clean/partial-tail/no-batch/
physical-corruption/partial-only/block-boundary-padding/non-zero-tail/
zero-tail cases. 5 unit tests for truncateAndPersist covering success/
ftruncate-fail/dir-fsync-fail/segment-fsync-fail/retry-after-failure.
- wal/recover_test.go: add TestRecoverPartialFragmentTailIdempotent
(H8 e2e regression: truncation point must be at last complete batch),
TestRecoverTruncationFailureFailsRecovery (C5 e2e regression: any
step failure fails Recover), TestRecoverInvalidBatchNotTruncatable
(design line 778-781: invalid batch content hard-fails, NOT truncatable).
Injection note: segmentFsyncFn and dirFsyncFn (from C6) are package-level
vars; tests that override either must not use t.Parallel().
Verified: each new test fails on pre-fix code by logical analysis and
passes after the fix. Full suite green including go test -race ./... .
Phase 1 simplification: emptyTrailingSegments is always nil in Phase 1
(truncated segment is always segments[last]). The parameter is kept in
truncateAndPersist's signature for forward compatibility with the C4 fix.
Audit context: docs/audit-3.2.md C5 and H8 (H8 Oracle-verified bg_ef425776).
Phase 1 default state had two data-loss paths in WAL recovery.
C2: resolveRecoverySegmentID fell back to CURRENT when MANIFEST=0.
Since segment_manager writes CURRENT on every segment create/rotate,
the first recovery in Phase 1 (MANIFEST always 0 without flush) would
start from the active segment, skipping earlier unflushed segments.
C3: Recover called manifest.Save after every recovery, advancing
recoverySegmentID past segments that were still the only durable copy
of their data (no SSTable flush yet). Next restart would filter those
segments out and permanently lose the data.
Per design §3.2 line 280, recovery must not update MANIFEST; per line
604-06, CURRENT must not be used as recovery start. Both fixes are
required together — fixing C3 alone leaves C2's data-loss window open.
Changes:
- wal/recover.go: remove manifest.Save calls on both success and
tail-repair paths; remove CURRENT fallback in resolveRecoverySegmentID.
RecoveryResult.NextSegmentID is now in-memory only (consumed by DB.Open
to seed the new WalWriter, but never persisted to MANIFEST).
- wal/recover_test.go: rewrite TestRecoverUpdatesManifest as
TestRecoverDoesNotUpdateManifest; add TestRecoverPreservesExistingManifest,
TestRecoverIdempotentClean, TestRecoverIdempotentAfterTruncation,
TestRecoverIgnoresCurrentFallback.
- db_test.go: add TestOpenThreeTimesKeepsData (three opens to catch C3's
second-restart data loss; single-segment to avoid unrelated C8 bug
where segment_manager passes byte offset as startSequence).
Verified: each new test fails on pre-fix code and passes after the fix.
Full suite green including -race.
Audit context: docs/audit-3.2.md (with Oracle revisions from bg_ef425776
and bg_2e86d33b; C8 added). Plan: .omo/plans/fix-c2-c3-wal-recovery.md
(Momus + Oracle reviewed v1.2).
- Sync() now flushes BlockWriter before fd.Sync() for durability
- Pre-validate encoding before sequence allocation (design doc compliance)
- Remove dead _ = rec assignment in recover.go
- Remove unused maxPayload field from SegmentWriter
- Handle Put error in MemTable.Publish with panic on invariant violation
- wal/recover.go: top-level Recover() function that orchestrates scan + replay + truncation
Reads MANIFEST/CURRENT for recovery segment ID, handles tail corruption by truncating
- Comprehensive tests for empty dir, full flow, tail corruption, and MANIFEST updates
- All tests pass with -race
dailz