Repository navigation
feat(core): add checkpoint namespaces - #140
kaihao-zhao wants to merge 1 commit into
Conversation
|
@unblocked-local-kaihao review |
![dev-unblocked[bot]](https://avatars.githubusercontent.com/in/213675?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 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.
| */ | ||
| public static class Builder extends HasMetadata.Builder<Builder> { | ||
| private String threadId; | ||
| private String checkpointNamespace; |
There was a problem hiding this comment.
The new checkpointNamespace field is declared on the Builder, but the copy constructor Builder(RunnableConfig config) (RunnableConfig.java:285-291) was not updated — it copies threadId, checkPointId, nextNode, streamMode and metadata, but not checkpointNamespace. Every config-copy path therefore silently loses the namespace, most critically AbstractCheckpointSaver.put() (AbstractCheckpointSaver.java:88), which returns RunnableConfig.builder(config).checkPointId(...).build(). During a real graph run, CompiledGraph.updateState (CompiledGraph.java:269) continues with the config returned by saver.put, so after the very first checkpoint write a tenant-namespaced config falls back to $default and all subsequent checkpoint reads/writes land in the wrong namespace — silently breaking the tenant isolation this PR is meant to provide. The same drop affects withCheckPointId, withStreamMode, updateMetadata, removeMetadata, StateSnapshot.of and SubCompiledGraphNodeAction. The new tests pass only because they call saver.put/get directly with the original configs and never use the returned config.
Fix in the copy constructor:
Builder( RunnableConfig config ) {
super( requireNonNull(config, "config cannot be null!").metadata );
this.threadId = config.threadId;
this.checkpointNamespace = config.checkpointNamespace;
this.checkPointId = config.checkPointId;
...
}| * @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 guard compares this.threadId against the requested namespace instead of this.checkpointNamespace. If a config's threadId happens to equal the requested namespace (e.g. threadId("tenant-acme") with no namespace, then withCheckpointNamespace("tenant-acme")), the method returns this and the namespace is silently never applied — checkpoints stay in the previous (default) namespace. Conversely, when the namespace is already equal but the threadId differs, a needless copy is made.
| if (Objects.equals(this.threadId, checkpointNamespace)) { | |
| if (Objects.equals(this.checkpointNamespace, checkpointNamespace)) { |
| return config.checkpointNamespace() | ||
| .map(targetFolder::resolve) | ||
| .orElse(targetFolder); |
There was a problem hiding this comment.
getNamespaceFolder resolves the raw checkpointNamespace value directly under targetFolder with no validation. A namespace containing .. segments (e.g. "../../shared") escapes the checkpoint root, and because Path.resolve replaces the target entirely when given an absolute path, a namespace like "/tmp/x" writes checkpoints outside the configured folder entirely. The documentation added in this PR (core-library.md:145) states namespaces "are trimmed and must contain only letters, digits, hyphens, or underscores", but no such validation exists anywhere in the code. Relatedly, BaseCheckpointSaver.checkpointKey joins with ":", so namespace "a" + threadId "b:c" collides with namespace "a:b" + threadId "c" in MemorySaver.
Fix: validate in Builder.checkpointNamespace (trim, then reject anything outside [A-Za-z0-9_-]), or at minimum in FileSystemSaver verify the resolved path still starts with targetFolder before use.
| default String checkpointKey(RunnableConfig config) { | ||
| return "%s:%s".formatted(checkpointNamespace(config), threadId(config)); | ||
| } |
There was a problem hiding this comment.
checkpointKey is only consumed by MemorySaver. The sibling savers in this repository — RedisSaver (RedisSaver.java:347), PostgresSaver (PostgresSaver.java:262), OracleSaver (OracleSaver.java:230), plus the MySQL, CockroachDB, DynamoDB and Hazelcast savers — still key storage purely by threadId(config), so a user who sets checkpointNamespace and later switches savers (or uses a DB-backed saver from the start) silently loses tenant isolation with no error or warning: two tenants sharing a threadId read and write each other's checkpoints. VersionedMemorySaver (core) is also inconsistent: get/put/list delegate to the namespaced MemorySaver, but its version history and versionsByThreadId key by bare threadId, merging histories of different namespaces with the same threadId. Either apply checkpointKey in these savers or make the ignored namespace fail loudly (or document per-saver support prominently).
Based on AGENT.md
| /** | ||
| * 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) — an empty Optional. The $default substitution happens later in BaseCheckpointSaver.checkpointNamespace, not here; a caller relying on this doc would never see $default.
| * @return the configured namespace, or {@code $default} when no namespace is set | |
| * @return the configured namespace, or {@link Optional#empty()} if no namespace is set |
| | 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.
The table states "Namespace values are trimmed and must contain only letters, digits, hyphens, or underscores", but Builder.checkpointNamespace (RunnableConfig.java:314-316) is a plain field assignment — no trimming, no charset check, no rejection. Either implement the documented validation (which would also close the path-traversal hole in FileSystemSaver.getNamespaceFolder) or correct the documentation to say values are used as-is.
| @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 should be namespaceFolder; the typo is repeated at lines 147, 157, 166 and 172. Rename all four occurrences.
| final var checkpointKey = checkpointKey(config); | ||
| return new Tag( threadId(config), _checkpointsByThread.remove( checkpointKey ) ); | ||
| } | ||
|
|
||
| @Override | ||
| protected LinkedList<Checkpoint> loadCheckpoints(RunnableConfig config) throws Exception { | ||
| final var threadId = threadId(config); | ||
| final var checkpointKey = checkpointKey(config); | ||
|
|
||
| return _checkpointsByThread.computeIfAbsent(threadId, k -> new LinkedList<>()); | ||
| return _checkpointsByThread.computeIfAbsent(checkpointKey, k -> new LinkedList<>()); |
There was a problem hiding this comment.
checkpointKey (BaseCheckpointSaver.java R48-49) builds the map key as "%s:%s".formatted(namespace, threadId) with no escaping or validation of either component. Since neither checkpointNamespace nor threadId is restricted, the pairs (namespace=tenant-a:x, threadId=b) and (namespace=tenant-a, threadId=x:b) produce the identical key tenant-a:x:b. A tenant who controls its own namespace string can therefore alias another tenant's entry in _checkpointsByThread and read, overwrite, or release() (delete) that tenant's checkpoints — a direct bypass of the tenant isolation this feature exists to provide. Fix: validate the namespace to a safe character set (as the documentation already promises) and either reject ":" in namespace/threadId or use an unambiguous encoding (e.g. length-prefixed or Base64URL of the namespace) when composing the key.
Risk Assessment
|
![unblocked-local-kaihao[bot]](https://avatars.githubusercontent.com/u/304571795?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found.
In reply to #140 (comment)
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 RunnableConfig withCheckpointNamespace(String checkpointNamespace) { | ||
| if (Objects.equals(this.threadId, checkpointNamespace)) { | ||
| return this; | ||
| } | ||
| return RunnableConfig.builder(this) | ||
| .checkpointNamespace(checkpointNamespace) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
The early-return guard compares this.threadId to the new checkpointNamespace, which is the wrong field. Following the established pattern in withCheckPointId (which compares this.checkPointId to the new value), this should compare this.checkpointNamespace.
The current code silently returns this (keeping the old namespace) whenever the thread ID happens to equal the requested namespace string, and always rebuilds when the namespace is actually unchanged. For example, a config with threadId("tenant-acme") calling withCheckpointNamespace("tenant-acme") would not apply the namespace at all.
public RunnableConfig withCheckpointNamespace(String checkpointNamespace) {
if (Objects.equals(this.checkpointNamespace, checkpointNamespace)) {
return this;
}
return RunnableConfig.builder(this)
.checkpointNamespace(checkpointNamespace)
.build();
}| private Path getNamespaceFolder(RunnableConfig config) { | ||
| return config.checkpointNamespace() | ||
| .map(targetFolder::resolve) | ||
| .orElse(targetFolder); | ||
| } |
There was a problem hiding this comment.
getNamespaceFolder passes the raw checkpointNamespace straight into targetFolder::resolve. The documentation added in this PR (core-library.md) states namespace values "are trimmed and must contain only letters, digits, hyphens, or underscores," but no trimming or validation is performed anywhere — not in Builder.checkpointNamespace, not in checkpointNamespace(RunnableConfig), and not here. A namespace like ../../etc or an absolute path would resolve outside targetFolder, and Files.createDirectories(outFilePath.getParent()) would happily create/resolve arbitrary directories. Since the PR's stated goal is tenant isolation, an application that forwards user-supplied tenant identifiers into checkpointNamespace is vulnerable to path traversal and cross-tenant file access.
Add validation (e.g. reject null/blank, trim, and require ^[A-Za-z0-9_-]+$) at the Builder.checkpointNamespace setter so the contract documented in core-library.md is actually enforced for every saver.
Based on AGENT.md
Risk AssessmentThe namespace setter has an equality check against |
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.