From 363313d99c32af7cce1f735330c082d1b5a6e086 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 10 Aug 2026 18:14:09 +0000 Subject: [PATCH] Confine cleanup and use held session locks --- docs/internal/fileops.md | 13 + docs/internal/workspace.md | 25 +- docs/operations.md | 6 +- docs/roadmap/implementation.md | 2 +- docs/troubleshooting.md | 5 +- internal/app/clean.go | 7 +- internal/app/post_publish_cleanup.go | 4 +- internal/app/restore.go | 12 +- internal/app/runner.go | 13 +- internal/artifacts/local.go | 87 +++++-- internal/artifacts/local_test.go | 147 ++++++++++- internal/artifacts/lock_common.go | 18 ++ internal/artifacts/lock_unix.go | 22 ++ internal/artifacts/lock_unsupported.go | 15 ++ internal/artifacts/lock_windows.go | 29 +++ internal/artifacts/store.go | 2 + internal/fileops/cleanup.go | 165 ++++++++++++ internal/fileops/cleanup_test.go | 79 ++++++ internal/fileops/confined.go | 33 +++ internal/fileops/directory.go | 255 ++++++++++++++++++- internal/fileops/rename_noreplace_darwin.go | 10 +- internal/fileops/rename_noreplace_linux.go | 10 +- internal/fileops/rename_noreplace_other.go | 5 + internal/fileops/rename_noreplace_windows.go | 25 +- internal/stage/prepare.go | 2 +- 25 files changed, 919 insertions(+), 72 deletions(-) create mode 100644 internal/artifacts/lock_common.go create mode 100644 internal/artifacts/lock_unix.go create mode 100644 internal/artifacts/lock_unsupported.go create mode 100644 internal/artifacts/lock_windows.go create mode 100644 internal/fileops/cleanup.go create mode 100644 internal/fileops/cleanup_test.go diff --git a/docs/internal/fileops.md b/docs/internal/fileops.md index ebd1c2e..07f2c8c 100644 --- a/docs/internal/fileops.md +++ b/docs/internal/fileops.md @@ -18,6 +18,19 @@ writer receives a `fileops`-owned, already-open sibling temporary file rather than a mutable destination path. Callers still own remote object selection, validation, conflict handling, and final mode. +Directory promotion keeps the verified destination parent open while it creates +the temporary tree, copies regular source entries, and performs the platform +no-replace rename. Platforms without a verified handle-relative atomic +no-replace primitive reject promotion before writing a temporary tree. + +## Cleanup Contract + +`RemoveAllUnderRoot` accepts an explicit root and a proper descendant. It opens +the root and each target ancestor without following symlinks, then removes the +tree through those directory handles. It rejects root deletion and any symlink +encountered in the target path or tree; repeated removal of a missing target is +successful. Command and post-publish policy remains owned by `internal/app`. + ## Replacement Contract `ReplaceFileAtomic` requires an existing destination directory. It creates a diff --git a/docs/internal/workspace.md b/docs/internal/workspace.md index 674744e..36192ae 100644 --- a/docs/internal/workspace.md +++ b/docs/internal/workspace.md @@ -13,8 +13,9 @@ previous-cache path construction. `SessionPathsFor` provides the session-scoped path model, and layout creation goes through `EnsureLayoutFor`. Callers should consume those helpers instead of rebuilding relative paths. -`internal/pathsafe` and application cleanup helpers enforce confinement for -relative destinations and deletion targets. +`internal/pathsafe` validates relative destinations. `internal/fileops` opens +cleanup roots and their descendants through no-follow directory handles before +removing them. `internal/fileops` owns the ordinary workspace mode contract. On POSIX, `WorkspaceDirectoryMode` is setgid `02775` and `WorkspaceFileMode` is `0664`. @@ -43,8 +44,11 @@ existing destination. Exact physical paths belong in ## Locking -`artifacts.LocalStore` enforces the single-writer session lock via `.lock` -(`ErrLockConflict` on contention). +`artifacts.LocalStore` enforces the single-writer session lock via an +operating-system lock held on `.lock` (`ErrLockConflict` on contention). The +file retains owner metadata after release or process death; its existence is +not evidence that a lock is active. Command and restore flows wait for this +lock only while their context remains active, and report a release failure. ## Cleanup Semantics @@ -54,10 +58,11 @@ Automatic post-publish cleanup: - requires `uploaded=true` and `current_pointer_written=true` metadata; - consumes the resolved cleanup policy described in [Configuration](../config.md); -- refuses unsafe deletes (root delete, out-of-root delete, symlink paths). +- refuses unsafe deletes (root delete, out-of-root delete, and symlinked + ancestors or entries). -Manual cleanup uses the same scoped-target checks. Invocation syntax and exact -deletion scope belong in [CLI](../cli.md#clean) and +Manual cleanup uses the same root-confined deletion mechanism. Invocation +syntax and exact deletion scope belong in [CLI](../cli.md#clean) and [Operations](../operations.md#cleanup). ## Invariants @@ -75,10 +80,10 @@ deletion scope belong in [CLI](../cli.md#clean) and - Run-local materialization: `internal/stage/run_local.go` - Immutable bundle promotion: `internal/fileops/directory.go` - Workspace modes: `internal/fileops/modes.go` -- Cleanup confinement: `internal/app/cleanup_targets.go`, - `internal/app/post_publish_cleanup.go` +- Cleanup confinement: `internal/fileops/cleanup.go`, + `internal/app/cleanup_targets.go`, `internal/app/post_publish_cleanup.go` - Tests: `internal/artifacts/paths_model_test.go`, `internal/artifacts/local_test.go`, `internal/stage/run_local_test.go`, `internal/fileops/directory_test.go`, `internal/fileops/modes_posix_test.go`, - `internal/app/cleanup_targets_test.go`, + `internal/fileops/cleanup_test.go`, `internal/app/cleanup_targets_test.go`, `internal/app/post_publish_cleanup_test.go` diff --git a/docs/operations.md b/docs/operations.md index 3bd2a40..6a7daf2 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -133,8 +133,8 @@ record does not fall back to the earlier outputs. The earlier run manifest and immutable bundle remain available for inspection, but downstream resolution requires a new current successful extraction record. -Atomic Notarius bundle promotion is supported on Linux, macOS, and Windows. -On other operating systems, extraction fails before copying the bundle into a +Atomic Notarius bundle promotion is supported on Linux and macOS. On Windows +and other operating systems, extraction fails before copying the bundle into a temporary promotion tree because Narratio has no verified atomic no-replace directory primitive there. This is an extraction limitation, not a broader platform-support guarantee for every Narratio workflow. @@ -329,6 +329,8 @@ Rules: - `clean` deletes work/spool session state; - cache is preserved unless `--clear-cache` is set; +- each deletion is confined beneath its configured workspace, spool, or cache + root and refuses symlinked paths; - automatic post-publish cleanup is gated by successful publish commit plus: - `pipeline.spool.delete_audio_after_publish=true` - `pipeline.workspace.cleanup_after_publish=true` diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index bf38a32..94b7e08 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -20,7 +20,7 @@ All stages are pending when this plan is created. | 2 | Enforce safe identifiers and fuzz path/source contracts | COR-002, TST-013 | Completed | | 3 | Consolidate crash-durable atomic file replacement | RSK-002, DUP-001, DUP-005 | Completed | | 4 | Add confined destination and download/install capabilities | COR-003, DUP-003, TST-003 | Completed | -| 5 | Confine recursive cleanup and replace sentinel locks | RSK-003 | Pending | +| 5 | Confine recursive cleanup and replace sentinel locks | RSK-003 | Completed | | 6 | Harden API-key file acquisition | RSK-010 | Pending | | 7 | Bound and verify external result acquisition | RSK-013, TST-007 | Pending | | 8 | Terminate owned subprocess trees | RSK-011 | Pending | diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index d83fa46..502bc9e 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -299,7 +299,7 @@ Symptom: Likely causes: - another process is running for the same session; -- stale lock left by interrupted process. +- a process still holds the operating-system lock while it is shutting down. Diagnostics: @@ -311,7 +311,8 @@ ps aux | grep narratio Safe fix: - wait for active process completion; -- remove stale lock only after confirming no live process owns it. +- retry after an interrupted holder has exited; the kernel releases its lock + even though the `.lock` metadata file remains for inspection. Relevant reference: [Operations: Local State Layout](./operations.md#local-state-layout). diff --git a/internal/app/clean.go b/internal/app/clean.go index 54744a0..ec34fb4 100644 --- a/internal/app/clean.go +++ b/internal/app/clean.go @@ -11,6 +11,7 @@ import ( "gitea.maximumdirect.net/eric/narratio/internal/artifacts" "gitea.maximumdirect.net/eric/narratio/internal/config" + "gitea.maximumdirect.net/eric/narratio/internal/fileops" ) // Clean removes local workspace/spool state while preserving durable cache @@ -135,7 +136,7 @@ func reportCleanScopedDir(out io.Writer, root, target, policy string, dryRun boo fmt.Fprintf(out, "Missing: %s\n", dir.TargetAbs) return nil } - if err := os.RemoveAll(dir.TargetAbs); err != nil { + if err := fileops.RemoveAllUnderRoot(dir.RootAbs, dir.TargetAbs); err != nil { return fmt.Errorf("cleanup policy %s: remove %q: %w", policy, dir.TargetAbs, err) } fmt.Fprintf(out, "Deleted: %s\n", dir.TargetAbs) @@ -160,7 +161,7 @@ func reportCleanRootChildren(out io.Writer, root, policy string, dryRun bool) er fmt.Fprintf(out, "Would delete: %s\n", entry) continue } - if err := os.RemoveAll(entry); err != nil { + if err := fileops.RemoveAllUnderRoot(rootAbs, entry); err != nil { return fmt.Errorf("cleanup policy %s: remove %q: %w", policy, entry, err) } fmt.Fprintf(out, "Deleted: %s\n", entry) @@ -265,7 +266,7 @@ func reportCleanScopedFile(out io.Writer, root, target, policy string, dryRun bo fmt.Fprintf(out, "Missing cache file: %s\n", file.TargetAbs) return false, nil } - if err := os.Remove(file.TargetAbs); err != nil { + if err := fileops.RemoveAllUnderRoot(file.RootAbs, file.TargetAbs); err != nil { return false, fmt.Errorf("cleanup policy %s: remove %q: %w", policy, file.TargetAbs, err) } fmt.Fprintf(out, "Deleted cache file: %s\n", file.TargetAbs) diff --git a/internal/app/post_publish_cleanup.go b/internal/app/post_publish_cleanup.go index 7a1ec44..f758703 100644 --- a/internal/app/post_publish_cleanup.go +++ b/internal/app/post_publish_cleanup.go @@ -3,12 +3,12 @@ package app import ( "context" "fmt" - "os" "path/filepath" "strings" "gitea.maximumdirect.net/eric/narratio/internal/artifacts" "gitea.maximumdirect.net/eric/narratio/internal/config" + "gitea.maximumdirect.net/eric/narratio/internal/fileops" "gitea.maximumdirect.net/eric/narratio/internal/manifest" ) @@ -167,7 +167,7 @@ func removeRunScopedDir(root, target, policy string) error { if !dir.Exists { return nil } - if err := os.RemoveAll(dir.TargetAbs); err != nil { + if err := fileops.RemoveAllUnderRoot(dir.RootAbs, dir.TargetAbs); err != nil { return fmt.Errorf("cleanup policy %s: remove %q: %w", policy, dir.TargetAbs, err) } return nil diff --git a/internal/app/restore.go b/internal/app/restore.go index 57091fa..8dc0f78 100644 --- a/internal/app/restore.go +++ b/internal/app/restore.go @@ -22,7 +22,7 @@ var buildRestorePlanFn = buildRestorePlan var executeRestorePlanFn = executeRestorePlan // Restore validates restore CLI/config inputs and storage preflight for future restore phases. -func Restore(ctx context.Context, args []string, out io.Writer) error { +func Restore(ctx context.Context, args []string, out io.Writer) (resultErr error) { positionalSessionID, args := pullLeadingSessionID(args) fs := flag.NewFlagSet("restore", flag.ContinueOnError) fs.SetOutput(out) @@ -96,12 +96,18 @@ func Restore(ctx context.Context, args []string, out io.Writer) error { if _, err := artifactStore.EnsureLayoutFor(cfg.Session.Campaign, cfg.Session.SessionID); err != nil { return fmt.Errorf("restore: prepare workdir: %w", err) } - lock, err := artifactStore.AcquireSessionLockFor(cfg.Session.Campaign, cfg.Session.SessionID) + lock, err := artifactStore.AcquireSessionLockForContext(ctx, cfg.Session.Campaign, cfg.Session.SessionID) if err != nil { return fmt.Errorf("restore: acquire session lock: %w", err) } defer func() { - _ = artifactStore.ReleaseSessionLock(lock) + if releaseErr := artifactStore.ReleaseSessionLock(lock); releaseErr != nil { + if resultErr == nil { + resultErr = fmt.Errorf("restore: release session lock: %w", releaseErr) + } else { + resultErr = errors.Join(resultErr, fmt.Errorf("restore: release session lock: %w", releaseErr)) + } + } }() if plan.ConflictCount > 0 && !force { diff --git a/internal/app/runner.go b/internal/app/runner.go index 4fc9e48..55f436f 100644 --- a/internal/app/runner.go +++ b/internal/app/runner.go @@ -2,6 +2,7 @@ package app import ( "context" + "errors" "fmt" "log/slog" "os" @@ -39,7 +40,7 @@ type RunSummary struct { var executeStagesFn = executeStages -func executeStages(ctx context.Context, cfg *config.Config, stages []stage.Stage, opts RunOptions) (*RunSummary, error) { +func executeStages(ctx context.Context, cfg *config.Config, stages []stage.Stage, opts RunOptions) (summary *RunSummary, resultErr error) { env := opts.Env if env == nil { env = &Env{} @@ -111,12 +112,18 @@ func executeStages(ctx context.Context, cfg *config.Config, stages []stage.Stage return nil, fmt.Errorf("prepare workdir: %w", err) } - lock, err := artifactStore.AcquireSessionLockFor(cfg.Session.Campaign, cfg.Session.SessionID) + lock, err := artifactStore.AcquireSessionLockForContext(ctx, cfg.Session.Campaign, cfg.Session.SessionID) if err != nil { return nil, fmt.Errorf("acquire session lock: %w", err) } defer func() { - _ = artifactStore.ReleaseSessionLock(lock) + if releaseErr := artifactStore.ReleaseSessionLock(lock); releaseErr != nil { + if resultErr == nil { + resultErr = fmt.Errorf("release session lock: %w", releaseErr) + } else { + resultErr = errors.Join(resultErr, fmt.Errorf("release session lock: %w", releaseErr)) + } + } }() manifestPath := paths.ManifestPath diff --git a/internal/artifacts/local.go b/internal/artifacts/local.go index 80346fc..38f99f3 100644 --- a/internal/artifacts/local.go +++ b/internal/artifacts/local.go @@ -1,6 +1,7 @@ package artifacts import ( + "context" "errors" "fmt" "io" @@ -14,7 +15,7 @@ import ( "gitea.maximumdirect.net/eric/narratio/internal/pathsafe" ) -// ErrLockConflict is returned when a session lock already exists. +// ErrLockConflict is returned when a session lock is currently held. var ErrLockConflict = errors.New("session workdir is already locked") // LockHandle tracks a held lock for a session work directory. @@ -178,32 +179,74 @@ func (s *LocalStore) AcquireSessionLockFor(campaign, sessionID string) (*LockHan return s.acquireSessionLockForPaths(paths) } -func (s *LocalStore) acquireSessionLockForPaths(paths SessionPaths) (*LockHandle, error) { - f, err := os.OpenFile(paths.LockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, fileops.WorkspaceFileMode) +// AcquireSessionLockForContext waits for a session lock until it becomes +// available or ctx is cancelled. +func (s *LocalStore) AcquireSessionLockForContext(ctx context.Context, campaign, sessionID string) (*LockHandle, error) { + if ctx == nil { + ctx = context.Background() + } + paths, err := s.EnsureLayoutFor(campaign, sessionID) if err != nil { - if errors.Is(err, os.ErrExist) { - return nil, fmt.Errorf("%w: %s", ErrLockConflict, paths.LockPath) + return nil, err + } + for { + lock, err := s.acquireSessionLockForPaths(paths) + if !errors.Is(err, ErrLockConflict) { + return lock, err } + timer := time.NewTimer(100 * time.Millisecond) + select { + case <-ctx.Done(): + if !timer.Stop() { + <-timer.C + } + return nil, fmt.Errorf("wait for session lock %q: %w", paths.LockPath, ctx.Err()) + case <-timer.C: + } + } +} + +func (s *LocalStore) acquireSessionLockForPaths(paths SessionPaths) (*LockHandle, error) { + f, err := fileops.OpenFileConfined(paths.LockPath, os.O_CREATE|os.O_RDWR, fileops.WorkspaceFileMode) + if err != nil { return nil, fmt.Errorf("acquire lock %q: %w", paths.LockPath, err) } + if err := acquireHeldFileLockFn(f); err != nil { + closeErr := f.Close() + if errors.Is(err, errHeldLockConflict) { + return nil, fmt.Errorf("%w: %s", ErrLockConflict, paths.LockPath) + } + if closeErr != nil { + return nil, errors.Join(fmt.Errorf("acquire lock %q: hold: %w", paths.LockPath, err), closeErr) + } + return nil, fmt.Errorf("acquire lock %q: hold: %w", paths.LockPath, err) + } + failed := true + defer func() { + if failed { + _ = releaseHeldFileLockFn(f) + _ = f.Close() + } + }() if err := f.Chmod(fileops.WorkspaceFileMode); err != nil { - _ = f.Close() - _ = os.Remove(paths.LockPath) return nil, fmt.Errorf("acquire lock %q: set permissions: %w", paths.LockPath, err) } + if err := f.Truncate(0); err != nil { + return nil, fmt.Errorf("acquire lock %q: clear metadata: %w", paths.LockPath, err) + } + if _, err := f.Seek(0, io.SeekStart); err != nil { + return nil, fmt.Errorf("acquire lock %q: seek metadata: %w", paths.LockPath, err) + } metadata := "pid=" + strconv.Itoa(os.Getpid()) + "\nacquired_at=" + time.Now().UTC().Format(time.RFC3339Nano) + "\n" if _, err := io.WriteString(f, metadata); err != nil { - _ = f.Close() - _ = os.Remove(paths.LockPath) return nil, fmt.Errorf("acquire lock %q: write metadata: %w", paths.LockPath, err) } if err := f.Sync(); err != nil { - _ = f.Close() - _ = os.Remove(paths.LockPath) return nil, fmt.Errorf("acquire lock %q: sync: %w", paths.LockPath, err) } + failed = false return &LockHandle{path: paths.LockPath, file: f}, nil } @@ -213,23 +256,21 @@ func (s *LocalStore) ReleaseSessionLock(lock *LockHandle) error { return nil } - var closeErr error - if lock.file != nil { - closeErr = lock.file.Close() - lock.file = nil + if lock.file == nil { + return nil } - - removeErr := os.Remove(lock.path) - if errors.Is(removeErr, os.ErrNotExist) { - removeErr = nil + releaseErr := releaseHeldFileLockFn(lock.file) + closeErr := closeHeldLockFile(lock.file) + lock.file = nil + if releaseErr != nil && closeErr != nil { + return fmt.Errorf("release lock %q: %w", lock.path, errors.Join(releaseErr, closeErr)) + } + if releaseErr != nil { + return fmt.Errorf("release lock %q: unlock: %w", lock.path, releaseErr) } - if closeErr != nil { return fmt.Errorf("release lock %q: close: %w", lock.path, closeErr) } - if removeErr != nil { - return fmt.Errorf("release lock %q: remove: %w", lock.path, removeErr) - } return nil } diff --git a/internal/artifacts/local_test.go b/internal/artifacts/local_test.go index de52b72..5229a7e 100644 --- a/internal/artifacts/local_test.go +++ b/internal/artifacts/local_test.go @@ -1,11 +1,14 @@ package artifacts import ( + "context" "errors" "os" + "os/exec" "path/filepath" "strings" "testing" + "time" ) func TestEnsureLayoutCreatesExpectedDirectories(t *testing.T) { @@ -93,8 +96,148 @@ func TestLockAcquireRelease(t *testing.T) { if err != nil { t.Fatalf("Exists() error = %v", err) } - if exists { - t.Fatalf("expected lock file %q to be removed", lock.path) + if !exists { + t.Fatalf("expected lock metadata file %q to remain", lock.path) + } + secondLock, err := store.AcquireSessionLockFor("sample-campaign", "session-1") + if err != nil { + t.Fatalf("AcquireSessionLockFor() after release error = %v", err) + } + if err := store.ReleaseSessionLock(secondLock); err != nil { + t.Fatalf("ReleaseSessionLock(second lock) error = %v", err) + } +} + +func TestLockWaitHonorsCancellation(t *testing.T) { + store := NewLocalStore(t.TempDir()) + lock, err := store.AcquireSessionLockFor("sample-campaign", "session-1") + if err != nil { + t.Fatalf("AcquireSessionLockFor() error = %v", err) + } + defer func() { _ = store.ReleaseSessionLock(lock) }() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _, err = store.AcquireSessionLockForContext(ctx, "sample-campaign", "session-1") + if !errors.Is(err, context.Canceled) { + t.Fatalf("AcquireSessionLockForContext() error = %v, want context cancellation", err) + } +} + +func TestLockWaitsUntilRelease(t *testing.T) { + store := NewLocalStore(t.TempDir()) + first, err := store.AcquireSessionLockFor("sample-campaign", "session-1") + if err != nil { + t.Fatalf("AcquireSessionLockFor() error = %v", err) + } + acquired := make(chan *LockHandle, 1) + errs := make(chan error, 1) + go func() { + lock, err := store.AcquireSessionLockForContext(context.Background(), "sample-campaign", "session-1") + if err != nil { + errs <- err + return + } + acquired <- lock + }() + + select { + case err := <-errs: + t.Fatalf("AcquireSessionLockForContext() error = %v", err) + case lock := <-acquired: + _ = store.ReleaseSessionLock(lock) + t.Fatal("AcquireSessionLockForContext() acquired before the held lock was released") + case <-time.After(150 * time.Millisecond): + } + if err := store.ReleaseSessionLock(first); err != nil { + t.Fatalf("ReleaseSessionLock(first) error = %v", err) + } + select { + case err := <-errs: + t.Fatalf("AcquireSessionLockForContext() error = %v", err) + case lock := <-acquired: + if err := store.ReleaseSessionLock(lock); err != nil { + t.Fatalf("ReleaseSessionLock(waiting lock) error = %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("AcquireSessionLockForContext() did not acquire after release") + } +} + +func TestLockRecoversAfterLockHolderDies(t *testing.T) { + if os.Getenv("NARRATIO_LOCK_PROCESS_HELPER") == "1" { + store := NewLocalStore(os.Getenv("NARRATIO_LOCK_PROCESS_ROOT")) + lock, err := store.AcquireSessionLockFor("sample-campaign", "session-1") + if err != nil { + os.Exit(2) + } + if err := os.WriteFile(os.Getenv("NARRATIO_LOCK_PROCESS_READY"), []byte(lock.path), 0o600); err != nil { + os.Exit(3) + } + select {} + } + + root := t.TempDir() + ready := filepath.Join(t.TempDir(), "ready") + cmd := exec.Command(os.Args[0], "-test.run=^TestLockRecoversAfterLockHolderDies$") + cmd.Env = append(os.Environ(), + "NARRATIO_LOCK_PROCESS_HELPER=1", + "NARRATIO_LOCK_PROCESS_ROOT="+root, + "NARRATIO_LOCK_PROCESS_READY="+ready, + ) + if err := cmd.Start(); err != nil { + t.Fatalf("start lock-holder process: %v", err) + } + defer func() { + if cmd.Process != nil { + _ = cmd.Process.Kill() + } + _ = cmd.Wait() + }() + deadline := time.Now().Add(2 * time.Second) + for { + if _, err := os.Stat(ready); err == nil { + break + } + if time.Now().After(deadline) { + t.Fatal("lock-holder process did not signal readiness") + } + time.Sleep(10 * time.Millisecond) + } + + store := NewLocalStore(root) + if _, err := store.AcquireSessionLockFor("sample-campaign", "session-1"); !errors.Is(err, ErrLockConflict) { + t.Fatalf("AcquireSessionLockFor() error = %v, want lock conflict", err) + } + if err := cmd.Process.Kill(); err != nil { + t.Fatalf("kill lock-holder process: %v", err) + } + if err := cmd.Wait(); err == nil { + t.Fatal("lock-holder process exited without being killed") + } + cmd.Process = nil + + lock, err := store.AcquireSessionLockFor("sample-campaign", "session-1") + if err != nil { + t.Fatalf("AcquireSessionLockFor() after holder death error = %v", err) + } + if err := store.ReleaseSessionLock(lock); err != nil { + t.Fatalf("ReleaseSessionLock() error = %v", err) + } +} + +func TestReleaseSessionLockReportsUnlockFailure(t *testing.T) { + store := NewLocalStore(t.TempDir()) + lock, err := store.AcquireSessionLockFor("sample-campaign", "session-1") + if err != nil { + t.Fatalf("AcquireSessionLockFor() error = %v", err) + } + + original := releaseHeldFileLockFn + releaseHeldFileLockFn = func(*os.File) error { return errors.New("unlock failed") } + t.Cleanup(func() { releaseHeldFileLockFn = original }) + if err := store.ReleaseSessionLock(lock); err == nil || !strings.Contains(err.Error(), "unlock failed") { + t.Fatalf("ReleaseSessionLock() error = %v, want unlock failure", err) } } diff --git a/internal/artifacts/lock_common.go b/internal/artifacts/lock_common.go new file mode 100644 index 0000000..341f6df --- /dev/null +++ b/internal/artifacts/lock_common.go @@ -0,0 +1,18 @@ +package artifacts + +import ( + "errors" + "os" +) + +var errHeldLockConflict = errors.New("held lock conflict") + +var acquireHeldFileLockFn = acquireHeldFileLock +var releaseHeldFileLockFn = releaseHeldFileLock + +func closeHeldLockFile(file *os.File) error { + if file == nil { + return nil + } + return file.Close() +} diff --git a/internal/artifacts/lock_unix.go b/internal/artifacts/lock_unix.go new file mode 100644 index 0000000..908481e --- /dev/null +++ b/internal/artifacts/lock_unix.go @@ -0,0 +1,22 @@ +//go:build linux || darwin + +package artifacts + +import ( + "errors" + "os" + + "golang.org/x/sys/unix" +) + +func acquireHeldFileLock(file *os.File) error { + err := unix.Flock(int(file.Fd()), unix.LOCK_EX|unix.LOCK_NB) + if errors.Is(err, unix.EWOULDBLOCK) || errors.Is(err, unix.EAGAIN) { + return errHeldLockConflict + } + return err +} + +func releaseHeldFileLock(file *os.File) error { + return unix.Flock(int(file.Fd()), unix.LOCK_UN) +} diff --git a/internal/artifacts/lock_unsupported.go b/internal/artifacts/lock_unsupported.go new file mode 100644 index 0000000..87f0abf --- /dev/null +++ b/internal/artifacts/lock_unsupported.go @@ -0,0 +1,15 @@ +//go:build !linux && !darwin && !windows + +package artifacts + +import ( + "fmt" + "os" + "runtime" +) + +func acquireHeldFileLock(_ *os.File) error { + return fmt.Errorf("held file locks are unsupported on %s", runtime.GOOS) +} + +func releaseHeldFileLock(_ *os.File) error { return nil } diff --git a/internal/artifacts/lock_windows.go b/internal/artifacts/lock_windows.go new file mode 100644 index 0000000..1ba35d2 --- /dev/null +++ b/internal/artifacts/lock_windows.go @@ -0,0 +1,29 @@ +//go:build windows + +package artifacts + +import ( + "errors" + "os" + + "golang.org/x/sys/windows" +) + +func acquireHeldFileLock(file *os.File) error { + err := windows.LockFileEx( + windows.Handle(file.Fd()), + windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, + 0, + 1, + 0, + &windows.Overlapped{}, + ) + if errors.Is(err, windows.ERROR_LOCK_VIOLATION) { + return errHeldLockConflict + } + return err +} + +func releaseHeldFileLock(file *os.File) error { + return windows.UnlockFileEx(windows.Handle(file.Fd()), 0, 1, 0, &windows.Overlapped{}) +} diff --git a/internal/artifacts/store.go b/internal/artifacts/store.go index e2806b1..d2d2fd2 100644 --- a/internal/artifacts/store.go +++ b/internal/artifacts/store.go @@ -1,6 +1,7 @@ package artifacts import ( + "context" "os" "gitea.maximumdirect.net/eric/narratio/internal/artifactmodel" @@ -30,5 +31,6 @@ type Store interface { WriteFileAtomic(path string, data []byte, perm os.FileMode) error Checksum(path string) (string, error) AcquireSessionLockFor(campaign, sessionID string) (*LockHandle, error) + AcquireSessionLockForContext(ctx context.Context, campaign, sessionID string) (*LockHandle, error) ReleaseSessionLock(lock *LockHandle) error } diff --git a/internal/fileops/cleanup.go b/internal/fileops/cleanup.go new file mode 100644 index 0000000..26a8b78 --- /dev/null +++ b/internal/fileops/cleanup.go @@ -0,0 +1,165 @@ +package fileops + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "sort" + "strings" +) + +// RemoveAllUnderRoot removes target and its contents only after opening root and +// every target ancestor without following symbolic links. target must name a +// proper descendant of root. Symlinks are rejected rather than followed or +// removed so a failed cleanup never silently changes its scope. +func RemoveAllUnderRoot(rootPath, target string) error { + root, targetName, err := openConfinedCleanupTarget(rootPath, target) + if err != nil { + return err + } + defer func() { _ = root.Close() }() + return removeConfinedEntry(root, targetName) +} + +func openConfinedCleanupTarget(rootPath, target string) (*os.Root, string, error) { + if strings.TrimSpace(rootPath) == "" { + return nil, "", fmt.Errorf("cleanup root is required") + } + if strings.TrimSpace(target) == "" { + return nil, "", fmt.Errorf("cleanup target is required") + } + rootAbs, err := filepath.Abs(rootPath) + if err != nil { + return nil, "", fmt.Errorf("resolve cleanup root: %w", err) + } + targetAbs, err := filepath.Abs(target) + if err != nil { + return nil, "", fmt.Errorf("resolve cleanup target: %w", err) + } + relative, err := filepath.Rel(rootAbs, targetAbs) + if err != nil { + return nil, "", fmt.Errorf("resolve cleanup target below root: %w", err) + } + if relative == "." || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + return nil, "", fmt.Errorf("cleanup target %q must be below root %q", targetAbs, rootAbs) + } + parts, err := relativePathParts(relative) + if err != nil { + return nil, "", err + } + root, err := openConfinedDirectory(rootAbs) + if err != nil { + return nil, "", err + } + for _, part := range parts[:len(parts)-1] { + child, err := openConfinedChild(root, part, false, 0) + if err != nil { + _ = root.Close() + return nil, "", err + } + _ = root.Close() + root = child + } + return root, parts[len(parts)-1], nil +} + +func openConfinedDirectory(path string) (*os.Root, error) { + abs, err := filepath.Abs(path) + if err != nil { + return nil, fmt.Errorf("resolve directory: %w", err) + } + parts, err := absolutePathParts(abs) + if err != nil { + return nil, err + } + rootPath := filesystemRoot(abs) + root, err := os.OpenRoot(rootPath) + if err != nil { + return nil, fmt.Errorf("open filesystem root %q: %w", rootPath, err) + } + for _, part := range parts { + child, err := openConfinedChild(root, part, false, 0) + if err != nil { + _ = root.Close() + return nil, err + } + _ = root.Close() + root = child + } + return root, nil +} + +func relativePathParts(path string) ([]string, error) { + parts := strings.FieldsFunc(path, func(r rune) bool { return r == filepath.Separator || r == '/' || r == '\\' }) + if len(parts) == 0 { + return nil, fmt.Errorf("cleanup target is required") + } + for _, part := range parts { + if part == "" || part == "." || part == ".." { + return nil, fmt.Errorf("unsafe cleanup path component %q", part) + } + } + return parts, nil +} + +func removeConfinedEntry(parent *os.Root, name string) error { + info, err := parent.Lstat(name) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("inspect cleanup entry %q: %w", name, err) + } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing to delete symlink path %q", name) + } + if !info.IsDir() { + if err := parent.Remove(name); err != nil { + return fmt.Errorf("remove cleanup entry %q: %w", name, err) + } + return nil + } + + child, err := openConfinedChild(parent, name, false, 0) + if err != nil { + return err + } + err = removeConfinedChildren(child) + closeErr := child.Close() + if err != nil { + if closeErr != nil { + return errors.Join(err, closeErr) + } + return err + } + if closeErr != nil { + return fmt.Errorf("close cleanup directory %q: %w", name, closeErr) + } + if err := parent.Remove(name); err != nil { + return fmt.Errorf("remove cleanup directory %q: %w", name, err) + } + return nil +} + +func removeConfinedChildren(root *os.Root) error { + directory, err := root.Open(".") + if err != nil { + return fmt.Errorf("open cleanup directory: %w", err) + } + entries, readErr := directory.ReadDir(-1) + closeErr := directory.Close() + if readErr != nil { + return fmt.Errorf("read cleanup directory: %w", readErr) + } + if closeErr != nil { + return fmt.Errorf("close cleanup directory: %w", closeErr) + } + sort.Slice(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() }) + for _, entry := range entries { + if err := removeConfinedEntry(root, entry.Name()); err != nil { + return err + } + } + return nil +} diff --git a/internal/fileops/cleanup_test.go b/internal/fileops/cleanup_test.go new file mode 100644 index 0000000..a5bdeea --- /dev/null +++ b/internal/fileops/cleanup_test.go @@ -0,0 +1,79 @@ +package fileops + +import ( + "os" + "path/filepath" + "testing" +) + +func TestRemoveAllUnderRootRemovesNestedTreeIdempotently(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "runs", "run-1") + mustWriteFile(t, filepath.Join(target, "nested", "result.txt"), []byte("result"), 0o640) + + if err := RemoveAllUnderRoot(root, target); err != nil { + t.Fatalf("RemoveAllUnderRoot() error = %v", err) + } + if _, err := os.Lstat(target); !os.IsNotExist(err) { + t.Fatalf("Lstat(%q) error = %v, want not exist", target, err) + } + if err := RemoveAllUnderRoot(root, target); err != nil { + t.Fatalf("second RemoveAllUnderRoot() error = %v", err) + } +} + +func TestRemoveAllUnderRootRejectsUnsafeTargets(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + target := filepath.Join(root, "runs", "run-1") + mustWriteFile(t, filepath.Join(target, "result.txt"), []byte("result"), 0o640) + sentinel := filepath.Join(outside, "sentinel.txt") + mustWriteFile(t, sentinel, []byte("outside"), 0o640) + + if err := RemoveAllUnderRoot(root, root); err == nil { + t.Fatal("RemoveAllUnderRoot(root, root) error = nil, want root rejection") + } + if err := RemoveAllUnderRoot(root, filepath.Join(outside, "target")); err == nil { + t.Fatal("RemoveAllUnderRoot(outside) error = nil, want outside-root rejection") + } + if err := os.Symlink(outside, filepath.Join(root, "runs")); err == nil { + t.Fatal("Symlink() error = nil, want collision because runs already exists") + } + if err := os.RemoveAll(filepath.Join(root, "runs")); err != nil { + t.Fatalf("RemoveAll(runs) error = %v", err) + } + if err := os.Symlink(outside, filepath.Join(root, "runs")); err != nil { + t.Fatalf("Symlink() error = %v", err) + } + if err := RemoveAllUnderRoot(root, target); err == nil { + t.Fatal("RemoveAllUnderRoot(symlinked ancestor) error = nil, want rejection") + } + data, err := os.ReadFile(sentinel) + if err != nil { + t.Fatalf("ReadFile(sentinel) error = %v", err) + } + if string(data) != "outside" { + t.Fatalf("sentinel content = %q, want outside", data) + } +} + +func TestRemoveAllUnderRootRejectsSymlinkInTree(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + target := filepath.Join(root, "runs", "run-1") + if err := os.MkdirAll(target, 0o750); err != nil { + t.Fatalf("MkdirAll(target) error = %v", err) + } + sentinel := filepath.Join(outside, "sentinel.txt") + mustWriteFile(t, sentinel, []byte("outside"), 0o640) + if err := os.Symlink(outside, filepath.Join(target, "link")); err != nil { + t.Fatalf("Symlink() error = %v", err) + } + + if err := RemoveAllUnderRoot(root, target); err == nil { + t.Fatal("RemoveAllUnderRoot() error = nil, want symlink rejection") + } + if _, err := os.Stat(sentinel); err != nil { + t.Fatalf("outside sentinel was changed: %v", err) + } +} diff --git a/internal/fileops/confined.go b/internal/fileops/confined.go index ae328f8..ebff677 100644 --- a/internal/fileops/confined.go +++ b/internal/fileops/confined.go @@ -183,6 +183,39 @@ func syncOpenedDirectory(parent *os.Root) error { return syncDirectoryFile(directory, parent.Name()) } +// OpenFileConfined opens a file after verifying its parent hierarchy without +// following symbolic links. Existing symbolic-link leaves are rejected. +func OpenFileConfined(path string, flags int, mode os.FileMode) (*os.File, error) { + parent, name, err := openConfinedParent(path, false, 0) + if err != nil { + return nil, err + } + defer func() { _ = parent.Close() }() + if info, err := parent.Lstat(name); err == nil && info.Mode()&os.ModeSymlink != 0 { + return nil, fmt.Errorf("destination file %q is a symbolic link", name) + } else if err != nil && !errors.Is(err, os.ErrNotExist) { + return nil, fmt.Errorf("inspect destination file %q: %w", name, err) + } + file, err := parent.OpenFile(name, flags, mode) + if err != nil { + return nil, err + } + opened, err := file.Stat() + if err != nil { + _ = file.Close() + return nil, fmt.Errorf("inspect opened destination file %q: %w", name, err) + } + current, err := parent.Lstat(name) + if err != nil || current.Mode()&os.ModeSymlink != 0 || !os.SameFile(opened, current) { + _ = file.Close() + if err != nil { + return nil, fmt.Errorf("reinspect destination file %q: %w", name, err) + } + return nil, fmt.Errorf("destination file %q changed while being opened", name) + } + return file, nil +} + // DownloadedTempFile is a completed, destination-confined temporary file. It // keeps its parent directory open until the caller installs or cleans it up. type DownloadedTempFile struct { diff --git a/internal/fileops/directory.go b/internal/fileops/directory.go index 857541c..5747f63 100644 --- a/internal/fileops/directory.go +++ b/internal/fileops/directory.go @@ -1,6 +1,8 @@ package fileops import ( + "crypto/rand" + "encoding/hex" "errors" "fmt" "io" @@ -21,14 +23,257 @@ func PromoteDirectory(src, dst string) error { if err := checkAtomicDirectoryPromotionSupport(); err != nil { return err } - parent, _, err := openConfinedParent(dst, false, 0) + parent, destinationName, err := openConfinedParent(dst, false, 0) if err != nil { return fmt.Errorf("open destination parent: %w", err) } - if err := parent.Close(); err != nil { - return fmt.Errorf("close destination parent: %w", err) + defer func() { _ = parent.Close() }() + return promoteDirectoryConfined(src, dst, parent, destinationName, sourceTraversalHooks{}) +} + +func promoteDirectoryConfined(src, dst string, parent *os.Root, destinationName string, hooks sourceTraversalHooks) (resultErr error) { + if strings.TrimSpace(src) == "" || strings.TrimSpace(dst) == "" { + return fmt.Errorf("source and destination directory paths are required") } - return promoteDirectory(src, dst, renameDirectoryNoReplace) + sourceInfo, err := os.Lstat(src) + if err != nil { + return fmt.Errorf("inspect source directory: %w", err) + } + if sourceInfo.Mode()&os.ModeSymlink != 0 || !sourceInfo.IsDir() { + return fmt.Errorf("source path %q is not a directory", src) + } + if _, err := parent.Lstat(destinationName); err == nil { + return fmt.Errorf("destination path %q already exists", dst) + } else if !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("inspect destination path: %w", err) + } + insideSource, err := pathWithin(src, dst) + if err != nil { + return err + } + if insideSource { + return fmt.Errorf("destination path %q must not be inside source directory %q", dst, src) + } + + temporary, temporaryName, err := createSiblingTempDirectory(parent, destinationName) + if err != nil { + return err + } + removeTemporary := true + defer func() { + var closeErr error + if temporary != nil { + closeErr = temporary.Close() + } + if removeTemporary { + removeErr := removeConfinedEntry(parent, temporaryName) + if removeErr != nil { + resultErr = errors.Join(resultErr, fmt.Errorf("remove temporary destination directory: %w", removeErr)) + } + } + if closeErr != nil { + resultErr = errors.Join(resultErr, fmt.Errorf("close temporary destination directory: %w", closeErr)) + } + }() + + sourceRoot, err := openVerifiedSourceRoot(src, sourceInfo, hooks) + if err != nil { + return err + } + defer func() { _ = sourceRoot.Close() }() + if err := copyRegularTreeToRoot(sourceRoot, src, temporary, hooks); err != nil { + return err + } + if err := setOpenedDirectoryMode(temporary, WorkspaceDirectoryMode); err != nil { + return fmt.Errorf("set temporary root permissions: %w", err) + } + if err := syncOpenedDirectory(temporary); err != nil { + return fmt.Errorf("sync temporary root: %w", err) + } + if err := temporary.Close(); err != nil { + return fmt.Errorf("close temporary root: %w", err) + } + temporary = nil + + directory, err := parent.Open(".") + if err != nil { + return fmt.Errorf("open destination parent for promotion: %w", err) + } + renameErr := renameDirectoryNoReplaceAt(directory, temporaryName, destinationName) + closeErr := directory.Close() + if renameErr != nil { + return fmt.Errorf("install promoted directory: %w", renameErr) + } + if closeErr != nil { + return fmt.Errorf("close destination parent after promotion: %w", closeErr) + } + removeTemporary = false + if err := syncOpenedDirectory(parent); err != nil { + return fmt.Errorf("sync destination parent: %w", err) + } + return nil +} + +func createSiblingTempDirectory(parent *os.Root, base string) (*os.Root, string, error) { + for attempt := 0; attempt < 100; attempt++ { + var randomBytes [16]byte + if _, err := rand.Read(randomBytes[:]); err != nil { + return nil, "", fmt.Errorf("generate temporary directory name: %w", err) + } + name := "." + base + ".tmp-" + hex.EncodeToString(randomBytes[:]) + if err := parent.Mkdir(name, WorkspaceDirectoryMode.Perm()); err != nil { + if errors.Is(err, os.ErrExist) { + continue + } + return nil, "", fmt.Errorf("create temporary destination directory: %w", err) + } + directory, err := openConfinedChild(parent, name, false, 0) + if err != nil { + _ = parent.Remove(name) + return nil, "", err + } + return directory, name, nil + } + return nil, "", fmt.Errorf("create unique temporary destination directory") +} + +func copyRegularTreeToRoot(src *os.Root, sourcePath string, dst *os.Root, hooks sourceTraversalHooks) error { + directory, err := src.Open(".") + if err != nil { + return fmt.Errorf("open source directory %q for traversal: %w", sourcePath, err) + } + entries, readErr := directory.ReadDir(-1) + closeErr := directory.Close() + if readErr != nil { + return fmt.Errorf("read source directory %q: %w", sourcePath, readErr) + } + if closeErr != nil { + return fmt.Errorf("close source directory %q: %w", sourcePath, closeErr) + } + sort.Slice(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() }) + for _, entry := range entries { + entryPath := filepath.Join(sourcePath, entry.Name()) + info, err := src.Lstat(entry.Name()) + if err != nil { + return fmt.Errorf("inspect source entry %q: %w", entryPath, err) + } + if hooks.afterEntryInspect != nil { + hooks.afterEntryInspect(entryPath) + } + switch { + case info.Mode().IsRegular(): + if err := copyRegularFileToRoot(src, dst, entry.Name(), entryPath, info); err != nil { + return err + } + case info.IsDir(): + if err := copyRegularDirectoryToRoot(src, dst, entry.Name(), entryPath, info, hooks); err != nil { + return err + } + default: + return fmt.Errorf("source entry %q has unsupported file type %s", entryPath, info.Mode().Type()) + } + } + return nil +} + +func copyRegularDirectoryToRoot(sourceParent, destinationParent *os.Root, name, sourcePath string, inspected os.FileInfo, hooks sourceTraversalHooks) error { + source, err := openVerifiedChildDirectory(sourceParent, name, sourcePath, inspected) + if err != nil { + return err + } + defer func() { _ = source.Close() }() + if err := destinationParent.Mkdir(name, WorkspaceDirectoryMode.Perm()); err != nil { + return fmt.Errorf("create destination directory %q: %w", sourcePath, err) + } + destination, err := openConfinedChild(destinationParent, name, false, 0) + if err != nil { + return err + } + err = copyRegularTreeToRoot(source, sourcePath, destination, hooks) + if err == nil { + err = setOpenedDirectoryMode(destination, WorkspaceDirectoryMode) + } + if err == nil { + err = syncOpenedDirectory(destination) + } + closeErr := destination.Close() + if err != nil { + return err + } + if closeErr != nil { + return fmt.Errorf("close destination directory %q: %w", sourcePath, closeErr) + } + return nil +} + +func copyRegularFileToRoot(sourceRoot, destinationRoot *os.Root, name, sourcePath string, inspected os.FileInfo) error { + in, err := openVerifiedSourceFile(sourceRoot, name, sourcePath, inspected) + if err != nil { + return err + } + defer func() { _ = in.Close() }() + out, err := destinationRoot.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_EXCL, WorkspaceFileMode) + if err != nil { + return fmt.Errorf("create destination file %q: %w", sourcePath, err) + } + if _, err := io.Copy(out, in); err != nil { + _ = out.Close() + return fmt.Errorf("copy source file %q: %w", sourcePath, err) + } + if err := out.Chmod(WorkspaceFileMode); err != nil { + _ = out.Close() + return fmt.Errorf("set destination file permissions %q: %w", sourcePath, err) + } + if err := out.Sync(); err != nil { + _ = out.Close() + return fmt.Errorf("sync destination file %q: %w", sourcePath, err) + } + if err := out.Close(); err != nil { + return fmt.Errorf("close destination file %q: %w", sourcePath, err) + } + return nil +} + +func openVerifiedChildDirectory(parent *os.Root, name, sourcePath string, inspected os.FileInfo) (*os.Root, error) { + child, err := parent.OpenRoot(name) + if err != nil { + return nil, fmt.Errorf("open source directory %q: %w", sourcePath, err) + } + opened, err := child.Stat(".") + if err != nil { + _ = child.Close() + return nil, fmt.Errorf("inspect opened source directory %q: %w", sourcePath, err) + } + current, err := parent.Lstat(name) + if err != nil || current.Mode()&os.ModeSymlink != 0 || !current.IsDir() || !os.SameFile(opened, current) { + _ = child.Close() + if err != nil { + return nil, fmt.Errorf("reinspect source directory %q: %w", sourcePath, err) + } + return nil, fmt.Errorf("source directory %q changed while being copied", sourcePath) + } + return child, nil +} + +func openVerifiedSourceFile(root *os.Root, name, sourcePath string, inspected os.FileInfo) (*os.File, error) { + in, err := root.Open(name) + if err != nil { + return nil, fmt.Errorf("open source file %q: %w", sourcePath, err) + } + opened, err := in.Stat() + if err != nil { + _ = in.Close() + return nil, fmt.Errorf("inspect opened source file %q: %w", sourcePath, err) + } + current, err := root.Lstat(name) + if err != nil || current.Mode()&os.ModeSymlink != 0 || !current.Mode().IsRegular() || !os.SameFile(opened, current) { + _ = in.Close() + if err != nil { + return nil, fmt.Errorf("reinspect source file %q: %w", sourcePath, err) + } + return nil, fmt.Errorf("source file %q changed while being copied", sourcePath) + } + return in, nil } func promoteDirectory(src, dst string, install func(string, string) error) error { @@ -87,7 +332,7 @@ func promoteDirectoryWithHooks( removeTemporary := true defer func() { if removeTemporary { - _ = os.RemoveAll(temporary) + _ = RemoveAllUnderRoot(destinationParent, temporary) } }() diff --git a/internal/fileops/rename_noreplace_darwin.go b/internal/fileops/rename_noreplace_darwin.go index 399fa48..36bd3d0 100644 --- a/internal/fileops/rename_noreplace_darwin.go +++ b/internal/fileops/rename_noreplace_darwin.go @@ -2,10 +2,18 @@ package fileops -import "golang.org/x/sys/unix" +import ( + "os" + + "golang.org/x/sys/unix" +) func checkAtomicDirectoryPromotionSupport() error { return nil } func renameDirectoryNoReplace(src, dst string) error { return unix.RenamexNp(src, dst, unix.RENAME_EXCL) } + +func renameDirectoryNoReplaceAt(parent *os.File, src, dst string) error { + return unix.RenameatxNp(int(parent.Fd()), src, int(parent.Fd()), dst, unix.RENAME_EXCL) +} diff --git a/internal/fileops/rename_noreplace_linux.go b/internal/fileops/rename_noreplace_linux.go index 42fe2e5..db65b90 100644 --- a/internal/fileops/rename_noreplace_linux.go +++ b/internal/fileops/rename_noreplace_linux.go @@ -2,10 +2,18 @@ package fileops -import "golang.org/x/sys/unix" +import ( + "os" + + "golang.org/x/sys/unix" +) func checkAtomicDirectoryPromotionSupport() error { return nil } func renameDirectoryNoReplace(src, dst string) error { return unix.Renameat2(unix.AT_FDCWD, src, unix.AT_FDCWD, dst, unix.RENAME_NOREPLACE) } + +func renameDirectoryNoReplaceAt(parent *os.File, src, dst string) error { + return unix.Renameat2(int(parent.Fd()), src, int(parent.Fd()), dst, unix.RENAME_NOREPLACE) +} diff --git a/internal/fileops/rename_noreplace_other.go b/internal/fileops/rename_noreplace_other.go index c6a7d5c..cb93a9b 100644 --- a/internal/fileops/rename_noreplace_other.go +++ b/internal/fileops/rename_noreplace_other.go @@ -4,6 +4,7 @@ package fileops import ( "fmt" + "os" "runtime" ) @@ -14,3 +15,7 @@ func checkAtomicDirectoryPromotionSupport() error { func renameDirectoryNoReplace(_, _ string) error { return checkAtomicDirectoryPromotionSupport() } + +func renameDirectoryNoReplaceAt(_ *os.File, _, _ string) error { + return checkAtomicDirectoryPromotionSupport() +} diff --git a/internal/fileops/rename_noreplace_windows.go b/internal/fileops/rename_noreplace_windows.go index 41fbe14..cb345f3 100644 --- a/internal/fileops/rename_noreplace_windows.go +++ b/internal/fileops/rename_noreplace_windows.go @@ -2,18 +2,17 @@ package fileops -import "golang.org/x/sys/windows" +import ( + "fmt" + "os" +) -func checkAtomicDirectoryPromotionSupport() error { return nil } - -func renameDirectoryNoReplace(src, dst string) error { - from, err := windows.UTF16PtrFromString(src) - if err != nil { - return err - } - to, err := windows.UTF16PtrFromString(dst) - if err != nil { - return err - } - return windows.MoveFileEx(from, to, 0) +func checkAtomicDirectoryPromotionSupport() error { + return fmt.Errorf("%w on windows", ErrAtomicDirectoryPromotionUnsupported) +} + +func renameDirectoryNoReplace(src, dst string) error { return checkAtomicDirectoryPromotionSupport() } + +func renameDirectoryNoReplaceAt(parent *os.File, src, dst string) error { + return checkAtomicDirectoryPromotionSupport() } diff --git a/internal/stage/prepare.go b/internal/stage/prepare.go index 6d87dc5..5984709 100644 --- a/internal/stage/prepare.go +++ b/internal/stage/prepare.go @@ -515,7 +515,7 @@ func clearManagedPreviousState(paths artifacts.SessionPaths) error { if filepath.Base(previousDir) != config.PathPreviousDirSegment { return fmt.Errorf("refusing to clear non-previous path %q", previousDir) } - if err := os.RemoveAll(previousDir); err != nil { + if err := fileops.RemoveAllUnderRoot(sessionRoot, previousDir); err != nil { return err } return fileops.EnsureWorkspaceDirectory(previousDir)