fix(delegation): stop re-running schema DDL and add jittered retry to async_delegation writes - #63843
pierrenode wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Stopping redundant schema DDL re-runs and adding jittered backoff addresses real inefficiency in delegation hot paths. The fix shape (idempotent + retry-then-jitter) is standard.
Strengths
- Adds jittered backoff — clearly the right move to avoid lockstep retries from concurrent delimiters.
- Two-file change keeps the diff readable.
- Includes a negative test for the lock-retry path.
Reviewed by Hermes Agent in batch mode
|
Thanks for targeting a real shared- Problems
Suggested changes
Automated hermes-sweeper review. |
eef0bfc to
2f731f5
Compare
… async_delegation writes tools/async_delegation.py's _connect() performed WAL setup and schema DDL (CREATE TABLE / PRAGMA table_info / ALTER TABLE) on every single connection to the shared state.db file hermes_state.py's SessionDB also owns, and its durable writes (_persist_dispatch, _delete_durable_delegation, _prune_durable_records, etc.) had no application-level retry on SQLITE_BUSY at all — a single BEGIN IMMEDIATE contention against a concurrent gateway/cron writer raised straight through. Caches schema readiness per resolved db path (HERMES_HOME, and so state.db, can change across profiles within one process and across isolated test fixtures) so the DDL only runs once per path, and adds _execute_write() with jittered BEGIN IMMEDIATE retry, mirroring hermes_state.py's SessionDB._execute_write. _connect() itself can raise SQLITE_BUSY (the PRAGMA or a schema-cache-miss DDL statement contends on the same shared file), so it runs inside the retry-protected scope, and the jittered sleep happens with the module lock released so a busy retry doesn't stall every other same-process durable-delegation write for the full jitter window. _execute_write() wraps the existing _transaction() context manager (added since this fix was first written, to close leaked connections — NousResearch#69567) rather than duplicating its connect/commit/rollback/close logic, so both fixes compose: every write still gets guaranteed connection cleanup AND jittered retry. All 13 call sites route through it. restore_undelivered_completions() collects events to enqueue and only calls target_queue.put() once the retried transaction has committed, rather than in-loop — a queue.put() isn't rolled back the way the DB writes around it are, so enqueueing before commit would risk re-delivering the same completion twice on a retried attempt. Adds a schema-DDL-runs-once regression test, a schema-DDL-reruns-for- a-different-path test, a deterministic BEGIN-IMMEDIATE-busy retry test, a deterministic connect-itself-busy retry test, and a test proving the lock is released during the jitter sleep.
2f731f5 to
545e139
Compare
|
Substantially rewritten against current `upstream/main`, not a mechanical rebase. Since this PR was opened, an unrelated commit (`#69594`, fixing a connection-leak/FD-exhaustion bug — #69567) refactored every write call site in this file from bare `with _DB_LOCK, _connect() as conn:` onto a new `_transaction()` context manager that guarantees the connection is always closed. That refactor did not touch the two problems this PR fixes — schema DDL still re-ran on every single `_connect()` call, and there was still no retry on `SQLITE_BUSY`/`database is locked` anywhere in the file — so both bugs were still fully live, just underneath a different call-site shape than this PR originally targeted. Re-integration, not reapplication:
Targeted suite (25 tests, 5 new) passes, mutation-verified both fixes independently (removing the schema-cache check makes the DDL-runs-once test fail; removing the retry loop makes all 3 retry tests fail with the injected `OperationalError` propagating instead of being retried). Broader sweep across every test file that references `async_delegation` (133 tests: CLI delivery, gateway completion/relay routing, TUI session lifecycle, restored-delegation ownership, cronjob background, FD-leak) plus `api_server`/`journal_mode`/`process_registry` (221 more) all pass clean. Ruff clean. Fresh competitor search found no other PR touching this file's write path. |
Summary
tools/async_delegation.py(backingdelegate_task(background=True)'s durable completion persistence, added earlier today) opens a fresh ad hocsqlite3connection on every single call, against the SAMEstate.dbfilehermes_state.py'sSessionDBowns — its own class docstring says "gateway + cron processes share one state.db" and documents that unnecessary write-lock hold time "starves competing writers." Two problems against that shared file:_connect()executedCREATE TABLE IF NOT EXISTS+PRAGMA table_info+ up to 5 conditionalALTER TABLEstatements on every single call (read or write), even though the schema never changes at runtime. Fixed by recording "schema already ensured" per resolved db path and skipping the DDL once it's already run for that path. (Keyed by path, not a single flag, sinceHERMES_HOMEcan change across profiles within one process — and does across test fixtures.)SessionDB._execute_write(hermes_state.py) was deliberately built with a short per-connection timeout plus app-level jittered retry specifically because — per that method's own docstring — SQLite's built-in busy-timeout backoff is deterministic and convoy-prone under real concurrent load.async_delegation.pywrites into the exact same contended file but had none of that mitigation. Added a local_execute_write()mirroringSessionDB's pattern (BEGIN IMMEDIATE+ jittered retry onSQLITE_BUSY) and moved the 10 actual write functions onto it. The 2 read-only functions (restore_undelivered_completions,get_durable_delegation) are untouched — wrapping aSELECTinBEGIN IMMEDIATEwould itself add unnecessary write-lock contention.Also lowered
_connect()'s own timeout from 10s to 1.0s (matchingSessionDB's connection timeout): now that retries happen at the app level, a fresh connection per retry attempt with a 10s built-in timeout would multiply into a much longer worst-case stall than the retry loop (15 attempts × short jitter) is meant to bound.Test plan
test_connect_only_runs_schema_ddl_once_per_db_path— spies onCREATE TABLE/ALTER TABLEexecution via asqlite3.Connectionsubclass (the base type is immutable and can't be monkeypatched directly) and asserts the DDL runs on the first_connect()call for a path but not the second.test_connect_reruns_ddl_for_a_different_db_path— a different resolved path is correctly treated as a cache miss (guards against ano such tableregression from over-aggressive caching).test_execute_write_retries_on_database_locked— a fake connection wrapper raisessqlite3.OperationalError("database is locked")on the first twoBEGIN IMMEDIATEattempts, then succeeds; asserts_execute_writeretries transparently rather than propagating the error. Deterministic (no real wall-clock lock contention), so it isolates the retry logic itself.AttributeErrorfor the now-removed constants / DDL re-running), then restored the fix and confirmed all pass.uv run --frozen --extra dev python -m pytest tests/tools/test_async_delegation.py tests/gateway/test_async_delegation_session_binding.py— 35 passed, 1 pre-existing failure (test_crashed_runner_produces_error_completion) confirmed unrelated: reproduces identically against pre-fix code in the same run order (test-order-dependent flake, not a regression from this change).uv run --frozen --extra dev python -m pytest tests/cli/test_cli_background_status_indicator.py tests/tui_gateway/test_delegation_session_lifecycle.py tests/gateway/test_scale_to_zero_watcher.py— 48 passed (no regression in dependent consumers).ruff checkon both changed files — clean.Update (rebase onto current main)
Rebasing onto current
mainsurfaced that this exact area had independently evolved upstream (connection-leak fix via_transaction(), anorigin_session_idcolumn, and_connect()already routing throughapply_wal_with_fallback()instead of a barePRAGMA). Reconciled by keeping all of upstream's additions and layering this PR's schema-caching + jittered-retry on top via a shared_initialize_schema(conn, path)used by both the pre-existing_transaction()call sites and this PR's_execute_write().While here, also unified
_connect()'sapply_wal_with_fallback()call to use the samedb_label="state.db"SessionDBitself uses for this physical file — this folds in the fix from #64491 (opened separately before the rebase surfaced they'd collide), which is now redundant and can be closed. A distinct label would have double-logged the same NFS/SMB WAL-fallback warning for one physical file. Added #64491's own 2 regression tests (test_connect_routes_through_shared_wal_fallback_helper,test_connect_falls_back_gracefully_on_wal_incompatible_filesystem), mutation-verified.