gabriel / musehub public
clone-corruption-empty-external-parent-base.md markdown
127 lines 7.1 KB
Raw
sha256:34035d72cef530c1ab9d6a6f53be18d803dde705fc3157617d70352a96d0747b Merge branch 'fix/wire-push-external-parent-manifest' into dev Human 8 days ago

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.
File History 1 commit
sha256:34035d72cef530c1ab9d6a6f53be18d803dde705fc3157617d70352a96d0747b Merge branch 'fix/wire-push-external-parent-manifest' into dev Human 8 days ago