Push path writes pack- prefixed manifest checksums (bucket contract violation + duplicate packs) #205

New issue

Closed

opened 2026-09-08 17:36:39 +00:00 by crueber · 3 comments

crueber commented

2026-09-08 17:36:39 +00:00

Owner

Found while investigating #203 (not its cause — separate issue, separate fix).

The wire contract (docs/go/02_storage_protobuf.md: PackRef.checksum = 'pack trailing SHA, hex; key = wal/.pack'; MASTER_RUST_SPEC §5.1: wal/<checksum>.pack) requires BARE-hex checksums. The push path violates it:

internal/server/bind_wal.go newLocalPack: sum := strings.TrimSuffix(name, '.idx') on git's on-disk pack-<hex>.idx keeps the pack- infix → PreparedPack.Checksum = 'pack-<hex>' → manifest entry + bucket key wal/pack-<hex>.pack. Worse, PackPath is built as packDir + '/' + checksum + '.pack' = pack-pack-<hex>.pack, which never exists for a fresh ingest → Stat fails → PackPath='' → uploadPack SILENTLY SKIPS the pack-body upload.
Same TrimSuffix-only derivation in internal/repoimport/task.go:345 and cmd/walhub/ops.go:439 (diff.New basenames are pack-<hex>.idx).
The maintain layer uniformly assumes bare checksums (checksumFromPackPath strips the infix, localPackState strips it, installPackFile prepends pack-, compact.go keep-list prepends pack-).

Live evidence (bucket of acme/demo): manifest lists THREE packs — pack-30d755…, pack-7a9ac6…, AND pack-pack-7a9ac6… (same bytes as the second, re-uploaded under a doubly-prefixed key after a materialized file was re-ingested as 'new'). Readers stay self-consistent (keys derive from stored checksums), but manifests bloat with duplicates, every push risks another prefix layer, and buckets are Rust-incompatible.

Fix sketch: strip the pack- infix at the three producers + build local paths as pack-<bare>.pack/.idx in newLocalPack. Readers are shape-agnostic (all derive from the stored checksum), so mixed old/new manifests keep working; a repair/fsck pass could dedupe + rewrite old entries.

Found while investigating #203 (not its cause — separate issue, separate fix). The wire contract (docs/go/02_storage_protobuf.md: PackRef.checksum = 'pack trailing SHA, hex; key = wal/<checksum>.pack'; MASTER_RUST_SPEC §5.1: `wal/<checksum>.pack`) requires BARE-hex checksums. The push path violates it: - `internal/server/bind_wal.go newLocalPack`: `sum := strings.TrimSuffix(name, '.idx')` on git's on-disk `pack-<hex>.idx` keeps the `pack-` infix → `PreparedPack.Checksum = 'pack-<hex>'` → manifest entry + bucket key `wal/pack-<hex>.pack`. Worse, `PackPath` is built as `packDir + '/' + checksum + '.pack'` = `pack-pack-<hex>.pack`, which never exists for a fresh ingest → Stat fails → PackPath='' → `uploadPack` SILENTLY SKIPS the pack-body upload. - Same TrimSuffix-only derivation in `internal/repoimport/task.go:345` and `cmd/walhub/ops.go:439` (diff.New basenames are `pack-<hex>.idx`). - The maintain layer uniformly assumes bare checksums (`checksumFromPackPath` strips the infix, `localPackState` strips it, `installPackFile` prepends `pack-`, compact.go keep-list prepends `pack-`). Live evidence (bucket of acme/demo): manifest lists THREE packs — `pack-30d755…`, `pack-7a9ac6…`, AND `pack-pack-7a9ac6…` (same bytes as the second, re-uploaded under a doubly-prefixed key after a materialized file was re-ingested as 'new'). Readers stay self-consistent (keys derive from stored checksums), but manifests bloat with duplicates, every push risks another prefix layer, and buckets are Rust-incompatible. Fix sketch: strip the `pack-` infix at the three producers + build local paths as `pack-<bare>.pack/.idx` in newLocalPack. Readers are shape-agnostic (all derive from the stored checksum), so mixed old/new manifests keep working; a repair/fsck pass could dedupe + rewrite old entries.

crueber referenced this issue

2026-09-08 17:39:28 +00:00

Tree fetch 404 toast on repos whose backend is healthy #203

crueber referenced this issue from a commit

2026-09-08 18:02:57 +00:00

Fix #205: bare-hex pack checksums at all producers

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

2026-09-08 18:03:04 +00:00

Fix #205: bare-hex pack checksums #207

crueber commented

2026-09-08 18:03:07 +00:00

Author

Owner

Fix open as PR #207 (branch fix/issue-205): bare-hex checksums at all three producers + upload-skip fixed + same-file install no-op. Readers verified shape-agnostic (no changes); repair/fsck pass decided out of scope (compaction converges, documented in 02). All -race suites green, cover gate holds.

crueber commented

2026-09-08 18:10:31 +00:00

Author

Owner

REVIEW PR #207 (fix/issue-205, commit bb74dd7) — PUSH path + bucket contract. Verdict: READY TO MERGE (no blocking issues; no code changes made by reviewer).

Walked in scratch worktree /tmp/pr207 (since removed); main worktree left clean.

SEMANTICS

Single-strip (maintain.go:24-32) correct: bare in→bare out; on-disk materialized legacy pack-pack-<hex>.idx strips one layer → pack-<hex> = stored checksum, never re-prefixed. Triple-prefixed on-disk names (hand-producible only; git never writes them) would claim a new self-consistent entry that compaction converges — acceptable, noted not fixed.
newLocalPack completeness (bind_wal.go:196-258): pack-less idx → nil cannot drop a legitimate pack — git/index-pack writes pack before idx, and the loop keeps scanning older complete pairs after skipping a stray. nil yields a refs-only entry vs the old hollow broken-manifest entry: strictly better.
Known-set (bind_wal.go:199-206): registers stored + bare-stripped forms. Walked legacy (pack-<hex> stored, materialized pack-pack-<hex> → known), bare (direct), and doubled-legacy cases — no republish storms; fresh bare content alongside legacy converges via compaction.
installPackFile same-file no-op (wal/publish.go:1036-1053): os.SameFile inode check is the correct condition; read-only (0444) preservation pinned by TestPublish_AddPackSameFileSkipsInstall incl. mode assertion.

READERS SHAPE-AGNOSTIC — enumerated, all derive from stored checksum: wal/reconcile.go presence/materialize/side-files/gcSuperseded-delete, maintain/compact.go keep list, revindex.go, rebuild.go, maintain/util.go localPackState (single-strip, consistent with helper), store.PackKey/IdxKey pure derivations. No hex/length validation of checksums in prod code. Mixed old/new manifests pinned by TestPublish_LegacyPrefixedChecksumReads (legacy materializes under pack-pack-<hex>, coexists + syncs with bare). Compaction supersede/delete keys derive from stored values, so prefixed keys are handled; repair/fsck deferral documented in 02 and sane (old entries self-consistent, live sets converge on bare).

UPLOAD-SKIP FIX verified by tests, not just code-read: TestReceivePackPushBareChecksums asserts wal/.pack+.idx exist in store + zero pack-\* keys under wal/ after two real HTTP receive-pack pushes; x_gaps FindIngestedPack now stats PackPath and asserts manifest + store keys.

X_GAPS UPDATES legitimate (strengthened, not weakened): x_gaps7 pins nil for pack-less idx (behavior change, correct); x_gaps8 writes complete pairs (adaptation, same skip-known-and-dirs coverage); x_gaps FindIngestedPack 'latent no-commit' fix genuine — seed fetch creates refs/heads/main locally so old err-only assert passed on Seq-0 per-ref conflict; new asserts res.Seq!=0 + bare manifest + store keys. packName gen→pack mirrors ingest canonical names.

GATES (scratch worktree, -race): git ok; wal ok; repoimport ok; maintain ok; cmd/walhub ok; server ok except pre-existing TestUIAssetConcepts env failure (missing make web concepts/*.gif — fails identically without this PR; PR touches no UI serving). Coverage: git 95.1 / wal 95.4 / server 95.6 / repoimport 95.7 / maintain 96.5 (≥95 holds); cmd/walhub 15.8% pre-existing (gate covers internal/... only). gofmt clean, go vet clean, no new non-stdlib imports (diff adds none). make e2e not run (noted as optional); HTTP-push + import e2e in-suite cover the paths with real git packs.

DOCS accurate: 02 wire-contract note, 04 single-strip rationale, 05 SameFile, 06 newLocalPack, 11 CLI, features/10 — all match the code. No main-worktree modifications.

REVIEW PR #207 (fix/issue-205, commit bb74dd7) — PUSH path + bucket contract. Verdict: READY TO MERGE (no blocking issues; no code changes made by reviewer). Walked in scratch worktree /tmp/pr207 (since removed); main worktree left clean. SEMANTICS - Single-strip (maintain.go:24-32) correct: bare in→bare out; on-disk materialized legacy `pack-pack-<hex>.idx` strips one layer → `pack-<hex>` = stored checksum, never re-prefixed. Triple-prefixed on-disk names (hand-producible only; git never writes them) would claim a new self-consistent entry that compaction converges — acceptable, noted not fixed. - newLocalPack completeness (bind_wal.go:196-258): pack-less idx → nil cannot drop a legitimate pack — git/index-pack writes pack before idx, and the loop keeps scanning older complete pairs after skipping a stray. nil yields a refs-only entry vs the old hollow broken-manifest entry: strictly better. - Known-set (bind_wal.go:199-206): registers stored + bare-stripped forms. Walked legacy (`pack-<hex>` stored, materialized `pack-pack-<hex>` → known), bare (direct), and doubled-legacy cases — no republish storms; fresh bare content alongside legacy converges via compaction. - installPackFile same-file no-op (wal/publish.go:1036-1053): os.SameFile inode check is the correct condition; read-only (0444) preservation pinned by TestPublish_AddPackSameFileSkipsInstall incl. mode assertion. READERS SHAPE-AGNOSTIC — enumerated, all derive from stored checksum: wal/reconcile.go presence/materialize/side-files/gcSuperseded-delete, maintain/compact.go keep list, revindex.go, rebuild.go, maintain/util.go localPackState (single-strip, consistent with helper), store.PackKey/IdxKey pure derivations. No hex/length validation of checksums in prod code. Mixed old/new manifests pinned by TestPublish_LegacyPrefixedChecksumReads (legacy materializes under `pack-pack-<hex>`, coexists + syncs with bare). Compaction supersede/delete keys derive from stored values, so prefixed keys are handled; repair/fsck deferral documented in 02 and sane (old entries self-consistent, live sets converge on bare). UPLOAD-SKIP FIX verified by tests, not just code-read: TestReceivePackPushBareChecksums asserts wal/<bare>.pack+.idx exist in store + zero `pack-\*` keys under wal/ after two real HTTP receive-pack pushes; x_gaps FindIngestedPack now stats PackPath and asserts manifest + store keys. X_GAPS UPDATES legitimate (strengthened, not weakened): x_gaps7 pins nil for pack-less idx (behavior change, correct); x_gaps8 writes complete pairs (adaptation, same skip-known-and-dirs coverage); x_gaps FindIngestedPack 'latent no-commit' fix genuine — seed fetch creates refs/heads/main locally so old err-only assert passed on Seq-0 per-ref conflict; new asserts res.Seq!=0 + bare manifest + store keys. packName gen→pack mirrors ingest canonical names. GATES (scratch worktree, -race): git ok; wal ok; repoimport ok; maintain ok; cmd/walhub ok; server ok except pre-existing TestUIAssetConcepts env failure (missing `make web` concepts/*.gif — fails identically without this PR; PR touches no UI serving). Coverage: git 95.1 / wal 95.4 / server 95.6 / repoimport 95.7 / maintain 96.5 (≥95 holds); cmd/walhub 15.8% pre-existing (gate covers internal/... only). gofmt clean, go vet clean, no new non-stdlib imports (diff adds none). make e2e not run (noted as optional); HTTP-push + import e2e in-suite cover the paths with real git packs. DOCS accurate: 02 wire-contract note, 04 single-strip rationale, 05 SameFile, 06 newLocalPack, 11 CLI, features/10 — all match the code. No main-worktree modifications.

crueber closed this issue

2026-09-08 18:11:04 +00:00

crueber referenced this issue from a commit

2026-09-08 18:11:04 +00:00

Merge pull request 'Fix #205: bare-hex pack checksums' (#207) from fix/issue-205 into main

crueber commented

2026-09-08 18:11:05 +00:00

Author

Owner

Fixed by PR #207 (review: strip semantics, completeness rule, reader agnosticism, no republish storms verified; all gates green), merged. Closing.

crueber added the

git storage