Fix #106: pre-claim schema_migrations before running DDL - #122
Conversation
Two MCP servers starting against the same DB both read `applied` before either commits, then both call `executescript`; SQLite serialises the writes so one succeeds and the other dies with "table already exists", killing that session's server. Contend on the version row via `INSERT OR IGNORE` before any DDL runs — SQLite serialises writers, so one process wins (rowcount == 1) and runs the migration while the other sees rowcount == 0 and trusts the winner. On DDL failure, release the claim so the next start retries rather than skipping a half-applied migration. Two regression tests: one simulates a concurrent runner by pre-recording the row + creating the target table between `_applied_versions` and the claim, the other pins the claim-release-on-failure contract.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Claude BugBot Analysis
Found 1 potential bug in this PR.
high: 1
The new concurrent-safe migration claim mechanism improves the common race case, but introduces a new failure mode: if the winning process's DDL fails after a loser has already skipped (trusting the winner), the loser silently continues with an incomplete schema instead of also failing.
BugBot flagged: the first cut of #106 let a losing process skip the version as soon as it saw a claim row, so if the winner's DDL then failed, the loser returned from apply_migrations with a broken schema (the winner's rollback deleted its claim but the loser was already past that version). Two-phase claim closes that gap: the claim row is INSERTed with applied_at explicitly NULL, meaning "in progress"; the winner UPDATEs it to CURRENT_TIMESTAMP only after executescript succeeds, or DELETEs it on failure. _applied_versions filters to applied_at IS NOT NULL, and losers _wait_for_peer poll on the row: transition to non-NULL is "skip", disappearance is "peer failed, retry the claim ourselves", and a bounded timeout raises rather than silently returning with a partial schema — the whole point of the fix. Three regression tests: peer-failed-and-loser-retries, stuck-peer times out, _applied_versions excludes in-progress claims. The already-completed-peer test now sets applied_at explicitly non-NULL to match the two-phase contract.
|
Good catch — pushed The first cut trusted the bare claim row, so a loser that skipped past a version whose winner then failed would return with a broken schema. The fix is a two-phase claim: the row is inserted with Three new regression tests cover the peer-failed-then-loser-retries path, the stuck-peer timeout, and the Generated by Claude Code |
There was a problem hiding this comment.
🔴 Claude BugBot Analysis
Found 1 potential bug in this PR.
high: 1
The new two-phase claim protocol correctly fixes the prior 'loser trusts winner silently' race via _wait_for_peer, but the claim row can still be leaked permanently (causing all future runs to hang 120s then error) if sql_file.read_text() or _mark_complete() fails, since only executescript() is covered by the release-on-failure handler.
BugBot flagged: only executescript was inside the try/except that releases the claim, so a failure in sql_file.read_text (OSError / UnicodeDecodeError) BEFORE the try — or in _mark_complete AFTER it — would leak the row with applied_at IS NULL forever, making every future start poll _CLAIM_WAIT_SECONDS then raise until someone hand-cleans schema_migrations. Move read_text inside the try, and move _mark_complete inside too so a transient failure on the completion UPDATE also releases the claim. Two regression tests monkeypatch each failure point and assert no row survives.
|
Also a real leak — pushed
Generated by Claude Code |
There was a problem hiding this comment.
🟢 Claude BugBot Analysis
The rewritten two-phase claim protocol in better_memory/db/schema.py correctly fixes both previously reported issues (loser no longer blindly trusts the winner, and the claim row is now released on any failure during read_text, executescript, or _mark_complete); no new bugs were found in the diff.
No bugs were detected in this PR.
2 participants
Closes #106
Summary
Two MCP servers starting against the same DB both read the
appliedset before either commits, then both callexecutescriptand race — SQLite serialises writers so one succeeds and the other dies withtable observation_trigram_fts already exists, killing that session's server (server.py:165-168callsapply_migrationsunconditionally at every start).Contend on the version row via
INSERT OR IGNORE INTO schema_migrations (version) VALUES (?)before any DDL runs, then commit and inspectrowcount. SQLite's writer serialisation gives us one winner (rowcount == 1, proceeds with the DDL) and one loser (rowcount == 0, trusts the winner and skips). Onexecutescriptfailure we release the claim so the next start retries rather than skipping past a half-applied migration.Two regression tests: one monkeypatches
_applied_versionsto reproduce the stale-snapshot window (concurrent runner has already recorded the row + created the table between our snapshot and our claim), the other pins the claim-release-on-failure contract. Fulltests/db/suite passes (111/111).Confidence
~90%. The fix is precisely one of the two options the issue names ("Take an exclusive claim before executing (
INSERT INTO schema_migrations ... ON CONFLICT DO NOTHING, commit, proceed only ifrowcount == 1)"): a self-contained change in one function, with two focused regression tests and no touch to the migration files themselves. The atomicity-of-executescriptconcern the issue notes as related (#27) is deliberately out of scope; the claim-release path preserves the pre-fix "failure ⇒ not marked applied" contract.This PR was generated by a scheduled Claude routine that scans open issues and opens PRs only when confidence is ≥ 90%.
Generated by Claude Code