From 610ee13ab395768ff5f2dfaff899fb3ba2a5a514 Mon Sep 17 00:00:00 2001 From: Ophestra Date: Fri, 16 Jan 2026 18:09:10 +0900 Subject: [PATCH] internal/pkg: lock on-filesystem cache Any fine-grained file-based locking here significantly hurts performance and is not part of the use case of the package. This change guarantees exclusive access to prevent inconsistent state on the filesystem. Signed-off-by: Ophestra --- internal/pkg/pkg.go | 58 ++++++++++++++++++++++++++++++++++++---- internal/pkg/pkg_test.go | 34 ++++++++++++++++------- 2 files changed, 77 insertions(+), 15 deletions(-) diff --git a/internal/pkg/pkg.go b/internal/pkg/pkg.go index d127453..e0b50cf 100644 --- a/internal/pkg/pkg.go +++ b/internal/pkg/pkg.go @@ -22,10 +22,12 @@ import ( "strings" "sync" "syscall" + "testing" "unique" "unsafe" "hakurei.app/container/check" + "hakurei.app/internal/lockedfile" "hakurei.app/message" ) @@ -353,6 +355,10 @@ const ( ) const ( + // fileLock is the file name appended to Cache.base for guaranteeing + // exclusive access to the cache directory. + fileLock = "lock" + // dirIdentifier is the directory name appended to Cache.base for storing // artifacts named after their [ID]. dirIdentifier = "identifier" @@ -438,6 +444,11 @@ type Cache struct { identPending map[unique.Handle[ID]]<-chan struct{} // Synchronises access to ident and corresponding filesystem entries. identMu sync.RWMutex + + // Unlocks the on-filesystem cache. Must only be called from Close. + unlock func() + // Synchronises calls to Close. + closeOnce sync.Once } // IsStrict returns whether the [Cache] strictly verifies checksums. @@ -1405,19 +1416,44 @@ func (pending *pendingArtifactDep) cure(c *Cache) { } // Close cancels all pending cures and waits for them to clean up. -func (c *Cache) Close() { c.cancel(); c.wg.Wait() } +func (c *Cache) Close() { + c.closeOnce.Do(func() { + c.cancel() + c.wg.Wait() + c.unlock() + }) +} -// New returns the address to a new instance of [Cache]. Concurrent cures of -// dependency [Artifact] is limited to the caller-supplied value, however direct -// calls to [Cache.Cure] is not subject to this limitation. +// Open returns the address of a newly opened instance of [Cache]. +// +// Concurrent cures of a [FloodArtifact] dependency graph is limited to the +// caller-supplied value, however direct calls to [Cache.Cure] is not subject +// to this limitation. // // A cures value of 0 or lower is equivalent to the value returned by // [runtime.NumCPU]. -func New( +// +// A successful call to Open guarantees exclusive access to the on-filesystem +// cache for the resulting instance of [Cache]. The [Cache.Close] method cancels +// and waits for pending cures on [Cache] before releasing this lock and must be +// called once the [Cache] is no longer needed. +func Open( ctx context.Context, msg message.Msg, cures int, base *check.Absolute, +) (*Cache, error) { + return open(ctx, msg, cures, base, true) +} + +// open implements Open but allows omitting the [lockedfile] lock when called +// from a test. This is used to simulate invalid states in the test suite. +func open( + ctx context.Context, + msg message.Msg, + cures int, + base *check.Absolute, + lock bool, ) (*Cache, error) { for _, name := range []string{ dirIdentifier, @@ -1443,6 +1479,18 @@ func New( c.cureDep = cureDep c.identPool.New = func() any { return new(extIdent) } + if lock || !testing.Testing() { + if unlock, err := lockedfile.MutexAt( + base.Append(fileLock).String(), + ).Lock(); err != nil { + return nil, err + } else { + c.unlock = unlock + } + } else { + c.unlock = func() {} + } + if cures < 1 { cures = runtime.NumCPU() } diff --git a/internal/pkg/pkg_test.go b/internal/pkg/pkg_test.go index 7e22aa6..586385b 100644 --- a/internal/pkg/pkg_test.go +++ b/internal/pkg/pkg_test.go @@ -28,6 +28,15 @@ import ( "hakurei.app/message" ) +//go:linkname unsafeOpen hakurei.app/internal/pkg.open +func unsafeOpen( + ctx context.Context, + msg message.Msg, + cures int, + base *check.Absolute, + lock bool, +) (*pkg.Cache, error) + func TestMain(m *testing.M) { container.TryArgv0(nil); os.Exit(m.Run()) } // overrideIdent overrides the ID method of [Artifact]. @@ -213,7 +222,7 @@ func TestIdent(t *testing.T) { var cache *pkg.Cache if a, err := check.NewAbs(t.TempDir()); err != nil { t.Fatal(err) - } else if cache, err = pkg.New(t.Context(), msg, 0, a); err != nil { + } else if cache, err = pkg.Open(t.Context(), msg, 0, a); err != nil { t.Fatal(err) } t.Cleanup(cache.Close) @@ -274,8 +283,8 @@ func checkWithCache(t *testing.T, testCases []cacheTestCase) { msg.SwapVerbose(testing.Verbose()) var scrubFunc func() error // scrub after hashing - if c, err := pkg.New(t.Context(), msg, 0, base); err != nil { - t.Fatalf("New: error = %v", err) + if c, err := pkg.Open(t.Context(), msg, 0, base); err != nil { + t.Fatalf("Open: error = %v", err) } else { t.Cleanup(c.Close) if tc.early != nil { @@ -294,6 +303,11 @@ func checkWithCache(t *testing.T, testCases []cacheTestCase) { restoreTemp = true } + // destroy lock file to avoid changing cache checksums + if err := os.Remove(base.Append("lock").String()); err != nil { + t.Fatal(err) + } + var checksum pkg.Checksum if err := pkg.HashDir(&checksum, base); err != nil { t.Fatalf("HashDir: error = %v", err) @@ -522,12 +536,12 @@ func TestCache(t *testing.T) { )}, }) - if c0, err := pkg.New( + if c0, err := unsafeOpen( t.Context(), message.New(nil), - 0, base, + 0, base, false, ); err != nil { - t.Fatalf("New: error = %v", err) + t.Fatalf("open: error = %v", err) } else { t.Cleanup(c.Close) // check doubled cancel cureMany(t, c0, []cureStep{ @@ -1017,12 +1031,12 @@ func TestNew(t *testing.T) { Path: container.Nonexistent, Err: syscall.ENOENT, } - if _, err := pkg.New( + if _, err := pkg.Open( t.Context(), message.New(nil), 0, check.MustAbs(container.Nonexistent), ); !reflect.DeepEqual(err, wantErr) { - t.Errorf("New: error = %#v, want %#v", err, wantErr) + t.Errorf("Open: error = %#v, want %#v", err, wantErr) } }) @@ -1045,12 +1059,12 @@ func TestNew(t *testing.T) { Path: tempDir.Append("cache").String(), Err: syscall.EACCES, } - if _, err := pkg.New( + if _, err := pkg.Open( t.Context(), message.New(nil), 0, tempDir.Append("cache"), ); !reflect.DeepEqual(err, wantErr) { - t.Errorf("New: error = %#v, want %#v", err, wantErr) + t.Errorf("Open: error = %#v, want %#v", err, wantErr) } }) }