Fork post-share failure strands target name (needs manual bucket cleanup) #432
Labels
No labels
actions
bug
cli
duplicate
enhancement
fork
forum
git storage
help wanted
insights
invalid
issues
moderation
oidc
ownership transfer
packages
pr/merge protection rules
projects
pull requests
question
releases
sponsorships
tags
webhooks
wiki
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
crueber/walhub#432
Loading…
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?
Follow-up flagged by the #424 review (PR #431 findings). If the fork share succeeds but a later step (access bootstrap / provenance write) fails, the target repo name is permanently stranded: no data loss, but the name can't be reused without manual bucket cleanup. Commit order is share → access → provenance, with a pre-check/adopt gap in internal/pulls/merge.go. Three remediation options from the review: (1) reserve-then-commit with rollback deleting the shared prefix on failure; (2) adopt-or-fail the existing prefix on retry; (3) strand-and-report with a maintain sweeper. Pick one and implement.
1
Fix PR: #433 — chose remediation (1) reserve-then-commit with rollback (justification in the PR description). Ready for review; not merged.
Review PR #433 (fix/issue-432, reserve-then-commit + RollbackShare). Adversarial rollback-safety pass, verified in scratch worktree (removed afterward); main worktree untouched.
(1) Choice justified: YES. Adopt-or-fail (2) rejection is sound — empty-parent fork manifest is Registry.Create-shaped (HeadSeq 0/MinSeq 0/Rev 1), so an unprovenanced manifest is indistinguishable from a racing repo create; adopting risks hijacking a live prefix. Sweeper (3) has the same ownership-detection problem plus delayed reuse. Reserve-then-commit reuses the existing Create arbitration, no new machinery.
(2) Deletes ONLY attempt-owned keys: YES. RollbackShare (internal/pulls/forkexec.go:257) deletes exactly access.json (skipped when fork.json disputes the prefix) + checkpoint pair at cm.HeadSeq (skipped when HeadSeq==0, matching ShareManifest which writes none for empty parents) + manifest last. Exhaustive vs ShareManifest's Creates: checkpoints use PutCreate so success proves they are ours; manifest Create arbitrates. Never packs/parent keys/parent index; fork.json never deleted. One considered non-issue: access.json deleted without proving creation — but deletion only happens when manifest is provably ours (Rev 1), so no live repo exists to own it; worst case removes an orphan from an aborted creator.
(3) Ownership check: YES, fresh GET Repo==child AND Revision==1 (forkexec.go:291); absent=nil no-op, foreign/advanced=ErrConflict, corrupt=ErrCorrupt, fork.json GET fault=fail-closed passthrough — all refuse with zero deletes. Bystander suites (parent manifest/packs/access/fork.json/index, sibling manifest) asserted present. Residual note (not blocking): GET-then-DELETE is not version-guarded, so a manifest advance landing between the check and the deletes would still be deleted; no in-protocol writer can hit that window (fork.json uncommitted, index unlisted), but a Delete-with-IfVersion would close it fully.
(4) Racing create: YES. Adopted shares never roll back (sharedThisAttempt gate, merge.go:817; pinned by TestFork432AdoptedShareNeverRollsBack). Empty-parent byte-identical case safe: share-412 adopts ONLY with fork.json Parent==parentID, else 409 hands-off (merge.go:783-793). Post-delete arrivals win a clean prefix = intended reuse.
(5) GC coordination: YES. Rolled-back child is never in any parent meta/forks.json, and forkNetworkLive (internal/maintain/forknet.go:82-99) is index-driven with manifest-404 skip-subtree, so a racing pass cannot reference or resurrect it; GC only pins packs, never deletes manifests — no harmful interaction either direction.
(6) Retry reuses name: PROVEN (TestFork432AccessFailureRollsBackAndReusesName: rollback empties prefix incl. access.json, healed retry completes incl. index listing).
(7) Failure paths: ONE LEAK FOUND AND FIXED. Access-bootstrap, provenance-412 (own-adopt vs foreign/unreadable-409+rollback), provenance-write errors all wired. But the adoptedShare Root-backfill CAS failure (merge.go:888) returned without rollback — share-won + stale-own-adopt + backfill exhaustion stranded the reservation (the #432 class on one path). Fixed in
b4ef573(pushed to origin/fix/issue-432): rollback('provenance backfill') before return; no-op for pure adopts via the sharedThisAttempt guard. Post-fork.json-commit paths (parent index, counter, event) correctly do NOT roll back — committed-or-narrated by design. New TestFork432BackfillFailureRollsBackShare fails on unfixed code (manifest stranded) and passes with the fix.(8) Hygiene: gofmt clean, go vet clean, package cover 95.9% (>=95% gate), no new deps, docs updated in same change (docs/features/03_pull_requests.md Decisions, law 12), ### Concurrency subsection present (law 13_concurrency). Pre-existing flake noted: TestGetPRHeadDrift fails intermittently under -count=20 on main too (SSE stream timing, unrelated to this PR); all fork tests green at -count=20.
Final: pulls -race count=1 ok, pulls fork-only -race count=20 ok, maintain -race ok, go build ok.
MERGE RECOMMENDATION: ready to merge (after CI confirms
b4ef573).Fixed by PR #433 (review clean + one backfill rollback leak fixed by reviewer with regression test; prefix-scoping + retry proven), merged. Closing.