gabriel / musehub public
dead-mpack-gc-consolidation.md markdown
132 lines 7.3 KB
Raw
sha256:34035d72cef530c1ab9d6a6f53be18d803dde705fc3157617d70352a96d0747b Merge branch 'fix/wire-push-external-parent-manifest' into dev Human 8 days ago

process_mpack_gc_job / purge_stale_mpack_index_entries are dead code, and their docstrings contradict the real storage model

Discovered during: musehub#113 (silent partial clone investigation, itself a child of muse#63, MWP-9). Split out as its own ticket rather than fixed inline, since it's unrelated to #113's actual root cause and this workspace's convention is to file new findings separately rather than patch them mid-investigation. Status: confirmed dead code with an evidence trail below.

⚠️ Correction history — a prior correction to this ticket was itself wrong; original recommendation restored

This ticket briefly carried a correction claiming process_mpack_gc_job was load-bearing (not dead code) because a multi-branch cache-HIT invariant in wire_fetch_mpack could supposedly never be satisfied without it. That claim was based on testing against a stale local musehub_worker container running month-old code (Docker image dated 2026-05-15). After restarting the worker against current code and re-testing, process_fetch_mpack_prebuild_job correctly builds one shared mpack across all tips exactly as documented — the invariant is satisfiable by current code, with no need for consolidation. The original "remove as dead code" recommendation below is restored. Full detail and the actual (unrelated) root cause of #113's bug is in musehub#113's own retraction/correction section.

Finding

Two functions in musehub/services/ are fully built, fully unit-tested, and never invoked by any production code path:

Function Location Own dedicated tests
process_mpack_gc_job musehub/services/musehub_wire_fetch.py:913 tests/test_mpack_gc_phase4.py (438 lines, 5 tests: PG-1, PG-2, PG-3, PG-4, PG-6)
purge_stale_mpack_index_entries musehub/services/musehub_wire_push.py:1133 tests/test_mpack_index_stale_cleanup.py (135 lines, 3 tests: SC-1, SC-2, SC-3)

Evidence both are unreachable in production, not just untested:

  • musehub/worker.py's job dispatch (worker.py:107-129) only actively processes two job types: "mpack.index" and "fetch.mpack.prebuild". Every other job type is a silent no-op (pass). There is no "mpack.gc" (or any GC-related) job type check anywhere in the dispatcher — not even a recognized-but-disabled one, unlike the ~17 intel job types which at least hit the no-op branch by name.
  • content-grep "purge_stale_mpack_index_entries|process_mpack_gc_job" across the entire repo returns exactly four production-code hits: the two function definitions, one re-export (musehub_wire.py:63, for process_mpack_gc_job only — purge_stale_mpack_index_entries isn't even re-exported), and one historical doc mention (docs/issues/mwp-6-reenable-wire-suite.md:99, a completed-campaign summary listing it as part of the wire-suite corpus at the time). No admin API route, CLI command, cron script, or deploy/ script calls either function.
  • muse code dead --high-confidence-only --json does not flag either function. This is an important nuance, not a contradiction: the tool's dead-code detection is reference-graph based, and both functions genuinely are referenced — by their own dedicated test files. What's actually dead is the runtime dispatch path, which no static reference check can see (it requires knowing that worker.py's job-type string matching never produces the string "mpack.gc"). This is a different, subtler category than what muse code dead is designed to catch — worth keeping in mind for future dead-code hunts, though out of scope to fix the tool itself here.

The docstring/code contradiction

purge_stale_mpack_index_entries's docstring claims:

"Safe to delete: objects with stale entries have valid s3:// storage_uri and are served directly from S3 without needing the mpack path."

This is false as of the current object-write path. process_mpack_index_job (musehub/services/musehub_wire_push.py:1668-1686, the only place MusehubObject.storage_uri is written) always writes:

mpack_uri = f"mpack://{mpack_key}"

Never s3://. There is no code path anywhere in the repo that writes an s3://-scheme storage_uri. If purge_stale_mpack_index_entries ever did run (it currently doesn't — see above), its own safety assumption for why deleting an index row is "safe" would not hold: the object's only retrieval path (mpack://{mpack_key}) is exactly what the row being deleted points to. Deleting the row without an s3:// fallback would orphan the object.

Why this matters even though nothing calls it today

If either function is ever wired into the worker (e.g., someone notices per-push upload mpacks piling up in storage and decides to "finish" this feature), the s3:// assumption would make purge_stale_mpack_index_entries actively unsafe — it would delete index rows believing a fallback exists that doesn't, silently orphaning objects. This is a landmine, not just clutter.

Recommendation

Per this workspace's "no legacy, no dead code, delete on sight" convention: remove both functions, their MPackGCResult/PurgeResult TypedDicts, the re-export in musehub_wire.py:60,63, and both dedicated test files (tests/test_mpack_gc_phase4.py, tests/test_mpack_index_stale_cleanup.py) in the same commit — implementation, tests, and exports together, per the workspace's "when you remove something, remove it completely" rule.

If, instead, this consolidation feature is actually wanted (reducing per-push mpack storage sprawl over time), the correct fix is the opposite direction: wire process_mpack_gc_job into worker.py's dispatch table under a real "mpack.gc" job type, decide a real trigger/schedule for it, and fix purge_stale_mpack_index_entries's docstring to describe the actual mpack:// storage model (or, if a genuine s3:// fallback is desired, implement it before trusting the purge to be safe). This is the larger, riskier option and should only be chosen if there's a real, current need for per-push-mpack storage cleanup — not decided speculatively here.

Deliverables

  • [ ] Decide: remove (default recommendation, restored — see correction history at top) or wire in + fix docstring.
  • [ ] If removing: delete process_mpack_gc_job, MPackGCResult, purge_stale_mpack_index_entries, PurgeResult, the re-export lines in musehub_wire.py, and both dedicated test files, in one commit.
  • [ ] If wiring in: add "mpack.gc" to worker.py's dispatch, decide a trigger/schedule, and correct the docstring to match the real mpack:// storage model (or implement the s3:// fallback the docstring currently assumes).
  • [ ] Either way: update docs/issues/mwp-6-reenable-wire-suite.md's historical mention only if it becomes actively misleading — it's a completed-campaign record, not a living spec, so leave as-is unless removal makes the line factually wrong about current test file existence.

Out of scope

  • Root-causing musehub#113 (the actual bug this was discovered while investigating) — unrelated to this finding.
  • Auditing the rest of the codebase for similar "tested but never dispatched" job types beyond the two found here.
File History 4 commits
sha256:34035d72cef530c1ab9d6a6f53be18d803dde705fc3157617d70352a96d0747b Merge branch 'fix/wire-push-external-parent-manifest' into dev Human 8 days ago
sha256:fc04e4cae9e1774d6a21b65c45daeed0e6787eb581d13aa1b03bfe9384a34226 Merge branch 'fix/two-column-scroll-layout' into dev Human 8 days ago
sha256:408916fc5973ba59c6e4eebaa80ebdcc801c0a63205651e25009d11548f79454 chore: bump version to 0.2.0.dev2 — nightly.2, matching muse Sonnet 4.6 patch 11 days ago
sha256:d035733f21ccff27735fddebfbbe0ed24565a32a22db8de5885402262671ecd2 chore: bump version to 0.2.0rc15 for musehub#113 fix release Sonnet 4.6 patch 14 days ago