Skip to content

fix(kanban): serialize first-use schema initialization - #21395

Closed
qWaitCrypto wants to merge 2 commits into
NousResearch:mainfrom
qWaitCrypto:fix/kanban-init-race
Closed

fix(kanban): serialize first-use schema initialization#21395
qWaitCrypto wants to merge 2 commits into
NousResearch:mainfrom
qWaitCrypto:fix/kanban-init-race

Conversation

@qWaitCrypto

@qWaitCrypto qWaitCrypto commented May 7, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a kanban SQLite initialization race during gateway startup.

The gateway can start the kanban notifier watcher and dispatcher watcher at nearly the same time. On their first tick, both paths can touch the same board DB and race through hermes_cli.kanban_db.connect() / init_db(). Before this change, the module-level _INITIALIZED_PATHS cache was checked and updated without synchronization, so two threads in the same process could both decide that the same DB path still needed schema initialization and then run _migrate_add_optional_columns() concurrently.

Depending on timing, startup could fail with either:

  • sqlite3.OperationalError: duplicate column name: consecutive_failures
  • sqlite3.OperationalError: database is locked

This PR applies the proper fix in hermes_cli/kanban_db.py by serializing the full first-use check -> init -> migrate -> set path with a module-level threading.RLock. Normal post-initialization connections remain unlocked.

In addition to serializing in-process first-use initialization, this PR also makes optional-column migrations tolerate duplicate-column races. If another process adds an additive migration column between PRAGMA table_info(...) and ALTER TABLE ... ADD COLUMN, _ensure_column() re-reads the schema and suppresses only confirmed duplicate-column races.

Related Issue

Fixes #21374.
Fixes #21378.
Likely addresses #21708.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Serialize first-use kanban DB schema initialization with a module-level threading.RLock in hermes_cli/kanban_db.py.
  • Add shared helpers so connect() and init_db() use the same locked initialization path instead of duplicating check/init/set logic.
  • Ensure failed initialization does not mark a DB path as initialized and closes the initialization connection before re-raising.
  • Keep ordinary already-initialized connect() calls off the global lock.
  • Add a concurrent-connect regression test in tests/hermes_cli/test_kanban_core_functionality.py that exercises multiple threads opening the same fresh DB and verifies the migration columns exist afterward.
  • Added _ensure_column() to make additive migrations duplicate-column tolerant after schema re-read confirmation.
  • Updated _migrate_add_optional_columns() to use _ensure_column() for task/task_events optional columns.
  • Added regression tests for duplicate-column races and follow-up index creation.-

How to Test

  1. Run the new/updated kanban tests:
    python -m pytest tests/hermes_cli/test_kanban_core_functionality.py tests/hermes_cli/test_kanban_boards.py tests/tools/test_kanban_tools.py -q
  2. Confirm the suite passes, including the concurrent initialization regression test.
  3. Optionally reproduce the original startup shape conceptually by noting that concurrent first-use connect() calls against the same fresh DB no longer race into duplicate-column or early locked-DB failures.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: WSL / Linux

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Relevant test command:

python -m pytest tests/hermes_cli/test_kanban_core_functionality.py tests/hermes_cli/test_kanban_boards.py tests/tools/test_kanban_tools.py -q

Observed result:

239 passed in 76.40s

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets labels May 7, 2026
@fermaf

fermaf commented May 8, 2026

Copy link
Copy Markdown

I reproduced the same class of Kanban gateway startup failure locally on v0.13.0 / Linux:

ERROR gateway.run: kanban dispatcher: tick failed on board default
sqlite3.OperationalError: duplicate column name: consecutive_failures

The gateway service itself stayed active, but the embedded Kanban dispatcher lost its first tick while opening the board through:

gateway/run.py::_tick_once_for_board
  -> hermes_cli.kanban_db.connect(board=slug)
  -> _migrate_add_optional_columns(conn)

Local mitigation tested

I locally hardened _migrate_add_optional_columns() so additive column migrations tolerate the classic SQLite race where another connection adds the same column between:

PRAGMA table_info(...)
ALTER TABLE ... ADD COLUMN ...

Specifically, the local patch:

  • refreshes the PRAGMA table_info(tasks) column snapshot after each additive migration
  • catches only sqlite3.OperationalError messages containing duplicate column name
  • re-reads the schema after that duplicate-column error and only suppresses the exception if the requested column now exists
  • applies the same duplicate-column tolerance to task_events.run_id

Validation from my local environment:

hermes gateway restart
hermes gateway status

# after restart:
new duplicate column name: consecutive_failures occurrences: 0
new kanban dispatcher: tick failed occurrences: 0

Earlier local targeted test run after the hotfix passed:

tests/hermes_cli/test_kanban_db.py: 60 passed
tests/hermes_cli/test_kanban_db.py tests/hermes_cli/test_kanban_boards.py: 110 passed

Design feedback on this PR

I think the RLock / _ensure_initialized() direction in this PR is the right architectural fix for the in-process gateway race. Serializing first-use initialization inside kanban_db.py is cleaner than trying to make every caller avoid init_db() or sequence notifier/dispatcher startup manually.

One possible complementary hardening: keep the migration itself idempotent/tolerant of duplicate-column races as a defensive layer. The lock in this PR protects concurrent threads inside one Hermes process, but duplicate-column races can still happen if:

  • a CLI Hermes process and the gateway process touch the same board DB during an update/restart
  • an older Hermes process is still running while a newer one starts
  • external or plugin code opens/migrates the same SQLite file without going through this new in-process lock

So my suggestion is:

  1. keep this PR's serialized first-use initialization as the primary fix
  2. optionally add a small _ensure_column / duplicate-column-tolerant helper inside _migrate_add_optional_columns() as a belt-and-suspenders defense for inter-process/version-mixed races

That combination would cover both observed symptoms:

  • duplicate column name: consecutive_failures / max_retries
  • first-tick database is locked races during gateway startup

Local note

I re-ran the currently relevant targeted suite in my checkout after reviewing this PR:

./venv/bin/python -m pytest tests/hermes_cli/test_kanban_db.py tests/hermes_cli/test_kanban_boards.py -q -o 'addopts='

Current result in my checkout:

109 passed, 1 failed

The failing test is:

tests/hermes_cli/test_kanban_boards.py::TestCLI::test_boards_create_and_switch
AssertionError: assert 'default' == 'myproj'

I confirmed the same failure reproduces in a clean detached worktree at my current HEAD without the local kanban_db.py hotfix, so it appears unrelated to the duplicate-column migration change.

@qWaitCrypto

Copy link
Copy Markdown
Contributor Author

I reproduced the same class of Kanban gateway startup failure locally on v0.13.0 / Linux:

ERROR gateway.run: kanban dispatcher: tick failed on board default
sqlite3.OperationalError: duplicate column name: consecutive_failures

The gateway service itself stayed active, but the embedded Kanban dispatcher lost its first tick while opening the board through:

gateway/run.py::_tick_once_for_board
  -> hermes_cli.kanban_db.connect(board=slug)
  -> _migrate_add_optional_columns(conn)

Local mitigation tested

I locally hardened _migrate_add_optional_columns() so additive column migrations tolerate the classic SQLite race where another connection adds the same column between:

PRAGMA table_info(...)
ALTER TABLE ... ADD COLUMN ...

Specifically, the local patch:

  • refreshes the PRAGMA table_info(tasks) column snapshot after each additive migration
  • catches only sqlite3.OperationalError messages containing duplicate column name
  • re-reads the schema after that duplicate-column error and only suppresses the exception if the requested column now exists
  • applies the same duplicate-column tolerance to task_events.run_id

Validation from my local environment:

hermes gateway restart
hermes gateway status

# after restart:
new duplicate column name: consecutive_failures occurrences: 0
new kanban dispatcher: tick failed occurrences: 0

Earlier local targeted test run after the hotfix passed:

tests/hermes_cli/test_kanban_db.py: 60 passed
tests/hermes_cli/test_kanban_db.py tests/hermes_cli/test_kanban_boards.py: 110 passed

Design feedback on this PR

I think the RLock / _ensure_initialized() direction in this PR is the right architectural fix for the in-process gateway race. Serializing first-use initialization inside kanban_db.py is cleaner than trying to make every caller avoid init_db() or sequence notifier/dispatcher startup manually.

One possible complementary hardening: keep the migration itself idempotent/tolerant of duplicate-column races as a defensive layer. The lock in this PR protects concurrent threads inside one Hermes process, but duplicate-column races can still happen if:

  • a CLI Hermes process and the gateway process touch the same board DB during an update/restart
  • an older Hermes process is still running while a newer one starts
  • external or plugin code opens/migrates the same SQLite file without going through this new in-process lock

So my suggestion is:

  1. keep this PR's serialized first-use initialization as the primary fix
  2. optionally add a small _ensure_column / duplicate-column-tolerant helper inside _migrate_add_optional_columns() as a belt-and-suspenders defense for inter-process/version-mixed races

That combination would cover both observed symptoms:

  • duplicate column name: consecutive_failures / max_retries
  • first-tick database is locked races during gateway startup

Local note

I re-ran the currently relevant targeted suite in my checkout after reviewing this PR:

./venv/bin/python -m pytest tests/hermes_cli/test_kanban_db.py tests/hermes_cli/test_kanban_boards.py -q -o 'addopts='

Current result in my checkout:

109 passed, 1 failed

The failing test is:

tests/hermes_cli/test_kanban_boards.py::TestCLI::test_boards_create_and_switch
AssertionError: assert 'default' == 'myproj'

I confirmed the same failure reproduces in a clean detached worktree at my current HEAD without the local kanban_db.py hotfix, so it appears unrelated to the duplicate-column migration change.

Thanks — I agreed with that scope expansion and updated the branch accordingly.

This PR now has both layers:

  1. fix(kanban): serialize first-use schema initialization

    • addresses the same-process notifier/dispatcher startup race
  2. fix(kanban): tolerate duplicate-column migrations

    • adds duplicate-column-tolerant optional-column migration so _migrate_add_optional_columns() also handles cross-
      process / mixed-version races where another migrator wins between PRAGMA table_info(...) and ALTER TABLE ... ADD COLUMN ...

I also incorporated the indexed-column detail: the migration now runs CREATE INDEX IF NOT EXISTS ... whenever the
migrated column exists, not only when the current process added it. That now covers both tasks.idempotency_key and
task_events.run_id.

@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded. The duplicate-column race this PR addressed was fixed on main via PR #22627 (commit 7869838) using _add_column_if_missing(). Your _ensure_column() did re-verify the schema after a duplicate-column catch — that's marginally stronger than what landed — but the simpler fix already covers the user-facing crash, and the additional RLock/_ensure_column machinery duplicates code that's now in main without changing observable behaviour.

The complementary half of the bug (the redundant init_db() call at gateway/run.py that triggered the race in the first place) was just merged via PR #22994 — that closes the related #21378.

Really appreciated the thoroughness here, especially the threading test. Thanks for the work @qWaitCrypto.

@teknium1 teknium1 closed this May 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

4 participants