diff --git a/internal/cli/run_access.go b/internal/cli/run_access.go index 9612879..3070298 100644 --- a/internal/cli/run_access.go +++ b/internal/cli/run_access.go @@ -3,6 +3,7 @@ package cli import ( "bytes" "context" + "errors" "fmt" "io" "os" @@ -155,7 +156,14 @@ func runProvision(a *Args, stdout, stderr io.Writer) int { // design decision made here. logPath := filepath.Join(v.Dir, "last-provision.log") done := make(chan error, 1) - go func() { done <- sshx.Provision(context.Background(), v) }() + // core.WithProvisionLock, not core.Apply: this path drives sshx.Provision + // directly (its cloud-VM handling and JSON shape differ from Apply's), so + // it takes the same per-VM lock itself instead of going through Apply. + go func() { + done <- core.WithProvisionLock(v.Dir, func() error { + return sshx.Provision(context.Background(), v) + }) + }() // Under --json the recipe's raw bytes must not reach stdout: they would sit // in the middle of the JSON Lines stream and every consumer's json.loads @@ -170,6 +178,15 @@ func runProvision(a *Args, stdout, stderr io.Writer) int { if lw != nil { lw.Flush() } + if errors.Is(perr, core.ErrProvisionInProgress) { + // Another run already holds the VM's provision lock; that run owns + // the error, so this one exits clean instead of reporting it too. + if a.JSON { + return a.ok(stdout, map[string]any{"vm": a.VM, "provisioned": false, "skipped_reason": "provision already running"}) + } + fmt.Fprintf(stdout, "%s: provision already running\n", a.VM) + return ExitOK + } if perr != nil { return a.fail(stdout, stderr, perr) } diff --git a/internal/cli/run_apply.go b/internal/cli/run_apply.go index e726d90..3a80269 100644 --- a/internal/cli/run_apply.go +++ b/internal/cli/run_apply.go @@ -2,6 +2,7 @@ package cli import ( "context" + "errors" "fmt" "io" @@ -48,6 +49,16 @@ func runApply(a *Args, stdout, stderr io.Writer) int { if lw != nil { lw.Flush() } + if errors.Is(aerr, core.ErrProvisionInProgress) { + // Another run already holds the VM's provision lock. That run's + // caller owns the error; this one exits clean rather than reporting + // somebody else's concurrent apply as its own failure. + if a.JSON { + return a.ok(stdout, map[string]any{"vm": a.VM, "applied": false, "skipped_reason": "provision already running"}) + } + fmt.Fprintf(stdout, "%s: provision already running\n", a.VM) + return ExitOK + } if aerr != nil { // core.ErrAppliedAtBoot is a real outcome for a cloud VM, mapped to // applied_at_boot by wire's error table; it is not special-cased into diff --git a/internal/cli/run_vm.go b/internal/cli/run_vm.go index 8a3acac..931b3da 100644 --- a/internal/cli/run_vm.go +++ b/internal/cli/run_vm.go @@ -130,6 +130,13 @@ func afterStart(a *Args, v core.VM, stdout, stderr io.Writer) int { done := make(chan error, 1) go func() { done <- core.Apply(context.Background(), a.VM, core.ApplyOpts{}) }() if err := streamFile(v.Paths.ApplyLog, stdout, done); err != nil { + if errors.Is(err, core.ErrProvisionInProgress) { + // A concurrent `apply` or `provision` already holds the lock; that + // run owns the error. `up` still started the VM, so this is not a + // failure of the up command. + fmt.Fprintf(stdout, "%s: provision already running\n", a.VM) + return ExitOK + } return a.fail(stdout, stderr, err) } fmt.Fprintf(stdout, "%s provisioned\n", a.VM) diff --git a/internal/core/apply.go b/internal/core/apply.go index 823bff6..c82f3b6 100644 --- a/internal/core/apply.go +++ b/internal/core/apply.go @@ -62,16 +62,32 @@ type ApplyOpts struct { // cancellation in the gap between two recipes does not start one more ssh // process just to kill it. Apply passes ctx straight through and adds no // second cancellation layer of its own. +// +// Apply holds an exclusive flock on v.Dir/provision.lock for the whole run. +// Two concurrent runs against the same VM would each start apk (or the +// guest's own package manager) and race on its database lock; the second +// Apply sees the lock held and returns ErrProvisionInProgress instead. func Apply(ctx context.Context, name string, opts ApplyOpts) error { v, err := load(name) if err != nil { return err } + err = WithProvisionLock(v.Dir, func() error { + return applyLocked(ctx, v, opts) + }) + if errors.Is(err, ErrProvisionInProgress) { + return fmt.Errorf("%w: %s", ErrProvisionInProgress, name) + } + return err +} + +// applyLocked is Apply's body, run while Apply holds name's provision lock. +func applyLocked(ctx context.Context, v *config.VM, opts ApplyOpts) error { if !qemu.Running(v) { - return fmt.Errorf("%w: %s", ErrNotRunning, name) + return fmt.Errorf("%w: %s", ErrNotRunning, v.Name) } if backend.For(v).Name() == "cloudinit" { - return fmt.Errorf("%w: %s", ErrAppliedAtBoot, name) + return fmt.Errorf("%w: %s", ErrAppliedAtBoot, v.Name) } targets := v.Recipes @@ -82,7 +98,7 @@ func Apply(ctx context.Context, name string, opts ApplyOpts) error { } for _, o := range opts.Only { if !have[o] { - return fmt.Errorf("%w: recipe %q is not one of %s's recipes", ErrRecipeNotApplicable, o, name) + return fmt.Errorf("%w: recipe %q is not one of %s's recipes", ErrRecipeNotApplicable, o, v.Name) } } targets = opts.Only diff --git a/internal/core/apply_test.go b/internal/core/apply_test.go index f5f9082..0362653 100644 --- a/internal/core/apply_test.go +++ b/internal/core/apply_test.go @@ -31,6 +31,37 @@ func TestApplyStoppedVMIsRefused(t *testing.T) { } } +// A second Apply against a VM another run already holds the provision lock +// for must refuse before it reaches ssh, so it never races the first run's +// apk (or other package manager) invocation on the guest. +func TestApplyRefusesWhileLockIsHeld(t *testing.T) { + dir := root(t) + v := &config.VM{Name: "work", Mode: "live", OS: "alpine", Backend: "apkovl", RAM: 512, CPUs: 1, SSHPort: 2200} + if err := v.Save(); err != nil { + t.Fatal(err) + } + v.Dir = filepath.Join(dir, "work") + stop := fakeRunning(t, v) + defer stop() + + release := make(chan struct{}) + held := make(chan struct{}) + go func() { + WithProvisionLock(v.Dir, func() error { + close(held) + <-release + return nil + }) + }() + <-held + defer close(release) + + err := Apply(context.Background(), "work", ApplyOpts{}) + if !errors.Is(err, ErrProvisionInProgress) { + t.Fatalf("err = %v, want ErrProvisionInProgress", err) + } +} + // A cloudinit VM's recipes ran from the cloud-init seed at first boot; // there is no post-boot ssh step for Apply to drive, and running one would // mean piping a YAML fragment to `sh -s`. Apply must refuse this outright diff --git a/internal/core/lock.go b/internal/core/lock.go new file mode 100644 index 0000000..4d20dcb --- /dev/null +++ b/internal/core/lock.go @@ -0,0 +1,47 @@ +package core + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "syscall" +) + +// ErrProvisionInProgress is returned when a second Apply finds +// /provision.lock already held. Two concurrent runs against the +// same guest each start apk (or the guest's own package manager); the +// second one hits apk's database lock and fails with exit 99. Refusing the +// second Apply up front avoids that race instead of surfacing it as a +// guest-side failure. +var ErrProvisionInProgress = errors.New("provision already in progress") + +// WithProvisionLock runs fn while holding an exclusive, non-blocking flock +// on dir/provision.lock. It returns ErrProvisionInProgress without calling +// fn when another process already holds the lock. +// +// flock ties the lock to the holding process. A crashed run releases it +// when the kernel closes its file descriptors, so a lock file left on disk +// after a crash never blocks the next run. The file itself is never removed. +// +// Apply is one caller. internal/cli's runProvision is the other: it drives +// sshx.Provision directly rather than through Apply, so it takes this lock +// itself around that call. +func WithProvisionLock(dir string, fn func() error) error { + path := filepath.Join(dir, "provision.lock") + f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o644) + if err != nil { + return fmt.Errorf("open provision lock: %w", err) + } + defer f.Close() + + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + if errors.Is(err, syscall.EWOULDBLOCK) { + return ErrProvisionInProgress + } + return fmt.Errorf("lock %s: %w", path, err) + } + defer syscall.Flock(int(f.Fd()), syscall.LOCK_UN) + + return fn() +} diff --git a/internal/core/lock_test.go b/internal/core/lock_test.go new file mode 100644 index 0000000..a4982e6 --- /dev/null +++ b/internal/core/lock_test.go @@ -0,0 +1,61 @@ +package core + +import ( + "errors" + "sync" + "testing" +) + +// TestWithProvisionLockSerializes holds the lock from inside fn, then +// checks a concurrent withProvisionLock on the same dir gets +// ErrProvisionInProgress instead of running its own fn. +func TestWithProvisionLockSerializes(t *testing.T) { + dir := t.TempDir() + + holding := make(chan struct{}) + release := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + _ = WithProvisionLock(dir, func() error { + close(holding) + <-release + return nil + }) + }() + + <-holding + var secondRan bool + err := WithProvisionLock(dir, func() error { + secondRan = true + return nil + }) + close(release) + wg.Wait() + + if !errors.Is(err, ErrProvisionInProgress) { + t.Fatalf("second withProvisionLock: got %v, want ErrProvisionInProgress", err) + } + if secondRan { + t.Fatal("second withProvisionLock ran fn while the first held the lock") + } +} + +// TestWithProvisionLockReacquiresAfterRelease checks the lock is free again +// once the first holder's fn returns. +func TestWithProvisionLockReacquiresAfterRelease(t *testing.T) { + dir := t.TempDir() + + if err := WithProvisionLock(dir, func() error { return nil }); err != nil { + t.Fatalf("first withProvisionLock: %v", err) + } + + var secondRan bool + if err := WithProvisionLock(dir, func() error { secondRan = true; return nil }); err != nil { + t.Fatalf("second withProvisionLock: %v", err) + } + if !secondRan { + t.Fatal("second withProvisionLock did not run fn after the first released the lock") + } +} diff --git a/internal/tui/app.go b/internal/tui/app.go index 018db47..8dd5188 100644 --- a/internal/tui/app.go +++ b/internal/tui/app.go @@ -1,6 +1,7 @@ package tui import ( + "errors" "path/filepath" "strings" @@ -352,6 +353,14 @@ func (m model) updateApp(msg tea.Msg) (tea.Model, tea.Cmd) { return m, m.startProvision(*v) case provisionDoneMsg: delete(m.provisioning, msg.name) + if errors.Is(msg.err, core.ErrProvisionInProgress) { + // m.provisioning already stops a second provision started from this + // process. This is the cross-process case: another stoat process (a + // second TUI, or the CLI) holds the VM's lock. Not an error toast, + // since the user did nothing wrong. + cmd := m.showToast("provision already running for "+msg.name, false) + return m, cmd + } if msg.err != nil { cmd := m.showToast(msg.name+": "+msg.err.Error(), true) return m, cmd