fix(sqlite): initialise an empty --lock-file before the Windows byte lock
On the msvcrt branch of scripts/ensure_state_db_read_indexes.py the tool locked byte 0 of --lock-file right after open(..., "a+"). When that open had just created the file (or the file was empty) the lock range lay beyond EOF, which Windows refuses, so the documented locked-maintenance workflow raised instead of acquiring the lock (greptile P1). Write one byte when the lock file is empty, then seek(0) and lock as before. Only the Windows branch changes; POSIX flock is untouched and an existing (deployment-owned) lock file is never rewritten. Tests (tests/test_pr7445_readonly_review_followups.py): a strict msvcrt stand-in that refuses to lock past EOF proves the lock is taken on byte 0 of a fresh empty file, that an existing lock file keeps its content, and that a held lock still propagates before the database is touched.
This commit is contained in:
1 parent
f5a7ef2049
commit
70146b74d9
3 files changed
+98
-2
No files matched your search
@@ -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.
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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() == []
|
||||
Reference in new issue
Block a user