diff --git a/README.md b/README.md index 64e8f1e..ab1c9f0 100644 --- a/README.md +++ b/README.md @@ -171,18 +171,20 @@ vaultik version ### locking -Every command that opens the local index — `snapshot create`, `snapshot -list`, `snapshot verify`, `snapshot purge`, `snapshot remove`, `snapshot -restore`, `prune`, `info`, and `remote info`/`remote nuke` — takes a +Commands that write persistent state — `snapshot create`, `snapshot +purge`, `snapshot remove`, `prune`, and `remote nuke` — take a process-wide lock at `$XDG_DATA_HOME/vaultik/vaultik.pid` (`~/.local/share/vaultik/vaultik.pid` on Linux) for the whole run. Only -one such command runs at a time: a second one exits immediately with an -"already running" error rather than waiting. The lock is not scoped to -mutating commands, so read-only commands are affected too — `vaultik -snapshot list` fails while a backup is in progress; scoping it so -read-only commands run during a backup is tracked in -[issue #150](https://git.eeqj.de/sneak/vaultik/issues/150). `config`, -`database delete`, `completion`, and `version` do not take the lock. +one of them runs at a time: a second one exits immediately with an +"already running" error rather than waiting, so two writers can never +corrupt the local index or the destination store. + +Read-only commands — `info`, `snapshot list`, `snapshot verify`, and +`remote info` — do not take the lock and are never blocked, so they run +even while a backup is in progress. `snapshot restore` does not take the +lock either: it writes only to the target directory you name, not the +local index or the destination store. `config`, `database delete`, +`completion`, and `version` do not take the lock. ### stdout and stderr diff --git a/internal/cli/app.go b/internal/cli/app.go index 5000ea2..4a366cf 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -33,14 +33,33 @@ import ( // may take before we give up. const shutdownTimeout = 30 * time.Second -// AppOptions contains common options for creating the fx application. -// It includes the configuration file path, logging options, and additional -// fx modules and invocations that should be included in the application. +// lockMode says whether a command mutates persistent state — the local +// index database or the remote store — and so must hold the process-wide +// PID lock, or only reads that state and may run alongside a mutator. +type lockMode int + +const ( + // mutating commands (snapshot create, snapshot purge, snapshot remove, + // prune, remote nuke) write the local index or the remote store. They + // hold the PID lock so that at most one runs at a time. + mutating lockMode = iota + // readOnly commands (info, snapshot list, snapshot verify, remote info, + // snapshot restore) do not write the local index or the remote store, + // so they run without the lock and are never blocked by a running + // mutator. restore writes only to the target directory it is given. + readOnly +) + +// AppOptions contains common options for creating and running the fx +// application: the configuration file path, logging options, additional fx +// modules and invocations, and whether the command mutates persistent +// state (which decides whether it takes the PID lock). type AppOptions struct { ConfigPath string LogOptions log.Options Modules []fx.Option Invokes []fx.Option + Mode lockMode } // setupGlobals records the startup time and, when an output-suppression @@ -281,14 +300,15 @@ func RunOperation( } // runVaultikApp runs the standard single-operation command lifecycle -// shared by the list/purge/verify/remove/remote-info subcommands: +// shared by the snapshot list/purge/remove and remote nuke subcommands: // resolve the config, then run op against the Vaultik instance through // RunOperation, reporting a failure prefixed with failMsg (suppressed -// while suppressErrors is true, e.g. under --json). jsonOutput marks a -// command whose stdout is a JSON document: it quiets the UI but, unlike -// Quiet, leaves the stderr log level alone. +// while suppressErrors is true, e.g. under --json). mode says whether the +// command takes the PID lock. jsonOutput marks a command whose stdout is a +// JSON document: it quiets the UI but, unlike Quiet, leaves the stderr log +// level alone. func runVaultikApp( - cmd *cobra.Command, jsonOutput, suppressErrors bool, + cmd *cobra.Command, mode lockMode, jsonOutput, suppressErrors bool, failMsg string, op func(v *vaultik.Vaultik) error, ) error { configPath, err := ResolveConfigPath() @@ -306,6 +326,7 @@ func runVaultikApp( Quiet: rootFlags.Quiet, JSON: jsonOutput, }, + Mode: mode, }, op, func(err error) { if suppressErrors { return @@ -319,28 +340,46 @@ func runVaultikApp( // RunWithApp is a helper that creates and runs an fx app with the given options. // It combines NewApp and RunApp into a single convenient function. This is the // preferred way to run CLI commands that need the full application context. -// It acquires a PID lock before starting to prevent concurrent instances. +// A mutating command takes the process-wide PID lock before starting so that +// only one runs at a time; a read-only command runs without it and is not +// blocked while a mutator holds the lock (opts.Mode). func RunWithApp(ctx context.Context, opts AppOptions) error { - // Acquire PID lock to prevent concurrent instances - lockDir := filepath.Join(xdg.DataHome, "vaultik") - - lock, err := pidlock.Acquire(lockDir) + release, err := acquireLockIfMutating(opts.Mode, + filepath.Join(xdg.DataHome, "vaultik")) if err != nil { - if errors.Is(err, pidlock.ErrAlreadyRunning) { - return fmt.Errorf("cannot start: %w", err) - } - - return fmt.Errorf("failed to acquire lock: %w", err) + return err } - defer func() { - err := lock.Release() - if err != nil { - log.Warn("Failed to release PID lock", "error", err) - } - }() + defer release() app := NewApp(opts) return RunApp(ctx, app) } + +// acquireLockIfMutating takes the process-wide PID lock in lockDir for a +// mutating command and returns a function that releases it. A read-only +// command takes no lock, so it returns a no-op release and is never blocked +// while a mutator holds the lock. ErrAlreadyRunning (another mutator holds +// the lock) is surfaced as a "cannot start" error. +func acquireLockIfMutating(mode lockMode, lockDir string) (func(), error) { + if mode != mutating { + return func() {}, nil + } + + lock, err := pidlock.Acquire(lockDir) + if err != nil { + if errors.Is(err, pidlock.ErrAlreadyRunning) { + return nil, fmt.Errorf("cannot start: %w", err) + } + + return nil, fmt.Errorf("failed to acquire lock: %w", err) + } + + return func() { + err := lock.Release() + if err != nil { + log.Warn("Failed to release PID lock", "error", err) + } + }, nil +} diff --git a/internal/cli/app_test.go b/internal/cli/app_test.go index d865cc6..102bfae 100644 --- a/internal/cli/app_test.go +++ b/internal/cli/app_test.go @@ -2,7 +2,10 @@ package cli //nolint:testpackage // needs access to unexported cleanStartupError import ( "errors" + "path/filepath" "testing" + + "sneak.berlin/go/vaultik/internal/pidlock" ) func TestCleanStartupError(t *testing.T) { @@ -53,3 +56,42 @@ func TestCleanStartupError(t *testing.T) { }) } } + +// TestLockScopedToMutatingCommands proves the partition the PID lock now +// enforces: a read-only command runs while a mutator holds the lock, and +// two mutating commands still mutually exclude. +func TestLockScopedToMutatingCommands(t *testing.T) { + t.Parallel() + + lockDir := filepath.Join(t.TempDir(), "vaultik") + + // A mutating command takes the process-wide lock. + releaseMutator, err := acquireLockIfMutating(mutating, lockDir) + if err != nil { + t.Fatalf("mutating command could not acquire lock: %v", err) + } + + // A read-only command runs to completion even while the lock is held. + releaseReader, err := acquireLockIfMutating(readOnly, lockDir) + if err != nil { + t.Fatalf("read-only command was blocked by held lock: %v", err) + } + + releaseReader() + + // A second mutating command is refused while the first holds the lock. + _, err = acquireLockIfMutating(mutating, lockDir) + if !errors.Is(err, pidlock.ErrAlreadyRunning) { + t.Fatalf("second mutating command was not excluded, got: %v", err) + } + + // Once the first mutator releases, another mutating command may run. + releaseMutator() + + release, err := acquireLockIfMutating(mutating, lockDir) + if err != nil { + t.Fatalf("mutating command could not acquire released lock: %v", err) + } + + release() +} diff --git a/internal/cli/info.go b/internal/cli/info.go index fe5a652..27d40de 100644 --- a/internal/cli/info.go +++ b/internal/cli/info.go @@ -35,6 +35,7 @@ func NewInfoCommand() *cobra.Command { Debug: rootFlags.Debug, Quiet: rootFlags.Quiet, }, + Mode: readOnly, }, func(v *vaultik.Vaultik) error { return v.ShowInfo() }, func(err error) { diff --git a/internal/cli/prune.go b/internal/cli/prune.go index 81feed8..b3b6040 100644 --- a/internal/cli/prune.go +++ b/internal/cli/prune.go @@ -44,6 +44,7 @@ work (e.g. after a crashed backup or to reclaim storage).`, Quiet: rootFlags.Quiet, JSON: opts.JSON, }, + Mode: mutating, }, func(v *vaultik.Vaultik) error { return v.Prune(opts) }, func(err error) { diff --git a/internal/cli/remote.go b/internal/cli/remote.go index 9cc7d74..e561298 100644 --- a/internal/cli/remote.go +++ b/internal/cli/remote.go @@ -45,7 +45,7 @@ This is destructive and irreversible. Requires --force.`, return errNukeNeedsForce } - return runVaultikApp(cmd, false, false, "Remote nuke failed", + return runVaultikApp(cmd, mutating, false, false, "Remote nuke failed", func(v *vaultik.Vaultik) error { return v.NukeRemote(true) }) @@ -88,6 +88,7 @@ func newRemoteInfoCommand() *cobra.Command { Quiet: rootFlags.Quiet, JSON: jsonOutput, }, + Mode: readOnly, }, func(v *vaultik.Vaultik) error { return v.RemoteInfo(jsonOutput) }, func(err error) { diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 8007ad4..fe32d60 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -92,6 +92,7 @@ specifying a path using --config or by setting VAULTIK_CONFIG to a path.`, Cron: opts.Cron, Quiet: rootFlags.Quiet, }, + Mode: mutating, }, func(v *vaultik.Vaultik) error { return v.CreateSnapshot(opts) }, func(err error) { @@ -125,7 +126,7 @@ func newSnapshotListCommand() *cobra.Command { Long: "Lists all snapshots with their ID, timestamp, and compressed size", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { - return runVaultikApp(cmd, false, false, + return runVaultikApp(cmd, readOnly, false, false, "Failed to list snapshots", func(v *vaultik.Vaultik) error { return v.ListSnapshots(jsonOutput) @@ -161,7 +162,7 @@ restrict the operation to specific snapshot names.`, return errPurgeCriteriaBoth } - return runVaultikApp(cmd, false, false, + return runVaultikApp(cmd, mutating, false, false, "Failed to purge snapshots", func(v *vaultik.Vaultik) error { return v.PurgeSnapshotsWithOptions(opts) @@ -212,6 +213,7 @@ func newSnapshotVerifyCommand() *cobra.Command { Quiet: rootFlags.Quiet, JSON: opts.JSON, }, + Mode: readOnly, }, func(v *vaultik.Vaultik) error { return v.VerifySnapshotWithOptions(snapshotID, opts) }, func(err error) { @@ -259,7 +261,7 @@ To wipe the entire destination store and start over, use 'vaultik remote nuke --force' — it is the single supported entry point for that.`, Args: requireSnapshotIDArg, RunE: func(cmd *cobra.Command, args []string) error { - return runVaultikApp(cmd, opts.JSON, opts.JSON, + return runVaultikApp(cmd, mutating, opts.JSON, opts.JSON, "Failed to remove snapshot", func(v *vaultik.Vaultik) error { _, err := v.RemoveSnapshot(args[0], opts) diff --git a/internal/cli/snapshot_restore.go b/internal/cli/snapshot_restore.go index d8e60d2..45707be 100644 --- a/internal/cli/snapshot_restore.go +++ b/internal/cli/snapshot_restore.go @@ -88,6 +88,7 @@ func runRestore(cmd *cobra.Command, args []string, opts *RestoreOptions) error { Debug: rootFlags.Debug, Quiet: rootFlags.Quiet, }, + Mode: readOnly, }, func(v *vaultik.Vaultik) error { return v.Restore(&vaultik.RestoreOptions{ SnapshotID: snapshotID,