Audit: fork feature vs git directly — clone/fetch/push, object completeness, ref copying, parent deletion/GC, provenance (findings as comments) #449
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 project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
crueber/walhub#449
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?
What's requested
An audit of the fork feature (docs/features/03_pull_requests.md §7, issues #424/#432) against git's direct expectations: clone/fetch/push on a fork, object completeness, ref copying, parent deletion/GC, and provenance. The user observed that a fork's store prefix only seems to contain a checkpoint — this ticket answers whether that shape is correct and whether the feature holds up end to end. Findings are documented as comments on this ticket (one per finding, #331 convention). No code changes.
Scope & method (static, commit
527d425ca65e7325f2c1a8d4e2a244ed30a56df2)internal/pulls/forkexec.go(share + rollback),internal/pulls/merge.go(StartFork/runFork),internal/pulls/forks.go,internal/wal/forkread.go(read fallback),internal/wal/remote.go+internal/wal/reconcile.go(engine prefixes, materialize, remote-index),internal/maintain/forknet.go+compact.go(fork-network GC),cmd/walhub/forkshare.go(refs seam + summary projection),internal/api/summary.go(delete + fork projection/ETag),internal/wal/registry.go+cmd/walhub/serve.go(Delete path).Acceptance criteria
Finding 1 — ANSWER: "fork store only contains a checkpoint" is the designed shape, not data loss
Evidence.
internal/pulls/forkexec.goShareManifest(step 5–6): the fork gets exactly three objects under its own prefix — a refs snapshot (checkpoint_refs_<seq>.pb), a checkpoint (checkpoint_<seq>.pb), andmanifest.pbwithmin_seq = seq+1and an empty segment range. No pack bytes are copied: the manifest references the PARENT's pack checksums verbatim, and pack content is served by the read fallback ininternal/wal/forkread.go(sharedGet,downloadShared) plus the ancestorprefixeslist on the remote block reader (internal/wal/remote.goreadRaw, wired ininternal/wal/reconcile.goengineFor). The docs ratify this ("Fork pack sharing is by read fallback, not by key magic", docs/features/03_pull_requests.md Decisions section, law-12 amendment to §7).Severity. Non-finding (design). The observation is exactly what §7 as amended specifies: dedup by construction, one byte of shared pack stored once under the parent prefix.
Recommendation. None for the code. A docs nicety only: the fork create success page/summary could say "shares objects with " so the sparse prefix doesn't read as a broken fork. Filed here as the recorded answer, not a defect.
Finding 2 — Clone/fetch/push on a fresh fork holds up; connectivity is protected by materialization, but a push racing first materialization relies on the fallback path
Evidence. Serving uses the local materialized copy (
internal/wal/reconcile.gomaterialize→fetchPackFile→downloadShared/fetchSideFile, both fork-fallback aware). Push connectivity (internal/git/receive.goCheckConnectivity) runs over the serving copy, so history objects living only under the parent prefix resolve because materialization fetched them through the fallback. The remote-reader path (internal/wal/remote.goremoteEngine.readRaw) appends ancestor prefixes per block read; the block cache keys on the full store key so per-prefix entries stay correct.Severity. Design OK, with one residual risk: the fallback is failure-path-only and per-object. A fork whose parent has been compacted (old checksums superseded) depends on
forkNetworkLivehaving pinned those packs (see Finding 4); if the parent's GC ever races a fork's materialization, the child's fetch 404s mid-clone with no auto-repair (no re-share/re-sync mechanism re-resolves a fork's manifest to a fresh parent pack set — the child manifest is frozen at fork time,MinSeq = seq+1,LogSegments: nil).Recommendation. Follow-up ticket candidate: a "fork re-share" maintenance op that rewrites a child's manifest onto the parent's current pack set (the same ShareManifest machinery) when materialization records ancestor misses.
Finding 3 — Ref copying is complete and consistent: full live ref set + HEAD, but only branches
Evidence.
cmd/walhub/forkshare.goParentRefssnapshots the parent's entire live ref set at refs-sync level;ShareManifestcopies all of it into a freshRefSnapshot(sorted, deduped by contract) and defaults child HEAD to the parent'sHeadTarget(or the requestedbranch, existence-checked against the consistent snapshot —merge.gonormalizeForkBranchrejects tags/non-branch namespaces explicitly). Consistency is guarded:consistentParentre-reads the manifest around the refs read and retries while HeadSeq/Revision move (forkexec.go, bounded bymaxForkConsistentReads = 10), and pack closure is verified by HEAD on every shared pack before any child object is written (step 4, fail-loud).Severity. Design OK. Note, not a defect: tags and
refs/pull/*in the parent are NOT forked (branch-only input validation). Users forking to get tags will not have them; a PR referencing the fork head still works because PR heads are resolved through the fork's manifest per 03 §7.Recommendation. If tag forking is wanted later it is a scope change to
normalizeForkBranch/ForkRefselection — not a correctness bug today.Finding 4 — Parent-side GC is genuinely load-bearing and fail-closed; the documented delete hole is real and UNMITIGATED for parent deletion
Evidence (two halves).
internal/maintain/compact.gogcSupersededconsultsforkNetworkLive(internal/maintain/forknet.go) before deleting anything — a transitive, probe-capped (64) walk overmeta/forks.json+ children's manifests, failing closed on any doubt (transport error, corrupt manifest/index, probe-cap exhaustion). A pack referenced by any live network manifest is never deleted even when its own repo superseded it. Retention (retention_superseded, 7d) plus this rule means a compaction on the parent right after a fork does not brick the fork.Registry.Delete(internal/wal/registry.go) and the APIrepoDelete(internal/api/summary.go,cmd/walhub/serve.goadapter) have NO fork-network awareness — deleting a repo deletes EVERYTHING underrepos/<o>/<r>/, including the packs every fork's manifest references. The read fallback then fails with NotFound on each ancestor and the fork becomes uncloneable/unfetchable, with no error surfaced anywhere (the child's fsck will catch it on the next maintenance pass, too late). docs/features/03 §7 Decisions explicitly records this as a "Known hole, documented not fixed: deleting a fork-network member strands descendants" — so it is a RATIFIED deviation, not spec drift — but it is also the single biggest git-correctness gap in the feature:DELETE /api/v1/repos/{o}/{r}on a parent with children silently destroys them.Severity. Ratified deviation (cite, per #331 convention), but flagging severity as high: the guard the GC carefully builds (fail-closed, never delete a live fork's packs) is bypassed entirely by repo deletion, and the UI offers repo deletion with no fork-warning.
Recommendation. A delete-guard ticket:
repoDeletemust refuse (409, listing the live children frommeta/forks.jsontransitively) or offer cascade transfer, before the feature can be called git-complete.Finding 5 — Provenance is solid at creation but never maintained afterward: dead index rows, no
merged_upstream_atwriter, no counter decrementEvidence.
fork.json(Parent + Root + ForkedAt) is Create-once and adopted-on-retry with a Root backfill (merge.gorunFork); the Root pointer genuinely helps — the read fallback short-circuits dead intermediates via Root (forkread.goresolveForkChain). Good.meta/forks.jsonis append-only: nothing removes a row when a child is deleted, and nothing decrements the social counter (IncForksininternal/social/service.gohas no decrement caller). A deleted fork remains listed byGET …/forks(pulls/forks.goreads the index as the source of truth) and the parent's fork count/ETag stay inflated. The GC walk tolerates dead rows (manifest-404 → skip subtree) so this is list-correctness, not data safety.ForkDoc.MergedUpstreamAtis declared (pulls/model.go) and the doc says the CAS is formerged_upstream_at, but no code path ever writes it — the "sync upstream" implied by the model is unimplemented (honest as long as no UI promises it).cmd/walhub/forkshare.goforkSummaryOf) reads both objects and fails open to absent on store errors — display metadata can never fail the summary, matching the ETag~f<version>.<count>.<parent-hash>suffix ininternal/api/summary.go.Severity. Spec drift / unimplemented surface (index cleanup + counter decrement missing;
merged_upstream_atdeclared but dead).Recommendation. One follow-up ticket: reconcile the forks index and social counter on child deletion (delete-guard hook or a maintenance reconciliation pass), and either implement or explicitly retire
merged_upstream_atfrom the model/doc.Finding 6 — SUMMARY: fork feature is git-sound on the happy path; 1 non-finding answer, 1 high-severity ratified hole, 2 spec-drift items
Audited commit:
527d425ca65e7325f2c1a8d4e2a244ed30a56df2(main, fresh pull). Static audit — no code run or changed.repoDelete/Registry.Deletehas NO fork guard — deleting a parent strands all children (ratified deviation, high severity)merged_upstream_atdeclared but never writtenBottom line: the fork feature holds up against git directly for clone, fetch, and push on live networks; the checkpoint-only store prefix is correct by design. The gap that would bite an operator is parent deletion (#4), which the spec acknowledges but nothing enforces.
Scope guard honored: no code changes. Follow-up candidates above become tickets on the user's ruling per finding.
[F1 — High] Cross-fork PRs with fork-unique commits have no object bridge into the base serving copy (open 503s; mergeability/merge/diff fail)
Evidence (audited tree
e8bc499c):rev-list --objects --stdin --not --allin the base dir. A head object absent from the base serving copy is a git fatal (bad object, exit != 0), i.e. an error, not reachable==false.Why it breaks: pushes to a fork land under the child prefix. The read fallback (internal/wal/forkread.go) is child→ancestor only — the base handle can never see fork-unique objects. So any cross-fork PR whose head contains commits pushed after the fork point: open → 503 "reachability check"; diff/commits → 503; mergeable recompute → error; merge task → MergeBase/TrialMerge failure. Only heads fully contained in the shared pack set work, which contradicts §7 ("the PR records the fork-local head ref and the diff endpoint resolves through the fork; the merge task fetches nothing — shared packs make it local"). That sentence is true only for objects already in the shared set — precisely the objects a fork PR does not need help with. No test covers a cross-fork merge with post-fork commits (fork424/fork432 suites cover create/list/rollback only).
Recommendation: (a) treat rev-list object-errors as unreachable (not 503) for cross-fork heads at open/diffDir, so the fork-local path engages as spec'd; (b) run the merge computation where both object sets meet — e.g. trial-merge/commit-tree in headDir against the live base sha, or fetch-bridge the fork tip pack into a scratch dir — and pack the merge tip from there. Until then §7 should carry a known-limitation note that cross-fork merge requires the head to be base-contained.
[F2 — Medium] Deleting a parent repo has no fork guard: the whole prefix (including shared packs) is wiped while children still reference it
Evidence:
Effect: after a parent delete, live children 404 on every shared-pack read (fallback chain ends at a missing prefix; forkread.go:67-79 treats absent fork.json/objects as chain end). The fork-network GC rule is vacuous once the root is gone, and the Root-pointer dead-middle bypass only helps while the root prefix exists.
Status note: this is the documented hole, not a new discovery, and Forgejo issue #451 ("Fork-deletion safety… preserves the pack set as a meta repository and re-points children") already tracks the fix. Recording it here so the "parent deletion/GC" leg of this audit is explicitly answered.
Recommendation: implement the #451 design (tombstone/meta-repository + child re-point); until then, fail closed — refuse Delete when meta/forks.json lists children (or at minimum narrate the stranding in the delete response).
[F3 — Medium] Deleting a fork child leaves a stale row in the parent index (ghost forks; counter drifts; GC-safe by accident)
Evidence:
Why storage stays safe: internal/maintain/forknet.go:94-99 treats a child manifest 404 as "deleted, skipping subtree", so a ghost row pins nothing and cannot block a sweep. The failure is user-visible (stale list/count/ETag), not a safety hole.
Recommendation: add a delete-time seam (e.g. Service.UnlistFork called from the registry/serve Delete path) that CAS-removes the child row and best-effort-decrements the counter; or document a repair (
GET …/forksself-heals 404 rows on read). Either way, pin the ghost-list case in a test.[F4 — Medium] A crashed fork attempt wedges the target name: retry 412s on its own orphan checkpoint keys and 409s permanently
Evidence:
Sequence: attempt 1 Creates the checkpoint pair, then crashes (or is killed) before the manifest Create. No rollback runs — RollbackShare only fires on pre-commit failures within a live attempt (merge.go:816-825), and the Decisions note (03 §Decisions, #432 entry) explicitly lists "a process crash between the share and the rollback still strands the prefix". Attempt 2 re-runs ShareManifest, hits 412 on its own orphan checkpoint keys, finds no fork.json, and reports 409 taken — retryable never; only hand-deletion of the orphan keys unblocks it. The 409 message is also misleading (no fork exists).
Recommendation: on the share-412 path, when the child manifest is absent AND no fork.json claims the prefix, the occupying checkpoint keys are unreferenced garbage by definition (no manifest points at them) — delete exactly those keys and retry the share once, inside the executor. That keeps the fail-closed property (a foreign manifest still 409s) while making crash retries converge. Pin with a test that pre-places orphan checkpoint keys without a manifest.
[F5 — Low] RollbackShare can delete an access.json this attempt did not create
Evidence:
The rollback therefore proves ownership of the manifest (Repo==child, Revision==1 — forkexec.go:291-293) but assumes ownership of access.json. Window: any flow that plants access.json without winning the manifest Create (placeholder-create writes the eager access.json default; a hand-placed file; a previous crashed attempt whose manifest was since reaped) loses that file to an unrelated fork retry rollback. Narrow, and the file is re-synthesizable at read time, hence Low.
Recommendation: delete access.json only on proven creation — e.g. HEAD the key before EnsureRepoAccess and skip the delete when it pre-existed (or compare versions). One line of provenance tracking in runFork closes it.
[F6 — Low] forkChain handle-lifetime cache can go stale when Root is backfilled
Evidence:
Impact is degraded-only, not broken: Parent never moves, so the fallback still walks the full ancestry and finds every object; a stale chain only misses the Root shortcut (dead-middle bypass + ordering). The window needs a long-lived handle + a backfill landing mid-life + a deleted middle — rare cubed.
Recommendation: either version-guard the cache (conditional GET on fork.json; re-resolve on version change — cheap, failure-path only) or narrow the comment to "Parent never moves; Root may be backfilled once" so the staleness is a documented tolerance, not an assertion.
[F7 — Low] GC probe cap (~32 children) defers a popular parent sweep indefinitely; "one level per pass" comment is stale
Evidence:
Recommendation: (a) emit a distinct notice/metric on cap deferral (it is a capacity signal, not an error); longer-term, page the walk across passes with a persisted cursor instead of restarting from the parent each time. (b) Fix the two comments to "transitively within the probe cap".
[F8 — Low] fork.json merged_upstream_at is a dead field: documented but never written
Evidence:
So provenance today is {parent, root, forked_at} and nothing ever records an upstream merge. Either a planned seam that never landed or a leftover from the pre-implementation spec.
Recommendation: decide and document — either wire it (stamp it when a cross-fork PR merges, next to the merged event in runMerge) or amend the §7 table + ForkDoc comment to drop the field (pre-1.0: delete the shape, no shim). As-is the doc over-promises provenance the bucket never contains.
Answer to the checkpoint observation: DESIGN, not defect.
A freshly forked child prefix should contain (almost) only a checkpoint. Per the ratified law-12 amendment (docs/features/03 §Decisions, "Fork pack sharing is by read fallback"), the fork writes under its own prefix: manifest.pb (parent pack set referenced verbatim by checksum, min_seq=seq+1, empty segments — internal/pulls/forkexec.go:198-221), the checkpoint pair at the shared seq (fresh refs snapshot + checkpoint — forkexec.go:166-195), fork.json provenance, and access.json. Zero wal/*.pack bytes are copied — that is the point: if bytes were copied, the fork-network GC rule would be vacuous.
Reads resolve through the ancestry fallback (internal/wal/forkread.go): materialization (downloadShared/sharedGet — reconcile.go:199-257), the remote-index build (reconcile.go:452), and the remote block reader (remote.go:357-379, per-prefix cache keys) all try the own prefix first and walk Parent+Root outward only on miss. An inspector listing the child prefix sees manifest + checkpoint + small JSON and no packs — exactly the reported observation, and exactly the intended shape. The fallback runs on the failure path only, so hot paths are unchanged when objects are local (law 6).
Caveats from this audit that bound the "by design" verdict: the fallback is child→ancestor only, so the reverse leg (base serving fork-unique objects, needed for cross-fork PR open/merge/diff) is missing — see F1. And deleting the parent strands children (documented hole, #451) — see F2. Within those bounds, checkpoint-only is correct.
Audit summary (static, read-only; no code changes; worktree untouched).
Audited SHA:
e8bc499cad(origin/main at audit time).Pinned SHA in ticket:
527d425ca6.Drift: 12 commits, all outside the scope paths (SolidJS pill/header/mention UI #438-#447 plus identity/issues mention-tok follow-ups).
git log 527d425..HEAD -- internal/pulls internal/wal internal/maintain internal/api cmd/walhub docs/features/03_pull_requests.mdis empty — every file in the ticket scope is byte-identical between the pinned and audited commits. Findings below apply equally to both.Findings posted as individual comments: 8 total — 1 High (F1), 3 Medium (F2-F4), 4 Low (F5-F8), plus the checkpoint-observation answer above.
Areas verified clean (no finding): fork clone/fetch — materialize, side-file fetch, remote-index build, and remote block reads are all fork-aware with own-prefix-first semantics and per-prefix cache keys; push to a fork is an ordinary publish under the child prefix; the refs snapshot copy is tear-protected (consistentParent manifest→refs→manifest + closure HEADs); empty-parent forks take the Registry.Create shape; the maintain sweep is fail-closed (transport/corrupt/cap doubt aborts with nothing deleted; manifest-404 skips); rollback ownership (Repo==child && Revision==1, never packs/parents); single-flight + Create-arbitration on share/provenance/index; summary fork projection with ~f ETag suffix under the mutable-collab class. The §7 law-12 "sharing is by read fallback" amendment is cited as normative, not re-reported.
Unverifiable statically (per ticket rule, no reproduction): whether stock-git rev-list actually exits non-zero (vs printing missing) for absent objects in F1's exact argv — asserted from git semantics, worth one live check when F1 is worked; and any runtime-only behavior (timings, stores beyond the contract).
Audit findings converted to child issues: #456 (F1 cross-fork object bridge, High), #457 (F3+F8 provenance maintenance), #458 (F4+F5 failure-path residue), #459 (F6 chain cache), #460 (F7 probe cap); F2 is already #451. First-pass comments 4466-4472 overlap on checkpoint/GC/provenance — no separate tickets needed for those. This ticket closes once all children are closed.
All children closed: #451 (deletion safety), #456 (object bridge), #457 (provenance), #458 (residue), #459 (chain cache), #460 (probe cap). Closing the audit parent.