diff --git a/docs/internal/fileops.md b/docs/internal/fileops.md index 0dc6936..0c8b61d 100644 --- a/docs/internal/fileops.md +++ b/docs/internal/fileops.md @@ -1,17 +1,13 @@ # Internal: File Operations -`internal/fileops` owns the narrow mechanics for durable replacement of one +`internal/fileops` owns the narrow mechanics for atomically visible replacement of one byte file. Callers keep ownership of serialization, validation, cancellation, and destination-directory policy. -## Destination Confinement +## Destination Paths -Before it creates, replaces, or installs a destination file, `fileops` opens -each ancestor from the filesystem root and rejects symbolic links or components -that change during traversal. The resulting parent-directory handle is retained -for sibling temporary-file creation and rename, so a later pathname swap cannot -redirect the replacement. Existing destination symlinks are replaced as leaf -entries; their targets are never followed. +Ordinary workspace replacement uses standard operating-system path semantics. +Callers with a root-scoped safety contract own that validation at their boundary. Remote object acquisition uses a writer supplied by the storage owner. The writer receives a `fileops`-owned, already-open sibling temporary file rather @@ -31,41 +27,28 @@ 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`. -## Confined Reads +## Bounded Reads -`ReadRegularFileUnderRoot` is the no-follow, bounded read primitive for a -caller-selected root and relative file path; `ReadRegularFile` is its -path-based convenience wrapper. They verify every ancestor through directory -handles and admit only a stable regular-file handle. Callers enforce their own -byte limits and access policy. Credential mode policy and environment -precedence remain owned by `internal/app`. +`ReadFileBounded` opens a normal path and reads at most its caller-supplied +limit. Credential mode policy and environment precedence remain owned by +`internal/app`. ## Replacement Contract `ReplaceFileAtomic` requires an existing destination directory. It creates a sibling temporary file, writes the complete byte sequence, applies the -caller-supplied mode, syncs and closes the file, runs an optional pre-rename -check, replaces the destination with a rename, then syncs the containing -directory. +caller-supplied mode, closes the file, runs an optional pre-rename check, and +replaces the destination with a rename. The pre-rename check is the last point at which a caller can cancel without installing a new destination. A failure before the rename leaves the old -destination unchanged and removes the temporary file; any cleanup failure is -returned alongside the primary failure. A failure after the rename may leave -the new file visible, but it is not reported as crash-durable. +destination unchanged and removes the temporary file. Successful replacement +provides atomic visibility; it makes no crash- or power-loss durability claim. Replacement follows the operating system's same-filesystem rename semantics. If a platform cannot replace an existing destination, the operation returns an error and never removes the old file as an emulation step. -## Directory-Sync Support - -Linux and macOS attempt to sync the destination directory. Windows opens the -directory with backup semantics and flushes its buffers. If either operation -is unavailable for the platform, directory handle, or filesystem, -`ErrDirectorySyncUnsupported` is returned. Narratio does not treat that result -as successful crash-durable replacement. - `WriteFileAtomic`, copy helpers, and downloaded temporary-file installation retain their compatibility behavior of creating the destination parent with the repository's workspace permissions before using this contract. diff --git a/docs/internal/manifest.md b/docs/internal/manifest.md index 688f1e4..0c1ffe8 100644 --- a/docs/internal/manifest.md +++ b/docs/internal/manifest.md @@ -155,14 +155,11 @@ path is removable after migration and is never used to write new state. - validates loaded documents; - normalizes missing maps/stage records; -- writes through a sibling temporary file, syncing the completed file and - destination directory after atomic replacement; +- writes through a sibling temporary file and atomic replacement visibility; - updates `updated_at` on save. -If the operating system or filesystem cannot sync a directory, save returns an -explicit error instead of claiming crash-durable replacement. A returned error -after the rename can therefore leave the new manifest visible but not confirmed -durable; callers must reload it before retrying. +Manifest replacement provides atomic visibility, not a crash- or power-loss +durability guarantee. Callers reload after a returned save error before retrying. ## Execution Semantics diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index e685fe8..2e55030 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -173,7 +173,7 @@ During every stage: ## Stage 1 — Reduce Atomic Replacement, Modes, And Bounded Reads -**Status: Pending** +**Status: Completed** ### Goal diff --git a/internal/fileops/fileops.go b/internal/fileops/fileops.go index 17b4403..7e87d57 100644 --- a/internal/fileops/fileops.go +++ b/internal/fileops/fileops.go @@ -12,9 +12,8 @@ import ( "strings" ) -// ErrDirectorySyncUnsupported reports that the platform or filesystem cannot -// make a renamed directory entry crash-durable. The replacement may be visible, -// but callers must not treat it as having the same durability guarantee. +// ErrDirectorySyncUnsupported remains for legacy confined migration paths. +// The retained replacement entry points do not perform directory syncing. var ErrDirectorySyncUnsupported = errors.New("directory sync is unsupported") // ReplaceFileOptions describes the caller-owned parts of a byte-file @@ -26,13 +25,10 @@ type ReplaceFileOptions struct { BeforeRename func() error } -// ReplaceFileAtomic durably replaces dst with data. It creates a sibling -// temporary file, writes and syncs its bytes and mode, atomically renames it, -// then syncs the destination parent. On platforms where replacement of an -// existing destination or directory syncing is unavailable, it returns an -// error rather than claiming an equivalent durability guarantee. +// ReplaceFileAtomic replaces dst with data through a sibling temporary file. +// It provides atomic visibility, not a crash- or power-loss-durability claim. func ReplaceFileAtomic(dst string, data []byte, options ReplaceFileOptions) error { - return replaceFileFromReaderConfined(dst, bytes.NewReader(data), options) + return replaceFileFromReader(dst, bytes.NewReader(data), options) } // WriteFileAtomic creates the destination parent with workspace permissions @@ -85,7 +81,7 @@ func WriteReaderAtomicWithChecksum(dst string, src io.Reader, perm os.FileMode) } digest := sha256.New() - if err := replaceFileFromReaderConfined( + if err := replaceFileFromReader( dst, io.TeeReader(src, digest), ReplaceFileOptions{Mode: perm}, @@ -95,6 +91,46 @@ func WriteReaderAtomicWithChecksum(dst string, src io.Reader, perm os.FileMode) return hex.EncodeToString(digest.Sum(nil)), nil } +func replaceFileFromReader(dst string, source io.Reader, options ReplaceFileOptions) (resultErr error) { + if strings.TrimSpace(dst) == "" { + return fmt.Errorf("destination path is required") + } + tmp, err := os.CreateTemp(filepath.Dir(dst), "."+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 := os.Remove(tmpPath); err != nil && !errors.Is(err, os.ErrNotExist) { + resultErr = errors.Join(resultErr, fmt.Errorf("remove temporary file: %w", err)) + } + } + }() + if _, err := io.Copy(tmp, source); err != nil { + _ = tmp.Close() + return fmt.Errorf("write temp file: %w", err) + } + if err := tmp.Chmod(options.Mode); err != nil { + _ = tmp.Close() + return fmt.Errorf("set temp file permissions: %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 := os.Rename(tmpPath, dst); err != nil { + return fmt.Errorf("install temp file: %w", err) + } + removeTmp = false + 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. diff --git a/internal/fileops/fileops_test.go b/internal/fileops/fileops_test.go index 0fbf70a..b5e03d8 100644 --- a/internal/fileops/fileops_test.go +++ b/internal/fileops/fileops_test.go @@ -139,30 +139,23 @@ func TestInstallDownloadedTempFileSetsPermissions(t *testing.T) { } } -func TestWriteFileAtomicRejectsSymlinkedDestinationAncestor(t *testing.T) { +func TestWriteFileAtomicUsesOrdinaryDestinationPathSemantics(t *testing.T) { root := t.TempDir() outside := t.TempDir() - sentinel := filepath.Join(outside, "sentinel.txt") - if err := os.WriteFile(sentinel, []byte("unchanged"), 0o600); err != nil { - t.Fatalf("WriteFile(sentinel) error = %v", err) - } if err := os.Symlink(outside, filepath.Join(root, "redirect")); err != nil { t.Skipf("Symlink unavailable: %v", err) } - err := WriteFileAtomic(filepath.Join(root, "redirect", "output.txt"), []byte("new"), 0o640) - if err == nil { - t.Fatal("WriteFileAtomic() error = nil, want symlink ancestor rejection") + path := filepath.Join(root, "redirect", "output.txt") + if err := WriteFileAtomic(path, []byte("new"), 0o640); err != nil { + t.Fatalf("WriteFileAtomic() error = %v", err) } - data, err := os.ReadFile(sentinel) + data, err := os.ReadFile(filepath.Join(outside, "output.txt")) if err != nil { - t.Fatalf("ReadFile(sentinel) error = %v", err) + t.Fatalf("ReadFile(output) error = %v", err) } - if string(data) != "unchanged" { - t.Fatalf("outside sentinel = %q, want unchanged", data) - } - if _, err := os.Stat(filepath.Join(outside, "output.txt")); !os.IsNotExist(err) { - t.Fatalf("outside output exists: stat err = %v", err) + if string(data) != "new" { + t.Fatalf("outside output = %q, want new", data) } } diff --git a/internal/fileops/modes.go b/internal/fileops/modes.go index 4157aa6..78dc8e7 100644 --- a/internal/fileops/modes.go +++ b/internal/fileops/modes.go @@ -3,6 +3,7 @@ package fileops import ( "fmt" "os" + "path/filepath" "strings" ) @@ -22,8 +23,38 @@ func EnsureWorkspaceDirectory(directory string) error { return fmt.Errorf("workspace directory is required") } - if err := ensureConfinedDirectory(directory, WorkspaceDirectoryMode); err != nil { - return fmt.Errorf("set workspace directory permissions %q: %w", directory, err) + clean := filepath.Clean(directory) + ancestor := clean + var missing []string + for { + info, err := os.Stat(ancestor) + if err == nil { + if !info.IsDir() { + return fmt.Errorf("workspace directory %q is not a directory", ancestor) + } + break + } + if !os.IsNotExist(err) { + return fmt.Errorf("inspect workspace directory %q: %w", ancestor, err) + } + parent := filepath.Dir(ancestor) + if parent == ancestor { + return fmt.Errorf("find workspace directory ancestor for %q", clean) + } + missing = append(missing, ancestor) + ancestor = parent + } + for index := len(missing) - 1; index >= 0; index-- { + created := missing[index] + if err := os.Mkdir(created, WorkspaceDirectoryMode); err != nil && !os.IsExist(err) { + return fmt.Errorf("create workspace directory %q: %w", created, err) + } + if err := os.Chmod(created, WorkspaceDirectoryMode); err != nil { + return fmt.Errorf("set workspace directory permissions %q: %w", created, err) + } + } + if err := os.Chmod(clean, WorkspaceDirectoryMode); err != nil { + return fmt.Errorf("set workspace directory permissions %q: %w", clean, err) } return nil } diff --git a/internal/fileops/read_bounded.go b/internal/fileops/read_bounded.go new file mode 100644 index 0000000..03bb1e3 --- /dev/null +++ b/internal/fileops/read_bounded.go @@ -0,0 +1,32 @@ +package fileops + +import ( + "fmt" + "io" + "os" + "strings" +) + +// ReadFileBounded reads at most limit bytes from path using ordinary workspace +// path semantics. It does not return partial data when the limit is exceeded. +func ReadFileBounded(path string, limit int64) ([]byte, error) { + if strings.TrimSpace(path) == "" { + return nil, fmt.Errorf("file path is required") + } + if limit < 0 { + return nil, fmt.Errorf("file read limit %d is invalid", limit) + } + file, err := os.Open(path) + if err != nil { + return nil, fmt.Errorf("open file %q: %w", path, err) + } + defer func() { _ = file.Close() }() + data, err := io.ReadAll(io.LimitReader(file, limit+1)) + if err != nil { + return nil, fmt.Errorf("read file %q: %w", path, err) + } + if int64(len(data)) > limit { + return nil, fmt.Errorf("file %q exceeds %d-byte limit", path, limit) + } + return data, nil +}