Skip to content

feat(core): add checkpoint namespaces - #144

Open
kaihao-zhao wants to merge 3 commits into
mainfrom
codex/checkpoint-namespaces-review-fixture-v69
Open

kaihao-zhao wants to merge 3 commits into
mainfrom
codex/checkpoint-namespaces-review-fixture-v69

Conversation

@kaihao-zhao

Copy link
Copy Markdown

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.

@unblocked-local-kaihao unblocked-local-kaihao Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +167 to +174
public RunnableConfig withCheckpointNamespace(String checkpointNamespace) {
if (Objects.equals(this.threadId, checkpointNamespace)) {
return this;
}
return RunnableConfig.builder(this)
.checkpointNamespace(checkpointNamespace)
.build();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
    }

Comment on lines +71 to +75
private Path getNamespaceFolder(RunnableConfig config) {
return config.checkpointNamespace()
.map(targetFolder::resolve)
.orElse(targetFolder);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

@unblocked-local-kaihao

unblocked-local-kaihao Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Risk Assessment

Risk: high

FileSystemSaver resolves the namespace directly as a filesystem path without enforcing the documented character restrictions, allowing values such as ../ to escape the saver’s target directory and undermine isolation. hasCheckpointNamespace() also dereferences a null namespace when called on the default configuration.

Comment on lines +114 to +116
public boolean hasCheckpointNamespace() {
return checkpointNamespace != null || !checkpointNamespace.isBlank();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
public boolean hasCheckpointNamespace() {
return checkpointNamespace != null || !checkpointNamespace.isBlank();
}
public boolean hasCheckpointNamespace() {
return checkpointNamespace != null && !checkpointNamespace.isBlank();
}

@dev-unblocked dev-unblocked Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) returns RunnableConfig.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) calls config.updateMetadata(...), which rebuilds via new 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;
}

Comment on lines +114 to +115
public boolean hasCheckpointNamespace() {
return checkpointNamespace != null || !checkpointNamespace.isBlank();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
public boolean hasCheckpointNamespace() {
return checkpointNamespace != null || !checkpointNamespace.isBlank();
public boolean hasCheckpointNamespace() {
return checkpointNamespace != null && !checkpointNamespace.isBlank();
}

Comment on lines +14 to +16
public int checkpointCount(RunnableConfig config) {
return _checkpointsByThread.get(threadId(config)).size();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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:

Suggested change
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)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
if (Objects.equals(this.threadId, checkpointNamespace)) {
if (Objects.equals(this.checkpointNamespace, checkpointNamespace)) {

Comment on lines +71 to +75
private Path getNamespaceFolder(RunnableConfig config) {
return config.checkpointNamespace()
.map(targetFolder::resolve)
.orElse(targetFolder);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines +105 to +108
* @return the configured namespace, or {@code $default} when no namespace is set
*/
public Optional<String> checkpointNamespace() {
return ofNullable(checkpointNamespace);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +48 to +50
default String checkpointKey(RunnableConfig config) {
return "%s:%s".formatted(checkpointNamespace(config), threadId(config));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dev-unblocked

dev-unblocked Bot commented Oct 1, 2026

Copy link
Copy Markdown

Risk Assessment

Risk: high

The 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 hasCheckpointNamespace() (null causes an exception) and MemorySaver.checkpointCount() (uses the old, unqualified key).

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant