Skip to content

fix(checkpoint): return only the stored checkpoint's pending writes from the shallow savers - #213

Merged
limjoobin merged 1 commit into
redis-developer:mainfrom
ericdelorefice:fix/shallow-stale-pending-writes
Oct 23, 2026
Merged

limjoobin merged 1 commit into
redis-developer:mainfrom
ericdelorefice:fix/shallow-stale-pending-writes

Conversation

@ericdelorefice

Copy link
Copy Markdown
Contributor

Problem

#134 fixed #133 by no longer deleting a thread's writes when the shallow savers save a new checkpoint, so interrupt writes survive until they are consumed. The write registry (write_registry:{thread}:{ns}:shallow in the sync saver, write_keys_zset:{thread}:{ns}:shallow in the async one) is now cleared only by delete_thread or TTL, so it keeps the write keys of every earlier checkpoint in the thread.

ShallowRedisSaver._load_pending_writes (shallow.py 512-552) and AsyncShallowRedisSaver._aload_pending_writes (ashallow.py 619-672) return every write in that registry as the pending writes of the current checkpoint (the comments still say "All writes belong to the current checkpoint"). Each write document already stores the checkpoint_id it was written against, but the loaders do not look at it.

LangGraph applies the null-task pending writes of the checkpoint it resumes from (for example the update in Command(update=..., resume=...)) at the start of every run. So an update sent once is applied again on each later resume of the thread. RedisSaver, AsyncRedisSaver and LangGraph's InMemorySaver keep pending writes per checkpoint id and apply it once.

Reproduction

Against Redis 8.10.2 (search + JSON) on main, nothing mocked or injected: a graph of three nodes, each calling interrupt(), run with invoke({"log": ["start"]}), then Command(resume="A", update={"log": ["update"]}), then Command(resume="B") and Command(resume="C"), where log uses operator.add:

  • InMemorySaver and RedisSaver: ["start", "update", "ask1:A", "ask2:B", "ask3:C"].
  • ShallowRedisSaver and AsyncShallowRedisSaver: "update" appears four times. get_tuple returns 8, 12 and 14 pending writes after the three resumes, where RedisSaver returns 1, 1 and 0.

Two other effects of the same leftover writes:

  • With a plain field (no reducer) set by the update, the second plain resume raises InvalidUpdateError: At key 'status': Can receive only one value per step. Before that, get_state() already shows the old update's value in place of the newer value a node wrote.
  • With durability="exit" and no update at all, the second resume of the three-question graph raises RuntimeError: When there are multiple pending interrupts, you must specify the interrupt id when resuming.

I compared both shallow savers with InMemorySaver over four graphs (two interrupts in one node, a chain, two parallel interrupting nodes, an interrupt inside a subgraph) in each durability mode, checking the result, get_state().next, the pending interrupts and the values after every call. On main each shallow saver matched in 3 of the 12 runs; with this change, in all 12.

Change

Both loaders now keep only the write documents whose checkpoint_id is the stored checkpoint's id, the same set RedisSaver returns. Nothing is deleted: earlier writes stay in Redis as they do since #134, and the 7 tests in test_issue_133_hitl_shallow.py pass unchanged. The source change is two conditions and the comments (+8/-5 across 2 files).

This is a behaviour change: get_tuple, aget_tuple and ShallowRedisSaver.list no longer return writes made against earlier checkpoints. test_shallow_ulid_ttl_cache.py::test_put_writes_persist_for_hitl_support asserted that they do, so I changed it to check that the earlier checkpoint's writes are still stored in Redis and that get_tuple returns only the current checkpoint's write.

I left two things alone: the registry still grows until delete_thread or TTL (cleaning it up safely is a separate change), and the sync and async shallow savers still use different registry key names (mentioned in #211).

Testing

New tests/test_shallow_stale_pending_writes.py, integration tests against real Redis:

  • get_tuple / aget_tuple return no pending writes for a new checkpoint when the previous checkpoint had writes, and only the new checkpoint's write once it has one.
  • The three-question graph above gives the InMemorySaver log with each shallow saver, in each durability mode (sync, async, exit).
  • Controls that pass with and without the change: the InMemorySaver reference run, and a checkpoint's own writes are returned in order (sync and async).

The 8 new failing cases and the updated existing test fail on main (5 of 5 runs) and pass with the change (5 of 5 runs).

The shallow-saver test files (20 files: every file that uses either shallow saver except the sentinel tests, plus the TTL, registry and cross-slot files):

  • with this change: 177 passed
  • main: 168 passed, 9 failed (the 8 new cases and the updated test)

poetry run format, sort-imports and check-mypy are clean.

This PR was written with AI assistance (Claude Code); the tests above were run before opening it.

…rom the shallow savers

The shallow savers keep one write registry per thread and namespace, and
since redis-developer#134 it is no longer cleared when a new checkpoint is saved. The
loaders returned every write in it, so get_tuple/aget_tuple reported
writes made against earlier checkpoints as pending writes of the current
one. LangGraph applies null-task pending writes (the update in
Command(update=..., resume=...)) at the start of every run, so an update
was applied again on each later resume.

Keep only the writes whose checkpoint_id is the stored checkpoint's, as
RedisSaver does. Nothing is deleted, so the redis-developer#133 behaviour is unchanged.

@limjoobin limjoobin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@limjoobin
limjoobin merged commit 33f2d40 into redis-developer:main Oct 23, 2026
17 checks passed
limjoobin pushed a commit that referenced this pull request Oct 24, 2026
…ow savers (#219)

Runs the graph from #215 end to end on ShallowRedisSaver and
AsyncShallowRedisSaver: a plain (LastValue) channel set by
Command(resume=..., update=...) on two resumes of the same thread.
Before #213 the second resume raised InvalidUpdateError because the
first resume's update was loaded again as a pending write. Both turns
and the final state must match InMemorySaver.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HITL works unexpected

2 participants