mirror of
https://github.com/langchain-ai/langgraph.git
synced 2026-08-17 21:25:46 +02:00
a90ab44358
Fixes langchain-ai/langgraph#8384 `InMemorySaver.get_delta_channel_history` skipped the writes stored at the ancestor it seeded from whenever that ancestor's blob was a plain value rather than a `_DeltaSnapshot`, silently dropping the first write made after migrating a thread to `DeltaChannel`. ### Why the old rule was wrong A stored blob is the value *entering* its checkpoint; the writes stored under that same checkpoint are what produce its child. That's true for `_DeltaSnapshot` blobs and pre-delta plain values alike, so there was never a reason to treat them differently. Writes at ancestors *older* than the seed genuinely are subsumed by the seed value — but that's already guaranteed by terminating the walk, since the channel leaves `remaining` once its seed is found. The removed check re-solved that and overreached by one checkpoint. `BaseCheckpointSaver`, `SqliteSaver` and `PostgresSaver` never had this check. `InMemorySaver` was the only outlier. ### How I verified it Built a differential harness running the same migration scenarios through `InMemorySaver`, the `BaseCheckpointSaver` reference walk, and `SqliteSaver`. **4 of 11 scenarios agreed before this change; 11 of 11 after.** The loss is wider than one write — on the `add_messages` → `DeltaChannel` path it drops a real user message. Suites: `libs/checkpoint` 156 passed, `libs/langgraph` 1972 passed, `libs/checkpoint-sqlite` 117 passed, `libs/checkpoint-postgres` passed against PG 16. `make format`, `make lint` clean in each. ### Two things worth a closer look in review **1. I inverted two existing assertions** in `TestPreDeltaBlobTerminator` (`libs/checkpoint/tests/test_memory.py`). They encoded the old rule. Their fixture is the real migration shape — a plain-value blob carrying pending writes, with a delta-era child — which I confirmed against a dumped checkpoint chain from the issue's repro, so the assertions were wrong rather than the fixture being unrealistic. I added an ancestor *older* than the seed so the terminator still guards what it legitimately should: older writes stay excluded, the seed's own writes replay. **2. The new conformance test fails against Postgres**, for a reason unrelated to this change. Postgres `aput` leaves an inline `True` marker in `channel_values` only for `_DeltaSnapshot`; plain non-primitive values are popped with no marker, and seed detection is `(checkpoint -> 'channel_values' -> ch) IS NOT NULL`. So Postgres can't locate a plain-value seed at all: ``` seed stored as plain list: InMemorySaver -> [10, 20] AsyncPostgresSaver -> no seed key seed stored as _DeltaSnapshot: InMemorySaver -> found AsyncPostgresSaver -> found ``` The pre-existing `test_history_migration_plain_value_as_seed` already fails there too — conformance CI only validates `InMemorySaver`, so nobody was watching. Values still come out correct today (with no seed the walk runs to the root and replays everything), but early termination is lost: 1 write replayed on `InMemorySaver` vs 7 on Postgres for the same 6-turn thread. Filing separately rather than folding a write-path/format decision into this PR. ### Note on scope This touches three packages: the fix in `libs/checkpoint`, graph-level regression tests in `libs/langgraph` (the bug is only observable through a graph read), and the contract test in `libs/checkpoint-conformance` so third-party savers are covered too. --------- Co-authored-by: PiedPiper911 <32931126+PiedPiper911@users.noreply.github.com>