fix(langgraph): hydrate subgraph delta channels with the caller-resolved saver - #8538
fix(langgraph): hydrate subgraph delta channels with the caller-resolved saver#8538Elior Nataf Lackritz (eliornl) wants to merge 3 commits into
Conversation
`_prepare_state_snapshot` and `bulk_update_state` hydrated channels with `self.checkpointer`, which is `None` for a subgraph — it borrows the parent's saver through `CONFIG_KEY_CHECKPOINTER` at read time. Without a saver a `DeltaChannel` cannot walk ancestors to replay its writes, so it fell through to `from_checkpoint(MISSING)` and hydrated empty, silently. The callers already resolve the right saver a few lines above every call site. Pass it in, as a required keyword argument so no future call site can drop it. On the write path the empty value was persisted as a `_DeltaSnapshot` whenever the update reached the channel's snapshot cadence, losing history on disk. Fixes #8470 Co-authored-by: gururafiki <22777967+gururafiki@users.noreply.github.com> Co-authored-by: Yuan Gao <119447586+DavidGao520@users.noreply.github.com>
Sydney Runkle (sydney-runkle)
left a comment
There was a problem hiding this comment.
a few questions -- can we find any related bugs in deepagents?
is a checkpointer always none for subgraph? i think there are many options for how we can add: https://docs.langchain.com/oss/python/langgraph/use-subgraphs#subgraph-persistence...
|
basically i'm not actually sure this is incorrect, i think it might just be a feature of how subgraphs manage state, worth reading the above |
|
Good questions, and the doc changed my framing, so let me redo this properly. On whether it's a feature of subgraph state. I went through the three modes the doc describes, using the read path it documents ( Stateless ( The two that concern me are per-invocation and per-thread, during an interrupt. Both give: Same call, same instant, same state object. A subgraph-state rule would empty both. It's only the delta channel because that's the one type which stores no value in On whether a checkpointer is always On deepagents. I cloned it at 0.7.4, built a parent with one subagent using their own fake chat model from the unit tests so there's no network, ran a turn, and dumped every namespace. Subagents do checkpoint under But nothing that ships reaches that read. A subagent lives in the Related issues point the same way. #4818 is a user doing exactly that read, closed by mdrxy with "the Where I land. The documented limitation covers subgraphs reached through indirection, and its symptom is a hard error. This PR's case is a subgraph added as a node, which the docs list as supported, where you get a well-formed snapshot with one channel silently empty next to a correct one. Those seem like different things to me. I also couldn't find anywhere we'd decided the empty case is fine: no test in So I'd still merge, but deepagents isn't the argument for it. If that were the only case I'd close this. What keeps me on it is the node-added subgraph plus the write path, where |
Adds controls for the two cases where an empty subgraph read is correct: a `checkpointer=False` subgraph persists nothing, and a completed subgraph exposes no task state through `subgraphs=True`. Both pass before and after the fix, so the hydration change is pinned to the cases it should affect.
|
Hi! Thanks for opening PR and for review. I have experienced related problem in deepagents. Here is the opened issue for it: langchain-ai/deepagents#5136 |
|
Thanks, and good to see #5136 written up. Worth separating the two though, since they look alike from the outside. #5136 is the tool-indirection case: the subagent lives in the This PR is the case where the subgraph is statically discoverable, added as a node, which the docs list as supported. There you get no error at all, just a well-formed snapshot with a |
Dinesh Yadav (dineshyadav03)
left a comment
There was a problem hiding this comment.
Read through the diff and the linked repro. Confirmed the root cause: get_state/aget_state already resolve checkpointer correctly via CONFIG_KEY_CHECKPOINTER (falling back to self.checkpointer), but _prepare_state_snapshot/_aprepare_state_snapshot discarded that local and re-read self.checkpointer directly, which is None for any subgraph. Since DeltaChannel has no value in channel_values and needs a saver to replay ancestor writes, that produced an empty-but-valid-looking snapshot instead of an error. Good catch pulling in bulk_update_state/abulk_update_state too — that one persists the empty-hydrated channel as a _DeltaSnapshot blob once snapshot_frequency cadence hits, i.e. actual on-disk history loss.
Question: prepare_next_tasks in the same two methods still gets self.checkpointer and is called inert because _algo.py falls back to CONFIG_KEY_CHECKPOINTER internally. Is that fallback covered by a test, or only reasoned about? Given this whole bug was exactly this kind of "should be equivalent" assumption, I'd rather see the two expressions unified even if cosmetic today.
Making saver a required kwarg rather than defaulting to self.checkpointer is the right call. Tests are thorough. Approve.
Fixes #8470
Reported by gururafiki, with a self-contained
InMemorySaverrepro and the observation that non-delta channels in the same namespace hydrate fine, which is what makes the failure silent._prepare_state_snapshotre-derived the saver fromself.checkpointer, which isNonefor a subgraph, so everyDeltaChannelin a nested subgraph hydrated empty on read, silently.Why the old resolution was wrong
A
DeltaChannelstores no value inchannel_values, so reconstructing it means walking ancestors and replaying their writes, which needs a saver. A subgraph has none of its own: it borrows the parent's throughCONFIG_KEY_CHECKPOINTERat read time, exactly as the four public readers already resolve it a few lines above the call._prepare_state_snapshotignored that and went back toself.checkpointer. With no saver the walk never ran, and the channel fell through tofrom_checkpoint(MISSING): an empty value, indistinguishable from a channel that was never written, raising nothing. Non-delta channels in the same namespace hydrate fine, so the payload looks healthy.Recovering it inside the function isn't possible:
get_state_historypassescheckpoint_tuple.config, the checkpointer's stored config, which never carries a live saver. The caller is the only place the resolved saver exists, so the caller has to pass it.How I verified it
tests/test_delta_channel_subgraph.pyadds 12 tests. 8 fail on main, 4 pass. The 4 that pass are controls for modes that stay unchanged (the two root-graph cases, a stateless subgraph, a completed subgraph). The 8 failures, by surface:get_state/aget_stateon a subgraph ns{'msgs': []}get_state_history/aget_state_history{'msgs': []}get_state(subgraphs=True)task state, subgraph interrupted{'msgs': []}update_state/aupdate_state['manual'], prior history destroyedTests use
InMemorySaver: the defect is inPregel's saver resolution rather than in any checkpointer implementation, so the storage backend is irrelevant to the reproduction.Full suite
1968 -> 1980 passed, 4 skipped, exactly the 12 new tests.make format,lint_package,lint_testsclean;check_sdk_methods.pypasses.Two things worth a closer look in review
1. This fixes a write path as well as a read path, and both are load-bearing.
bulk_update_state/abulk_update_statecarried the same expression, and that half is worse than a misleading read: when the update reaches the channel's snapshot cadence,create_checkpointpersists a_DeltaSnapshotbuilt from the empty-hydrated channel, losing history on disk. I checked whether it could be split: reverting only the twobulk_update_statehunks leavestest_subgraph_update_state_preserves_delta_channel_historyand its async twin failing while the other 10 pass. Splitting would ship a fix that still corrupts history on write.The write-path tests use
snapshot_frequency=2, which is what makes the update reach the cadence and force a snapshot blob. At the default 1000 nothing is written, the loss stays latent, and the assertion would hold either way.2.
prepare_next_tasksin the same two methods still receivesself.checkpointer, and I left it. It is inert on the read path:_algo.py:915builds the task config ascheckpointer or configurable.get(CONFIG_KEY_CHECKPOINTER), so a subgraph'sNonealready falls back to the config value, andPregelTaskcarries noconfigfield, so that config never reaches a reader. Changing it would be cosmetic, but I'll make it symmetric if you'd rather not leave two different expressions side by side.Note on the signature
saveris a required keyword argument rather than a defaulted one. A silent fallback is what caused this bug, and both methods are private with no other callers, so there is nothing to stay compatible with.recursestays defaulted becauseNoneis meaningful there (do not resolve subgraph task states); forsaver,Noneis only ever a bug.Net -6 lines of logic: passing an already-narrowed saver removes the
isinstancedance at every site.