refactor: extract shared atomic-write/relocation-rollback engine into internal/atomicfs #123

Merged

dominik.polakovics merged 1 commit from afk/116 into main

2026-07-19 01:09:49 +02:00

dominik.polakovics commented

2026-07-19 01:01:23 +02:00

Owner

Closes #116

What

Extracts the relocation-rollback discipline that internal/pageops and internal/mediaops each implemented in parallel into one leaf package, internal/atomicfs, and consolidates the three file-write helpers:

atomicfs.Relocate(oldAbs, newAbs, edits, write) — the engine: Lstat target-absence pre-check, one atomic os.Rename (collision-shaped rename errors mapped to the ErrTargetExists sentinel), ordered writes through an explicit WriteFunc parameter, and on failure best-effort restoration of original bytes plus the reverse rename, with a typed *WriteError whose RollbackErr carries the dual-failure "the site needs manual attention" case.
atomicfs.WriteFileKeepMode — the permission-preserving write, previously duplicated byte-identically in both packages; all pageops call sites (applyToFile, Reorder's ladder writes, create's writeLadderStep default) now use it.
atomicfs.ReplaceFile — the server's temp-file-then-rename save helper, moved with unchanged semantics (same-directory temp file, mode preserved from the existing target, rename over the target).

pageops.relocate and mediaops.relocate are now thin adapters: they keep all their precompute logic and their own conflict-vs-internal error-kind classification (errors.Is(err, atomicfs.ErrTargetExists) → conflict, byte-identical message; everything else internal), so HTTP status mapping is unchanged.

Testing

The crash-safety matrix (ordered writes, mid-phase failure → restore + reverse rename, reverse rename also failing → manual-attention error, target-exists refusals, post-rename edit paths inside a renamed directory) lives in internal/atomicfs's own suite, with failures injected through the write parameter — no permission tricks, so it runs under CI's root runner.
The five pageops/mediaops rollback tests that used to swap the package seam vars (writeRelocateEdit/writeMediaEdit, both deleted) remain as thinner integration coverage using read-only files, following the pre-existing TestMoveRollbackOnAliasFailure pattern (skipped under root; verified running and passing here as uid 992).
go build ./..., go test ./... -count=1, go vet ./... all green; gofmt clean on every touched package.

Deliberate small behavior notes (kinds/status unchanged)

pageops relocation now performs the explicit Lstat target-absence pre-check mediaops always had (stricter, still classified conflict; previously pageops only mapped rename errors). All existing conflict/no-op tests pass.
The write-failure message mediaops formats changed from "rewriting the referencing pages failed (…)" to the engine's canonical "writing the relocation edits failed (…)" (pageops' phrasing, now the single source of truth), and write errors embed the raw os error instead of a "writing page: " prefix. No test asserted these strings; error kinds are untouched.

scaffold/site/AGENTS.md untouched — purely internal refactoring, nothing agent-facing changes (per the issue's out-of-scope note).

🤖 Generated with Claude Code

Closes #116 ## What Extracts the relocation-rollback discipline that `internal/pageops` and `internal/mediaops` each implemented in parallel into one leaf package, `internal/atomicfs`, and consolidates the three file-write helpers: - **`atomicfs.Relocate(oldAbs, newAbs, edits, write)`** — the engine: Lstat target-absence pre-check, one atomic `os.Rename` (collision-shaped rename errors mapped to the `ErrTargetExists` sentinel), ordered writes through an **explicit `WriteFunc` parameter**, and on failure best-effort restoration of original bytes plus the reverse rename, with a typed `*WriteError` whose `RollbackErr` carries the dual-failure "the site needs manual attention" case. - **`atomicfs.WriteFileKeepMode`** — the permission-preserving write, previously duplicated byte-identically in both packages; all pageops call sites (`applyToFile`, `Reorder`'s ladder writes, `create`'s `writeLadderStep` default) now use it. - **`atomicfs.ReplaceFile`** — the server's temp-file-then-rename save helper, moved with unchanged semantics (same-directory temp file, mode preserved from the existing target, rename over the target). `pageops.relocate` and `mediaops.relocate` are now thin adapters: they keep all their precompute logic and their own conflict-vs-internal error-kind classification (`errors.Is(err, atomicfs.ErrTargetExists)` → conflict, byte-identical message; everything else internal), so HTTP status mapping is unchanged. ## Testing - The crash-safety matrix (ordered writes, mid-phase failure → restore + reverse rename, reverse rename also failing → manual-attention error, target-exists refusals, post-rename edit paths inside a renamed directory) lives in `internal/atomicfs`'s own suite, with failures injected **through the write parameter** — no permission tricks, so it runs under CI's root runner. - The five pageops/mediaops rollback tests that used to swap the package seam vars (`writeRelocateEdit`/`writeMediaEdit`, both deleted) remain as thinner integration coverage using read-only files, following the pre-existing `TestMoveRollbackOnAliasFailure` pattern (skipped under root; verified running and passing here as uid 992). - `go build ./...`, `go test ./... -count=1`, `go vet ./...` all green; gofmt clean on every touched package. ## Deliberate small behavior notes (kinds/status unchanged) - pageops relocation now performs the explicit Lstat target-absence pre-check mediaops always had (stricter, still classified conflict; previously pageops only mapped rename errors). All existing conflict/no-op tests pass. - The write-failure message mediaops formats changed from "rewriting the referencing pages failed (…)" to the engine's canonical "writing the relocation edits failed (…)" (pageops' phrasing, now the single source of truth), and write errors embed the raw os error instead of a "writing page: " prefix. No test asserted these strings; error kinds are untouched. `scaffold/site/AGENTS.md` untouched — purely internal refactoring, nothing agent-facing changes (per the issue's out-of-scope note). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

dominik.polakovics added 1 commit

2026-07-19 01:01:23 +02:00

refactor: extract shared atomic-write/relocation-rollback engine into internal/atomicfs

ci / build-and-test (pull_request) Successful in 6m16s

Details

8b622db4b8

The "precompute edits in memory, one atomic os.Rename, write through a
seam, restore original bytes + reverse rename on failure" discipline was
implemented in parallel in pageops and mediaops, with the write seam as a
hidden package-level function variable in each, and a third atomic-write
helper in server.

internal/atomicfs now owns the engine: the Lstat target-absence
pre-check, the atomic rename (collision errors mapped to an
ErrTargetExists sentinel), the ordered writes through an explicit
WriteFunc parameter, and the rollback with the dual-failure
"manual attention" WriteError. pageops.relocate and mediaops.relocate
are thin adapters that keep their own conflict-vs-internal error-kind
classification; WriteFileKeepMode exists exactly once; the server's page
save uses atomicfs.ReplaceFile with unchanged temp-file-then-rename
semantics.

The crash-safety matrix lives in atomicfs's own tests, injected through
the write parameter so it runs under a root test runner; the
pageops/mediaops rollback tests remain as thinner integration coverage
using read-only files (skipped under root), and neither package has an
ambient seam variable any more.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

dominik.polakovics commented

2026-07-19 01:08:45 +02:00

Author

Owner

This was generated by AI while landing a PR.

Landing validation — PASS

Verification signal relied on: Forgejo Actions ci / build-and-test (pull_request) — success (6m16s, run 113). Not re-run locally; the PR is a single commit (8b622db) with no post-CI changes.

Checks performed

Conventions — title is Conventional Commits (refactor:); body carries a working Closes #116, so the merge will auto-close the issue and release the afk/116 claim.
Mergeable — clean merge against current main (534add4); no conflict, single commit.
Acceptance criteria vs #116 — all met: one engine (atomicfs.Relocate) owns rename + ordered writes + rollback, and neither pageops nor mediaops retains a rollback loop; the write seam is a WriteFunc parameter with no package-level seam var in the new package (both writeRelocateEdit and writeMediaEdit deleted); WriteFileKeepMode exists exactly once and all non-relocation call sites (applyToFile, Reorder, writeLadderStep) use it; the server's save path uses ReplaceFile with the temp-file-then-rename semantics unchanged; nine crash-safety tests live in internal/atomicfs covering mid-phase restore + reverse rename and the dual-failure manual-attention error.
Error classification — unchanged in both adapters: errors.Is(err, atomicfs.ErrTargetExists) → conflict, everything else internal, so HTTP status mapping is untouched.
The riskiest change, reviewed specifically — pageops relocation newly performs the Lstat target-absence pre-check it previously lacked. Verified this cannot turn a former no-op into a conflict: Rename returns early when slug == b.slug (title-only write, nothing relocated), uniqueSlugExcept guarantees the new slug is free among siblings, and MoveAt's same-parent case is a pure reposition that never reaches relocate. relocate is therefore never called with oldAbs == newAbs.

Non-blocking nit

atomicfs.WriteError.Error's doc comment claims the two templates are "byte-exact with the messages pageops and mediaops produced before this engine was extracted". That is true for pageops but not for mediaops, whose wording changed from "rewriting the referencing pages failed (…)" to the canonical "writing the relocation edits failed (…)". The PR body discloses this deliberately and no test asserts the strings — only the code comment overstates it. Worth a one-line fix on a future touch; not worth blocking on.

Verdict: PASS — proceeding to the human merge gate.

> *This was generated by AI while landing a PR.* ## Landing validation — PASS **Verification signal relied on:** Forgejo Actions `ci / build-and-test (pull_request)` — **success** (6m16s, run 113). Not re-run locally; the PR is a single commit (`8b622db`) with no post-CI changes. **Checks performed** - **Conventions** — title is Conventional Commits (`refactor:`); body carries a working `Closes #116`, so the merge will auto-close the issue and release the `afk/116` claim. - **Mergeable** — clean merge against current `main` (`534add4`); no conflict, single commit. - **Acceptance criteria vs #116** — all met: one engine (`atomicfs.Relocate`) owns rename + ordered writes + rollback, and neither `pageops` nor `mediaops` retains a rollback loop; the write seam is a `WriteFunc` parameter with no package-level seam var in the new package (both `writeRelocateEdit` and `writeMediaEdit` deleted); `WriteFileKeepMode` exists exactly once and all non-relocation call sites (`applyToFile`, `Reorder`, `writeLadderStep`) use it; the server's save path uses `ReplaceFile` with the temp-file-then-rename semantics unchanged; nine crash-safety tests live in `internal/atomicfs` covering mid-phase restore + reverse rename and the dual-failure manual-attention error. - **Error classification** — unchanged in both adapters: `errors.Is(err, atomicfs.ErrTargetExists)` → conflict, everything else internal, so HTTP status mapping is untouched. - **The riskiest change, reviewed specifically** — `pageops` relocation newly performs the `Lstat` target-absence pre-check it previously lacked. Verified this cannot turn a former no-op into a conflict: `Rename` returns early when `slug == b.slug` (title-only write, nothing relocated), `uniqueSlugExcept` guarantees the new slug is free among siblings, and `MoveAt`'s same-parent case is a pure reposition that never reaches `relocate`. `relocate` is therefore never called with `oldAbs == newAbs`. **Non-blocking nit** `atomicfs.WriteError.Error`'s doc comment claims the two templates are "byte-exact with the messages pageops and mediaops produced before this engine was extracted". That is true for `pageops` but not for `mediaops`, whose wording changed from "rewriting the referencing pages failed (…)" to the canonical "writing the relocation edits failed (…)". The PR body discloses this deliberately and no test asserts the strings — only the code comment overstates it. Worth a one-line fix on a future touch; not worth blocking on. **Verdict: PASS** — proceeding to the human merge gate.

dominik.polakovics merged commit ff23fdf33d into main

2026-07-19 01:09:49 +02:00

dominik.polakovics referenced this pull request from a commit

2026-07-19 01:09:50 +02:00

Merge pull request 'refactor: extract shared atomic-write/relocation-rollback engine into internal/atomicfs' (#123) from afk/116 into main