fix(state): make malformed-DB backup copies atomic with the live-connection registry - #71982
fix(state): make malformed-DB backup copies atomic with the live-connection registry#71982stalker023reg wants to merge 2 commits into
Conversation
…ection registry The snapshot path in session_recovery was moved onto offline_file_access when the live-connection registry landed (NousResearch#71779), but the two other raw-copy sites kept the older shape: check has_live_connection(), then copy. That is a check/use race the module's own docstring warns about — a connection opened in the window between the check and the raw close() has its POSIX advisory locks cancelled, which is the documented route to "database disk image is malformed" and exactly what the registry exists to prevent. Route both remaining sites through offline_file_access, which holds the connection-lifecycle lock across the whole raw copy so connect_tracked blocks instead of slipping into the gap: * hermes_state._backup_db_file — the malformed-DB backup taken before schema surgery, called from SessionDB open-time self-heal, where the gateway, dashboard, and dispatcher share a process and hold other connections to the same state.db. * hermes_cli.kanban_db._backup_corrupt_db — the corrupt-board quarantine, which fingerprints the whole file (a second raw read) before copying. _backup_db_file keeps a registry-less fallback: in constrained embeds where hermes_cli.sqlite_safe_read can't be imported, there is no tracked connection to endanger, so the best-effort copy still runs. This is a different axis than NousResearch#69609, which serializes the schema surgery across processes with a flock sidecar: that guards two repairers against each other, while this guards the backup copy against a connection opened in the same process. Neither change touches the other's lines. Tested on: Linux (pytest).
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for completing the raw-copy audit. The production change matches the existing offline_file_access contract: current main still has bare check-then-copy paths in hermes_state.py:978-994 and hermes_cli/kanban_db.py:1764-1802, while the guard explicitly exists to make that operation atomic (hermes_cli/sqlite_safe_read.py:383-409).
Problems
tests/test_raw_copy_offline_guard.py:88-91can recordlanded_during_copy=Falsemerely because the connector has not been scheduled throughconnect_trackedbefore the fixed 0.5-second join expires.startedis set before that call and thestarted.wait(timeout=5)result is ignored, so this does not reliably distinguish a guarded implementation from an unguarded one.
Suggested changes
- Use the event-controlled park/release structure already used by
tests/hermes_cli/test_session_recovery.py:305-389: hold the copy open, confirm the connector attempted its call but did not open, then release and confirm it opens.
This is an automated hermes-sweeper review.
| thread.start() | ||
| started.wait(timeout=5) | ||
| # Give the connector every chance to slip in if nothing blocks it. | ||
| thread.join(timeout=0.5) |
There was a problem hiding this comment.
started is set before connect_tracked, and this fixed 0.5-second join can expire simply because the connector was not scheduled. The resulting False is then treated as proof of locking, so an unguarded implementation can pass. Please use a park/release event protocol like tests/hermes_cli/test_session_recovery.py:305-389 to prove the connector attempted to open while the copy is held.
There was a problem hiding this comment.
Fair point. The 0.5-second join is inherently a timing dependency - a slow CI host could let the connector miss the window entirely and still return False, giving a false pass on an unguarded implementation.
I'll rework the fixture to use a park/release event pair: block inside _copy2_with_race until the connector has had a chance to actually call into connect_tracked, then measure the verdict from within the lock's scope rather than from a wall-clock join. That way the probe either lands and the verdict is True, or it can't land (blocked by the guard) and stays False - no scheduling dependency.
There was a problem hiding this comment.
Done. Replaced _connect_attempt_during with a park/release event protocol matching the pattern in tests/hermes_cli/test_session_recovery.py:
inside_copy/release_copypark the copy thread inside the patchedcopy2until explicitly released.connect_attemptedis set immediately beforeconnect_trackedis called, so a "still blocked" assertion can't pass just because the connector thread hadn't been scheduled yet.connection_openedis set inside the connector once the connection actually lands.
The assertion sequence in both atomic tests is now:
- Start copier, wait for
inside_copy(copy is parked, holds the lock). - Start connector, wait for
connect_attempted(connector is at the lock boundary). - Assert
connection_openedhas NOT fired within 1 s (lock ordering, not scheduling). - Release copy, assert
connection_openedfires within 30 s (lock released, connect succeeds).
Committed as test(state): replace timing-based join with park/release event protocol.
teknium1
added
the
sweeper:blast-contained
3 participants
What does this PR do?
Makes the two remaining raw-copy sites atomic with the live-connection registry, so a connection opened mid-copy can no longer have its POSIX advisory locks cancelled by the copy's
close().When the registry landed, the snapshot path in
session_recoverywas converted tooffline_file_access(#71779) precisely because a barehas_live_connection()check followed by raw file I/O is a check/use race — the context manager's own docstring names that pattern as the thing it exists to close. Two sites kept the older bare-check shape:hermes_state._backup_db_file— the malformed-DB backup taken before schema surgery. Reachable fromSessionDBopen-time self-heal, where the gateway, dashboard and dispatcher share one process and hold other connections to the samestate.db.hermes_cli.kanban_db._backup_corrupt_db— the corrupt-board quarantine, which additionally fingerprints the whole file (a second raw read) before copying.A
connect_tracked()that lands in the window between the check and the copy'sclose()loses its POSIX locks — the documented route todatabase disk image is malformed(sqlite.org/howtocorrupt §2.5), and the exact corruption class the registry was introduced to prevent. The window is small, but these functions run precisely when the database is already being repaired, i.e. when another surface retrying its open is most likely.Related Issue
No dedicated issue; this finishes the audit that #71724 / #71779 started. A different axis than #69609, which puts a
flocksidecar around the schema surgery: that guards two repairer processes against each other, while this guards the backup copy against a connection opened in the same process. Neither change touches the other's lines, so they can land in either order.Type of Change
Changes Made
hermes_state.py—_backup_db_fileruns its copies insideoffline_file_access(db_path, what="back up");LiveConnectionErrorkeeps the existing refusal log line andNonereturn. The registry-less fallback is preserved: in constrained embeds wherehermes_cli.sqlite_safe_readcan't be imported there is no tracked connection to endanger, so the best-effort copy still runs unguarded, as before.hermes_cli/kanban_db.py—_backup_corrupt_dbwraps the fingerprint + copy + sidecar block inoffline_file_access(resolved, what="quarantine"); the body moves to_backup_corrupt_db_lockedwith the lock requirement stated in its docstring. Refusal behaviour (log +None) unchanged.tests/test_raw_copy_offline_guard.py— six tests, three per site: refusal with a live connection; the race itself (a real thread attemptsconnect_trackedwhile the copy is in flight and must block on the lifecycle lock instead of landing in the gap); and the site-specific invariant (registry-less fallback for_backup_db_file, sidecar copies for_backup_corrupt_db).No behaviour change for callers: same return values, same log lines, same refusal conditions. The only difference is that the refusal decision and the raw I/O are now one atomic step.
How to Test
scripts/run_tests.sh tests/test_raw_copy_offline_guard.py -q— 6 passed.*_copy_is_atomic_with_the_registrytests were run againstmainfirst and fail there (the racingconnect_trackedlands mid-copy); they pass with this change. The refusal tests pass on both — that contract already existed and is pinned, not claimed as new.ruff check hermes_state.py hermes_cli/kanban_db.py tests/test_raw_copy_offline_guard.py— passed.python scripts/check-windows-footguns.pyon the three files — clean.Neighbouring suites:
tests/test_sqlite_lock_safe_inspection.pyand the bulk oftests/test_state_db_malformed_repair.py/tests/hermes_cli/test_kanban_db.pyare green; 4 failures in the latter two are identical on cleanmainin this environment (FTS5/subprocess setup it lacks) and unrelated to this change.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — the full suite was not run; the targeted tests listed above wereDocumentation & Housekeeping
docs/, docstrings) — the moved kanban body carries its lock-requirement docstring; no user-facing docs affectedcli-config.yaml.exampleif I added/changed config keys — N/A; no config changesCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Athreading+ file copies, no POSIX-only primitives; tests run on all platforms