Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
# pyright: reportPrivateUsage=false
# Classes in this module (RunContext, StepWrapper, FunctionalWorkflow) form a
# cohesive unit and intentionally access each other's underscore-prefixed members.
import asyncio
import functools
import hashlib
import inspect
Expand Down Expand Up @@ -1023,9 +1024,15 @@ async def _run_core(
# Use a mutable list so the closure can update prev_checkpoint_id
ckpt_chain: list[str | None] = [prev_checkpoint_id]
if storage is not None:
# Concurrent steps (e.g. via asyncio.gather) may complete around
# the same time. Serialize the read-save-update of the chain head
# so each checkpoint links to the previous one instead of creating
# sibling root checkpoints that fork the lineage.
ckpt_chain_lock = asyncio.Lock()

async def _on_step_completed() -> None:
ckpt_chain[0] = await self._save_checkpoint(ctx, storage, ckpt_chain[0])
async with ckpt_chain_lock:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the final and HITL checkpoint saves use the same lock and update ckpt_chain[0] as well? This lock currently serializes only _on_step_completed. A sibling step can still finish while asyncio.gather is unwinding a WorkflowInterrupted, after which the handler at line 1117 calls _save_checkpoint(ctx, storage, ckpt_chain[0]) outside the lock. I reproduced this on f68cb2b with a storage barrier that holds the first save until the concurrent second save enters: the step-completion save and HITL save both receive previous_checkpoint_id=None, producing 2 checkpoints with 2 roots. The clean-completion save at line 1082 has the same bypass. Routing every read/save/update of ckpt_chain[0] through one locked helper, and extending the regression to gather a completed sibling with request_info(), would close the remaining race.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the current change is incomplete while the final and HITL checkpoint paths can still bypass the lock and fork the lineage. We should route all three checkpoint paths through the same locked read/save/update helper and cover the request_info() case before merging.

ckpt_chain[0] = await self._save_checkpoint(ctx, storage, ckpt_chain[0])

ctx._on_step_completed = _on_step_completed

Expand Down
63 changes: 63 additions & 0 deletions python/packages/core/tests/workflow/test_functional_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
InMemoryCheckpointStorage,
RunContext,
StepWrapper,
WorkflowCheckpoint,
WorkflowEvent,
WorkflowRunResult,
WorkflowRunState,
Expand Down Expand Up @@ -68,6 +69,21 @@ def decorate(fn: Callable[..., Awaitable[Any]]) -> FunctionalWorkflow:
return decorate(func) if func is not None else decorate


class _YieldingCheckpointStorage(InMemoryCheckpointStorage):
"""In-memory checkpoint storage whose ``save()`` yields to the event loop.

Real backends (files, databases) suspend while persisting, which lets two
concurrent per-step checkpoint saves interleave. The plain in-memory
implementation never suspends, so it cannot exercise the race this file's
parallel-checkpoint regression test guards against. Yielding here
reproduces that interleaving deterministically.
"""

async def save(self, checkpoint: WorkflowCheckpoint) -> str:
await asyncio.sleep(0)
return await super().save(checkpoint)


@step
async def add_one(x: int) -> int:
return x + 1
Expand Down Expand Up @@ -797,6 +813,53 @@ async def wf(x: int) -> int:
checkpoints = await storage.list_checkpoints(workflow_name="wf")
assert len(checkpoints) == 3 # 2 from first run + 1 final from restore

async def test_parallel_steps_keep_single_checkpoint_lineage(self):
"""Concurrent step completions must not fork the checkpoint chain.

When steps run via ``asyncio.gather``, their completion callbacks can
overlap: both read the same ``previous_checkpoint_id`` before either
writes the updated chain head back, creating sibling root checkpoints
that leave part of the history unreachable from the latest checkpoint.

A storage whose ``save()`` yields to the event loop reproduces the
interleaving a real (file/database) backend would see, deterministically.
"""
storage = _YieldingCheckpointStorage()

@step
async def left(value: int) -> int:
return value + 1

@step
async def right(value: int) -> int:
return value + 2

@built_workflow(checkpoint_storage=storage)
async def parallel(value: int) -> list[int]:
return await asyncio.gather(left(value), right(value))

result = await parallel.run(1)
assert result.get_outputs() == [[2, 3]]

checkpoints = await storage.list_checkpoints(workflow_name="parallel")
by_id = {cp.checkpoint_id: cp for cp in checkpoints}

# Both steps plus the final save.
assert len(checkpoints) == 3

# Exactly one root: the chain head is read and updated under a lock.
roots = [cp for cp in checkpoints if cp.previous_checkpoint_id is None]
assert len(roots) == 1

# Every checkpoint is reachable from the latest one - no forked lineage.
latest = await storage.get_latest(workflow_name="parallel")
reachable: set[str] = set()
cursor = latest
while cursor is not None:
reachable.add(cursor.checkpoint_id)
cursor = by_id.get(cursor.previous_checkpoint_id) if cursor.previous_checkpoint_id else None
assert reachable == set(by_id)


# ---------------------------------------------------------------------------
# Branching / control flow
Expand Down
Loading