Skip to content

fix(langgraph): delete item when store put receives None - #5886

Open
varun-projects wants to merge 1 commit into
volcengine:mainfrom
varun-projects:fix/5821-store-put-none-deletes
Open

varun-projects wants to merge 1 commit into
volcengine:mainfrom
varun-projects:fix/5821-store-put-none-deletes

Conversation

@varun-projects

@varun-projects varun-projects commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Description

OpenVikingStore.put(namespace, key, None) persisted a record with a null value instead of
deleting the item. LangGraph's BaseStore.put() contract defines a None value as a
deletion. The adapter's batch() already dispatched PutOp with a None value to
delete(), and the inherited aput() reaches the same code through abatch(), but the sync
put() override wrote the None straight through the ordinary JSON record path.

The sync put() now delegates the None case to the existing delete() method rather than
introducing a second deletion path. The delegation sits after the existing unsupported-TTL
guard, so put(..., None, ttl=...) continues to raise NotImplementedError and stays
consistent with aput(), which raises in BaseStore before it ever reaches abatch(). The
value annotation was widened to dict[str, Any] | None to match BaseStore.

The issue was reproduced before the change, not merely inferred from the code: a standalone
script compared OpenVikingStore (backed by the deterministic InMemoryOpenVikingClient
fixture) against langgraph.store.memory.InMemoryStore as the reference implementation, and
confirmed the divergence on both an existing and a missing key. No deployed server or model
call was involved.

One correction to the issue's Expected Behavior that reviewers should be aware of:
InMemoryStore does not drop a namespace from list_namespaces() after deletion. It
keeps an empty namespace entry behind after any deletion, including an explicit delete(),
as an artifact of its defaultdict storage. OpenVikingStore derives namespaces from the
stored records, so a deleted item drops its namespace once it is the last record. That matches
this adapter's pre-existing delete() behaviour and what the issue asks for, so it was left
as is, and the comparison against the reference store is scoped to get() and search().

Human Involvement

  • A human participated in the implementation or review loop
  • This PR was generated entirely by AI agents without human participation in the loop

Related Issue

Fixes #5821

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

  • OpenVikingStore.put() now routes a None value to the existing delete() method and
    returns, so get() yields None, search() omits the item, the namespace is dropped once
    its last record is removed, and deleting a missing key is a no-op.
  • Widened the put() value annotation to dict[str, Any] | None to match the BaseStore
    signature.
  • Extended the existing test_langgraph_store_round_trip_and_semantic_search contract test to
    cover deletion through put(None) on both an existing and a missing key.
  • Documented the deletion semantics in the LangGraph store section of
    docs/en/agent-integrations/07-langchain-langgraph.md and its docs/zh/ translation, which
    previously did not mention deletion at all.

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Validated with the same commands as the repository's own langchain-tests CI job, after
uv pip install -e sdk/python and uv pip install -e "examples/langchain[langgraph,test,dev]":

ruff format --check examples/langchain/src examples/langchain/tests \
    openviking/integrations/langchain tests/integration/langchain_langgraph \
    tests/unit/test_langchain*.py
# 42 files already formatted

ruff check <same paths>
# All checks passed!

mypy --config-file examples/langchain/pyproject.toml examples/langchain/src/langchain_openviking
# Success: no issues found in 15 source files

python -m pytest -q --noconftest -o addopts='' \
    tests/unit/test_langchain*.py tests/integration/langchain_langgraph
# 206 passed

That count matches this branch's unmodified base, since this extends an existing test rather
than adding one.

The extended assertions were confirmed to guard the regression: with the store.py change
reverted and the new test kept, the test fails with
AssertionError: assert Item(..., value=None, ...) is None; with the change applied it
passes.

A temporary comparison script against LangGraph's InMemoryStore confirmed the reported
behaviour before the change and the corrected behaviour after it. It also confirmed that the
aput(), batch(), abatch(), and explicit delete() paths were already correct and
needed no edits — OpenVikingStore does not override aput, so it routes
aput -> abatch -> batch -> PutOp(value=None) -> delete().

The deletion dispatch runs before the existing namespace = tuple(namespace) normalization, so
I also confirmed that a caller passing a list namespace still deletes correctly:
put(["u", "a"], "k", None) leaves get() returning None and search() empty. delete()
only unpacks the namespace when building URIs, so both sequence types behave identically.

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Additional Notes

The TTL guard intentionally runs before the deletion dispatch so the sync and async public
methods agree: BaseStore.aput raises for an unsupported TTL before dispatching, and
OpenVikingStore.supports_ttl is False, so placing the None check first would have made
sync put delete silently where aput raises. Both BaseStore.put and BaseStore.aput
likewise validate ttl before inspecting value.

A separate pre-existing inconsistency remains out of scope: batch([PutOp(..., value=None, ttl=...)]) deletes without raising, because batch calls delete() without forwarding
ttl. Backend-removal error handling for #5774 is being addressed separately in #5777 and is
unchanged here.

LangGraph's BaseStore contract treats a None value passed to put() as a
deletion. OpenVikingStore.batch() already dispatched PutOp with a None
value to delete(), and the inherited aput() reaches the same path through
abatch(), but the sync put() override wrote None straight through the
ordinary JSON record path.

Before this change, put(namespace, key, None) stored a record whose value
was null: get() returned an Item with value None, search() still listed
the record, and list_namespaces() kept exposing the namespace. Calling it
on a key that did not exist created a new null-valued record instead of
doing nothing.

The sync put() now delegates the None case to the existing delete(),
after the unsupported-TTL guard so that put(..., None, ttl=...) still
raises NotImplementedError and stays consistent with aput(). get() now
returns None, search() omits the item, the namespace disappears once its
last record is removed, and deleting a missing key is a no-op. The value
annotation was widened to dict[str, Any] | None to match BaseStore.
@varun-projects
varun-projects force-pushed the fix/5821-store-put-none-deletes branch from 92f7f60 to 8578bd0 Compare October 10, 2026 16:53

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

[Bug]: LangGraph Store put(None) persists an item instead of deleting it

1 participant