3 Commits

Include renames

Author	SHA1	Message	Date
Elior Nataf LackritzandGitHub	a5dbacae0d	fix(checkpoint-postgres,checkpoint-sqlite): treat an empty checkpoint_id as the latest in delta history (#9262) ... `PostgresSaver` and `SqliteSaver`, sync and async, read a DeltaChannel's history from the latest checkpoint only when the config's `checkpoint_id` was missing or `None`. An empty one was looked up as an id, matched nothing, and came back with no seed and no writes, while their own `get_tuple` treats an empty id as the latest checkpoint, as the default implementation does. They now check for a missing or empty id the way `get_tuple` does. The in-memory saver gets the same in #9257. Graphs always pass the id of a saved checkpoint, so only direct callers of the saver API hit this. ## Tests - `test_{sync,async}_empty_checkpoint_id_reads_from_the_latest_checkpoint` in `checkpoint-postgres/tests/test_delta_pagination.py` and `checkpoint-sqlite/tests/test_delta_parent_walk.py`: all four fail on main. - The `checkpoint-postgres` (Postgres 16) and `checkpoint-sqlite` suites pass, and `lint_package` and `lint_tests` are clean in both.	2026-10-09 13:44:51 -04:00
Elior Nataf LackritzandGitHub	93a5a28008	perf(checkpoint-postgres): start the delta walk at the target (#9248) ... Reading a DeltaChannel at an older checkpoint (the state at a past checkpoint, each entry of `get_state_history`, a fork or replay from one) walked `checkpoints` down from the newest row, so every newer row was read before the target's chain. A checkpoint's ancestors have smaller ids, the order `get_tuple` already relies on to find the latest checkpoint, so the walk now starts at the target: the first page is `checkpoint_id <= target`, later pages stay `< cursor`. The cursor is never NULL anymore, so the `(cursor IS NULL OR checkpoint_id < cursor)` predicate goes too. Once Postgres caches a generic plan for the prepared statement (`from_conn_string` sets `prepare_threshold=0`), that predicate is a filter rather than an index condition, and each later page re-read every newer row. `get_delta_channel_history` on a 6,000-checkpoint thread with a snapshot every 200 checkpoints, median of three runs of 60 reads each, same results before and after: | target | main | this PR | |---|---|---| | newest | 3.1 ms | 2.7 ms | | 1,000 back | 4.5 ms | 2.9 ms | | 3,000 back | 8.9 ms | 2.8 ms | | 5,000 back | 15.7 ms | 2.8 ms | The target is now always the first row read, so the pagination tests from #8556 can no longer leave it off the first page; they are renamed for what they still cover, a walk that continues across pages. ## Test plan - [x] `test_{sync,async}_walk_reads_nothing_newer_than_the_target` record the rows the walk reads; both fail on main, which also reads the three newer checkpoints. - [x] `checkpoint-postgres` suite on Postgres 16: 281 passed, 3 skipped. - [x] `langgraph` DeltaChannel tests against this saver (`-k "delta and not redis"`): 1,154 passed, 1 skipped.	2026-10-09 08:31:15 -04:00
3 people	c0279f0910	fix(checkpoint-postgres): derive the delta walk cursor once the target loads (#8556) ... ## Summary `get_delta_channel_history` on Postgres returns an empty history for any `DeltaChannel` on a target checkpoint that is not within the first stage-1 pagination page (1024 rows) of the thread. No exception, no warning: the channel just hydrates empty. Fixes #8448 ## Problem Stage 1 pages `checkpoints` newest-first from the head of the thread, and after each page `_try_advance_walks` tries to move every not-yet-seeded channel's walk along the partial `parent_of` map accumulated so far. The walk starts at the target's parent: ```python if ch not in walk_cursor_by_ch: walk_cursor_by_ch[ch] = parent_of.get(target_id) ``` The target can be any checkpoint in the thread, not just the head, so on the first page `parent_of` frequently has no row for it yet. `.get` then returns `None`, which is also what a target with no parent returns, and the two are stored identically. Because the initialisation is guarded by `ch not in walk_cursor_by_ch`, it never runs again: once the walk is parked at `None` it stays there even after the target's real row and real parent load on a later page. The result is an empty chain and no seed. Downstream `channels_from_checkpoint` does ```python replay_ch = delta_spec.from_checkpoint(history.get("seed", MISSING)) replay_ch.replay_writes(history["writes"]) ``` so `get_state`, `get_state_history` and `update_state` against an older checkpoint reconstruct a `messages` channel as `[]` on a thread with hundreds of real messages. ## Fix Start the walk only once `target_id` is actually present in `parent_of`, so "the target has not loaded yet" stops sharing a representation with "the target is a root": ```python if ch not in walk_cursor_by_ch: if target_id not in parent_of: continue walk_cursor_by_ch[ch] = parent_of[target_id] ``` `_try_advance_walks` is a static method on `BasePostgresSaver`, so `PostgresSaver` and `AsyncPostgresSaver` are both covered by the one change. ## Why it's safe `continue` leaves the channel exactly as it was, so a later page retries. The three existing stop conditions are untouched: a channel that finds its seed still seeds, one that reaches a real root still parks at `None`, and one waiting on an ancestor still keeps its cursor. Paging still terminates on a short page, which is what ends the run for a target that really is a root. ## Long-term The sibling sqlite implementation avoids this class of bug differently, by starting its stage-1 scan at the target (`checkpoint_id <= ?`) instead of at the head. Postgres could adopt the same bound and would then never fetch a checkpoint newer than the target at all, which looks like the bigger win on a long thread. It makes the read path depend on ancestors always sorting below their descendants, though, which sqlite already assumes but the Postgres fast path currently does not. #8550 now reports that assumption as a bug in sqlite, on the grounds that ancestry is defined by `parent_checkpoint_id` and the contract does not require ids to be monotonic, so the bound is the wrong direction to move Postgres in. Paging the full thread and following parent pointers is what keeps this path correct when ids are not monotonic, and with this fix Postgres returns the right history for #8550's scenario at every page size. ## Test plan New `libs/checkpoint-postgres/tests/test_delta_pagination.py`. Page size is monkeypatched rather than writing 1024+ real checkpoints per case, since the only thing that decides the behaviour is which page the target lands on. - [x] `test_async_target_older_than_the_first_page` and its sync twin, parametrised over page sizes `[_DELTA_PAGE_SIZE, 3, 2, 1]`. The thread has 8 checkpoints with a snapshot at step 1 and the target at step 4, so every size at or below 3 leaves the target off the first page. The real page size is the control. - [x] `test_root_target_has_no_history_and_still_terminates` covers the case where a `None` cursor is the correct answer, at page size 1 so the paging loop runs the length of the thread. - [x] 6 of the 9 fail on `main` (`expected a snapshot seed, got '<missing>'`); the 3 that pass are the two controls and the root case. - [x] `make format`, `make lint_package`, `make lint_tests` clean. - [x] Full `libs/checkpoint-postgres` suite, rebased on current `main`: 279 passed, 3 skipped on Postgres 16. - [x] Graph-level repro with `_DELTA_PAGE_SIZE = 5`: 10 invocations, then `get_state` on the 8th-newest checkpoint returns `[]` on `main` and the full history on this branch. Thanks to @Navneet-Scaler for the report, the mechanism write-up, and the fix in #8453, which this matches. Co-authored-by: Navneet-Scaler <147032454+Navneet-Scaler@users.noreply.github.com>	2026-09-30 12:16:55 -04:00