Commit bc0eaa5ce5689a89b279110a7cba2d8e0441892d
1 parent
b2908009
G: Work Graph node parallel execution (topological_layers + threaded run_graph)
Implements docs/todo/work-graph-parallel-execution-design.md: run_graph() now schedules graph_manifest nodes layer-by-layer (topological_layers(), Kahn's algorithm grouped by round instead of flattened), running a multi-node layer's nodes concurrently via ThreadPoolExecutor (default 4 workers) with a barrier between layers; a single-node layer stays synchronous, matching every real graph_manifest observed so far. topological_order() is now a thin flattening wrapper, unchanged behavior (regression-tested). _LockedConn/_FetchedRows makes the existing scattered conn.execute() call sites in work_node_run.py/work_graph_orchestrate.py/execution_log.py safe to call from multiple threads without touching those call sites -- note the lock must cover the full execute()-through-fetchone()/fetchall() cycle, not just execute() itself, or two threads can still collide on the shared sqlite3 connection handle. That gap was caught for real (not theoretically) by RunGraphRealSqliteConcurrencyTest during development -- committing the fixed version, not the first draft. create_worktree() (work_node_run.py) gets a module-level lock around the `git worktree add` call for concurrent layers touching the same repo. New tests: layer grouping, same-layer concurrency + inter-layer barrier, one node's failure not cancelling a concurrent sibling (WorkGraphLayerError aggregates), breakpoint-recovery resume of a concurrent layer, a quota-wait timeout coexisting with a sibling's real persisted output, real (non-mocked) sqlite under concurrent writes across several rounds, and real concurrent `git worktree add` calls. Full src (575 tests) + supervisor (125 tests) suites pass with zero regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Showing
4 changed files
with
391 additions
and
9 deletions
docs/todo/graph-engineering-open-items.md
| ... | ... | @@ -106,7 +106,7 @@ |
| 106 | 106 | |
| 107 | 107 | ## G. Work Graph 节点并行执行(Python 线程池方案) |
| 108 | 108 | |
| 109 | -**状态**:✅ 已确认,标记为核心优先级 → 需求文档见 `docs/work-graph-parallel-execution-requirements.md` | |
| 109 | +**状态**:✅ 已实现(2026-08-06)→ 需求文档见 `docs/work-graph-parallel-execution-requirements.md` | |
| 110 | 110 | |
| 111 | 111 | **来源**:14 步版对照 §3 开放问题 1 |
| 112 | 112 | |
| ... | ... | @@ -114,7 +114,7 @@ |
| 114 | 114 | |
| 115 | 115 | **确认结果(2026-08-05)**:确认要做,用户强调"这个很核心"。触发规则明确为:**只要 Work Graph 规划了 fan-out/fan-in 的并行节点,节点触发时就应该并行执行**——这跟需求文档 FR-1/FR-2 已经写的内容一致,不需要改需求文档的功能需求本身,只需要在文档里把这句话作为明确的验收口径固化下来。 |
| 116 | 116 | |
| 117 | -**待办**:✅ 详细设计已产出 → `docs/work-graph-parallel-execution-design.md`。核心设计:新增 `topological_layers()`(层内并发、层间屏障),`_LockedConn` 线程安全代理复用 `backend_server.py` 的锁模式,`create_worktree()` 加防御性锁,`WorkGraphLayerError` 聚合层内多个失败。已知遗留限制(诚实记录,非本轮解决):单节点配额等待可长达数小时,会拖住整层其他已完成节点的返回——需要更细粒度的可中断调度才能解决,超出本次范围。下一步:实施(见设计文档第 11 节的分步实施建议),或先评审设计。 | |
| 117 | +**待办**:✅ 已实现(2026-08-06),按设计文档第 11 节的分步实施顺序落地——`src/work_graph_orchestrate.py`:`topological_layers()`(层内并发、层间屏障,`topological_order()` 改为薄封装)、`_LockedConn`/`_FetchedRows`(线程安全 conn 代理,**注意**:实现比设计文档原稿更严格——锁必须覆盖 `execute()` 到 `fetchone()`/`fetchall()` 的整个周期,不能只锁 `execute()` 本身;这个差异是 `RunGraphRealSqliteConcurrencyTest` 真实跑出 `database is locked`/空 `file_path` 类错误后修的,不是理论推导)、`run_graph()` 改层级调度 + `_run_one_node()` + `WorkGraphLayerError`;`src/work_node_run.py`:`create_worktree()` 加 `_worktree_creation_lock`。新增测试:`src/test_work_graph_orchestrate.py`(`TopologicalLayersTest`、`RunGraphConcurrencyTest` 同层并发+层间屏障、`RunGraphLayerErrorTest` 失败不取消同层兄弟节点、`RunGraphConcurrentResumeTest`、`RunGraphQuotaCoexistsWithNormalCompletionTest`、`RunGraphRealSqliteConcurrencyTest` 真实 sqlite 多轮并发)、`src/test_work_node_run.py`(`ConcurrentCreateWorktreeTest` 真实 git worktree 并发创建)。已知遗留限制(设计文档已诚实记录,本轮未解决):单节点配额等待可长达数小时,会拖住整层其他已完成节点的返回——需要更细粒度的可中断调度才能解决,超出本次范围。`python3 -m unittest discover -s src`(575 tests)+ `discover -s supervisor -p "test_*.py"`(125 tests)全绿,零回归。 | |
| 118 | 118 | |
| 119 | 119 | --- |
| 120 | 120 | |
| ... | ... | @@ -128,7 +128,7 @@ |
| 128 | 128 | | D | ✅ 已确认,需求文档已产出 | `docs/work-graph-time-travel-debugging-requirements.md` | |
| 129 | 129 | | E | ✅ 已确认,需求文档已产出(范围比原开放问题更大) | `docs/dynamic-work-graph-design-skill-requirements.md` | |
| 130 | 130 | | F | ✅ 已确认,需求文档已产出 | `docs/verifier-fan-out-jury-requirements.md` | |
| 131 | -| G | ✅ 已确认,核心优先级,需求文档已产出 | `docs/work-graph-parallel-execution-requirements.md` | | |
| 131 | +| G | ✅ 已实现(2026-08-06) | `docs/work-graph-parallel-execution-requirements.md` + `docs/work-graph-parallel-execution-design.md` | | |
| 132 | 132 | |
| 133 | 133 | **当前状态**:A 已落地;B/C/D/E/F/G 六项全部完成需求文档(BRD/PRD 层面),均未进入详细设计(SDD 层面)。 |
| 134 | 134 | ... | ... |
src/test_work_graph_orchestrate.py
| ... | ... | @@ -10,6 +10,8 @@ Run: python3 -m unittest src.test_work_graph_orchestrate -v |
| 10 | 10 | import json |
| 11 | 11 | import os |
| 12 | 12 | import tempfile |
| 13 | +import threading | |
| 14 | +import time | |
| 13 | 15 | import unittest |
| 14 | 16 | from unittest.mock import patch |
| 15 | 17 | |
| ... | ... | @@ -65,6 +67,70 @@ class TopologicalOrderTest(unittest.TestCase): |
| 65 | 67 | self.assertEqual([n["node_id"] for n in wgo.topological_order(manifest)], ["a", "b"]) |
| 66 | 68 | |
| 67 | 69 | |
| 70 | +class TopologicalLayersTest(unittest.TestCase): | |
| 71 | + """G: topological_layers() -- the layer-grouped algorithm run_graph()'s | |
| 72 | + concurrent scheduler is built on. topological_order() is now a thin | |
| 73 | + flattening wrapper over this (tested above, unchanged behavior).""" | |
| 74 | + | |
| 75 | + def test_diamond_produces_three_layers(self): | |
| 76 | + manifest = { | |
| 77 | + "nodes": [ | |
| 78 | + {"node_id": "n1", "depends_on": []}, | |
| 79 | + {"node_id": "n2", "depends_on": ["n1"]}, | |
| 80 | + {"node_id": "n3", "depends_on": ["n1"]}, | |
| 81 | + {"node_id": "n4", "depends_on": ["n2", "n3"]}, | |
| 82 | + ], | |
| 83 | + "edges": [], | |
| 84 | + } | |
| 85 | + layers = wgo.topological_layers(manifest) | |
| 86 | + self.assertEqual( | |
| 87 | + [[n["node_id"] for n in layer] for layer in layers], | |
| 88 | + [["n1"], ["n2", "n3"], ["n4"]], | |
| 89 | + ) | |
| 90 | + | |
| 91 | + def test_linear_chain_is_all_single_node_layers(self): | |
| 92 | + manifest = { | |
| 93 | + "nodes": [ | |
| 94 | + {"node_id": "brd", "depends_on": []}, | |
| 95 | + {"node_id": "prd", "depends_on": ["brd"]}, | |
| 96 | + {"node_id": "sdd", "depends_on": ["prd"]}, | |
| 97 | + ], | |
| 98 | + "edges": [], | |
| 99 | + } | |
| 100 | + layers = wgo.topological_layers(manifest) | |
| 101 | + self.assertEqual( | |
| 102 | + [[n["node_id"] for n in layer] for layer in layers], | |
| 103 | + [["brd"], ["prd"], ["sdd"]], | |
| 104 | + ) | |
| 105 | + | |
| 106 | + def test_independent_nodes_are_a_single_layer(self): | |
| 107 | + manifest = {"nodes": [{"node_id": "b"}, {"node_id": "a"}], "edges": []} | |
| 108 | + layers = wgo.topological_layers(manifest) | |
| 109 | + self.assertEqual(len(layers), 1) | |
| 110 | + self.assertEqual([n["node_id"] for n in layers[0]], ["a", "b"]) | |
| 111 | + | |
| 112 | + def test_cycle_raises(self): | |
| 113 | + manifest = { | |
| 114 | + "nodes": [{"node_id": "x", "depends_on": ["y"]}, {"node_id": "y", "depends_on": ["x"]}], | |
| 115 | + "edges": [], | |
| 116 | + } | |
| 117 | + with self.assertRaises(ValueError): | |
| 118 | + wgo.topological_layers(manifest) | |
| 119 | + | |
| 120 | + def test_topological_order_matches_flattened_layers(self): | |
| 121 | + manifest = { | |
| 122 | + "nodes": [ | |
| 123 | + {"node_id": "n1", "depends_on": []}, | |
| 124 | + {"node_id": "n2", "depends_on": ["n1"]}, | |
| 125 | + {"node_id": "n3", "depends_on": ["n1"]}, | |
| 126 | + ], | |
| 127 | + "edges": [], | |
| 128 | + } | |
| 129 | + layers = wgo.topological_layers(manifest) | |
| 130 | + flat = [n["node_id"] for layer in layers for n in layer] | |
| 131 | + self.assertEqual([n["node_id"] for n in wgo.topological_order(manifest)], flat) | |
| 132 | + | |
| 133 | + | |
| 68 | 134 | class RunGraphTestBase(unittest.TestCase): |
| 69 | 135 | def setUp(self): |
| 70 | 136 | # DU-01: run_fn now really calls execution_log.append_log(), which |
| ... | ... | @@ -78,7 +144,10 @@ class RunGraphTestBase(unittest.TestCase): |
| 78 | 144 | os.environ["GRAPHLEMON_HOME"] = self.home_tmp.name |
| 79 | 145 | self.addCleanup(self._restore_graphlemon_home) |
| 80 | 146 | |
| 81 | - self.conn = schema.connect(":memory:") | |
| 147 | + # G: check_same_thread=False -- run_graph()'s concurrent layers call | |
| 148 | + # into this conn (via _LockedConn) from ThreadPoolExecutor worker | |
| 149 | + # threads, not just the thread that created the connection. | |
| 150 | + self.conn = schema.connect(":memory:", check_same_thread=False) | |
| 82 | 151 | schema.create_schema(self.conn) |
| 83 | 152 | self.conn.execute( |
| 84 | 153 | "INSERT INTO project (project_id, name, target_folder_path, has_git_repo, status) " |
| ... | ... | @@ -404,5 +473,254 @@ class RunFnLogsExecutionEventsTest(RunGraphTestBase): |
| 404 | 473 | self.assertEqual(events[1]["quota_limit_type"], "session") |
| 405 | 474 | |
| 406 | 475 | |
| 476 | +class RunGraphConcurrencyTest(RunGraphTestBase): | |
| 477 | + """G, design doc §10 acceptance criteria 1-2: same-layer nodes actually | |
| 478 | + run concurrently (overlapping wall-clock intervals), and the next layer | |
| 479 | + never starts until every node in the previous layer has finished.""" | |
| 480 | + | |
| 481 | + def test_same_layer_nodes_run_concurrently(self): | |
| 482 | + manifest = { | |
| 483 | + "nodes": [{"node_id": "a", "depends_on": []}, {"node_id": "b", "depends_on": []}], | |
| 484 | + "edges": [], | |
| 485 | + } | |
| 486 | + events = [] | |
| 487 | + events_lock = threading.Lock() | |
| 488 | + | |
| 489 | + def fake_execute(cwd, name, args, output_schema, timeout=600): | |
| 490 | + start = time.monotonic() | |
| 491 | + time.sleep(0.05) | |
| 492 | + end = time.monotonic() | |
| 493 | + with events_lock: | |
| 494 | + events.append((start, end)) | |
| 495 | + return {"content": name} | |
| 496 | + | |
| 497 | + with patch.object(work_graph_execute, "write_workflow_script"), \ | |
| 498 | + patch.object(work_graph_execute, "execute_work_graph", side_effect=fake_execute): | |
| 499 | + wgo.run_graph( | |
| 500 | + self.conn, "run-1", "planner", "planner-doc-writing", "/tmp/proj-1", manifest, | |
| 501 | + lambda node, outputs: "p-" + node["node_id"], _IN_SCHEMA, _OUT_SCHEMA, | |
| 502 | + ) | |
| 503 | + | |
| 504 | + self.assertEqual(len(events), 2) | |
| 505 | + (start_a, end_a), (start_b, end_b) = events | |
| 506 | + self.assertLess(start_a, end_b) | |
| 507 | + self.assertLess(start_b, end_a) | |
| 508 | + | |
| 509 | + def test_next_layer_waits_for_entire_previous_layer(self): | |
| 510 | + manifest = { | |
| 511 | + "nodes": [ | |
| 512 | + {"node_id": "a", "depends_on": []}, | |
| 513 | + {"node_id": "b", "depends_on": []}, | |
| 514 | + {"node_id": "c", "depends_on": ["a", "b"]}, | |
| 515 | + ], | |
| 516 | + "edges": [], | |
| 517 | + } | |
| 518 | + events = [] | |
| 519 | + events_lock = threading.Lock() | |
| 520 | + | |
| 521 | + def fake_execute(cwd, name, args, output_schema, timeout=600): | |
| 522 | + start = time.monotonic() | |
| 523 | + time.sleep(0.03) | |
| 524 | + end = time.monotonic() | |
| 525 | + with events_lock: | |
| 526 | + events.append((start, end)) | |
| 527 | + return {"content": "x"} | |
| 528 | + | |
| 529 | + with patch.object(work_graph_execute, "write_workflow_script"), \ | |
| 530 | + patch.object(work_graph_execute, "execute_work_graph", side_effect=fake_execute): | |
| 531 | + wgo.run_graph( | |
| 532 | + self.conn, "run-1", "planner", "planner-doc-writing", "/tmp/proj-1", manifest, | |
| 533 | + lambda node, outputs: "p", _IN_SCHEMA, _OUT_SCHEMA, | |
| 534 | + ) | |
| 535 | + | |
| 536 | + self.assertEqual(len(events), 3) | |
| 537 | + layer1_ends = [end for _, end in events[:2]] | |
| 538 | + layer2_start, _ = events[2] | |
| 539 | + self.assertGreaterEqual(layer2_start, max(layer1_ends)) | |
| 540 | + | |
| 541 | + | |
| 542 | +class RunGraphLayerErrorTest(RunGraphTestBase): | |
| 543 | + """G, design doc §10 acceptance criterion (FR-5): one node failing in a | |
| 544 | + concurrent layer must not cancel an already-running sibling, and all | |
| 545 | + failures in the layer are aggregated into one WorkGraphLayerError.""" | |
| 546 | + | |
| 547 | + def test_one_failing_node_does_not_cancel_sibling_and_aggregates_errors(self): | |
| 548 | + manifest = { | |
| 549 | + "nodes": [{"node_id": "a", "depends_on": []}, {"node_id": "b", "depends_on": []}], | |
| 550 | + "edges": [], | |
| 551 | + } | |
| 552 | + calls = [] | |
| 553 | + | |
| 554 | + def fake_run_work_node(conn, node_exec_id, input_payload, input_schema, output_schema, run_fn, **kwargs): | |
| 555 | + calls.append(input_payload["node_id"]) | |
| 556 | + if input_payload["node_id"] == "a": | |
| 557 | + raise RuntimeError("boom on a") | |
| 558 | + return {"content": "b done"} | |
| 559 | + | |
| 560 | + with patch.object(work_graph_execute, "write_workflow_script"), \ | |
| 561 | + patch.object(work_node_run, "run_work_node", side_effect=fake_run_work_node): | |
| 562 | + with self.assertRaises(wgo.WorkGraphLayerError) as ctx: | |
| 563 | + wgo.run_graph( | |
| 564 | + self.conn, "run-1", "planner", "planner-doc-writing", "/tmp/proj-1", manifest, | |
| 565 | + lambda node, outputs: "p", _IN_SCHEMA, _OUT_SCHEMA, | |
| 566 | + ) | |
| 567 | + | |
| 568 | + self.assertEqual(sorted(calls), ["a", "b"]) # b was actually attempted, not cancelled | |
| 569 | + error_node_ids = [node_id for node_id, exc in ctx.exception.errors] | |
| 570 | + self.assertEqual(error_node_ids, ["a"]) | |
| 571 | + | |
| 572 | + | |
| 573 | +class RunGraphConcurrentResumeTest(RunGraphTestBase): | |
| 574 | + """G, design doc §10 acceptance criterion 3: breakpoint-recovery still | |
| 575 | + works when the interrupted run had a concurrent (2+ node) layer -- a | |
| 576 | + second run_graph() call for the same run_id must not re-run nodes the | |
| 577 | + first call already finished.""" | |
| 578 | + | |
| 579 | + def test_concurrent_layer_resume_reruns_nothing(self): | |
| 580 | + manifest = { | |
| 581 | + "nodes": [{"node_id": "a", "depends_on": []}, {"node_id": "b", "depends_on": []}], | |
| 582 | + "edges": [], | |
| 583 | + } | |
| 584 | + calls = [] | |
| 585 | + | |
| 586 | + def fake_execute(cwd, name, args, output_schema, timeout=600): | |
| 587 | + calls.append(name) | |
| 588 | + return {"content": "c-" + name} | |
| 589 | + | |
| 590 | + with patch.object(work_graph_execute, "write_workflow_script"), \ | |
| 591 | + patch.object(work_graph_execute, "execute_work_graph", side_effect=fake_execute): | |
| 592 | + wgo.run_graph( | |
| 593 | + self.conn, "run-1", "planner", "planner-doc-writing", "/tmp/proj-1", manifest, | |
| 594 | + lambda node, outputs: "p", _IN_SCHEMA, _OUT_SCHEMA, | |
| 595 | + ) | |
| 596 | + self.assertEqual(len(calls), 2) | |
| 597 | + | |
| 598 | + with patch.object(work_graph_execute, "write_workflow_script"), \ | |
| 599 | + patch.object(work_graph_execute, "execute_work_graph") as mock_execute2: | |
| 600 | + mock_execute2.side_effect = AssertionError("should not be called -- both nodes already done") | |
| 601 | + outputs = wgo.run_graph( | |
| 602 | + self.conn, "run-1", "planner", "planner-doc-writing", "/tmp/proj-1", manifest, | |
| 603 | + lambda node, outputs: "p", _IN_SCHEMA, _OUT_SCHEMA, | |
| 604 | + ) | |
| 605 | + self.assertEqual(set(outputs.keys()), {"a", "b"}) | |
| 606 | + | |
| 607 | + | |
| 608 | +class RunGraphQuotaCoexistsWithNormalCompletionTest(RunGraphTestBase): | |
| 609 | + """G, design doc §10 acceptance criterion 6: a node stuck on a quota-wait | |
| 610 | + timeout in a concurrent layer must not lose its sibling's real, already- | |
| 611 | + persisted output -- the sibling's row is 'done' with its real payload | |
| 612 | + even though the overall layer raises.""" | |
| 613 | + | |
| 614 | + def test_quota_timeout_on_one_node_does_not_erase_siblings_persisted_output(self): | |
| 615 | + manifest = { | |
| 616 | + "nodes": [{"node_id": "a", "depends_on": []}, {"node_id": "b", "depends_on": []}], | |
| 617 | + "edges": [], | |
| 618 | + } | |
| 619 | + prompt_by_script = {} | |
| 620 | + prompt_lock = threading.Lock() | |
| 621 | + | |
| 622 | + def fake_write_script(cwd, script_name, script_content): | |
| 623 | + with prompt_lock: | |
| 624 | + prompt_by_script[script_name] = script_content | |
| 625 | + | |
| 626 | + def fake_execute(cwd, name, args, output_schema, timeout=600): | |
| 627 | + content = prompt_by_script[name] | |
| 628 | + if '"p-a"' in content: | |
| 629 | + raise work_graph_execute.UsageLimitReachedError("hit your session limit", "session") | |
| 630 | + return {"content": "b done"} | |
| 631 | + | |
| 632 | + now_calls = {"n": 0} | |
| 633 | + now_lock = threading.Lock() | |
| 634 | + | |
| 635 | + def fake_now(): | |
| 636 | + # first call records quota_wait_started_at as "0"; second call | |
| 637 | + # (computing elapsed) reports far past the session timeout, so | |
| 638 | + # this times out immediately instead of really sleeping. | |
| 639 | + with now_lock: | |
| 640 | + now_calls["n"] += 1 | |
| 641 | + return 0.0 if now_calls["n"] == 1 else 999999.0 | |
| 642 | + | |
| 643 | + with patch.object(work_graph_execute, "write_workflow_script", side_effect=fake_write_script), \ | |
| 644 | + patch.object(work_graph_execute, "execute_work_graph", side_effect=fake_execute): | |
| 645 | + with self.assertRaises(wgo.WorkGraphLayerError) as ctx: | |
| 646 | + wgo.run_graph( | |
| 647 | + self.conn, "run-1", "planner", "planner-doc-writing", "/tmp/proj-1", manifest, | |
| 648 | + lambda node, outputs: "p-" + node["node_id"], _IN_SCHEMA, _OUT_SCHEMA, | |
| 649 | + sleep=lambda s: None, now=fake_now, | |
| 650 | + ) | |
| 651 | + | |
| 652 | + error_node_ids = [node_id for node_id, exc in ctx.exception.errors] | |
| 653 | + self.assertEqual(error_node_ids, ["a"]) | |
| 654 | + self.assertIsInstance(dict(ctx.exception.errors)["a"], work_node_run.UsageLimitWaitTimeoutError) | |
| 655 | + | |
| 656 | + b_row = self.conn.execute( | |
| 657 | + "SELECT status, output_payload FROM work_node_execution WHERE run_id = 'run-1' AND node_id = 'b'" | |
| 658 | + ).fetchone() | |
| 659 | + self.assertEqual(b_row[0], "done") | |
| 660 | + self.assertEqual(json.loads(b_row[1]), {"content": "b done"}) | |
| 661 | + | |
| 662 | + | |
| 663 | +class RunGraphRealSqliteConcurrencyTest(unittest.TestCase): | |
| 664 | + """G, design doc §10 acceptance criterion 4: a real (non-:memory:, | |
| 665 | + non-mocked-conn) sqlite database under genuine concurrent write | |
| 666 | + pressure from a multi-node layer must never raise 'database is | |
| 667 | + locked' -- run several rounds since races like this often only | |
| 668 | + reproduce probabilistically.""" | |
| 669 | + | |
| 670 | + def setUp(self): | |
| 671 | + self.home_tmp = tempfile.TemporaryDirectory() | |
| 672 | + self.addCleanup(self.home_tmp.cleanup) | |
| 673 | + self.old_graphlemon_home = os.environ.get("GRAPHLEMON_HOME") | |
| 674 | + os.environ["GRAPHLEMON_HOME"] = self.home_tmp.name | |
| 675 | + self.addCleanup(self._restore_graphlemon_home) | |
| 676 | + self.db_tmp = tempfile.TemporaryDirectory() | |
| 677 | + self.addCleanup(self.db_tmp.cleanup) | |
| 678 | + | |
| 679 | + def _restore_graphlemon_home(self): | |
| 680 | + if self.old_graphlemon_home is None: | |
| 681 | + os.environ.pop("GRAPHLEMON_HOME", None) | |
| 682 | + else: | |
| 683 | + os.environ["GRAPHLEMON_HOME"] = self.old_graphlemon_home | |
| 684 | + | |
| 685 | + def test_multiple_rounds_of_concurrent_layer_writes_never_raise_locked(self): | |
| 686 | + manifest = { | |
| 687 | + "nodes": [{"node_id": n, "depends_on": []} for n in ("a", "b", "c", "d")], | |
| 688 | + "edges": [], | |
| 689 | + } | |
| 690 | + | |
| 691 | + def fake_execute(cwd, name, args, output_schema, timeout=600): | |
| 692 | + time.sleep(0.005) | |
| 693 | + return {"content": name} | |
| 694 | + | |
| 695 | + for round_n in range(5): | |
| 696 | + db_path = os.path.join(self.db_tmp.name, "test_{}.db".format(round_n)) | |
| 697 | + conn = schema.connect(db_path, check_same_thread=False) | |
| 698 | + schema.create_schema(conn) | |
| 699 | + conn.execute( | |
| 700 | + "INSERT INTO project (project_id, name, target_folder_path, has_git_repo, status) " | |
| 701 | + "VALUES ('proj-1', 'P', '/tmp/proj-1', 1, 'active')" | |
| 702 | + ) | |
| 703 | + conn.execute( | |
| 704 | + "INSERT INTO task (task_id, project_id, goal_text, task_type, status, assigned_org_node) " | |
| 705 | + "VALUES ('task-1', 'proj-1', 'goal', 'goal', 'coding', 'planner')" | |
| 706 | + ) | |
| 707 | + conn.execute( | |
| 708 | + "INSERT INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status) " | |
| 709 | + "VALUES ('run-1', 'task-1', 'planner', '{}', 'running')" | |
| 710 | + ) | |
| 711 | + conn.commit() | |
| 712 | + | |
| 713 | + try: | |
| 714 | + with patch.object(work_graph_execute, "write_workflow_script"), \ | |
| 715 | + patch.object(work_graph_execute, "execute_work_graph", side_effect=fake_execute): | |
| 716 | + outputs = wgo.run_graph( | |
| 717 | + conn, "run-1", "planner", "planner-doc-writing", "/tmp/proj-1", manifest, | |
| 718 | + lambda node, outputs: "p", _IN_SCHEMA, _OUT_SCHEMA, | |
| 719 | + ) | |
| 720 | + self.assertEqual(set(outputs.keys()), {"a", "b", "c", "d"}) | |
| 721 | + finally: | |
| 722 | + conn.close() | |
| 723 | + | |
| 724 | + | |
| 407 | 725 | if __name__ == "__main__": |
| 408 | 726 | unittest.main() | ... | ... |
src/test_work_node_run.py
| ... | ... | @@ -7,6 +7,7 @@ so it's tested for real here, not mocked. |
| 7 | 7 | Run: python3 -m unittest src.test_work_node_run -v |
| 8 | 8 | """ |
| 9 | 9 | |
| 10 | +import concurrent.futures | |
| 10 | 11 | import json |
| 11 | 12 | import os |
| 12 | 13 | import subprocess |
| ... | ... | @@ -618,5 +619,36 @@ class ResumeWorkGraphRunTest(WorkNodeRunTestBase): |
| 618 | 619 | work_node_run.resume_work_graph_run(self.conn, "run-1") |
| 619 | 620 | |
| 620 | 621 | |
| 622 | +class ConcurrentCreateWorktreeTest(unittest.TestCase): | |
| 623 | + """G, design doc §10 acceptance criterion 5: `create_worktree()` calls | |
| 624 | + from multiple threads against the SAME repo at the same time must not | |
| 625 | + race on git's shared `.git/worktrees/` metadata -- real git, real | |
| 626 | + threads, no mocking.""" | |
| 627 | + | |
| 628 | + def setUp(self): | |
| 629 | + self.tmp = tempfile.TemporaryDirectory() | |
| 630 | + self.addCleanup(self.tmp.cleanup) | |
| 631 | + _init_git_repo(self.tmp.name) | |
| 632 | + | |
| 633 | + def test_concurrent_create_worktree_calls_all_succeed_with_unique_paths(self): | |
| 634 | + node_exec_ids = ["node-{}".format(i) for i in range(8)] | |
| 635 | + with concurrent.futures.ThreadPoolExecutor(max_workers=8) as pool: | |
| 636 | + paths = list(pool.map( | |
| 637 | + lambda node_exec_id: work_node_run.create_worktree(self.tmp.name, node_exec_id), | |
| 638 | + node_exec_ids, | |
| 639 | + )) | |
| 640 | + | |
| 641 | + self.assertEqual(len(paths), len(set(paths)), "every worktree path must be unique") | |
| 642 | + for path in paths: | |
| 643 | + self.assertTrue(os.path.isdir(path)) | |
| 644 | + | |
| 645 | + listing = subprocess.run( | |
| 646 | + ["git", "worktree", "list", "--porcelain"], cwd=self.tmp.name, check=True, capture_output=True, text=True, | |
| 647 | + ).stdout | |
| 648 | + worktree_lines = [line for line in listing.splitlines() if line.startswith("worktree ")] | |
| 649 | + # +1 for the main checkout itself, which `git worktree list` always includes. | |
| 650 | + self.assertEqual(len(worktree_lines), len(node_exec_ids) + 1) | |
| 651 | + | |
| 652 | + | |
| 621 | 653 | if __name__ == "__main__": |
| 622 | 654 | unittest.main() | ... | ... |
src/work_graph_orchestrate.py
| ... | ... | @@ -97,10 +97,42 @@ class WorkGraphLayerError(Exception): |
| 97 | 97 | super().__init__("{} node(s) failed in one layer: {}".format(len(errors), summary)) |
| 98 | 98 | |
| 99 | 99 | |
| 100 | +class _FetchedRows: | |
| 101 | + """A pre-materialized stand-in for a sqlite3.Cursor, holding rows | |
| 102 | + already fetched while _LockedConn's lock was held (see _LockedConn's | |
| 103 | + own docstring for why this matters -- a real sqlite3.Cursor's | |
| 104 | + fetchone()/fetchall() touch the shared connection handle just as much | |
| 105 | + as execute() does, so returning a live Cursor and letting the caller | |
| 106 | + fetch from it AFTER the lock is released reopens the exact race the | |
| 107 | + lock exists to close).""" | |
| 108 | + | |
| 109 | + def __init__(self, rows): | |
| 110 | + self._rows = rows | |
| 111 | + self._index = 0 | |
| 112 | + | |
| 113 | + def fetchone(self): | |
| 114 | + if self._index >= len(self._rows): | |
| 115 | + return None | |
| 116 | + row = self._rows[self._index] | |
| 117 | + self._index += 1 | |
| 118 | + return row | |
| 119 | + | |
| 120 | + def fetchall(self): | |
| 121 | + rows = self._rows[self._index:] | |
| 122 | + self._index = len(self._rows) | |
| 123 | + return rows | |
| 124 | + | |
| 125 | + | |
| 100 | 126 | class _LockedConn: |
| 101 | - """sqlite3.Connection-compatible thread-safe proxy: execute/executemany/ | |
| 102 | - commit each take a shared lock for the duration of the call. SQLite only | |
| 103 | - ever allows one writer at a time regardless -- this doesn't create new | |
| 127 | + """sqlite3.Connection-compatible thread-safe proxy: execute/executemany | |
| 128 | + each take a shared lock for the FULL statement-execution-plus-fetch | |
| 129 | + cycle (not just the `execute()` call itself -- a sqlite3.Cursor's | |
| 130 | + fetchone()/fetchall() also touch the shared connection handle, so a | |
| 131 | + lock that released before the caller's `.fetchone()` runs would still | |
| 132 | + let two threads collide on the same connection; this was caught for | |
| 133 | + real by RunGraphRealSqliteConcurrencyTest during development, not | |
| 134 | + theoretical), commit() takes it for the commit call. SQLite only ever | |
| 135 | + allows one writer at a time regardless -- this doesn't create new | |
| 104 | 136 | concurrency, it just makes the existing many-scattered-call-sites code |
| 105 | 137 | (`work_node_run.py`/`work_graph_orchestrate.py`/`execution_log.py`) safe |
| 106 | 138 | to call from multiple threads without rewriting every call site, since |
| ... | ... | @@ -117,11 +149,11 @@ class _LockedConn: |
| 117 | 149 | |
| 118 | 150 | def execute(self, *args, **kwargs): |
| 119 | 151 | with self._lock: |
| 120 | - return self._conn.execute(*args, **kwargs) | |
| 152 | + return _FetchedRows(self._conn.execute(*args, **kwargs).fetchall()) | |
| 121 | 153 | |
| 122 | 154 | def executemany(self, *args, **kwargs): |
| 123 | 155 | with self._lock: |
| 124 | - return self._conn.executemany(*args, **kwargs) | |
| 156 | + return _FetchedRows(self._conn.executemany(*args, **kwargs).fetchall()) | |
| 125 | 157 | |
| 126 | 158 | def commit(self): |
| 127 | 159 | with self._lock: | ... | ... |