diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index d17513cc0b..a50a676f3c 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -17,6 +17,7 @@ import tempfile import threading import uuid +from collections import ChainMap from concurrent.futures import Future, ThreadPoolExecutor from datetime import datetime, timezone from pathlib import Path @@ -891,6 +892,76 @@ def append_log(self, entry: dict[str, Any]) -> None: f.write(json.dumps(entry) + "\n") +# Nested step keys that may contain a list of steps, mirroring +# ``overlays/merge.py``'s ``_NESTED_LIST_KEYS`` (this module cannot import +# that one without a circular import: ``overlays`` imports ``WorkflowDefinition`` +# from here). +_NESTED_STEP_LIST_KEYS = ("then", "else", "steps", "default") + + +def _rename_step_tree_ids( + step: dict[str, Any], prefix: str, suffix: str, *, default_id: str | None = None +) -> tuple[dict[str, Any], dict[str, str]]: + """Return a copy of *step* with every id in its subtree rewritten to + ``f"{prefix}:{orig_id}:{suffix}"``, plus a ``{new_id: original_id}`` map. + + A loop iteration or fan-out item previously renamed only the id of the + step it iterates over directly (the immediate loop-body/fan-out-template + step). A step nested one level deeper — e.g. a ``shell`` step inside an + ``if`` inside a ``while`` body or fan-out ``step:`` template — kept its + bare, unnamespaced id across every iteration/item, so each iteration/item + silently overwrote the previous one's entry in ``context.steps`` / + ``state.step_results`` under that same key: only the last iteration's or + item's result for that nested step ever survived. + + Recurses into ``then``, ``else``, ``steps``, ``default``, and ``cases.*`` + — the same nesting keys ``overlays/merge.py`` walks for step-tree + attribution — so every descendant gets a unique id, not just the direct + child. ``default_id`` supplies the fallback used only when the top-level + *step* itself has no ``id`` (mirroring each caller's own historical + fallback, e.g. fan-out's ``template.get("id", "item")``); a validated + workflow requires an id on every nested step, so nested frames that lack + one are left unrenamed rather than guessing a name. + """ + new_step = dict(step) + id_map: dict[str, str] = {} + orig_id = new_step.get("id") or default_id + if isinstance(orig_id, str): + new_id = f"{prefix}:{orig_id}:{suffix}" + new_step["id"] = new_id + id_map[new_id] = orig_id + for key in _NESTED_STEP_LIST_KEYS: + nested = new_step.get(key) + if isinstance(nested, list): + renamed_list = [] + for child in nested: + if isinstance(child, dict): + new_child, child_map = _rename_step_tree_ids(child, prefix, suffix) + renamed_list.append(new_child) + id_map.update(child_map) + else: + renamed_list.append(child) + new_step[key] = renamed_list + cases = new_step.get("cases") + if isinstance(cases, dict): + new_cases = {} + for case_key, case_steps in cases.items(): + if isinstance(case_steps, list): + renamed_cases = [] + for child in case_steps: + if isinstance(child, dict): + new_child, child_map = _rename_step_tree_ids(child, prefix, suffix) + renamed_cases.append(new_child) + id_map.update(child_map) + else: + renamed_cases.append(child) + new_cases[case_key] = renamed_cases + else: + new_cases[case_key] = case_steps + new_step["cases"] = new_cases + return new_step, id_map + + # -- Workflow Engine ------------------------------------------------------ @@ -1175,8 +1246,27 @@ def _execute_steps( registry: dict[str, Any], *, step_offset: int = 0, + alias_map: dict[str, str] | None = None, + alias_local_only: bool = False, ) -> None: - """Execute a list of steps sequentially.""" + """Execute a list of steps sequentially. + + ``alias_map`` (``{namespaced_id: original_id}``, from + ``_rename_step_tree_ids``) mirrors each recorded step's result to its + original, unprefixed id *immediately* after that step finishes -- not + after its whole subtree finishes -- so a sibling step later in the + same loop iteration / fan-out item that references an earlier + sibling by its original id (``steps.``) sees that value right + away. It is propagated through the recursive nested-step call below + so descendants nested arbitrarily deep (e.g. an ``if`` inside the + namespaced step) are aliased too, not just the immediate child. + + ``alias_local_only`` routes that mirror through ``context.steps`` + only, never ``state.step_results``. A concurrent fan-out item passes + a private overlay as ``context.steps`` and this flag so concurrently + running items never race to write the same original id in shared + state; see ``_run_fan_out``. + """ for i, step_config in enumerate(steps): step_id = step_config.get("id", f"step-{i}") step_type = step_config.get("type", "command") @@ -1231,6 +1321,13 @@ def _execute_steps( "error": result.error, } self._record_result(context, state, step_id, step_data) + if alias_map is not None: + orig_id = alias_map.get(step_id) + if orig_id is not None: + if alias_local_only: + context.steps[orig_id] = step_data + else: + self._record_result(context, state, orig_id, step_data) state.append_log( { @@ -1315,18 +1412,14 @@ def _execute_steps( # A step-path stack for exact nested resume is a future # enhancement. if result.next_steps: - self._execute_steps( - result.next_steps, context, state, registry, - step_offset=-1, - ) - if state.status in ( - RunStatus.PAUSED, - RunStatus.FAILED, - RunStatus.ABORTED, - ): - return - - # Loop iteration: while/do-while re-evaluate after body + # Loop iteration: while/do-while re-evaluate after body. Every + # iteration -- including the first -- is namespaced and run + # through the same _rename_step_tree_ids + alias_map path, so + # each has its own state.step_results entry (see + # _rename_step_tree_ids). Previously only iterations after the + # first were namespaced: the first ran with bare ids and no + # dedicated entry, and iteration 1's aliasing then silently + # overwrote it, making iteration 0's result unrecoverable. if step_type in ("while", "do-while"): from .expressions import evaluate_condition @@ -1343,23 +1436,30 @@ def _execute_steps( ): max_iters = 10 condition = step_config.get("condition", False) - for _loop_iter in range(max_iters - 1): - if not evaluate_condition(condition, context): + for _loop_iter in range(max_iters): + if _loop_iter > 0 and not evaluate_condition( + condition, context + ): break - # Namespace nested step IDs per iteration - # so logs and state keys are unique. - # Execute one step at a time and alias each - # result back to the unprefixed key so that - # later steps in the same body and the loop - # condition see the latest values. + # Namespace nested step IDs (recursively, including + # descendants nested inside e.g. an 'if' in the loop + # body — see _rename_step_tree_ids) per iteration so + # logs and state keys are unique. Execute one step at + # a time; alias_map aliases each renamed id in the + # subtree back to its original, unprefixed id + # immediately as that step completes (not after the + # whole iteration finishes), so later steps in the + # same body and the loop condition see the latest + # values. for ns_idx, ns in enumerate(result.next_steps): - ns_copy = dict(ns) - orig = ns_copy.get("id") - base_id = orig or f"step-{ns_idx}" - ns_copy["id"] = f"{step_id}:{base_id}:{_loop_iter + 1}" + ns_copy, id_map = _rename_step_tree_ids( + ns, step_id, str(_loop_iter), + default_id=f"step-{ns_idx}", + ) self._execute_steps( [ns_copy], context, state, registry, - step_offset=-1, + step_offset=-1, alias_map=id_map, + alias_local_only=alias_local_only, ) if state.status in ( RunStatus.PAUSED, @@ -1367,11 +1467,18 @@ def _execute_steps( RunStatus.ABORTED, ): return - if orig and ns_copy["id"] in context.steps: - self._record_result( - context, state, orig, - context.steps[ns_copy["id"]], - ) + else: + self._execute_steps( + result.next_steps, context, state, registry, + step_offset=-1, alias_map=alias_map, + alias_local_only=alias_local_only, + ) + if state.status in ( + RunStatus.PAUSED, + RunStatus.FAILED, + RunStatus.ABORTED, + ): + return # Fan-out: execute the nested step template once per item. Honors # max_concurrency — <=1 runs sequentially (default, historical @@ -1457,18 +1564,65 @@ def item_id(idx: int) -> str: # Per-item ID grammar: parentId:templateId:index. return f"{step_id}:{base_id}:{idx}" - def run_item(idx: int, item_ctx: StepContext) -> Any: - item_step = dict(template) - item_step["id"] = item_id(idx) - self._execute_steps( - [item_step], item_ctx, state, registry, step_offset=-1, + def run_item( + idx: int, item_ctx: StepContext, *, local_only: bool + ) -> tuple[Any, dict[str, dict[str, Any]]]: + # Namespace every id in the template's subtree (not just the + # template's own top-level id) so a step nested inside e.g. an + # 'if'/'switch' branch of the fan-out template gets a unique key + # per item instead of colliding across items — and, more + # seriously, potentially colliding with an unrelated step of the + # same id elsewhere in the workflow (fan-out templates are + # exempted from the global id-uniqueness check specifically + # because runtime namespacing was assumed to make collisions + # safe; see _rename_step_tree_ids). + item_step, id_map = _rename_step_tree_ids( + template, step_id, str(idx), default_id=base_id, ) - # Read back through the context that was actually executed against, - # not the outer closure — clearer and robust if StepContext copying - # ever stops sharing the steps dict by reference. - return item_ctx.steps.get(item_step["id"], {}).get("output", {}) - - # Sequential path — identical to the historical behavior. + # ``local_only`` (concurrent path): give this item a private + # overlay for its ``.steps`` reads/writes. Namespaced results + # still land in the real ``state.step_results`` (via + # _record_result's unconditional write — see _execute_steps), + # but the immediate bare-id alias (see alias_map below) writes + # only into this overlay. That lets a later sibling step in + # THIS item's template resolve an earlier sibling by its + # original id via the overlay, without ever mutating the + # shared steps dict that other concurrently-running items also + # read from — the actual race Copilot flagged: every worker + # writing the same bare-id key could otherwise expose another + # item's value to a sibling read. The caller applies exactly + # one item's aliases to shared state — deterministically the + # last item in item order — once every item has finished. + original_steps = item_ctx.steps + local_overlay: dict[str, dict[str, Any]] = {} + if local_only: + item_ctx.steps = ChainMap(local_overlay, original_steps) + try: + self._execute_steps( + [item_step], item_ctx, state, registry, step_offset=-1, + alias_map=id_map, alias_local_only=local_only, + ) + finally: + item_ctx.steps = original_steps + alias_records: dict[str, dict[str, Any]] = {} + steps_view = local_overlay if local_only else item_ctx.steps + for new_id, orig_id in id_map.items(): + if new_id in steps_view: + data = steps_view[new_id] + if local_only: + # Publish the namespaced (disjoint, per-item) result + # into the truly-shared steps dict explicitly — safe + # even under concurrency since each item only ever + # writes its own namespaced keys here. + original_steps[new_id] = data + alias_records[orig_id] = data + # Read back through the local view, not the outer closure — + # clearer and robust if StepContext copying ever stops sharing + # the steps dict by reference. + return steps_view.get(item_step["id"], {}).get("output", {}), alias_records + + # Sequential path — identical to the historical behavior, plus + # immediate (not post-subtree) bare-id aliasing. if workers <= 1: results: list[Any] = [] previous_item = context.item @@ -1477,7 +1631,10 @@ def run_item(idx: int, item_ctx: StepContext) -> Any: try: for item_idx, item_val in enumerate(items): context.item = item_val - results.append(run_item(item_idx, context)) + output, _alias_records = run_item( + item_idx, context, local_only=False + ) + results.append(output) if state.status in halting: break finally: @@ -1488,11 +1645,13 @@ def run_item(idx: int, item_ctx: StepContext) -> Any: # Concurrent path — bounded sliding window; results assembled in item order. n = len(items) slots: list[Any] = [None] * n + alias_slots: list[dict[str, dict[str, Any]]] = [{}] * n - def run_isolated(idx: int) -> Any: + def run_isolated(idx: int) -> tuple[Any, dict[str, dict[str, Any]]]: # Each item runs against its own context copy so context.item is not - # clobbered across threads; the shared steps dict is written only on the - # disjoint parentId:templateId:index key (GIL-safe on distinct keys). + # clobbered across threads; local_only=True gives it a private + # steps overlay so its immediate bare-id aliases cannot race a + # concurrently-running sibling item's aliases (see run_item). return run_item( idx, dataclasses.replace( @@ -1500,6 +1659,7 @@ def run_isolated(idx: int) -> Any: item=items[idx], inside_fan_out=True, ), + local_only=True, ) def item_halt_status(idx: int) -> RunStatus | None: @@ -1509,7 +1669,13 @@ def item_halt_status(idx: int) -> RunStatus | None: # misattributed here. Mirrors the sequential mapping: PAUSED -> PAUSED; # FAILED -> ABORTED when aborted, else FAILED, unless continue_on_error # routes around it. - rec = context.steps.get(item_id(idx)) + # Reads from state.step_results (not context.steps): a concurrent + # item's steps overlay is private (see run_item), so + # context.steps is no longer guaranteed to carry this item's + # namespaced entry, while state.step_results always does — every + # namespaced write reaches it unconditionally regardless of the + # overlay. + rec = state.step_results.get(item_id(idx)) if rec is None: # Ran but recorded nothing — only when the item failed before # record_step_result (e.g. an unknown step type returns early). @@ -1554,7 +1720,7 @@ def item_halt_status(idx: int) -> RunStatus | None: # change ever breaks that invariant. break try: - slots[idx] = fut.result() + slots[idx], alias_slots[idx] = fut.result() except Exception: # A genuine exception escaping a step (not a normal step # FAILED, which sets state.status) must not be masked: cancel @@ -1575,6 +1741,16 @@ def item_halt_status(idx: int) -> RunStatus | None: other.cancel() break + # Apply exactly one item's bare-id aliases to the real shared state — + # deterministically the last item in item order (the halting item, if + # any, else the last one collected) — now that the pool has joined + # and this runs single-threaded again, so it can never race a + # concurrently-running item the way writing it during run_item would. + last_idx = halt[0] if halt is not None else (collected - 1 if collected else None) + if last_idx is not None: + for orig_id, data in alias_slots[last_idx].items(): + self._record_result(context, state, orig_id, data) + if halt is not None: halted_at, halted_status = halt # A later in-flight item may have overwritten state.status before the @@ -1588,7 +1764,7 @@ def item_halt_status(idx: int) -> RunStatus | None: # third-party step returning FAILED with no message never inherits # an unrelated concurrent item's error; this mirrors the sequential # path, which sets state.error = result.error verbatim. - halt_rec = context.steps.get(item_id(halted_at)) + halt_rec = state.step_results.get(item_id(halted_at)) if isinstance(halt_rec, dict): state.error = halt_rec.get("error") return slots[: halted_at + 1] diff --git a/tests/test_workflows.py b/tests/test_workflows.py index d599f3c6a4..8c9135419f 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -6298,6 +6298,245 @@ def test_loop_with_bool_max_iterations_uses_default_cap(self, project_dir): # Falls back to the default cap of 10, not range(True - 1) == 1 run. assert counter_file.read_text(encoding="utf-8").strip() == "10" + def test_while_loop_namespaces_nested_descendant_steps(self, project_dir): + """A step nested one level deeper than the loop body's direct child + (e.g. a `shell` step inside an `if` inside the `while` body) must get + a unique namespaced key per iteration, not just the immediate child. + + Previously only the direct child's id was namespaced + (`retry-loop:guard:1`); the grandchild `leaf` kept its bare id across + every iteration, so each iteration silently overwrote the previous + one's entry in `state.step_results["leaf"]` and no per-iteration + record of it ever existed. + """ + from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition + from specify_cli.workflows.base import RunStatus + + yaml_str = """ +schema_version: "1.0" +workflow: + id: "while-nested-descendant" + name: "While Nested Descendant" + version: "1.0.0" +steps: + - id: retry-loop + type: while + condition: "true" + max_iterations: 3 + steps: + - id: guard + type: if + condition: "true" + then: + - id: leaf + type: shell + run: "echo tick" +""" + definition = WorkflowDefinition.from_string(yaml_str) + engine = WorkflowEngine(project_dir) + state = engine.execute(definition) + + assert state.status == RunStatus.COMPLETED + # The unprefixed key still holds the latest iteration's result + # (sibling steps in the loop body and the loop condition read it). + assert state.step_results["leaf"]["output"]["stdout"] == "tick\n" + # Every iteration's grandchild result is separately recoverable -- + # including the FIRST iteration. The first iteration previously ran + # through a separate, unnamespaced code path before the loop-specific + # namespacing logic was reached, so it had no dedicated entry and was + # immediately overwritten by iteration 1's aliasing the moment that + # iteration ran. + assert "retry-loop:leaf:0" in state.step_results + assert "retry-loop:leaf:1" in state.step_results + assert "retry-loop:leaf:2" in state.step_results + + def test_while_loop_sibling_step_sees_immediate_alias(self, tmp_path): + """A step nested inside an `if` in a `while` body that references an + earlier SIBLING nested in the SAME `if` branch by its bare id must + see that sibling's value from the SAME iteration -- not a stale + value left over from a previous iteration. + + Aliasing a namespaced descendant back to its bare id previously + happened only after the entire renamed subtree (here, the whole + `if` step, both its own id and its branch's) finished executing -- + so a later sibling in the same branch that read the earlier one by + its bare id ran before that iteration's alias was ever written, and + so saw the previous iteration's aliased value instead. + """ + from specify_cli.workflows.base import ( + RunStatus, + StepBase, + StepContext, + StepResult, + StepStatus, + ) + from specify_cli.workflows.engine import RunState, WorkflowEngine + from specify_cli.workflows.steps.if_then import IfThenStep + from specify_cli.workflows.steps.while_loop import WhileStep + + call_count = {"n": 0} + + class _WriteStep(StepBase): + type_key = "write" + + def execute(self, config, context): + n = call_count["n"] + call_count["n"] += 1 + return StepResult( + status=StepStatus.COMPLETED, output={"marker": f"value-{n}"} + ) + + class _ReadStep(StepBase): + type_key = "read" + + def execute(self, config, context): + seen = context.steps.get("first", {}).get("output", {}).get("marker") + return StepResult(status=StepStatus.COMPLETED, output={"seen": seen}) + + engine = WorkflowEngine(project_root=tmp_path) + context = StepContext() + state = RunState(run_id="r", workflow_id="w", project_root=tmp_path) + state.status = RunStatus.RUNNING + registry = { + "while": WhileStep(), + "if": IfThenStep(), + "write": _WriteStep(), + "read": _ReadStep(), + } + steps = [ + { + "id": "retry-loop", + "type": "while", + "condition": "true", + "max_iterations": 2, + "steps": [ + { + "id": "guard", + "type": "if", + "condition": "true", + "then": [ + {"id": "first", "type": "write"}, + {"id": "second", "type": "read"}, + ], + }, + ], + }, + ] + engine._execute_steps(steps, context, state, registry) + + assert state.status == RunStatus.RUNNING + assert state.step_results["retry-loop:second:0"]["output"]["seen"] == "value-0" + assert state.step_results["retry-loop:second:1"]["output"]["seen"] == "value-1" + + def test_fan_out_concurrent_sibling_step_isolated_per_item(self, tmp_path): + """A later sibling step in a CONCURRENT fan-out item's template that + references an earlier sibling by its bare id must see THIS item's + value -- not a stale value, and not a value written by a DIFFERENT, + concurrently-running item through the same shared bare-id key. + + A barrier forces every item's first sibling to complete at roughly + the same time, maximizing the window for a racy implementation to + leak one item's value onto another's read of the shared bare-id key. + """ + import threading + + from specify_cli.workflows.base import ( + RunStatus, + StepBase, + StepContext, + StepResult, + StepStatus, + ) + from specify_cli.workflows.engine import RunState, WorkflowEngine + from specify_cli.workflows.steps.if_then import IfThenStep + + n = 4 + barrier = threading.Barrier(n, timeout=5) + + class _WriteStep(StepBase): + type_key = "write" + + def execute(self, config, context): + barrier.wait() + return StepResult( + status=StepStatus.COMPLETED, output={"marker": context.item} + ) + + class _ReadStep(StepBase): + type_key = "read" + + def execute(self, config, context): + seen = context.steps.get("first", {}).get("output", {}).get("marker") + return StepResult(status=StepStatus.COMPLETED, output={"seen": seen}) + + engine = WorkflowEngine(project_root=tmp_path) + context = StepContext() + state = RunState(run_id="r", workflow_id="w", project_root=tmp_path) + state.status = RunStatus.RUNNING + registry = {"if": IfThenStep(), "write": _WriteStep(), "read": _ReadStep()} + template = { + "id": "item", + "type": "if", + "condition": "true", + "then": [ + {"id": "first", "type": "write"}, + {"id": "second", "type": "read"}, + ], + } + items = list(range(n)) + engine._run_fan_out(items, template, "fan", context, state, registry, n) + + for i in items: + assert state.step_results[f"fan:second:{i}"]["output"]["seen"] == i + + def test_fan_out_namespaces_nested_descendant_steps(self, project_dir): + """A step nested inside a fan-out template's `if`/`switch` branch + must get a unique namespaced key per item, not just the template's + own top-level id. + + Previously only the template's own id was namespaced + (`fan:item:0`); a grandchild step like `leaf` kept its bare id + across every item, so each item silently overwrote the previous + item's entry in `state.step_results["leaf"]` — losing every item's + nested result except the last. Nested/template step ids are exempt + from the workflow's global id-uniqueness validation specifically + because runtime namespacing is assumed to make collisions safe, so + an unnamespaced grandchild id can also collide with an unrelated + step of the same id elsewhere in the workflow. + """ + from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition + from specify_cli.workflows.base import RunStatus + + yaml_str = """ +schema_version: "1.0" +workflow: + id: "fan-out-nested-descendant" + name: "Fan Out Nested Descendant" + version: "1.0.0" +steps: + - id: fan + type: fan-out + items: "{{ ['a', 'b', 'c'] }}" + max_concurrency: 1 + step: + id: item + type: if + condition: "true" + then: + - id: leaf + type: shell + run: "echo {{ item }}" +""" + definition = WorkflowDefinition.from_string(yaml_str) + engine = WorkflowEngine(project_dir) + state = engine.execute(definition) + + assert state.status == RunStatus.COMPLETED + # Every item's grandchild result is separately recoverable. + assert state.step_results["fan:leaf:0"]["output"]["stdout"] == "a\n" + assert state.step_results["fan:leaf:1"]["output"]["stdout"] == "b\n" + assert state.step_results["fan:leaf:2"]["output"]["stdout"] == "c\n" + def test_do_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): """Do-while loop must still run to max_iterations when the condition never becomes false.