Summary
Follow-up from the PR #1 review (suggestion #2, by Antigravity/Gemini). Several WalError variants could carry more diagnostic context, which would make production failures much easier to triage:
InvalidConfig → carry a reason, e.g. InvalidConfig { detail: &'static str } ("max_record_size exceeds segment_size - 91"), mirroring the existing Corruption { detail } pattern.FsyncFailed → wrap the underlying std::io::Error so the OS-level cause (e.g. ENOSPC, hardware failure) is visible. This would also let Error::source() chain through it (today source() only returns Some for Io).BadSegmentHeader → carry the Lsn of the offending segment, so operators know exactly which file is broken (mirrors Corruption/TornMidLog, which already carry segment).
Why this isn't done in PR #1
The §10 error enum in docs/wal_design_v6.md is normative and currently specifies these three variants without fields (only Corruption { segment, offset, detail } and TornMidLog { segment, offset } carry context). The repo's working rules (CLAUDE.md: "implement to the contract / don't silently diverge from the design doc") mean changing these matchable shapes should follow a design-doc amendment rather than land ad hoc — especially since the whole codebase (M2–M5) will construct and match on these variants.
The design doc is maintained separately, so this issue is the handoff point: amend §10 first, then implement to the updated contract.
Proposed §10 shape (for discussion)
InvalidConfig { detail: &'static str },
FsyncFailed { source: std::io::Error }, // + Error::source() returns Some(source)
BadSegmentHeader { segment: Lsn },Acceptance / notes
- Update §10 (and any cross-refs, e.g. §5.3 for
InvalidConfig, §12 for FsyncFailed) in docs/wal_design_v6.md.
- Extend
Error::source() to chain through FsyncFailed.
- Update
Display impls and tests accordingly.
- Purely diagnostic — does not touch the durability contract (D1–D12); error content is strictly downstream of durability decisions.
Filed by Claude per maintainer request; spec amendment to be picked up by the maintainer.
Summary
Follow-up from the PR #1 review (suggestion #2, by Antigravity/Gemini). Several
WalErrorvariants could carry more diagnostic context, which would make production failures much easier to triage:InvalidConfig→ carry a reason, e.g.InvalidConfig { detail: &'static str }("max_record_size exceeds segment_size - 91"), mirroring the existingCorruption { detail }pattern.FsyncFailed→ wrap the underlyingstd::io::Errorso the OS-level cause (e.g.ENOSPC, hardware failure) is visible. This would also letError::source()chain through it (todaysource()only returnsSomeforIo).BadSegmentHeader→ carry theLsnof the offending segment, so operators know exactly which file is broken (mirrorsCorruption/TornMidLog, which already carrysegment).Why this isn't done in PR #1
The §10 error enum in
docs/wal_design_v6.mdis normative and currently specifies these three variants without fields (onlyCorruption { segment, offset, detail }andTornMidLog { segment, offset }carry context). The repo's working rules (CLAUDE.md: "implement to the contract / don't silently diverge from the design doc") mean changing these matchable shapes should follow a design-doc amendment rather than land ad hoc — especially since the whole codebase (M2–M5) will construct and match on these variants.The design doc is maintained separately, so this issue is the handoff point: amend §10 first, then implement to the updated contract.
Proposed §10 shape (for discussion)
Acceptance / notes
InvalidConfig, §12 forFsyncFailed) indocs/wal_design_v6.md.Error::source()to chain throughFsyncFailed.Displayimpls and tests accordingly.Filed by Claude per maintainer request; spec amendment to be picked up by the maintainer.