From b366dfa9ac1d6b308139b6170821f59ba76d20d4 Mon Sep 17 00:00:00 2001 From: Cheviiot <153805936+Cheviiot@users.noreply.github.com> Date: Sat, 25 Jul 2026 18:10:56 +1000 Subject: [PATCH] Add a concurrency lock for download/install against the same destination combineDirTrees' merge logic assumes it's the only thing moving files into a given target at a time; two `vintner download`/`install` runs racing against the same --dest could otherwise interleave os.Rename calls and corrupt the tree instead of erroring cleanly. Take an exclusive, non-blocking flock(2) on the destination for the duration of each run, so a second invocation fails immediately with a clear message instead of silently colliding with the first. --- cmd/vintner/download.go | 7 ++ internal/install/install.go | 7 ++ internal/lock/lock.go | 49 ++++++++++++++ internal/lock/lock_test.go | 124 ++++++++++++++++++++++++++++++++++++ 4 files changed, 187 insertions(+) create mode 100644 internal/lock/lock.go create mode 100644 internal/lock/lock_test.go diff --git a/cmd/vintner/download.go b/cmd/vintner/download.go index d25649b..ee8e001 100644 --- a/cmd/vintner/download.go +++ b/cmd/vintner/download.go @@ -10,6 +10,7 @@ import ( "github.com/Cheviiot/vintner/internal/download" "github.com/Cheviiot/vintner/internal/i18n" + "github.com/Cheviiot/vintner/internal/lock" ) func runDownload(args []string) int { @@ -177,6 +178,12 @@ func runDownload(args []string) int { fmt.Fprintln(os.Stderr, "vintner download:", err) return 1 } + unlock, err := lock.Acquire(destAbs) + if err != nil { + fmt.Fprintln(os.Stderr, "vintner download:", err) + return 1 + } + defer unlock() unpack := destAbs if !*onlyUnpack { diff --git a/internal/install/install.go b/internal/install/install.go index 15d3b63..f9efe06 100644 --- a/internal/install/install.go +++ b/internal/install/install.go @@ -15,6 +15,7 @@ import ( "strings" "github.com/Cheviiot/vintner/assets" + "github.com/Cheviiot/vintner/internal/lock" "github.com/Cheviiot/vintner/internal/wineenv" ) @@ -34,6 +35,12 @@ func Install(dest, selfBinary string) error { return fmt.Errorf("destination %q is not a directory", dest) } + unlock, err := lock.Acquire(dest) + if err != nil { + return err + } + defer unlock() + // Targets are relative so the whole installed tree stays relocatable - // moving or renaming dest doesn't break these symlinks the way an // absolute target baked in at install time would. diff --git a/internal/lock/lock.go b/internal/lock/lock.go new file mode 100644 index 0000000..1a6ecab --- /dev/null +++ b/internal/lock/lock.go @@ -0,0 +1,49 @@ +// Package lock guards a destination directory against two `vintner +// download`/`install` runs mutating it at the same time. +package lock + +import ( + "fmt" + "os" + "path/filepath" + "syscall" +) + +// FileName is the advisory lock file `download` and `install` both take +// out against their (usually shared) destination directory before +// touching anything in it - concurrent download+install, or two +// downloads, against the same dest could otherwise interleave badly: +// combineDirTrees' merge logic assumes it's the only thing moving files +// into a given target at a time, and two `os.Rename` calls racing for the +// same destination path is exactly the kind of thing that corrupts a tree +// instead of erroring cleanly. +const FileName = ".vintner.lock" + +// Acquire takes an exclusive, non-blocking lock on dest (creating dest if +// it doesn't exist yet) and returns a func to release it, which the caller +// must defer. If another vintner process already holds the lock, returns +// an error immediately instead of blocking - there's no reason a second +// invocation should silently queue up and wait for the first to finish +// touching the same directory; the caller should simply not have started +// it yet. Uses flock(2), so a crashed holder's lock is released +// automatically by the kernel when its file descriptor closes - never +// needs manual cleanup, unlike a plain "does a file exist" lock +// convention would. +func Acquire(dest string) (unlock func(), err error) { + if err := os.MkdirAll(dest, 0o755); err != nil { + return nil, err + } + path := filepath.Join(dest, FileName) + f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o644) + if err != nil { + return nil, err + } + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + f.Close() + return nil, fmt.Errorf("another vintner download/install is already running against %s", dest) + } + return func() { + syscall.Flock(int(f.Fd()), syscall.LOCK_UN) + f.Close() + }, nil +} diff --git a/internal/lock/lock_test.go b/internal/lock/lock_test.go new file mode 100644 index 0000000..e88b9c9 --- /dev/null +++ b/internal/lock/lock_test.go @@ -0,0 +1,124 @@ +package lock + +import ( + "io" + "os" + "os/exec" + "path/filepath" + "testing" + "time" +) + +func TestAcquireAndRelease(t *testing.T) { + dest := t.TempDir() + + unlock, err := Acquire(dest) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(dest, FileName)); err != nil { + t.Errorf("expected the lock file to exist while held: %v", err) + } + unlock() + + // Released - a second Acquire against the same dest must now succeed. + unlock2, err := Acquire(dest) + if err != nil { + t.Fatalf("Acquire after release failed: %v", err) + } + unlock2() +} + +func TestAcquireCreatesDestIfMissing(t *testing.T) { + dest := filepath.Join(t.TempDir(), "does", "not", "exist", "yet") + unlock, err := Acquire(dest) + if err != nil { + t.Fatal(err) + } + defer unlock() + if fi, err := os.Stat(dest); err != nil || !fi.IsDir() { + t.Errorf("expected Acquire to create %s, stat err: %v", dest, err) + } +} + +func TestAcquireFailsWhileAlreadyHeld(t *testing.T) { + dest := t.TempDir() + + unlock, err := Acquire(dest) + if err != nil { + t.Fatal(err) + } + defer unlock() + + if _, err := Acquire(dest); err == nil { + t.Fatal("expected a second Acquire against the same dest, while the first is still held, to fail") + } +} + +func TestAcquireSucceedsAfterHolderReleases(t *testing.T) { + dest := t.TempDir() + + unlock1, err := Acquire(dest) + if err != nil { + t.Fatal(err) + } + unlock1() + + unlock2, err := Acquire(dest) + if err != nil { + t.Fatalf("Acquire should succeed once the first holder released: %v", err) + } + unlock2() +} + +// TestAcquireFailsAcrossRealProcesses is the real end-to-end check: flock +// is per-open-file-description, not per-process or per-thread, so a lock +// held by *this* test process via one fd could in principle still be +// re-acquirable by another fd in the same process depending on the +// platform's exact semantics. Spawning this test binary as a genuinely +// separate child process (via the standard TestMain re-exec trick) and +// having it hold the lock while the parent tries to acquire it is what +// actually proves two independent `vintner download`/`install` processes +// contend correctly, not just two Go-level calls in one process. +func TestAcquireFailsAcrossRealProcesses(t *testing.T) { + if os.Getenv("VINTNER_LOCK_TEST_HOLD") != "" { + unlock, err := Acquire(os.Getenv("VINTNER_LOCK_TEST_HOLD")) + if err != nil { + os.Exit(2) + } + defer unlock() + // Signal readiness, then wait to be killed by the parent. A plain + // `select {}` here would have zero other goroutines able to ever + // wake it, which Go's runtime provably detects as a deadlock and + // crashes on ("fatal error: all goroutines are asleep") - a real + // timer avoids that. + os.Stdout.WriteString("locked\n") + time.Sleep(time.Minute) + } + + dest := t.TempDir() + exe, err := os.Executable() + if err != nil { + t.Fatal(err) + } + + cmd := exec.Command(exe, "-test.run=TestAcquireFailsAcrossRealProcesses") + cmd.Env = append(os.Environ(), "VINTNER_LOCK_TEST_HOLD="+dest) + stdout, err := cmd.StdoutPipe() + if err != nil { + t.Fatal(err) + } + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + defer cmd.Process.Kill() + + buf := make([]byte, len("locked\n")) + if _, err := io.ReadFull(stdout, buf); err != nil || string(buf) != "locked\n" { + t.Fatalf("child process didn't report holding the lock: %v (%q)", err, buf) + } + + if _, err := Acquire(dest); err == nil { + t.Fatal("expected Acquire to fail while a separate process holds the lock") + } +}