[omp major-4] Failed import wedges target: committed manifest, no access.json, un-actionable 409 #79

New issue

Closed

opened 2026-09-05 00:15:04 +00:00 by crueber · 3 comments

crueber commented

2026-09-05 00:15:04 +00:00

Owner

[omp major-4] Failed import wedges target: committed manifest, no access.json, un-actionable 409

internal/repoimport/task.go:109-125 vs :135-231, service.go:210-215: reg.Create (manifest CAS commit point) runs BEFORE pack ingest, ref publish, repack, ensureImporterAdmin, writeImportDoc. Any later failure (413 pack at :139-141, publish/publishPack/repack/doc errors) leaves a repo with partial packs, zero refs, NO importer-admin binding (access.json lands at :208) under the allow-all absent-policy default. Retry: manifest-present + no import.json → 409 "delete and retry" (:213-214) — but the importer may lack delete rights (write ≠ delete on org targets). Nobody can complete or remove the import.

Fix

Make failure non-wedging: commit the manifest LAST (after ingest+refs+admin+doc succeed), or roll back the manifest on failure, or grant the retry path (resume-to-complete for the same canonical source, or allow the importer to delete/repair their own wedged target). Recommended: manifest commit last + same-source resume; keep "delete and retry" only for genuinely foreign manifests (B3 semantics). Deterministic fault-injection regression tests (fail at ingest/admin/doc → retry succeeds; state never wedged). Coverage gate holds; doc Decisions entry in 10 (law 12).

Acceptance criteria

No failure sequence leaves an uncompletable, undeletable-by-caller target.

# [omp major-4] Failed import wedges target: committed manifest, no access.json, un-actionable 409 `internal/repoimport/task.go:109-125` vs `:135-231`, `service.go:210-215`: `reg.Create` (manifest CAS commit point) runs BEFORE pack ingest, ref publish, repack, `ensureImporterAdmin`, `writeImportDoc`. Any later failure (413 pack at `:139-141`, publish/publishPack/repack/doc errors) leaves a repo with partial packs, zero refs, NO importer-admin binding (`access.json` lands at `:208`) under the allow-all absent-policy default. Retry: manifest-present + no import.json → 409 "delete and retry" (`:213-214`) — but the importer may lack delete rights (write ≠ delete on org targets). Nobody can complete or remove the import. ## Fix Make failure non-wedging: commit the manifest LAST (after ingest+refs+admin+doc succeed), or roll back the manifest on failure, or grant the retry path (resume-to-complete for the same canonical source, or allow the importer to delete/repair their own wedged target). Recommended: manifest commit last + same-source resume; keep "delete and retry" only for genuinely foreign manifests (B3 semantics). Deterministic fault-injection regression tests (fail at ingest/admin/doc → retry succeeds; state never wedged). Coverage gate holds; doc Decisions entry in 10 (law 12). ## Acceptance criteria - [ ] No failure sequence leaves an uncompletable, undeletable-by-caller target.

crueber referenced this issue from a commit

2026-09-05 02:22:51 +00:00

Fix #79: commit manifest provisionally — import.json claim + same-source resume

crueber referenced this issue from a pull request that will close it,

2026-09-05 02:22:55 +00:00

Fix #79: commit manifest last + same-source resume #89

crueber commented

2026-09-05 02:22:59 +00:00

Author

Owner

Fix ready for review: PR #89 (#89) — claim-first + same-source resume-to-complete, provisional manifest with rollback, 409 kept for genuinely foreign manifests only. No failure sequence leaves a caller-unfixable target; concurrent same-target imports still elect one winner (three CAS points documented). Tests: fault injection at ingest/admin/doc, Begin matrix, concurrency; 95.9% coverage, -race green.

Fix ready for review: PR #89 (https://git.packden.us/crueber/walhub/pulls/89) — claim-first + same-source resume-to-complete, provisional manifest with rollback, 409 kept for genuinely foreign manifests only. No failure sequence leaves a caller-unfixable target; concurrent same-target imports still elect one winner (three CAS points documented). Tests: fault injection at ingest/admin/doc, Begin matrix, concurrency; 95.9% coverage, -race green.

crueber referenced this issue from a commit

2026-09-05 02:29:42 +00:00

Review #89: resume runs own no claim in pre-commit drain guard

crueber commented

2026-09-05 02:29:53 +00:00

Author

Owner

PR #89 review (bf807ed, reviewer fixes pushed to origin/fix/issue-79):

VERDICT: ready to merge.

WALKED:

Claim-takeover safety (doc.go resolveClaim/claimExpired): takeover needs expired lease AND manifest absent AND version-checked PutUpdate (IfVersion, one bounded retry, re-read re-decides — no ABA; memory versions are counter-unique, filesystem stat-tokens backed by content re-check). A live import past the claim window always has a manifest, so it can never look takeover-eligible. Pre-commit window is bounded by clone_timeout (1800s) inside a 2100s lease (clone_timeout+git_timeout), leaving a 5min skew margin; wall-clock expiry is fine at NTP-scale skew. Undated/unparseable leases never expire (fail closed). Genuine.
Exactly-one-winner: 8-way TestIssue79ConcurrentSameTarget asserts 1 leader + 1 shared id via the mu-serialized single-flight (in-instance); cross-instance double-leaders converge via the three CAS points (claim Create / manifest Create / completion Update). Test is genuine (all Begins land while the leader clones; -count=10 green).
Rollback ownership: FOUND + FIXED one hole — the pre-commit drain/cancel guards passed ownedClaim=true unconditionally, so a drain landing in a RESUME run's claim window deleted the shared claim while the prior manifest survived, re-wedging the retry on a foreign-manifest 409. Now mode==claimFresh (matches rollbackImport's own 'resume runs own neither' contract). All other sites already correct (completeBody failure, resume-open arms); claim delete is version-checked so a concurrent takeover is never removed under.
Divergent refs: create-only (OldOid=zero) + pre-check abort 409; TestIssue79DivergentRefAborts covers it. (Note: HEAD symref update carries no OldOid — negligible, HEAD is source-derived and data-ref divergence 409s first.)
Resume-converge: same manifest version asserted (TestIssue79AdminFailureResumes); packs presence-probed, refs create-or-skip. Re-repack on resume may add a superseded tier-2 base — content-addressed, benign; doc residual broadened to say so.
B3 preserved: Begin matrix verified — manifest+no sidecar 409 foreign, manifest+complete+same-source no-op, in-progress+same-source 202 resume, live foreign 409, expired+manifestless 202 takeover.
'Manifest last impossible': TRUE — verified reg.Create fuses manifest PutCreate with handle construction (registry.go:211) and AddPack/Publish/FullRepack are handle methods; Open requires an existing manifest. No way to stage content without the commit.
Residuals now honest: added pre-fix manifest-without-sidecar as 409-by-design (indistinguishable from foreign) + broadened pack-dup residual.
Law 1: only new import is stdlib time; no go.mod/web changes. CAS discipline throughout; no lock held across I/O (probe outside mu); resolveClaim bounded (retried flag).

TESTS (scratch worktree /tmp/pr89, origin/fix/issue-79 + review commit): go test -race ./internal/repoimport/... ok (27s); coverage 95.8% (gate 95%); new TestIssue79DrainRefusalKeepsResumeClaim verified to FAIL pre-fix ('shared claim deleted') and PASS post-fix; Issue79 + claim/race units -count=10 green; gofmt/vet clean. web/dist absent in scratch worktree so cmd/walhub vet's embed error is a pre-existing worktree artifact (PR touches no web surface).

PR #89 review (bf807ed, reviewer fixes pushed to origin/fix/issue-79): VERDICT: ready to merge. WALKED: - Claim-takeover safety (doc.go resolveClaim/claimExpired): takeover needs expired lease AND manifest absent AND version-checked PutUpdate (IfVersion, one bounded retry, re-read re-decides — no ABA; memory versions are counter-unique, filesystem stat-tokens backed by content re-check). A live import past the claim window always has a manifest, so it can never look takeover-eligible. Pre-commit window is bounded by clone_timeout (1800s) inside a 2100s lease (clone_timeout+git_timeout), leaving a 5min skew margin; wall-clock expiry is fine at NTP-scale skew. Undated/unparseable leases never expire (fail closed). Genuine. - Exactly-one-winner: 8-way TestIssue79ConcurrentSameTarget asserts 1 leader + 1 shared id via the mu-serialized single-flight (in-instance); cross-instance double-leaders converge via the three CAS points (claim Create / manifest Create / completion Update). Test is genuine (all Begins land while the leader clones; -count=10 green). - Rollback ownership: FOUND + FIXED one hole — the pre-commit drain/cancel guards passed ownedClaim=true unconditionally, so a drain landing in a RESUME run's claim window deleted the shared claim while the prior manifest survived, re-wedging the retry on a foreign-manifest 409. Now mode==claimFresh (matches rollbackImport's own 'resume runs own neither' contract). All other sites already correct (completeBody failure, resume-open arms); claim delete is version-checked so a concurrent takeover is never removed under. - Divergent refs: create-only (OldOid=zero) + pre-check abort 409; TestIssue79DivergentRefAborts covers it. (Note: HEAD symref update carries no OldOid — negligible, HEAD is source-derived and data-ref divergence 409s first.) - Resume-converge: same manifest version asserted (TestIssue79AdminFailureResumes); packs presence-probed, refs create-or-skip. Re-repack on resume may add a superseded tier-2 base — content-addressed, benign; doc residual broadened to say so. - B3 preserved: Begin matrix verified — manifest+no sidecar 409 foreign, manifest+complete+same-source no-op, in-progress+same-source 202 resume, live foreign 409, expired+manifestless 202 takeover. - 'Manifest last impossible': TRUE — verified reg.Create fuses manifest PutCreate with handle construction (registry.go:211) and AddPack/Publish/FullRepack are handle methods; Open requires an existing manifest. No way to stage content without the commit. - Residuals now honest: added pre-fix manifest-without-sidecar as 409-by-design (indistinguishable from foreign) + broadened pack-dup residual. - Law 1: only new import is stdlib time; no go.mod/web changes. CAS discipline throughout; no lock held across I/O (probe outside mu); resolveClaim bounded (retried flag). TESTS (scratch worktree /tmp/pr89, origin/fix/issue-79 + review commit): go test -race ./internal/repoimport/... ok (27s); coverage 95.8% (gate 95%); new TestIssue79DrainRefusalKeepsResumeClaim verified to FAIL pre-fix ('shared claim deleted') and PASS post-fix; Issue79 + claim/race units -count=10 green; gofmt/vet clean. web/dist absent in scratch worktree so cmd/walhub vet's embed error is a pre-existing worktree artifact (PR touches no web surface).

crueber closed this issue

2026-09-05 02:30:08 +00:00

crueber commented

2026-09-05 02:30:09 +00:00

Author

Owner

Fixed by PR #89 incl. review-found resume-claim rollback fix, merged. Closing.

crueber referenced this issue from a commit

2026-09-05 02:30:09 +00:00

Merge pull request 'Fix #79: commit manifest last + same-source resume' (#89) from fix/issue-79 into main

crueber referenced this issue

2026-09-09 18:39:43 +00:00