Skip to content

Make schema-column migrations idempotent under concurrent startup - #346

Merged
stephenschoettler merged 1 commit into
stephenschoettler:mainfrom
ai-ag2026:pr/atomic-schema-migrations
Jul 8, 2026
Merged

stephenschoettler merged 1 commit into
stephenschoettler:mainfrom
ai-ag2026:pr/atomic-schema-migrations

Conversation

@ai-ag2026

Copy link
Copy Markdown
Contributor

Summary

  • Add add_column_if_missing(conn, existing_columns, column, alter_sql) in db_bootstrap.py and route all column migrations through it. It keeps the existing "skip if already present" check and additionally swallows exactly the duplicate column name OperationalError (any other error still propagates), making each ALTER TABLE ... ADD COLUMN idempotent under concurrency.
  • Converted call sites: ensure_lifecycle_state_columns / ensure_message_origin_columns (db_bootstrap.py), MessageStore._ensure_source_column / _ensure_conversation_id_column (store.py), SummaryDAG._ensure_source_window_columns (dag.py).

Why

In the multi-agent deployment (gateway + CLI sessions + sub-agents) every process opens its own connection to the same lcm.db and runs the startup migrations concurrently. Each column migration did check-PRAGMA table_info-then-ALTER in autocommit with no guard, so two processes could both observe a column as absent and both issue the ALTER. The loser raised sqlite3.OperationalError: duplicate column name, which propagated out of _init_db and crashed store construction. This bites precisely at an upgrade boundary, when many processes restart together.

Behaviour is unchanged on the non-racing path (same check, same ALTERs); only the concurrent loser now skips instead of crashing.

Validation

  • New regression test TestConcurrentStartupMigration (tests/test_crash_safe_wal.py) races ensure_message_origin_columns from 8 threads, each with its own connection to one seeded pre-v5 DB (FTS-free, to isolate the column DDL). Without the guard 7/8 threads raise duplicate column name: conversation_id; with it every thread migrates and the column exists exactly once.
  • ruff check . -> clean.
  • PYTHONPATH=<hermes-agent> python -m pytest -q -o addopts= -> 1576 passed, 12 xfailed.
  • scripts/validate_release.sh --full -> PASS: release validation full (all 12 gates, incl. low-fd).

Notes

  • Out of scope / follow-up: the FTS structural rebuild (repair_external_content_fts) has a separate concurrency window on a different trigger (missing shadow tables / corruption, not column upgrades — a column migration does not change row counts, so it does not enter that path). Left for a focused follow-up so this PR stays scoped to the verified crash.
  • Rollback: revert the commit; migrations return to the prior unguarded check-then-ALTER.

Refs

Surfaced by a comparison against the more mature lossless-claw, whose runLcmMigrations serializes migrations under BEGIN EXCLUSIVE.

In the multi-agent deployment (gateway + CLI sessions + sub-agents)
every process opens its own connection to the same lcm.db and runs the
startup migrations concurrently. Each column migration did a
check-`PRAGMA table_info`-then-`ALTER TABLE ADD COLUMN` in autocommit
with no guard, so two processes could both observe a column as absent
and both issue the ALTER; the loser raised
`sqlite3.OperationalError: duplicate column name`, which propagated out
of `_init_db` and crashed store construction. This bites exactly at an
upgrade boundary, when many processes restart together.

Add `add_column_if_missing(conn, existing_columns, column, alter_sql)`
in db_bootstrap.py: it keeps the existing "skip if already present"
check and additionally swallows exactly the `duplicate column name`
OperationalError (any other error still propagates), making the ALTER
idempotent under concurrency. Route all eleven column migrations
through it (db_bootstrap `ensure_lifecycle_state_columns` /
`ensure_message_origin_columns`, `store._ensure_source_column` /
`_ensure_conversation_id_column`, `dag._ensure_source_window_columns`).

Behaviour is unchanged on the non-racing path (same check, same ALTERs);
only the concurrent loser now skips instead of crashing.

Rollback: revert this commit; the migrations return to the prior
unguarded check-then-ALTER.

Validation:
- New regression test `TestConcurrentStartupMigration` races
  `ensure_message_origin_columns` from 8 threads (each its own
  connection to one seeded pre-v5 DB, FTS-free to isolate the column
  DDL). Without the guard 7/8 threads raise
  `duplicate column name: conversation_id`; with it every thread
  migrates and the column exists exactly once.
- ruff clean; full suite passes.

Note (out of scope, follow-up): the FTS structural rebuild
(`repair_external_content_fts`) has a separate concurrency window on a
different trigger (missing shadow tables / corruption, not column
upgrades); a column migration does not change row counts so it does not
enter that path. Left for a focused follow-up.

@stephenschoettler stephenschoettler left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved via Hermes Kanban reviewer gate t_d925191a. Evidence: live CI green 6/6, no unresolved review threads, and local validation passed on the reviewed head.

@stephenschoettler
stephenschoettler merged commit da91922 into stephenschoettler:main Jul 8, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants