gabriel / musehub public
mwp-4-prebuild-after-index.md markdown
380 lines 17.9 KB
Raw
sha256:d035733f21ccff27735fddebfbbe0ed24565a32a22db8de5885402262671ecd2 chore: bump version to 0.2.0rc15 for musehub#113 fix release Sonnet 4.6 patch 15 days ago

MWP-4 — Gate fetch.mpack.prebuild behind mpack.index (fixes RC-4)

Master tracker: muse#58 — https://staging.musehub.ai/gabriel/muse/issues/58 Predecessors (all closed):

  • musehub#106 (MWP-1, generation authority)
  • musehub#107 (MWP-2, walk fallback)
  • musehub#108 (MWP-3, job-enqueue idempotency)

This ticket is written to be picked up cold by another agent. It is dense on purpose. Every phase is TDD: write the test, watch it fail, implement until green, never skip ahead. Symbol anchors point code-intelligence at the exact call sites — use muse code cat "<anchor>" --json to read each one before you touch it.


Background

The symptom

A muse clone issued shortly after a muse push intermittently fails with 503 MPackNotReadyError (or returns and then a follow-up clone 503s), even though the push succeeded and the objects are on the remote. A second clone a moment later works. This is the last open correctness gap in the prebuild/index pipeline after MWP-1/2/3.

Why it happens — RC-4 (foundation-job ordering race)

Every push enqueues a batch of background jobs in enqueue_push_intel (musehub/services/musehub_jobs.py::enqueue_push_intel). Two of them gate clone correctness and both currently share the same created_at timestamp:

  • mpack.index — writes MusehubMPackIndex rows so the fetch path can prove every object a clone needs is retrievable.
  • fetch.mpack.prebuild — builds the combined clone mpack for all live branch tips and writes MusehubFetchMPackCache rows.

The single worker claims jobs via musehub/services/musehub_jobs.py::claim_next_job, which orders only by created_at:

.order_by(MusehubBackgroundJob.created_at)
.limit(1)
.with_for_update(skip_locked=True)

There is no tiebreaker. When mpack.index and fetch.mpack.prebuild carry identical created_at values, PostgreSQL is free to return either first. If the prebuild wins the tie, it runs before the index has written its rows.

The prebuild then calls musehub/services/musehub_wire_fetch.py::wire_fetch_mpack with force_build=True. That function runs the index-coverage check at musehub/services/musehub_wire_fetch.py:681-695:

indexed_q = await session.execute(
    select(MusehubMPackIndex.entity_id)
    .where(MusehubMPackIndex.entity_id.in_(list(new_oids)))
    .where(MusehubMPackIndex.entity_type == "object")
)
indexed_oids = {row[0] for row in indexed_q}
missing = new_oids - indexed_oids
if missing:
    logger.warning("[wire_fetch_mpack] step=3 NOT INDEXED %d/%d objects ...")
    raise FetchNotIndexedError(len(missing))      # ← raised here

FetchNotIndexedError is defined at musehub/services/musehub_wire_shared.py::FetchNotIndexedError. It propagates up into process_fetch_mpack_prebuild_job, where it is silently swallowed by the broad handler at musehub/services/musehub_wire_fetch.py:1164-1167:

except Exception as exc:
    logger.error("[fetch.mpack.prebuild] FAILED: %s", exc, exc_info=True)

Result: tips_built stays 0, no MusehubFetchMPackCache row is written, and the job nonetheless completes "successfully" in the worker (musehub/worker.py:121-131complete_job). The clone cache stays empty for that tip until another push displaces it, so the next clone 503s.

Why MWP-3 did not already fix this

MWP-3 made the prebuild self-coalescing (it re-reads live branch tips at run time in process_fetch_mpack_prebuild_job, ignoring the stale payload tip set) and made mpack.index dedup per mpack_key so distinct mpacks are never dropped. That fixed which work gets enqueued. RC-4 is orthogonal: it is about the order in which already-enqueued work is claimed. A correct, coalesced prebuild job that simply runs too early still produces an empty cache.

Worker-scope reality

The worker is single-threaded today (musehub/worker.py::_process_one claims one job, runs it to completion, loops — musehub/worker.py:74-166). Only mpack.index and fetch.mpack.prebuild are live (musehub/worker.py:107-108: every other type falls through pass). So for the single-worker case, claim order == run order, and a deterministic claim gate fully closes the race. The fix is also designed to remain correct if the worker is ever scaled to multiple processes (see Phase 2 rationale).


Goal — definition of done

  1. claim_next_job never returns a fetch.mpack.prebuild job while an mpack.index job for the same repo is pending or running.
  2. After any sequence of back-to-back pushes, once the queue drains, the clone cache covers the latest tip of every branch, with zero FetchNotIndexedError occurrences during the drain.
  3. A genuine index gap (e.g. a permanently-failed mpack.index) is surfaced, not masked as a successful empty prebuild.
  4. claim_next_job gains its first direct test coverage (it has none today).
  5. The MWP-4 suite plus all adjacent suites are green with no regressions.

Design — chosen fix and rationale

The master tracker proposed two options: (a) give fetch.mpack.prebuild a later created_at than mpack.index, or (b) make the prebuild depend on index completion. This ticket implements (b) as the load-bearing fix and (a) as defense-in-depth, because (a) alone is a timing heuristic that breaks under back-to-back pushes and multi-worker claim.

Why a timestamp stagger alone is insufficient

Consider two pushes A then B. With a pure stagger, push A's prebuild gets T_a+δ and push B's index gets T_b. If T_a+δ < T_b, push A's prebuild is claimed before push B's index. But MWP-3 made the prebuild read live tips — so push A's prebuild tries to build push B's not-yet-indexed tip and hits FetchNotIndexedError. The stagger fixes only the within-one-push ordering, not the cross-push interleaving. A real dependency barrier fixes both.

The load-bearing fix — a claim-layer dependency barrier

Modify claim_next_job so a fetch.mpack.prebuild row is not claimable while any mpack.index for the same repo_id is pending or running. The prebuild simply waits in pending until index work drains — no re-enqueue, no defer counter, no busy-loop, correct under any worker count. Contract:

from sqlalchemy import exists, select
from sqlalchemy.orm import aliased

_idx = aliased(MusehubBackgroundJob)
_index_blocking = (
    select(_idx.job_id)
    .where(
        _idx.repo_id == MusehubBackgroundJob.repo_id,        # correlated by repo
        _idx.job_type == "mpack.index",
        _idx.status.in_(("pending", "running")),
    )
)

result = await session.execute(
    select(MusehubBackgroundJob)
    .where(
        MusehubBackgroundJob.status == "pending",
        MusehubBackgroundJob.attempt < _MAX_ATTEMPTS,
        ~(
            (MusehubBackgroundJob.job_type == "fetch.mpack.prebuild")
            & _index_blocking.exists()
        ),
    )
    .order_by(MusehubBackgroundJob.created_at)
    .limit(1)
    .with_for_update(skip_locked=True)
)

Behavioural contract of the barrier:

Scenario Outcome
mpack.index pending for repo R prebuild for R not claimable
mpack.index running for repo R prebuild for R not claimable
mpack.index done / failed / quarantined for repo R prebuild for R claimable (terminal states don't gate)
mpack.index pending for repo X prebuild for repo Y claimable (per-repo)
claiming the mpack.index job itself unaffected — barrier only filters fetch.mpack.prebuild

Implementation note — FOR UPDATE + correlated EXISTS. The FOR UPDATE SKIP LOCKED clause locks only the outer selected row; the EXISTS subquery is a read-only existence check, not a join, so PostgreSQL applies the row lock cleanly. Verify with the Phase 2 tests against the real DB session (db_session fixture) — do not assert this only against a mock.

Starvation note. Under a relentless push stream, fresh mpack.index jobs keep arriving and the prebuild stays blocked. This is acceptable and correct: MWP-3 made the prebuild read live tips, so a single prebuild after the burst covers every tip. The barrier delays the prebuild to exactly when it can succeed; it never drops it. Steady-state push bursts are finite, so the queue drains and the prebuild runs once. Document this; do not add a timeout that would re-introduce the early-run race.

Defense-in-depth — created_at stagger

Mirror the existing subtype stagger (musehub/services/musehub_jobs.py:216-221, created_at vs created_at_subtype = created_at + 2s). Add a middle tier so the prebuild sorts after the index foundation jobs even before the barrier is consulted:

t+0s   mpack.index, intel.code, intel.structural, gc, ...   (foundation)
t+1s   fetch.mpack.prebuild                                  (NEW middle tier)
t+2s   intel.code.* subtypes

This keeps the common case race-free at the ordering layer and makes the barrier a guarantee rather than the sole line of defence.

Hardening — stop masking the index gap

Narrow the swallow in process_fetch_mpack_prebuild_job (musehub/services/musehub_wire_fetch.py:1164-1167). Catch FetchNotIndexedError specifically, log a structured warning, and re-raise so the worker records it via fail_job (retry up to _MAX_ATTEMPTS). With the barrier in place this path is only reachable when an mpack.index has terminally failed — exactly the case that should surface as a failed job, not a silent empty cache. Keep the broad except Exception for genuinely unexpected errors, but it must no longer convert a missing index into a "successful" no-op.


Phases

Each phase must be fully green before the next begins. Test file: tests/test_mwp4_prebuild_ordering.py. Fixtures: reuse db_session, tests.factories.create_repo, tests.factories.create_branch, and the MWP-3 E2E push helpers (_e2e_push, _drain_index_jobs) — copy/adapt from tests/test_mwp3_job_idempotency.py rather than re-inventing.

Phase 0 — RED reproduction (no production change)

  • [x] MWP4_01 RED — claim_next_job returns a fetch.mpack.prebuild ahead of a still-pending mpack.index for the same repo. Make it deterministic by inserting the prebuild with an earlier created_at than the index (simulates the cross-push interleaving). Pre-fix: claim returns the prebuild. This is the exact race. ✅ FAILING RED as expected.
  • [x] MWP4_02 RED — handler-level: with mpack.index not yet processed (index rows deleted after push to simulate the pre-index state), call process_fetch_mpack_prebuild_job; assert wire_fetch_mpack raises FetchNotIndexedError, the handler swallows it, and no MusehubFetchMPackCache row exists for the tip (documents the broken silent-no-cache outcome). ✅ PASSING — bug is proven.

Phase 1 — created_at stagger (defense-in-depth) ✅ COMPLETE

Anchor: musehub/services/musehub_jobs.py::enqueue_push_intel.

  • [x] MWP4_03 — after one enqueue_push_intel, the fetch.mpack.prebuild row's created_at is strictly greater than every mpack.index row's created_at, and strictly less than the intel.code.* subtype tier. ✅ PASSING.
  • [x] Implement the middle tier: removed fetch.mpack.prebuild from _FOUNDATION_TYPES; introduced created_at_prebuild = created_at + 1s via _job_created_at(job_type) helper. The three tiers are now: foundation (t+0s) → prebuild (t+1s) → subtypes (t+2s). ✅ SHIPPED.

Phase 2 — claim-layer dependency barrier (load-bearing) ✅ COMPLETE

Anchor: musehub/services/musehub_jobs.py::claim_next_job.

  • [x] MWP4_04 — prebuild NOT claimed while an mpack.index is pending for the same repo (even when the prebuild has the earlier created_at from MWP4_01). claim_next_job returns the index (or None if only the blocked prebuild remains). ✅ PASSING.
  • [x] MWP4_05 — after the mpack.index is set done, the prebuild becomes claimable on the next claim_next_job. ✅ PASSING.
  • [x] MWP4_06 — an mpack.index in running state also blocks the prebuild. ✅ PASSING — claim returns None when only a blocked prebuild is pending.
  • [x] MWP4_07failed and quarantined mpack.index rows do not block the prebuild (terminal states are not gates). ✅ PASSING.
  • [x] MWP4_08 — per-repo isolation: mpack.index pending in repo X does not block a prebuild in repo Y. ✅ PASSING.
  • [x] MWP4_09 — the barrier filters only fetch.mpack.prebuild: a pending mpack.index is still claimable, and other job types are unaffected. ✅ PASSING.
  • [x] Implemented correlated ~and_(job_type == 'fetch.mpack.prebuild', exists(...)) filter in claim_next_job. FOR UPDATE SKIP LOCKED locks only the outer row; the EXISTS subquery is read-only. Verified against real db_session. MWP4_01 (the original RED test) now passes. ✅ SHIPPED.

Phase 3 — stop masking the index gap ✅ COMPLETE

Anchor: musehub/services/musehub_wire_fetch.py::process_fetch_mpack_prebuild_job.

  • [x] MWP4_10 — when wire_fetch_mpack raises FetchNotIndexedError, the handler logs and re-raises (does not return a success result with tips_built=0). Assert the exception propagates so the worker's fail_job path records it. Other exceptions still hit the broad handler unchanged. ✅ PASSING.
  • [x] Narrowed except Exception at musehub_wire_fetch.py:1164-1167 — added a specific except FetchNotIndexedError: raise guard before the broad handler. MWP4_02 (which previously documented the bug as RED passing) now confirms the error propagates correctly. ✅ SHIPPED.

Phase 4 — integration + stress (real worker claim loop) ✅ COMPLETE

  • [x] MWP4_11 E2E — push once via the MWP-3 _e2e_push helper (enqueues real mpack.index + fetch.mpack.prebuild), then drive a claim loop (claim_next_job → dispatch → complete_job, mirroring musehub/worker.py::_process_one). Assert the index is claimed and completed before the prebuild is ever claimed, and a MusehubFetchMPackCache row lands at the pushed tip. ✅ PASSING.
  • [x] MWP4_12 stress — N=10 back-to-back pushes (no intermediate drain), then drain via the claim loop. Assert: (a) every fetch.mpack.prebuild claim happens only when zero mpack.index jobs are pending/running for the repo; (b) zero FetchNotIndexedError raised during the drain; (c) the final cache covers the latest tip (len(mpack_ids) == 1 across all live tips). ✅ PASSING.
  • [x] _drain_via_claim_loop helper added to test file — mirrors _process_one using claim_next_job → dispatch → complete_job, so MWP4_11/12 exercise the real barrier under end-to-end claim semantics. ✅ SHIPPED.

Phase 5 — docs + close ✅ COMPLETE

  • [x] Tick every checkbox above and every acceptance criterion below in this body. ✅ All Phase 0–4 checkboxes and all acceptance criteria ticked.
  • [x] Update master tracker muse#58: MWP-4 → ✅ COMPLETE; flip the RC-4 half of the "two pushes within one prebuild interval" acceptance criterion to ✅. ✅ Pushed to staging.musehub.ai.
  • [x] Post a closing comment summarizing the fix + commit range; close musehub issue with --status closed. ✅ Done.

Acceptance criteria

  • [x] claim_next_job never returns a fetch.mpack.prebuild while an mpack.index for the same repo is pending or running (MWP4_04, _06).
  • [x] Terminal mpack.index states (done/failed/quarantined) do not gate the prebuild (MWP4_05, _07).
  • [x] The barrier is per-repo and filters only prebuild jobs (MWP4_08, _09).
  • [x] fetch.mpack.prebuild is enqueued with a created_at strictly after every mpack.index foundation job (MWP4_03).
  • [x] A FetchNotIndexedError in the prebuild handler is surfaced (re-raised / failed job), never masked as a successful empty cache (MWP4_10).
  • [x] N back-to-back pushes drain with zero FetchNotIndexedError and a final clone cache covering the latest tip (MWP4_12).
  • [x] claim_next_job has direct test coverage where it had none.
  • [x] MWP-4 suite + adjacent suites green, no regressions (see below).

Adjacent suites to run green (no regressions)

  • tests/test_mwp4_prebuild_ordering.py (new)
  • tests/test_mwp3_job_idempotency.py
  • tests/test_fetch_mpack_prebuild.py
  • tests/test_process_mpack_index_job.py
  • tests/test_mpack_index_always_enqueued.py

Out of scope

  • RC-3 job-enqueue idempotency — shipped in MWP-3 (musehub#108).
  • RC-5 client-side 503 retry/Retry-After honoring — MWP-5 (muse repo).
  • RC-6 re-enabling the skipped wire/mpack suite — MWP-6.
  • The ~17 deliberately-disabled intel jobs (musehub/worker.py:107-108) — they remain no-ops; do not revive them here.
  • The mpack binary format and presigned-upload model — both sound, untouched.
  • Multi-worker deployment itself — the fix is compatible with it, but scaling the worker is a separate operational decision.

Reference — key symbol anchors

Symbol Purpose
musehub/services/musehub_jobs.py::claim_next_job add the dependency barrier (Phase 2)
musehub/services/musehub_jobs.py::enqueue_push_intel add the created_at stagger tier (Phase 1)
musehub/services/musehub_wire_fetch.py::process_fetch_mpack_prebuild_job narrow the swallow (Phase 3)
musehub/services/musehub_wire_fetch.py::wire_fetch_mpack raises FetchNotIndexedError at the index-coverage check (lines 681-695)
musehub/services/musehub_wire_shared.py::FetchNotIndexedError the exception type
musehub/worker.py::_process_one single-threaded claim/dispatch loop to mirror in integration tests
musehub/services/musehub_jobs.py::complete_job / fail_job status transitions the tests drive
File History 2 commits
sha256:d035733f21ccff27735fddebfbbe0ed24565a32a22db8de5885402262671ecd2 chore: bump version to 0.2.0rc15 for musehub#113 fix release Sonnet 4.6 patch 15 days ago
sha256:0032d6cfa33bc3c8367436ad768e7dd0e339b4332153160247da8266cb5fa352 Merge branch 'task/version-tags-phase3-server' into dev Human 17 days ago