Fork provenance maintenance: stale parent-index rows, drifting counter, dead merged_upstream_at #457
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#457
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?
Child of #449 (audit findings F3 Med comment 4558 + F8 Low comment 4564; first pass Finding 5 comment 4471 overlaps). (1) Deleting a fork child never removes its parent meta/forks.json row and never decrements the social counter (merge.go:897-918 append-only; zero removal writers repo-wide) — ghost forks, user-visible. (2) fork.json merged_upstream_at is documented (03_pull_requests.md:32, model.go:145-156) but has zero writers/readers. Fix: child-delete sweeps the parent row + counter (GC-safe via existing 404-skip), and either write merged_upstream_at on merge or remove the field/doc claim.
Fix ready for review: #468 (branch fix/issue-457). Child-delete sweeps parent row + counter post-linearization (GC-safe by order); merged_upstream_at now written on cross-fork merges (decision: write, not remove — one conditional PUT, zero trips same-repo). Table-driven tests + e2e, -race green, coverage pulls 95.9% / social 99.5%, vet/fmt clean, docs 03+07 updated.
PR #468 review (scratch worktree /tmp/pr468 @
83b0608, main worktree untouched, no browser — backend-only change, tests + reasoning):VERDICT: ready to merge (after my pushed fix — see below).
WHAT I VERIFIED
(1) Sweep correctness — UnlistFork (internal/pulls/forks.go:110): nil index = miss/no-write; absent row = miss/no-write/ETag-stable; Version++ only inside the found-branch; corrupt index errors (fail closed); empty-not-null '[]' on last-row removal (make() non-nil slice, pinned by exact-match test). DecForks (internal/social/service.go:385): field-scoped, floored at 0, absent object stays absent (no mint — asserted via GetBytes/IsNotFound), corrupt errors. Counter fires only when removed==true. All pinned by table tests.
(2) Ordering/GC — PROVEN. Sweep order is child-wipe (Registry.Delete linearizes) THEN row removal (serve.go:709-714). GC walk forkNetworkLive (internal/maintain/forknet.go:69-98) reads the parent index first, then probes each listed child manifest with 404-skip. Window state {row present, manifest gone} hits the existing 404-skip; post-removal the row is never queued. Existing TestForkNetworkGCMetaParent + TestE2E_ForkDeleteKeepsChildrenWorking still pass.
(3) merged_upstream_at — stamped in runMerge (merge.go:322) with the same mergedAt string assigned to pr.MergedAt (merge.go:279/308), gated pr.Head.Repo != pr.Base.Repo (same idiom as merge.go:136), zero trips same-repo (test asserts no fork.json minted). Missing child doc = narrated no-op; store failure narrates, never fails the landed merge. No reader added — documented in the 03 decision note with the N-trip rationale. Matches the issue's 'write' option.
(4) No-children/delete paths — ordinary deletes pay exactly one extra exact-key GET (forkParentOf pre-read); nil/absent/corrupt all read as no-parent, delete proceeds unswept. Delete is not a law-6 budgeted hot path (push/refs/checkpoint), so no budget impact; acceptable and tested ('plain repo is silent', 'corrupt fork.json is silent but deletes', 'nil callback is safe'). repoimport rollback calls wal reg.Delete directly (internal/repoimport/task.go:406), never the sweep — the doc claim checks out.
(5) Law 8 — internal/wal untouched (diff touches pulls/social/e2e/cmd-walhub/docs only); sweep lives in composition (buildCollab wiring, repoRegistry callback); maintain mirrors the index shape locally, no upward import. Lawful.
(6) Convergence — CAS loops everywhere (10-attempt unlist, 8-attempt dec); relist-after-sweep converges to exactly one row (TestUnlistForkRelistConverges); 8 racing Incs + 4 racing Decs land exactly at seed+N-M (TestDecForksConcurrentConverges).
(7) Gate — pulls 95.9%, social 99.5% (>=95); gofmt clean; go vet clean; go build OK; no go.mod/go.sum change (no new deps); docs 03 §7 + decisions and 07 §6 updated in-change (law 12).
ONE REAL BUG FOUND AND FIXED (pushed as
83b0608to origin/fix/issue-457):UnlistFork's 'removed' closure flag was set by ANY CAS attempt that saw the row, including a losing attempt whose write never landed (412 -> re-read finds row gone -> returns no-write but stale true). wal Registry.Delete is idempotent-success, so two concurrent DELETEs of the same child both reach the sweep and could double-decrement the counter for a single row removal. Fix: reset 'removed=false' at the top of each CAS invocation so only the landed write reports. Added deterministic regression test 'concurrent double sweep decrements once' (barrier-started pair, memory store enforces CAS): FAILS without the fix (decs = 2, want exactly 1 — reproduced), passes 30/30 -race with it.
NITS (non-blocking): 03 decision note says the stamp is 'one conditional PUT' — the CAS loop is 1 GET + 1 conditional PUT; merge task is not a budgeted path so leaving the wording.
TEST RESULTS (scratch worktree, generous timeouts): pulls -race ok; social -race ok; cmd/walhub -race ok (incl. new sweep table + stress x30); maintain -race ok; wal -race ok; e2e TestE2E_ForkChildDeleteSweepsProvenance + TestE2E_ForkDeleteKeepsChildrenWorking ok.
MERGE RECOMMENDATION: ready to merge.
Fixed by PR #468 (review clean + one double-decrement race fixed by reviewer with regression test; sweep ordering + convergence verified), merged. Closing.