fix(langgraph): don't replay an abandoned branch into a DeltaChannel fork - #8548
fix(langgraph): don't replay an abandoned branch into a DeltaChannel fork#8548Elior Nataf Lackritz (eliornl) wants to merge 6 commits into
Conversation
…fork Addressing an older checkpoint creates a fork: the shared base ends up with two children and keeps the checkpoint_writes of the branch the fork abandons. Nothing records which child consumed which write, so the DeltaChannel ancestor walk collected the abandoned branch's writes too. Live execution was correct; only the reconstruction after a reload was wrong, and it was wrong on every saver. Fixed on the write side, so no saver changes are needed. A run launched against an explicitly addressed checkpoint forces every DeltaChannel to snapshot into its first checkpoint, terminating the walk inside the fork instead of at the shared base. This mirrors the existing force-snapshot for Overwrite writes, hence the rename to _delta_channels_forced_snapshot. update_state against an older checkpoint takes the same path, for the same reason is_fresh_thread already does. A channel with no value at the fork base cannot carry a snapshot blob yet, so the request stays queued until the first superstep that gives it one. Cost is one snapshot per addressed run, not per superstep. Fixes #8443 Co-Authored-By: AnnaSuSu <64579968+AnnaSuSu@users.noreply.github.com> Co-Authored-By: UditDewan <194863456+UditDewan@users.noreply.github.com>
perform_superstep returns the config of the checkpoint it just wrote and bulk_update_state feeds that back in, so from the second superstep on the incoming config always names a checkpoint whether or not the caller addressed one. Deriving the fork flag from it made every superstep after the first force-snapshot every available DeltaChannel and reset its cadence, storing the whole growing value once per superstep. Resolve the flag once from the caller's config and pass it explicitly, true only for the first superstep. The clear-tasks recursion carries it through, since the checkpoint written there has no delta snapshot and so leaves a fork unsealed. Caught by the Open SWE review bot on #8548.
The as_node INPUT, END and __copy__ paths write a checkpoint and return before create_checkpoint_plan_for_update_state_api runs, so a bulk update whose first superstep took one of them left the branch unsealed. Only INPUT actually leaked: END absorbs the base's already-run task writes, so its delta and plain channels agree. Sealing on a later superstep does not help. By then that superstep has reconstructed its value by walking through the unsealed checkpoint into the shared base, so it snapshots an already-corrupted list. The fork's first checkpoint is the one that has to carry the blob, which is what create_fork_checkpoint does. That snapshot was still being dropped by put: these paths apply writes to the input channel, not the delta channel, so nothing bumped the delta channel's version and it never entered new_versions. Pass get_next_version for the manual bump, the same reason exit mode needs it, and derive new_versions from the returned checkpoint. fork_pending tracks what is still owed, mirroring _delta_channels_awaiting_fork_snapshot in _loop.py. Caught by the Open SWE review bot on #8548.
|
Reviewed against Your fork seal is a precondition for the mirror fix, not just adjacent to itI built the That move only works because of this PR. Same patch, same two-sided probe, On One trap in it, in case it saves you a roundThe move has to be conditional on Worth noting the deferred case is already covered by this PR and needs nothing extra: an unforked Cost triggerAgree with keeping the trigger coarse. Addressing the latest checkpoint for optimistic concurrency does pay one snapshot, but that checkpoint genuinely can have pending writes from an interrupted run, and a blob you did not need is cheaper than silent corruption. The same reasoning is what makes I have the mirror fix written and verified on top of this branch — sync and async, 26 parametrized cases green across memory/sqlite/postgres, red on |
|
Thanks for digging in, and for testing both directions. The sibling half is what would catch a fix that overshoots. You are right about the ordering and I will fix the PR body. I had it down as adjacent work when it is actually blocked on this landing. I rebuilt your write move locally to check it rather than take it on trust, deferring the On Confirmed the two pieces the reasoning rests on as well. The walk starts at Please open it as your own PR once this merges. You wrote it and verified it, so it should carry your name rather than a trailer in mine. On the gate that closed #8476: you do not need a new issue to get around it. Reading Separately, the empty-channel case still listed under Not covered is a different gap and your patch does not touch it, so that one stays open either way. A delta channel with no value at the fork base cannot carry a snapshot blob, so the boundary goes unrecorded and the walk runs past it. |
MemorySaverAssertImmutable recorded the checkpoint object handed to put, then compared it against one read back through get. Those two are not the same shape: channel_values are stored per (channel, version), so a channel a step did not write is refilled from the blob its inherited version still points at. Every channel except DeltaChannel writes its value into channel_values on every checkpoint, so the two agreed by accident. A DeltaChannel stores nothing except at a snapshot, so once one snapshots and a later step does not write it, the saver reports a checkpoint that changed after it was written when nothing was mutated. Reproducible on main with no fork involved: a delta channel with snapshot_frequency=1 written by the first node and left alone by the next two trips the assertion. Existing delta tests miss it only because they all use snapshot_frequency=1000. Record what the saver reads back instead. Comparing read-back against read-back still catches a checkpoint whose stored data really changed.
A DeltaChannel that was never written on the branch being forked has no value to snapshot and no entry in channel_versions, so create_checkpoint skipped it and the fork's first checkpoint recorded no boundary at all. The walk then ran past the fork into the shared base and collected the abandoned branch's writes, the same failure this branch already fixes for channels that do have a value. Two shapes leaked. A run forking off a checkpoint older than the channel's first value and never writing that channel returned ['in-1'] where the plain-channel oracle returned []. A bulk update writing the delta key only in its second superstep returned ['in-1', 's2'] against ['s2']. No new blob type is needed. _DeltaSnapshot already carries the value and is already serialized by every saver, and from_checkpoint turns MISSING into typ(), so _DeltaSnapshot(typ()) reconstructs to the same empty value the channel would have had. What was missing is a version: without one, put drops the blob as not-a-new-version, so mint a first one. Deferring the seal to a later superstep does not work. That superstep reconstructs through the still-unsealed checkpoint and would only bake the corrupted value into its own snapshot. Checked that minting a version does not fire nodes that subscribe to the channel: a raw Pregel node subscribed directly to the delta channel stays silent across the fork. Reported by the Open SWE review bot on #8548.
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
There was a problem hiding this comment.
Community review, does not clear the merge gate.
This is a large, intricate patch (regression for #8443) touching checkpoint invariants across _checkpoint.py, _loop.py and main.py's bulk_update_state/abulk_update_state. I read the whole diff and the new test suite in tests/test_delta_channel_fork.py.
Core mechanism as I traced it: when a run is addressed at an explicit checkpoint (a fork/time-travel), the fork's shared base still carries the abandoned branch's pending writes, and nothing records which child consumed which write. Previously only is_fresh_thread forced a full delta-channel snapshot to stop the ancestor walk; this adds is_fork for the same reason, snapshotting every delta channel (including unavailable ones, via the new include_unavailable flag) on the fork's first checkpoint so the walk terminates inside the fork instead of reaching the base.
I specifically checked the "several superstep paths write a checkpoint and return before the plan runs" problem the comments call out (as_node of INPUT/END/copy). The new create_fork_checkpoint helper and the fork_pending/_delta_channels_awaiting_fork_snapshot bookkeeping both persist the "still owed a fork snapshot" state across supersteps rather than clearing it after the first one, which is what test_fork_by_bulk_update_whose_first_superstep_skips_the_plan (parametrized over INPUT/END/copy) is checking. I also confirmed the rename _delta_channels_with_overwrite -> _delta_channels_forced_snapshot is applied consistently everywhere it's read or written.
I did not run the test suite myself, and I don't have deep enough working knowledge of DeltaChannel's ancestor-walk/versioning internals to independently re-derive every invariant here from scratch, so I'm reviewing the reasoning and consistency of the diff rather than certifying the whole mechanism. The test coverage looks genuinely thorough: sync/async, all durabilities, fork before any value exists, fork via update_state/bulk_update_state, and explicit guards that an unaddressed run does not change the normal snapshot_frequency cadence (cost containment).
Fixes #8443
Problem
Addressing an older checkpoint creates a fork: the shared base ends up with two children, and it keeps the
checkpoint_writesof the branch the fork abandons. Nothing in the stored data records which child consumed which write, so theDeltaChannelancestor walk inget_delta_channel_historycollects the abandoned branch's writes too.Live execution is correct. Only the reconstruction after a reload is wrong:
fullchannels are unaffected: their checkpoints carry completechannel_valuesand need no replay. In a graph carrying both, the plain channel is right and the delta channel is not, which is what the new tests assert against.This is not one saver's slip. The new test file fails on every backend on
main:memory,memory_migrate_sends,sqlite,sqlite_aes,sqlite_aio, andpostgresin all three pool modes, sync and async.InMemorySaver, the base walk used by SQLite, andPostgresSaver's paged-SQL override are each wrong independently, because write-to-child ownership simply is not recorded.Fix
Write side, so no saver changes are needed. A run launched against an explicitly addressed checkpoint forces every
DeltaChannelto snapshot into its first checkpoint, which terminates the walk inside the fork instead of at the shared base. This mirrors the existing force-snapshot forOverwritewrites, which exists for the same reason; the set that carried those channels now carries both cases, hence its rename to_delta_channels_forced_snapshot._loop.py: queue every delta channel for a forced snapshot when the addressedcheckpoint_idnames a real checkpoint. Read offcheckpoint_configrather thanconfig, so a subgraph resolved through a checkpoint map during time travel is covered too, matching what__enter__already does to decide the same question. Checks the value, not key presence likeis_replaying, because subgraph task configs always carry an explicitNonethere. Drained in_put_checkpointrather than_first, so channels that only get a value later in the run are covered._checkpoint.py:create_checkpoint_plan_for_update_state_api(..., is_fork=True)forces the same full snapshot forupdate_stateagainst an older checkpoint, for the same reasonis_fresh_threadalready does.main.pyreads this offconfig, notcheckpoint_config, because therecheckpoint_confighas already been merged with the loaded checkpoint's own id and would be truthy on every call.main.py: passis_forkthrough bothbulk_update_statepaths. Two details there are load-bearing. The flag cannot be read off the per-superstep config, becauseperform_superstepreturns the config of the checkpoint it just wrote and the driver feeds that back in, so from the second superstep on it always names a checkpoint whether the caller addressed one or not. And the fork has to be sealed by whichever checkpoint the fork writes first: theas_nodeINPUT and END paths write one and return before reaching the plan, socreate_fork_checkpointsnapshots there too. Sealing later is too late, because by then the next superstep has already reconstructed a wrong in-memory value through the shared base and would just bake it into the snapshot.fork_pendingtracks what is still owed, mirroring_delta_channels_awaiting_fork_snapshotin_loop.py.create_fork_checkpointpassesget_next_versionfor the same reason exit mode does: those paths apply writes to the input channel rather than the delta channel, so nothing bumps the delta channel's version andputwould drop the blob as not-a-new-version. Theirnew_versionsis therefore derived from the returned checkpoint rather than the one passed in.A channel with no value at the fork base cannot carry a snapshot blob yet (
create_checkpointskips channels absent fromchannel_versions), so it stays queued until the first superstep that gives it one. Without that, forking off the checkpoint that predates a thread's first input still leaks.Cost, and why the trigger is coarse
One snapshot per addressed run, not per superstep. Measured on a six-node graph: the forked run writes 8 checkpoints and exactly 1 snapshot blob. Runs with no addressed checkpoint are untouched, and
test_unaddressed_run_keeps_snapshot_cadenceguards that.The trigger fires on any truthy
checkpoint_id, including a caller that addresses the thread's latest checkpoint for optimistic concurrency rather than to fork. That caller pays one snapshot per run. I considered narrowing it to "the addressed checkpoint has pending writes for this delta channel", which is where the leak actually comes from and would make that case free, and decided against it for two reasons: it would depend onCheckpointTuple.pending_writesbeing complete, so a saver that leaves them out goes back to leaking silently; and addressing the latest checkpoint genuinely does fork when that checkpoint carries pending writes from an interrupted run. Snapshotting when it was not needed costs a blob; not snapshotting when it was needed is silent corruption. Happy to trade that off differently if you'd rather.Tests
tests/test_delta_channel_fork.py, parametrized over the sharedsync_checkpointer/async_checkpointerfixtures and, for the run paths, over all three durabilities. Every graph carries aDeltaChanneland a plain reducer channel fed the same values, so the plain channel is the oracle: after a fork the two must agree.invokewith new input, sync and asyncupdate_state/aupdate_statebulk_update_statewhose first superstep isas_nodeINPUT, END or__copy__, each of which skips the planbulk_update_statewrites a snapshot blob91 of 133 cases fail on
main, all 133 pass with the fix. Of the 42 that pass either way, 21 are the cadence guards and the rest are the END and__copy__variants, which turn out not to leak: END legitimately absorbs the base's already-run task writes, so the delta and plain channels agree there. They are kept because the oracle is what distinguishes that from a leak, and because both paths now go through the same sealing code.make format,make lint_package,make lint_testsclean. Fulllibs/langgraphsuite against Docker postgres and redis: 3601 passed, 2 failed, bothtests/test_remote_graph.pyconnection errors that reproduce identically on unpatchedmain(they need the dev server, which I did not start).Not covered
update_statepersists its writes against the addressed checkpoint, so on a fork those writes also become visible to the abandoned branch's reads. Verified on this branch: after forking withupdate_state, re-reading the abandoned head returns['in-1', 'first-out', 'patched', 'in-2', 'second-out']while its plain channel correctly returns['in-1', 'first-out', 'in-2', 'second-out']. That is the mirror of this bug rather than this bug, and it predates this change.It is left out here because it is a separate change, not because it is hard: Aari (@AnnaSuSu) has it written and verified on top of this branch, and it is worth saying that it is blocked on this PR rather than merely adjacent to it. The fix moves those writes onto the checkpoint
update_statecreates, which is where the INPUT and PUSH writes inbulk_update_statealready go. That only works if the new checkpoint carries a snapshot blob, becauseget_delta_channel_historystarts its walk attarget_tuple.parent_configand so never collects a checkpoint's own writes for itself.create_fork_checkpointis what now guarantees the blob. Reproduced locally, deferring thechannel_writesloop past theputand targetingnext_config if is_fork else checkpoint_config:On
mainthe fork loses the update that created it. The third row is why the move has to be conditional on the fork flag this PR already threads through.A delta channel with no value at the fork base, that the fork does not write in the same superstep, is still not sealed.
create_checkpointskips channels absent fromchannel_versionsandget()raises on an unavailable channel, so there is no blob to write and the fork boundary goes unrecorded. Two verified cases, both on this branch:This is narrower than the bug being fixed (on
mainevery fork leaks) but it is the same class. Closing it needs the fork boundary represented in the first persisted checkpoint even when the channel has no value, which means either a sentinel for "empty as of here" in the checkpoint format, or synthesising achannel_versionsentry for a never-written channel, which reaches intoversions_seenand task triggering. That felt like a call for a maintainer rather than something to fold in here. Happy to take either direction in this PR or a follow-up.Threads already forked before this change keep the state they have. The ownership information was never recorded, so there is nothing to recover from at read time.
Thanks to Aari (@AnnaSuSu) for the report, the minimal reproduction, and for working out that ownership is unrecoverable at read time and proposing the force-snapshot approach taken here, and to udit (@UditDewan) for the implementation in #8476, including the case where a channel has no value at the fork base and so cannot carry the blob until a later superstep. Both are credited as co-authors on the commit.