Skip to content

fix(redis): publish complete chat-memory snapshots atomically - #73

Open
PoisonGodv wants to merge 2 commits into
agentic-ai-java:mainfrom
PoisonGodv:fix/51-redisson-serialization-preflight
Open

PoisonGodv wants to merge 2 commits into
agentic-ai-java:mainfrom
PoisonGodv:fix/51-redisson-serialization-preflight

Conversation

@PoisonGodv

@PoisonGodv PoisonGodv commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Describe what this PR does / why we need it

Concurrent Redis chat-memory saves execute deletion and list insertion separately. Readers can observe an empty gap, concurrent writers can mix snapshots, and a rejected write can leave the old history deleted. Redisson also needs to finish serialization before accessing Redis.

Publish complete replacement snapshots atomically across Jedis, Lettuce, and Redisson while retaining the existing conversation keys and ordered list format.

Does this pull request fix one issue?

NONE. Refs #51. This submits the three-client snapshot implementation for review; it does not merge application-level read-modify-write operations or guarantee rollback after a timeout.

Describe how you did it

Keep all argument validation and complete message serialization ahead of Redis mutations, including the Redisson preflight. Share one Lua script that builds a uniquely named temporary list in bounded chunks and publishes it with RENAME. Readers continue using the original conversation key. Empty snapshots use a single DEL.

Lua does not roll back earlier commands on error. Check that the temporary name is unused and verify cleanup permission before writing. Catch staging/publication command rejections and remove that invocation's temporary list before returning the error. There is no client-side staging interval, lease, or repository-added timeout retry.

Match the temporary key to the destination's actual Cluster slot, including untagged, brace-containing, and Unicode names. Redisson retains its configured value codec and calculates the slot from its mapped destination name; passing the temporary key as raw bytes prevents a second NameMapper transformation.

Preserve the successful-save TTL policy: nonempty replacement removes the previous expiration, and empty replacement deletes the key. Local failures and command rejections before publication preserve old history and expiration, subject to natural expiry. Concurrent saves remain whole-snapshot last-publisher-wins. Public APIs and dependency versions are unchanged.

Describe how to verify it

Use Java 21, as in repository CI, with Docker or an explicitly isolated Redis:

./mvnw -B -pl memory-repository/argi-model-chat-memory-repository-redis -am \
  -Dtest=RedisSnapshotIntegrationTest,RedissonSaveAllFailureTest,RedissonSaveAllIntegrationTest \
  -Dredis.test.host=127.0.0.1 -Dredis.test.port=<port> test
./mvnw -B -pl memory-repository/argi-model-chat-memory-repository-redis -am \
  -Dredis.test.host=127.0.0.1 -Dredis.test.port=<port> test

Run the identical snapshot contract against a real Cluster with three reachable master nodes:

./mvnw -B -pl memory-repository/argi-model-chat-memory-repository-redis -am \
  -Dtest=RedisSnapshotIntegrationTest \
  '-Dredis.test.cluster=<host1:port1>,<host2:port2>,<host3:port3>' test

The explicit endpoints must be test-owned: command-rejection cases create/remove temporary ACL users on each node. Without an endpoint the tests use Testcontainers and skip when Docker is unavailable.

Verified local results with Temurin 21 and Redis 8.2.2:

  • Complete Redis memory module: 152 tests, zero failures, errors, or skips, including the existing TLS and namespace suites.
  • Same snapshot contract on a real three-master Cluster with all 16384 slots assigned: 12 tests, zero failures, errors, or skips. The cases exercise all three clients, concurrent readers/writers, legacy raw records and all writer/reader pairings, ordered replacement in chunks, TTL/empty snapshots, serializer failures, ACL command rejections, staging collisions, and custom Redisson codec/name mapping. This is sampled key coverage on a complete Cluster topology, not a claim that every slot was exercised.
  • Identical final regression source on the published serialization-preflight baseline: 12 tests, 3 assertion failures and 1 assertion-related error, zero skips. All three clients lose old history after denied RPUSH; a concurrent reader observes an empty snapshot. The final implementation passes those assertions.
  • Root/module Checkstyle: zero violations. Scoped Spring JavaFormat 0.0.39 validation passed for the new helper, new contract test, and modified preflight test. License-eye 0.9.0 repository check: 583 valid, 188 ignored, zero invalid.

Independent review checked the complete changed code and test source, raw failure traces, topology evidence, identical baseline/fixed test hashes, and byte-for-byte patch reconstruction. The reviewed implementation required no source changes, so the valid backend runs were reused. Whole-repository build, formatting, and lint were not run locally; a passing Java 17 runtime run is not claimed.

Special notes for reviews

Nonempty saves now require EVAL/EVALSHA, EXISTS, DEL, RPUSH, and RENAME permissions, plus access to both conversation keys and temporary names matching {argi-*}:snapshot:*. Existing ACLs restricted to only the conversation prefix need adjustment before upgrading. The README documents this requirement. No stored-data migration is needed for the verified UTF-8 configuration; other default JVM encodings were not certified.

A network timeout can leave publication outcome unknown even if the server completed the replacement. Cluster validation used three masters without replicas; failover, resharding, crash recovery, memory-limit behavior, and performance were not certified.

Serialize the complete replacement before accessing the Redis list so local serialization errors preserve existing history and expiration. Cover first/later message failures and real Redis replacement semantics.

Refs agentic-ai-java#51
Stage serialized messages in a unique same-slot list within one Lua invocation and publish with RENAME. Preflight cleanup permission and preserve old history on rejected staging/publication commands. Retain successful replacement TTL and empty-snapshot semantics.

Cover real Redis standalone and three-master Cluster behavior across Jedis, Lettuce and Redisson, including concurrent readers/writers, legacy bytes, ACL failures and custom Redisson codecs/name mapping. Document the scripting and temporary-namespace ACL requirement.

Refs agentic-ai-java#51
@PoisonGodv PoisonGodv changed the title fix(redis): preflight Redisson snapshot serialization failures fix(redis): publish complete chat-memory snapshots atomically Nov 4, 2026
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.

1 participant