gabriel / musehub public
Closed #134 Bug
filed by gabriel human · 55 days ago

Clone corruption: push silently substitutes {} for an unresolvable external parent snapshot

0 Anchors
Blast radius
Churn 30d
0 Proposals

Clone corruption: push silently substitutes {} for an unresolvable external parent snapshot

Background

aaronrene reported being blocked from starting musehub#84 — cloning gabriel/musehub either produced "no commits" or a truncated working tree, matching the exact failure mode already tracked in muse#35 (clone exits 0 but writes far fewer commits than the remote has). Reproduced directly against staging.musehub.ai with real instrumentation.

Confirmed, traced to exact lines — not assumed

Reproducing muse clone https://staging.musehub.ai/gabriel/musehub today: 1185/1444 commits land; 9 snapshots fail client-side hash verification (⚠️ apply_mpack: snapshot ... hash mismatch (reconstructed=...) — skipped), and every commit downstream of those 9 (~260 commits, including everything pushed this session) gets dropped as "parent not in mpack or local store".

Pulled one of the 9 failing snapshots (sha256:ff9c4d233...) directly from the real staging RDS database (not the abandoned local musehub_postgres container on the EC2 box — that was a dead end, see below). Its manifest, as actually served by /fetch/mpack, does not reproduce its own snapshot_id under any directories variant (stored, derived, or none). The manifest is missing every file that should have been inherited from its parent snapshot.

Root cause, in musehub/services/musehub_wire_push.py:

# lines 715-729
_external_parent_sids = {
    _sd.get("parent_snapshot_id")
    for _sd in _raw_snaps
    if _sd.get("parent_snapshot_id") and _sd.get("parent_snapshot_id") not in _snap_ids_in_mpack
}
_parent_snap_manifests: dict[str, dict] = {}
if _external_parent_sids:
    _psnap_rows = (await session.execute(
        select(MusehubSnapshot.snapshot_id, MusehubSnapshot.manifest_blob)
        .where(MusehubSnapshot.snapshot_id.in_(_external_parent_sids))
    )).all()
    for _psid, _pblob in _psnap_rows:
        if _pblob:
            _parent_snap_manifests[_psid] = dict(_msgpack.unpackb(_pblob, raw=False))

# line 764
_parent_base = _parent_snap_manifests.get(_parent_sid) or {}

When a pushed snapshot's parent is not included in this push's own payload (an "external" parent — already on the server from an earlier push), the server looks it up by manifest_blob alone. Only root/head snapshots of a given push batch ever get a manifest_blob (see lines 786-798) — every other snapshot is stored delta-only (manifest_blob=NULL, reconstructible only by walking delta_blob + parent_snapshot_id). If the external parent is delta-only, or its row is missing outright, _pblob is falsy, the snapshot_id is never added to _parent_snap_manifests, and line 764 silently falls back to {}. The child's delta then gets applied on top of nothing — the new manifest permanently and irrecoverably drops every inherited file, and is written to the DB in that broken state. No later read-side fix can repair it; the wrong bytes are already committed to manifest_blob.

_reconstruct_manifest (the delta-chain walker in musehub_wire_shared.py) is imported into this file (line 51) but never called here — the exact tool needed to fix this already exists elsewhere in the codebase and just isn't wired into the write path.

The corrupted snapshot's parent is a total phantom — even worse than delta-only

For the specific snapshot reproduced above, its parent_snapshot_id (sha256:1fcf68ea...) has no row at all in musehub_snapshots, and no musehub_commits row has that value as its own snapshot_id either — checked directly against the real RDS instance. It isn't merely delta-only; it never existed (or was later deleted, e.g. by a GC job, while still referenced). How this specific phantom reference came to exist is not yet root-caused and may be a second, distinct bug (or GC-vs-reachability bug) — out of scope for the fix below, which must hold regardless: the server must never silently persist a manifest it knows is missing data, whether the parent is delta-only or fully absent.

A regression test already exists for this exact bug — and is a false positive

tests/test_push_delta_only_parent_manifest.py::test_child_of_delta_only_parent_keeps_complete_manifest sets up precisely this topology (push 1: A→B→C, push 2: branch off B, add D) and currently passes. It doesn't actually exercise the vulnerable code path: muse push computes have from the literal remote branch-tip commit IDs only (walk_commits's prune=lambda cid: cid in have_set — no ancestor-closure expansion, confirmed in muse/core/mpack.py). Since main's tip is C, not B, pushing feat (which forks at B) does not prune at B — the walk continues past it, and A+B get resent as part of push 2's own payload. That makes B an in-payload sibling, not an external parent, so the buggy _parent_snap_manifests path is never reached. The test provides false confidence; this is why the bug has persisted through multiple prior "fix" landings (see symbol-log on _snap_row_to_wire_s3, May 29 – June 13, all read-side fixes for a write-side bug).

Also ruled out this session

  • Staging Alembic migration state: current (0074, matches dev) — checked against the real RDS instance directly, not the abandoned local musehub_postgres container.
  • muse code migrate (client-side ID-format rewrite): doesn't apply — that command operates on a local .muse/ working directory, not MuseHub's server-side Postgres rows.

Also found and cleaned up (unrelated but discovered during this investigation)

musehub_postgres + musehub_runner on the staging EC2 instance (i-07547cd20bee2dea5) were a legacy docker-compose stack predating the RDS/ECR blue-green deploy, doing nothing for the live site (musehub_runner was stuck in a DNS-resolution-failure loop). Backed up via pg_dump (verified checksum, saved to gabriel's machine) and stopped — not deleted; the Docker volume is untouched and the stop is fully reversible.

Goal

  1. Fix wire_push_unpack_mpack's external-parent resolution: when a parent's manifest_blob is NULL, fall back to delta-chain reconstruction (_reconstruct_manifest_validated or equivalent) instead of {}. When reconstruction itself fails (phantom/unrecoverable parent), reject the push loudly rather than persist a manifest that will never hash-verify.
  2. Replace or fix test_push_delta_only_parent_manifest.py so it actually forces the external-parent path (e.g. call wire_push_unpack_mpack directly with a hand-built payload, bypassing muse push's have-negotiation) — a CLI-level integration test cannot reliably force this topology given how have is computed.
  3. Decide whether to repair the already-corrupted rows on staging once the write path is fixed (separate follow-up — this ticket is the write-path fix itself).

Out of Scope

  • Root-causing how the specific phantom parent (1fcf68ea...) came to be referenced with no backing row — possibly a GC/reachability bug, tracked separately if it recurs after this fix lands.
  • Repairing already-corrupted snapshot rows on staging.
  • musehub#131 (wire push has no repo-authorization check) — unrelated class of bug, found in an earlier session.
Activity2
gabriel opened this issue 55 days ago
gabriel 55 days ago

Theory confirmed cheaply — no more staging round-trips needed. Tested and fixed on branch fix/wire-push-external-parent-manifest.

How the theory was tested: a CLI-level muse push integration test can't reliably force the external-parent path — have negotiation is a literal branch-tip match with no ancestor-closure expansion, so a real second push resends the whole chain back to the nearest tip, masking the bug (this is exactly why test_push_delta_only_parent_manifest.py already existed and already passed without catching it). Instead, called wire_push_unpack_mpack directly with a hand-built wire payload containing only the new commit — full control over what's "external," no staging access, no CLI subprocess, ~1.5s per test.

Result: confirmed the theory on the first try (external=1 loaded=0 in the server's own instrumentation), then fixed it:

  1. When an external parent's manifest_blob lookup misses, reconstruct via _reconstruct_manifest_validated (delta-chain walk) instead of defaulting to {}.
  2. Hash-verify every resolved snapshot against its own snapshot_id before writing — a genuinely unrecoverable parent (the phantom-reference case found on staging) now rejects the push loudly instead of silently persisting unverifiable data.

TDD: tests/test_wire_push_external_parent_reconstruction.py, 2 tests, red-first, both green. Full wire-push regression sweep: 211 passed (42 direct + 169 broader mpack/push/ intel suite).

Not yet merged to dev — staying on the feature branch per convention. Repairing the already-corrupted rows on staging is a separate follow-up (goal #3 in the issue body), not done yet.

gabriel 55 days ago

Fully resolved and verified — closing.

Correction to my earlier analysis: the "2 remaining unfixable" snapshots were never actually corrupted. My investigation used directories_from_manifest() to guess a directories list instead of reading the real one stored on the object — that guess was wrong for these two, making them look like permanent identity mismatches. Once I read the actual stored directories field, both hash-verified cleanly. All 3 corrupted snapshots were fixed the simple way, via wire_repair_snapshot — no history rewrite needed.

Also hit and fixed an operational snag: manually deleting a stale musehub_fetch_mpack_cache row via direct SQL skips the job-enqueue side effect wire_repair_snapshot normally handles, causing a temporary "server busy" 503 loop. Fixed by re-submitting the repair (idempotent), which re-triggered the rebuild correctly.

Final verification — fresh clone into a clean /tmp directory:

muse clone https://staging.musehub.ai/gabriel/musehub musehub --json

18.2s total, 1444/1444 commits, 0 hash mismatches, 999 blobs, clean working tree (1009 files). aaronrene confirmed unblocked on #84.

The write-path fix itself (fix/wire-push-external-parent-manifest) is still real and still worth merging — it prevents this exact class of corruption from recurring on future incremental pushes. Not yet merged.

closed this issue 55 days ago