From 694b1675cf53f45a10ba194beeb1718e6fb87072 Mon Sep 17 00:00:00 2001 From: Tom Softreck Date: Thu, 15 Oct 2026 17:20:38 +0200 Subject: [PATCH] fix: revalidate allocator preparation races under lock (ticket-217) --- planfile/core/allocator_sprints.py | 6 ++- planfile/core/store.py | 14 ++++-- project/TICKETS.md | 1 + project/ticket-217/README.md | 25 +++++++++ project/ticket-217/intent.json | 78 +++++++++++++++++++++++++++++ tests/test_allocator_preparation.py | 45 +++++++++++++++++ 6 files changed, 164 insertions(+), 5 deletions(-) create mode 100644 project/ticket-217/README.md create mode 100644 project/ticket-217/intent.json diff --git a/planfile/core/allocator_sprints.py b/planfile/core/allocator_sprints.py index 8c14b2e..720934d 100644 --- a/planfile/core/allocator_sprints.py +++ b/planfile/core/allocator_sprints.py @@ -57,6 +57,10 @@ def _digest(value: dict) -> str: return hashlib.sha256(json.dumps(value, sort_keys=True).encode()).hexdigest() +class SourceChangedError(RuntimeError): + """Authoritative source changed during optimistic preparation.""" + + MAX_LOCKED_REBUILD_BYTES = 5_000_000 @@ -96,7 +100,7 @@ def sprint_highwater(source: Path, prefix: str, *, max_rebuild_bytes: int | None data = load_yaml_text(text) highest = record_highwater(records(data), prefix) if signature(source) != before: - raise RuntimeError("Sprint changed while rebuilding allocator identity evidence") + raise SourceChangedError("Sprint changed while rebuilding allocator identity evidence") value = {"version": 1, "signature": before, "prefix": prefix, "source": source.name, "highest": highest} value["checksum"] = _digest(value) temporary = None diff --git a/planfile/core/store.py b/planfile/core/store.py index 814b13f..febb36a 100644 --- a/planfile/core/store.py +++ b/planfile/core/store.py @@ -693,6 +693,7 @@ def allocation_lock(self): again by _known_ticket_highwater while the mutation lock is held. """ from planfile.core.allocator_sprints import ( + SourceChangedError, signature, sprint_highwater, ) @@ -701,10 +702,15 @@ def allocation_lock(self): if not self._uses_sharded_storage(): prefix = self._read_config().get("prefix", "PLF") for source in self._all_sprint_files(): - before = signature(source) - highest = sprint_highwater(source, prefix) - if signature(source) == before: - prepared[(str(source), prefix)] = (before, highest) + try: + before = signature(source) + highest = sprint_highwater(source, prefix) + if signature(source) == before: + prepared[(str(source), prefix)] = (before, highest) + except (SourceChangedError, FileNotFoundError): + # Re-enumerate and rebuild bounded authoritative evidence + # under the lock; an optimistic race is not a reservation. + continue with self.mutation_lock(): # Scalar summaries also allow safe allocation when the optional # persistent cache cannot be written. Publish only inside the lock. diff --git a/project/TICKETS.md b/project/TICKETS.md index 7764f02..2f60802 100644 --- a/project/TICKETS.md +++ b/project/TICKETS.md @@ -126,4 +126,5 @@ This file indexes governance tickets without taking ownership of | **ticket-214** | [`README.md`](./ticket-214/README.md) | - | - | - | - | - | | **ticket-215** | [`README.md`](./ticket-215/README.md) | - | - | - | - | - | | **ticket-216** | [`README.md`](./ticket-216/README.md) | - | - | - | - | - | +| **ticket-217** | [`README.md`](./ticket-217/README.md) | - | - | - | - | - | diff --git a/project/ticket-217/README.md b/project/ticket-217/README.md new file mode 100644 index 0000000..3bf07cd --- /dev/null +++ b/project/ticket-217/README.md @@ -0,0 +1,25 @@ +# Ticket 217: Revalidate allocator preparation races under mutation lock + +- **ID**: ticket-217 +- **Owner**: agent:codex +- **Status**: IN_PROGRESS +- **Workflow state**: PUBLICATION +- **Created**: 2026-10-15 + +## Goal and scope + +PLF-18410 followup. SESSION_EXECUTION_AUTHORIZATION: user requests continued repairs, tests and protected merging. A source modification during cold allocator preparation currently aborts before acquiring the lock. Revalidate fresh authoritative state under the existing bounded locked fallback. + +## Acceptance criteria + +- [x] AC-01: A small source changed during preparation allocates above the fresh high-water mark. +- [x] AC-02: Large changed sources fail closed without reservation writes; unrelated parser errors propagate. +- [ ] AC-03: Regression suite, governance and independent protected delivery pass. + +## Boundaries + +One writer; maximum active session 120 minutes. No storage migration, changed identity rules or weakened rebuild bound. Cold-start latency remains separate followup work. + +## Validation evidence + +Before fix: 2 regression failures, 11 passes. After fix: 13 focused tests pass; changed files pass Ruff. Full suite and protected publication receipts are held outside the checkout. diff --git a/project/ticket-217/intent.json b/project/ticket-217/intent.json new file mode 100644 index 0000000..8b6614b --- /dev/null +++ b/project/ticket-217/intent.json @@ -0,0 +1,78 @@ +{ + "schema": "new-project.intent/v3", + "ticket": "ticket-217", + "summary": "Revalidate allocator preparation races under mutation lock", + "workstream": "application", + "classification": { + "kind": "BUG", + "priority": "P1", + "origin": "requested" + }, + "allowedPaths": [ + "project/ticket-217/**", + "TODO.md", + "project/TICKETS.md", + "planfile/core/allocator_sprints.py", + "planfile/core/store.py", + "tests/test_allocator_preparation.py" + ], + "forbiddenPaths": [ + "project/ticket-*/user-*.md" + ], + "stacks": [ + "python" + ], + "dependsOn": [], + "conflictsWith": [], + "integrationTicket": null, + "delivery": { + "acceptedBaseSha": "c0fcf9cb24c196e066946e133307c70863aeb9da", + "targetBranch": "main", + "outcome": "Revalidate optimistic allocator source races under the mutation lock", + "nonGoals": [ + "No changes to allocation identity rules, cache format or rebuild limits" + ], + "complexity": "S", + "estimatedMinutes": 30, + "budgets": { + "maxImplementationFiles": 3, + "maxAffectedComponents": 1, + "maxPublicInterfaceChanges": 0, + "maxRuntimeDependencies": 0 + }, + "architecture": { + "status": "accepted", + "decision": "Catch only typed source races during preparation and retain bounded authoritative locked fallback", + "components": [ + { + "name": "allocation", + "paths": [ + "planfile/core/allocator_sprints.py", + "planfile/core/store.py", + "tests/test_allocator_preparation.py" + ] + } + ], + "responsibilityChanges": false, + "interfaceChanges": [], + "dataChanges": [], + "ui": { + "impact": "none", + "states": [], + "evidence": [] + }, + "rollback": "Revert race handling; no data migration" + }, + "runtimeDependencies": [], + "validation": [ + { + "criterion": "AC-03", + "commands": [ + "python -m pytest -q", + "./project/governance-check.sh" + ], + "evidence": "Full suite and independent protected CI receipts" + } + ] + } +} diff --git a/tests/test_allocator_preparation.py b/tests/test_allocator_preparation.py index b7209dd..4b2b35d 100644 --- a/tests/test_allocator_preparation.py +++ b/tests/test_allocator_preparation.py @@ -140,3 +140,48 @@ def observed(): allocated = [t.id for t in pf.create_tickets_bulk([{"name": "one"}, {"name": "two"}])] assert preparations == [True] assert allocated == (["PLF-901", "PLF-902"] if entry == "bulk" else ["PLF-901"]) + + +@pytest.mark.parametrize("large", [False, True]) +def test_source_changed_during_preparation_is_revalidated(tmp_path, monkeypatch, large): + store = Planfile(tmp_path).store + store.init() + path = archive(store) + original = evidence.load_yaml_text + changed = False + + def concurrent_parse(text): + nonlocal changed + result = original(text) + if "PLF-900" in text and not changed: + changed = True + path.write_text(text.replace("PLF-900", "PLF-999") + + ("#" + "x" * 101 if large else "")) + return result + + monkeypatch.setattr(evidence, "load_yaml_text", concurrent_parse) + monkeypatch.setattr(evidence, "MAX_LOCKED_REBUILD_BYTES", 100) + before = store._read_config() + if large: + with pytest.raises(RuntimeError, match="allocator_evidence_changed_retry"): + store.next_id() + assert store._read_config() == before + else: + assert store.next_id() == "PLF-1000" + assert changed + assert store._prepared_allocator_evidence == {} + + +def test_preparation_preserves_unrelated_parser_failure(tmp_path, monkeypatch): + store = Planfile(tmp_path).store + store.init() + archive(store) + + def invalid(text): + raise RuntimeError("invalid source") + + monkeypatch.setattr(evidence, "load_yaml_text", invalid) + before = store._read_config() + with pytest.raises(RuntimeError, match="invalid source"): + store.next_id() + assert store._read_config() == before