more code review and plans
This commit is contained in:
@@ -0,0 +1,643 @@
|
|||||||
|
# Concurrent Foreach Item Overlays Implementation Plan
|
||||||
|
|
||||||
|
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||||
|
|
||||||
|
**Goal:** Make concurrent foreach item frames read their own buffered state writes so multi-step item bodies are safe.
|
||||||
|
|
||||||
|
**Architecture:** Keep `RunState.state` as committed parent state. Store item-local overlay patches in the foreach parent barrier, keyed by item index/frame id. `state_view_for_frame(...)` should return parent state plus that item's staged overlay; sibling overlays remain invisible. This plan does not implement sibling write conflict policy; Slice 3 owns write semantics across different item lineages.
|
||||||
|
|
||||||
|
**Tech Stack:** Python 3.14, dataclasses, pytest, existing `StatePatch`, `ForeachBarrierState`, `safe_resolve_path`, and runtime scheduler modules.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Boundary With Slice 3
|
||||||
|
|
||||||
|
This slice answers:
|
||||||
|
|
||||||
|
- Can node B in the same concurrent item read node A's buffered state write?
|
||||||
|
- Can multi-step item bodies run without reading stale parent state?
|
||||||
|
- Are sibling item overlays isolated from each other?
|
||||||
|
|
||||||
|
This slice does **not** answer:
|
||||||
|
|
||||||
|
- Should two sibling items be allowed to write the same state path without a reducer?
|
||||||
|
- Should ancestor/descendant sibling writes conflict?
|
||||||
|
- Should barrier trace `state_changes` show raw per-item inputs or final aggregate values?
|
||||||
|
|
||||||
|
Those are Slice 3 write semantics. Do not add broad write-conflict policy here except what is already enforced by `build_output_patch(...)` for a single node output.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Files
|
||||||
|
|
||||||
|
- Modify: `src/wf_core/runtime/foreach_state.py`
|
||||||
|
- Accumulate successful item patches per item instead of storing only one patch.
|
||||||
|
- Expose helpers to get item overlay patches by frame/index.
|
||||||
|
- Modify: `src/wf_core/runtime/ops/overlays.py`
|
||||||
|
- Replace the current no-op seam with parent-state plus item-local staged writes.
|
||||||
|
- Modify: `src/wf_core/runtime/ops/nodes.py`
|
||||||
|
- Build output patches against the frame-visible state view, not always `run.state`.
|
||||||
|
- Append item-local patches for concurrent item frames.
|
||||||
|
- Modify: `src/wf_core/runtime/ops/foreach.py`
|
||||||
|
- Remove the single-node item-body guard.
|
||||||
|
- Keep `item_error.action != "fail"` unsupported.
|
||||||
|
- Test: `tests/core/test_concurrent_foreach.py`
|
||||||
|
- Add multi-step item-body tests.
|
||||||
|
- Test: `tests/core/test_foreach_barrier_state.py`
|
||||||
|
- Add item patch accumulation tests.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 1: Add Failing Multi-Step Overlay Tests
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `tests/core/test_concurrent_foreach.py`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Add a two-node item body test**
|
||||||
|
|
||||||
|
Append this test:
|
||||||
|
|
||||||
|
```python
|
||||||
|
def test_sync_concurrent_foreach_item_reads_own_buffered_write() -> None:
|
||||||
|
workflow = _workflow(
|
||||||
|
state_schema=StateSchema.from_field_map(
|
||||||
|
{
|
||||||
|
"items": StateField(type="array"),
|
||||||
|
"scratch": StateField(type="string"),
|
||||||
|
"seen": StateField(
|
||||||
|
type="array",
|
||||||
|
reducer=ReducerRef(name="wf.std.append"),
|
||||||
|
),
|
||||||
|
}
|
||||||
|
),
|
||||||
|
foreach=ForeachNode.model_validate(
|
||||||
|
{
|
||||||
|
"id": "each",
|
||||||
|
"type": "foreach",
|
||||||
|
"over": "state.items",
|
||||||
|
"as": "item",
|
||||||
|
"mode": "concurrent",
|
||||||
|
"concurrent": {"max_active": 2, "max_outstanding": 2},
|
||||||
|
}
|
||||||
|
),
|
||||||
|
)
|
||||||
|
workflow.node_defs.extend(
|
||||||
|
[
|
||||||
|
NodeDef(
|
||||||
|
name="read_scratch",
|
||||||
|
input_schema=SchemaRef(
|
||||||
|
type="object",
|
||||||
|
properties={"scratch": {}},
|
||||||
|
required=["scratch"],
|
||||||
|
),
|
||||||
|
output_schema=SchemaRef(
|
||||||
|
type="object",
|
||||||
|
properties={"seen": {}},
|
||||||
|
required=["seen"],
|
||||||
|
),
|
||||||
|
outcomes=["ok"],
|
||||||
|
)
|
||||||
|
]
|
||||||
|
)
|
||||||
|
workflow.nodes.append(
|
||||||
|
NodeUse.model_validate(
|
||||||
|
{
|
||||||
|
"id": "read_scratch",
|
||||||
|
"type": "node",
|
||||||
|
"node": "read_scratch",
|
||||||
|
"input": [{"target": "scratch", "path": "state.scratch"}],
|
||||||
|
"output": [{"source": "seen", "target": "state.seen"}],
|
||||||
|
}
|
||||||
|
)
|
||||||
|
)
|
||||||
|
workflow.edges = [
|
||||||
|
Edge.model_validate({"from": "each", "outcome": "loop", "to": "record"}),
|
||||||
|
Edge.model_validate({"from": "record", "outcome": "ok", "to": "read_scratch"}),
|
||||||
|
Edge.model_validate({"from": "read_scratch", "outcome": "ok", "to": END}),
|
||||||
|
Edge.model_validate({"from": "each", "outcome": "done", "to": END}),
|
||||||
|
]
|
||||||
|
|
||||||
|
run = execute_workflow(
|
||||||
|
workflow,
|
||||||
|
{"items": ["a", "b", "c"]},
|
||||||
|
{
|
||||||
|
"record": lambda payload, _ctx: {
|
||||||
|
"outcome": "ok",
|
||||||
|
"output": {"scratch": f"scratch:{payload['value']}"},
|
||||||
|
},
|
||||||
|
"read_scratch": lambda payload, _ctx: {
|
||||||
|
"outcome": "ok",
|
||||||
|
"output": {"seen": payload["scratch"]},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
|
assert run.state["seen"] == ["scratch:a", "scratch:b", "scratch:c"]
|
||||||
|
```
|
||||||
|
|
||||||
|
Important detail: update the existing `record` `NodeUse` in `_workflow(...)` or inside this test so it writes `scratch` to `state.scratch` for this workflow. If `_workflow(...)` is too fixed for that, create a small dedicated helper for this test instead of making `_workflow(...)` harder to read.
|
||||||
|
|
||||||
|
- [ ] **Step 2: Add a sibling isolation test**
|
||||||
|
|
||||||
|
Append this test:
|
||||||
|
|
||||||
|
```python
|
||||||
|
def test_sync_concurrent_foreach_sibling_overlays_do_not_leak() -> None:
|
||||||
|
workflow = _workflow(
|
||||||
|
state_schema=StateSchema.from_field_map(
|
||||||
|
{
|
||||||
|
"items": StateField(type="array"),
|
||||||
|
"scratch": StateField(type="string"),
|
||||||
|
"seen": StateField(
|
||||||
|
type="array",
|
||||||
|
reducer=ReducerRef(name="wf.std.append"),
|
||||||
|
),
|
||||||
|
}
|
||||||
|
),
|
||||||
|
foreach=ForeachNode.model_validate(
|
||||||
|
{
|
||||||
|
"id": "each",
|
||||||
|
"type": "foreach",
|
||||||
|
"over": "state.items",
|
||||||
|
"as": "item",
|
||||||
|
"mode": "concurrent",
|
||||||
|
"concurrent": {"max_active": 2, "max_outstanding": 2},
|
||||||
|
}
|
||||||
|
),
|
||||||
|
)
|
||||||
|
workflow.node_defs.extend(
|
||||||
|
[
|
||||||
|
NodeDef(
|
||||||
|
name="read_scratch",
|
||||||
|
input_schema=SchemaRef(
|
||||||
|
type="object",
|
||||||
|
properties={"scratch": {}},
|
||||||
|
required=["scratch"],
|
||||||
|
),
|
||||||
|
output_schema=SchemaRef(
|
||||||
|
type="object",
|
||||||
|
properties={"seen": {}},
|
||||||
|
required=["seen"],
|
||||||
|
),
|
||||||
|
outcomes=["ok"],
|
||||||
|
)
|
||||||
|
]
|
||||||
|
)
|
||||||
|
workflow.nodes.append(
|
||||||
|
NodeUse.model_validate(
|
||||||
|
{
|
||||||
|
"id": "read_scratch",
|
||||||
|
"type": "node",
|
||||||
|
"node": "read_scratch",
|
||||||
|
"input": [{"target": "scratch", "path": "state.scratch"}],
|
||||||
|
"output": [{"source": "seen", "target": "state.seen"}],
|
||||||
|
}
|
||||||
|
)
|
||||||
|
)
|
||||||
|
workflow.edges = [
|
||||||
|
Edge.model_validate({"from": "each", "outcome": "loop", "to": "record"}),
|
||||||
|
Edge.model_validate({"from": "record", "outcome": "ok", "to": "read_scratch"}),
|
||||||
|
Edge.model_validate({"from": "read_scratch", "outcome": "ok", "to": END}),
|
||||||
|
Edge.model_validate({"from": "each", "outcome": "done", "to": END}),
|
||||||
|
]
|
||||||
|
|
||||||
|
run = execute_workflow(
|
||||||
|
workflow,
|
||||||
|
{"items": ["a", "b"]},
|
||||||
|
{
|
||||||
|
"record": lambda payload, _ctx: {
|
||||||
|
"outcome": "ok",
|
||||||
|
"output": {"scratch": payload["value"]},
|
||||||
|
},
|
||||||
|
"read_scratch": lambda payload, _ctx: {
|
||||||
|
"outcome": "ok",
|
||||||
|
"output": {"seen": payload["scratch"]},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
|
assert run.state["seen"] == ["a", "b"]
|
||||||
|
```
|
||||||
|
|
||||||
|
This catches the bad implementation where item `b` sees item `a`'s staged write or vice versa.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Run tests and verify failure**
|
||||||
|
|
||||||
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
uv run pytest tests/core/test_concurrent_foreach.py::test_sync_concurrent_foreach_item_reads_own_buffered_write tests/core/test_concurrent_foreach.py::test_sync_concurrent_foreach_sibling_overlays_do_not_leak -q
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected before implementation:
|
||||||
|
|
||||||
|
```text
|
||||||
|
FAILED with "concurrent foreach v1 only supports loop bodies with one node"
|
||||||
|
```
|
||||||
|
|
||||||
|
or, if the guard was already removed by another worker:
|
||||||
|
|
||||||
|
```text
|
||||||
|
FAILED because state.scratch is missing/stale
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 2: Accumulate Per-Item Patches
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `src/wf_core/runtime/foreach_state.py`
|
||||||
|
- Modify: `tests/core/test_foreach_barrier_state.py`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Add patch accumulation tests**
|
||||||
|
|
||||||
|
Append to `tests/core/test_foreach_barrier_state.py`:
|
||||||
|
|
||||||
|
```python
|
||||||
|
def test_foreach_barrier_accumulates_multiple_patches_for_one_item() -> None:
|
||||||
|
barrier = ForeachBarrierState(mode="concurrent")
|
||||||
|
|
||||||
|
barrier.add_success_patch(
|
||||||
|
index=0,
|
||||||
|
frame_id="child-0",
|
||||||
|
patch=StatePatch(changes={"state.scratch": "a"}),
|
||||||
|
)
|
||||||
|
barrier.add_success_patch(
|
||||||
|
index=0,
|
||||||
|
frame_id="child-0",
|
||||||
|
patch=StatePatch(changes={"state.seen": "a"}),
|
||||||
|
)
|
||||||
|
|
||||||
|
result = barrier.pending_results[0]
|
||||||
|
assert result.patch.changes["state.scratch"] == "a"
|
||||||
|
assert result.patch.changes["state.seen"] == "a"
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Run test and verify failure**
|
||||||
|
|
||||||
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
uv run pytest tests/core/test_foreach_barrier_state.py::test_foreach_barrier_accumulates_multiple_patches_for_one_item -q
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected before implementation:
|
||||||
|
|
||||||
|
```text
|
||||||
|
FAILED with "already recorded"
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 3: Replace duplicate rejection with patch merge**
|
||||||
|
|
||||||
|
In `src/wf_core/runtime/foreach_state.py`, change `add_success_patch(...)` to merge changes for the same item:
|
||||||
|
|
||||||
|
```python
|
||||||
|
def add_success_patch(
|
||||||
|
self, *, index: int, frame_id: str, patch: StatePatch
|
||||||
|
) -> None:
|
||||||
|
"""Buffer or extend successful item patches by item index.
|
||||||
|
|
||||||
|
A multi-step item body may produce multiple node patches. They are
|
||||||
|
accumulated for the same item lineage and replayed by the barrier in
|
||||||
|
item index order. Overlap inside one item remains governed by the normal
|
||||||
|
node output patch rules for each node; Slice 3 owns sibling conflict
|
||||||
|
policy at the barrier.
|
||||||
|
"""
|
||||||
|
existing = self.pending_results.get(index)
|
||||||
|
if existing is None:
|
||||||
|
self.pending_results[index] = PendingItemResult(
|
||||||
|
index=index,
|
||||||
|
frame_id=frame_id,
|
||||||
|
status="succeeded",
|
||||||
|
patch=patch,
|
||||||
|
)
|
||||||
|
return
|
||||||
|
if existing.frame_id != frame_id:
|
||||||
|
raise WorkflowExecutionError(
|
||||||
|
f"foreach item result for index {index!r} belongs to frame "
|
||||||
|
f"{existing.frame_id!r}, got {frame_id!r}"
|
||||||
|
)
|
||||||
|
existing.patch.changes.update(patch.changes)
|
||||||
|
```
|
||||||
|
|
||||||
|
This intentionally updates only `changes`; the barrier replays changes into a fresh staged state later. Do not try to merge `_prepared_writes` here.
|
||||||
|
|
||||||
|
- [ ] **Step 4: Update duplicate test**
|
||||||
|
|
||||||
|
Replace the prior duplicate-item-result test with a frame-mismatch test:
|
||||||
|
|
||||||
|
```python
|
||||||
|
def test_foreach_barrier_rejects_item_result_frame_mismatch() -> None:
|
||||||
|
barrier = ForeachBarrierState(mode="concurrent")
|
||||||
|
patch = StatePatch(changes={"state.count": 1})
|
||||||
|
|
||||||
|
barrier.add_success_patch(index=0, frame_id="child-0", patch=patch)
|
||||||
|
|
||||||
|
with pytest.raises(WorkflowExecutionError, match="belongs to frame"):
|
||||||
|
barrier.add_success_patch(index=0, frame_id="child-1", patch=patch)
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 5: Verify barrier tests**
|
||||||
|
|
||||||
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
uv run pytest tests/core/test_foreach_barrier_state.py -q
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: pass.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 3: Build Item-Local State Views
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `src/wf_core/runtime/ops/overlays.py`
|
||||||
|
- Test: `tests/core/test_concurrent_foreach.py`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Implement overlay state view**
|
||||||
|
|
||||||
|
Replace `state_view_for_frame(...)` in `src/wf_core/runtime/ops/overlays.py`:
|
||||||
|
|
||||||
|
```python
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from copy import deepcopy
|
||||||
|
from typing import Any
|
||||||
|
|
||||||
|
from wf_core.run_state import ExecutionFrame, RunState
|
||||||
|
from wf_core.runtime.foreach_state import ForeachBarrierState, item_frame_owner
|
||||||
|
from wf_core.runtime.ops.state import safe_set_nested_value
|
||||||
|
from wf_core.paths import StatePath
|
||||||
|
|
||||||
|
|
||||||
|
def state_view_for_frame(run: RunState, frame: ExecutionFrame) -> dict[str, Any]:
|
||||||
|
"""Return committed parent state plus this frame's item-local overlay.
|
||||||
|
|
||||||
|
Concurrent foreach item frames buffer writes in the parent barrier until the
|
||||||
|
foreach barrier commits. Later nodes in the same item must still read those
|
||||||
|
earlier writes, while sibling item frames must not see them.
|
||||||
|
"""
|
||||||
|
owner = item_frame_owner(frame)
|
||||||
|
if owner is None:
|
||||||
|
return run.state
|
||||||
|
|
||||||
|
parent_frame_id, foreach_node_id, item_index = owner
|
||||||
|
parent_frame = run.frames[parent_frame_id]
|
||||||
|
barrier = ForeachBarrierState.from_frame(parent_frame, foreach_node_id)
|
||||||
|
if barrier is None or barrier.mode != "concurrent":
|
||||||
|
return run.state
|
||||||
|
|
||||||
|
pending = barrier.pending_results.get(item_index)
|
||||||
|
if pending is None:
|
||||||
|
return run.state
|
||||||
|
|
||||||
|
state_view = deepcopy(run.state)
|
||||||
|
for destination, value in pending.patch.changes.items():
|
||||||
|
path = StatePath.parse(destination)
|
||||||
|
safe_set_nested_value(state_view, list(path.parts), value)
|
||||||
|
return state_view
|
||||||
|
```
|
||||||
|
|
||||||
|
Do not apply reducers here. The overlay view is an item-local read model, not the final parent commit. Reducers are applied at patch build time for each node and again at the barrier for aggregate commit.
|
||||||
|
|
||||||
|
- [ ] **Step 2: Verify overlay tests still fail on guard**
|
||||||
|
|
||||||
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
uv run pytest tests/core/test_concurrent_foreach.py::test_sync_concurrent_foreach_item_reads_own_buffered_write -q
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: if the single-node guard is still present, failure remains the guard. If guard was removed by another worker, this may already pass.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 4: Build Output Patches Against Frame State View
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `src/wf_core/runtime/ops/nodes.py`
|
||||||
|
- Test: `tests/core/test_concurrent_foreach.py`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Reuse the resolved state view for output patching**
|
||||||
|
|
||||||
|
Currently `_resolve_node_execution(...)` computes `state_view` but returns only input/context. Change it to return the state view too:
|
||||||
|
|
||||||
|
```python
|
||||||
|
) -> tuple[dict[str, Any], RuntimeContext, dict[str, Any]]:
|
||||||
|
```
|
||||||
|
|
||||||
|
Return:
|
||||||
|
|
||||||
|
```python
|
||||||
|
return resolved_input, context, state_view
|
||||||
|
```
|
||||||
|
|
||||||
|
Update both callers:
|
||||||
|
|
||||||
|
```python
|
||||||
|
resolved_input, context, state_view = _resolve_node_execution(...)
|
||||||
|
```
|
||||||
|
|
||||||
|
Then pass `state_view` into `_finalize_node_execution(...)`:
|
||||||
|
|
||||||
|
```python
|
||||||
|
state=state_view,
|
||||||
|
```
|
||||||
|
|
||||||
|
Add a parameter to `_finalize_node_execution(...)`:
|
||||||
|
|
||||||
|
```python
|
||||||
|
state_view: dict[str, Any],
|
||||||
|
```
|
||||||
|
|
||||||
|
And change `build_output_patch(...)` call from:
|
||||||
|
|
||||||
|
```python
|
||||||
|
run.state,
|
||||||
|
```
|
||||||
|
|
||||||
|
to:
|
||||||
|
|
||||||
|
```python
|
||||||
|
state_view,
|
||||||
|
```
|
||||||
|
|
||||||
|
This is required for node B in one item to build a patch using node A's staged value.
|
||||||
|
|
||||||
|
- [ ] **Step 2: Run focused overlay test**
|
||||||
|
|
||||||
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
uv run pytest tests/core/test_concurrent_foreach.py::test_sync_concurrent_foreach_item_reads_own_buffered_write -q
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: still fails until the single-node guard is removed.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 5: Lift The Single-Node Concurrent Body Restriction
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `src/wf_core/runtime/ops/foreach.py`
|
||||||
|
- Modify: `tests/core/test_concurrent_foreach.py`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Remove the old rejection test**
|
||||||
|
|
||||||
|
Delete or rewrite `test_sync_concurrent_foreach_rejects_multi_step_item_body_for_now`.
|
||||||
|
|
||||||
|
If preserving regression coverage is preferred, replace it with:
|
||||||
|
|
||||||
|
```python
|
||||||
|
def test_sync_concurrent_foreach_allows_multi_step_item_body_with_overlay() -> None:
|
||||||
|
# Use the same workflow shape as
|
||||||
|
# test_sync_concurrent_foreach_item_reads_own_buffered_write.
|
||||||
|
# Assert the workflow completes and output contains all expected values.
|
||||||
|
```
|
||||||
|
|
||||||
|
Prefer not duplicating the full workflow; extract a helper:
|
||||||
|
|
||||||
|
```python
|
||||||
|
def _multi_step_overlay_workflow() -> Workflow:
|
||||||
|
...
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Remove validation call and helper**
|
||||||
|
|
||||||
|
In `src/wf_core/runtime/ops/foreach.py`, remove:
|
||||||
|
|
||||||
|
```python
|
||||||
|
_validate_single_node_loop_body(index, step)
|
||||||
|
```
|
||||||
|
|
||||||
|
Delete `_validate_single_node_loop_body(...)`.
|
||||||
|
|
||||||
|
Remove unused imports:
|
||||||
|
|
||||||
|
```python
|
||||||
|
from wf_core.models.steps import NodeUse
|
||||||
|
from wf_core.tokens import END
|
||||||
|
```
|
||||||
|
|
||||||
|
Keep graph traversal validation out of this slice. Normal workflow validation and runtime edge lookup still define whether graph topology is routable.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Verify multi-step overlay tests**
|
||||||
|
|
||||||
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
uv run pytest tests/core/test_concurrent_foreach.py::test_sync_concurrent_foreach_item_reads_own_buffered_write tests/core/test_concurrent_foreach.py::test_sync_concurrent_foreach_sibling_overlays_do_not_leak -q
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: pass.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 6: Document Overlay Semantics
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `docs/adr/0002-concurrent-foreach-policy-and-barrier-commits.md`
|
||||||
|
- Modify: `docs/superpowers/plans/2026-05-22-concurrent-foreach-phase4-roadmap.md`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Update ADR current-state note**
|
||||||
|
|
||||||
|
In `docs/adr/0002-concurrent-foreach-policy-and-barrier-commits.md`, replace the V1 limitation paragraph:
|
||||||
|
|
||||||
|
```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
|
||||||
|
Current sync execution supports item-local read overlays for concurrent foreach
|
||||||
|
item frames. `RunState.state` remains committed parent state, while
|
||||||
|
`state_view_for_frame` overlays the current item's buffered writes for reads by
|
||||||
|
later nodes in the same item lineage. Sibling overlays remain invisible until
|
||||||
|
the foreach barrier commits.
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Update roadmap slice statuses**
|
||||||
|
|
||||||
|
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.
|
||||||
|
|
||||||
|
Use:
|
||||||
|
|
||||||
|
```markdown
|
||||||
|
Plan:
|
||||||
|
|
||||||
|
- See [`2026-05-22-concurrent-foreach-item-overlays.md`](2026-05-22-concurrent-foreach-item-overlays.md).
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 3: Verify docs reference no stale limitation**
|
||||||
|
|
||||||
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
rg -n "one node|no-op overlay|multi-step concurrent item bodies are rejected" docs src tests
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: no stale claims except historical plan text in the already-completed V1 plan.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 7: Verification
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- No new files unless tests require helper extraction.
|
||||||
|
|
||||||
|
- [ ] **Step 1: Run focused core tests**
|
||||||
|
|
||||||
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
uv run pytest tests/core/test_concurrent_foreach.py tests/core/test_foreach_barrier_state.py tests/core/test_scheduler.py tests/core/test_atomic_state_patches.py -q
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: pass.
|
||||||
|
|
||||||
|
- [ ] **Step 2: Run authoring smoke tests**
|
||||||
|
|
||||||
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
uv run pytest tests/authoring/test_demo_workflow.py tests/authoring/test_builder.py tests/authoring/test_ops.py -q
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: pass.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Run full suite**
|
||||||
|
|
||||||
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
uv run pytest -q
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: pass, allowing known intentional environment-only skips.
|
||||||
|
|
||||||
|
- [ ] **Step 4: Run lint/type/format checks**
|
||||||
|
|
||||||
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
uvx ruff check src tests
|
||||||
|
uvx ruff format --check src tests docs
|
||||||
|
uv run basedpyright --level error src tests
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: all pass with 0 type errors.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Self-Review
|
||||||
|
|
||||||
|
- Spec coverage: the plan makes item-local overlays real, supports multi-step concurrent item bodies, keeps sibling overlays isolated, and explicitly defers sibling write conflict policy.
|
||||||
|
- Placeholder scan: no task uses “TBD” or “add tests” without concrete test content.
|
||||||
|
- Type consistency: the plan uses current names: `ForeachBarrierState`, `PendingItemResult`, `StatePatch`, `state_view_for_frame`, `item_frame_owner`, and `build_output_patch`.
|
||||||
@@ -23,7 +23,11 @@ Already implemented:
|
|||||||
- `StatePatch`, `build_output_patch(...)`, and `commit_state_patch(...)` exist.
|
- `StatePatch`, `build_output_patch(...)`, and `commit_state_patch(...)` exist.
|
||||||
- `ForeachBarrierState`, `PendingItemResult`, and `ItemErrorRecord` exist.
|
- `ForeachBarrierState`, `PendingItemResult`, and `ItemErrorRecord` exist.
|
||||||
- Serial foreach progress now uses `ForeachBarrierState`.
|
- Serial foreach progress now uses `ForeachBarrierState`.
|
||||||
- Runtime still rejects `foreach(mode="concurrent")`.
|
- Sync `foreach(mode="concurrent")` runs with fail-only item policy, bounded
|
||||||
|
admission, deterministic interleaving, and barrier commits for single-node
|
||||||
|
item bodies.
|
||||||
|
- Item-local overlays are not implemented yet, so multi-step concurrent item
|
||||||
|
bodies remain rejected until Slice 2.
|
||||||
|
|
||||||
## Non-Goals For Phase 4
|
## Non-Goals For Phase 4
|
||||||
|
|
||||||
@@ -52,9 +56,30 @@ Plan:
|
|||||||
|
|
||||||
- See [`2026-05-22-concurrent-foreach-v1-sync-fail-only.md`](2026-05-22-concurrent-foreach-v1-sync-fail-only.md).
|
- See [`2026-05-22-concurrent-foreach-v1-sync-fail-only.md`](2026-05-22-concurrent-foreach-v1-sync-fail-only.md).
|
||||||
|
|
||||||
## Slice 2: Barrier Commit Conflict Semantics
|
## Slice 2: Item-Local Overlays
|
||||||
|
|
||||||
Implement after Slice 1 if V1 keeps conflict behavior too conservative.
|
Implement next because Slice 1 intentionally supports only `loop -> one node ->
|
||||||
|
END` item bodies. Overlays let later nodes in one item read earlier buffered
|
||||||
|
writes from the same item without exposing those writes to siblings.
|
||||||
|
|
||||||
|
Scope:
|
||||||
|
|
||||||
|
- `state_view_for_frame(...)` returns committed parent state plus current item
|
||||||
|
overlay for concurrent foreach item frames.
|
||||||
|
- Parent `RunState.state` remains unchanged until the barrier commits.
|
||||||
|
- Item patches accumulate across multiple nodes in the same item lineage.
|
||||||
|
- Multi-step concurrent item bodies are supported.
|
||||||
|
- Sibling item overlays remain isolated.
|
||||||
|
- Sibling write conflict policy remains deferred to Slice 3.
|
||||||
|
|
||||||
|
Plan:
|
||||||
|
|
||||||
|
- See [`2026-05-22-concurrent-foreach-item-overlays.md`](2026-05-22-concurrent-foreach-item-overlays.md).
|
||||||
|
|
||||||
|
## Slice 3: Barrier Commit Conflict Semantics
|
||||||
|
|
||||||
|
Implement after Slice 2 so conflict checks operate on real item-local overlays
|
||||||
|
and multi-step item patches.
|
||||||
|
|
||||||
Scope:
|
Scope:
|
||||||
|
|
||||||
@@ -77,7 +102,7 @@ Key tests:
|
|||||||
- `test_concurrent_foreach_applies_reducer_in_item_index_order`
|
- `test_concurrent_foreach_applies_reducer_in_item_index_order`
|
||||||
- `test_concurrent_foreach_rejects_ancestor_descendant_write_conflict`
|
- `test_concurrent_foreach_rejects_ancestor_descendant_write_conflict`
|
||||||
|
|
||||||
## Slice 3: Item Error Policies
|
## Slice 4: Item Error Policies
|
||||||
|
|
||||||
Implement after barrier success commits are correct.
|
Implement after barrier success commits are correct.
|
||||||
|
|
||||||
@@ -101,7 +126,7 @@ Key tests:
|
|||||||
- `test_concurrent_foreach_collect_writes_ordered_error_records`
|
- `test_concurrent_foreach_collect_writes_ordered_error_records`
|
||||||
- `test_concurrent_foreach_collect_writes_empty_list_on_clean_success`
|
- `test_concurrent_foreach_collect_writes_empty_list_on_clean_success`
|
||||||
|
|
||||||
## Slice 4: Async Concurrent Foreach
|
## Slice 5: Async Concurrent Foreach
|
||||||
|
|
||||||
Implement only after sync semantics are stable.
|
Implement only after sync semantics are stable.
|
||||||
|
|
||||||
@@ -126,7 +151,7 @@ Key tests:
|
|||||||
- `test_async_concurrent_foreach_respects_max_active`
|
- `test_async_concurrent_foreach_respects_max_active`
|
||||||
- `test_async_concurrent_foreach_commits_in_item_index_order`
|
- `test_async_concurrent_foreach_commits_in_item_index_order`
|
||||||
|
|
||||||
## Slice 5: Interrupt Quiescence
|
## Slice 6: Interrupt Quiescence
|
||||||
|
|
||||||
Implement after async execution exists.
|
Implement after async execution exists.
|
||||||
|
|
||||||
@@ -154,10 +179,11 @@ Key tests:
|
|||||||
## Execution Order
|
## Execution Order
|
||||||
|
|
||||||
1. Sync concurrent foreach, fail-only.
|
1. Sync concurrent foreach, fail-only.
|
||||||
2. Barrier conflict semantics, if not fully covered by slice 1.
|
2. Item-local overlays for multi-step item bodies.
|
||||||
3. `skip` / `collect` item error policies.
|
3. Barrier conflict semantics.
|
||||||
4. Async concurrent foreach.
|
4. `skip` / `collect` item error policies.
|
||||||
5. Interrupt quiescence.
|
5. Async concurrent foreach.
|
||||||
|
6. Interrupt quiescence.
|
||||||
|
|
||||||
## Self-Review
|
## Self-Review
|
||||||
|
|
||||||
|
|||||||
@@ -222,6 +222,13 @@ class ForeachBarrierState:
|
|||||||
|
|
||||||
def finish_child(self, frame_id: str) -> None:
|
def finish_child(self, frame_id: str) -> None:
|
||||||
"""Record one child frame as no longer active or outstanding."""
|
"""Record one child frame as no longer active or outstanding."""
|
||||||
|
if (
|
||||||
|
frame_id not in self.active_frame_ids
|
||||||
|
or frame_id not in self.outstanding_frame_ids
|
||||||
|
):
|
||||||
|
raise WorkflowExecutionError(
|
||||||
|
f"foreach child frame {frame_id!r} is not active"
|
||||||
|
)
|
||||||
self.active_frame_ids = tuple(
|
self.active_frame_ids = tuple(
|
||||||
item for item in self.active_frame_ids if item != frame_id
|
item for item in self.active_frame_ids if item != frame_id
|
||||||
)
|
)
|
||||||
@@ -233,6 +240,10 @@ class ForeachBarrierState:
|
|||||||
self, *, index: int, frame_id: str, patch: StatePatch
|
self, *, index: int, frame_id: str, patch: StatePatch
|
||||||
) -> None:
|
) -> None:
|
||||||
"""Buffer one successful item patch by item index."""
|
"""Buffer one successful item patch by item index."""
|
||||||
|
if index in self.pending_results:
|
||||||
|
raise WorkflowExecutionError(
|
||||||
|
f"foreach item result for index {index!r} already recorded"
|
||||||
|
)
|
||||||
self.pending_results[index] = PendingItemResult(
|
self.pending_results[index] = PendingItemResult(
|
||||||
index=index,
|
index=index,
|
||||||
frame_id=frame_id,
|
frame_id=frame_id,
|
||||||
|
|||||||
@@ -223,11 +223,10 @@ def _admit_concurrent_children(
|
|||||||
index: WorkflowIndex,
|
index: WorkflowIndex,
|
||||||
barrier: ForeachBarrierState,
|
barrier: ForeachBarrierState,
|
||||||
iterable: list[object],
|
iterable: list[object],
|
||||||
) -> int:
|
) -> None:
|
||||||
if step.concurrent is None:
|
if step.concurrent is None:
|
||||||
raise WorkflowExecutionError("concurrent foreach requires concurrent policy")
|
raise WorkflowExecutionError("concurrent foreach requires concurrent policy")
|
||||||
|
|
||||||
admitted = 0
|
|
||||||
loop_start = index.next_node_id(frame.node_id, "loop")
|
loop_start = index.next_node_id(frame.node_id, "loop")
|
||||||
while (
|
while (
|
||||||
barrier.next_index < len(iterable)
|
barrier.next_index < len(iterable)
|
||||||
@@ -274,8 +273,6 @@ def _admit_concurrent_children(
|
|||||||
state_changes={},
|
state_changes={},
|
||||||
),
|
),
|
||||||
)
|
)
|
||||||
admitted += 1
|
|
||||||
return admitted
|
|
||||||
|
|
||||||
|
|
||||||
def _finish_concurrent_foreach(
|
def _finish_concurrent_foreach(
|
||||||
|
|||||||
@@ -158,6 +158,11 @@ def build_barrier_patch(
|
|||||||
writes cannot be blindly merged because reducers must see the value produced
|
writes cannot be blindly merged because reducers must see the value produced
|
||||||
by earlier item patches. The barrier therefore replays trace-facing incoming
|
by earlier item patches. The barrier therefore replays trace-facing incoming
|
||||||
changes against a single staged state in deterministic item order.
|
changes against a single staged state in deterministic item order.
|
||||||
|
|
||||||
|
Unlike ordinary node patches, barrier patch `changes` report the final
|
||||||
|
committed aggregate values. A barrier trace is the single visible state
|
||||||
|
commit for all buffered item patches, so showing raw per-item incoming
|
||||||
|
values would hide what actually landed in `RunState.state`.
|
||||||
"""
|
"""
|
||||||
state_fields = workflow.state_schema.field_index()
|
state_fields = workflow.state_schema.field_index()
|
||||||
staged_state = deepcopy(state)
|
staged_state = deepcopy(state)
|
||||||
|
|||||||
@@ -92,6 +92,7 @@ def test_sync_concurrent_foreach_respects_max_active_by_refill_trace() -> None:
|
|||||||
]
|
]
|
||||||
assert loop_entries[0].resolved_input["active_count"] == 0
|
assert loop_entries[0].resolved_input["active_count"] == 0
|
||||||
assert loop_entries[1].resolved_input["active_count"] == 1
|
assert loop_entries[1].resolved_input["active_count"] == 1
|
||||||
|
assert any(entry.resolved_input["active_count"] > 0 for entry in loop_entries)
|
||||||
assert all(entry.resolved_input["active_count"] < 2 for entry in loop_entries)
|
assert all(entry.resolved_input["active_count"] < 2 for entry in loop_entries)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -70,7 +70,13 @@ def test_foreach_barrier_tracks_active_and_outstanding_children() -> None:
|
|||||||
barrier = ForeachBarrierState()
|
barrier = ForeachBarrierState()
|
||||||
|
|
||||||
barrier.start_child("child-0")
|
barrier.start_child("child-0")
|
||||||
|
assert barrier.active_frame_ids == ("child-0",)
|
||||||
|
assert barrier.outstanding_frame_ids == ("child-0",)
|
||||||
|
|
||||||
barrier.start_child("child-1")
|
barrier.start_child("child-1")
|
||||||
|
assert barrier.active_frame_ids == ("child-0", "child-1")
|
||||||
|
assert barrier.outstanding_frame_ids == ("child-0", "child-1")
|
||||||
|
|
||||||
barrier.finish_child("child-0")
|
barrier.finish_child("child-0")
|
||||||
|
|
||||||
assert barrier.active_frame_ids == ("child-1",)
|
assert barrier.active_frame_ids == ("child-1",)
|
||||||
@@ -85,6 +91,23 @@ def test_foreach_barrier_rejects_duplicate_child_start() -> None:
|
|||||||
barrier.start_child("child-0")
|
barrier.start_child("child-0")
|
||||||
|
|
||||||
|
|
||||||
|
def test_foreach_barrier_rejects_finishing_unknown_child() -> None:
|
||||||
|
barrier = ForeachBarrierState()
|
||||||
|
|
||||||
|
with pytest.raises(WorkflowExecutionError, match="not active"):
|
||||||
|
barrier.finish_child("child-0")
|
||||||
|
|
||||||
|
|
||||||
|
def test_foreach_barrier_rejects_duplicate_item_result() -> None:
|
||||||
|
barrier = ForeachBarrierState()
|
||||||
|
patch = StatePatch(changes={"state.count": 1})
|
||||||
|
|
||||||
|
barrier.add_success_patch(index=0, frame_id="child-0", patch=patch)
|
||||||
|
|
||||||
|
with pytest.raises(WorkflowExecutionError, match="already recorded"):
|
||||||
|
barrier.add_success_patch(index=0, frame_id="child-0", patch=patch)
|
||||||
|
|
||||||
|
|
||||||
def test_item_error_record_rejects_negative_index() -> None:
|
def test_item_error_record_rejects_negative_index() -> None:
|
||||||
with pytest.raises(WorkflowExecutionError, match="index"):
|
with pytest.raises(WorkflowExecutionError, match="index"):
|
||||||
ItemErrorRecord.from_metadata(
|
ItemErrorRecord.from_metadata(
|
||||||
|
|||||||
Reference in New Issue
Block a user