go-kv/.omo/plans/fix-c6-segment-dir-fsync.md

# 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）