even more code review

This commit is contained in:
lda
2026-05-22 22:10:44 +07:00 Verified
parent 6c1c8abc52
commit 9fe9c1452b
8 changed files with 51 additions and 53 deletions
+7 -5
View File
@@ -100,8 +100,10 @@ _Avoid_: Job, invocation
browser, or external service calls. browser, or external service calls.
- Future concurrency-specific foreach settings should live in a nested - Future concurrency-specific foreach settings should live in a nested
policy object rather than expanding `ForeachNode` with many top-level fields. policy object rather than expanding `ForeachNode` with many top-level fields.
- Item error handling is foreach-wide, not concurrent-only. Serial and concurrent - Item error handling is foreach-wide, not concurrent-only. Today, serial
foreach can both profit from `fail`, `skip`, or `collect` item failure policy. foreach supports `fail`; concurrent foreach supports `fail`, `skip`, and
`collect`. Future serial foreach can reuse the same `skip`/`collect` policy
shape when its execution path is upgraded.
- `collect` item error policy must declare an explicit destination for - `collect` item error policy must declare an explicit destination for
structured item errors. Collected errors should be ordered by item index, not structured item errors. Collected errors should be ordered by item index, not
async completion order. async completion order.
@@ -271,9 +273,9 @@ _Avoid_: Job, invocation
branch policy. branch policy.
- Unsupported concurrent foreach semantics should be rejected by validation before - Unsupported concurrent foreach semantics should be rejected by validation before
runtime. Runtime may stay defensive, but validation owns the user-facing gate. runtime. Runtime may stay defensive, but validation owns the user-facing gate.
- `on_item_error="collect"` and `"skip"` are future policy shapes unless runtime - `on_item_error="collect"` and `"skip"` are supported for concurrent foreach.
support is explicitly implemented. Current scheduler work should make official Serial foreach still behaves as fail-only until its execution path explicitly
support easier, not pretend it already exists. adopts barrier-buffered item error handling.
- A **Trace** records actual scheduler execution order; grouping or sorting by - A **Trace** records actual scheduler execution order; grouping or sorting by
foreach index is a presentation concern. foreach index is a presentation concern.
- Concurrent child frames may write to the same state path only through a - Concurrent child frames may write to the same state path only through a
@@ -120,7 +120,7 @@ def test_barrier_rejects_sibling_same_path_writes_with_explicit_replace() -> Non
edges=[], edges=[],
) )
with pytest.raises(WorkflowExecutionError, match="requires an explicit reducer"): with pytest.raises(WorkflowExecutionError, match="mergeable reducer"):
build_barrier_patch( build_barrier_patch(
workflow, workflow,
[ [
@@ -131,7 +131,7 @@ def test_barrier_rejects_sibling_same_path_writes_with_explicit_replace() -> Non
) )
``` ```
- [ ] **Step 4: Add test for explicit reducer allowing same-path writes** - [ ] **Step 4: Add test for mergeable reducer allowing same-path writes**
Append: Append:
@@ -265,19 +265,20 @@ class _BarrierWrite:
source_key: str source_key: str
``` ```
- [ ] **Step 2: Add explicit reducer predicate** - [ ] **Step 2: Add reducer policy predicate**
Add below `build_barrier_patch(...)` or near private helpers: Add below `build_barrier_patch(...)` or near private helpers:
```python ```python
def _has_explicit_non_replace_reducer( def _allows_sibling_writes(
path: StatePath, path: StatePath,
state_fields: Mapping[StatePath, StateFieldDecl], state_fields: Mapping[StatePath, StateFieldDecl],
reducers: Mapping[str, ReducerDefinition] | None,
) -> bool: ) -> bool:
field = state_fields.get(path) field = state_fields.get(path)
if field is None or field.reducer is None: if field is None or field.reducer is None:
return False return False
return field.reducer.name != "wf.std.replace" return reducer_allows_sibling_writes(field.reducer, reducers)
``` ```
If `StateFieldDecl.reducer` is never `None` for undeclared/default fields, inspect the actual model and adjust: If `StateFieldDecl.reducer` is never `None` for undeclared/default fields, inspect the actual model and adjust:
@@ -286,7 +287,7 @@ If `StateFieldDecl.reducer` is never `None` for undeclared/default fields, inspe
return field.reducer.name != "wf.std.replace" return field.reducer.name != "wf.std.replace"
``` ```
but preserve the rule: only an explicit declared non-replace reducer allows sibling same-path writes. but preserve the rule: only a mergeable reducer allows sibling same-path writes.
- [ ] **Step 3: Add overlap predicate for barrier paths** - [ ] **Step 3: Add overlap predicate for barrier paths**
@@ -344,11 +345,11 @@ def validate_barrier_writes(
if left.item_index == right.item_index: if left.item_index == right.item_index:
continue continue
if left.path == right.path: if left.path == right.path:
if _has_explicit_non_replace_reducer(left.path, state_fields): if _allows_sibling_writes(left.path, state_fields, reducers):
continue continue
raise WorkflowExecutionError( raise WorkflowExecutionError(
"multiple sibling writes to " "multiple sibling writes to "
f"{left.source_key!r} require an explicit reducer" f"{left.source_key!r} require a mergeable reducer"
) )
if _state_paths_overlap(left.path, right.path): if _state_paths_overlap(left.path, right.path):
raise WorkflowExecutionError( raise WorkflowExecutionError(
@@ -464,7 +465,7 @@ Append:
def test_sync_concurrent_foreach_rejects_sibling_replace_writes() -> None: def test_sync_concurrent_foreach_rejects_sibling_replace_writes() -> None:
workflow = _same_path_replace_workflow() workflow = _same_path_replace_workflow()
with pytest.raises(WorkflowExecutionError, match="explicit reducer"): with pytest.raises(WorkflowExecutionError, match="mergeable reducer"):
execute_workflow( execute_workflow(
workflow, workflow,
{"items": ["a", "b"]}, {"items": ["a", "b"]},
@@ -501,10 +502,11 @@ Expected: pass.
- Modify: `docs/adr/0002-concurrent-foreach-policy-and-barrier-commits.md` - Modify: `docs/adr/0002-concurrent-foreach-policy-and-barrier-commits.md`
- Modify: `docs/superpowers/plans/2026-05-22-concurrent-foreach-phase4-roadmap.md` - Modify: `docs/superpowers/plans/2026-05-22-concurrent-foreach-phase4-roadmap.md`
- [ ] **Step 1: Update ADR merge rules current state** - [ ] **Step 1: Verify ADR merge rules current state**
In `docs/adr/0002-concurrent-foreach-policy-and-barrier-commits.md`, under In `docs/adr/0002-concurrent-foreach-policy-and-barrier-commits.md`, under
`## Merge and Reducer Rules`, append: `## Merge and Reducer Rules`, verify that the implementation status is
documented:
```markdown ```markdown
Current barrier validation enforces this policy for sibling foreach item Current barrier validation enforces this policy for sibling foreach item
@@ -513,10 +515,10 @@ destination state path. Ancestor/descendant sibling writes are rejected until a
future explicit deep merge policy exists. future explicit deep merge policy exists.
``` ```
- [ ] **Step 2: Update roadmap Slice 3** - [ ] **Step 2: Verify roadmap Slice 3**
In `docs/superpowers/plans/2026-05-22-concurrent-foreach-phase4-roadmap.md`, In `docs/superpowers/plans/2026-05-22-concurrent-foreach-phase4-roadmap.md`,
under Slice 3, add: under Slice 3, verify the plan points to this slice:
```markdown ```markdown
Plan: Plan:
@@ -531,7 +533,7 @@ If implementing immediately, also mark it as implemented in Current State after
Run: Run:
```bash ```bash
rg -n "sibling writes|ancestor/descendant|explicit reducer|barrier write" docs/adr/0002-concurrent-foreach-policy-and-barrier-commits.md docs/superpowers/plans/2026-05-22-concurrent-foreach-phase4-roadmap.md rg -n "sibling writes|ancestor/descendant|mergeable reducer|barrier write" docs/adr/0002-concurrent-foreach-policy-and-barrier-commits.md docs/superpowers/plans/2026-05-22-concurrent-foreach-phase4-roadmap.md
``` ```
Expected: the ADR and roadmap both mention the semantics. Expected: the ADR and roadmap both mention the semantics.
@@ -589,6 +591,6 @@ Expected: all pass with 0 type errors.
## Self-Review ## Self-Review
- Spec coverage: the plan covers same-path sibling writes, explicit reducer requirements, replace rejection, ancestor/descendant rejection, deterministic reducer order, end-to-end foreach behavior, and docs. - Spec coverage: the plan covers same-path sibling writes, mergeable reducer requirements, replace rejection, ancestor/descendant rejection, deterministic reducer order, end-to-end foreach behavior, and docs.
- Placeholder scan: all tasks include concrete code or exact commands; no TBD placeholders. - Placeholder scan: all tasks include concrete code or exact commands; no TBD placeholders.
- Type consistency: the plan uses existing `StatePatch`, `StatePath`, `StateFieldDecl`, `ReducerRef`, `StateSchema.from_field_map`, and `WorkflowExecutionError`. - Type consistency: the plan uses existing `StatePatch`, `StatePath`, `StateFieldDecl`, `ReducerRef`, `StateSchema.from_field_map`, and `WorkflowExecutionError`.
@@ -544,17 +544,8 @@ Expected: pass.
- [ ] **Step 1: Update ADR current-state note** - [ ] **Step 1: Update ADR current-state note**
In `docs/adr/0002-concurrent-foreach-policy-and-barrier-commits.md`, replace the V1 limitation paragraph: In `docs/adr/0002-concurrent-foreach-policy-and-barrier-commits.md`, verify the
barrier commit section contains:
```markdown
Current sync V1 implements the barrier commit path only for `loop -> one node ->
END` item bodies. The runtime includes an explicit no-op overlay seam
(`state_view_for_frame`) so the next slice can add lineage-local reads without
rewiring node execution. Until that seam becomes real, multi-step concurrent
item bodies are rejected instead of reading stale parent state.
```
with:
```markdown ```markdown
Current sync execution supports item-local read overlays for concurrent foreach Current sync execution supports item-local read overlays for concurrent foreach
@@ -564,10 +555,10 @@ later nodes in the same item lineage. Sibling overlays remain invisible until
the foreach barrier commits. the foreach barrier commits.
``` ```
- [ ] **Step 2: Update roadmap slice statuses** - [ ] **Step 2: Verify roadmap slice statuses**
In `docs/superpowers/plans/2026-05-22-concurrent-foreach-phase4-roadmap.md`, In `docs/superpowers/plans/2026-05-22-concurrent-foreach-phase4-roadmap.md`,
mark Slice 1 as implemented and add/link this plan under Slice 2. verify Slice 1 is marked implemented and this plan is linked under Slice 2.
Use: Use:
+2 -3
View File
@@ -139,9 +139,8 @@ limits and intended adapter seam.
- Frames are no longer only a serial execution stack: the runtime has a ready - Frames are no longer only a serial execution stack: the runtime has a ready
queue and `BLOCKED` frame state. Concurrent foreach and native subgraphs still queue and `BLOCKED` frame state. Concurrent foreach and native subgraphs still
need more work: lineage isolation, barrier merge semantics, pending child need more work: lineage isolation, barrier merge semantics, pending child
results, and explicit child workflow/deployment identity. Async runtime can results, and explicit child workflow/deployment identity. Concurrent foreach
later add simultaneous async node handler execution, but the workflow mode is is the primary current use case for async concurrent node handler execution.
still concurrent foreach.
- Runtime errors are still ordinary exceptions plus failed run status. A richer - Runtime errors are still ordinary exceptions plus failed run status. A richer
error payload can be added later, but should be designed as part of trace/run error payload can be added later, but should be designed as part of trace/run
state rather than scattered exceptions. state rather than scattered exceptions.
+6 -4
View File
@@ -154,10 +154,11 @@ def execute_node_use(
f"no handler registered for node def {node.node!r}" f"no handler registered for node def {node.node!r}"
) )
frame = run.current_frame()
resolved_input, context, state_view = _resolve_node_execution( resolved_input, context, state_view = _resolve_node_execution(
workflow=workflow, workflow=workflow,
run=run, run=run,
frame=run.current_frame(), frame=frame,
node=node, node=node,
node_def=node_def, node_def=node_def,
) )
@@ -165,7 +166,7 @@ def execute_node_use(
return _finalize_node_execution( return _finalize_node_execution(
workflow=workflow, workflow=workflow,
run=run, run=run,
frame=run.current_frame(), frame=frame,
node=node, node=node,
node_def=node_def, node_def=node_def,
resolved_input=resolved_input, resolved_input=resolved_input,
@@ -189,10 +190,11 @@ async def execute_node_use_async(
f"no handler registered for node def {node.node!r}" f"no handler registered for node def {node.node!r}"
) )
frame = run.current_frame()
resolved_input, context, state_view = _resolve_node_execution( resolved_input, context, state_view = _resolve_node_execution(
workflow=workflow, workflow=workflow,
run=run, run=run,
frame=run.current_frame(), frame=frame,
node=node, node=node,
node_def=node_def, node_def=node_def,
) )
@@ -204,7 +206,7 @@ async def execute_node_use_async(
return _finalize_node_execution( return _finalize_node_execution(
workflow=workflow, workflow=workflow,
run=run, run=run,
frame=run.current_frame(), frame=frame,
node=node, node=node,
node_def=node_def, node_def=node_def,
resolved_input=resolved_input, resolved_input=resolved_input,
+2
View File
@@ -30,6 +30,8 @@ def state_view_for_frame(run: RunState, frame: ExecutionFrame) -> dict[str, Any]
if pending is None: if pending is None:
return run.state return run.state
# Correctness first: this full copy isolates sibling reads. If state grows
# large, replace this with a lazy/copy-on-write overlay.
state_view = deepcopy(run.state) state_view = deepcopy(run.state)
for destination, value in pending.patch.changes.items(): for destination, value in pending.patch.changes.items():
path = StatePath.parse(destination) path = StatePath.parse(destination)
+12 -12
View File
@@ -111,18 +111,18 @@ def enqueue_frame(run: RunState, frame_id: str, *, front: bool = False) -> None:
def select_next_frame(run: RunState) -> ExecutionFrame | None: def select_next_frame(run: RunState) -> ExecutionFrame | None:
"""Select the next ready frame and update compatibility cursor fields.""" """Select the next ready frame and update compatibility cursor fields."""
while run.ready_frame_ids: if not run.ready_frame_ids:
frame_id = run.ready_frame_ids.pop(0) return None
frame = _frame(run, frame_id) frame_id = run.ready_frame_ids.pop(0)
if frame.status != FrameStatus.PENDING: frame = _frame(run, frame_id)
raise WorkflowExecutionError( if frame.status != FrameStatus.PENDING:
f"ready frame {frame_id!r} has status {frame.status!s}" raise WorkflowExecutionError(
) f"ready frame {frame_id!r} has status {frame.status!s}"
frame.status = FrameStatus.RUNNING )
run.current_frame_id = frame.id frame.status = FrameStatus.RUNNING
run.sync_from_current_frame() run.current_frame_id = frame.id
return frame run.sync_from_current_frame()
return None return frame
def mark_frame_pending(run: RunState, frame_id: str, *, front: bool = False) -> None: def mark_frame_pending(run: RunState, frame_id: str, *, front: bool = False) -> None:
+1 -1
View File
@@ -237,7 +237,7 @@ async def _step_async_foreach_item_batch(
"""Run one batch of ready concurrent-foreach item node handlers. """Run one batch of ready concurrent-foreach item node handlers.
Only handler awaits run concurrently. Finalization, tracing, and frame Only handler awaits run concurrently. Finalization, tracing, and frame
advancement happen afterward in frame-id order so `RunState` is mutated advancement happen afterward in ready-queue order so `RunState` is mutated
deterministically. deterministically.
""" """
frames = [first_frame, *_claim_matching_async_item_frames(run, index, first_frame)] frames = [first_frame, *_claim_matching_async_item_frames(run, index, first_frame)]