Close chat history service in AiService

[InAnYan]

Related issues and pull requests

Closes _____

PR Description

Steps to test

Checklist

• I own the copyright of the code submitted and I license it under the MIT license
• [.] I manually tested my changes in running JabRef (always required)
• [.] I added JUnit tests for changes (if applicable)
• [.] I added screenshots in the PR description (if change is visible to the user)
• [.] I added a screenshot in the PR description showing a library with a single entry with me as author and as title the issue number
• [.] I described the change in CHANGELOG.md in a way that can be understood by the average user (if change is visible to the user)
• [.] I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Close chat history service in AiService

🐞 Bug fix

Walkthroughs

Description

• Close chat history service in AiService.close() method
• Ensures proper resource cleanup and prevents resource leaks
• Aligns with existing service shutdown pattern

Diagram

flowchart LR
  AiService["AiService.close()"] -- "calls" --> chatHistoryService["chatHistoryService.close()"]
  AiService -- "calls" --> cachedThreadPool["cachedThreadPool.shutdownNow()"]
  AiService -- "calls" --> jabRefChatLanguageModel["jabRefChatLanguageModel.close()"]
  AiService -- "calls" --> jabRefEmbeddingModel["jabRefEmbeddingModel.close()"]

Loading

File Changes

1. jablib/src/main/java/org/jabref/logic/ai/AiService.java 🐞 Bug fix +2/-0

Add chat history service cleanup on shutdown

• Added chatHistoryService.close() call in the close() method
• Ensures chat history service is properly closed during AiService shutdown
• Maintains consistency with other service cleanup operations

jablib/src/main/java/org/jabref/logic/ai/AiService.java

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2)   📘 Rule violations (1)   📎 Requirement gaps (0)   🖥 UI issues (0)   🎨 UX Issues (0)

🐞\ ☼ Reliability (2)

📘\ ☼ Reliability (1)

1. Shutdown stops on exception 🐞 ☼

Description

AiService.close() now calls chatHistoryService.close() before shutting down the executor/models; if
chatHistoryService.close() throws while persisting MVStore data, the remaining shutdown steps are
skipped. This can leave thread pools/models/storages unclosed and can also make try-with-resources
call sites fail during cleanup.

Code

jablib/src/main/java/org/jabref/logic/ai/AiService.java[140]

+        chatHistoryService.close();

Evidence

AiService.close() performs shutdown in a linear sequence with no try/finally, so any exception from
the newly-added chatHistoryService.close() prevents cachedThreadPool.shutdownNow() and model/storage
closure. ChatHistoryService.close() persists and closes its MVStore-backed storage via
commit()/close() without exception handling; MVStoreBase directly calls mvStore.commit()/close(),
which may throw runtime exceptions (e.g., MVStoreException), making this a concrete abort path.

jablib/src/main/java/org/jabref/logic/ai/AiService.java[136-149]
jablib/src/main/java/org/jabref/logic/ai/chatting/ChatHistoryService.java[194-204]
jablib/src/main/java/org/jabref/logic/ai/util/MVStoreBase.java[55-65]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`AiService.close()` now calls `chatHistoryService.close()` early, but the method has no exception-safety. If `chatHistoryService.close()` throws (e.g., during MVStore commit/close), the rest of `AiService.close()` is skipped, leaving executor/models/stores unclosed.
### Issue Context
`ChatHistoryService.close()` performs persistence and then commits/closes its MVStore-backed storage. The MVStore wrapper (`MVStoreBase`) does not catch exceptions.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/ai/AiService.java[136-149]
- jablib/src/main/java/org/jabref/logic/ai/chatting/ChatHistoryService.java[194-204]
- jablib/src/main/java/org/jabref/logic/ai/util/MVStoreBase.java[55-65]
### Suggested fix
Refactor `AiService.close()` to always execute the remaining shutdown steps even if chat history persistence fails. For example:
- Wrap `chatHistoryService.close()` in a `try { ... } catch (Exception e) { ... }` (log) and continue, **or**
- Use a `try/finally` where `chatHistoryService.close()` is in the `try` and the remaining shutdown steps are in `finally`, optionally collecting and rethrowing a final exception with suppressed exceptions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. AiService.close() untested change 📘 ☼

Description

AiService.close() now closes chatHistoryService, changing shutdown behavior in
org.jabref.logic without any corresponding test update in this PR. This risks regressions (e.g.,
persistence/locking behavior) going undetected.

Code

jablib/src/main/java/org/jabref/logic/ai/AiService.java[140]

+        chatHistoryService.close();

Evidence

The compliance checklist requires test updates for behavioral changes in org.jabref.logic. The PR
adds a new shutdown step (chatHistoryService.close()) in AiService.close(), but no test changes
are present in the provided diff to validate the new behavior.

AGENTS.md
AGENTS.md
jablib/src/main/java/org/jabref/logic/ai/AiService.java[136-145]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`AiService.close()` now invokes `chatHistoryService.close()`, which is a behavioral change in the logic layer, but the PR does not add/update tests to verify this shutdown behavior.
## Issue Context
This change affects resource lifecycle (chat history persistence/commit + underlying storage close) and should be covered to prevent regressions.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/ai/AiService.java[136-149]
- jablib/src/test/java/org/jabref/logic/ai/AiServiceTest.java[1-200]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Chat history close race 🐞 ☼

Description

AiService.close() now triggers ChatHistoryService.close(), which reads and persists JavaFX
ObservableList chat histories from a non-JavaFX shutdown thread while chat histories are also
mutated by background tasks. This cross-thread access can race during shutdown and break persistence
or throw at runtime.

Code

jablib/src/main/java/org/jabref/logic/ai/AiService.java[140]

+        chatHistoryService.close();

Evidence

AiChatComponent executes aiChatLogic.execute(userMessage) via
BackgroundTask.wrap(...).executeWith(taskExecutor), and BackgroundTask’s call() runs off the
JavaFX thread. AiChatLogic.execute() mutates the ObservableList via chatHistory.add(...). During
application shutdown, JabRefGUI.stop() runs aiService.close() on a separate virtual thread (in
parallel with task executor shutdown), so ChatHistoryService.close() may persist while background
tasks are still adding messages. Persistence ultimately iterates the provided List via size/get in
MVStoreChatHistoryStorage.storeMessagesForMap(), which is unsafe against concurrent mutation of the
backing ObservableList.

jabgui/src/main/java/org/jabref/gui/JabRefGUI.java[485-546]
jabgui/src/main/java/org/jabref/gui/ai/components/aichat/AiChatComponent.java[335-373]
jablib/src/main/java/org/jabref/logic/util/BackgroundTask.java[183-213]
jablib/src/main/java/org/jabref/logic/ai/chatting/AiChatLogic.java[144-196]
jablib/src/main/java/org/jabref/logic/ai/chatting/ChatHistoryService.java[114-134]
jablib/src/main/java/org/jabref/logic/ai/chatting/chathistory/storages/MVStoreChatHistoryStorage.java[101-107]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ChatHistoryService.close()` persists chat histories backed by JavaFX `ObservableList`s while those lists are mutated by background tasks (`BackgroundTask.call()`), and `AiService.close()` is invoked from a non-JavaFX shutdown thread. This introduces a shutdown race that can break persistence or throw at runtime.
### Issue Context
- `AiChatLogic.execute()` mutates `ObservableList<ChatMessage>`.
- `AiChatComponent` runs `AiChatLogic.execute()` inside `BackgroundTask.wrap(...).executeWith(taskExecutor)`.
- `JabRefGUI.stop()` runs `aiService.close()` on a separate thread in parallel with other shutdown actions.
- `ChatHistoryService.close*` passes the live `ObservableList` to storage; `MVStoreChatHistoryStorage.storeMessagesForMap` iterates via size/get.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/ai/chatting/ChatHistoryService.java[114-134]
- jablib/src/main/java/org/jabref/logic/ai/chatting/ChatHistoryService.java[194-204]
- jablib/src/main/java/org/jabref/logic/ai/chatting/chathistory/storages/MVStoreChatHistoryStorage.java[101-107]
- jabgui/src/main/java/org/jabref/gui/ai/components/aichat/AiChatComponent.java[347-373]
- jablib/src/main/java/org/jabref/logic/util/BackgroundTask.java[183-213]
### Suggested fix
Make persistence operate on a snapshot rather than the live `ObservableList`:
- In `closeChatHistoryForEntry` / `closeChatHistoryForGroup`, create a defensive copy (e.g., `List.copyOf(chatHistoryManagementRecord.chatHistory())` or `new ArrayList<>(...)`) and pass that snapshot into `implementation.storeMessagesFor*`.
Optionally (if desired for stronger guarantees), introduce a `closed` flag / synchronization in `ChatHistoryService` to prevent concurrent mutation during close, or ensure close runs after chat tasks are stopped.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-free-for-open-source-projects]

1. Shutdown stops on exception 🐞 Bug ☼ Reliability

AiService.close() now calls chatHistoryService.close() before shutting down the executor/models; if
chatHistoryService.close() throws while persisting MVStore data, the remaining shutdown steps are
skipped. This can leave thread pools/models/storages unclosed and can also make try-with-resources
call sites fail during cleanup.

Agent Prompt

### Issue description
`AiService.close()` now calls `chatHistoryService.close()` early, but the method has no exception-safety. If `chatHistoryService.close()` throws (e.g., during MVStore commit/close), the rest of `AiService.close()` is skipped, leaving executor/models/stores unclosed.

### Issue Context
`ChatHistoryService.close()` performs persistence and then commits/closes its MVStore-backed storage. The MVStore wrapper (`MVStoreBase`) does not catch exceptions.

### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/ai/AiService.java[136-149]
- jablib/src/main/java/org/jabref/logic/ai/chatting/ChatHistoryService.java[194-204]
- jablib/src/main/java/org/jabref/logic/ai/util/MVStoreBase.java[55-65]

### Suggested fix
Refactor `AiService.close()` to always execute the remaining shutdown steps even if chat history persistence fails. For example:
- Wrap `chatHistoryService.close()` in a `try { ... } catch (Exception e) { ... }` (log) and continue, **or**
- Use a `try/finally` where `chatHistoryService.close()` is in the `try` and the remaining shutdown steps are in `finally`, optionally collecting and rethrowing a final exception with suppressed exceptions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[Siedlerchr]

I think the qodo comment is valid

[InAnYan]

Well, yeah, but the same thing can be applied to every line in the close method, isn't it?