Archive v1.2 milestone: ROADMAP + REQUIREMENTS + phases to milestones/. Evolve PROJECT.md with v1.2 validated requirements and key decisions. Update RETROSPECTIVE.md with v1.2 lessons and cross-milestone trends. Clean STATE.md for next milestone.
259 lines
12 KiB
Markdown
259 lines
12 KiB
Markdown
---
|
|
phase: 15-schema-migration-write-safety
|
|
plan: 02
|
|
type: execute
|
|
wave: 1
|
|
depends_on: []
|
|
files_modified:
|
|
- backend/fileutil/atomicwrite.go
|
|
- backend/fileutil/atomicwrite_test.go
|
|
autonomous: true
|
|
requirements: [SCHEMA-02, WRITE-05]
|
|
|
|
must_haves:
|
|
truths:
|
|
- "AtomicWrite writes to a temp file then renames to target — original file is never in a half-written state"
|
|
- "Temp files use .yj-tmp suffix"
|
|
- "Cross-filesystem writes are rejected with a clear error"
|
|
- "Original file permissions are preserved on the new file"
|
|
- "Orphaned .yj-tmp files for the target path are cleaned up before writing"
|
|
- "Unit tests verify all behaviors including crash simulation"
|
|
artifacts:
|
|
- path: "backend/fileutil/atomicwrite.go"
|
|
provides: "General-purpose atomic file write utility"
|
|
exports: ["AtomicWrite"]
|
|
min_lines: 40
|
|
- path: "backend/fileutil/atomicwrite_test.go"
|
|
provides: "Comprehensive tests for atomic write"
|
|
min_lines: 80
|
|
key_links:
|
|
- from: "backend/fileutil/atomicwrite.go"
|
|
to: "os.Rename"
|
|
via: "atomic rename from temp to target"
|
|
pattern: "os\\.Rename"
|
|
- from: "backend/fileutil/atomicwrite.go"
|
|
to: "os.Stat"
|
|
via: "preserve original file permissions"
|
|
pattern: "os\\.Stat"
|
|
---
|
|
|
|
<objective>
|
|
Create a general-purpose atomic file write utility package for safe file modifications.
|
|
|
|
Purpose: Phase 16+ tag writers need to modify audio files without risk of corruption. This utility handles write-to-temp-then-rename, permission preservation, cross-directory rejection, and orphan cleanup. Callback API pattern: `AtomicWrite(targetPath, func(tempFile *os.File) error)`.
|
|
|
|
Output: New `backend/fileutil` package with AtomicWrite function and comprehensive tests.
|
|
</objective>
|
|
|
|
<execution_context>
|
|
@/home/caleb/.config/opencode/get-shit-done/workflows/execute-plan.md
|
|
@/home/caleb/.config/opencode/get-shit-done/templates/summary.md
|
|
</execution_context>
|
|
|
|
<context>
|
|
@.planning/PROJECT.md
|
|
@.planning/ROADMAP.md
|
|
@.planning/STATE.md
|
|
@.planning/phases/15-schema-migration-write-safety/15-CONTEXT.md
|
|
|
|
<interfaces>
|
|
<!-- Reference implementation in codebase (not importable — in cmd/ tool): -->
|
|
|
|
From backend/events/cmd/genevents/main.go:
|
|
```go
|
|
// writeAtomic writes data to a temporary file in the same directory as path,
|
|
// then renames it into place for atomic replacement.
|
|
func writeAtomic(path, data string) error {
|
|
dir := filepath.Dir(path)
|
|
tmp, err := os.CreateTemp(dir, ".genevents-*.tmp")
|
|
if err != nil {
|
|
return err
|
|
}
|
|
tmpName := tmp.Name()
|
|
if _, err := tmp.WriteString(data); err != nil {
|
|
_ = tmp.Close()
|
|
_ = os.Remove(tmpName)
|
|
return err
|
|
}
|
|
if err := tmp.Close(); err != nil {
|
|
_ = os.Remove(tmpName)
|
|
return err
|
|
}
|
|
return os.Rename(tmpName, path)
|
|
}
|
|
```
|
|
|
|
<!-- This is the starting pattern. AtomicWrite generalizes it with:
|
|
- Callback API (func(f *os.File) error) instead of string data
|
|
- .yj-tmp suffix (not random pattern)
|
|
- Permission preservation
|
|
- Cross-filesystem rejection
|
|
- Orphan cleanup
|
|
-->
|
|
</interfaces>
|
|
</context>
|
|
|
|
<tasks>
|
|
|
|
<task type="auto">
|
|
<name>Task 1: Create backend/fileutil package with AtomicWrite</name>
|
|
<files>
|
|
backend/fileutil/atomicwrite.go
|
|
</files>
|
|
<action>
|
|
Create a new package `backend/fileutil` with an `AtomicWrite` function.
|
|
|
|
**Package doc comment:**
|
|
```go
|
|
// Package fileutil provides file system utilities for safe file operations.
|
|
package fileutil
|
|
```
|
|
|
|
**API:**
|
|
```go
|
|
func AtomicWrite(targetPath string, fn func(tmp *os.File) error) error
|
|
```
|
|
|
|
**Implementation requirements (from CONTEXT.md locked decisions):**
|
|
|
|
1. **Temp file naming**: Use `targetPath + ".yj-tmp"` as the temp file path. Do NOT use `os.CreateTemp` with random patterns — the deterministic suffix enables orphan cleanup. Example: writing to `song.mp3` creates `song.mp3.yj-tmp`.
|
|
|
|
2. **Orphan cleanup**: Before creating the temp file, check if `targetPath + ".yj-tmp"` already exists (orphan from a previous crash). If it does, remove it. If removal fails (permissions, file lock), log at debug level and continue — don't block the operation. Accept an optional `*slog.Logger` parameter or use a package-level approach. Per CONTEXT.md: "If an orphaned temp file can't be deleted (permissions, file lock), log a warning and continue."
|
|
|
|
Decision: Use a `slog.Logger` parameter for consistency with codebase conventions. Signature becomes:
|
|
```go
|
|
func AtomicWrite(logger *slog.Logger, targetPath string, fn func(tmp *os.File) error) error
|
|
```
|
|
|
|
3. **Cross-filesystem rejection**: Before the rename, verify the temp file and target are on the same filesystem. The simplest approach: since the temp file is created in the same directory as the target (using `filepath.Dir(targetPath)`), same-directory guarantees same filesystem. But the function should still guard against the caller passing a targetPath that resolves across mount points. Use an explicit check: call `os.Stat` on the parent directory and compare device IDs. Actually — the simpler and more robust approach per CONTEXT.md: "Cross-filesystem writes rejected with a clear error — no fallback to copy-then-delete." Since the temp file is always in the same dir as target, `os.Rename` will fail if the directory itself is somehow cross-device. Let `os.Rename` return the error naturally, and wrap it with a clear message mentioning cross-filesystem. Define a sentinel error:
|
|
```go
|
|
var ErrCrossDevice = errors.New("atomic write: cross-device rename not supported")
|
|
```
|
|
After `os.Rename` fails, check if the error is `syscall.EXDEV` (cross-device link) and wrap with `ErrCrossDevice`. For other rename errors, wrap normally.
|
|
|
|
4. **Permission preservation**: Before writing, `os.Stat(targetPath)` to get the current file mode. If the target exists, apply `os.Chmod(tmpPath, mode)` before the rename. If the target doesn't exist, use `0644` as default (per CONTEXT.md).
|
|
|
|
5. **Cleanup on error**: If the callback `fn` returns an error, or if `Close()` fails, or if `Chmod` fails — remove the temp file before returning. Always clean up on failure.
|
|
|
|
6. **Implementation flow:**
|
|
```
|
|
a. Clean orphaned .yj-tmp file (if exists)
|
|
b. Stat target for permissions (os.Stat, handle not-exist)
|
|
c. Create temp file (os.Create on targetPath + ".yj-tmp")
|
|
d. Call fn(tmpFile) — caller writes data
|
|
e. Sync temp file (tmpFile.Sync() for durability)
|
|
f. Close temp file
|
|
g. Chmod temp file to match target permissions
|
|
h. Rename temp file to target (atomic)
|
|
i. On any error in d-h: remove temp file, return wrapped error
|
|
```
|
|
|
|
**Sentinel errors:**
|
|
```go
|
|
var ErrCrossDevice = errors.New("atomic write: cross-device rename not supported")
|
|
```
|
|
|
|
**What to avoid:**
|
|
- Do NOT use `os.CreateTemp` with random patterns — the deterministic `.yj-tmp` suffix is a locked decision
|
|
- Do NOT use `io.Copy` fallback for cross-device — rejection is the correct behavior per CONTEXT.md
|
|
- Do NOT make this audio-file-specific — it's a general-purpose utility per CONTEXT.md ("not audio-file-specific")
|
|
</action>
|
|
<verify>
|
|
<automated>cd /mnt/vault/dev/golang/yellowjacket && go vet -tags webkit2_41 ./backend/fileutil/ && go build -tags webkit2_41 ./backend/fileutil/</automated>
|
|
</verify>
|
|
<done>
|
|
- `backend/fileutil/atomicwrite.go` exists with exported `AtomicWrite` function
|
|
- `ErrCrossDevice` sentinel error exported
|
|
- Package compiles without errors
|
|
- Function signature: `AtomicWrite(logger *slog.Logger, targetPath string, fn func(tmp *os.File) error) error`
|
|
</done>
|
|
</task>
|
|
|
|
<task type="auto">
|
|
<name>Task 2: Add comprehensive tests for AtomicWrite</name>
|
|
<files>
|
|
backend/fileutil/atomicwrite_test.go
|
|
</files>
|
|
<action>
|
|
Create `backend/fileutil/atomicwrite_test.go` with comprehensive table-driven tests.
|
|
|
|
**Test cases to implement:**
|
|
|
|
1. **TestAtomicWrite_Success** — Happy path:
|
|
- Create a target file with known content and specific permissions (e.g., 0o755)
|
|
- Call AtomicWrite to overwrite with new content
|
|
- Verify: target has new content, permissions preserved, no .yj-tmp file remains
|
|
|
|
2. **TestAtomicWrite_NewFile** — Target doesn't exist:
|
|
- Call AtomicWrite on a path that doesn't exist yet
|
|
- Verify: file created with new content, permissions are 0644, no .yj-tmp remains
|
|
|
|
3. **TestAtomicWrite_CallbackError** — Callback returns error:
|
|
- Call AtomicWrite with a callback that returns an error after partial write
|
|
- Verify: original file content is unchanged, no .yj-tmp file remains, error propagated
|
|
|
|
4. **TestAtomicWrite_OrphanCleanup** — Crash simulation:
|
|
- Create a `.yj-tmp` orphan file manually (simulating previous crash)
|
|
- Call AtomicWrite on the same target
|
|
- Verify: orphan was cleaned up, new write succeeded, target has correct content
|
|
|
|
5. **TestAtomicWrite_CrossDirectoryRejection** — Different directory:
|
|
- This test verifies the behavior when rename would cross filesystems
|
|
- Since we can't easily create cross-filesystem scenarios in CI, test that the temp file is always created in the same directory as the target:
|
|
- Call AtomicWrite on a file in `t.TempDir()/subdir/file.txt`
|
|
- During the callback, verify the temp file exists at `t.TempDir()/subdir/file.txt.yj-tmp`
|
|
- This confirms the temp file is always same-dir, making cross-device impossible in normal use
|
|
|
|
6. **TestAtomicWrite_PermissionPreservation** — Table-driven with different modes:
|
|
- Test with 0o644, 0o755, 0o600
|
|
- Verify each mode is preserved after atomic write
|
|
|
|
7. **TestAtomicWrite_SyncAndClose** — Verify file is properly synced:
|
|
- Write substantial data (e.g., 1MB)
|
|
- Verify target file size matches expected after AtomicWrite
|
|
|
|
**All tests must follow codebase patterns:**
|
|
- `package fileutil` (internal test, same package)
|
|
- `t.Parallel()` at top level and in subtests
|
|
- `t.TempDir()` for all file operations
|
|
- `t.Fatalf` for setup failures, `t.Errorf` for assertion failures
|
|
- No assertion libraries — raw comparisons
|
|
- `//nolint:mnd` for magic numbers in test data where needed
|
|
|
|
**Logger for tests:** Use `slog.Default()` — tests don't need special log handling.
|
|
</action>
|
|
<verify>
|
|
<automated>cd /mnt/vault/dev/golang/yellowjacket && go test -tags webkit2_41 -v -race -count=1 -timeout 30s ./backend/fileutil/</automated>
|
|
</verify>
|
|
<done>
|
|
- All 7 test functions pass
|
|
- Tests verify: successful write, new file creation, callback error rollback, orphan cleanup, same-dir temp file, permission preservation, proper sync
|
|
- Race detector passes (no concurrency issues)
|
|
- `make test` passes (full test suite including new tests)
|
|
- `make lint` passes (no lint violations in new code)
|
|
</done>
|
|
</task>
|
|
|
|
</tasks>
|
|
|
|
<verification>
|
|
1. `go test -tags webkit2_41 -v -race -count=1 ./backend/fileutil/` — all tests pass
|
|
2. `make test` — full test suite passes
|
|
3. `make lint` — no lint violations
|
|
4. `go vet -tags webkit2_41 ./backend/fileutil/` — clean
|
|
5. Grep verification: `grep -rn '\.yj-tmp' backend/fileutil/` confirms .yj-tmp suffix usage
|
|
6. Grep verification: `grep -n 'ErrCrossDevice' backend/fileutil/atomicwrite.go` confirms sentinel exported
|
|
</verification>
|
|
|
|
<success_criteria>
|
|
- `backend/fileutil/` package exists with `AtomicWrite` function and `ErrCrossDevice` sentinel
|
|
- AtomicWrite uses `.yj-tmp` suffix, callback API, permission preservation, orphan cleanup, cross-device rejection
|
|
- 7 test functions covering success, new file, callback error, orphan cleanup, same-dir, permissions, sync
|
|
- All tests pass with race detector
|
|
- Full `make test` and `make lint` pass
|
|
</success_criteria>
|
|
|
|
<output>
|
|
After completion, create `.planning/phases/15-schema-migration-write-safety/15-02-SUMMARY.md`
|
|
</output>
|