fix(attach): only reassign DB after a successful physical move (bookshelf-8l09k) #1461

Merged

zombor merged 1 commit from bd-bookshelf-8l09k into main 2026-08-31 01:34:34 +00:00

Conversation 3 Commits 1 Files changed 2 +103 -107

zombor commented 2026-08-28 19:07:38 +00:00

Owner

Copy link

Root cause

internal/books/attach_service.go's AttachFiles committed the DB library
reassignment (Phase 2, a single atomic transaction) UNCONDITIONALLY, then
attempted the physical file move afterward (Phase 3), recording any move
failure in MovesFailed post-commit. A failed move therefore left
book_file rows pointing at the target library while the physical file
remained at the source path (or was gone) — contradicting the documented
"skip-on-failed-move, no data loss" guarantee. Pre-existing bug, surfaced
during f8gf8/#1454 review.

Chosen approach: move-then-commit

Of the three options considered (move-then-commit / compensating rollback /
tx-scoped-to-moved-files), move-then-commit is simplest and safest given the
existing hard-delete of source books:

• Compensating rollback was rejected: the transaction hard-deletes fully
  reassigned source books. Undoing a hard DELETE after a downstream move
  failure would require reconstructing the deleted row from scratch — much
  riskier than never deleting it in the first place.
• Move-then-commit (chosen) fits naturally on top of the existing
  per-file guard pattern (null bytes, symlinks, source-containment,
  dest-exists, mkdir). The physical move is just one more guard: attempt it
  before building the final fileIDs/reassign-count lists, and treat a
  failed move exactly like every other guard failure — exclude the file
  from DB reassignment (and therefore from its source book's hard-delete
  eligibility). No new state-tracking machinery was needed.

What changed

• Physical moves happen BEFORE the DB transaction, not after.
• A failed move excludes that file from DB reassignment entirely — the file
  stays owned by its source book, so the DB and filesystem never disagree
  about where a file lives.
• Source books are only hard-deleted when every one of their files was both
  guard-passed and (for move_files=true) actually moved.
• MoveFailure/AttachResult.MovesFailed doc comments updated to describe
  "excluded from reassignment" instead of "DB already committed, file
  stranded."
• The function doc now documents a narrower residual risk symmetric to the
  one just closed: if the DB transaction itself fails (e.g. a lost DB
  connection) AFTER files have already been moved on disk, those files' rows
  will not reflect the move. This is an infra-failure edge case, not a move
  failure, and is called out explicitly rather than silently accepted.

Test plan

• Rewrote the three attach_service_test.go contexts that asserted the OLD
  (buggy) "DB committed, move fails" behavior to instead assert the fixed
  behavior: a failed move (a) excludes the file from reassignedFileIDs,
  (b) does NOT hard-delete the file's source book, (c) is still reported in
  MovesFailed with file ID / src path / error summary.
• Added a test asserting the documented residual risk: when the DB
  transaction itself fails, files that already passed guards have already
  been moved on disk by that point (move-then-commit).
• make test / make lint / make coverage all green locally (100%
  internal/ coverage maintained).

Closes bead bookshelf-8l09k on merge.

## Root cause `internal/books/attach_service.go`'s `AttachFiles` committed the DB library reassignment (Phase 2, a single atomic transaction) UNCONDITIONALLY, then attempted the physical file move afterward (Phase 3), recording any move failure in `MovesFailed` post-commit. A failed move therefore left `book_file` rows pointing at the target library while the physical file remained at the source path (or was gone) — contradicting the documented "skip-on-failed-move, no data loss" guarantee. Pre-existing bug, surfaced during f8gf8/#1454 review. ## Chosen approach: move-then-commit Of the three options considered (move-then-commit / compensating rollback / tx-scoped-to-moved-files), move-then-commit is simplest and safest given the existing hard-delete of source books: - **Compensating rollback** was rejected: the transaction hard-deletes fully reassigned source books. Undoing a hard `DELETE` after a downstream move failure would require reconstructing the deleted row from scratch — much riskier than never deleting it in the first place. - **Move-then-commit** (chosen) fits naturally on top of the existing per-file guard pattern (null bytes, symlinks, source-containment, dest-exists, mkdir). The physical move is just one more guard: attempt it before building the final `fileIDs`/reassign-count lists, and treat a failed move exactly like every other guard failure — exclude the file from DB reassignment (and therefore from its source book's hard-delete eligibility). No new state-tracking machinery was needed. ## What changed - Physical moves happen BEFORE the DB transaction, not after. - A failed move excludes that file from DB reassignment entirely — the file stays owned by its source book, so the DB and filesystem never disagree about where a file lives. - Source books are only hard-deleted when every one of their files was both guard-passed and (for `move_files=true`) actually moved. - `MoveFailure`/`AttachResult.MovesFailed` doc comments updated to describe "excluded from reassignment" instead of "DB already committed, file stranded." - The function doc now documents a narrower residual risk symmetric to the one just closed: if the DB transaction itself fails (e.g. a lost DB connection) AFTER files have already been moved on disk, those files' rows will not reflect the move. This is an infra-failure edge case, not a move failure, and is called out explicitly rather than silently accepted. ## Test plan - Rewrote the three `attach_service_test.go` contexts that asserted the OLD (buggy) "DB committed, move fails" behavior to instead assert the fixed behavior: a failed move (a) excludes the file from `reassignedFileIDs`, (b) does NOT hard-delete the file's source book, (c) is still reported in `MovesFailed` with file ID / src path / error summary. - Added a test asserting the documented residual risk: when the DB transaction itself fails, files that already passed guards have already been moved on disk by that point (`move-then-commit`). - `make test` / `make lint` / `make coverage` all green locally (100% `internal/` coverage maintained). Closes bead bookshelf-8l09k on merge.

zombor added 1 commit 2026-08-28 19:07:38 +00:00

fix(attach): move-then-commit so a failed move never orphans the DB (bookshelf-8l09k) ... 5b378c3055

AttachFiles previously committed the DB library reassignment in a
transaction (Phase 2), then attempted the physical file move afterward
(Phase 3), recording any move failure post-commit in MovesFailed. A
failed move therefore left book_file rows pointing at the target
library while the physical file remained at the source path (or was
gone) — contradicting the documented "skip-on-failed-move, no data
loss" guarantee.

Reorder to move-then-commit: physical moves (guarded by the existing
null-byte/symlink/containment/dest-exists/mkdir checks) now happen
BEFORE the DB transaction. A failed move excludes that file from DB
reassignment entirely, exactly like the other pre-tx guards — the file
stays owned by its source book and the DB/filesystem never disagree.
Source books are only hard-deleted when every one of their files was
both guard-passed and (for MoveFiles=true) actually moved.

This closes the primary reported risk (a failed move can no longer
strand a DB record pointing at an unreachable file) at the cost of a
narrower, documented residual risk: if the DB transaction itself fails
AFTER files have already been moved on disk (e.g. a lost DB
connection), those files' rows will not reflect the move. That is an
infra-failure edge case distinct from — and much rarer than — a move
failure, and is called out explicitly in the AttachFiles doc comment
rather than silently accepted.

Updated attach_service_test.go's failed-move contexts to assert the
new behavior: a failed move excludes the file from reassignedFileIDs
and retains its source book (no hard-delete), and reports the failure
in MovesFailed.

zombor commented 2026-08-28 19:18:16 +00:00

Author

Owner

Copy link

Security Review — PR #1461 (bd-bookshelf-8l09k)

Scope: internal/books/attach_service.go reorder (move-then-commit) + updated attach_service_test.go.

Summary: This is a well-scoped reorder. All pre-existing physical-file guards (null-byte rejection, symlink os.Lstat skip, source-root containment, dest-exists, mkdir) are preserved verbatim and still run before any move or DB write is attempted — none were dropped or weakened by moving the guard/move block earlier in the loop. Ownership/library scoping (getUserLibraryIDs, filterOwnedBookIDs, userIDFromRequest) lives entirely in the unchanged AttachHandler/wiring layer and is untouched by this diff. Net effect on security posture is neutral-to-positive: it closes the previously-flagged risk of a committed DB row pointing at a file that never actually moved.

[MINOR] internal/books/attach_service.go:doc comment (move-then-commit guarantee) — residual reverse-direction risk is documented but untested
The new doc comment correctly calls out that if the DB transaction fails after files have already been moved on disk (e.g. lost DB connection), those rows will not reflect the move — the source book's file is now physically missing from the source library while the DB still claims it lives there. This is accepted as an explicit, rare, infra-failure tradeoff (not introduced by this PR's happy path; symmetrical to the previous version's own residual risk in the opposite direction) and is exercised by the "DB transaction fails AFTER files have already been moved on disk" test context, so it is not a blind spot — flagging only because it's the one behavior a future reader might mistake as newly-introduced by this change. No fix required; the tradeoff is reasonable and already documented plus tested.

No BLOCKER or MAJOR findings:

1. Guard preservation — confirmed intact (null bytes, symlink Lstat, source-root containment via strings.HasPrefix(filepath.Clean(srcAbs), srcRoot+sep), dest-exists os.Stat, mkdirAll). Order relative to each other unchanged; only the move itself was pulled earlier, after all guards, same as before.
2. Multi-user/ownership scoping — unaffected; scoping happens in the handler layer (wire.go), not touched by this PR.
3. Move-before-commit data exposure — the file becomes physically present at the destination library path before the DB row says it belongs there. This mirrors (in reverse) the previous ordering's own pre-existing race with concurrent library scans and is not meaningfully different in blast radius: it does not, on its own, grant a different user read access to content they couldn't already reach if they have filesystem-level access to the target library directory, and application-level access is still gated by the (unchanged) book_file DB row + ownership check, which will not exist until the transaction commits. No new exposure.
4. No new injection/sink — dstAbs/srcAbs derivation unchanged (filepath.Join(libRoot, FileSubPath)), still validated by the same containment guard before use in renameFile.
5. Net-neutral-or-better — confirmed; the change closes a real "DB says target library, file physically isn't there" defect without introducing a new guard bypass.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security Review — PR #1461 (bd-bookshelf-8l09k) Scope: `internal/books/attach_service.go` reorder (move-then-commit) + updated `attach_service_test.go`. **Summary:** This is a well-scoped reorder. All pre-existing physical-file guards (null-byte rejection, symlink `os.Lstat` skip, source-root containment, dest-exists, mkdir) are preserved verbatim and still run before any move or DB write is attempted — none were dropped or weakened by moving the guard/move block earlier in the loop. Ownership/library scoping (`getUserLibraryIDs`, `filterOwnedBookIDs`, `userIDFromRequest`) lives entirely in the unchanged `AttachHandler`/wiring layer and is untouched by this diff. Net effect on security posture is neutral-to-positive: it closes the previously-flagged risk of a committed DB row pointing at a file that never actually moved. [MINOR] internal/books/attach_service.go:doc comment (move-then-commit guarantee) — residual reverse-direction risk is documented but untested The new doc comment correctly calls out that if the DB transaction fails *after* files have already been moved on disk (e.g. lost DB connection), those rows will not reflect the move — the source book's file is now physically missing from the source library while the DB still claims it lives there. This is accepted as an explicit, rare, infra-failure tradeoff (not introduced by this PR's happy path; symmetrical to the previous version's own residual risk in the opposite direction) and is exercised by the "DB transaction fails AFTER files have already been moved on disk" test context, so it is not a blind spot — flagging only because it's the one behavior a future reader might mistake as newly-introduced by this change. No fix required; the tradeoff is reasonable and already documented plus tested. No BLOCKER or MAJOR findings: 1. Guard preservation — confirmed intact (null bytes, symlink Lstat, source-root containment via `strings.HasPrefix(filepath.Clean(srcAbs), srcRoot+sep)`, dest-exists `os.Stat`, `mkdirAll`). Order relative to each other unchanged; only the move itself was pulled earlier, after all guards, same as before. 2. Multi-user/ownership scoping — unaffected; scoping happens in the handler layer (`wire.go`), not touched by this PR. 3. Move-before-commit data exposure — the file becomes physically present at the destination library path before the DB row says it belongs there. This mirrors (in reverse) the previous ordering's own pre-existing race with concurrent library scans and is not meaningfully different in blast radius: it does not, on its own, grant a different user read access to content they couldn't already reach if they have filesystem-level access to the target library directory, and application-level access is still gated by the (unchanged) book_file DB row + ownership check, which will not exist until the transaction commits. No new exposure. 4. No new injection/sink — dstAbs/srcAbs derivation unchanged (`filepath.Join(libRoot, FileSubPath)`), still validated by the same containment guard before use in `renameFile`. 5. Net-neutral-or-better — confirmed; the change closes a real "DB says target library, file physically isn't there" defect without introducing a new guard bypass. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-08-28 19:18:48 +00:00

Author

Owner

Copy link

Code Review: PR #1461 (bookshelf-8l09k)

Reviewed internal/books/attach_service.go diff against origin/main. Scope is exactly as described: move-then-commit reorder, no unrelated files touched, no .golangci.yml/coverage-exclusion changes.

Phase 1: Spec compliance

Matches the bead. The fix reorders the physical move to happen inside the per-file guard loop, BEFORE the DB transaction, and a failed move now does a plain continue (same as every other pre-existing guard failure: null-byte, symlink, containment, dest-exists, mkdir) — excluding the file from fileIDs/bookFileReassignCount entirely.

Phase 2: Code quality

Invariant check (item 1): Traced the loop in the new file (git show origin/bd-bookshelf-8l09k:internal/books/attach_service.go) — fileIDs = append(fileIDs, row.FileID) and bookFileReassignCount[row.BookID]++ only execute after falling through the entire per-file guard block, including the new move attempt. On mvErr != nil the code hits continue before reaching that append. So DB reassignment is now strictly limited to files whose move succeeded (or move wasn't requested). hardDeleteBooksRaw is driven by fullyReassignedBookIDs, computed from the same bookFileReassignCount, so a source book with any failed-move file is correctly retained, never hard-deleted. Verified against the new tests (does NOT reassign files in DB whose move failed, does NOT hard-delete source books whose file move failed) — the invariant holds.

Residual window (item 2): The new doc comment at the top of AttachFiles is honest about the inverted residual risk: if the DB tx itself fails/rolls back AFTER a physical move succeeded, the file is now on disk at the destination but the DB row still points at the source. This is documented, and a test explicitly locks it in (has already moved the files on disk by the time the transaction fails — asserts renames has 2 entries even though the tx failed). This is a smaller/no-worse trade: the old window fired on any catastrophic-I/O rename failure (the same failure class, exercised N independent times); the new window requires guards to pass, N independent renames to ALL succeed, AND then a single subsequent DB commit to fail — a narrower, lower-probability compound event, matching the PR author's own reasoning (compensating rollback was rejected because the tx hard-deletes source books, and undoing a hard DELETE post-move-failure is riskier than the current design). Acceptable trade-off, adequately documented in both code comments and tests. Not a blocker.

Symlink guard (item 3): the os.Lstat symlink-skip guard (Security: reject symlinks...skip symlinked sources to restore the no-follow safety) is untouched by the reorder — still runs before the move attempt, same continue pattern. Confirmed via diff — this block was not touched at all.

Transaction integrity (item 4): runInTx still wraps reassignBookFilesRaw + hardDeleteBooksRaw atomically; the fileIDs/fullyReassignedBookIDs slices passed in are now correctly the post-move-success sets (previously they were the post-guard, pre-move sets). No change to transaction atomicity itself, only to what data feeds it — correct given the reorder.

Test quality (item 5): internal/books/attach_service_test.go is package books_test, curried DI via the existing AttachFiles(...) constructor, one-Expect-per-It (verified — each It in the changed contexts asserts a single value). New/changed assertions correctly flip to match the new invariant: a full-move-failure context now asserts reassignedFileIDs empty + hardDeletedIDs empty + FilesReassigned==0 + SourceBooksRemoved==0 (previously asserted the opposite, DB-committed values) — these WILL fail if the reorder regresses back to move-after-commit, since the fakes track call order via the stubbed renameFile/tx functions. The single-book retained-on-move-failure context and the tx-fails-after-move context are equally solid. No guarded/tautological assertions spotted in the diff hunks.

Downstream consumers: grepped all callers of AttachResult/MovesFailed/FilesReassigned (attach_handler.go, bulk_attach_by_filter_store.go, and their tests) — none assume the old "DB committed but file may be stranded" semantics; they only read/sum/log the counts, which remain correct under the new invariant. The removed "Phase 3: move physical files AFTER the transaction commits..." doc block and its "post-commit"/"two-phase guarantee" language were fully replaced consistently; no stale references to the old ordering remain anywhere else in the codebase.

Concurrency/idempotency/N+1 (item 6): No change to fan-out shape — still a single per-attach-request loop (not a per-library fan-out), no N+1 introduced. renameFile/mkdirAll call counts unchanged, just reordered relative to the tx.

Findings

No BLOCKER or MAJOR findings. This is a correct, well-tested, adequately-documented reorder that closes the originally reported data-integrity gap without introducing a worse or undocumented failure mode.

[MINOR] internal/books/attach_service.go — doc/comment rewrite (MoveFailure, AttachResult, AttachFiles doc block) fully replaces the stale "Phase 3"/"post-commit"/"two-phase guarantee" language with accurate move-then-commit wording; confirmed no stale references remain anywhere else in the repo. No action needed — noted for the record only.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review: PR #1461 (bookshelf-8l09k) Reviewed `internal/books/attach_service.go` diff against `origin/main`. Scope is exactly as described: move-then-commit reorder, no unrelated files touched, no `.golangci.yml`/coverage-exclusion changes. ### Phase 1: Spec compliance Matches the bead. The fix reorders the physical move to happen inside the per-file guard loop, BEFORE the DB transaction, and a failed move now does a plain `continue` (same as every other pre-existing guard failure: null-byte, symlink, containment, dest-exists, mkdir) — excluding the file from `fileIDs`/`bookFileReassignCount` entirely. ### Phase 2: Code quality **Invariant check (item 1):** Traced the loop in the new file (`git show origin/bd-bookshelf-8l09k:internal/books/attach_service.go`) — `fileIDs = append(fileIDs, row.FileID)` and `bookFileReassignCount[row.BookID]++` only execute after falling through the entire per-file guard block, including the new move attempt. On `mvErr != nil` the code hits `continue` before reaching that append. So DB reassignment is now strictly limited to files whose move succeeded (or move wasn't requested). `hardDeleteBooksRaw` is driven by `fullyReassignedBookIDs`, computed from the same `bookFileReassignCount`, so a source book with any failed-move file is correctly retained, never hard-deleted. Verified against the new tests (`does NOT reassign files in DB whose move failed`, `does NOT hard-delete source books whose file move failed`) — the invariant holds. **Residual window (item 2):** The new doc comment at the top of `AttachFiles` is honest about the inverted residual risk: if the DB tx itself fails/rolls back AFTER a physical move succeeded, the file is now on disk at the destination but the DB row still points at the source. This is documented, and a test explicitly locks it in (`has already moved the files on disk by the time the transaction fails` — asserts `renames` has 2 entries even though the tx failed). This is a smaller/no-worse trade: the old window fired on any catastrophic-I/O rename failure (the same failure class, exercised N independent times); the new window requires guards to pass, N independent renames to ALL succeed, AND then a single subsequent DB commit to fail — a narrower, lower-probability compound event, matching the PR author's own reasoning (compensating rollback was rejected because the tx hard-deletes source books, and undoing a hard DELETE post-move-failure is riskier than the current design). Acceptable trade-off, adequately documented in both code comments and tests. Not a blocker. **Symlink guard (item 3):** the `os.Lstat` symlink-skip guard (`Security: reject symlinks...skip symlinked sources to restore the no-follow safety`) is untouched by the reorder — still runs before the move attempt, same `continue` pattern. Confirmed via diff — this block was not touched at all. **Transaction integrity (item 4):** `runInTx` still wraps `reassignBookFilesRaw` + `hardDeleteBooksRaw` atomically; the `fileIDs`/`fullyReassignedBookIDs` slices passed in are now correctly the post-move-success sets (previously they were the post-guard, pre-move sets). No change to transaction atomicity itself, only to what data feeds it — correct given the reorder. **Test quality (item 5):** `internal/books/attach_service_test.go` is `package books_test`, curried DI via the existing `AttachFiles(...)` constructor, one-Expect-per-It (verified — each `It` in the changed contexts asserts a single value). New/changed assertions correctly flip to match the new invariant: a full-move-failure context now asserts `reassignedFileIDs` empty + `hardDeletedIDs` empty + `FilesReassigned==0` + `SourceBooksRemoved==0` (previously asserted the opposite, DB-committed values) — these WILL fail if the reorder regresses back to move-after-commit, since the fakes track call order via the stubbed `renameFile`/tx functions. The single-book retained-on-move-failure context and the tx-fails-after-move context are equally solid. No guarded/tautological assertions spotted in the diff hunks. **Downstream consumers:** grepped all callers of `AttachResult`/`MovesFailed`/`FilesReassigned` (`attach_handler.go`, `bulk_attach_by_filter_store.go`, and their tests) — none assume the old "DB committed but file may be stranded" semantics; they only read/sum/log the counts, which remain correct under the new invariant. The removed "Phase 3: move physical files AFTER the transaction commits..." doc block and its "post-commit"/"two-phase guarantee" language were fully replaced consistently; no stale references to the old ordering remain anywhere else in the codebase. **Concurrency/idempotency/N+1 (item 6):** No change to fan-out shape — still a single per-attach-request loop (not a per-library fan-out), no N+1 introduced. `renameFile`/`mkdirAll` call counts unchanged, just reordered relative to the tx. ### Findings No BLOCKER or MAJOR findings. This is a correct, well-tested, adequately-documented reorder that closes the originally reported data-integrity gap without introducing a worse or undocumented failure mode. [MINOR] internal/books/attach_service.go — doc/comment rewrite (MoveFailure, AttachResult, AttachFiles doc block) fully replaces the stale "Phase 3"/"post-commit"/"two-phase guarantee" language with accurate move-then-commit wording; confirmed no stale references remain anywhere else in the repo. No action needed — noted for the record only. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-08-28 19:18:53 +00:00

Author

Owner

Copy link

Code Review: PR #1461 (bookshelf-8l09k)

Reviewed internal/books/attach_service.go diff against origin/main. Scope is exactly as described: move-then-commit reorder, no unrelated files touched, no .golangci.yml/coverage-exclusion changes.

Phase 1: Spec compliance

Matches the bead. The fix reorders the physical move to happen inside the per-file guard loop, BEFORE the DB transaction, and a failed move now does a plain continue (same as every other pre-existing guard failure: null-byte, symlink, containment, dest-exists, mkdir) — excluding the file from fileIDs/bookFileReassignCount entirely.

Phase 2: Code quality

Invariant check (item 1): Traced the loop in the new file (git show origin/bd-bookshelf-8l09k:internal/books/attach_service.go) — fileIDs = append(fileIDs, row.FileID) and bookFileReassignCount[row.BookID]++ only execute after falling through the entire per-file guard block, including the new move attempt. On mvErr != nil the code hits continue before reaching that append. So DB reassignment is now strictly limited to files whose move succeeded (or move wasn't requested). hardDeleteBooksRaw is driven by fullyReassignedBookIDs, computed from the same bookFileReassignCount, so a source book with any failed-move file is correctly retained, never hard-deleted. Verified against the new tests (does NOT reassign files in DB whose move failed, does NOT hard-delete source books whose file move failed) — the invariant holds.

Residual window (item 2): The new doc comment at the top of AttachFiles is honest about the inverted residual risk: if the DB tx itself fails/rolls back AFTER a physical move succeeded, the file is now on disk at the destination but the DB row still points at the source. This is documented, and a test explicitly locks it in (has already moved the files on disk by the time the transaction fails — asserts renames has 2 entries even though the tx failed). This is a smaller/no-worse trade: the old window fired on any catastrophic-I/O rename failure (the same failure class, exercised N independent times); the new window requires guards to pass, N independent renames to ALL succeed, AND then a single subsequent DB commit to fail — a narrower, lower-probability compound event, matching the PR author's own reasoning (compensating rollback was rejected because the tx hard-deletes source books, and undoing a hard DELETE post-move-failure is riskier than the current design). Acceptable trade-off, adequately documented in both code comments and tests. Not a blocker.

Symlink guard (item 3): the os.Lstat symlink-skip guard (Security: reject symlinks...skip symlinked sources to restore the no-follow safety) is untouched by the reorder — still runs before the move attempt, same continue pattern. Confirmed via diff — this block was not touched at all.

Transaction integrity (item 4): runInTx still wraps reassignBookFilesRaw + hardDeleteBooksRaw atomically; the fileIDs/fullyReassignedBookIDs slices passed in are now correctly the post-move-success sets (previously they were the post-guard, pre-move sets). No change to transaction atomicity itself, only to what data feeds it — correct given the reorder.

Test quality (item 5): internal/books/attach_service_test.go is package books_test, curried DI via the existing AttachFiles(...) constructor, one-Expect-per-It (verified — each It in the changed contexts asserts a single value). New/changed assertions correctly flip to match the new invariant: a full-move-failure context now asserts reassignedFileIDs empty + hardDeletedIDs empty + FilesReassigned==0 + SourceBooksRemoved==0 (previously asserted the opposite, DB-committed values) — these WILL fail if the reorder regresses back to move-after-commit, since the fakes track call order via the stubbed renameFile/tx functions. The single-book retained-on-move-failure context and the tx-fails-after-move context are equally solid. No guarded/tautological assertions spotted in the diff hunks.

Downstream consumers: grepped all callers of AttachResult/MovesFailed/FilesReassigned (attach_handler.go, bulk_attach_by_filter_store.go, and their tests) — none assume the old "DB committed but file may be stranded" semantics; they only read/sum/log the counts, which remain correct under the new invariant. The removed "Phase 3: move physical files AFTER the transaction commits..." doc block and its "post-commit"/"two-phase guarantee" language were fully replaced consistently; no stale references to the old ordering remain anywhere else in the codebase.

Concurrency/idempotency/N+1 (item 6): No change to fan-out shape — still a single per-attach-request loop (not a per-library fan-out), no N+1 introduced. renameFile/mkdirAll call counts unchanged, just reordered relative to the tx.

Findings

No BLOCKER or MAJOR findings. This is a correct, well-tested, adequately-documented reorder that closes the originally reported data-integrity gap without introducing a worse or undocumented failure mode.

[MINOR] internal/books/attach_service.go — doc/comment rewrite (MoveFailure, AttachResult, AttachFiles doc block) fully replaces the stale "Phase 3"/"post-commit"/"two-phase guarantee" language with accurate move-then-commit wording; confirmed no stale references remain anywhere else in the repo. No action needed — noted for the record only.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review: PR #1461 (bookshelf-8l09k) Reviewed `internal/books/attach_service.go` diff against `origin/main`. Scope is exactly as described: move-then-commit reorder, no unrelated files touched, no `.golangci.yml`/coverage-exclusion changes. ### Phase 1: Spec compliance Matches the bead. The fix reorders the physical move to happen inside the per-file guard loop, BEFORE the DB transaction, and a failed move now does a plain `continue` (same as every other pre-existing guard failure: null-byte, symlink, containment, dest-exists, mkdir) — excluding the file from `fileIDs`/`bookFileReassignCount` entirely. ### Phase 2: Code quality **Invariant check (item 1):** Traced the loop in the new file (`git show origin/bd-bookshelf-8l09k:internal/books/attach_service.go`) — `fileIDs = append(fileIDs, row.FileID)` and `bookFileReassignCount[row.BookID]++` only execute after falling through the entire per-file guard block, including the new move attempt. On `mvErr != nil` the code hits `continue` before reaching that append. So DB reassignment is now strictly limited to files whose move succeeded (or move wasn't requested). `hardDeleteBooksRaw` is driven by `fullyReassignedBookIDs`, computed from the same `bookFileReassignCount`, so a source book with any failed-move file is correctly retained, never hard-deleted. Verified against the new tests (`does NOT reassign files in DB whose move failed`, `does NOT hard-delete source books whose file move failed`) — the invariant holds. **Residual window (item 2):** The new doc comment at the top of `AttachFiles` is honest about the inverted residual risk: if the DB tx itself fails/rolls back AFTER a physical move succeeded, the file is now on disk at the destination but the DB row still points at the source. This is documented, and a test explicitly locks it in (`has already moved the files on disk by the time the transaction fails` — asserts `renames` has 2 entries even though the tx failed). This is a smaller/no-worse trade: the old window fired on any catastrophic-I/O rename failure (the same failure class, exercised N independent times); the new window requires guards to pass, N independent renames to ALL succeed, AND then a single subsequent DB commit to fail — a narrower, lower-probability compound event, matching the PR author's own reasoning (compensating rollback was rejected because the tx hard-deletes source books, and undoing a hard DELETE post-move-failure is riskier than the current design). Acceptable trade-off, adequately documented in both code comments and tests. Not a blocker. **Symlink guard (item 3):** the `os.Lstat` symlink-skip guard (`Security: reject symlinks...skip symlinked sources to restore the no-follow safety`) is untouched by the reorder — still runs before the move attempt, same `continue` pattern. Confirmed via diff — this block was not touched at all. **Transaction integrity (item 4):** `runInTx` still wraps `reassignBookFilesRaw` + `hardDeleteBooksRaw` atomically; the `fileIDs`/`fullyReassignedBookIDs` slices passed in are now correctly the post-move-success sets (previously they were the post-guard, pre-move sets). No change to transaction atomicity itself, only to what data feeds it — correct given the reorder. **Test quality (item 5):** `internal/books/attach_service_test.go` is `package books_test`, curried DI via the existing `AttachFiles(...)` constructor, one-Expect-per-It (verified — each `It` in the changed contexts asserts a single value). New/changed assertions correctly flip to match the new invariant: a full-move-failure context now asserts `reassignedFileIDs` empty + `hardDeletedIDs` empty + `FilesReassigned==0` + `SourceBooksRemoved==0` (previously asserted the opposite, DB-committed values) — these WILL fail if the reorder regresses back to move-after-commit, since the fakes track call order via the stubbed `renameFile`/tx functions. The single-book retained-on-move-failure context and the tx-fails-after-move context are equally solid. No guarded/tautological assertions spotted in the diff hunks. **Downstream consumers:** grepped all callers of `AttachResult`/`MovesFailed`/`FilesReassigned` (`attach_handler.go`, `bulk_attach_by_filter_store.go`, and their tests) — none assume the old "DB committed but file may be stranded" semantics; they only read/sum/log the counts, which remain correct under the new invariant. The removed "Phase 3: move physical files AFTER the transaction commits..." doc block and its "post-commit"/"two-phase guarantee" language were fully replaced consistently; no stale references to the old ordering remain anywhere else in the codebase. **Concurrency/idempotency/N+1 (item 6):** No change to fan-out shape — still a single per-attach-request loop (not a per-library fan-out), no N+1 introduced. `renameFile`/`mkdirAll` call counts unchanged, just reordered relative to the tx. ### Findings No BLOCKER or MAJOR findings. This is a correct, well-tested, adequately-documented reorder that closes the originally reported data-integrity gap without introducing a worse or undocumented failure mode. [MINOR] internal/books/attach_service.go — doc/comment rewrite (MoveFailure, AttachResult, AttachFiles doc block) fully replaces the stale "Phase 3"/"post-commit"/"two-phase guarantee" language with accurate move-then-commit wording; confirmed no stale references remain anywhere else in the repo. No action needed — noted for the record only. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor referenced this pull request 2026-08-30 00:07:37 +00:00

flake(wfengine): extend tester timeout so deadlock-detection stops false-firing under CI load (bookshelf-v8o1o) #1462

zombor force-pushed bd-bookshelf-8l09k from 5b378c3055 to ab27f059d2 2026-08-31 01:26:02 +00:00 Compare

zombor merged commit 79a578ab8f into main 2026-08-31 01:34:34 +00:00

zombor referenced this pull request from a commit 2026-08-31 01:34:39 +00:00

Merge pull request 'fix(attach): only reassign DB after a successful physical move (bookshelf-8l09k)' (#1461) from bd-bookshelf-8l09k into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1461

No description provided.