mirror of
https://github.com/langchain-ai/langgraph.git
synced 2026-08-17 21:25:46 +02:00
Elior Nataf LackritzandGitHub
d569e18f4b
Fixes langchain-ai/langgraph#8534 `put` splits stored values in two: primitives stay inline in the checkpoint's `channel_values`, everything else moves to `checkpoint_blobs`, and only `_DeltaSnapshot` leaves an inline marker behind when it moves. Stage-1 seed detection tested for that marker, so a plain value — what a thread migrated from `BinaryOperatorAggregate` leaves behind — was invisible to the walk. ### Effect Migrated threads found no seed, walked to the root, and replayed every write on every read. Values still came out correct, because replaying an additive reducer from empty rebuilds the same list, which is why nothing looked wrong. What was lost is early termination — the entire point of `DeltaChannel`: <!-- linear:table-colwidths:266,266,266 --> | thread length | writes replayed, before | after | | -- | -- | -- | | 2 turns | 3 | 1 | | 6 turns | 7 | 1 | | 20 turns | 21 | 1 | Read latency is flat at \~0.6ms across all three after the change. ### Approach Stage 1 now checks both places a value can live rather than trusting the marker. It probes `checkpoint_blobs`: ```sql EXISTS (SELECT 1 FROM checkpoint_blobs b0 WHERE b0.thread_id = checkpoints.thread_id AND b0.checkpoint_ns = checkpoints.checkpoint_ns AND b0.channel = %s AND b0.version = checkpoint -> 'channel_versions' ->> %s AND b0.type <> 'empty') AS hb_0 ``` and selects the inline value alongside it, since `None`, `str`, `int`, `float` and `bool` stay in `channel_values` with no blob row: ```sql checkpoint -> 'channel_values' -> %s AS inline_0 ``` The blob predicate matches `checkpoint_blobs`' primary key `(thread_id, checkpoint_ns, channel, version)` exactly, so it is one index lookup per row per channel, bounded by the 1024-row page. I picked reading storage over the cheaper alternative — also writing the marker for plain values — because **that would not fix any thread already on disk.** Existing checkpoints have no marker and there is nowhere to add one retroactively. The seed resolves to the blob when one exists and the inline value otherwise. That ordering is also what keeps a genuine inline `true` — a `bool` channel holding `True` — distinguishable from the literal `true` marker `put` inlines for a `_DeltaSnapshot`: only the snapshot has a blob. `None` is deliberately not treated as a seed; a JSON null is indistinguishable from "nothing stored" at this layer, so the walk continues and replay from empty is correct. Params go from two to four per channel; both callers updated. The inline half came out of review on this PR — a blob-only probe would have left scalar-aggregate migrations (an integer sum, say) still replaying their full history. ### On the `type <> 'empty'` predicate Being upfront since it isn't demonstrable with a test: `put` does not currently produce `empty` rows on this path — `blob_versions` is filtered to keys present in `channel_values`, so `_dump_blobs`' empty branch is unreachable from it. I confirmed there are no `empty` rows in a populated test database. I kept it because stage 2 already applies the same check when resolving the seed blob. Without it the two stages could disagree: stage 1 terminates the walk on a row stage 2 then discards, producing no seed *and* a truncated write chain — the same failure shape this function exists to avoid. Rationale is in the docstring so the next reader doesn't have to ask. Happy to drop it if you'd rather not carry an unexercised predicate. ### Tests `libs/checkpoint-postgres/tests/test_delta_plain_value_seed.py` — blob-stored plain-value seed, `_DeltaSnapshot` seed, a version bump with nothing stored (which must not stop the walk short of an older real value), inline primitives (`int`, `str`, `float`, `None`), and inline `True` versus the snapshot marker. Each fails against the behaviour it fixes. Verified: postgres suite 269 passed on PG 15 and 16; delta-channel conformance against `AsyncPostgresSaver` went from 6 of 8 to 8 of 8, including the pre-existing `test_history_migration_plain_value_as_seed` failure this was causing; `make lint` clean. ### Not included I wanted a Postgres conformance runner alongside `checkpoint-sqlite`'s, but it needs `langgraph-checkpoint-conformance` as a dev dependency and the contributing guide asks for maintainer sign-off before adding one. The direct tests above cover the same ground without it. Worth flagging separately: **conformance effectively runs against** `InMemorySaver` **only today.** `libs/checkpoint-conformance/tests/` contains just `test_validate_memory.py`, and `checkpoint-sqlite`'s `test_conformance_delta.py` silently skips because the package isn't installed in its test environment (`importorskip`). Wiring it up for sqlite and postgres is what would have caught this bug, and langchain-ai/langgraph#8534 notes it. Sqlite is unaffected by the bug itself — it stores `channel_values` inline and inspects them directly. `langgraph-api` already resolves seeds by version rather than by marker.