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).
This commit is contained in:
@@ -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
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
|
||||
+13
-4
@@ -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{
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user