diff --git a/docs/internal/artifacts.md b/docs/internal/artifacts.md index 0cc6b27..ca1a117 100644 --- a/docs/internal/artifacts.md +++ b/docs/internal/artifacts.md @@ -183,7 +183,9 @@ Each read uses the generation and size metadata returned with its opened body. Actual bytes remain subject to a limit-plus-one read even if size metadata is absent or inaccurate. Immutable selections then retain their declared-size, checksum, generation, and identity checks. No current-state control object is -downloaded through a temporary file. +downloaded through a temporary file. Artifact and previous-session downloads +validate their complete sibling temporary file against available committed +identity before replacing a canonical local path. Core helpers: diff --git a/docs/internal/command-restore.md b/docs/internal/command-restore.md index 5f8b38c..f22923a 100644 --- a/docs/internal/command-restore.md +++ b/docs/internal/command-restore.md @@ -53,7 +53,8 @@ Execution order and safety: - non-manifest downloads happen before manifest install; - `manifest.json` installs last; -- downloads use sibling temp files plus atomic rename; +- downloads use sibling temp files, validate the complete object, then use an + atomic rename; - manifest replacement is validated before rename; - each committed object is verified against its declared checksum, size, and generation before installation; @@ -73,9 +74,9 @@ Audio restore path: - uses `audio.MaterializeS3Audio`; - integrates spool and S3 audio cache paths; -- reuses cached audio only when its no-follow regular file, content digest, and +- reuses cached audio only when its opened regular file, content digest, and identity sidecar all match the selected remote object version; otherwise it - refreshes through the durable download path. + refreshes through the validated download path. ## Reporting Contract diff --git a/docs/internal/storage.md b/docs/internal/storage.md index bc6658f..dfc86f5 100644 --- a/docs/internal/storage.md +++ b/docs/internal/storage.md @@ -55,6 +55,11 @@ not own discovery, defaults, or configuration validation. - bounded reads never retain more than the caller's limit plus one byte and do not replace owner-specific size policy. - publish ordering semantics are owned by stage/app code, not storage adapters. +- A caller that installs remote content into a canonical workspace path first + writes a sibling temporary file, validates the complete object against its + owner-specific contract, and only then replaces the destination. Temporary + files used solely for remote comparisons are removed by their callers and do + not represent installed workspace state. ## Implementation And Tests diff --git a/docs/operations.md b/docs/operations.md index ef8cdde..9c26f19 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -498,6 +498,8 @@ Cache layout (durable S3 audio cache): Each cached audio file has an adjacent managed identity record. It binds the file to its remote object version and verified digest; deleting or altering the record simply causes Narratio to download and verify the object again. +Remote audio and restored files are fully transferred into sibling temporary +files and validated before they replace their canonical workspace paths. ### Workspace Permissions diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index e63a365..bb3572a 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -304,7 +304,7 @@ loading. ## Stage 3 — Validate Downloads Before Canonical Installation -**Status: Pending** +**Status: Completed** ### Goal diff --git a/internal/app/restore_execute.go b/internal/app/restore_execute.go index 7dca615..39cd839 100644 --- a/internal/app/restore_execute.go +++ b/internal/app/restore_execute.go @@ -7,6 +7,7 @@ import ( "encoding/hex" "fmt" "io" + "os" "path/filepath" "strings" @@ -101,10 +102,7 @@ func executeRestoreDownloadAction( return executeRestoreAudioAction(ctx, cfg, safeLocalPath, action, store) } - if err := fileops.EnsureWorkspaceDirectory(filepath.Dir(safeLocalPath)); err != nil { - return fmt.Errorf("create destination directory: %w", err) - } - temporary, err := fileops.DownloadToSiblingTemp(safeLocalPath, func(destination io.Writer) error { + temporaryPath, err := downloadRestoreTemp(safeLocalPath, func(destination io.Writer) error { if len(action.VerifiedContent) > 0 { _, err := io.Copy(destination, bytes.NewReader(action.VerifiedContent)) return err @@ -114,13 +112,13 @@ func executeRestoreDownloadAction( if err != nil { return fmt.Errorf("download to temp file: %w", err) } - defer func() { _ = temporary.Cleanup() }() - if err := verifyRestoredObject(ctx, store, action, temporary); err != nil { + defer func() { _ = os.Remove(temporaryPath) }() + if err := verifyRestoredObject(ctx, store, action, temporaryPath); err != nil { return err } if isRestoreManifest(action.LocalRelativePath) { - file, err := temporary.Open() + file, err := os.Open(temporaryPath) if err != nil { return fmt.Errorf("open restored manifest: %w", err) } @@ -143,7 +141,7 @@ func executeRestoreDownloadAction( return nil } - if err := temporary.Install(filepath.Base(safeLocalPath), fileops.WorkspaceFileMode); err != nil { + if err := fileops.InstallSiblingTempFile(temporaryPath, safeLocalPath, fileops.WorkspaceFileMode); err != nil { return fmt.Errorf("install file atomically: %w", err) } @@ -155,14 +153,42 @@ func isRestoreManifest(relativePath string) bool { return clean == config.PathManifestFile || clean == config.PathPreviousDirSegment+"/"+config.PathManifestFile } -func verifyRestoredObject(ctx context.Context, store storage.ObjectStore, action RestoreAction, temporary *fileops.DownloadedTempFile) error { +func downloadRestoreTemp(destination string, download func(io.Writer) error) (string, error) { + if strings.TrimSpace(destination) == "" { + return "", fmt.Errorf("restore destination path is required") + } + if download == nil { + return "", fmt.Errorf("restore temporary writer is required") + } + directory := filepath.Dir(destination) + if err := fileops.EnsureWorkspaceDirectory(directory); err != nil { + return "", fmt.Errorf("create destination directory: %w", err) + } + temporary, err := os.CreateTemp(directory, "."+filepath.Base(destination)+".tmp-*") + if err != nil { + return "", fmt.Errorf("create sibling temporary file: %w", err) + } + temporaryPath := temporary.Name() + if err := download(temporary); err != nil { + _ = temporary.Close() + _ = os.Remove(temporaryPath) + return "", err + } + if err := temporary.Close(); err != nil { + _ = os.Remove(temporaryPath) + return "", fmt.Errorf("close sibling temporary file: %w", err) + } + return temporaryPath, nil +} + +func verifyRestoredObject(ctx context.Context, store storage.ObjectStore, action RestoreAction, temporaryPath string) error { if strings.TrimSpace(action.SHA256) == "" && strings.TrimSpace(action.Generation) == "" { return nil } if strings.TrimSpace(action.SHA256) == "" || strings.TrimSpace(action.Generation) == "" { return fmt.Errorf("committed object identity for %q is incomplete", action.RemoteKey) } - file, err := temporary.Open() + file, err := os.Open(temporaryPath) if err != nil { return fmt.Errorf("open downloaded object for verification: %w", err) } diff --git a/internal/audio/cache_identity.go b/internal/audio/cache_identity.go index 512e4e8..d6cc756 100644 --- a/internal/audio/cache_identity.go +++ b/internal/audio/cache_identity.go @@ -114,22 +114,25 @@ func materializeVerifiedCachedAudio(ctx context.Context, req S3MaterializeReques if err := fileops.EnsureWorkspaceDirectory(filepath.Dir(req.DestPath)); err != nil { return "", false, fmt.Errorf("create audio destination directory: %w", err) } - temporary, err := fileops.DownloadToSiblingTemp(req.DestPath, func(destination io.Writer) error { + temporaryPath, err := writeAudioTemp(req.DestPath, func(destination io.Writer) error { _, err := io.Copy(destination, source) return err }) if err != nil { return "", false, fmt.Errorf("copy verified cached audio: %w", err) } - defer func() { _ = temporary.Cleanup() }() - checksum, size, err := temporaryChecksum(temporary) + defer func() { _ = os.Remove(temporaryPath) }() + checksum, size, err := checksumAudioTemp(temporaryPath) if err != nil { return "", false, err } + if err := validateLocalAudio(temporaryPath, identity.Size); err != nil { + return "", false, nil + } if size != identity.Size || checksum != identity.SHA256 { return "", false, nil } - if err := temporary.Install(filepath.Base(req.DestPath), fileops.WorkspaceFileMode); err != nil { + if err := fileops.InstallSiblingTempFile(temporaryPath, req.DestPath, fileops.WorkspaceFileMode); err != nil { return "", false, fmt.Errorf("install verified cached audio: %w", err) } return checksum, true, nil @@ -147,8 +150,8 @@ func cacheRelativePath(cacheRoot, cachePath string) (string, error) { return normalized, nil } -func temporaryChecksum(temporary *fileops.DownloadedTempFile) (string, int64, error) { - file, err := temporary.Open() +func checksumAudioTemp(path string) (string, int64, error) { + file, err := os.Open(path) if err != nil { return "", 0, fmt.Errorf("open cached audio copy: %w", err) } diff --git a/internal/audio/s3_audio.go b/internal/audio/s3_audio.go index 831aa36..4221178 100644 --- a/internal/audio/s3_audio.go +++ b/internal/audio/s3_audio.go @@ -70,12 +70,9 @@ func MaterializeS3Audio(ctx context.Context, req S3MaterializeRequest) (S3Materi if spoolPath == "" { return S3MaterializeResult{}, fmt.Errorf("spool path is required for s3 audio download") } - if err := downloadObjectAtomic(ctx, req.Store, key, spoolPath); err != nil { + if err := downloadObjectAtomic(ctx, req.Store, key, spoolPath, req.Object.Size); err != nil { return S3MaterializeResult{}, fmt.Errorf("download s3 audio object %q: %w", key, err) } - if err := validateLocalAudio(spoolPath, req.Object.Size); err != nil { - return S3MaterializeResult{}, fmt.Errorf("validate downloaded audio %q: %w", spoolPath, err) - } checksum, err := fileops.CopyFileAtomicWithChecksum(spoolPath, req.DestPath, fileops.WorkspaceFileMode) if err != nil { @@ -118,15 +115,47 @@ func validateLocalAudio(path string, expectedSize int64) error { return nil } -func downloadObjectAtomic(ctx context.Context, store storage.ObjectStore, key, destPath string) error { - if strings.TrimSpace(destPath) == "" { - return fmt.Errorf("destination path is required") - } - dir := filepath.Dir(destPath) - if err := fileops.EnsureWorkspaceDirectory(dir); err != nil { - return fmt.Errorf("create destination directory: %w", err) - } - return fileops.DownloadAndInstall(destPath, fileops.WorkspaceFileMode, func(destination io.Writer) error { +func downloadObjectAtomic(ctx context.Context, store storage.ObjectStore, key, destPath string, expectedSize int64) error { + temporaryPath, err := writeAudioTemp(destPath, func(destination io.Writer) error { return storage.DownloadTo(ctx, store, key, destination) }) + if err != nil { + return err + } + defer func() { _ = os.Remove(temporaryPath) }() + if err := validateLocalAudio(temporaryPath, expectedSize); err != nil { + return fmt.Errorf("validate downloaded audio: %w", err) + } + if err := fileops.InstallSiblingTempFile(temporaryPath, destPath, fileops.WorkspaceFileMode); err != nil { + return err + } + return nil +} + +func writeAudioTemp(destination string, write func(io.Writer) error) (string, error) { + if strings.TrimSpace(destination) == "" { + return "", fmt.Errorf("destination path is required") + } + if write == nil { + return "", fmt.Errorf("temporary audio writer is required") + } + dir := filepath.Dir(destination) + if err := fileops.EnsureWorkspaceDirectory(dir); err != nil { + return "", fmt.Errorf("create destination directory: %w", err) + } + temporary, err := os.CreateTemp(dir, "."+filepath.Base(destination)+".tmp-*") + if err != nil { + return "", fmt.Errorf("create sibling temporary file: %w", err) + } + temporaryPath := temporary.Name() + if err := write(temporary); err != nil { + _ = temporary.Close() + _ = os.Remove(temporaryPath) + return "", fmt.Errorf("write sibling temporary file: %w", err) + } + if err := temporary.Close(); err != nil { + _ = os.Remove(temporaryPath) + return "", fmt.Errorf("close sibling temporary file: %w", err) + } + return temporaryPath, nil } diff --git a/internal/audio/s3_audio_test.go b/internal/audio/s3_audio_test.go index 7a3a281..9d7b1d5 100644 --- a/internal/audio/s3_audio_test.go +++ b/internal/audio/s3_audio_test.go @@ -288,6 +288,30 @@ func TestMaterializeS3AudioDownloadFailureLeavesDestinationMissing(t *testing.T) assertMissing(t, req.DestPath) } +func TestMaterializeS3AudioRejectsSizeMismatchBeforeReplacingSpool(t *testing.T) { + root := t.TempDir() + fake := &storage.FakeBackend{} + key := "dnd/campaigns/forsaken/sessions/2026-04-19/audio/alice.flac" + fake.SeedObject(storage.FakeObject{Key: key, Data: []byte("audio")}) + req := testMaterializeRequest(t, root, fake, key, int64(len("audio")+1)) + writeAudioTestFile(t, req.SpoolPath, "existing-audio") + + _, err := MaterializeS3Audio(context.Background(), req) + if err == nil || !strings.Contains(err.Error(), "size") { + t.Fatalf("error = %v, want size validation failure", err) + } + assertFileEquals(t, req.SpoolPath, "existing-audio") + entries, err := os.ReadDir(filepath.Dir(req.SpoolPath)) + if err != nil { + t.Fatalf("read spool directory: %v", err) + } + for _, entry := range entries { + if strings.HasPrefix(entry.Name(), ".alice.flac.tmp-") { + t.Fatalf("unexpected temporary file residue: %q", entry.Name()) + } + } +} + func testMaterializeRequest(t *testing.T, root string, fake *storage.FakeBackend, key string, size int64) S3MaterializeRequest { t.Helper() return S3MaterializeRequest{ diff --git a/internal/fileops/confined.go b/internal/fileops/confined.go index 7815a92..0d4aa2e 100644 --- a/internal/fileops/confined.go +++ b/internal/fileops/confined.go @@ -5,7 +5,6 @@ import ( "encoding/hex" "errors" "fmt" - "io" "os" "path/filepath" "strings" @@ -325,232 +324,3 @@ func validateOpenedConfinedFile(parent *os.Root, name string, file *os.File) (*o } 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 { - parent *os.Root - name string -} - -// DownloadToSiblingTemp writes a remote object into a sibling temporary file. -// The caller supplies transport behavior through download and retains object -// identity and validation policy. The destination parent must already exist. -func DownloadToSiblingTemp(destination string, download func(io.Writer) error) (*DownloadedTempFile, error) { - if download == nil { - return nil, fmt.Errorf("download function is required") - } - parent, name, err := openConfinedParent(destination, false, 0) - if err != nil { - return nil, err - } - file, tempName, err := createSiblingTemp(parent, name) - if err != nil { - _ = parent.Close() - return nil, fmt.Errorf("create sibling temporary file: %w", err) - } - if err := download(file); err != nil { - _ = file.Close() - _ = parent.Remove(tempName) - _ = parent.Close() - return nil, err - } - if err := file.Sync(); err != nil { - _ = file.Close() - _ = parent.Remove(tempName) - _ = parent.Close() - return nil, fmt.Errorf("sync downloaded temporary file: %w", err) - } - if err := file.Close(); err != nil { - _ = parent.Remove(tempName) - _ = parent.Close() - return nil, fmt.Errorf("close downloaded temporary file: %w", err) - } - return &DownloadedTempFile{parent: parent, name: tempName}, nil -} - -// Open returns a read handle for validation before installation. -func (f *DownloadedTempFile) Open() (*os.File, error) { - if f == nil || f.parent == nil || f.name == "" { - return nil, fmt.Errorf("downloaded temporary file is unavailable") - } - return f.parent.Open(f.name) -} - -// Install atomically replaces destinationName in the opened parent directory. -func (f *DownloadedTempFile) Install(destinationName string, mode os.FileMode) error { - if f == nil || f.parent == nil || f.name == "" { - return fmt.Errorf("downloaded temporary file is unavailable") - } - if filepath.Base(destinationName) != destinationName || destinationName == "." || destinationName == "" { - return fmt.Errorf("destination file name is required") - } - file, err := f.parent.Open(f.name) - if err != nil { - return fmt.Errorf("open downloaded temporary file: %w", err) - } - if err := file.Chmod(mode); err != nil { - _ = file.Close() - return fmt.Errorf("set downloaded temporary file permissions: %w", err) - } - if err := file.Sync(); err != nil { - _ = file.Close() - return fmt.Errorf("sync downloaded temporary file: %w", err) - } - if err := file.Close(); err != nil { - return fmt.Errorf("close downloaded temporary file: %w", err) - } - if err := f.parent.Rename(f.name, destinationName); err != nil { - return fmt.Errorf("install downloaded temporary file: %w", err) - } - f.name = "" - if err := syncOpenedDirectory(f.parent); err != nil { - return fmt.Errorf("sync destination directory after downloaded-file replacement: %w", err) - } - return f.closeParent() -} - -// Cleanup removes an uninstalled temporary file. It is safe to call repeatedly. -func (f *DownloadedTempFile) Cleanup() error { - if f == nil || f.parent == nil { - return nil - } - var err error - if f.name != "" { - err = f.parent.Remove(f.name) - if errors.Is(err, os.ErrNotExist) { - err = nil - } - f.name = "" - } - closeErr := f.closeParent() - if err != nil && closeErr != nil { - return errors.Join(err, closeErr) - } - if err != nil { - return err - } - return closeErr -} - -func (f *DownloadedTempFile) closeParent() error { - if f.parent == nil { - return nil - } - err := f.parent.Close() - f.parent = nil - return err -} - -// DownloadAndInstall obtains remote bytes through a confined sibling temporary -// file and installs them with the caller-selected final mode. -func DownloadAndInstall(destination string, mode os.FileMode, download func(io.Writer) error) error { - temporary, err := DownloadToSiblingTemp(destination, download) - if err != nil { - return err - } - defer func() { _ = temporary.Cleanup() }() - return temporary.Install(filepath.Base(destination), mode) -} - -func replaceFileFromReaderConfined(dst string, source io.Reader, options ReplaceFileOptions) (resultErr error) { - parent, name, err := openConfinedParent(dst, false, 0) - if err != nil { - return err - } - defer func() { _ = parent.Close() }() - temporary, tempName, err := createSiblingTemp(parent, name) - if err != nil { - return fmt.Errorf("create temp file: %w", err) - } - removeTemp := true - defer func() { - if removeTemp { - if err := parent.Remove(tempName); err != nil && !errors.Is(err, os.ErrNotExist) { - resultErr = errors.Join(resultErr, fmt.Errorf("remove temporary file: %w", err)) - } - } - }() - - if _, err := io.Copy(temporary, source); err != nil { - return closeTemporaryFileAfterError(temporary, fmt.Errorf("write temp file: %w", err)) - } - if err := temporary.Chmod(options.Mode); err != nil { - return closeTemporaryFileAfterError(temporary, fmt.Errorf("set temp file permissions: %w", err)) - } - if err := temporary.Sync(); err != nil { - return closeTemporaryFileAfterError(temporary, fmt.Errorf("sync temp file: %w", err)) - } - if err := temporary.Close(); err != nil { - return fmt.Errorf("close temp file: %w", err) - } - if options.BeforeRename != nil { - if err := options.BeforeRename(); err != nil { - return fmt.Errorf("check before file replacement: %w", err) - } - } - if err := parent.Rename(tempName, name); err != nil { - return fmt.Errorf("install temp file: %w", err) - } - removeTemp = false - if err := syncOpenedDirectory(parent); err != nil { - return fmt.Errorf("sync destination directory after replacement: %w", err) - } - return nil -} - -func installDownloadedTempFileConfined(tmpPath, dst string, mode os.FileMode) error { - if filepath.Clean(filepath.Dir(tmpPath)) != filepath.Clean(filepath.Dir(dst)) { - source, err := os.Open(tmpPath) - if err != nil { - return fmt.Errorf("open downloaded temporary file: %w", err) - } - err = replaceFileFromReaderConfined(dst, source, ReplaceFileOptions{Mode: mode}) - closeErr := source.Close() - if err != nil { - return fmt.Errorf("install downloaded file: %w", err) - } - if closeErr != nil { - return fmt.Errorf("close downloaded temporary file: %w", closeErr) - } - if err := os.Remove(tmpPath); err != nil { - return fmt.Errorf("remove installed temporary file: %w", err) - } - return nil - } - parent, destinationName, err := openConfinedParent(dst, false, 0) - if err != nil { - return err - } - defer func() { _ = parent.Close() }() - temporaryName := filepath.Base(tmpPath) - info, err := parent.Lstat(temporaryName) - if err != nil { - return fmt.Errorf("inspect downloaded temporary file: %w", err) - } - if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() { - return fmt.Errorf("downloaded temporary file is not a regular file") - } - file, err := parent.Open(temporaryName) - if err != nil { - return fmt.Errorf("open downloaded temporary file: %w", err) - } - if err := file.Chmod(mode); err != nil { - _ = file.Close() - return fmt.Errorf("set temp file permissions: %w", err) - } - if err := file.Sync(); err != nil { - _ = file.Close() - return fmt.Errorf("sync downloaded temp file: %w", err) - } - if err := file.Close(); err != nil { - return fmt.Errorf("close downloaded temp file: %w", err) - } - if err := parent.Rename(temporaryName, destinationName); err != nil { - return fmt.Errorf("install downloaded file: %w", err) - } - if err := syncOpenedDirectory(parent); err != nil { - return fmt.Errorf("sync destination directory after downloaded-file replacement: %w", err) - } - return nil -} diff --git a/internal/fileops/fileops.go b/internal/fileops/fileops.go index 7e87d57..8e35ef3 100644 --- a/internal/fileops/fileops.go +++ b/internal/fileops/fileops.go @@ -12,8 +12,8 @@ import ( "strings" ) -// ErrDirectorySyncUnsupported remains for legacy confined migration paths. -// The retained replacement entry points do not perform directory syncing. +// ErrDirectorySyncUnsupported is used only by the retained directory-promotion +// implementation. Single-file replacement does not perform directory syncing. var ErrDirectorySyncUnsupported = errors.New("directory sync is unsupported") // ReplaceFileOptions describes the caller-owned parts of a byte-file @@ -131,147 +131,21 @@ func replaceFileFromReader(dst string, source io.Reader, options ReplaceFileOpti return nil } -// InstallDownloadedTempFile installs a fully downloaded sibling temporary file -// at dst. On failure before the rename, the caller retains ownership of tmpPath -// and is responsible for cleanup. -func InstallDownloadedTempFile(tmpPath, dst string, perm os.FileMode) error { +// InstallSiblingTempFile installs a fully written and validated sibling +// temporary file at dst. The caller retains ownership of tmpPath if installation +// fails and is responsible for removing it. +func InstallSiblingTempFile(tmpPath, dst string, perm os.FileMode) error { if strings.TrimSpace(tmpPath) == "" || strings.TrimSpace(dst) == "" { return fmt.Errorf("temp and destination paths are required") } - if err := EnsureWorkspaceDirectory(filepath.Dir(dst)); err != nil { - return fmt.Errorf("create destination directory: %w", err) + if filepath.Clean(filepath.Dir(tmpPath)) != filepath.Clean(filepath.Dir(dst)) { + return fmt.Errorf("temporary file must be a sibling of the destination") } - return installDownloadedTempFileConfined(tmpPath, dst, perm) -} - -type temporaryFile interface { - io.Writer - Name() string - Sync() error - Close() error -} - -type atomicReplacementOperations struct { - createTemp func(string, string) (temporaryFile, error) - chmod func(string, os.FileMode) error - rename func(string, string) error - remove func(string) error - syncDirectory func(string) error - syncFile func(string) error -} - -var systemAtomicReplacementOperations = atomicReplacementOperations{ - createTemp: func(dir, pattern string) (temporaryFile, error) { - return os.CreateTemp(dir, pattern) - }, - chmod: os.Chmod, - rename: os.Rename, - remove: os.Remove, - syncDirectory: syncDirectory, - syncFile: syncFile, -} - -func replaceFileAtomicWithOperations( - dst string, - data []byte, - options ReplaceFileOptions, - ops atomicReplacementOperations, -) error { - return replaceFileFromReaderWithOperations(dst, bytes.NewReader(data), options, ops) -} - -func replaceFileFromReaderWithOperations( - dst string, - source io.Reader, - options ReplaceFileOptions, - ops atomicReplacementOperations, -) (resultErr error) { - if strings.TrimSpace(dst) == "" { - return fmt.Errorf("destination path is required") - } - - dir := filepath.Dir(dst) - tmp, err := ops.createTemp(dir, "."+filepath.Base(dst)+".tmp-*") - if err != nil { - return fmt.Errorf("create temp file: %w", err) - } - tmpPath := tmp.Name() - removeTmp := true - defer func() { - if removeTmp { - if err := ops.remove(tmpPath); err != nil { - cleanupErr := fmt.Errorf("remove temporary file: %w", err) - if resultErr == nil { - resultErr = cleanupErr - } else { - resultErr = errors.Join(resultErr, cleanupErr) - } - } - } - }() - - if _, err := io.Copy(tmp, source); err != nil { - return closeTemporaryFileAfterError(tmp, fmt.Errorf("write temp file: %w", err)) - } - if err := ops.chmod(tmpPath, options.Mode); err != nil { - return closeTemporaryFileAfterError(tmp, fmt.Errorf("set temp file permissions: %w", err)) - } - if err := tmp.Sync(); err != nil { - return closeTemporaryFileAfterError(tmp, fmt.Errorf("sync temp file: %w", err)) - } - if err := tmp.Close(); err != nil { - return fmt.Errorf("close temp file: %w", err) - } - if options.BeforeRename != nil { - if err := options.BeforeRename(); err != nil { - return fmt.Errorf("check before file replacement: %w", err) - } - } - if err := ops.rename(tmpPath, dst); err != nil { - return fmt.Errorf("install temp file: %w", err) - } - removeTmp = false - if err := ops.syncDirectory(dir); err != nil { - return fmt.Errorf("sync destination directory after replacement: %w", err) - } - return nil -} - -func closeTemporaryFileAfterError(tmp temporaryFile, operationErr error) error { - if err := tmp.Close(); err != nil { - return errors.Join(operationErr, fmt.Errorf("close temporary file: %w", err)) - } - return operationErr -} - -func installDownloadedTempFileWithOperations( - tmpPath, dst string, - perm os.FileMode, - ops atomicReplacementOperations, -) error { - if err := ops.chmod(tmpPath, perm); err != nil { + if err := os.Chmod(tmpPath, perm); err != nil { return fmt.Errorf("set temp file permissions: %w", err) } - if err := ops.syncFile(tmpPath); err != nil { - return fmt.Errorf("sync downloaded temp file: %w", err) - } - if err := ops.rename(tmpPath, dst); err != nil { + if err := os.Rename(tmpPath, dst); err != nil { return fmt.Errorf("install downloaded file: %w", err) } - if err := ops.syncDirectory(filepath.Dir(dst)); err != nil { - return fmt.Errorf("sync destination directory after downloaded-file replacement: %w", err) - } return nil } - -func syncFile(path string) error { - file, err := os.Open(path) - if err != nil { - return err - } - if err := file.Sync(); err != nil { - _ = file.Close() - return err - } - return file.Close() -} diff --git a/internal/fileops/fileops_test.go b/internal/fileops/fileops_test.go index b5e03d8..8a02f59 100644 --- a/internal/fileops/fileops_test.go +++ b/internal/fileops/fileops_test.go @@ -3,16 +3,12 @@ package fileops import ( "crypto/sha256" "encoding/hex" - "errors" - "io" "os" "path/filepath" "strings" "testing" ) -var errAtomicReplacementTest = errors.New("atomic replacement test failure") - func TestWriteFileAtomicOverwritesAndLeavesNoTempFile(t *testing.T) { root := t.TempDir() dst := filepath.Join(root, "out", "value.txt") @@ -116,16 +112,16 @@ func TestCopyFileAtomicCleansTempFileOnInstallFailure(t *testing.T) { assertNoMatchingTempFiles(t, root, ".blocked.tmp-") } -func TestInstallDownloadedTempFileSetsPermissions(t *testing.T) { +func TestInstallSiblingTempFileSetsPermissions(t *testing.T) { root := t.TempDir() + dst := filepath.Join(root, "payload.json") tmpPath := filepath.Join(root, ".payload.tmp") - dst := filepath.Join(root, "out", "payload.json") if err := os.WriteFile(tmpPath, []byte("{\"ok\":true}\n"), 0o600); err != nil { t.Fatalf("WriteFile(temp) error = %v", err) } - if err := InstallDownloadedTempFile(tmpPath, dst, 0o644); err != nil { - t.Fatalf("InstallDownloadedTempFile() error = %v", err) + if err := InstallSiblingTempFile(tmpPath, dst, 0o644); err != nil { + t.Fatalf("InstallSiblingTempFile() error = %v", err) } if _, err := os.Stat(tmpPath); !os.IsNotExist(err) { t.Fatalf("temp file still exists: stat err = %v", err) @@ -186,253 +182,6 @@ func TestWriteFileAtomicReplacesLeafSymlinkWithoutFollowingIt(t *testing.T) { } } -func TestDownloadAndInstallRejectsSymlinkedDestinationAncestor(t *testing.T) { - root := t.TempDir() - outside := t.TempDir() - if err := os.Symlink(outside, filepath.Join(root, "redirect")); err != nil { - t.Skipf("Symlink unavailable: %v", err) - } - - err := DownloadAndInstall(filepath.Join(root, "redirect", "output.txt"), 0o640, func(io.Writer) error { - return nil - }) - if err == nil { - t.Fatal("DownloadAndInstall() error = nil, want symlink ancestor rejection") - } - entries, err := os.ReadDir(outside) - if err != nil { - t.Fatalf("ReadDir(outside) error = %v", err) - } - if len(entries) != 0 { - t.Fatalf("outside destination received entries: %v", entries) - } -} - -func TestReplaceFileAtomicOrdersDurableOperations(t *testing.T) { - events := make([]string, 0, 8) - tmp := &recordingTemporaryFile{name: "/work/.result.tmp-1", events: &events} - ops := recordingAtomicReplacementOperations(&events, tmp) - - err := replaceFileAtomicWithOperations( - "/work/result", - []byte("replacement"), - ReplaceFileOptions{ - Mode: 0o640, - BeforeRename: func() error { - events = append(events, "check") - return nil - }, - }, - ops, - ) - if err != nil { - t.Fatalf("replaceFileAtomicWithOperations() error = %v", err) - } - - want := []string{"create", "write", "chmod:rw-r-----", "sync", "close", "check", "rename", "sync-directory"} - if !equalStrings(events, want) { - t.Fatalf("operation order = %v, want %v", events, want) - } -} - -func TestReplaceFileAtomicFailuresCleanUninstalledTemporaryFile(t *testing.T) { - tests := []struct { - name string - configure func(*recordingTemporaryFile, *atomicReplacementOperations) - wantErr error - wantRemove bool - }{ - { - name: "short write", - configure: func(tmp *recordingTemporaryFile, _ *atomicReplacementOperations) { - tmp.shortWrite = true - }, - wantErr: io.ErrShortWrite, - wantRemove: true, - }, - { - name: "write", - configure: func(tmp *recordingTemporaryFile, _ *atomicReplacementOperations) { - tmp.writeErr = errAtomicReplacementTest - }, - wantErr: errAtomicReplacementTest, - wantRemove: true, - }, - { - name: "sync", - configure: func(tmp *recordingTemporaryFile, _ *atomicReplacementOperations) { - tmp.syncErr = errAtomicReplacementTest - }, - wantErr: errAtomicReplacementTest, - wantRemove: true, - }, - { - name: "rename", - configure: func(_ *recordingTemporaryFile, ops *atomicReplacementOperations) { - ops.rename = func(string, string) error { return errAtomicReplacementTest } - }, - wantErr: errAtomicReplacementTest, - wantRemove: true, - }, - { - name: "directory sync", - configure: func(_ *recordingTemporaryFile, ops *atomicReplacementOperations) { - ops.syncDirectory = func(string) error { return ErrDirectorySyncUnsupported } - }, - wantErr: ErrDirectorySyncUnsupported, - wantRemove: false, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - events := make([]string, 0, 8) - tmp := &recordingTemporaryFile{name: "/work/.result.tmp-1", events: &events} - ops := recordingAtomicReplacementOperations(&events, tmp) - test.configure(tmp, &ops) - - err := replaceFileAtomicWithOperations("/work/result", []byte("replacement"), ReplaceFileOptions{Mode: 0o640}, ops) - if !errors.Is(err, test.wantErr) { - t.Fatalf("replaceFileAtomicWithOperations() error = %v, want %v", err, test.wantErr) - } - gotRemove := containsString(events, "remove") - if gotRemove != test.wantRemove { - t.Fatalf("remove temporary file = %t, want %t; operations = %v", gotRemove, test.wantRemove, events) - } - }) - } -} - -func TestReplaceFileAtomicReportsCleanupFailureWithPrimaryFailure(t *testing.T) { - events := make([]string, 0, 5) - tmp := &recordingTemporaryFile{ - name: "/work/.result.tmp-1", - events: &events, - writeErr: errAtomicReplacementTest, - } - ops := recordingAtomicReplacementOperations(&events, tmp) - cleanupErr := errors.New("temporary cleanup failed") - ops.remove = func(string) error { - events = append(events, "remove") - return cleanupErr - } - - err := replaceFileAtomicWithOperations("/work/result", []byte("replacement"), ReplaceFileOptions{Mode: 0o640}, ops) - if !errors.Is(err, errAtomicReplacementTest) { - t.Fatalf("replacement error = %v, want write failure", err) - } - if !errors.Is(err, cleanupErr) { - t.Fatalf("replacement error = %v, want cleanup failure", err) - } -} - -func TestInstallDownloadedTempFileOrdersDurableOperations(t *testing.T) { - events := make([]string, 0, 4) - ops := atomicReplacementOperations{ - chmod: func(string, os.FileMode) error { - events = append(events, "chmod") - return nil - }, - syncFile: func(string) error { - events = append(events, "sync-file") - return nil - }, - rename: func(string, string) error { - events = append(events, "rename") - return nil - }, - syncDirectory: func(string) error { - events = append(events, "sync-directory") - return nil - }, - } - - if err := installDownloadedTempFileWithOperations("/work/.download.tmp", "/work/result", 0o640, ops); err != nil { - t.Fatalf("installDownloadedTempFileWithOperations() error = %v", err) - } - want := []string{"chmod", "sync-file", "rename", "sync-directory"} - if !equalStrings(events, want) { - t.Fatalf("operation order = %v, want %v", events, want) - } -} - -type recordingTemporaryFile struct { - name string - events *[]string - shortWrite bool - writeErr error - syncErr error -} - -func (f *recordingTemporaryFile) Write(data []byte) (int, error) { - *f.events = append(*f.events, "write") - if f.writeErr != nil { - return 0, f.writeErr - } - if f.shortWrite { - return len(data) - 1, nil - } - return len(data), nil -} - -func (f *recordingTemporaryFile) Name() string { return f.name } - -func (f *recordingTemporaryFile) Sync() error { - *f.events = append(*f.events, "sync") - return f.syncErr -} - -func (f *recordingTemporaryFile) Close() error { - *f.events = append(*f.events, "close") - return nil -} - -func recordingAtomicReplacementOperations(events *[]string, tmp temporaryFile) atomicReplacementOperations { - return atomicReplacementOperations{ - createTemp: func(string, string) (temporaryFile, error) { - *events = append(*events, "create") - return tmp, nil - }, - chmod: func(_ string, mode os.FileMode) error { - *events = append(*events, "chmod:"+mode.Perm().String()[1:]) - return nil - }, - rename: func(string, string) error { - *events = append(*events, "rename") - return nil - }, - remove: func(string) error { - *events = append(*events, "remove") - return nil - }, - syncDirectory: func(string) error { - *events = append(*events, "sync-directory") - return nil - }, - } -} - -func containsString(values []string, target string) bool { - for _, value := range values { - if value == target { - return true - } - } - return false -} - -func equalStrings(got, want []string) bool { - if len(got) != len(want) { - return false - } - for i := range got { - if got[i] != want[i] { - return false - } - } - return true -} - func assertNoMatchingTempFiles(t *testing.T, dir, prefix string) { t.Helper() entries, err := os.ReadDir(dir) diff --git a/internal/stage/prepare_previous.go b/internal/stage/prepare_previous.go index 02e551f..09edeb3 100644 --- a/internal/stage/prepare_previous.go +++ b/internal/stage/prepare_previous.go @@ -5,8 +5,10 @@ import ( "context" "fmt" "io" + "os" "path/filepath" "sort" + "strings" "gitea.maximumdirect.net/eric/narratio/internal/adapters/storage" "gitea.maximumdirect.net/eric/narratio/internal/artifacts" @@ -58,23 +60,23 @@ func hydratePreviousSessionArtifacts( return nil, fmt.Errorf("create previous-session path directory for %q: %w", record.LocalPath, err) } - if err := fileops.DownloadAndInstall(record.LocalPath, fileops.WorkspaceFileMode, func(destination io.Writer) error { + temporaryPath, err := writePreviousTemp(record.LocalPath, func(destination io.Writer) error { if len(record.VerifiedContent) > 0 { _, err := io.Copy(destination, bytes.NewReader(record.VerifiedContent)) return err } return storage.DownloadTo(ctx, env.ObjectStore, record.RemoteKey, destination) - }); err != nil { + }) + if err != nil { + return nil, fmt.Errorf("write previous-session object %q: %w", record.RemoteKey, err) + } + defer func() { _ = os.Remove(temporaryPath) }() + checksum, err := validatePreviousTemp(record, temporaryPath) + if err != nil { return nil, err } - if record.Kind == preparePreviousInputKindArtifact { - if err := requireNonEmptyFile(record.LocalPath, "previous-session artifact "+record.RequirementName); err != nil { - return nil, err - } - } - checksum, err := env.ArtifactStore.Checksum(record.LocalPath) - if err != nil { - return nil, fmt.Errorf("checksum previous-session object %q: %w", record.RemoteKey, err) + if err := fileops.InstallSiblingTempFile(temporaryPath, record.LocalPath, fileops.WorkspaceFileMode); err != nil { + return nil, fmt.Errorf("install previous-session object %q: %w", record.RemoteKey, err) } result.Inputs = append(result.Inputs, manifest.InputRecord{ @@ -100,3 +102,55 @@ func hydratePreviousSessionArtifacts( }) return result, nil } + +func writePreviousTemp(destination string, write func(io.Writer) error) (string, error) { + if strings.TrimSpace(destination) == "" { + return "", fmt.Errorf("previous-session destination path is required") + } + if write == nil { + return "", fmt.Errorf("previous-session temporary writer is required") + } + directory := filepath.Dir(destination) + if err := fileops.EnsureWorkspaceDirectory(directory); err != nil { + return "", fmt.Errorf("create previous-session path directory: %w", err) + } + temporary, err := os.CreateTemp(directory, "."+filepath.Base(destination)+".tmp-*") + if err != nil { + return "", fmt.Errorf("create sibling temporary file: %w", err) + } + temporaryPath := temporary.Name() + if err := write(temporary); err != nil { + _ = temporary.Close() + _ = os.Remove(temporaryPath) + return "", err + } + if err := temporary.Close(); err != nil { + _ = os.Remove(temporaryPath) + return "", fmt.Errorf("close sibling temporary file: %w", err) + } + return temporaryPath, nil +} + +func validatePreviousTemp(record previouscache.Record, temporaryPath string) (string, error) { + info, err := os.Stat(temporaryPath) + if err != nil { + return "", fmt.Errorf("inspect previous-session object %q: %w", record.RemoteKey, err) + } + hasDeclaredSize := record.Size > 0 || strings.TrimSpace(record.SHA256) != "" || strings.TrimSpace(record.Generation) != "" + if hasDeclaredSize && info.Size() != record.Size { + return "", fmt.Errorf("previous-session object %q size mismatch: got %d, want %d", record.RemoteKey, info.Size(), record.Size) + } + if record.Kind == preparePreviousInputKindArtifact { + if err := requireNonEmptyFile(temporaryPath, "previous-session artifact "+record.RequirementName); err != nil { + return "", err + } + } + checksum, err := artifacts.SHA256File(temporaryPath) + if err != nil { + return "", fmt.Errorf("checksum previous-session object %q: %w", record.RemoteKey, err) + } + if expected := strings.TrimSpace(record.SHA256); expected != "" && !strings.EqualFold(checksum, expected) { + return "", fmt.Errorf("previous-session object %q checksum mismatch", record.RemoteKey) + } + return checksum, nil +}