Skip to content

Make Neo4jChatMemoryRepository.saveAll atomic - #6798

Open
dlwldn30 wants to merge 1 commit into
spring-projects:mainfrom
Goatshave:neo4j-atomic-save-all
Open

Make Neo4jChatMemoryRepository.saveAll atomic#6798
dlwldn30 wants to merge 1 commit into
spring-projects:mainfrom
Goatshave:neo4j-atomic-save-all

Conversation

@dlwldn30

Copy link
Copy Markdown

Problem

Neo4jChatMemoryRepository.saveAll() replaces a conversation by calling
deleteByConversationId() first. That method opens its own session, runs the
two delete statements in a transaction and commits. Only then does saveAll()
open a second session to write the replacement messages.

If that write fails, the delete is already committed, so the conversation is
left with neither its previous messages nor the new ones.

JdbcChatMemoryRepository already runs the equivalent delete-and-insert flow
inside a TransactionTemplate. The same defect on the MongoDB repository is
tracked in #6770.

Solution

Extract the delete statements into
deleteConversation(SimpleQueryRunner, String) and run them inside the same
write transaction as the inserts. deleteByConversationId() keeps its own
transaction and reuses that method, so its behaviour is unchanged.

Both statements are MATCH ... DETACH DELETE, and the inserts derive
msg.idx from a count taken inside the transaction, so the combined unit of
work stays safe under the driver's retry behaviour.

Testing

Neo4jChatMemoryRepositoryIT.saveAllKeepsTheExistingConversationWhenAWriteFails
saves two messages, then attempts a replacement whose second message cannot be
serialized — the same Optional-typed metadata that the existing
saveAssistantMessageWithOptionalMetadataFails relies on.

Reverting only the production change and keeping the test shows the
conversation is wiped:

Expecting actual:
  []
to contain exactly (and in same order):
  ["First message", "Second message"]

With the fix the two original messages survive the failed write.

./mvnw -Dmaven.build.cache.enabled=false -Pintegration-tests \
  -pl memory-repositories/spring-ai-model-chat-memory-repository-neo4j verify

32 tests run, 0 failures, 0 errors, 0 skips. The downstream
spring-ai-autoconfigure-model-chat-memory-repository-neo4j module passes too
(3 tests). spring-javaformat:apply and checkstyle:check reported no
violations.

Follow-up (not in this PR)

deleteByConversationId() is the only method here still using an unmanaged
transaction, so unlike the others it does not get the driver's retry behaviour
on transient failures. That is unrelated to atomicity, so I left it out — happy
to follow up separately if you would like it aligned with the rest of the class.


Contributed on behalf of Goatshave.

saveAll() removed the existing conversation through
deleteByConversationId(), which opens its own session and commits, and
only then opened a second session to write the replacement messages. If
that write failed, the conversation was left with neither its previous
messages nor the new ones.

Run the delete and the inserts in a single write transaction by
extracting the delete statements into a method that takes the runner to
execute them on. deleteByConversationId() keeps its own transaction and
reuses that method.

JdbcChatMemoryRepository already wraps the equivalent delete-and-insert
flow in a TransactionTemplate. The same defect on the MongoDB
repository is tracked in spring-projects#6770.

Signed-off-by: dlwldn30 <dlwldn30@naver.com>
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.

2 participants