ai-yang opened a new issue, #726:
URL: https://github.com/apache/rocketmq-dashboard/issues/726
## Summary
`LlmConfigService.saveConfig` publishes the new configuration to its
in-memory `overrides` field before `SettingsService.saveGeneralSettings`
succeeds. If the settings write throws, the save request fails but the rejected
configuration remains active in the running process.
This makes the operation non-atomic: callers observe a failure while
subsequent config reads, model discovery, and LLM requests can use values that
were never successfully saved.
## Affected baseline
- Branch: `rocketmq-studio`
- Commit: `bbf1b7e0cf25a5065ba049b5450cc8155569f710`
- Java: 21
## Reproduction
The failure is deterministic with a synchronous exception from the settings
boundary:
```java
@Test
void saveConfigShouldKeepCurrentConfigWhenPersistenceFails() {
doThrow(new IllegalStateException("persistence failed"))
.when(settingsService).saveGeneralSettings(any(GeneralSettingsVO.class));
assertThatThrownBy(() ->
llmConfigService.saveConfig(LlmConfigVO.builder()
.provider("deepseek")
.apiKey("sk-deepseek")
.apiBase("https://api.deepseek.com/v1";)
.model("deepseek-chat")
.maxTokens(8192)
.temperature(0.2)
.enabled(true)
.build()))
.isInstanceOf(IllegalStateException.class)
.hasMessage("persistence failed");
assertThat(llmConfigService.getConfig().getProvider()).isEqualTo("openai");
assertThat(llmConfigService.getConfig().getModel()).isEqualTo("gpt-4o");
}
```
Run with:
```text
JAVA_HOME=<jdk21> mvn -o \
-Dtest=LlmConfigServiceTest#saveConfigShouldKeepCurrentConfigWhenPersistenceFails
\
test
```
The test failed 5/5 times at the intended business assertion, with no test
errors:
```text
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
expected: "openai"
but was: "deepseek"
```
No sleeps, timers, network calls, or probabilistic scheduling are involved.
## Root cause
`saveConfig` currently executes these steps in this order:
1. normalize and validate the request;
2. assign `overrides = copy(normalized)`;
3. load the current general settings;
4. call `settingsService.saveGeneralSettings(...)`.
`getConfig` gives `overrides` precedence over the settings service.
Therefore any exception after step 2 leaves the new provider/model active even
though the caller receives a failed save result. A later restart or service
reconstruction can then expose the older stored configuration again.
## Expected behavior
Publishing the new in-memory configuration and saving the corresponding
general settings should be one logical operation. If loading or saving general
settings fails, `getConfig()` should continue returning the configuration that
was active before the failed call.
## Suggested fix
Build the complete settings value first, persist it through
`SettingsService`, and only publish a copied `overrides` value after the
persistence call succeeds. Keep the existing synchronization so another thread
cannot observe an intermediate state.
Add a regression test for the exception path in `LlmConfigServiceTest`.
Expected files:
-
`server/src/main/java/org/apache/rocketmq/studio/ops/ai/LlmConfigService.java`
-
`server/src/test/java/org/apache/rocketmq/studio/ops/ai/LlmConfigServiceTest.java`
This does not require an API or data-format change.
## Validation performed
- Deterministic reproduction: failed at the expected assertion 5/5 on Java
21.
- Maven Checkstyle: 0 violations.
- `git diff --check`: clean.
- The project does not configure SpotBugs in its POM. An additional SpotBugs
4.9.8.2 scan reported 168 existing repository-wide findings; none were
attributed to `LlmConfigService` (`ClassStats bugs='0'`). The research diff
changes test code only.
## Duplicate search
I searched open and closed issues, open/closed/merged pull requests,
commits, the class name, method names, error semantics, and the expected files
using terms including `LlmConfigService`, `saveConfig`, `saveGeneralSettings`,
`LLM config save`, `persistence failure`, `atomic`, and `rollback`.
No existing item covers this failed-save state leak. Related but distinct
items are:
- #651 (merged): introduced the current real LLM gateway/config flow but
does not cover settings-write failure atomicity.
- #536 (merged): prevents exposing LLM API keys; unrelated to failed-save
state publication.
- #693 (closed, not merged) / #665: provider-pattern refactoring; no
failed-save atomicity fix.
- #545 (closed): broader LLM gateway work and explicitly different
persistence scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
