chore: complete v1.2 Tag Editing milestone
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.
This commit is contained in:
yonlu committed
1 parent
e37535b115
commit
2256f8f329
84 files changed
+861
-10857
No files matched your search
@@ -0,0 +1,258 @@
|
||||
---
|
||||
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>
|
||||
Reference in new issue
Block a user