Repository navigation
feat(core): add checkpoint namespaces - #134
kaihao-zhao wants to merge 1 commit into
Conversation
![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.
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.
withCheckpointNamespace compares this.threadId against the incoming checkpointNamespace parameter. This is a copy-paste error; it should compare this.checkpointNamespace.
As-is, the method will:
- Skip the update when
threadIdhappens to equal the desired namespace (silently returning the wrong config). - Unnecessarily rebuild the config when the namespace is already set to the desired value.
| public RunnableConfig withCheckpointNamespace(String checkpointNamespace) { | |
| if (Objects.equals(this.threadId, checkpointNamespace)) { | |
| return this; | |
| } | |
| return RunnableConfig.builder(this) | |
| .checkpointNamespace(checkpointNamespace) | |
| .build(); | |
| } | |
| 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.
checkpointNamespace is resolved directly as a subdirectory of targetFolder with no validation. A value like ../../etc or ../other-tenant would escape the intended storage root, allowing one tenant to read/overwrite another tenant's checkpoints or arbitrary files.
The documentation (core-library.md) states "Namespace values are trimmed and must contain only letters, digits, hyphens, or underscores" but this constraint is never enforced in code. Validate in the builder or here that the namespace matches the documented pattern (e.g. [A-Za-z0-9_-]+) and verify that the resolved path is still under targetFolder:
private Path getNamespaceFolder(RunnableConfig config) {
return config.checkpointNamespace()
.map(ns -> {
if (!ns.matches("[A-Za-z0-9_-]+")) {
throw new IllegalArgumentException(
"Invalid checkpoint namespace: " + ns);
}
return targetFolder.resolve(ns);
})
.orElse(targetFolder);
}
Risk AssessmentThe new namespace is used directly as a filesystem path component, but the implementation does not enforce the documented character restrictions; values containing path traversal or absolute paths can escape the saver’s target folder. No schema changes are shown, so neither schema policy matches. |
|
@dev-unblocked 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.
In reply to #134 (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 @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.
What changed — the new checkpointNamespace field was added to Builder, but the copy constructor Builder(RunnableConfig config) (RunnableConfig.java:285-291) was not updated: it copies threadId, checkPointId, nextNode, streamMode — not checkpointNamespace.
Where it breaks — every code path that rebuilds a config goes through that copy constructor and silently loses the namespace:
CompiledGraph.java:674—AsyncNodeGenerator's constructor doesRunnableConfig.builder(config).checkPointId(null)at the start of everyinvoke/streamrun, sothis.config(used byaddCheckpointat CompiledGraph.java:837/914) has no namespace. All checkpoints written during a real graph run land in the default namespace, not the configured one.AbstractCheckpointSaver.java:88—putreturnsRunnableConfig.builder(config).checkPointId(...).build(), so the config handed back to callers (and toCompiledGraph.updateState, CompiledGraph.java:267-275) points at the default namespace after the first write.updateMetadata/withCheckPointId/withStreamMode(RunnableConfig.java:321, 148, 128),StateSnapshot.of(StateSnapshot.java:37) andSubCompiledGraphNodeAction(SubCompiledGraphNodeAction.java:64) all drop it too.
Impact — the feature only works when the saver is called directly with the original config (exactly what the new tests do, which is why they pass). In an actual graph execution, a namespaced thread's checkpoints are split between the tenant namespace and $default, breaking both isolation and resume.
Fix — add the missing copy in Builder(RunnableConfig config):
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;
}| * @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 the wrong field: this.threadId instead of this.checkpointNamespace. Two failures follow: (1) if the threadId happens to equal the requested namespace, the method returns this and the namespace is silently not changed; (2) if the config has no threadId, withCheckpointNamespace(null) (a request to clear the namespace) returns this, keeping the old namespace. Compare against the field the method is supposed to replace:
| 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.
What changed — getNamespaceFolder resolves the raw, user-supplied namespace string into the filesystem: config.checkpointNamespace().map(targetFolder::resolve).
Where it breaks — nothing validates the namespace (the docs at src/site/mkdocs/core/core-library.md:145 promise trimming and a [A-Za-z0-9_-] charset, but no such code exists). Path.resolve accepts ../ segments and absolute paths, so a namespace of ../../somewhere or /etc makes the saver read and write checkpoint files outside targetFolder. An empty-string namespace is also treated as "present" by ofNullable, diverging from the default-namespace behavior.
Impact — for a feature whose stated purpose is tenant isolation, a tenant-controlled namespace becomes an arbitrary file read/write primitive relative to the store root.
Fix — validate/normalize the namespace once, where it enters the system (e.g. in Builder.checkpointNamespace: trim, reject empty, and enforce the documented charset), which also makes the documented contract true.
| * @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.
checkpointKey joins namespace and threadId with ":" and no escaping. Since neither component is validated, namespace "a" + threadId "b:c" and namespace "a:b" + threadId "c" both produce the key "a:b:c". MemorySaver (MemorySaver.java:25 and :32) uses this key for both remove and computeIfAbsent, so two distinct tenants sharing the key read and write each other's checkpoint lists — the exact cross-tenant leak this feature exists to prevent. Use a separator that cannot appear in either component (and validate the namespace per the documented charset), or key the map by a namespace/threadId pair instead of a flat string.
| default String checkpointNamespace(RunnableConfig config) { | ||
| return config.checkpointNamespace().orElse(CHECKPOINT_NAMESPACE_DEFAULT); | ||
| } |
There was a problem hiding this comment.
What changed — this diff adds the checkpointNamespace accessor for savers, but not every saver site applies it.
Where it breaks — VersionedMemorySaver delegates list/get/put to the namespaced MemorySaver, but its own release (VersionedMemorySaver.java:172-180) keys _checkpointsHistoryByThread by raw config.threadId().orElse(THREAD_ID_DEFAULT). Two tenants using the same threadId therefore share one version history: versionsByThreadId/lastVersionByThreadId/getCheckpointsByVersion return one tenant's checkpoints to the other.
Fix — key the history map with checkpointKey(config) (or checkpointNamespace(config) + threadId) so version history is partitioned the same way the checkpoint lists are.
| /** | ||
| * 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, never the literal "$default". The $default fallback only exists in BaseCheckpointSaver.checkpointNamespace. Align the doc with the actual contract:
| * @return the configured namespace, or {@code $default} when no namespace is set | |
| * @return the configured namespace wrapped in an {@code Optional}, or an empty {@code Optional} 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 docs state: "Namespace values are trimmed and must contain only letters, digits, hyphens, or underscores." No code does this — Builder.checkpointNamespace (RunnableConfig.java:314-316) stores the string verbatim, and neither BaseCheckpointSaver.checkpointKey nor FileSystemSaver.getNamespaceFolder sanitizes it. This is not just a doc gap: the missing validation is what enables the path-escape in FileSystemSaver.getNamespaceFolder and the ":" key collisions in MemorySaver reported separately. Either implement the documented validation in the builder (preferred — it makes the doc true and closes both code issues) or correct the documentation.
| @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 "namespaceFolder", and the typo propagates to the Files.list call, the error log, and the backup path resolution (lines 157, 158 and 172 in this file). Rename the local to namespaceFolder at all four usages.
Risk Assessment
|
There was a problem hiding this comment.
1 issue 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.
Builder copy constructor silently drops checkpointNamespace
The copy constructor copies threadId, checkPointId, nextNode, and streamMode but omits checkpointNamespace. This means every with* method that clones via RunnableConfig.builder(this) (e.g. withCheckPointId, withStreamMode, withNextNode) will silently lose the namespace from the resulting config.
For example, calling config.withCheckPointId("cp-2") on a config that has checkpointNamespace = "tenant-a" will return a new config with checkpointNamespace = null, breaking tenant isolation.
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;
}
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.