Commit b2908009610cfeb42c7b7d57f8ca8d349d78d878
1 parent
b39f4114
graphlemon: auto-commit uncommitted changes for 0123a9cc-01da-4ce6-9622-a210731a…
…de13-main-checkout-drift
Showing
2 changed files
with
191 additions
and
54 deletions
src/work_graph_orchestrate.py
| @@ -64,7 +64,9 @@ exception, re-raised unchanged after logging), `quota_limit` (the | @@ -64,7 +64,9 @@ exception, re-raised unchanged after logging), `quota_limit` (the | ||
| 64 | actually happened (N attempts), not a bug. | 64 | actually happened (N attempts), not a bug. |
| 65 | """ | 65 | """ |
| 66 | 66 | ||
| 67 | +import concurrent.futures | ||
| 67 | import json | 68 | import json |
| 69 | +import threading | ||
| 68 | import time | 70 | import time |
| 69 | import uuid | 71 | import uuid |
| 70 | 72 | ||
| @@ -83,9 +85,61 @@ class ConsistencyCheckFailedError(Exception): | @@ -83,9 +85,61 @@ class ConsistencyCheckFailedError(Exception): | ||
| 83 | 'blocked' by the time this is raised.""" | 85 | 'blocked' by the time this is raised.""" |
| 84 | 86 | ||
| 85 | 87 | ||
| 86 | -def topological_order(graph_manifest): | ||
| 87 | - """Kahn's algorithm. Dependencies are read from BOTH `node['depends_on']` | ||
| 88 | - and `graph_manifest['edges']` and merged -- `plan_work_graph()`'s own | 88 | +class WorkGraphLayerError(Exception): |
| 89 | + """One or more nodes in a concurrently-executed layer failed -- | ||
| 90 | + `errors` is the full [(node_id, exception), ...] list (not just the | ||
| 91 | + first failure), so a caller can tell whether every failure was the | ||
| 92 | + same recoverable kind or something more serious.""" | ||
| 93 | + | ||
| 94 | + def __init__(self, errors): | ||
| 95 | + self.errors = errors | ||
| 96 | + summary = "; ".join("{}: {}".format(node_id, e) for node_id, e in errors) | ||
| 97 | + super().__init__("{} node(s) failed in one layer: {}".format(len(errors), summary)) | ||
| 98 | + | ||
| 99 | + | ||
| 100 | +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 | ||
| 104 | + concurrency, it just makes the existing many-scattered-call-sites code | ||
| 105 | + (`work_node_run.py`/`work_graph_orchestrate.py`/`execution_log.py`) safe | ||
| 106 | + to call from multiple threads without rewriting every call site, since | ||
| 107 | + the lock is held only per-statement, not around the (slow) `claude -p` | ||
| 108 | + call itself. Mirrors `backend_server.py`'s existing shared-connection + | ||
| 109 | + `threading.Lock()` pattern, generalized to per-statement granularity. | ||
| 110 | + Attribute access not explicitly wrapped here (cursor()/rollback()/ | ||
| 111 | + close()) passes through unlocked -- nothing on the concurrent path in | ||
| 112 | + this module uses them.""" | ||
| 113 | + | ||
| 114 | + def __init__(self, conn, lock=None): | ||
| 115 | + self._conn = conn | ||
| 116 | + self._lock = lock or threading.Lock() | ||
| 117 | + | ||
| 118 | + def execute(self, *args, **kwargs): | ||
| 119 | + with self._lock: | ||
| 120 | + return self._conn.execute(*args, **kwargs) | ||
| 121 | + | ||
| 122 | + def executemany(self, *args, **kwargs): | ||
| 123 | + with self._lock: | ||
| 124 | + return self._conn.executemany(*args, **kwargs) | ||
| 125 | + | ||
| 126 | + def commit(self): | ||
| 127 | + with self._lock: | ||
| 128 | + return self._conn.commit() | ||
| 129 | + | ||
| 130 | + def __getattr__(self, name): | ||
| 131 | + return getattr(self._conn, name) | ||
| 132 | + | ||
| 133 | + | ||
| 134 | +def topological_layers(graph_manifest): | ||
| 135 | + """Kahn's algorithm, layered: returns List[List[node]], where each inner | ||
| 136 | + list is one round's batch of in-degree-zero nodes -- nodes within a | ||
| 137 | + layer are, by construction, mutually independent (neither depends on | ||
| 138 | + the other), so they're safe to run concurrently. Layers are ordered | ||
| 139 | + (layer N+1 only becomes ready once layer N is fully accounted for); | ||
| 140 | + each layer's nodes are sorted by node_id for deterministic test | ||
| 141 | + assertions. Dependencies are read from BOTH `node['depends_on']` and | ||
| 142 | + `graph_manifest['edges']` and merged -- `plan_work_graph()`'s own | ||
| 89 | output isn't schema-validated (it can't be, `--json-schema` doesn't | 143 | output isn't schema-validated (it can't be, `--json-schema` doesn't |
| 90 | work with `--agent`, see work_graph_plan.py), so a real LLM response | 144 | work with `--agent`, see work_graph_plan.py), so a real LLM response |
| 91 | might populate one and not the other; trusting only one would silently | 145 | might populate one and not the other; trusting only one would silently |
| @@ -106,21 +160,30 @@ def topological_order(graph_manifest): | @@ -106,21 +160,30 @@ def topological_order(graph_manifest): | ||
| 106 | if dep in dependents: | 160 | if dep in dependents: |
| 107 | dependents[dep].append(node_id) | 161 | dependents[dep].append(node_id) |
| 108 | 162 | ||
| 109 | - ready = sorted(node_id for node_id, degree in in_degree.items() if degree == 0) | ||
| 110 | - ordered = [] | ||
| 111 | - while ready: | ||
| 112 | - node_id = ready.pop(0) | ||
| 113 | - ordered.append(nodes[node_id]) | ||
| 114 | - for dependent in sorted(dependents[node_id]): | ||
| 115 | - in_degree[dependent] -= 1 | ||
| 116 | - if in_degree[dependent] == 0: | ||
| 117 | - ready.append(dependent) | ||
| 118 | - ready.sort() | ||
| 119 | - if len(ordered) != len(nodes): | 163 | + layers = [] |
| 164 | + current = sorted(node_id for node_id, degree in in_degree.items() if degree == 0) | ||
| 165 | + seen_count = 0 | ||
| 166 | + while current: | ||
| 167 | + layers.append([nodes[node_id] for node_id in current]) | ||
| 168 | + seen_count += len(current) | ||
| 169 | + next_layer = set() | ||
| 170 | + for node_id in current: | ||
| 171 | + for dependent in dependents[node_id]: | ||
| 172 | + in_degree[dependent] -= 1 | ||
| 173 | + if in_degree[dependent] == 0: | ||
| 174 | + next_layer.add(dependent) | ||
| 175 | + current = sorted(next_layer) | ||
| 176 | + if seen_count != len(nodes): | ||
| 120 | raise ValueError( | 177 | raise ValueError( |
| 121 | "graph_manifest has a dependency cycle or a depends_on/edge references an unknown node_id" | 178 | "graph_manifest has a dependency cycle or a depends_on/edge references an unknown node_id" |
| 122 | ) | 179 | ) |
| 123 | - return ordered | 180 | + return layers |
| 181 | + | ||
| 182 | + | ||
| 183 | +def topological_order(graph_manifest): | ||
| 184 | + """Flat view of topological_layers() -- kept for callers (e.g. | ||
| 185 | + e2e_pipeline.py) that only need a total order, not the layer grouping.""" | ||
| 186 | + return [node for layer in topological_layers(graph_manifest) for node in layer] | ||
| 124 | 187 | ||
| 125 | 188 | ||
| 126 | def ensure_placeholder_skill(conn, skill_id, owner_org_node): | 189 | def ensure_placeholder_skill(conn, skill_id, owner_org_node): |
| @@ -193,25 +256,37 @@ def _make_run_fn(conn, target_folder_path, node_exec_id, org_node, prompt, outpu | @@ -193,25 +256,37 @@ def _make_run_fn(conn, target_folder_path, node_exec_id, org_node, prompt, outpu | ||
| 193 | return run_fn | 256 | return run_fn |
| 194 | 257 | ||
| 195 | 258 | ||
| 259 | +_DEFAULT_MAX_WORKERS = 4 # see this module's own docstring section on G for rationale | ||
| 260 | + | ||
| 261 | + | ||
| 196 | def run_graph( | 262 | def run_graph( |
| 197 | conn, run_id, org_node, skill_id, target_folder_path, graph_manifest, | 263 | conn, run_id, org_node, skill_id, target_folder_path, graph_manifest, |
| 198 | build_prompt, input_schema, output_schema, timeout=600, needs_worktree=False, | 264 | build_prompt, input_schema, output_schema, timeout=600, needs_worktree=False, |
| 199 | - verify_fn=None, sleep=time.sleep, now=time.time, | 265 | + verify_fn=None, sleep=time.sleep, now=time.time, max_workers=_DEFAULT_MAX_WORKERS, |
| 200 | ): | 266 | ): |
| 201 | - """Runs every node in `graph_manifest` (topologically ordered) through | ||
| 202 | - `run_work_node()`, each backed by a real one-node dynamic-workflow | ||
| 203 | - `agent()` call using `org_node`'s migrated persona. | 267 | + """Runs every node in `graph_manifest` through `run_work_node()`, each |
| 268 | + backed by a real one-node dynamic-workflow `agent()` call using | ||
| 269 | + `org_node`'s migrated persona. Nodes are grouped into topological | ||
| 270 | + layers (`topological_layers()`) and run layer-by-layer with a barrier | ||
| 271 | + in between; within a layer -- nodes that are mutually independent by | ||
| 272 | + construction -- a layer with 2+ nodes runs them concurrently via a | ||
| 273 | + `ThreadPoolExecutor` (bounded by `max_workers`), a layer with exactly 1 | ||
| 274 | + node runs it synchronously (no thread-pool overhead for the common | ||
| 275 | + case: every real graph_manifest observed so far is a single-node-per- | ||
| 276 | + layer chain). | ||
| 204 | 277 | ||
| 205 | `build_prompt(node, outputs)` is called right before running that | 278 | `build_prompt(node, outputs)` is called right before running that |
| 206 | - node -- `outputs` contains every already-completed node's | ||
| 207 | - output_payload so far (topological order guarantees a node's | ||
| 208 | - dependencies are always already present), so prompts needing upstream | ||
| 209 | - content (e.g. "here is the approved BRD...") can be built just-in-time | ||
| 210 | - instead of requiring the whole graph's prompts precomputed before | ||
| 211 | - anything runs. This also makes resume-after-restart work correctly: | ||
| 212 | - `run_work_node` short-circuits already-`done` nodes and returns their | ||
| 213 | - persisted output, which lands in `outputs` the same as a freshly-run | ||
| 214 | - one would. | 279 | + node -- `outputs` contains every already-completed EARLIER layer's |
| 280 | + output_payload (a layer never starts until every node in the previous | ||
| 281 | + layer has finished, so this is always safe to read without locking), | ||
| 282 | + so prompts needing upstream content (e.g. "here is the approved | ||
| 283 | + BRD...") can be built just-in-time instead of requiring the whole | ||
| 284 | + graph's prompts precomputed before anything runs. `build_prompt` never | ||
| 285 | + sees a same-layer sibling's output -- by definition, same-layer nodes | ||
| 286 | + don't depend on each other. This also makes resume-after-restart work | ||
| 287 | + correctly: `run_work_node` short-circuits already-`done` nodes and | ||
| 288 | + returns their persisted output, which lands in `outputs` the same as a | ||
| 289 | + freshly-run one would. | ||
| 215 | 290 | ||
| 216 | `needs_worktree=True` (03-C §3.6: "并行写代码的节点必须先申请 | 291 | `needs_worktree=True` (03-C §3.6: "并行写代码的节点必须先申请 |
| 217 | git_worktree_ref") isolates each node's actual file changes in its own | 292 | git_worktree_ref") isolates each node's actual file changes in its own |
| @@ -239,37 +314,89 @@ def run_graph( | @@ -239,37 +314,89 @@ def run_graph( | ||
| 239 | passes: `work_graph_run.status` -> 'verifying' and the regenerated | 314 | passes: `work_graph_run.status` -> 'verifying' and the regenerated |
| 240 | output replaces the original in `outputs`. | 315 | output replaces the original in `outputs`. |
| 241 | 316 | ||
| 317 | + **G (2026-08-06, Work Graph parallel execution)**: a multi-node layer's | ||
| 318 | + failures are collected from every node in that layer (a sibling's | ||
| 319 | + failure never cancels an already-running sibling -- they're | ||
| 320 | + independent work, no reason to throw away completed work), then raised | ||
| 321 | + together as `WorkGraphLayerError` once the whole layer has settled. | ||
| 322 | + One real limitation, accepted deliberately (see | ||
| 323 | + `docs/todo/work-graph-parallel-execution-design.md` §6): a node | ||
| 324 | + waiting on a quota-backoff sleep inside `run_work_node` blocks the | ||
| 325 | + thread pool worker running it, which in turn blocks the layer barrier | ||
| 326 | + even after every sibling has finished -- Python threads have no clean | ||
| 327 | + preemptible cancel, and building one is out of scope for this pass. | ||
| 328 | + | ||
| 242 | Returns {node_id: output_payload}. | 329 | Returns {node_id: output_payload}. |
| 243 | """ | 330 | """ |
| 244 | ensure_placeholder_skill(conn, skill_id, org_node) | 331 | ensure_placeholder_skill(conn, skill_id, org_node) |
| 245 | effective_needs_worktree = needs_worktree or paths.is_self_target(target_folder_path) | 332 | effective_needs_worktree = needs_worktree or paths.is_self_target(target_folder_path) |
| 246 | - ordered = topological_order(graph_manifest) | 333 | + layers = topological_layers(graph_manifest) |
| 247 | outputs = {} | 334 | outputs = {} |
| 248 | - for node in ordered: | ||
| 249 | - node_id = node["node_id"] | ||
| 250 | - node_exec_id = _ensure_node_row( | ||
| 251 | - conn, run_id, node_id, org_node, skill_id, | ||
| 252 | - "work_graph_node_input.schema.json", "work_graph_node_output.schema.json", | ||
| 253 | - {"node_id": node_id}, | ||
| 254 | - ) | ||
| 255 | - prompt = build_prompt(node, outputs) | ||
| 256 | - run_fn = _make_run_fn(conn, target_folder_path, node_exec_id, org_node, prompt, output_schema, timeout) | ||
| 257 | - output = work_node_run.run_work_node( | ||
| 258 | - conn, node_exec_id, {"node_id": node_id}, input_schema, output_schema, run_fn, | ||
| 259 | - needs_worktree=effective_needs_worktree, repo_path=target_folder_path, | ||
| 260 | - sleep=sleep, now=now, | ||
| 261 | - ) | ||
| 262 | - | ||
| 263 | - if verify_fn is not None: | ||
| 264 | - output = _verify_and_maybe_regenerate( | ||
| 265 | - conn, run_id, org_node, skill_id, target_folder_path, node, node_id, prompt, | ||
| 266 | - output, input_schema, output_schema, timeout, effective_needs_worktree, verify_fn, sleep, now, | 335 | + locked_conn = _LockedConn(conn) |
| 336 | + | ||
| 337 | + for layer in layers: | ||
| 338 | + if len(layer) == 1: | ||
| 339 | + node = layer[0] | ||
| 340 | + outputs[node["node_id"]] = _run_one_node( | ||
| 341 | + conn, run_id, org_node, skill_id, target_folder_path, node, outputs, | ||
| 342 | + build_prompt, input_schema, output_schema, timeout, | ||
| 343 | + effective_needs_worktree, verify_fn, sleep, now, | ||
| 267 | ) | 344 | ) |
| 345 | + continue | ||
| 346 | + | ||
| 347 | + worker_count = min(max_workers, len(layer)) | ||
| 348 | + errors = [] | ||
| 349 | + with concurrent.futures.ThreadPoolExecutor(max_workers=worker_count) as pool: | ||
| 350 | + futures = { | ||
| 351 | + pool.submit( | ||
| 352 | + _run_one_node, locked_conn, run_id, org_node, skill_id, target_folder_path, | ||
| 353 | + node, outputs, build_prompt, input_schema, output_schema, timeout, | ||
| 354 | + effective_needs_worktree, verify_fn, sleep, now, | ||
| 355 | + ): node | ||
| 356 | + for node in layer | ||
| 357 | + } | ||
| 358 | + done, _ = concurrent.futures.wait(futures, return_when=concurrent.futures.ALL_COMPLETED) | ||
| 359 | + for future in done: | ||
| 360 | + node = futures[future] | ||
| 361 | + try: | ||
| 362 | + outputs[node["node_id"]] = future.result() | ||
| 363 | + except Exception as e: | ||
| 364 | + errors.append((node["node_id"], e)) | ||
| 365 | + | ||
| 366 | + if errors: | ||
| 367 | + raise WorkGraphLayerError(errors) | ||
| 268 | 368 | ||
| 269 | - outputs[node_id] = output | ||
| 270 | return outputs | 369 | return outputs |
| 271 | 370 | ||
| 272 | 371 | ||
| 372 | +def _run_one_node( | ||
| 373 | + conn, run_id, org_node, skill_id, target_folder_path, node, outputs, | ||
| 374 | + build_prompt, input_schema, output_schema, timeout, needs_worktree, verify_fn, sleep, now, | ||
| 375 | +): | ||
| 376 | + """One node's full lifecycle (row creation, prompt, run, optional | ||
| 377 | + verify+regenerate) -- shared by both the synchronous single-node-layer | ||
| 378 | + path and the ThreadPoolExecutor multi-node-layer path in run_graph(), | ||
| 379 | + so there's exactly one implementation of "run a node" to maintain.""" | ||
| 380 | + node_id = node["node_id"] | ||
| 381 | + node_exec_id = _ensure_node_row( | ||
| 382 | + conn, run_id, node_id, org_node, skill_id, | ||
| 383 | + "work_graph_node_input.schema.json", "work_graph_node_output.schema.json", | ||
| 384 | + {"node_id": node_id}, | ||
| 385 | + ) | ||
| 386 | + prompt = build_prompt(node, outputs) | ||
| 387 | + run_fn = _make_run_fn(conn, target_folder_path, node_exec_id, org_node, prompt, output_schema, timeout) | ||
| 388 | + output = work_node_run.run_work_node( | ||
| 389 | + conn, node_exec_id, {"node_id": node_id}, input_schema, output_schema, run_fn, | ||
| 390 | + needs_worktree=needs_worktree, repo_path=target_folder_path, sleep=sleep, now=now, | ||
| 391 | + ) | ||
| 392 | + if verify_fn is not None: | ||
| 393 | + output = _verify_and_maybe_regenerate( | ||
| 394 | + conn, run_id, org_node, skill_id, target_folder_path, node, node_id, prompt, | ||
| 395 | + output, input_schema, output_schema, timeout, needs_worktree, verify_fn, sleep, now, | ||
| 396 | + ) | ||
| 397 | + return output | ||
| 398 | + | ||
| 399 | + | ||
| 273 | def _verify_and_maybe_regenerate( | 400 | def _verify_and_maybe_regenerate( |
| 274 | conn, run_id, org_node, skill_id, target_folder_path, node, node_id, prompt, | 401 | conn, run_id, org_node, skill_id, target_folder_path, node, node_id, prompt, |
| 275 | output, input_schema, output_schema, timeout, needs_worktree, verify_fn, sleep, now, | 402 | output, input_schema, output_schema, timeout, needs_worktree, verify_fn, sleep, now, |
src/work_node_run.py
| @@ -32,6 +32,7 @@ import json | @@ -32,6 +32,7 @@ import json | ||
| 32 | import os | 32 | import os |
| 33 | import re | 33 | import re |
| 34 | import subprocess | 34 | import subprocess |
| 35 | +import threading | ||
| 35 | import time | 36 | import time |
| 36 | import uuid | 37 | import uuid |
| 37 | from datetime import datetime, timezone | 38 | from datetime import datetime, timezone |
| @@ -105,15 +106,24 @@ def _quota_limit_type(text): | @@ -105,15 +106,24 @@ def _quota_limit_type(text): | ||
| 105 | return match.group(1).lower() if match else None | 106 | return match.group(1).lower() if match else None |
| 106 | 107 | ||
| 107 | 108 | ||
| 109 | +_worktree_creation_lock = threading.Lock() | ||
| 110 | + | ||
| 111 | + | ||
| 108 | def create_worktree(repo_path, node_exec_id): | 112 | def create_worktree(repo_path, node_exec_id): |
| 109 | """`git worktree add` for an isolated code-writing node (01-M | 113 | """`git worktree add` for an isolated code-writing node (01-M |
| 110 | - work_node_execution.git_worktree_ref).""" | 114 | + work_node_execution.git_worktree_ref). Serialized across threads via a |
| 115 | + module-level lock -- `git worktree add` mutates shared `.git/worktrees/` | ||
| 116 | + metadata, and concurrent Work Graph layers (G) may call this from | ||
| 117 | + multiple threads for the same repo_path at once. The lock only wraps | ||
| 118 | + this one git invocation (milliseconds), not the node's actual work, so | ||
| 119 | + it doesn't meaningfully reduce the parallelism gained elsewhere.""" | ||
| 111 | worktree_path = os.path.join(repo_path, ".graphlemon-worktrees", node_exec_id) | 120 | worktree_path = os.path.join(repo_path, ".graphlemon-worktrees", node_exec_id) |
| 112 | branch = "graphlemon/" + node_exec_id | 121 | branch = "graphlemon/" + node_exec_id |
| 113 | - subprocess.run( | ||
| 114 | - ["git", "worktree", "add", "-b", branch, worktree_path], | ||
| 115 | - cwd=repo_path, check=True, capture_output=True, text=True, | ||
| 116 | - ) | 122 | + with _worktree_creation_lock: |
| 123 | + subprocess.run( | ||
| 124 | + ["git", "worktree", "add", "-b", branch, worktree_path], | ||
| 125 | + cwd=repo_path, check=True, capture_output=True, text=True, | ||
| 126 | + ) | ||
| 117 | return worktree_path | 127 | return worktree_path |
| 118 | 128 | ||
| 119 | 129 |