fix: order delta channel replay by task path - #8544
fix: order delta channel replay by task path#8544Elior Nataf Lackritz (eliornl) wants to merge 2 commits into
Conversation
DeltaChannel reconstructs its value by replaying ancestor writes through the reducer. Every saver ordered a checkpoint's writes by (task_id, idx), but live execution applies them in task-path order: apply_writes sorts a super-step's tasks by task_path_str(task.path[:3]) before calling channel.update. task_id is a hash of the path, so the two orders are unrelated, and two or more tasks writing one DeltaChannel in a single super-step replayed in an arbitrary permutation. Reducers are only required to be batching-invariant, not order-invariant, so the permutation changes the value: get_state disagreed with what invoke returned, and continuing the thread persisted the reordered replay as the base for later writes. Replay now orders by (task_path, task_id, idx), following the precedent already set for the Send channel by SELECT_PENDING_SENDS_SQL. InMemorySaver and the postgres savers already persisted task_path and only needed the sort key; sqlite accepted task_path on put_writes and dropped it, so the writes table gains the column plus a probe-guarded ALTER for databases created by earlier versions. Writes stored without a task_path sort first within their checkpoint, which is where live execution applies the task-less input writes that carry "". The graph-level regression tests run against the full async_checkpointer matrix (memory, sqlite, postgres in three pool modes) because each saver reconstructs delta channels through its own override; 20 of the 25 cases fail on main, and the 5 that pass are the sequential control. Co-authored-by: ErenAta16 <149434812+ErenAta16@users.noreply.github.com> Co-authored-by: ragnarok268 <58264829+ragnarok268@users.noreply.github.com>
ErenAta16 (ErenAta16)
left a comment
There was a problem hiding this comment.
I filed #8382, so not a neutral reviewer. Checked the premise against _algo.py rather than the description, and both halves hold.
Live execution really does sort by path, not by id (_algo.py:255-256):
# (we use them for eg. task ids which aren't good for sorting)
tasks = sorted(tasks, key=lambda t: task_path_str(t.path[:3]))That comment is the strongest argument in the PR and it is already in the tree. The codebase knew task ids are not sortable, apply_writes acted on it, and the delta-channel replay did not.
And the two orders genuinely disagree, because task_id is a digest of the path rather than a transform of it (_algo.py:834-842):
task_id = task_id_func( # _xxhash_str, or _uuid5_str for v1 checkpoints
checkpoint_id_bytes, checkpoint_ns, str(step),
PUSH, task_path_str(task_path[1]), str(task_path[2]),
)so sorting by task_id sorts by hash output. It is not "a different but stable order", it is unrelated to the order the reducer was fed during execution, which is what makes batching-invariance insufficient.
The tests are the part I would call out. They cannot pass by coincidence:
The two task_ids are assigned so they sort in the opposite order from their task_paths. A saver ordering by
(task_id, idx)therefore returns these writes reversed, rather than passing by happening to agree.
That is the difference between a test that pins the fix and one that pins whatever the fixture happened to produce. Pinning the constants so the fixture's own uuid4 always sorts between them (aaaa… / zzzz…, since uuid hex digits are all <= f) removes the last source of flake, and it is the kind of thing that is obvious only after it bites someone.
Putting it in checkpoint-conformance rather than in one saver's test file is also the right call, since the bug is a contract violation and every backend has to satisfy it, not just the one that got patched.
I also verified the ordering claim the second test rests on: task_path_str prefixes tuples with ~ (_algo.py:1414-1415), and "" precedes ~, so pathless writes really do sort ahead of every path-carrying one, which is where live execution applies graph input.
One question about historical data, and it is the only thing I would want answered before this merges. Rows written before the task_path column existed get "", which means they all sort first, among themselves by task_id. For a thread checkpointed by an older version, replay therefore keeps the old, wrong order for those writes and only becomes correct for writes recorded after the upgrade.
That may well be the right call, since there is no way to recover a path that was never stored, and inventing one would be worse. But it means a continued thread can replay half in the old order and half in the new, and the failure mode I described in #8382 (the reordered replay becoming the base that later writes build on) still applies to that prefix. If that is the intended semantics, it is worth a sentence in the migration notes, because "upgrade fixes it" and "upgrade fixes it for new writes" are very different promises for anyone with long-lived threads.
Nothing else from me. The diagnosis matches what I reported and the fix is at the right layer.
Fixes #8382
DeltaChannelrebuilds its value by replaying ancestor writes through the reducer. Every saver ordered a checkpoint's writes by(task_id, idx), but live execution applies them in task-path order:apply_writessorts a super-step's tasks bytask_path_str(task.path[:3])before callingchannel.update.task_idis a hash of the path, so the two orders are unrelated, and two or more tasks writing oneDeltaChannelin a single super-step replayed in an arbitrary permutation.Reducers are only required to be batching-invariant, not order-invariant, so that permutation changes the value.
get_statedisagreed with whatinvokehad returned, and continuing the thread persisted the reordered replay as the base for later writes.Replay now orders by
(task_path, task_id, idx), the same orderingSELECT_PENDING_SENDS_SQLalready uses for the Send channel.Per-backend
InMemorySavertask_pathtask_pathtask_pathin the stage-2 selecttask_pathonput_writesand dropped itwritesgains the column, plus a probe-guardedALTERfor existing databasesWrites stored without a
task_pathsort first within their checkpoint. That is not only a legacy case: a task-less write (graph input) persists""today, and first is where live execution applies it.Compatibility
Worth calling out because it is asymmetric and not visible in the diff:
task_pathwas already on disk, so old threads replay in the corrected order after upgrade''and sort equal, so old threads keep their existing orderThat is the intended outcome. Sqlite genuinely has no recoverable order for those rows, so nothing is reinterpreted under a rule that never applied to them. New sqlite writes are ordered correctly from the migration onward.
Downgrade is safe in both directions: older code's
INSERTomits the column and it carries aDEFAULT.Sqlite has no
ADD COLUMN IF NOT EXISTS, sosetup()probespragma_table_infofirst and treatsduplicate column nameas success, since two connections opening the same file can both pass the probe and only one can win theALTER.How it was verified
Against
origin/mainin a clean worktree, copying only the new test files in:tests/test_delta_channel_parallel_order.pyruns on the fullasync_checkpointermatrix (memory, sqlite, postgres in three pool modes). 20 of 25 cases fail on main; the 5 that pass are the sequential control, which is there to localise this to parallel writers rather than to delta replay in general.task_pathschema: the column is added, old rows backfill to'',setup()is repeatable, and losing the migration race no longer raises.Suites run locally with Docker:
checkpoint156,checkpoint-sqlite124,checkpoint-postgres264,langgraph3493,prebuilt228 (downstream). The only failures are the twotest_remote_graphcases that need a live server and fail identically on main.Notes for review
The pre-existing function-level imports in
test_delta_channel_history.pyare left alone here; they belong with the PLC0415 rollout rather than this fix.Thanks to ErenAta16 (@ErenAta16) for the report, the reproduction, and for tracing
task_paththrough all three backends' storage, and to Brandon (@ragnarok268) for the conformance-test design that assigns task ids in the opposite order to their task paths so the test cannot pass by coincidence.