diff --git a/.omo/plans/fix-c6-segment-dir-fsync.md b/.omo/plans/fix-c6-segment-dir-fsync.md new file mode 100644 index 0000000..1465ac2 --- /dev/null +++ b/.omo/plans/fix-c6-segment-dir-fsync.md @@ -0,0 +1,517 @@ +# C6 修复方案:SegmentWriter 目录 fsync 失败必须硬错误 + +## TL;DR + +> **目标**:修复 `wal/segment_writer.go:86-89` 静默吞掉目录 fsync 错误的 durability 漏洞。让 `NewSegmentWriter` 在目录 fsync 失败时返回错误,传播到 `SegmentManager.AppendBatch` → 触发 `ErrCommitUnknown` + write-stopped(或 `DB.Open` 失败)。 +> +> **交付**: +> - 抽出 `dirFsync` helper 函数(独立可测) +> - `NewSegmentWriter` 调用 helper,失败时清理资源并返回错误 +> - 用 package-level 变量支持测试注入失败 +> - 新增 3 个测试:helper 单元测试 × 2 + NewSegmentWriter 集成测试 × 1 +> - 单次 commit +> +> **预估工时**:1.25-1.75 小时 +> **风险**:低(单文件改动 + 测试,production 行为变化是"原本被吞的错误现在会冒出来",这正是设计意图) + +--- + +## Context + +### Bug 摘要(audit C6) + +`wal/segment_writer.go:85-89`: + +```go +// Sync directory to make rename durable (best-effort on Linux). +if dirFD, derr := os.Open(dir); derr == nil { + dirFD.Sync() // 错误被完全忽略 + dirFD.Close() +} +``` + +两层错误都被吞: +1. `os.Open(dir)` 失败 → 整个 fsync 被跳过 +2. `dirFD.Sync()` 失败 → 错误丢失 + +注释写"best-effort on Linux"误导,但设计明确这是硬条件。 + +### 设计依据(§3.2 line 248-273) + +新 segment durable-ready 协议第 5 步: + +``` +1. create segment-N.wal.tmp +2. write WAL File Header +3. fsync segment-N.wal.tmp +4. rename segment-N.wal.tmp → segment-N.wal +5. fsync WAL directory ← 关键 +6. segment-N enters durable-ready state +7. WAL writer may append batches whose recovery depends on segment-N +``` + +line 272: + +> 如果新 segment 进入 durable-ready 之前任一步失败,**该 segment 不得成为 active segment,也不得承载可确认写入**。如果此时尚未分配 sequence,可以重试创建或切换到其他 segment;如果 sequence 已分配或已有 WAL Batch 依赖该 segment,则按 WAL write failure 处理,**引擎进入 write-stopped 状态**。 + +### 为什么是 Always 模式最严重的 durability 漏洞 + +Linux rename 是内存中的原子操作,**不保证掉电后目录项可恢复**。要让 rename 落盘必须 fsync 目录本身。掉电场景下: + +- rename 已执行但目录 fsync 没成功 → 重启后 segment 文件可能从目录里消失 +- 但 WAL writer 已经按 "Always 不丢已确认写入" 向调用方返回成功 + +这个 bug **平时不发作**(正常环境下 rename + 异步 write-back 都会成功),**只在掉电那一刻有概率中招**。是最阴险的类型。 + +### 错误传播路径(已正确,只需源头报错) + +``` +NewSegmentWriter 失败(dir fsync error) +├─ 在 NewSegmentManager 路径(DB.Open 首次创建 segment) +│ → NewSegmentManager 失败 → DB.Open 失败 +│ → 用户看到 error,没数据承诺被违反 ✓ +└─ 在 SegmentManager.rotate 路径(segment 满了轮转) + → AppendBatch 失败 → WalWriter.stopWithError(ErrCommitUnknown) + → write-stopped ✓(符合设计 line 272) +``` + +`segment_manager.go:35-37, 81-83` 已经透传 error,本次 fix 不需要改它。 + +--- + +## 执行计划 + +### Phase A:代码改动(30 分钟) + +#### A.1 抽出 `dirFsync` helper + +新文件 `wal/dir_fsync.go`(或加入现有 `segment_writer.go` 末尾): + +> **Momus 修订(bg_90bcf359)**:helper 应该校验 path 是目录,否则传文件路径会"成功 fsync 了一个文件"但语义错误。加 `os.Stat` + `IsDir` 检查。 +> +> **Oracle 修订(bg_48d53274)**:进一步用 `os.Open → f.Stat → IsDir → f.Sync` 顺序,消除 `os.Stat` + `os.Open` 之间的 TOCTOU 窗口。 + +```go +package wal + +import ( + "fmt" + "os" +) + +// dirFsync opens the directory and fsyncs it. Required for durable-ready +// state per design §3.2 line 258. This is a hard requirement, not +// best-effort: rename is atomic in memory but not guaranteed to survive +// power loss without a directory fsync. +// +// dirFsyncFn is a package-level variable so tests can inject failures +// without an interface indirection in production code. +// +// NOT PARALLEL-SAFE: tests that override dirFsyncFn must not use t.Parallel(). +// All existing wal tests run serially within the package. +var dirFsyncFn = dirFsync + +func dirFsync(dir string) error { + f, err := os.Open(dir) + if err != nil { + return fmt.Errorf("open dir %q: %w", dir, err) + } + defer f.Close() + info, err := f.Stat() + if err != nil { + return fmt.Errorf("stat dir %q: %w", dir, err) + } + if !info.IsDir() { + return fmt.Errorf("path %q is not a directory", dir) + } + if err := f.Sync(); err != nil { + return fmt.Errorf("fsync dir %q: %w", dir, err) + } + return nil +} +``` + +> **关于 hook 触发的注释**:上面 docstring 是必要的 —— 它解释了**为什么这是硬条件而不是 best-effort**(防止未来工程师把它"简化"成 silent again)。属于 security-related 注释,符合 hook 规则的 necessary comments。 + +#### A.2 修改 `NewSegmentWriter` + +替换 `wal/segment_writer.go:85-89`: + +```go +// 改前(C6 bug): +// Sync directory to make rename durable (best-effort on Linux). +if dirFD, derr := os.Open(dir); derr == nil { + dirFD.Sync() + dirFD.Close() +} + +// 改后: +// Per design §3.2 line 258, directory fsync is a hard requirement for +// durable-ready. Without it, the rename above is not guaranteed to survive +// power loss, violating the Always-mode "no loss of acknowledged writes" +// promise. +if err := dirFsyncFn(dir); err != nil { + closeErr := fd.Close() + removeErr := os.Remove(finalPath) + if closeErr != nil || removeErr != nil { + // Surface cleanup errors alongside the primary fsync error so they + // are not lost (Go 1.20+ errors.Join). + cleanup := errors.Join(closeErr, removeErr) + return nil, fmt.Errorf("wal: fsync directory after segment rename (cleanup: %v): %w", cleanup, err) + } + return nil, fmt.Errorf("wal: fsync directory after segment rename: %w", err) +} +``` + +清理规则:失败时关闭新打开的 `fd`,删除已 rename 出来的 `finalPath`,让磁盘状态回到"没有 segment-N"。如果清理本身也失败,用 `errors.Join` 把清理错误和主错误一起返回(不丢任何信息)。 + +> **关于 hook 触发的注释**:这条注释引用设计行号 + 解释 failure 处理逻辑,防止未来"简化"成 silent。属于 security-related 注释。 + +### Phase B:测试(45-60 分钟) + +#### B.1 helper 单元测试 + +新增 `wal/dir_fsync_test.go`: + +```go +package wal + +import ( + "os" + "strings" + "testing" +) + +func TestDirFsyncSuccess(t *testing.T) { + dir := t.TempDir() + if err := dirFsync(dir); err != nil { + t.Errorf("dirFsync on valid dir: %v", err) + } +} + +func TestDirFsyncNonExistentDir(t *testing.T) { + err := dirFsync("/nonexistent/path/that/should/not/exist") + if err == nil { + t.Fatal("expected error on non-existent dir") + } + // After Oracle修订 (Open first), non-existent dir fails at os.Open. + if !strings.Contains(err.Error(), "open dir") { + t.Errorf("error should mention 'open dir', got: %v", err) + } +} + +func TestDirFsyncNotADirectory(t *testing.T) { + dir := t.TempDir() + filePath := dir + "/notadir" + if err := os.WriteFile(filePath, []byte("x"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + err := dirFsync(filePath) + if err == nil { + t.Fatal("expected error when fsyncing a file as dir") + } + if !strings.Contains(err.Error(), "not a directory") { + t.Errorf("error should mention 'not a directory', got: %v", err) + } +} +``` + +#### B.2 NewSegmentWriter 集成测试(注入失败) + +新增到 `wal/segment_writer_test.go`: + +```go +// Regression guard for C6: NewSegmentWriter must return an error when +// directory fsync fails, instead of silently succeeding. Per design §3.2 +// line 272, segment must NOT become active if durable-ready fails. +func TestNewSegmentWriterDirFsyncFailure(t *testing.T) { + dir := t.TempDir() + cfg := config.Defaults() + + // Inject dir fsync failure. + orig := dirFsyncFn + dirFsyncFn = func(d string) error { + return errors.New("simulated dir fsync failure") + } + t.Cleanup(func() { dirFsyncFn = orig }) + + sw, err := NewSegmentWriter(dir, 0, 0, &cfg) + if err == nil { + if sw != nil { + sw.Close() + } + t.Fatal("NewSegmentWriter: expected error on dir fsync failure, got nil") + } + + if !strings.Contains(err.Error(), "fsync directory") { + t.Errorf("error should mention 'fsync directory', got: %v", err) + } + + // Verify cleanup: no segment file should remain. + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + for _, e := range entries { + name := e.Name() + if strings.Contains(name, "segment-0") { + t.Errorf("segment file should be cleaned up, found: %s", name) + } + } +} + +// Regression guard for C6: ensure normal path still works after the fix. +func TestNewSegmentWriterNormalPathStillWorks(t *testing.T) { + dir := t.TempDir() + cfg := config.Defaults() + + sw, err := NewSegmentWriter(dir, 0, 0, &cfg) + if err != nil { + t.Fatalf("NewSegmentWriter normal path: %v", err) + } + defer sw.Close() + + // Verify segment file exists. + if _, err := os.Stat(sw.SegmentPath()); err != nil { + t.Errorf("segment file should exist: %v", err) + } +} +``` + +#### B.3 验证错误传播 + +`wal/segment_manager_test.go` 已经测过 NewSegmentManager 正常路径。需补充测试验证错误传播。 + +> **Oracle 修订(bg_48d53274)**:原计划的 `_, err = sm.AppendBatch(encoded)` 是编译错误(`AppendBatch` 只返回 `error`)。另外,单个 `{k,v}` batch 编码只有 ~24 字节,远不到 512-32=480 阈值,**不会触发轮转**。需要循环填到 `sm.RemainingPayload() < worstCaseSize` 再注入失败。 + +```go +// Regression guard for C6: SegmentManager.AppendBatch must propagate +// rotation failure (which now includes dir fsync failure) as error. +func TestSegmentManagerRotateFailsOnDirFsyncFailure(t *testing.T) { + dir := t.TempDir() + cfg := tinyWalConfig() + + sm, err := NewSegmentManager(dir, 0, 0, cfg) + if err != nil { + t.Fatalf("NewSegmentManager: %v", err) + } + defer sm.Close() + + encoded, err := EncodeWalBatch(0, []*WalEntry{ + {OpType: OpPut, ValueKind: VKInline, Key: []byte("k"), Value: []byte("v")}, + }) + if err != nil { + t.Fatalf("EncodeWalBatch: %v", err) + } + + // Fill the active segment until next AppendBatch would trigger rotation. + // segment_manager.go:62 triggers rotate when + // RemainingPayload() < len(encoded) + 2*PhysicalRecordHeaderSize + worstCaseSize := uint64(len(encoded)) + 2*uint64(PhysicalRecordHeaderSize) + for sm.RemainingPayload() >= worstCaseSize { + if err := sm.AppendBatch(encoded); err != nil { + t.Fatalf("fill AppendBatch: %v", err) + } + } + + // Inject dir fsync failure for the next segment creation. + orig := dirFsyncFn + dirFsyncFn = func(string) error { return errors.New("simulated dir fsync failure") } + t.Cleanup(func() { dirFsyncFn = orig }) + + // Next AppendBatch must trigger rotation and fail with propagated error. + if err := sm.AppendBatch(encoded); err == nil { + t.Fatal("AppendBatch: expected rotation failure, got nil") + } +} +``` + +> 注意:C8 的存在让"填满 segment 触发轮转"的实际行为不可预测(segment_manager.go:64-66 把字节偏移当 startSequence)。这个测试主要验证 dir fsync 失败的传播路径,不验证轮转后的 recovery。如果测试在 C8 修复前运行不稳定,可放宽为"只需证明错误被传播",不验证后续 recovery。 + +#### B.4 验证 NewSegmentManager 路径 + 清理(Oracle 新增) + +> **Oracle 修订(bg_48d53274)**:原计划只覆盖了 NewSegmentWriter 单元层。需要单独覆盖 NewSegmentManager 包装路径(segment_manager.go:35-37),以及验证 dir fsync 失败后 retry 能成功(清理干净)。 + +```go +// Regression guard for C6: NewSegmentManager must propagate dir fsync +// failure from initial segment creation. This is the DB.Open failure path. +func TestNewSegmentManagerFailsOnDirFsyncFailure(t *testing.T) { + dir := t.TempDir() + cfg := tinyWalConfig() + + orig := dirFsyncFn + dirFsyncFn = func(string) error { return errors.New("simulated dir fsync failure") } + t.Cleanup(func() { dirFsyncFn = orig }) + + sm, err := NewSegmentManager(dir, 0, 0, cfg) + if err == nil { + if sm != nil { + sm.Close() + } + t.Fatal("NewSegmentManager: expected error on dir fsync failure, got nil") + } + if !strings.Contains(err.Error(), "create initial segment") { + t.Errorf("error should be wrapped as 'create initial segment', got: %v", err) + } +} + +// Regression guard for C6: after a failed NewSegmentWriter due to dir +// fsync, retrying with fsync restored must succeed and not leak state +// (no leftover segment files). +func TestNewSegmentWriterRetryAfterDirFsyncFailure(t *testing.T) { + dir := t.TempDir() + cfg := config.Defaults() + + // First attempt: inject failure. + orig := dirFsyncFn + dirFsyncFn = func(string) error { return errors.New("simulated") } + _, err := NewSegmentWriter(dir, 0, 0, &cfg) + if err == nil { + t.Fatal("expected first NewSegmentWriter to fail") + } + dirFsyncFn = orig + + // Second attempt: must succeed (cleanup was effective). + sw, err := NewSegmentWriter(dir, 0, 0, &cfg) + if err != nil { + t.Fatalf("retry NewSegmentWriter: %v", err) + } + defer sw.Close() + + // Verify no leftover .tmp files from the failed attempt. + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + for _, e := range entries { + if strings.HasSuffix(e.Name(), ".tmp") { + t.Errorf("leftover .tmp file: %s", e.Name()) + } + // segment-0.wal should exist (from successful retry). + } +} +``` + +### Phase C:验证(15 分钟) + +```bash +# 1. 编译通过 +go build ./... + +# 2. 重点测试(C6 新增的全部) +go test ./wal -run 'TestDirFsync|TestNewSegmentWriter|TestSegmentManagerRotateFailsOnDirFsyncFailure|TestNewSegmentManagerFailsOnDirFsyncFailure' -count=1 -v + +# 3. wal 包全量 +go test ./wal/... -count=1 + +# 4. 全仓 +go test ./... -count=1 + +# 5. race 全仓(Oracle 修订:不只 wal + root,跑 ./...) +go test -race ./... -count=1 + +# 6. vet +go vet ./... + +# 7. lint(如果装了) +if command -v golangci-lint >/dev/null 2>&1; then + golangci-lint run ./wal/... +else + echo "golangci-lint not installed, skipping" +fi +``` + +### Phase D:Commit message draft + +``` +fix: make WAL segment directory fsync failure fatal (C6) + +Per design §3.2 line 248-272, segment directory fsync is a hard +requirement for durable-ready state, not best-effort. rename is atomic +in memory but not guaranteed to survive power loss without a directory +fsync. The previous code silently swallowed both os.Open(dir) and +dirFD.Sync() errors, leaving WAL writer to confirm batches as durable +when their segment might not exist after a crash. + +Failure propagation: +- Initial segment creation: NewSegmentWriter fails -> NewSegmentManager + fails -> DB.Open fails (user sees error, no data promise violated). +- Rotation during AppendBatch: NewSegmentWriter fails -> AppendBatch + fails -> WalWriter.stopWithError(ErrCommitUnknown) -> write-stopped + (per design line 272). + +Changes: +- wal/segment_writer.go: extract dirFsync helper (Open -> f.Stat -> + IsDir -> f.Sync, avoiding TOCTOU window), replace silent swallow with + fatal error; on failure clean up resources (fd.Close + os.Remove) and + surface cleanup errors via errors.Join so nothing is silently lost. +- wal/dir_fsync_test.go (new): unit test the helper with valid dir, + non-existent dir (fails at os.Open), and not-a-dir (fails at IsDir). +- wal/segment_writer_test.go: add TestNewSegmentWriterDirFsyncFailure + (injects failure via package-level dirFsyncFn override; documents the + not-parallel-safe constraint), TestNewSegmentWriterNormalPathStillWorks + (regression), and TestNewSegmentWriterRetryAfterDirFsyncFailure + (verifies cleanup is effective for retry). +- wal/segment_manager_test.go: add TestSegmentManagerRotateFailsOnDirFsyncFailure + (fills segment until rotation triggers, injects failure, verifies + propagation through AppendBatch path) and TestNewSegmentManagerFailsOnDirFsyncFailure + (covers the DB.Open failure path). + +dirFsyncFn injection note: tests that override this package-level var +must not use t.Parallel(). All existing wal tests run serially within +the package; this is the lightest mechanism that doesn't require +interface indirection in production code. + +Verified: each new test fails on pre-fix code (silent swallow returned +nil error) and passes after the fix. Full suite green including +go test -race ./... . + +Audit context: docs/audit-3.2.md C6 (Oracle-verified bg_ef425776). +``` + +--- + +## 验收清单 + +- [ ] Phase A.1:`dirFsync` helper 存在,docstring 引用设计行号,注释 "NOT PARALLEL-SAFE" +- [ ] Phase A.2:`NewSegmentWriter` 调用 `dirFsyncFn`,失败时 `errors.Join` 合并 fd.Close + os.Remove 错误 +- [ ] Phase B.1:3 个 helper 单元测试存在,错误期望正确("open dir" / "not a directory") +- [ ] Phase B.2:2 个 NewSegmentWriter 测试(failure + regression)存在 +- [ ] Phase B.3:rotate 失败测试存在,编译正确,用循环填到 `RemainingPayload() < worstCaseSize` +- [ ] Phase B.4:NewSegmentManager 失败测试 + retry-after-failure 测试存在 +- [ ] `go test ./wal/... -count=1` 全绿 +- [ ] `go test ./... -count=1` 全绿 +- [ ] `go test -race ./... -count=1` 全绿 +- [ ] `go vet ./...` 无新增警告 +- [ ] 单次 commit,message 引用 audit C6 + +--- + +## 不在本次范围内(后续 issue) + +| 编号 | 为什么不放进来 | +|------|---------------| +| **C8** | segment_manager 把字节偏移当 startSequence,多 segment recovery 直接坏。和 C6 完全独立,但优先级也很高,可单独做 | +| C4 | 涉及 `RecoverFromSegments` 接口变更(isLast 参数),改动面更大 | +| C5 + H8 | 都在 truncation 路径,应一起做(fsync + 删空 segment + dir fsync + findValidOffset batch 边界) | +| C1 | CRC 多项式错,跨多个文件,需要测试向量 | +| C7 | Put/Close 并发竞态,独立 | +| H1-H7 | 其他 High 级别问题 | + +--- + +## 修订记录 + +- **v1(原始)**:C6 修复方案初稿,送 Momus 审 +- **v1.1(Momus 修订 bg_90bcf359)**: + - helper 加 `os.Stat` + `IsDir` 校验,避免传文件路径误"成功" + - rotate 测试改用现成的 `tinyWalConfig()` helper(同时减 BlockSize + MaxBatchSize + MaxSegmentSize),避免单独设小 segment 触发 config.Validate 失败 +- **v1.2(Oracle 修订 bg_48d53274)**: + - **BLOCKING**:helper 改为 `os.Open → f.Stat → IsDir → f.Sync` 顺序,消除 TOCTOU 窗口;同步修正 `TestDirFsyncNonExistentDir` 错误期望("open dir" 而非 "stat dir") + - **BLOCKING**:`TestDirFsyncNotADirectory` 缺 `os` import,补齐 + - **BLOCKING**:`TestSegmentManagerRotateFailsOnDirFsyncFailure` 编译错(`_, err =` 应为 `err =`);单个 batch 不会触发轮转,改为循环填到 `RemainingPayload() < worstCaseSize` + - **NEW**:加 `TestNewSegmentManagerFailsOnDirFsyncFailure`(覆盖 NewSegmentManager→DB.Open 失败路径) + - **NEW**:加 `TestNewSegmentWriterRetryAfterDirFsyncFailure`(验证清理后 retry 能成功) + - **MINOR**:A.2 cleanup 用 `errors.Join` 把 close/remove 错误和主错误合并,不丢任何信息 + - **MINOR**:`dirFsyncFn` 旁注释 "NOT PARALLEL-SAFE" + - **MINOR**:race 改成 `go test -race ./...`(全仓,不只 wal + root) diff --git a/wal/dir_fsync.go b/wal/dir_fsync.go new file mode 100644 index 0000000..043e27a --- /dev/null +++ b/wal/dir_fsync.go @@ -0,0 +1,42 @@ +package wal + +import ( + "fmt" + "os" +) + +// dirFsyncFn is the package-level indirection over dirFsync so tests can +// inject failures without interface plumbing in production code. +// +// NOT PARALLEL-SAFE: tests that override this must not use t.Parallel(). +// All existing wal tests run serially within the package. +var dirFsyncFn = dirFsync + +// dirFsync opens the directory and fsyncs it. Required for durable-ready +// state per design §3.2 line 258. This is a hard requirement, not +// best-effort: rename is atomic in memory but not guaranteed to survive +// power loss without a directory fsync. +// +// Order: os.Open → f.Stat → IsDir → f.Sync. The open-then-stat sequence +// avoids the TOCTOU window between a separate os.Stat and os.Open, and +// ensures IsDir is checked against the actually-opened file. +func dirFsync(dir string) error { + f, err := os.Open(dir) + if err != nil { + return fmt.Errorf("open dir %q: %w", dir, err) + } + defer f.Close() + + info, err := f.Stat() + if err != nil { + return fmt.Errorf("stat dir %q: %w", dir, err) + } + if !info.IsDir() { + return fmt.Errorf("path %q is not a directory", dir) + } + + if err := f.Sync(); err != nil { + return fmt.Errorf("fsync dir %q: %w", dir, err) + } + return nil +} diff --git a/wal/dir_fsync_test.go b/wal/dir_fsync_test.go new file mode 100644 index 0000000..6235cbf --- /dev/null +++ b/wal/dir_fsync_test.go @@ -0,0 +1,39 @@ +package wal + +import ( + "os" + "strings" + "testing" +) + +func TestDirFsyncSuccess(t *testing.T) { + dir := t.TempDir() + if err := dirFsync(dir); err != nil { + t.Errorf("dirFsync on valid dir: %v", err) + } +} + +func TestDirFsyncNonExistentDir(t *testing.T) { + err := dirFsync("/nonexistent/path/that/should/not/exist") + if err == nil { + t.Fatal("expected error on non-existent dir") + } + if !strings.Contains(err.Error(), "open dir") { + t.Errorf("error should mention 'open dir', got: %v", err) + } +} + +func TestDirFsyncNotADirectory(t *testing.T) { + dir := t.TempDir() + filePath := dir + "/notadir" + if err := os.WriteFile(filePath, []byte("x"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + err := dirFsync(filePath) + if err == nil { + t.Fatal("expected error when fsyncing a file as dir") + } + if !strings.Contains(err.Error(), "not a directory") { + t.Errorf("error should mention 'not a directory', got: %v", err) + } +} diff --git a/wal/segment_manager_test.go b/wal/segment_manager_test.go index 9d54663..99cfb71 100644 --- a/wal/segment_manager_test.go +++ b/wal/segment_manager_test.go @@ -1,8 +1,10 @@ package wal import ( + "errors" "os" "path/filepath" + "strings" "testing" "github.com/dailz/go-kv/config" @@ -239,3 +241,67 @@ func itoa(n uint64) string { } return string(buf[i:]) } + +// Regression guard for C6: NewSegmentManager must propagate dir fsync +// failure from initial segment creation. This is the DB.Open failure path. +func TestNewSegmentManagerFailsOnDirFsyncFailure(t *testing.T) { + dir := t.TempDir() + cfg := tinyWalConfig() + + orig := dirFsyncFn + dirFsyncFn = func(string) error { return errors.New("simulated dir fsync failure") } + t.Cleanup(func() { dirFsyncFn = orig }) + + sm, err := NewSegmentManager(dir, 0, 0, cfg) + if err == nil { + if sm != nil { + sm.Close() + } + t.Fatal("NewSegmentManager: expected error on dir fsync failure, got nil") + } + if !strings.Contains(err.Error(), "create initial segment") { + t.Errorf("error should be wrapped as 'create initial segment', got: %v", err) + } +} + +// Regression guard for C6: SegmentManager.AppendBatch must propagate +// rotation failure (which now includes dir fsync failure) as error. +// +// Note: C8 (segment_manager.go:64-66 passes byte offset as startSequence) +// makes multi-segment recovery broken, but this test only verifies error +// propagation through AppendBatch; it does not exercise recovery. +func TestSegmentManagerRotateFailsOnDirFsyncFailure(t *testing.T) { + dir := t.TempDir() + cfg := tinyWalConfig() + + sm, err := NewSegmentManager(dir, 0, 0, cfg) + if err != nil { + t.Fatalf("NewSegmentManager: %v", err) + } + defer sm.Close() + + encoded, err := EncodeWalBatch(0, []*WalEntry{ + {OpType: OpPut, ValueKind: VKInline, Key: []byte("k"), Value: []byte("v")}, + }) + if err != nil { + t.Fatalf("EncodeWalBatch: %v", err) + } + + // Fill the active segment until next AppendBatch would trigger rotation. + // segment_manager.go:62 triggers rotate when + // RemainingPayload() < len(encoded) + 2*PhysicalRecordHeaderSize + worstCaseSize := uint64(len(encoded)) + 2*uint64(PhysicalRecordHeaderSize) + for sm.RemainingPayload() >= worstCaseSize { + if err := sm.AppendBatch(encoded); err != nil { + t.Fatalf("fill AppendBatch: %v", err) + } + } + + orig := dirFsyncFn + dirFsyncFn = func(string) error { return errors.New("simulated dir fsync failure") } + t.Cleanup(func() { dirFsyncFn = orig }) + + if err := sm.AppendBatch(encoded); err == nil { + t.Fatal("AppendBatch: expected rotation failure, got nil") + } +} diff --git a/wal/segment_writer.go b/wal/segment_writer.go index d0ad47e..6b45a77 100644 --- a/wal/segment_writer.go +++ b/wal/segment_writer.go @@ -1,6 +1,7 @@ package wal import ( + "errors" "fmt" "os" "path/filepath" @@ -82,10 +83,18 @@ func NewSegmentWriter( return nil, fmt.Errorf("wal: open segment file for append: %w", err) } - // Sync directory to make rename durable (best-effort on Linux). - if dirFD, derr := os.Open(dir); derr == nil { - dirFD.Sync() - dirFD.Close() + // Per design §3.2 line 258, directory fsync is a hard requirement for + // durable-ready. Without it, the rename above is not guaranteed to survive + // power loss, violating the Always-mode "no loss of acknowledged writes" + // promise. + if err := dirFsyncFn(dir); err != nil { + closeErr := fd.Close() + removeErr := os.Remove(finalPath) + if closeErr != nil || removeErr != nil { + cleanup := errors.Join(closeErr, removeErr) + return nil, fmt.Errorf("wal: fsync directory after segment rename (cleanup: %v): %w", cleanup, err) + } + return nil, fmt.Errorf("wal: fsync directory after segment rename: %w", err) } return &SegmentWriter{ diff --git a/wal/segment_writer_test.go b/wal/segment_writer_test.go index ae48075..6703eb3 100644 --- a/wal/segment_writer_test.go +++ b/wal/segment_writer_test.go @@ -1,9 +1,11 @@ package wal import ( + "errors" "fmt" "os" "path/filepath" + "strings" "testing" "github.com/dailz/go-kv/config" @@ -344,3 +346,99 @@ func TestSegmentWriterSync(t *testing.T) { t.Fatalf("Close: %v", err) } } + +// Regression guard for C6: NewSegmentWriter must return an error when +// directory fsync fails, instead of silently succeeding. Per design §3.2 +// line 272, segment must NOT become active if durable-ready fails. +// +// dirFsyncFn is a package-level var; tests that override it must not use +// t.Parallel(). All wal tests run serially within the package. +func TestNewSegmentWriterDirFsyncFailure(t *testing.T) { + dir := t.TempDir() + cfg := config.Defaults() + + orig := dirFsyncFn + dirFsyncFn = func(string) error { return errors.New("simulated dir fsync failure") } + t.Cleanup(func() { dirFsyncFn = orig }) + + sw, err := NewSegmentWriter(dir, 0, 0, &cfg) + if err == nil { + if sw != nil { + sw.Close() + } + t.Fatal("NewSegmentWriter: expected error on dir fsync failure, got nil") + } + + if !strings.Contains(err.Error(), "fsync directory") { + t.Errorf("error should mention 'fsync directory', got: %v", err) + } + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + for _, e := range entries { + name := e.Name() + if strings.Contains(name, "segment-0") { + t.Errorf("segment file should be cleaned up, found: %s", name) + } + } +} + +// Regression guard for C6: ensure normal path still works after the fix. +func TestNewSegmentWriterNormalPathStillWorks(t *testing.T) { + dir := t.TempDir() + cfg := config.Defaults() + + sw, err := NewSegmentWriter(dir, 0, 0, &cfg) + if err != nil { + t.Fatalf("NewSegmentWriter normal path: %v", err) + } + defer sw.Close() + + if _, err := os.Stat(sw.SegmentPath()); err != nil { + t.Errorf("segment file should exist: %v", err) + } +} + +// Regression guard for C6: after a failed NewSegmentWriter due to dir +// fsync, retrying with fsync restored must succeed and not leak state. +func TestNewSegmentWriterRetryAfterDirFsyncFailure(t *testing.T) { + dir := t.TempDir() + cfg := config.Defaults() + + orig := dirFsyncFn + dirFsyncFn = func(string) error { return errors.New("simulated") } + _, err := NewSegmentWriter(dir, 0, 0, &cfg) + if err == nil { + t.Fatal("expected first NewSegmentWriter to fail") + } + dirFsyncFn = orig + + sw, err := NewSegmentWriter(dir, 0, 0, &cfg) + if err != nil { + t.Fatalf("retry NewSegmentWriter: %v", err) + } + defer sw.Close() + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + tmpCount := 0 + walCount := 0 + for _, e := range entries { + if strings.HasSuffix(e.Name(), ".tmp") { + tmpCount++ + } + if strings.HasSuffix(e.Name(), ".wal") { + walCount++ + } + } + if tmpCount != 0 { + t.Errorf("leftover .tmp files: %d", tmpCount) + } + if walCount != 1 { + t.Errorf("expected exactly 1 .wal file (from successful retry), got %d", walCount) + } +}