Skip to content

fix(core): make JsonSchemaUtils schema generation thread-safe - #2796

Open
jnduan wants to merge 1 commit into
agentscope-ai:mainfrom
jnduan:fix/jsonschema-thread-safety
Open

fix(core): make JsonSchemaUtils schema generation thread-safe#2796
jnduan wants to merge 1 commit into
agentscope-ai:mainfrom
jnduan:fix/jsonschema-thread-safety

Conversation

@jnduan

@jnduan jnduan commented Aug 21, 2026

Copy link
Copy Markdown

AgentScope-Java Version

2.0.3-SNAPSHOT (latest main, commit 0de9ed43)

Description

Fixes #2795

JsonSchemaUtils shares a single static victools SchemaGenerator across the
JVM without synchronization. victools' SchemaGenerator is not thread-safe
(its JacksonModule keeps an unsynchronized Map<Class, BeanDescription>
introspection cache), so concurrent structured calls — e.g. multiple agents
running in parallel with the same target class — race on the shared generator
and can fail with a ConcurrentModificationException. The linked issue has the
full root-cause analysis, including the identical bug fixed in Spring AI
(spring-ai#6207, resolved with the same synchronized-guard approach).

Changes:

  • Added a private SCHEMA_LOCK and guarded the two
    schemaGenerator.generateSchema(...) call sites
    (generateSchemaFromClass / generateSchemaFromType). Public API is
    unchanged.
  • Class Javadoc now documents the thread-safety guarantee.
  • Added concurrent regression tests for both guarded methods: 12 threads ×
    240 tasks released simultaneously via CountDownLatch, alternating between
    multiple POJOs so the victools introspection cache starts cold. Any exception
    propagates through Future#get and fails the test; all results are
    validated.

Note: the pre-fix race window is narrow and cannot be reproduced
deterministically, so the tests are regression protection rather than a
reproducer.

mvn -pl agentscope-core test: 2277 tests, 0 failures.

Checklist

Please check the following items before code is ready to be reviewed.

  • Code has been formatted with mvn spotless:apply
  • All tests are passing (mvn test)
  • Javadoc comments are complete and follow project conventions
  • Related documentation has been updated (e.g. links, examples, etc.)
  • Code is ready for review

@CLAassistant

CLAassistant commented Aug 21, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@jnduan
jnduan force-pushed the fix/jsonschema-thread-safety branch 2 times, most recently from 88c2470 to 8882daa Compare August 21, 2026 07:20
@jnduan

jnduan commented Aug 21, 2026

Copy link
Copy Markdown
Author

The failing check is unrelated to this PR. I dug into the job log:

  • The error comes from agentscope-harness, in JsonSessionDefaultLocationTest.perUserPartitioning_viaSharedAgentRoutedByRuntimeContext
  • The test's own assertions actually passed; it failed in JUnit's @TempDir teardown: Failed to delete temp directory ... DirectoryNotEmptyException: .../bob — a cleanup race where an async session-store write lands after teardown starts deleting
  • Neither this PR (agentscope-core/JsonSchemaUtils) nor the harness module reference each other; locally agentscope-core (2,282 tests) and agentscope-harness (833 tests) both pass on the rebased head 8882daa8

Looks like a flaky test. Could someone with permissions re-run the failed jobs? Thanks!

@skrcode

skrcode commented Aug 23, 2026

Copy link
Copy Markdown

I ran JAIPilot Cloud against this exact PR head. It found one follow-up: cache generated JSON schemas by Class/Type so repeated structured-output and tool schemas do not serialize on SCHEMA_LOCK and rerun reflective generation.

In the managed sandbox, the identical 16-thread, 80,000-call workload over two repeated classes moved from a 20,323 ms median to 899 ms across five observations per side. Four behavior locks preserve schema equality, independent returned-map mutation, and null handling; downstream structured-output tests and mvn -pl agentscope-core -am verify passed.

PR directly onto this source branch: jnduan#1
Cloud-generated draft and full evidence: skrcode#2

The cache retains one entry per encountered Class/Type, so this is optional and the lifecycle assumption should be reviewed for dynamic class-loading environments. No absolute production-latency claim is intended.

@jnduan
jnduan force-pushed the fix/jsonschema-thread-safety branch from 15859a9 to 16a0623 Compare August 23, 2026 09:33
@jnduan

jnduan commented Aug 23, 2026

Copy link
Copy Markdown
Author

Thanks for the analysis! Schema caching is indeed the planned follow-up — this PR was deliberately kept to the minimal synchronization fix (single concern), and caching will be submitted as a separate enhancement once this lands. The per-class lifecycle note is a good point to factor into that work.

dargoner pushed a commit to dargoner/agentscope-java that referenced this pull request Aug 25, 2026
合并 PR agentscope-ai#2796,为共享 SchemaGenerator 的两个生成入口增加同步保护,并补充并发回归测试。
@jnduan
jnduan force-pushed the fix/jsonschema-thread-safety branch 5 times, most recently from 7761e0c to 186f878 Compare September 2, 2026 10:05
The shared static victools SchemaGenerator is not thread-safe: its
JacksonModule keeps an unsynchronized introspection cache, so concurrent
structured calls can fail with ConcurrentModificationException. Guard
both schemaGenerator.generateSchema(...) call sites with a dedicated lock.
@jnduan
jnduan force-pushed the fix/jsonschema-thread-safety branch from 186f878 to 35080eb Compare September 3, 2026 01:32
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]: JsonSchemaUtils shared SchemaGenerator is not thread-safe (ConcurrentModificationException under concurrent structured calls)

4 participants