Repository navigation
feat(core): add checkpoint namespaces - #144
kaihao-zhao wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
2 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 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, but the sibling method withCheckPointId compares this.checkPointId to the new checkPointId. The intended guard is to return this only when the namespace is unchanged. As written, if a caller sets the namespace to a value equal to the current threadId, withCheckpointNamespace returns this and silently drops the requested namespace. Conversely, an unchanged namespace that differs from threadId produces an unnecessary copy. Compare against the namespace field instead.
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 resolves the raw namespace value directly under targetFolder via targetFolder::resolve. A namespace containing .. or path separators (e.g. ../../etc/cron.d) resolves outside targetFolder, and serialize then calls Files.createDirectories(outFilePath.getParent()) followed by Files.writeString, enabling arbitrary file write/read/delete. The added documentation in core-library.md states namespaces "are trimmed and must contain only letters, digits, hyphens, or underscores", but no validation or trimming exists anywhere in the builder, withCheckpointNamespace, or the saver. In the multi-tenant scenario this PR targets, tenant identifiers are frequently derived from external input, so the missing sanitization is exploitable.
Validate the namespace at the boundary (e.g. in Builder.checkpointNamespace / withCheckpointNamespace) against a safe pattern such as [A-Za-z0-9_-]+ after trimming, and reject ../empty values. At minimum, normalize and confirm the resolved folder stays within targetFolder before use:
private Path getNamespaceFolder(RunnableConfig config) {
return config.checkpointNamespace()
.map(ns -> {
Path resolved = targetFolder.resolve(ns).normalize();
if (!resolved.startsWith(targetFolder)) {
throw new IllegalArgumentException("Invalid checkpoint namespace: " + ns);
}
return resolved;
})
.orElse(targetFolder);
}
Risk Assessment
|
| public boolean hasCheckpointNamespace() { | ||
| return checkpointNamespace != null || !checkpointNamespace.isBlank(); | ||
| } |
There was a problem hiding this comment.
The || operator causes a NullPointerException whenever checkpointNamespace is null — which is the default state (the no-arg constructor at line 52 sets it to null). When the left operand checkpointNamespace != null is false, the || evaluates the right operand !checkpointNamespace.isBlank(), which dereferences the null reference and crashes.
Even if it didn't crash, the logic is inverted: || returns true for any non-null value (including blank/empty strings), defeating the "names a checkpoint namespace" intent described in the javadoc.
The operator should be && so the method returns true only when the namespace is both non-null and non-blank:
| public boolean hasCheckpointNamespace() { | |
| return checkpointNamespace != null || !checkpointNamespace.isBlank(); | |
| } | |
| public boolean hasCheckpointNamespace() { | |
| return checkpointNamespace != null && !checkpointNamespace.isBlank(); | |
| } |
There was a problem hiding this comment.
9 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 not copied by the Builder(RunnableConfig config) copy constructor (RunnableConfig.java:300-306), which copies threadId, checkPointId, nextNode, and streamMode but omits the namespace. Every config derived through that constructor silently loses its namespace:
AbstractCheckpointSaver.put(AbstractCheckpointSaver.java:88) returnsRunnableConfig.builder(config).checkPointId(...).build()— the config handed back after the first checkpoint has no namespace.CompiledGraph.updateRunnableConfigMetadata(CompiledGraph.java:321, called per node at line 953) callsconfig.updateMetadata(...), which rebuilds vianew RunnableConfig.Builder(this)on every node execution, so the running config loses its namespace after the first node.AsyncNodeGenerator(CompiledGraph.java:674) rebuilds the config at stream start, dropping the namespace before the first checkpoint is even written.
Consequently, during a real invoke/stream run with a namespaced config, addCheckpoint (CompiledGraph.java:404) and saver.get(config) (CompiledGraph.java:419) fall back to the $default namespace mid-run — tenant isolation silently stops working. The new tests only exercise the saver API directly, so they cannot catch this.
Fix 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;
this.nextNode = config.nextNode;
this.streamMode = config.streamMode;
}| public boolean hasCheckpointNamespace() { | ||
| return checkpointNamespace != null || !checkpointNamespace.isBlank(); |
There was a problem hiding this comment.
checkpointNamespace != null || !checkpointNamespace.isBlank() uses || instead of &&. When checkpointNamespace is null (the default for every existing config), the first operand is false, the second operand is evaluated, and null.isBlank() throws a NullPointerException. When the namespace is set but blank, the first operand is true and the method incorrectly reports true. The predicate should be a conjunction:
| public boolean hasCheckpointNamespace() { | |
| return checkpointNamespace != null || !checkpointNamespace.isBlank(); | |
| public boolean hasCheckpointNamespace() { | |
| return checkpointNamespace != null && !checkpointNamespace.isBlank(); | |
| } |
| public int checkpointCount(RunnableConfig config) { | ||
| return _checkpointsByThread.get(threadId(config)).size(); | ||
| } |
There was a problem hiding this comment.
_checkpointsByThread is now keyed by checkpointKey(config) ("namespace:threadId", see loadCheckpoints at MemorySaver.java:34-36), but checkpointCount looks up with the bare threadId(config). The lookup always misses, get(...) returns null, and .size() throws a NullPointerException on every call — even for a config with no namespace, since the stored key is "$default:<threadId>". It also ignores the namespace entirely. Use the same key the saver stores under:
| public int checkpointCount(RunnableConfig config) { | |
| return _checkpointsByThread.get(threadId(config)).size(); | |
| } | |
| public int checkpointCount(RunnableConfig config) { | |
| return _checkpointsByThread.getOrDefault(checkpointKey(config), new LinkedList<>()).size(); | |
| } |
| * @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 checks Objects.equals(this.threadId, checkpointNamespace), but the intent (matching the sibling withCheckPointId at line 159) is to return this only when the namespace is unchanged. As written, a config whose threadId happens to equal the requested namespace (e.g. threadId("tenant-globex").withCheckpointNamespace("tenant-globex")) returns this and silently keeps the old/absent namespace. Compare the field actually being replaced:
| 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.
config.checkpointNamespace().map(targetFolder::resolve) feeds the user-supplied namespace straight into Path.resolve with no validation. Path.resolve has two dangerous semantics here: an absolute namespace (e.g. "/tmp" or "C:\\x") replaces targetFolder entirely, and a relative namespace containing .. (e.g. "../shared") escapes the checkpoint root — checkpoint files for one tenant can then be written to or read from any directory the process can access. Since the namespace is also used verbatim by MemorySaver's flat key, the same unvalidated string reaches other savers. Validate the namespace before use (trim, reject blank, reject absolute paths and .. segments, and restrict to the documented charset [A-Za-z0-9_-]), e.g. in Builder.checkpointNamespace(...) or in BaseCheckpointSaver.checkpointNamespace(RunnableConfig).
| * @return the configured namespace, or {@code $default} when no namespace is set | ||
| */ | ||
| public Optional<String> checkpointNamespace() { | ||
| return ofNullable(checkpointNamespace); |
There was a problem hiding this comment.
The javadoc says "@return the configured namespace, or {@code $default} when no namespace is set", but the implementation returns ofNullable(checkpointNamespace) — i.e. Optional.empty() when no namespace is set. The $default fallback exists only in BaseCheckpointSaver.checkpointNamespace(RunnableConfig). Correct the doc to: "@return an {@code Optional} containing the configured namespace, or {@code Optional#empty()} if none 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 new documentation states "Namespace values are trimmed and must contain only letters, digits, hyphens, or underscores", but neither Builder.checkpointNamespace(...) nor any saver performs trimming or validation — the raw string is stored and, in FileSystemSaver, resolved directly as a filesystem path. Either implement the documented normalization/validation (which would also close the path-traversal hole in FileSystemSaver.getNamespaceFolder) or remove the claim from the docs.
| @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.
The new local is declared as namesapceFolder (transposed letters) and used under that spelling at lines 147, 157, 166, and 172. Rename it to namespaceFolder in all four places for readability.
| default String checkpointKey(RunnableConfig config) { | ||
| return "%s:%s".formatted(checkpointNamespace(config), threadId(config)); | ||
| } |
There was a problem hiding this comment.
checkpointKey builds the storage key as "%s:%s".formatted(checkpointNamespace(config), threadId(config)) with no escaping or validation of either component. Since both namespace and threadId are arbitrary user-supplied strings, the flat key is ambiguous: namespace "tenant-a" + threadId "shared:x" produces the same key "tenant-a:shared:x" as namespace "tenant-a:shared" + threadId "x". MemorySaver now uses this key for loadCheckpoints and releaseCheckpoints, so a tenant who controls its own namespace/threadId values can craft a config whose key collides with another tenant's key and read, overwrite, or release (_checkpointsByThread.remove) that tenant's checkpoints — a cross-tenant data access/destruction path that the namespace feature was added specifically to prevent.
Fix: use a separator/encoding that cannot appear in either component (e.g. URL-encode both parts, or use a length-prefixed key such as namespace.length() + ":" + namespace + threadId), or — better — enforce the documented namespace charset (letters, digits, hyphens, underscores only) and reject threadIds containing the separator.
Risk AssessmentThe filesystem saver resolves the namespace directly as a path without enforcing the documented character restrictions, so values containing path traversal or an absolute path can escape the target folder and undermine isolation. There are also direct defects in |
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.