From 2609a10508dc842a973895d1fc5aaeee556c78b5 Mon Sep 17 00:00:00 2001 From: JasonBuildAI Date: Sun, 11 Oct 2026 03:07:36 +0800 Subject: [PATCH 1/2] fix(file-lock): name the Windows holder sidecar without a .json suffix The Windows lock-holder sidecar was named `.json.lock.holder.json`, so any `*.json` state store over the locked directory matched it. External chat admission locks `.json` and then enumerates sibling requests with `ChatExternalConversations.pending()` (`glob("*.json")`), which read the holder metadata as a request and crashed at `_admit_prepared` with `KeyError: 'binding_id'` (issue #6128). The same name also fed the lark delivery store, which raised `KeyError: 'status'` while walking the same directory. The sidecar is advisory lock metadata, not state, so drop the trailing `.json` and keep `.holder` (`.json.lock.holder`). Every reader already resolves the path through `lock_holder_path`, so only name-based `*.json` filters need updating; the POSIX single-file holder record is unchanged. Closes #6128 Signed-off-by: JasonBuildAI --- docs/reference/protocols/file-lock-acquisition-v0.md | 2 +- loopx/file_lock.py | 5 ++++- tests/test_file_lock.py | 4 ++++ tests/test_loopx_turn_driver.py | 2 +- tests/test_loopx_turn_executor.py | 8 ++++---- tests/test_loopx_turn_journal_inspection.py | 4 ++-- 6 files changed, 16 insertions(+), 9 deletions(-) diff --git a/docs/reference/protocols/file-lock-acquisition-v0.md b/docs/reference/protocols/file-lock-acquisition-v0.md index a0364f7a08..1424dc470f 100644 --- a/docs/reference/protocols/file-lock-acquisition-v0.md +++ b/docs/reference/protocols/file-lock-acquisition-v0.md @@ -21,7 +21,7 @@ unbounded `LOCK_EX` wait is not part of this contract. ## Holder And Incident Records After acquisition, the holder writes public-safe JSON to the POSIX `*.lock` -file or atomically overwrites the Windows `*.lock.holder.json` sidecar: +file or atomically overwrites the Windows `*.lock.holder` sidecar: - stable hashed `lock_id` (never an absolute target path); - PID, agent id, operation, policy, and acquisition time; diff --git a/loopx/file_lock.py b/loopx/file_lock.py index fd5838575e..8e975ed9be 100644 --- a/loopx/file_lock.py +++ b/loopx/file_lock.py @@ -151,7 +151,10 @@ def _open_lock_descriptor(path: Path, *, flags: int) -> int: def lock_holder_path(path: Path) -> Path: lock_path = _lock_path(path) if os.name == "nt": - return lock_path.with_name(f"{lock_path.name}.holder.json") + # The sidecar is ephemeral lock metadata, not state. Its name must not + # end in .json so a *.json state walker never harvests a holder that a + # concurrent lock deletes mid-walk (issue #6128). + return lock_path.with_name(f"{lock_path.name}.holder") return lock_path diff --git a/tests/test_file_lock.py b/tests/test_file_lock.py index 4fa8ee1b26..f2f417dc97 100644 --- a/tests/test_file_lock.py +++ b/tests/test_file_lock.py @@ -97,6 +97,7 @@ def _acquire_and_release_cross_runtime_lock( def test_exclusive_lock_persists_public_safe_holder_metadata(tmp_path: Path) -> None: target = tmp_path / "state.json" + target.write_text("{}\n", encoding="utf-8") with exclusive_file_lock( target, @@ -113,6 +114,9 @@ def test_exclusive_lock_persists_public_safe_holder_metadata(tmp_path: Path) -> assert holder["operation"] == "todo-update" assert holder["acquired_at"].endswith("Z") assert "released_at" not in holder + # The Windows holder sidecar is ephemeral lock metadata: a *.json state + # walker must never harvest it and read it as a record (issue #6128). + assert sorted(p.name for p in tmp_path.glob("*.json")) == [target.name] released = json.loads(holder_path.read_text(encoding="utf-8")) assert released["released_at"].endswith("Z") diff --git a/tests/test_loopx_turn_driver.py b/tests/test_loopx_turn_driver.py index 7c93bdff98..d09048caf4 100644 --- a/tests/test_loopx_turn_driver.py +++ b/tests/test_loopx_turn_driver.py @@ -2057,7 +2057,7 @@ def interrupt_checkpoint(path, journal): for path in (runtime / "goals" / "loopx-turn-fixture" / "turns").glob( "*.json" ) - if not path.name.endswith(".lock.holder.json") + if not path.name.endswith(".lock.holder") ] assert len(journal_paths) == 1 stored = json.loads(journal_paths[0].read_text(encoding="utf-8")) diff --git a/tests/test_loopx_turn_executor.py b/tests/test_loopx_turn_executor.py index 0830d6a68e..4e30703b20 100644 --- a/tests/test_loopx_turn_executor.py +++ b/tests/test_loopx_turn_executor.py @@ -530,7 +530,7 @@ def _journal(runtime_root: Path) -> dict[str, object]: journal_paths = [ path for path in (runtime_root / "goals" / "fixture-goal" / "turns").glob("*.json") - if not path.name.endswith(".lock.holder.json") + if not path.name.endswith(".lock.holder") ] assert len(journal_paths) == 1 return json.loads(journal_paths[0].read_text(encoding="utf-8")) @@ -1226,7 +1226,7 @@ def _cadence_starts(runtime_root: Path) -> list[dict[str, object]]: stores = [] for path in sorted(runtime_root.rglob("*.json")): - if path.name.endswith(".lock.holder.json"): + if path.name.endswith(".lock.holder"): continue payload = json.loads(path.read_text(encoding="utf-8")) if ( @@ -1321,7 +1321,7 @@ def die_after_reservation(identity: Mapping[str, object]) -> dict[str, object]: assert not [ path for path in (runtime_root / "goals" / "fixture-goal" / "turns").glob("*.json") - if not path.name.endswith(".lock.holder.json") + if not path.name.endswith(".lock.holder") ] assert calls == {"host": 0, "writeback": 0, "spend": 0, "scheduler": 0} @@ -1702,7 +1702,7 @@ def host(request: dict[str, object]) -> dict[str, object]: for path in (tmp_path / "runtime" / "goals" / "fixture-goal" / "turns").glob( "*.json" ) - if not path.name.endswith(".lock.holder.json") + if not path.name.endswith(".lock.holder") ] assert len(journal_paths) == 1 journal = json.loads(journal_paths[0].read_text(encoding="utf-8")) diff --git a/tests/test_loopx_turn_journal_inspection.py b/tests/test_loopx_turn_journal_inspection.py index 7d77653359..6064195b65 100644 --- a/tests/test_loopx_turn_journal_inspection.py +++ b/tests/test_loopx_turn_journal_inspection.py @@ -216,7 +216,7 @@ def test_inspection_returns_versioned_allowlisted_projection_without_mutation( before_entries = { path.relative_to(tmp_path) for path in tmp_path.rglob("*") - if not path.name.endswith((".lock", ".lock.holder.json")) + if not path.name.endswith((".lock", ".lock.holder")) } result = executor.inspect_loopx_turn_journal( @@ -229,7 +229,7 @@ def test_inspection_returns_versioned_allowlisted_projection_without_mutation( after_entries = { path.relative_to(tmp_path) for path in tmp_path.rglob("*") - if not path.name.endswith((".lock", ".lock.holder.json")) + if not path.name.endswith((".lock", ".lock.holder")) } assert result == { "ok": True, From 96955af0c8e5a23db93852ca5ee8aecb6d86a0a3 Mon Sep 17 00:00:00 2001 From: JasonBuildAI Date: Sun, 11 Oct 2026 04:15:10 +0800 Subject: [PATCH 2/2] fix(file-lock): clear the pre-rename holder sidecar and guard record walkers The Windows holder sidecar lost its .json suffix, but a runtime root upgraded from a release that wrote `.holder.json` still carries that file, and a `*.json` state store reads it as a record: external chat admission then raised KeyError `binding_id`, and the lark delivery store raised KeyError `status` (issue #6128). Acquisition now best-effort deletes the legacy sibling while the lock is held, and the three record walkers accept only the 24-hex request reference that admission already enforces, matching the existing collaboration-inbox guard. Verified: pending()/health()/pending_delivery_paths() raise KeyError on the legacy sibling without these changes, and their new regression tests pass with them. Signed-off-by: JasonBuildAI --- .../protocols/file-lock-acquisition-v0.md | 5 ++++ .../native_chat/external_conversations.py | 15 ++++++++---- .../extensions/lark/private_conversations.py | 12 ++++++++-- loopx/file_lock.py | 23 ++++++++++++++++--- tests/test_collaboration_sidecar_scan.py | 16 +++++++++---- tests/test_file_lock.py | 20 ++++++++++++++-- tests/test_lark_private_conversations.py | 23 +++++++++++++++++++ 7 files changed, 97 insertions(+), 17 deletions(-) diff --git a/docs/reference/protocols/file-lock-acquisition-v0.md b/docs/reference/protocols/file-lock-acquisition-v0.md index 1424dc470f..e96803db91 100644 --- a/docs/reference/protocols/file-lock-acquisition-v0.md +++ b/docs/reference/protocols/file-lock-acquisition-v0.md @@ -32,6 +32,11 @@ handle from reading the locked byte. POSIX retains the existing single-file contract, where advisory metadata and `flock` share `*.lock`. In both cases the kernel lock, not the metadata file's existence, is authoritative. +The Windows sidecar name carries no `.json` suffix, so a `*.json` state store +never reads lock metadata as a record (issue #6128). Acquisition also deletes +any `.holder.json` sibling left by a release before that rename, while the +lock is held, so an upgraded runtime root stops presenting the old artifact. + A timeout appends one `file_lock_incident_v0` row to the sibling `*.lock.incidents.jsonl` channel. That append uses `O_APPEND` directly and does not acquire the blocked lock. The row contains holder and waiter identities, diff --git a/loopx/capabilities/native_chat/external_conversations.py b/loopx/capabilities/native_chat/external_conversations.py index bbd17dda79..e250f14308 100644 --- a/loopx/capabilities/native_chat/external_conversations.py +++ b/loopx/capabilities/native_chat/external_conversations.py @@ -9,12 +9,18 @@ from pathlib import Path from datetime import datetime, timedelta, timezone import hashlib +import re from typing import Any from ...chat_store import _atomic_write_json, _read_json from ...chat_attachments import normalize_chat_image_attachments from ...file_lock import exclusive_file_lock +# An external request reference is a 24-hex digest: it names the journal file +# and enters the lock path, so a record walker accepts only that shape and never +# a sibling lock artifact such as the holder sidecar (issue #6128). +_REQUEST_REF_PATTERN = re.compile(r"[a-f0-9]{24}") + def _commission_goal_identity(goal: dict[str, Any]) -> dict[str, str]: identity: dict[str, str] = {} @@ -37,10 +43,9 @@ def admit(self, *, binding_id: str, source: dict[str, Any], request_ref: str, message: str, command: str | None = None, attachments: list[dict[str, Any]] | None = None, origin: str = "lark") -> dict[str, Any]: - import re if origin not in {"lark", "external"}: raise ValueError("unsupported external conversation origin") - if not re.fullmatch(r"[a-f0-9]{24}", request_ref): + if not _REQUEST_REF_PATTERN.fullmatch(request_ref): raise ValueError("invalid external request reference") if command not in {None, "agents", "select_agent", "select_project", "status", "help", "new", "stop", "unsupported", "commission", "confirm_commission", "cancel_commission", "stop_commission", "resume_commission"}: raise ValueError("unsupported external conversation command") @@ -48,7 +53,7 @@ def admit(self, *, binding_id: str, source: dict[str, Any], request_ref: str, # Check path-safe shape here; the typed owner checks current grants # and provider identity after both fences are acquired. for ref in (binding_id, source.get("source_ref")): - if not isinstance(ref, str) or not re.fullmatch(r"[a-f0-9]{24}", ref): + if not isinstance(ref, str) or not _REQUEST_REF_PATTERN.fullmatch(ref): raise ValueError("invalid external conversation source reference") path = self.root / f"{request_ref}.json" with exclusive_file_lock(self.root / "source-fences" / f"{binding_id}.{source['source_ref']}.json", operation="route_external_chat_request"), exclusive_file_lock(path, operation="admit_external_chat_request"): @@ -157,7 +162,6 @@ def _admit_prepared(self, path: Path, row: dict[str, Any], selected: dict[str, A except (ValueError, OSError, KeyError): continue if row["command"] == "select_agent": - import re match = re.fullmatch(r"/agent ([a-f0-9]{24})", row["message"].strip()) chosen = next((item for item in choices if match and item["target_ref"] == match[1]), None) if chosen is None: @@ -251,7 +255,8 @@ def _record_steward_ingress(self, row: dict[str, Any], selected: dict[str, Any], message=row["message"], source_id=row["request_ref"]) def pending(self) -> list[dict[str, Any]]: - return [_read_json(path) for path in sorted(self.root.glob("*.json"))] + return [_read_json(path) for path in sorted(self.root.glob("*.json")) + if _REQUEST_REF_PATTERN.fullmatch(path.stem)] def read_request(self, request_ref: str) -> dict[str, Any]: return _read_json(self.root / f"{request_ref}.json") diff --git a/loopx/extensions/lark/private_conversations.py b/loopx/extensions/lark/private_conversations.py index c8b139b5e5..e9bee79f61 100644 --- a/loopx/extensions/lark/private_conversations.py +++ b/loopx/extensions/lark/private_conversations.py @@ -32,6 +32,11 @@ # Reserve edits for a final/closing notice; continue long Turns on a new draft. PROGRESS_EDIT_BUDGET = 12 +# A private delivery is keyed by its 24-hex request reference, which also names +# the journal file and its lock. A record walker accepts only that shape and +# never a sibling lock artifact such as the holder sidecar (issue #6128). +_REQUEST_REF_PATTERN = re.compile(r"[a-f0-9]{24}") + class LarkPrivateConversations: def __init__(self, *, controller: Any, runtime_root: Path, runner: Any, cli_bin: str, @@ -76,6 +81,8 @@ def profiles(self) -> dict[str, dict[str, str]]: def health(self) -> dict[str, dict[str, int]]: health: dict[str, dict[str, int]] = {} for path in self.root.glob("*.json"): + if not _REQUEST_REF_PATTERN.fullmatch(path.stem): + continue row = _read_json(path) entry = health.setdefault(row["binding_id"], {"pending_count": 0, "recovery_count": 0}) if row["status"] != "delivered": @@ -316,7 +323,7 @@ def return_inbox(self, *, route: dict[str, Any], session: dict[str, Any], selected = self.bindings.session_context(saved) client = str(turn.get("client_turn_id") or "") request = client.removeprefix("external-") - if client != f"external-{request}" or not re.fullmatch(r"[a-f0-9]{24}", request): + if client != f"external-{request}" or not _REQUEST_REF_PATTERN.fullmatch(request): raise ValueError("original private request unavailable") record = _read_json(self.root / f"{request}.json") native = self.core.read_request(request) @@ -571,7 +578,8 @@ def _progress(self, path: Path, record: dict[str, Any], *, inbox: Callable[[], P def pending_delivery_paths(self) -> list[Path]: """The durable transport store is the queue; no in-memory admission.""" return [path for path in sorted(self.root.glob("*.json")) - if _read_json(path)["status"] != "delivered"] + if _REQUEST_REF_PATTERN.fullmatch(path.stem) + and _read_json(path)["status"] != "delivered"] def reconcile(self) -> int: return sum(self.reconcile_request(path) for path in self.pending_delivery_paths()) diff --git a/loopx/file_lock.py b/loopx/file_lock.py index 8e975ed9be..364052521c 100644 --- a/loopx/file_lock.py +++ b/loopx/file_lock.py @@ -151,9 +151,9 @@ def _open_lock_descriptor(path: Path, *, flags: int) -> int: def lock_holder_path(path: Path) -> Path: lock_path = _lock_path(path) if os.name == "nt": - # The sidecar is ephemeral lock metadata, not state. Its name must not - # end in .json so a *.json state walker never harvests a holder that a - # concurrent lock deletes mid-walk (issue #6128). + # The sidecar is advisory lock metadata, not state. Its name must not + # end in .json: a *.json state walker reads every match as a record, so + # it would read this holder as one (issue #6128). return lock_path.with_name(f"{lock_path.name}.holder") return lock_path @@ -245,6 +245,22 @@ def _write_holder_sidecar(holder_path: Path, record: dict[str, object]) -> None: temporary_path.unlink(missing_ok=True) +def _discard_legacy_holder_sidecar(lock_path: Path) -> None: + """Best-effort removal of a pre-rename ``.holder.json`` sidecar. + + Releases before the holder rename wrote the Windows sidecar with a ``.json`` + suffix, so a ``*.json`` state store harvested lock metadata and read it as a + record (issue #6128). The renamed sidecar carries no suffix and no longer + matches, but an upgraded runtime root still holds the old file. Clear that + sibling while this lock is held so the fix also covers existing installs. + """ + + try: + lock_path.with_name(f"{lock_path.name}.holder.json").unlink() + except OSError: + return + + def _persist_holder_record( lock_file: TextIO, *, @@ -252,6 +268,7 @@ def _persist_holder_record( holder_path: Path, record: dict[str, object], ) -> None: + _discard_legacy_holder_sidecar(lock_path) if holder_path == lock_path: _write_holder_record(lock_file, record) return diff --git a/tests/test_collaboration_sidecar_scan.py b/tests/test_collaboration_sidecar_scan.py index 9585aabfd3..560e8e7878 100644 --- a/tests/test_collaboration_sidecar_scan.py +++ b/tests/test_collaboration_sidecar_scan.py @@ -45,15 +45,21 @@ def test_sidecars(tmp_path_factory): operation = next((store / "peer-operations").glob(f"*/{operation_id}.json")) for record in (entry, operation): - sidecar = record.with_name(f"{record.stem}.lock.lock.holder.json") - # Windows creates this on every lock acquisition. Simulate the same - # persistent artifact on POSIX so both CI platforms guard the scan. - if not sidecar.exists(): + # A Windows lock acquisition writes .lock beside this record and + # a .lock.holder sidecar; before the .json rename the sidecar was + # .lock.holder.json, which a *.json scan can still harvest on an + # upgraded store (issue #6128). Simulate all three on POSIX so both CI + # platforms guard the scan. + for sidecar in ( + record.with_name(f"{record.name}.lock"), + record.with_name(f"{record.name}.lock.holder"), + record.with_name(f"{record.name}.lock.holder.json"), + ): sidecar.write_text( json.dumps({"schema_version": "file_lock_holder_v0"}), encoding="utf-8", ) - assert sidecar.exists() + assert sidecar.exists() entry.with_name("foreign.json").write_text("{}", encoding="utf-8") page = pending(tmp_path, "delivery", "receiver") diff --git a/tests/test_file_lock.py b/tests/test_file_lock.py index f2f417dc97..45254f5bdf 100644 --- a/tests/test_file_lock.py +++ b/tests/test_file_lock.py @@ -114,8 +114,8 @@ def test_exclusive_lock_persists_public_safe_holder_metadata(tmp_path: Path) -> assert holder["operation"] == "todo-update" assert holder["acquired_at"].endswith("Z") assert "released_at" not in holder - # The Windows holder sidecar is ephemeral lock metadata: a *.json state - # walker must never harvest it and read it as a record (issue #6128). + # The Windows holder sidecar is advisory lock metadata, not a record: a + # *.json state store must never harvest it as one (issue #6128). assert sorted(p.name for p in tmp_path.glob("*.json")) == [target.name] released = json.loads(holder_path.read_text(encoding="utf-8")) @@ -124,6 +124,22 @@ def test_exclusive_lock_persists_public_safe_holder_metadata(tmp_path: Path) -> assert holder_path.exists() +def test_acquisition_discards_a_pre_rename_json_holder_sidecar(tmp_path: Path) -> None: + target = tmp_path / "state.json" + target.write_text("{}\n", encoding="utf-8") + # A release before the holder rename left this *.json sibling behind, and a + # *.json store then read lock metadata as a request. Acquisition clears it. + legacy = tmp_path / "state.json.lock.holder.json" + legacy.write_text('{"schema_version": "file_lock_holder_v0"}\n', encoding="utf-8") + + with exclusive_file_lock(target, operation="todo-update") as lock_path: + assert lock_path.name == "state.json.lock" + assert not legacy.exists() + + assert not legacy.exists() + assert sorted(p.name for p in tmp_path.glob("*.json")) == [target.name] + + @pytest.mark.skipif(os.name == "nt", reason="symlink creation requires privileges") def test_exclusive_lock_rejects_a_symlinked_lock_file(tmp_path: Path) -> None: target = tmp_path / "state.json" diff --git a/tests/test_lark_private_conversations.py b/tests/test_lark_private_conversations.py index 0f2ae4537f..5b2e8110f5 100644 --- a/tests/test_lark_private_conversations.py +++ b/tests/test_lark_private_conversations.py @@ -1337,3 +1337,26 @@ def test_scoped_recovery_does_not_probe_unrelated_app_and_revocation_blocks_repl assert provider.writes == [] and store.list_sessions() == [] finally: runtime.close() + + +def test_lock_metadata_sidecar_is_never_read_as_a_request(ordinary): # noqa: F811 + """A Windows lock sibling is metadata, not a delivery record (#6128).""" + _store, runtime, _provider, transport = connect(ordinary) + try: + reference = "0" * 24 + record = {"schema_version": "lark_private_chat_delivery_v0", "binding_id": "binding", + "status": "captured", "deliveries": {}} + for root in (transport.root, transport.core.root): + root.mkdir(parents=True, exist_ok=True) + (root / f"{reference}.json").write_text(json.dumps(record), encoding="utf-8") + (root / f"{reference}.json.lock").write_text("", encoding="utf-8") + # Releases before the holder rename left this *.json lock sibling, so + # a *.json store read the holder as a request and raised KeyError. + (root / f"{reference}.json.lock.holder.json").write_text( + json.dumps({"schema_version": "file_lock_holder_v0"}), encoding="utf-8") + + assert [path.stem for path in transport.pending_delivery_paths()] == [reference] + assert transport.health() == {"binding": {"pending_count": 1, "recovery_count": 0}} + assert [row["binding_id"] for row in transport.core.pending()] == ["binding"] + finally: + runtime.close()