fix(kanban): tolerate races on legacy-DB column migration - #21874
Closed
li0near wants to merge 1 commit into
Closed
Conversation
Symptom: every gateway start against a kanban DB created before the
`max_retries` column logged a benign but noisy traceback:
sqlite3.OperationalError: duplicate column name: max_retries
...
ERROR gateway.run: kanban dispatcher: tick failed on board default
Root cause: `_migrate_add_optional_columns` reads
`PRAGMA table_info(tasks)` once into `cols`, then issues a series of
`ALTER TABLE … ADD COLUMN`. Two callers can race on the same DB file
and both pass the pre-ALTER `cols` snapshot check before either ALTER
lands; the loser raises `duplicate column name`. The race actually
fires today inside the embedded gateway dispatcher, which calls
`_kb.connect(board=slug)` (runs migration) immediately followed by
`_kb.init_db(board=slug)` (deliberately busts the per-process cache
and re-runs the migration). It can also fire if a second process opens
the same DB during gateway startup. SQLite has no
`ADD COLUMN IF NOT EXISTS`, so the snapshot guard is not enough.
Fixes:
1. Wrap every optional-column ALTER in a new `_safe_add_column` helper
that swallows exactly `duplicate column name` and re-raises anything
else. Migration becomes idempotent against itself for any caller.
2. Drop the redundant `init_db()` call in
`_tick_once_for_board` (`gateway/run.py`). `connect()` already
runs the schema + migration on first open per process; the explicit
`init_db()` only existed to guarantee re-migration after a cache
bust, which we no longer need now that the helper is race-tolerant.
Tests:
- `test_migration_is_idempotent_against_itself` builds a legacy DB
shape and runs four concurrent migrators; without the fix at least
one would raise. Asserts no errors and that every expected column
was actually added (proves the helper didn't silently drop ALTERs).
- `test_safe_add_column_reraises_unrelated_errors` confirms the
helper does not mask other `OperationalError` cases (bad SQL,
missing table) so real schema bugs still surface.
Verified end-to-end on a real legacy `/opt/data/kanban.db` upgraded
through this code path: the column is present, the dispatcher tick
runs cleanly, no traceback in gateway.log.
Contributor
|
Merged via PR #22994 (rebase) — your gateway-side double-init fix shipped on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a benign-but-noisy traceback emitted on every gateway start against a kanban DB created before the
max_retriescolumn was added:Root cause
_migrate_add_optional_columnsreadsPRAGMA table_info(tasks)once into acolsset, then issues a series ofALTER TABLE … ADD COLUMN. Two callers can race on the same DB file and both pass the pre-ALTER snapshot check before either ALTER lands; the loser raisesduplicate column name. SQLite has noADD COLUMN IF NOT EXISTS, so the snapshot guard is not enough.The race fires today inside the embedded gateway dispatcher in
_tick_once_for_board(gateway/run.py), which calls_kb.connect(board=slug)(runs the migration) immediately followed by_kb.init_db(board=slug)— andinit_dbdeliberately busts the per-process_INITIALIZED_PATHScache and re-runs the migration. It can also fire if a second process opens the same DB during gateway startup.The error is harmless (by the time it fires, the column is already present and the next dispatcher tick succeeds), but the traceback is logged on every gateway start against a legacy DB and obscures real failures.
Fix
_safe_add_columnhelper wraps every optional-column ALTER and swallows exactlysqlite3.OperationalError: duplicate column name, re-raising anything else. Applied to all 13 optional-column ALTERs ontasksand the one ontask_events.init_db()call in_tick_once_for_board.connect()already runs the schema + migration on first open per process; the explicitinit_db()only existed to guarantee re-migration after a cache bust, which is no longer needed now that the helper is race-tolerant. Removes one source of the race entirely. The dropped call was already wrapped inexcept Exception: pass, so no callers depended on its result.Test Plan
New tests in
tests/hermes_cli/test_kanban_db.py:test_migration_is_idempotent_against_itself— builds a legacy DB shape (pre-migrationtasksschema), runs four concurrent migrators on it, asserts (a) no errors and (b) every expected column was actually added.test_safe_add_column_reraises_unrelated_errors— confirms the helper does NOT mask otherOperationalErrorcases (e.g. ALTER on a missing table) so real schema bugs still surface.Test results
Regression test fails on unpatched upstream/main (proves the test catches the bug, not just passes vacuously):
The
duplicate column name: tenanterror is the same class of failure as themax_retriestraceback in the bug report — the new test captures the general race, not just one column.With the patch applied — full kanban-db suite passes:
Broader kanban surface — 378/379 pass:
The one failure is
tests/hermes_cli/test_kanban_boards.py::TestCLI::test_boards_create_and_switch. I confirmed it reproduces on plain upstream/main with my changes reverted, so it is pre-existing and unrelated to this PR.End-to-end: verified on a real legacy
kanban.dbthat originally produced the traceback in the bug report. After the patch, the column is present, the dispatcher tick runs cleanly, andgateway.logis silent on restart.Risk
Low.
_safe_add_columnonly suppresses the exact "duplicate column name" substring and re-raises everything else; the migration logic is otherwise unchanged. Regression-tested.init_db()in_tick_once_for_boardwas already inside a bareexcept Exception: pass, so no caller could have depended on its return value or side effects beyond running the migration thatconnect()had already run on first open. Outer try/except in the dispatcher is unchanged.