gabriel / musehub public
Closed #3 Bug
filed by gabriel human · 37 days ago

Critical: muse hub proposal merge can silently corrupt dev if called twice (stale state read + stale-base re-merge)

0 Anchors
— Blast radius
— Churn 30d
0 Proposals

Summary

Discovered live while merging proposal #9 (musehub#144). muse hub proposal merge can be called a second time on a proposal that was already successfully merged, because the first call's state/merge_commit_id/merged_at update to musehub_proposals did not appear to persist immediately (or wasn't visible to a subsequent read). The guard that should reject a second merge (if proposal.state not in ("open", "approved"): raise RuntimeError(...), musehub/services/musehub_proposals.py::merge_proposal) saw the proposal as still "open" and allowed a second merge to proceed.

This is not just a duplicate-call annoyance — it caused real, silent data loss. The second merge re-ran the full merge algebra using proposal.from_branch/proposal.to_branch's current head commits as its 3-way-merge inputs. Because from_branch (feat/9a-4-f7-overseer-provenance) was a long-lived branch whose merge-base with dev was months stale (forked in July; dev had ~90 commits of unrelated work since), the second merge computed a 3-way merge against an ancient common ancestor and silently reverted or deleted 47 files relative to dev's real, correct state — including deleting a brand-new migration file and two just-added test files outright, and reverting ~15 other files (deploy scripts, security fixes, docs) to months-old content. No error, no conflict, no warning — the merge endpoint returned {"merged": true, ...} exactly like the first, correct call did.

What happened, concretely

  1. muse hub proposal merge <id> (1st call) → returned {"merged": true, "mergeCommitId": "sha256:8a1389ae..."}. dev correctly advanced to a commit containing all of dev's prior state plus the proposal's new files. Verified via direct RDS query: at this point musehub_proposals.state was still 'open', not 'merged', despite the API call succeeding and the branch genuinely advancing.
  2. Deployed dev's new head to staging, verified the new feature live and working.
  3. Re-read the proposal (muse hub proposal read) → state: "open", mergeCommitId: null. Investigating this apparent bookkeeping bug, re-ran the merge call to see if it would reproduce.
  4. muse hub proposal merge <id> (2nd call) → returned {"merged": true, "mergeCommitId": "sha256:4bbb33ce..."} — a different, new merge commit. This time musehub_proposals.state correctly became 'merged'. But dev's branch ref on staging now pointed at this second, stale-based merge commit, which — confirmed via muse snapshot-diff against the correct commit from step 1 — had 0 additions, 43 modifications (many regressive), and 4 deletions relative to the correct state.
  5. Caught this before propagating further (never reached local/production remotes). Fixed by force-resetting staging/dev's ref back to the correct commit from step 1, and directly correcting the musehub_proposals row's merge_commit_id to match.

Two distinct bugs, both real

  1. State-persistence bug: the first merge_proposal call's DB write to state/merge_commit_id/merged_at did not take effect (or wasn't read back correctly) even though the branch-advancing part of the same transaction clearly succeeded and was later independently confirmed live. Needs root-causing — possibly a session/commit-ordering issue, a stale read from a follow-up request hitting a different connection before replication/visibility, or a real transaction bug in merge_proposal. this is the more urgent of the two to root-cause, since it's what enables bug #2.
  2. No merge-base staleness guard: even setting aside bug #1, merge_proposal will happily compute and apply a 3-way merge against a merge-base that is arbitrarily old, with no check that to_branch's current head is otherwise unreachable from what the proposal's own anchors expect. A proposal open for months against a fast-moving target branch is a real, unremarkable scenario (not a misuse) — the merge algebra should either detect this and refuse/warn, or the merge computation should treat to_branch's current live manifest as authoritative-and-additive rather than diffing against a stale common ancestor that makes unrelated intervening changes look like reverts.

Suggested next steps

  1. Reproduce bug #1 in a test: merge a proposal, immediately re-read state in the same test process, assert it's 'merged' before any second merge is attempted — this should already catch the persistence gap in isolation.
  2. Add an idempotency/regression test: call merge twice in a row on the same proposal; the second call must return 409 (already merged), never a second successful merge.
  3. For bug #2: add a test where to_branch has diverged significantly from the proposal's original fork point, and assert the merge either refuses (with a clear error asking the caller to rebase) or correctly preserves all of to_branch's intervening independent changes — not just the files the merge happens to also touch.
  4. Audit whether other proposals sitting open against dev for a long time are at risk of the same silent-revert failure mode if merged as-is.

Mirrored from staging#182.

◎ Activity
●
gabriel opened this issue 37 days ago
✓
closed this issue 37 days ago