Make discussion state mutations concurrency-safe

src/discussion_framework/commands.py

@@ -6,7 +6,7 @@ import json
 import re
 import shutil
 
-from .model import ALL_ARTIFACT_KINDS, DiscussionError, DiscussionIndex, WORKFLOW_KINDS, artifact_path
+from .model import ALL_ARTIFACT_KINDS, DiscussionError, DiscussionIndex, WORKFLOW_KINDS, artifact_path, index_lock
 from .validation import validate, validate_or_raise
 
 
@@ -24,6 +24,7 @@ def init(root: Path) -> str:
     ):
         (discussion_dir / relative).mkdir(parents=True, exist_ok=True)
 
+    with index_lock(root):
         index_path = discussion_dir / "index.ndjson"
         if index_path.exists():
             return f"already initialized: {index_path}"
@@ -47,6 +48,7 @@ def start_agenda(
     if not title.strip():
         raise DiscussionError("title is required")
 
+    with index_lock(root):
         index = DiscussionIndex.load(root)
         normalized_ticket = _slugify(ticket)
         if not normalized_ticket:
@@ -107,6 +109,7 @@ def start_agenda(
 
 
 def accept_agenda(root: Path, discussion_id: str, agenda_id: str | None) -> dict:
+    with index_lock(root):
         index = DiscussionIndex.load(root)
         discussion = index.find_discussion(discussion_id)
         agenda = _select_artifact(discussion, "agendas", agenda_id)
@@ -125,6 +128,7 @@ def accept_agenda(root: Path, discussion_id: str, agenda_id: str | None) -> dict
 def add_decision(root: Path, discussion_id: str, agenda_id: str | None, title: str) -> dict:
     if not title.strip():
         raise DiscussionError("title is required")
+    with index_lock(root):
         index = DiscussionIndex.load(root)
         discussion = index.find_discussion(discussion_id)
         agenda = _select_artifact(discussion, "agendas", agenda_id)
@@ -165,6 +169,7 @@ def add_decision(root: Path, discussion_id: str, agenda_id: str | None, title: s
 
 
 def accept_decision(root: Path, discussion_id: str, decision_id: str | None) -> dict:
+    with index_lock(root):
         index = DiscussionIndex.load(root)
         discussion = index.find_discussion(discussion_id)
         decision = _select_artifact(discussion, "decisions", decision_id)
@@ -183,6 +188,7 @@ def accept_decision(root: Path, discussion_id: str, decision_id: str | None) ->
 def add_plan(root: Path, discussion_id: str, decision_id: str | None, title: str) -> dict:
     if not title.strip():
         raise DiscussionError("title is required")
+    with index_lock(root):
         index = DiscussionIndex.load(root)
         discussion = index.find_discussion(discussion_id)
         decision = _select_artifact(discussion, "decisions", decision_id)
@@ -225,6 +231,7 @@ def add_plan(root: Path, discussion_id: str, decision_id: str | None, title: str
 def mark_plan(root: Path, plan_id: str, status: str) -> dict:
     if status not in {"open", "review", "accepted", "in_progress", "done", "abandoned"}:
         raise DiscussionError(f"unsupported plan status: {status}")
+    with index_lock(root):
         index = DiscussionIndex.load(root)
         discussion, plan = _find_artifact(index, "plans", plan_id)
         current = plan.get("status")
@@ -424,6 +431,7 @@ def housekeep(root: Path, discussion_id: str, lesson: str | None, no_lesson: boo
     if bool(lesson) == bool(no_lesson):
         raise DiscussionError("choose exactly one of --lesson or --no-lesson")
 
+    with index_lock(root):
         index = DiscussionIndex.load(root)
         discussion = index.find_discussion(discussion_id)
 

src/discussion_framework/model.py

@@ -1,8 +1,11 @@
 from __future__ import annotations
 
+from contextlib import contextmanager
 from dataclasses import dataclass
 from pathlib import Path
+import fcntl
 import json
+import os
 
 
 COUNTERS = ("DSC", "AGD", "DEC", "PLN", "LSN", "CLSN")
@@ -14,6 +17,19 @@ class DiscussionError(RuntimeError):
     pass
 
 
+@contextmanager
+def index_lock(root: Path):
+    discussion_dir = root / "discussion"
+    discussion_dir.mkdir(parents=True, exist_ok=True)
+    lock_path = discussion_dir / ".index.lock"
+    with lock_path.open("a+", encoding="utf-8") as lock_file:
+        fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX)
+        try:
+            yield
+        finally:
+            fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN)
+
+
 @dataclass
 class DiscussionIndex:
     root: Path
@@ -55,7 +71,13 @@ class DiscussionIndex:
     def save(self) -> None:
         records = [self.meta, *self.discussions]
         text = "\n".join(json.dumps(record, separators=(",", ":"), ensure_ascii=False) for record in records)
-        self.path.write_text(text + "\n", encoding="utf-8")
+        tmp_path = self.path.with_name(f".{self.path.name}.{os.getpid()}.tmp")
+        try:
+            tmp_path.write_text(text + "\n", encoding="utf-8")
+            os.replace(tmp_path, self.path)
+        finally:
+            if tmp_path.exists():
+                tmp_path.unlink()
 
     def find_discussion(self, discussion_id: str) -> dict:
         for discussion in self.discussions:

tests/test_cli.py

@@ -3,7 +3,10 @@ from __future__ import annotations
 import contextlib
 import io
 import json
+import os
 from pathlib import Path
+import subprocess
+import sys
 import unittest
 
 from discussion_framework.cli import main
@@ -145,6 +148,52 @@ class CliTest(unittest.TestCase):
             self.assertIn("status: accepted", decision.read_text(encoding="utf-8"))
             self.assertIn("status: done", plan.read_text(encoding="utf-8"))
 
+    def test_parallel_add_plan_allocates_unique_ids(self) -> None:
+        with TempDir() as root:
+            main(["--root", str(root), "init"])
+            self.assertEqual(main(["--root", str(root), "start", "parallel-demo", "Parallel Demo"]), 0)
+            self.assertEqual(main(["--root", str(root), "accept-agenda", "DSC-0001"]), 0)
+            self.assertEqual(main(["--root", str(root), "add-decision", "DSC-0001", "--title", "Parallel Decision"]), 0)
+            self.assertEqual(main(["--root", str(root), "accept-decision", "DSC-0001"]), 0)
+
+            script = "from discussion_framework.cli import main; import sys; raise SystemExit(main(sys.argv[1:]))"
+            env = os.environ.copy()
+            src = str(Path(__file__).resolve().parents[1] / "src")
+            env["PYTHONPATH"] = src + os.pathsep + env["PYTHONPATH"] if env.get("PYTHONPATH") else src
+            processes = [
+                subprocess.Popen(
+                    [
+                        sys.executable,
+                        "-c",
+                        script,
+                        "--root",
+                        str(root),
+                        "add-plan",
+                        "DSC-0001",
+                        "--title",
+                        f"Parallel Plan {number}",
+                    ],
+                    env=env,
+                    stdout=subprocess.PIPE,
+                    stderr=subprocess.PIPE,
+                    text=True,
+                )
+                for number in range(1, 4)
+            ]
+
+            results = [process.communicate(timeout=10) + (process.returncode,) for process in processes]
+            self.assertEqual([returncode for *_streams, returncode in results], [0, 0, 0], results)
+
+            discussion = root / "discussion"
+            records = [json.loads(line) for line in (discussion / "index.ndjson").read_text(encoding="utf-8").splitlines()]
+            meta = records[0]
+            plans = records[1]["plans"]
+
+            self.assertEqual(meta["next_id"]["PLN"], 4)
+            self.assertEqual(sorted(plan["id"] for plan in plans), ["PLN-0001", "PLN-0002", "PLN-0003"])
+            for plan in plans:
+                self.assertTrue((discussion / "workflow" / "plans" / plan["file"]).exists())
+
     def test_housekeep_with_lesson_removes_workflow_artifacts(self) -> None:
         with TempDir() as root:
             main(["--root", str(root), "init"])