Skip to content

go/store/nbs: file_manifest.go: Always make manifest writes crash safe. Remove optional async manifest write support. #10319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
reltuk merged 2 commits into from
Jan 20, 2026
Merged

go/store/nbs: file_manifest.go: Always make manifest writes crash safe. Remove optional async manifest write support. #10319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ceeb9f5
go/store/nbs: file_manifest.go: Always make manifest writes crash saf…
reltuk Jan 15, 2026
14ac7f4
Merge remote-tracking branch 'origin/main' into aaron/oldgen-sync-man…
reltuk Jan 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 10 additions & 22 deletions go/store/nbs/file_manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func MaybeMigrateFileManifest(ctx context.Context, dir string) (bool, error) {
return nil
}

_, err = updateWithChecker(ctx, dir, syncFlush, check, contents.lock, contents, nil)
_, err = updateWithChecker(ctx, dir, check, contents.lock, contents, nil)

if err != nil {
return false, err
Expand All @@ -95,9 +95,9 @@ func MaybeMigrateFileManifest(ctx context.Context, dir string) (bool, error) {
}

// getFileManifest makes a new file manifest.
func getFileManifest(ctx context.Context, dir string, mode updateMode) (m manifest, err error) {
func getFileManifest(ctx context.Context, dir string) (m manifest, err error) {
lock := fslock.New(filepath.Join(dir, lockFileName))
m = fileManifest{dir: dir, mode: mode, lock: lock}
m = fileManifest{dir: dir, lock: lock}

var f *os.File
f, err = openIfExists(filepath.Join(dir, manifestFileName))
Expand All @@ -123,17 +123,9 @@ func getFileManifest(ctx context.Context, dir string, mode updateMode) (m manife
return
}

type updateMode byte

const (
asyncFlush updateMode = 0
syncFlush updateMode = 1
)

type fileManifest struct {
lock *fslock.Lock
dir string
mode updateMode
}

// Returns nil if path does not exist
Expand Down Expand Up @@ -190,7 +182,7 @@ func (fm fileManifest) Update(ctx context.Context, lastLock hash.Hash, newConten
return nil
}

return updateWithChecker(ctx, fm.dir, fm.mode, checker, lastLock, newContents, writeHook)
return updateWithChecker(ctx, fm.dir, checker, lastLock, newContents, writeHook)
}

func (fm fileManifest) UpdateGCGen(ctx context.Context, lastLock hash.Hash, newContents manifestContents, stats *Stats, writeHook func() error) (mc manifestContents, err error) {
Expand All @@ -207,7 +199,7 @@ func (fm fileManifest) UpdateGCGen(ctx context.Context, lastLock hash.Hash, newC
}
}()

return updateWithChecker(ctx, fm.dir, fm.mode, updateGCGenManifestCheck, lastLock, newContents, writeHook)
return updateWithChecker(ctx, fm.dir, updateGCGenManifestCheck, lastLock, newContents, writeHook)
}

// parseV5Manifest parses the v5 manifest from the Reader given. Assumes the first field (the manifest version and
Expand Down Expand Up @@ -369,7 +361,7 @@ func parseIfExists(_ context.Context, dir string, readHook func() error) (exists
}

// updateWithChecker updates the manifest if |validate| is satisfied, callers must hold the file lock.
func updateWithChecker(_ context.Context, dir string, mode updateMode, validate manifestChecker, lastLock hash.Hash, newContents manifestContents, writeHook func() error) (mc manifestContents, err error) {
func updateWithChecker(_ context.Context, dir string, validate manifestChecker, lastLock hash.Hash, newContents manifestContents, writeHook func() error) (mc manifestContents, err error) {
var tempManifestPath string

// Write a temporary manifest file, to be renamed over manifestFileName upon success.
Expand All @@ -394,10 +386,8 @@ func updateWithChecker(_ context.Context, dir string, mode updateMode, validate
return "", ferr
}

if mode == syncFlush {
if ferr = temp.Sync(); ferr != nil {
return "", ferr
}
if ferr = temp.Sync(); ferr != nil {
return "", ferr
}

return temp.Name(), nil
Expand Down Expand Up @@ -473,10 +463,8 @@ func updateWithChecker(_ context.Context, dir string, mode updateMode, validate
return manifestContents{}, err
}

if mode == syncFlush {
if err = file.SyncDirectoryHandle(dir); err != nil {
return manifestContents{}, err
}
if err = file.SyncDirectoryHandle(dir); err != nil {
return manifestContents{}, err
}

return newContents, nil
Expand Down
4 changes: 2 additions & 2 deletions go/store/nbs/file_manifest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import (
func makeFileManifestTempDir(t *testing.T) fileManifest {
dir, err := os.MkdirTemp("", "")
require.NoError(t, err)
fm, err := getFileManifest(context.Background(), dir, asyncFlush)
fm, err := getFileManifest(context.Background(), dir)
require.NoError(t, err)
return fm.(fileManifest)
}
Expand Down Expand Up @@ -106,7 +106,7 @@ func TestFileManifestUpdateEmpty(t *testing.T) {
assert.True(upstream.root.IsEmpty())
assert.Empty(upstream.specs)

fm2, err := getFileManifest(context.Background(), fm.dir, asyncFlush) // Open existent, but empty manifest
fm2, err := getFileManifest(context.Background(), fm.dir) // Open existent, but empty manifest
require.NoError(t, err)
exists, upstream, err := fm2.ParseIfExists(context.Background(), stats, nil)
require.NoError(t, err)
Expand Down
4 changes: 2 additions & 2 deletions go/store/nbs/journal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,7 @@ func (jm *journalManifest) Update(ctx context.Context, lastLock hash.Hash, newCo
}
return nil
}
return updateWithChecker(ctx, jm.dir, syncFlush, checker, lastLock, newContents, writeHook)
return updateWithChecker(ctx, jm.dir, checker, lastLock, newContents, writeHook)
}

// UpdateGCGen implements manifest.
Expand All @@ -649,7 +649,7 @@ func (jm *journalManifest) UpdateGCGen(ctx context.Context, lastLock hash.Hash,

t1 := time.Now()
defer func() { stats.WriteManifestLatency.SampleTimeSince(t1) }()
return updateWithChecker(ctx, jm.dir, syncFlush, updateGCGenManifestCheck, lastLock, newContents, writeHook)
return updateWithChecker(ctx, jm.dir, updateGCGenManifestCheck, lastLock, newContents, writeHook)
}

func updateGCGenManifestCheck(upstream, contents manifestContents) error {
Expand Down
2 changes: 1 addition & 1 deletion go/store/nbs/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ func newLocalStore(ctx context.Context, nbfVerStr string, dir string, memTableSi
return nil, fmt.Errorf("cannot create NBS store for directory containing chunk journal: %s", dir)
}

m, err := getFileManifest(ctx, dir, asyncFlush)
m, err := getFileManifest(ctx, dir)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion go/store/nbs/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func makeTestLocalStore(t *testing.T, maxTableFiles int) (st *NomsBlockStore, no
require.NoError(t, err)

// create a v5 manifest
fm, err := getFileManifest(ctx, nomsDir, asyncFlush)
fm, err := getFileManifest(ctx, nomsDir)
require.NoError(t, err)
_, err = fm.Update(ctx, hash.Hash{}, manifestContents{
nbfVers: constants.FormatDoltString,
Expand Down
Loading