Extract one atomic-write / relocation-rollback engine shared by pageops and mediaops #116
Labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Cloonar/deckle#116
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found by an architecture review (deepening opportunities — turning shallow modules into deep ones).
Files:
internal/pageops/relocate.go(~295-460),internal/mediaops/relocate.go(~22-70),internal/pageops/fmedit.go:159+internal/mediaops/relocate.go:30(writeFileKeepMode, identical twice),internal/server/server.go:1066(atomicWriteFile, a third variant).Problem: the "precompute edits in memory → single atomic
os.Rename→ write pages through a test-swappable seam → restore original bytes + reverse rename on failure" discipline is implemented in parallel inpageopsandmediaops— two adapters of the same idea, i.e. a real seam that was never extracted. In both packages the write seam for rollback tests is a hidden package-level function variable rather than a parameter.Solution: extract a relocation-engine module both packages drive; make the write seam an explicit parameter. Consolidate the three atomic-write helpers into it.
Benefits: Leverage — rollback correctness (the scariest code in a CMS that promises "never leave the site broken") is written once. Locality — crash-safety tests concentrate on one module instead of two mirrored suites, and the test seam stops being ambient package state.
Agent Brief
Category: enhancement
Summary: Extract the relocation-rollback engine that
pageopsandmediaopseach implement in parallel into one shared internal package, with the write seam as an explicit parameter, and consolidate the duplicated file-write helpers.Current behavior:
internal/pageopshas an unexportedrelocateengine behindRename/Move/MoveAt: every edit is precomputed in memory (nothing on disk touched), one atomicos.Renamerelocates the bundle, then each edit is written through a package-level function variable (writeRelocateEdit); on a write failure the files already written are restored from their original bytes (best-effort) and the rename is reversed, with a distinct "the site needs manual attention" error when the reverse rename fails too.internal/mediaopshas its own unexportedrelocatebehindRenameFile/MoveFile/RenameFolder/MoveFolder, mirroring the same discipline with its own seam variable (writeMediaEdit) plus an explicit pre-rename target-absence check (os.Lstat→ conflict error).writeFileKeepMode(overwrite a file preserving its permission bits) is duplicated byte-identically inpageopsandmediaops.internal/serverhas a third, mechanically different helper,atomicWriteFile(temp file in the same directory + rename over the target, preserving mode), used by the page-save path.Desired behavior:
A new leaf-level internal package (agent picks the name; it must not import
pageopsormediaops—mediaopsalready importspageops, so the engine has to sit below both) exporting:oldAbs,newAbs, the ordered edit list, and the write function as an explicit parameter — performing the target-absence check, the single atomic rename, the ordered writes, and on failure the best-effort byte restoration plus reverse rename (including the dual-failure "manual attention" error);Both
pageops.relocateandmediaops.relocatebecome thin adapters over the engine. Each package keeps its own error-kind classification (conflict vs internal, driving HTTP status mapping), so the engine must return errors adapters can classify — e.g. a typed/sentinel target-exists error.Unification caution:
pageopstoday has no explicit pre-rename existence check (collisions are guarded upstream and rename errors are mapped, including a "not empty" string match for directories), whilemediaopsrefuses viaLstatbefore renaming. Adopting the explicit target-absence check for both is acceptable — it is stricter and still a conflict — but every existing conflict/no-op test in both packages must keep passing.Rollback testing should gain locality: the crash-safety matrix (write failure mid-phase → restore + reverse rename; reverse rename also failing → manual-attention error) belongs in the new package's own test suite, exercised by injecting a failing writer through the parameter. The existing
pageops/mediaopsrollback tests may be kept as thinner integration coverage or consolidated; the new package itself must not introduce a package-level seam variable.Key interfaces:
Relocate(oldAbs, newAbs string, edits []..., write func(abs string, data []byte) error) error(exact shape up to the agent) + exported keep-mode write helper + exported temp-file atomic write helper.pageops: unexportedrelocate, seam varwriteRelocateEdit, duplicatedwriteFileKeepMode(also referenced byapplyToFile,Reorder's ladder writes, andcreate'swriteLadderStepdefault — those callers switch to the shared helper).mediaops: unexportedrelocate, seam varwriteMediaEdit, duplicatedwriteFileKeepMode.server:atomicWriteFileused by the page-save/write path.Acceptance criteria:
pageopsnormediaopscontains its own rollback loop any more.pageopsandmediaops(all call sites, not just relocation) use it.pageops/mediaopscallers is unchanged (existing error-kind tests pass).go test ./...passes.Out of scope:
pageops,mediaops, or the server.pageops' create-ladder andReorderwrite-with-restore loops (no rename involved) into the engine — they should adopt the shared write helper, but their rollback structure stays as is.scaffold/site/AGENTS.md— purely internal refactoring, nothing agent-facing changes.