From fcdd10a7bb040587e90b4e241233ef899e7a0b1f Mon Sep 17 00:00:00 2001 From: NovusEdge Date: Mon, 10 Aug 2026 16:25:51 +0300 Subject: [PATCH 1/2] fix(core): serialize provision runs with a per-vm lock Two concurrent Apply runs against one VM each start apk, and the second hits apk's own database lock and fails with exit 99. Apply now holds an exclusive flock on /provision.lock for the whole run and returns ErrProvisionInProgress when another run already holds it. --- internal/core/apply.go | 22 +++++++++++-- internal/core/apply_test.go | 31 +++++++++++++++++++ internal/core/lock.go | 47 ++++++++++++++++++++++++++++ internal/core/lock_test.go | 61 +++++++++++++++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 3 deletions(-) create mode 100644 internal/core/lock.go create mode 100644 internal/core/lock_test.go 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") + } +} From 1dfe9cf4ae98c5405fc9c223dec1682d62e4208f Mon Sep 17 00:00:00 2001 From: NovusEdge Date: Mon, 10 Aug 2026 16:25:58 +0300 Subject: [PATCH 2/2] fix(cli,tui): treat a concurrent provision as a skip, not a failure core.Apply's ErrProvisionInProgress means another run already holds the VM's lock, not that this run did anything wrong. The CLI's apply, provision, and up commands print a short notice and exit 0; the TUI shows a toast instead of the red error path. runProvision drives sshx.Provision directly rather than through Apply, so it takes core.WithProvisionLock itself around that call. --- internal/cli/run_access.go | 19 ++++++++++++++++++- internal/cli/run_apply.go | 11 +++++++++++ internal/cli/run_vm.go | 7 +++++++ internal/tui/app.go | 9 +++++++++ 4 files changed, 45 insertions(+), 1 deletion(-) 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/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