fix: capture transitions roll back the destination on a failed source remove (iss-186) - #203
Merged
Merged
Conversation
… remove (iss-186) commitTransition (internal/core/capture/workflow.go) wrote the destination file before removing the source. A non-ENOENT os.Remove(src) failure — EPERM/EROFS/EIO, e.g. an immutable attribute or a read-only remount on the source status dir — returned that error after dst had already landed, with no rollback. The issue id ended up present in both status directories at once (e.g. open/ and resolved/), and findIssue (alloc.go) rejects any id present in more than one file as ErrDuplicateIssueID — so the strand could never again be resolved or wontfixed without a human manually deleting one of the two copies. List/Status also double-counted it, since scanLedger does not dedupe. Fixed by rolling dst back (best-effort os.Remove(dst)) whenever the source remove fails for a reason other than "already gone", restoring the pre-call state so a retry is all that's needed once the underlying failure clears. A failed rollback is folded into the returned error rather than swallowed. A non-ENOENT remove failure has no portable way to trigger deterministically in a test — immutable attributes and read-only remounts are Linux/ext4- and permission-model-specific, and this repo's CI runs macOS and Linux — so a test-only seam (removeSourceHook, nil in production) lets a test force it, mirroring the existing beforeOrphanRemoveHook pattern already in alloc.go. Repro: internal/core/capture/workflow_test.go, TestTransitionRemoveFailureDoesNotStrandIssueInTwoDirs, watched failing on pre-fix code for the claimed reason (destination not rolled back) and passing after. Assisted-by: Claude:claude-opus-5
Moves iss-186 open/ -> resolved/ via abcd capture resolve, appends the DECISIONS.md round entry, and adds the CHANGELOG Fixed entry for the capture-transition rollback fix. Assisted-by: Claude:claude-opus-5
…e-transition-remove-rollback
….2 merge Merging main brought in the v0.4.2 release cut, which renamed [Unreleased] to a dated [0.4.2] section concurrently with this branch's own append to [Unreleased]. CHANGELOG.md's merge=union driver resolved the merge with no conflict markers, but a textual union has no notion of section boundaries: it placed the iss-186 entry inside the already-tagged [0.4.2] section instead of the (now empty) [Unreleased] one, mis-attributing an unreleased fix to a release that shipped without it. Moved the entry back under [Unreleased]. Assisted-by: Claude:claude-opus-5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes iss-186 (minor), surfaced during a bug-hunt loop sweep (round 1) and independently verified with a failing test.
The bug
commitTransition(internal/core/capture/workflow.go) writes the destination file before removing the source. A non-ENOENTos.Remove(src)failure — EPERM/EROFS/EIO, e.g. an immutable attribute or a read-only remount on the source status dir — returned that error afterdsthad already landed, with no rollback ofdst.The issue id ended up present in both status directories at once (e.g. both
open/andresolved/).findIssue(internal/core/capture/alloc.go) rejects any id present in more than one file asErrDuplicateIssueID, so the strand could never again be resolved or wontfixed — recovery required a human to manually delete one copy.List/Statusalso double-counted the id, sincescanLedgerdoes not dedupe.The fix
commitTransitionnow rolls the destination back (best-effortos.Remove(dst)) whenever the source remove fails for a reason other than "already gone", restoring the pre-call state (source present, destination absent) so a retry is all that's needed once the underlying failure clears. A failed rollback is folded into the returned error rather than swallowed.A non-ENOENT remove failure has no portable way to trigger deterministically in a test — immutable attributes and read-only remounts are Linux/ext4- and permission-model-specific, and this repo's CI runs macOS and Linux — so a test-only seam (
removeSourceHook, nil in production) lets a test force it, mirroring the existingbeforeOrphanRemoveHookpattern already inalloc.go.Evidence
internal/core/capture/workflow.go—removeSourceHook, rollback logic incommitTransitioninternal/core/capture/workflow_test.go—TestTransitionRemoveFailureDoesNotStrandIssueInTwoDirs, watched failing on pre-fix code for the claimed reason (destination not rolled back) and passing after.abcd/work/issues/resolved/iss-186-capture-transition-remove-failure-strands-issue.md— resolved viaabcd capture resolve.abcd/work/DECISIONS.md,CHANGELOG.md— updatedAn independent adversarial correctness review of the full diff (fresh subagent, no context from the fix) found the shipped rollback logic correct — no data loss possible (rollback only ever removes the file this same call just wrote, since a pre-existing file at that path would have already tripped
ErrDuplicateIssueIDbeforecommitTransitionran), no brokenerrors.Is/errors.Aschains, no race from the package-level test hook (not.Parallel()in this package, hook resets viadefer), and no other move path in the codebase (spec/store.go,intent/lifecycle.go) shares this two-copy window since both use a singleos.Rename. It flagged one real gap — the test's error assertion (err == nil) didn't confirm the injected failure was what surfaced, so an unrelated earlier error could make the test pass without exercising the rollback at all — fixed by assertingerrors.Is(err, injected). Verdict: MERGE.make preflightgreen;gofmt -l .clean;go run ./cmd/record-lint0 blockers.