Skip to content

fix(core): make missing checkpoint threadId observable and optionally strict - #163

Merged
yuluo-yx merged 1 commit into
agentic-ai-java:mainfrom
Carl-Zhou-CN:fix/154-checkpoint-default-thread-namespace
Oct 15, 2026
Merged

yuluo-yx merged 1 commit into
agentic-ai-java:mainfrom
Carl-Zhou-CN:fix/154-checkpoint-default-thread-namespace

Conversation

@Carl-Zhou-CN

Copy link
Copy Markdown
Contributor

Contributing to ARGI: when RunnableConfig omits threadId, checkpoint savers now log a one-shot warning instead of silently sharing the "$default" namespace, and an opt-in strict policy (argi.checkpoint.requireExplicitThreadId=true or CheckpointThreadIdPolicy.setRequireExplicitThreadId(true)) makes checkpoint operations fail fast until an explicit threadId is provided; includes offline regression tests for MemorySaver and VersionedMemoryCheckpointSaver, README documentation of the shared $default semantics, and a lint-script skip for the machine-local .awr runtime directory. Fixes #154

… strict

When RunnableConfig has no threadId, BaseCheckpointSaver#checkpointThreadId
silently fell back to the shared $default namespace, so unrelated callers
on the same saver instance could read each other's checkpoints without any
log or error (issue agentic-ai-java#154, originally alibaba/spring-ai-alibaba#4975).

- Log a one-shot warning when a config falls back to the shared default
  namespace (scoped by app_name/user_id metadata when present).
- Add CheckpointThreadIdPolicy with an opt-in strict mode enabled via the
  argi.checkpoint.requireExplicitThreadId system property or
  setRequireExplicitThreadId(true); checkpoint operations then fail fast
  with IllegalArgumentException until the caller provides a threadId.
- Add offline regression tests covering the shared-bucket reproduction on
  MemorySaver, the strict policy, per-identity scoping with app/user
  metadata, and the shared rule across saver implementations.
- Document the $default sharing semantics and the strict-mode switch in
  the argi-graph-core README and the checkpointThreadId javadoc.

Fixes agentic-ai-java#154
@Carl-Zhou-CN
Carl-Zhou-CN force-pushed the fix/154-checkpoint-default-thread-namespace branch from f9dd7ba to 4553c6f Compare October 15, 2026 04:10

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: fix(core): make missing checkpoint threadId observable and optionally strict (#154)

Verdict: APPROVE

This is a clean, well-scoped fix for a real cross-caller data-sharing / observability gap (issue #154): an omitted threadId silently fell back to the shared $default bucket, so unrelated callers on the same saver instance could read each other's checkpoints.

What's good

  • checkpointThreadId is the single chokepoint and cannot be bypassed. I verified every saver implementation routes through it: MemorySaver, RedisSaver, MongoSaver, FileSystemSaver, VersionedMemorySaver, and MemoryLeasedCheckpointSaver call it directly; H2Saver/MysqlSaver/OracleSaver/PostgresSaver inherit it via AbstractJdbcCheckpointSaver (none override get/put/list). So the warning + strict IllegalArgumentException fire uniformly on every saver operation.
  • One-shot warning (AtomicBoolean, never floods logs) plus a clear strict-mode error that names the argi.checkpoint.requireExplicitThreadId property.
  • Solid regression suite: shared-bucket reproduction, strict rejects get/put/list/checkpointThreadId, strict accepts an explicit id, per-identity scoping, strict-rejects-even-with-metadata, cross-saver. @AfterEach restores the policy for test isolation.
  • README + javadoc document the $default sharing semantics and the strict switch.

Non-blocking nits

  1. Javadoc on setRequireExplicitThreadId(false) says it "restores the startup default derived from the property," but the implementation sets the field to the passed value unconditionally (does not re-read the property). Minor doc/behavior mismatch — the test's restore helper correctly re-reads it.
  2. requireExplicitThreadId is a JVM-global mutable static. Fine for tests, but for production prefer the system-property form (immutable at startup) so one caller can't flip it process-wide.
  3. The PR body mentions "a lint-script skip for the machine-local .awr runtime directory," but that change is not in the diff (only 4 checkpoint-related files). Confirm it landed in another PR or drop it from the description.
  4. migrateLegacyThread still computes legacyThreadId via config.threadId().orElse(THREAD_ID_DEFAULT) directly before validating via checkpointThreadId; a legacy config with no threadId won't trigger the new warning on that path. Minor / intended (migration path).

No CI checks were reported on the branch at review time; the included offline suite is sufficient.

@yuluo-yx
yuluo-yx merged commit 88ea050 into agentic-ai-java:main Oct 15, 2026
7 checks passed
yuluo-yx pushed a commit that referenced this pull request Oct 17, 2026
… strict (#163)

When RunnableConfig has no threadId, BaseCheckpointSaver#checkpointThreadId
silently fell back to the shared $default namespace, so unrelated callers
on the same saver instance could read each other's checkpoints without any
log or error (issue #154, originally alibaba/spring-ai-alibaba#4975).

- Log a one-shot warning when a config falls back to the shared default
  namespace (scoped by app_name/user_id metadata when present).
- Add CheckpointThreadIdPolicy with an opt-in strict mode enabled via the
  argi.checkpoint.requireExplicitThreadId system property or
  setRequireExplicitThreadId(true); checkpoint operations then fail fast
  with IllegalArgumentException until the caller provides a threadId.
- Add offline regression tests covering the shared-bucket reproduction on
  MemorySaver, the strict policy, per-identity scoping with app/user
  metadata, and the shared rule across saver implementations.
- Document the $default sharing semantics and the strict-mode switch in
  the argi-graph-core README and the checkpointThreadId javadoc.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Missing threadId silently shares the default checkpoint namespace across callers

3 participants