Repository navigation
feat(core): add checkpoint namespaces - #131
kaihao-zhao wants to merge 1 commit into
Conversation
![dev-unblocked[bot]](https://avatars.githubusercontent.com/in/213675?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found.
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @dev-unblocked to request a review or summary, or ask follow-up questions.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
| * @return a configuration using the requested namespace | ||
| */ | ||
| public RunnableConfig withCheckpointNamespace(String checkpointNamespace) { | ||
| if (Objects.equals(this.threadId, checkpointNamespace)) { |
There was a problem hiding this comment.
The short-circuit check compares this.threadId with the new checkpointNamespace. This is a copy-paste error: when the thread ID happens to equal the requested namespace, the method returns this and silently ignores the namespace change; conversely, when the namespace is unchanged but the thread ID differs, a needless copy is created. Compare against this.checkpointNamespace as done for checkPointId in withCheckPointId.
| if (Objects.equals(this.threadId, checkpointNamespace)) { | |
| if (Objects.equals(this.checkpointNamespace, checkpointNamespace)) { |
| private Path getNamespaceFolder(RunnableConfig config) { | ||
| return config.checkpointNamespace() | ||
| .map(targetFolder::resolve) | ||
| .orElse(targetFolder); | ||
| } |
There was a problem hiding this comment.
getNamespaceFolder resolves the raw namespace string against targetFolder with no validation. A namespace such as ../other (or an absolute path, which Path.resolve adopts wholesale) makes getPath, serialize, releaseCheckpoints, and deleteFile read/write/delete files outside the checkpoint root. Since namespaces are intended for tenant IDs — plausibly derived from external input — this is a path-traversal risk. It also contradicts the new documentation in src/site/mkdocs/core/core-library.md, which states namespaces "are trimmed and must contain only letters, digits, hyphens, or underscores" — no such trimming or validation exists anywhere in the code. Enforce the documented contract, e.g.:
private static final Pattern NAMESPACE_PATTERN = Pattern.compile("[A-Za-z0-9_-]+");
private Path getNamespaceFolder(RunnableConfig config) {
return config.checkpointNamespace()
.map(String::trim)
.filter(ns -> NAMESPACE_PATTERN.matcher(ns).matches())
.map(targetFolder::resolve)
.orElse(targetFolder);
}(or reject invalid namespaces with an exception in the RunnableConfig.Builder so the guarantee holds for all savers).
| final var checkpointKey = checkpointKey(config); | ||
| return new Tag( threadId(config), _checkpointsByThread.remove( checkpointKey ) ); |
There was a problem hiding this comment.
MemorySaver now keys checkpoints by checkpointKey (namespace:threadId), and VersionedMemorySaver delegates put/get/list to it — so live checkpoints are isolated. However, VersionedMemorySaver.release stores the returned Tag in _checkpointsHistoryByThread keyed by bare threadId (see VersionedMemorySaver.java lines 175–184), and versionsByThreadId/lastVersionByThreadId/getCheckpointsByVersion are threadId-only. Two tenants sharing a thread ID therefore cross-contaminate each other's released-checkpoint history, silently breaking the tenant isolation this PR promises for the built-in memory savers. Either key the history by checkpointKey(config) too, or document that VersionedMemorySaver does not support namespaces.
| /** | ||
| * Returns the namespace used to partition checkpoints. | ||
| * | ||
| * @return the configured namespace, or {@code $default} when no namespace is set |
There was a problem hiding this comment.
The javadoc says the method returns $default when no namespace is set, but the implementation returns ofNullable(checkpointNamespace) — i.e., an empty Optional. The $default fallback only exists in BaseCheckpointSaver.checkpointNamespace(RunnableConfig). Correct the doc to match the actual contract.
| * @return the configured namespace, or {@code $default} when no namespace is set | |
| * @return an {@link Optional} containing the configured namespace, or empty if none is set |
Risk AssessmentThe new filesystem saver resolves the caller-supplied namespace directly as a path, but the documented trimming and character restrictions are not enforced, so namespace values can escape the target folder. Also, |
![unblocked-local-kaihao[bot]](https://avatars.githubusercontent.com/u/304571795?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found.
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @unblocked-local-kaihao to request a review or summary, or ask follow-up questions.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
| */ | ||
| public static class Builder extends HasMetadata.Builder<Builder> { | ||
| private String threadId; | ||
| private String checkpointNamespace; |
There was a problem hiding this comment.
What changed — The builder has a namespace field, but Builder(RunnableConfig config) never copies config.checkpointNamespace into it.
Where it breaks — CompiledGraph.java:674 copies the supplied config before execution, and AbstractCheckpointSaver.java:88 copies it when returning from put. Both copies lose the namespace.
Impact — A graph can read a tenant's checkpoint and then write subsequent checkpoints to the default namespace. Copy the namespace in the builder's copy constructor.
|
|
||
| private Path getNamespaceFolder(RunnableConfig config) { | ||
| return config.checkpointNamespace() | ||
| .map(targetFolder::resolve) |
There was a problem hiding this comment.
targetFolder.resolve accepts namespace strings such as ../../other or an absolute path; Java's path resolution does not confine either to targetFolder. A caller-controlled namespace can therefore direct checkpoint reads, writes, releases, and deletion outside the configured store. Validate namespaces as single directory names and enforce containment before file operations.
| * @return a configuration using the requested namespace | ||
| */ | ||
| public RunnableConfig withCheckpointNamespace(String checkpointNamespace) { | ||
| if (Objects.equals(this.threadId, checkpointNamespace)) { |
There was a problem hiding this comment.
The early return compares checkpointNamespace with threadId. If they match, the requested namespace is never set; this also prevents clearing a namespace when the thread ID is absent.
| if (Objects.equals(this.threadId, checkpointNamespace)) { | |
| if (Objects.equals(this.checkpointNamespace, checkpointNamespace)) { |
| * @return namespace-qualified thread key | ||
| */ | ||
| default String checkpointKey(RunnableConfig config) { | ||
| return "%s:%s".formatted(checkpointNamespace(config), threadId(config)); |
There was a problem hiding this comment.
MemorySaver.java:32 indexes its shared map with this key, but neither component is escaped. Namespace a:b with thread c and namespace a with thread b:c both produce a:b:c, so the two configurations share checkpoints. Use an injective encoding or a composite key.
| final var threadId = threadId(config); | ||
| return new Tag( threadId(config), _checkpointsByThread.remove( threadId ) ); | ||
| final var checkpointKey = checkpointKey(config); | ||
| return new Tag( threadId(config), _checkpointsByThread.remove( checkpointKey ) ); |
There was a problem hiding this comment.
What changed — Release removes a namespace-specific checkpoint list, but the returned tag still identifies it only by thread ID.
Where it breaks — VersionedMemorySaver.java:175-180 delegates to this release and stores tags in its history map under threadId alone. Its config-based version queries likewise discard the namespace.
Impact — Releasing the same thread ID in two namespaces combines their version histories. Include the namespace in version-history indexing and config-based lookups.
| | Attribute | Type | Description | | ||
| | --------- | ---- | ----------- | | ||
| | **threadId** | `String` | A unique identifier for the execution thread/session. Essential for checkpoint-based persistence, as it groups related executions together. Allows resuming interrupted graphs or maintaining conversation history. | | ||
| | **checkpointNamespace** | `String` | Optional partition for checkpoint data. Namespace values are trimmed and must contain only letters, digits, hyphens, or underscores. | |
There was a problem hiding this comment.
This says namespaces are trimmed and restricted to letters, digits, hyphens, and underscores, but RunnableConfig.Builder.checkpointNamespace stores the string unchanged. For example, " tenant-a " remains distinct from "tenant-a", and strings containing : are accepted. Implement the documented normalization and validation, or correct the contract.
| /** | ||
| * Returns the namespace used to partition checkpoints. | ||
| * | ||
| * @return the configured namespace, or {@code $default} when no namespace is set |
There was a problem hiding this comment.
The method returns Optional.empty() when the field is null, not an Optional containing $default. The fallback occurs in BaseCheckpointSaver; update this return documentation to describe the getter's actual result.
| graph.invoke(inputs, config); | ||
| ``` | ||
|
|
||
| Checkpoint namespaces allow separate applications or tenants to reuse the same thread ID without sharing state. When omitted, checkpoints continue to use the default namespace for backward compatibility. `MemorySaver` and `FileSystemSaver` both apply the namespace to list, get, put, release, and delete operations. |
There was a problem hiding this comment.
The sentence says both savers apply the namespace to deletion, but MemorySaver and its base interfaces have no delete operation. Only FileSystemSaver provides deleteFile; distinguish its operation from the methods both savers support.
| * | ||
| * <p> | ||
| * Each RunnableConfig is associated with a file in the provided targetFolder. | ||
| * Each RunnableConfig is associated with a file in a namespace folder under the provided targetFolder. |
There was a problem hiding this comment.
This says each configuration's file is in a namespace folder, but getNamespaceFolder returns targetFolder itself when no namespace is set. State that only explicitly namespaced configurations use subfolders; legacy files remain at the root.
| @Override | ||
| protected Tag releaseCheckpoints(RunnableConfig config, LinkedList<Checkpoint> checkpoints) throws Exception { | ||
| final var currentPath = getPath(config); | ||
| final var namesapceFolder = getNamespaceFolder(config); |
There was a problem hiding this comment.
namesapceFolder misspells “namespace” and differs from getNamespaceFolder. Rename it consistently at its declaration and three uses.
Risk Assessment
|
1 participant
Adds checkpoint namespace support to RunnableConfig and applies namespace isolation to the built-in memory and filesystem checkpoint savers. Includes API documentation, backward-compatible default behavior, and focused tests for tenant isolation. Test command: ./mvnw -pl langgraph4j-core test.