diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index c6fcffd8..618efa9c 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -232,7 +232,7 @@ python3 scripts/ensure_state_db_read_indexes.py --db ~/.hermes/state.db --confir ``` - `--confirm-drained` is mandatory: it is your assertion that no agent turn is running against the database. The tool does not verify it. -- `--lock-file PATH` (optional) additionally holds an exclusive non-blocking lock on `PATH` (`flock` on POSIX, `msvcrt.locking` on Windows) for deployments that serialise agent turns on a lock file; a held lock makes the tool exit without touching the database. Without `--lock-file` no lock primitive is required, so the script runs on native Windows as well. +- `--lock-file PATH` (optional) additionally holds an exclusive non-blocking lock on `PATH` (`flock` on POSIX, `msvcrt.locking` on Windows) for deployments that serialise agent turns on a lock file; a held lock makes the tool exit without touching the database. Without `--lock-file` no lock primitive is required, so the script runs on native Windows as well. On Windows the lock covers byte 0 of `PATH`; a new or empty lock file is initialised with one byte first (an existing lock file is never rewritten). - The tool opens the database `mode=rw` (never `rwc`): a mistyped path raises instead of creating an empty database. Indexes are created inside one `BEGIN IMMEDIATE` transaction and rolled back on any error. - It is idempotent and prints a JSON status per index (`created` / `existing`). An existing index with a different table, key shape or collation is reported as `Incompatible index` and never replaced; an index that is not covering (`EXPLAIN QUERY PLAN`) is reported as `Index is not covering`. - Windows UNC profiles (`HERMES_HOME=\\server\share\...`) are supported: readers and this tool build the empty-authority URI `file:////server/share/state.db` that the bundled SQLite accepts. diff --git a/scripts/ensure_state_db_read_indexes.py b/scripts/ensure_state_db_read_indexes.py index 2513b0fc..2eedc9a4 100644 --- a/scripts/ensure_state_db_read_indexes.py +++ b/scripts/ensure_state_db_read_indexes.py @@ -26,6 +26,7 @@ silently replaced. import argparse from contextlib import closing, nullcontext import json +import os from pathlib import Path import sqlite3 import sys @@ -64,6 +65,12 @@ def _exclusive_lock(lock_file): fcntl.flock(handle, fcntl.LOCK_EX | fcntl.LOCK_NB) else: # Lock one byte at offset 0 without blocking; raises OSError when held. + # ``a+`` may have just created an empty file, and Windows cannot lock + # a range beyond EOF, so make sure byte 0 exists first. An existing + # (deployment-owned) lock file is left untouched. + if os.fstat(handle.fileno()).st_size == 0: + handle.write("\n") + handle.flush() handle.seek(0) msvcrt.locking(handle.fileno(), msvcrt.LK_NBLCK, 1) except BaseException: diff --git a/tests/test_pr7445_readonly_review_followups.py b/tests/test_pr7445_readonly_review_followups.py index a2ecd444..f99c1548 100644 --- a/tests/test_pr7445_readonly_review_followups.py +++ b/tests/test_pr7445_readonly_review_followups.py @@ -15,9 +15,13 @@ Contract under test: query string is always the strict ``mode=ro``; * ``scripts/ensure_state_db_read_indexes.py`` imports and runs its no-lock path on a platform without ``fcntl`` (native Windows), and refuses ``--lock-file`` - there with a clear error instead of an ``ImportError`` at import time. + there with a clear error instead of an ``ImportError`` at import time; +* on the ``msvcrt`` branch the tool locks byte 0 of the lock file only after + making sure that byte exists, so a fresh (empty) ``--lock-file`` is acquired + without relying on the CRT's beyond-EOF locking behaviour. """ import importlib +import os import sqlite3 import sys from contextlib import closing @@ -260,3 +264,88 @@ def test_maintenance_cli_without_lock_file_works_without_fcntl(tmp_path, mainten module.main() out = capsys.readouterr().out assert '"idx_messages_session": "created"' in out + + +# -------------------------------------------------------------------------- +# 4. The Windows lock branch acquires a fresh (empty) lock file. +# -------------------------------------------------------------------------- + +class _StrictMsvcrt: + """Stand-in for ``msvcrt`` that, like Windows, refuses to lock past EOF. + + Records every ``locking`` call with the file size at the current offset so + the test can prove byte 0 existed when the lock was taken. + """ + + LK_NBLCK = 2 + LK_UNLCK = 0 + + def __init__(self): + self.calls = [] + + def locking(self, fd, mode, nbytes): + offset = os.lseek(fd, 0, os.SEEK_CUR) + size = os.fstat(fd).st_size + self.calls.append((mode, offset, nbytes, size)) + if mode == self.LK_NBLCK and offset + nbytes > size: + raise OSError(36, "Resource deadlock avoided: lock region beyond end of file") + + +def test_windows_lock_branch_acquires_a_fresh_empty_lock_file(tmp_path, maintenance_without_fcntl, monkeypatch): + module = maintenance_without_fcntl + assert module.fcntl is None # the fixture already routes the tool to the msvcrt branch + fake = _StrictMsvcrt() + monkeypatch.setattr(module, "msvcrt", fake) + + path = tmp_path / "state.db" + _maintenance_schema(path) + lock = tmp_path / "turns.lock" + assert not lock.exists() + + result = module.ensure_read_indexes(path, confirmed_drained=True, lock_file=lock) + + assert set(result.values()) == {"created"} + # Exactly one non-blocking lock on byte 0, taken while that byte existed. + assert fake.calls == [(fake.LK_NBLCK, 0, 1, 1)] + assert lock.stat().st_size == 1 + with closing(sqlite3.connect(str(path))) as conn: + names = {row[1] for row in conn.execute("PRAGMA index_list(messages)")} + assert "idx_messages_session" in names + + +def test_windows_lock_branch_keeps_an_existing_lock_file_intact(tmp_path, maintenance_without_fcntl, monkeypatch): + module = maintenance_without_fcntl + fake = _StrictMsvcrt() + monkeypatch.setattr(module, "msvcrt", fake) + + path = tmp_path / "state.db" + _maintenance_schema(path) + lock = tmp_path / "turns.lock" + lock.write_bytes(b"deployment-owned\n") + + module.ensure_read_indexes(path, confirmed_drained=True, lock_file=lock) + + assert fake.calls == [(fake.LK_NBLCK, 0, 1, len(b"deployment-owned\n"))] + assert lock.read_bytes() == b"deployment-owned\n" + + +def test_windows_lock_branch_reports_a_held_lock_without_touching_the_db(tmp_path, maintenance_without_fcntl, monkeypatch): + module = maintenance_without_fcntl + + class _HeldMsvcrt(_StrictMsvcrt): + def locking(self, fd, mode, nbytes): + super().locking(fd, mode, nbytes) + raise PermissionError(13, "Permission denied: lock held") + + fake = _HeldMsvcrt() + monkeypatch.setattr(module, "msvcrt", fake) + path = tmp_path / "state.db" + _maintenance_schema(path) + lock = tmp_path / "turns.lock" + + with pytest.raises(PermissionError): + module.ensure_read_indexes(path, confirmed_drained=True, lock_file=lock) + + assert fake.calls == [(fake.LK_NBLCK, 0, 1, 1)] + with closing(sqlite3.connect(str(path))) as conn: + assert conn.execute("PRAGMA index_list(messages)").fetchall() == []