From 370ac5ff37e5c5b162615dfb4cdc291750dd36df Mon Sep 17 00:00:00 2001 From: Zhe Li Date: Tue, 25 Aug 2026 15:15:59 +0000 Subject: [PATCH] fix(registry): move instance lock outside data dir for Windows remove Windows cannot unlink state.json.lock while remove still holds the open handle; keep the lock at .locks/.lock so Delete can RemoveAll safely. --- internal/localnet/progress.go | 2 +- internal/registry/lock_unix.go | 9 +++++--- internal/registry/lock_windows.go | 9 +++++--- internal/registry/state.go | 20 ++++++++++++++++-- internal/registry/state_test.go | 35 +++++++++++++++++++++++++++++++ 5 files changed, 66 insertions(+), 9 deletions(-) diff --git a/internal/localnet/progress.go b/internal/localnet/progress.go index 91db2333..1b1ec093 100644 --- a/internal/localnet/progress.go +++ b/internal/localnet/progress.go @@ -27,7 +27,7 @@ const ( // catalogue lookup, upstream fallback if AllowUncurated. StepResolveVersion Step = "resolve_version" // StepAcquireLock: 2 · "Acquire instance lock" — flock on - // ~/.canton-devkit/localnet//.lock. + // ~/.canton-devkit/localnet/.locks/.lock. StepAcquireLock Step = "acquire_lock" // StepPreflight: 3 · "Run preflight checks" — docker daemon / // compose v2 / disk / memory thresholds. diff --git a/internal/registry/lock_unix.go b/internal/registry/lock_unix.go index 27b5f0f9..46640d44 100644 --- a/internal/registry/lock_unix.go +++ b/internal/registry/lock_unix.go @@ -13,15 +13,18 @@ import ( // blocking concurrent `localnet up`/`down` against the same instance from // the same host. The returned release function must be called via defer. // The Windows counterpart (LockFileEx) lives in lock_windows.go. +// +// The lock file lives at .locks/.lock under the registry root +// (outside the instance data dir) so Delete can RemoveAll the data dir +// while this lock is still held. func Lock(name string) (release func(), err error) { if err := ValidateName(name); err != nil { return nil, err } - dir := DataDirFor(name) - if err := os.MkdirAll(dir, 0o700); err != nil { + path := LockPathFor(name) + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return nil, fmt.Errorf("mkdir for lock: %w", err) } - path := filepath.Join(dir, "state.json.lock") f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0o600) if err != nil { diff --git a/internal/registry/lock_windows.go b/internal/registry/lock_windows.go index c50fc227..532ac6ad 100644 --- a/internal/registry/lock_windows.go +++ b/internal/registry/lock_windows.go @@ -21,15 +21,18 @@ import ( // LOCK_EX|LOCK_NB behaviour on Unix. The OS releases the lock byte-range // automatically when the handle is closed or the process exits, so there // is no stale-lock file to recover after a crash. +// +// The lock file lives at .locks/.lock under the registry root +// (outside the instance data dir) so Delete can RemoveAll the data dir +// while this lock is still held — Windows cannot unlink an open file. func Lock(name string) (release func(), err error) { if err := ValidateName(name); err != nil { return nil, err } - dir := DataDirFor(name) - if err := os.MkdirAll(dir, 0o700); err != nil { + path := LockPathFor(name) + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return nil, fmt.Errorf("mkdir for lock: %w", err) } - path := filepath.Join(dir, "state.json.lock") h, err := openLockHandle(path) if err != nil { diff --git a/internal/registry/state.go b/internal/registry/state.go index 444a495b..ad34cd7f 100644 --- a/internal/registry/state.go +++ b/internal/registry/state.go @@ -5,11 +5,15 @@ // On-disk layout under ~/.canton-devkit/localnet/: // // index.json — directory of every instance (for `list`) +// .locks/.lock — flock/LockFileEx advisory lock for concurrent ops // /state.json — full per-instance metadata -// /state.json.lock — flock advisory lock for concurrent ops // /overlay.env — generated env-file overlay (written by up) // /containers.yaml — generated container-rename overlay // +// The lock lives outside the instance data dir so `Delete` can +// `RemoveAll` the data dir while the lock is still held — required on +// Windows, which cannot unlink an open file. +// // All writes are atomic (tmp + rename), state.json is mode 0600 (it // holds captured JWTs), and concurrent up/down on the same instance is // rejected by the lock. @@ -181,7 +185,7 @@ var validInstanceName = regexp.MustCompile(`^[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9]) // ValidateName rejects instance names that could escape the registry // root or break tooling. Called from every public entry point that -// accepts a name (PathFor, DataDirFor, Read, Write, Delete, Lock). +// accepts a name (PathFor, DataDirFor, LockPathFor, Read, Write, Delete, Lock). // The check is deliberately conservative — it's far easier to widen // later than to narrow after users depend on quirky names. // @@ -248,6 +252,18 @@ func DataDirFor(name string) string { return filepath.Join(Root(), name) } +// LockPathFor returns the per-instance advisory lock path +// (.locks/.lock under the registry root). Kept outside the data +// dir so Delete can RemoveAll the instance directory while Lock still +// holds the file open (Windows cannot unlink an open handle). +// Same panic-on-invalid semantics as PathFor. +func LockPathFor(name string) string { + if err := ValidateName(name); err != nil { + panic(fmt.Sprintf("registry.LockPathFor called with invalid name: %v", err)) + } + return filepath.Join(Root(), ".locks", name+".lock") +} + // NewState builds a fresh State with defaults applied. Callers fill in the // rest before the first Write. func NewState(name, spliceVersion string) *State { diff --git a/internal/registry/state_test.go b/internal/registry/state_test.go index 8a2e46bc..6818d7fe 100644 --- a/internal/registry/state_test.go +++ b/internal/registry/state_test.go @@ -327,6 +327,41 @@ func TestLockExcludesConcurrentOps(t *testing.T) { rel2() } +// TestDeleteWhileLockedSucceeds is the regression for Windows remove: +// Delete must RemoveAll the instance data dir while Lock still holds +// the open handle. That only works because the lock lives outside the +// data dir (.locks/.lock). Runs on all platforms — do not skip +// on Windows. +func TestDeleteWhileLockedSucceeds(t *testing.T) { + useTmpRoot(t) + + if err := Write(NewState("alice", "0.6.4")); err != nil { + t.Fatal(err) + } + + release, err := Lock("alice") + if err != nil { + t.Fatalf("lock: %v", err) + } + + if err := Delete("alice"); err != nil { + release() + t.Fatalf("Delete while holding lock: %v", err) + } + if _, err := Read("alice"); err != ErrNotFound { + release() + t.Fatalf("after Delete: want ErrNotFound, got %v", err) + } + + release() + + release2, err := Lock("alice") + if err != nil { + t.Fatalf("re-lock after Delete+release: %v", err) + } + release2() +} + func TestConcurrentWritesDifferentInstancesAllSucceed(t *testing.T) { useTmpRoot(t)