mirror of
https://github.com/langchain-ai/langgraph.git
synced 2026-08-17 21:25:46 +02:00
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.
205 lines
8.4 KiB
Python
205 lines
8.4 KiB
Python
"""Seed detection for `DeltaChannel` histories on Postgres.
|
|
|
|
`put` splits stored values in two: primitives stay inline in the checkpoint's
|
|
`channel_values`, everything else moves to `checkpoint_blobs`. Only
|
|
`_DeltaSnapshot` leaves an inline marker behind when it moves, so the stage-1
|
|
walk has to check both places — a blob probe alone misses inline primitives, and
|
|
an inline-key check alone missed blob-stored plain values, which is what a thread
|
|
migrated from a pre-delta channel type leaves behind. See #8534.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from typing import Any
|
|
from uuid import uuid4
|
|
|
|
import pytest
|
|
from langgraph.checkpoint.base import Checkpoint, empty_checkpoint
|
|
from langgraph.checkpoint.base.id import uuid6
|
|
from langgraph.checkpoint.serde.types import _DeltaSnapshot
|
|
|
|
from langgraph.checkpoint.postgres.aio import AsyncPostgresSaver
|
|
from tests.conftest import DEFAULT_URI
|
|
|
|
CHANNEL = "items"
|
|
|
|
|
|
async def _build_chain(saver: AsyncPostgresSaver, seed_value: Any) -> tuple[str, dict]:
|
|
"""Store `seed_value` at step 1, then two steps that store nothing.
|
|
|
|
Every step carries a write so the walk has something to collect.
|
|
Returns `(thread_id, head_config)`.
|
|
"""
|
|
thread_id = str(uuid4())
|
|
parent: dict | None = None
|
|
for step in range(4):
|
|
config: dict = {"configurable": {"thread_id": thread_id, "checkpoint_ns": ""}}
|
|
if parent is not None:
|
|
config["configurable"]["checkpoint_id"] = parent["configurable"][
|
|
"checkpoint_id"
|
|
]
|
|
cp: Checkpoint = empty_checkpoint()
|
|
cp["id"] = str(uuid6(clock_seq=step))
|
|
new_versions: dict[str, Any] = {}
|
|
if step == 1:
|
|
cp["channel_values"][CHANNEL] = seed_value
|
|
cp["channel_versions"][CHANNEL] = "v1"
|
|
new_versions[CHANNEL] = "v1"
|
|
else:
|
|
cp["channel_versions"][CHANNEL] = f"v{step}"
|
|
parent = await saver.aput(
|
|
config, cp, {"source": "loop", "step": step, "parents": {}}, new_versions
|
|
)
|
|
await saver.aput_writes(parent, [(CHANNEL, f"w{step}")], str(uuid4()))
|
|
assert parent is not None
|
|
return thread_id, parent
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_plain_value_seed_is_found() -> None:
|
|
"""A pre-delta plain value must be located as the seed.
|
|
|
|
Before #8534 the walk ran to the root and returned no seed, which happens
|
|
to reconstruct correctly for additive reducers while costing an
|
|
O(thread length) replay on every read.
|
|
"""
|
|
async with AsyncPostgresSaver.from_conn_string(DEFAULT_URI) as saver:
|
|
await saver.setup()
|
|
_, head = await _build_chain(saver, [10, 20])
|
|
|
|
result = await saver.aget_delta_channel_history(config=head, channels=[CHANNEL])
|
|
entry = result[CHANNEL]
|
|
|
|
assert entry.get("seed") == [10, 20], (
|
|
f"expected the plain value as seed, got {entry.get('seed', '<missing>')}"
|
|
)
|
|
# Only the writes between the seed and the head's parent replay: step 1
|
|
# (the seed's own) and step 2. Step 0 is older than the seed, step 3 is
|
|
# pending at the head.
|
|
assert [w[2] for w in entry["writes"]] == ["w1", "w2"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_delta_snapshot_seed_is_found() -> None:
|
|
"""The `_DeltaSnapshot` path keeps working, so both seed kinds agree."""
|
|
async with AsyncPostgresSaver.from_conn_string(DEFAULT_URI) as saver:
|
|
await saver.setup()
|
|
_, head = await _build_chain(saver, _DeltaSnapshot([10, 20]))
|
|
|
|
result = await saver.aget_delta_channel_history(config=head, channels=[CHANNEL])
|
|
entry = result[CHANNEL]
|
|
|
|
seed = entry.get("seed")
|
|
assert isinstance(seed, _DeltaSnapshot), f"expected a snapshot, got {seed!r}"
|
|
assert seed.value == [10, 20]
|
|
assert [w[2] for w in entry["writes"]] == ["w1", "w2"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_version_bump_without_a_value_does_not_hide_an_older_seed() -> None:
|
|
"""A delta-era step bumps `channel_versions` without storing a value, so no
|
|
blob exists for that version. The probe must report no seed there and keep
|
|
walking rather than stopping at a version it cannot resolve.
|
|
|
|
Step 0 holds the real value; step 1 bumps the version with nothing stored.
|
|
Walking back from the head has to pass step 1 to reach step 0.
|
|
"""
|
|
async with AsyncPostgresSaver.from_conn_string(DEFAULT_URI) as saver:
|
|
await saver.setup()
|
|
thread_id = str(uuid4())
|
|
parent: dict | None = None
|
|
for step in range(4):
|
|
config: dict = {
|
|
"configurable": {"thread_id": thread_id, "checkpoint_ns": ""}
|
|
}
|
|
if parent is not None:
|
|
config["configurable"]["checkpoint_id"] = parent["configurable"][
|
|
"checkpoint_id"
|
|
]
|
|
cp: Checkpoint = empty_checkpoint()
|
|
cp["id"] = str(uuid6(clock_seq=step))
|
|
new_versions: dict[str, Any] = {}
|
|
if step == 0:
|
|
cp["channel_values"][CHANNEL] = [10, 20]
|
|
cp["channel_versions"][CHANNEL] = "v0"
|
|
new_versions[CHANNEL] = "v0"
|
|
elif step == 1:
|
|
# Version bumped, value absent -> no blob row written.
|
|
cp["channel_versions"][CHANNEL] = "v1"
|
|
new_versions[CHANNEL] = "v1"
|
|
else:
|
|
cp["channel_versions"][CHANNEL] = "v1"
|
|
parent = await saver.aput(
|
|
config,
|
|
cp,
|
|
{"source": "loop", "step": step, "parents": {}},
|
|
new_versions,
|
|
)
|
|
await saver.aput_writes(parent, [(CHANNEL, f"w{step}")], str(uuid4()))
|
|
assert parent is not None
|
|
|
|
result = await saver.aget_delta_channel_history(
|
|
config=parent, channels=[CHANNEL]
|
|
)
|
|
entry = result[CHANNEL]
|
|
|
|
assert entry.get("seed") == [10, 20], (
|
|
"the walk stopped at the empty blob instead of reaching the real "
|
|
f"value at step 0; got {entry.get('seed', '<missing>')}"
|
|
)
|
|
assert [w[2] for w in entry["writes"]] == ["w0", "w1", "w2"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_inline_primitive_seed_is_found() -> None:
|
|
"""`put` keeps `None`, `str`, `int`, `float` and `bool` in the checkpoint's
|
|
own `channel_values` with no blob row, so a blob probe alone cannot see
|
|
them. Stage 1 reads the inline value too and uses it when there is no blob.
|
|
"""
|
|
async with AsyncPostgresSaver.from_conn_string(DEFAULT_URI) as saver:
|
|
await saver.setup()
|
|
for seed_value in (42, "x", 3.5, None):
|
|
_, head = await _build_chain(saver, seed_value)
|
|
entry = (
|
|
await saver.aget_delta_channel_history(config=head, channels=[CHANNEL])
|
|
)[CHANNEL]
|
|
if seed_value is None:
|
|
# A JSON null is indistinguishable from "no value stored", so
|
|
# the walk keeps going; replay from empty is the correct result.
|
|
assert "seed" not in entry
|
|
else:
|
|
assert entry.get("seed") == seed_value, (
|
|
f"inline {type(seed_value).__name__} seed not found: "
|
|
f"{entry.get('seed', '<missing>')!r}"
|
|
)
|
|
assert [w[2] for w in entry["writes"]] == ["w1", "w2"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_inline_true_is_not_read_as_a_snapshot_marker() -> None:
|
|
"""`put` inlines a literal `true` in `channel_values` as the marker for a
|
|
`_DeltaSnapshot`, which is also what a genuine `bool` channel holding
|
|
`True` looks like. A blob exists only in the snapshot case, so preferring
|
|
the blob keeps the two apart.
|
|
"""
|
|
async with AsyncPostgresSaver.from_conn_string(DEFAULT_URI) as saver:
|
|
await saver.setup()
|
|
|
|
_, head = await _build_chain(saver, True)
|
|
entry = (
|
|
await saver.aget_delta_channel_history(config=head, channels=[CHANNEL])
|
|
)[CHANNEL]
|
|
assert entry.get("seed") is True, (
|
|
f"a real inline True must survive, got {entry.get('seed', '<missing>')!r}"
|
|
)
|
|
|
|
_, snap_head = await _build_chain(saver, _DeltaSnapshot(True))
|
|
snap_entry = (
|
|
await saver.aget_delta_channel_history(config=snap_head, channels=[CHANNEL])
|
|
)[CHANNEL]
|
|
seed = snap_entry.get("seed")
|
|
assert isinstance(seed, _DeltaSnapshot), (
|
|
f"the marker must resolve to the blob, not inline true; got {seed!r}"
|
|
)
|
|
assert seed.value is True
|