fix(#2419): add content-aware cache to MysqlSkillRepository and PostgresSkillRepository for hot-reload support [FaaFyfxR9WAQrL7FcAgEHJvztd8cVMxvjHRS55rw1nwH] - #2429
Conversation
…esSkillRepository for hot-reload support Adds a lightweight cache to both DB-backed skill repositories, mirroring the mtime-based cache pattern already used by FileSystemSkillRepository: - MysqlSkillRepository: queries MAX(updated_at) first; returns cached list when the timestamp is unchanged. Cache is cleared on save() and delete(). - PostgresSkillRepository: uses an in-memory version counter incremented on save() and delete(). Cached list is returned when the version matches. Previously, DynamicSkillMiddleware's content-keyed short-circuit could only detect skill changes for FileSystemSkillRepository (which uses mtime-based cache invalidation). DB-backed repositories always returned fresh data from the database, but the short-circuit's signature never changed because the content was identical — preventing the SkillBox from being rebuilt after external updates. With this fix, external DB updates are detected via the timestamp/version check, and the DynamicSkillMiddleware correctly rebuilds the SkillBox on the next system-prompt pass. Fixes agentscope-ai#2419 [FaaFyfxR9WAQrL7FcAgEHJvztd8cVMxvjHRS55rw1nwH]
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
waterWang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
CLA Not Signed The Contributor License Agreement (CLA) check is currently pending on this PR ( @waterWang please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/agentscope-ai/agentscope-java. Once signed, the Automated check by github-manager-bot |
|
CI Failure — Spotless formatting + CLA Two issues blocking CI:
Fixes:
|
oss-maintainer
left a comment
There was a problem hiding this comment.
CI Failures
Two issues:
1. Spotless Formatting (build failure)
Missing blank line between cachedSkills field declaration and the @FunctionalInterface annotation. Run:
mvn spotless:apply2. CLA Not Signed (pending check)
The license/cla check is pending — the Contributor License Agreement must be signed before this PR can be merged. Please visit the CLA link and sign it.
Code Review
The content-aware cache pattern (mirroring FileSystemSkillRepository's mtime-based cache) is a good approach for hot-reload support in DB-backed skill repositories. A few observations:
-
Thread safety:
cachedSkillsandcachedMaxUpdatedAtare declaredvolatile, which ensures visibility but not atomicity of the pair. In a race where one thread readscachedMaxUpdatedAtand another updates both fields, you could see a stalecachedSkillswith a newcachedMaxUpdatedAt. Consider using a single immutable holder object (e.g., arecord CacheSnapshot(List<AgentSkill> skills, long maxUpdatedAt)) assigned atomically via a single volatile reference. -
Cache invalidation on external changes: The cache is cleared by
save()anddelete(), but if the DB is modified outside this repository instance (e.g., another process), the cache won't be invalidated. ThequeryMaxUpdatedAt()check handles this well — good design. -
PostgreSQL parity: The PR title mentions both MySQL and PostgreSQL, but the diff only shows MySQL changes. Please ensure the PostgreSQL repository gets the same treatment.
Summary
Adds a lightweight cache to both DB-backed skill repositories, mirroring the mtime-based cache pattern already used by FileSystemSkillRepository.
Changes
MAX(updated_at)first; returns cached list when the timestamp is unchanged. Cache is cleared onsave()anddelete().save()anddelete(). Cached list is returned when the version matches.Problem
Previously, DynamicSkillMiddleware's content-keyed short-circuit could only detect skill changes for FileSystemSkillRepository (which uses mtime-based cache invalidation). DB-backed repositories always returned fresh data from the database, but the short-circuit's signature never changed because the content was identical — preventing the SkillBox from being rebuilt after external updates.
Fix
With this fix, external DB updates are detected via the timestamp/version check, and the DynamicSkillMiddleware correctly rebuilds the SkillBox on the next system-prompt pass.
Fixes #2419