gabriel / musehub public
issue-number-race-condition.md markdown
78 lines 3.2 KB
Raw
sha256:1dd27e6d5c24550177b01b0a171e8375c07cf046b549e7683364706b6522d3bc docs(#139): mark all 11 checklist items resolved, document … Sonnet 5 12 days ago

Concurrent issue creation assigns duplicate numbers, then 500s on read

Background

Discovered while bulk-creating test issues in parallel (40 concurrent muse hub issue create calls) for issue #139's layout verification: 20 issues landed with badly duplicated numbers — 13 issues numbered #3, 5 numbered #2, 2 numbered #1, none numbered higher despite 20 rows existing.

Root cause, traced in code:

_next_issue_number() (musehub/services/musehub_issues.py:104-110):

async def _next_issue_number(session: AsyncSession, repo_id: str) -> int:
    """Return the next sequential issue number for the given repo (1-based)."""
    stmt = select(func.max(MusehubIssue.number)).where(
        MusehubIssue.repo_id == repo_id
    )
    current_max: int | None = (await session.execute(stmt)).scalar_one_or_none()
    return (current_max or 0) + 1

Classic read-then-write race: two concurrent create_issue() calls can both read the same MAX(number) before either commits, so both compute and insert the same number. Nothing prevents this — ix_musehub_issues_repo_number (musehub_social_models.py:49) is a plain index, not a UniqueConstraint.

The duplicate then breaks reads. get_issue() (musehub/services/musehub_issues.py:239-258) does:

row = (await session.execute(stmt)).scalar_one_or_none()

.scalar_one_or_none() raises sqlalchemy.exc.MultipleResultsFound when more than one row matches (repo_id, number) — unhandled, surfaces as a bare 500 to the client. Confirmed live: opening any of the duplicated issues in the browser threw exactly this (htmx logged Response Status Error Code 500 from /gabriel/musehub/issues/3).

Impact

Low likelihood in normal single-user usage (issue creation is rarely truly concurrent), but real: any race — two agents filing issues at the same moment, a retry-on-timeout double-submit, a script bulk-creating issues in parallel — can silently corrupt numbering and then 500 on every subsequent read of the affected issue(s) until manually fixed in the DB.

Fix

  1. Add a DB-level UniqueConstraint("repo_id", "number") on musehub_issues (Alembic migration) — turns silent duplication into a loud, retryable integrity error instead of corrupted data.
  2. Make _next_issue_number + insert atomic under that constraint: catch the integrity error and retry with a fresh MAX+1, or move number assignment to a DB-side mechanism (e.g. a per-repo sequence/counter row updated with SELECT ... FOR UPDATE, or an atomic INSERT ... ON CONFLICT loop) so concurrent creates serialize instead of racing.
  3. Defensively, get_issue (and any other .scalar_one_or_none() call keyed on (repo_id, number)) should use .first() or LIMIT 1 so a pre-existing duplicate degrades to "shows one of them" instead of a hard 500 — belt-and-suspenders, not a substitute for fixing the constraint.

Repro

# Fire N concurrent creates against the same repo; inspect resulting numbers
for i in $(seq 1 20); do
  muse hub issue create --title "race test $i" --hub <hub-url> --json &
done
wait
muse hub issue list --hub <hub-url> --state all --limit 200 --json \
  | jq -r '.issues[].number' | sort -n | uniq -c
# Expect: no count > 1. Currently: heavy duplication.
File History 1 commit
sha256:1dd27e6d5c24550177b01b0a171e8375c07cf046b549e7683364706b6522d3bc docs(#139): mark all 11 checklist items resolved, document … Sonnet 5 12 days ago