ai-yang opened a new issue, #727:
URL: https://github.com/apache/rocketmq-dashboard/issues/727
### Baseline
`rocketmq-studio` at `bbf1b7e0cf25a5065ba049b5450cc8155569f710`.
### Problem
`ClusterService.updateClusterConfig` mutates the `ClusterVO` and
`ClusterConfigVO` returned by `ClusterRepository.findById` before it calls
`ClusterRepository.updateConfig`. Those objects are repository-owned live state
in the current in-memory implementation.
If `updateConfig` throws, the API call fails but the failed request has
already changed the stored object:
- an existing config is partially overwritten in place; and
- a cluster whose config was `null` retains the newly constructed config
object.
A later read can therefore expose values from an update that was reported as
failed.
### Deterministic reproduction
Two `ClusterServiceTest` cases use a synchronous Mockito `doThrow` from
`ClusterRepository.updateConfig` and inspect the same object returned by
`findById` after the exception:
1. Start with `flushDiskType=ASYNC_FLUSH` and `writeQueueNums=8`, request
`SYNC_FLUSH` and `16`, and make persistence throw. The stored config contains
`SYNC_FLUSH` and `16` after the failed call.
2. Start with a `null` config, request `ASYNC_FLUSH`, and make persistence
throw. The stored cluster contains a non-null config after the failed call.
The tests contain no sleeps, timers, network access, or concurrent
scheduling. In Java 21 they failed identically in five consecutive runs:
```text
Tests run: 2, Failures: 2, Errors: 0, Skipped: 0
expected: ASYNC_FLUSH
but was: SYNC_FLUSH
expected: null
but was: ClusterConfigVO(... flushDiskType=ASYNC_FLUSH ...)
```
Command:
```bash
mvn
-Dtest='ClusterServiceTest#updateConfigShouldNotMutateStoredConfigWhenRepositoryUpdateFails+updateConfigShouldLeaveNullStoredConfigWhenRepositoryUpdateFails'
test
```
### Expected behavior
If repository persistence fails, the cluster and its existing config should
remain exactly as they were before the request. Successful partial-update
behavior and the public API should remain unchanged.
### Root cause
The service uses `cluster.getConfig()` directly, applies every requested
setter to that live object, and calls `cluster.setConfig(config)` before the
repository update. The repository boundary therefore occurs after the
observable mutation rather than before it.
### Proposed scope
- Build a detached copy of the existing `ClusterConfigVO` (or a detached
empty config when it is null).
- Apply the partial update to that detached copy.
- Call `clusterRepository.updateConfig` before installing the new config on
the returned `ClusterVO`.
- Install/return the new config only after the repository call succeeds.
- Add regression coverage for both existing and null configs when the
repository throws.
The focused change should only need:
-
`server/src/main/java/org/apache/rocketmq/studio/cluster/broker/ClusterService.java`
-
`server/src/test/java/org/apache/rocketmq/studio/cluster/broker/ClusterServiceTest.java`
### Duplicate search
I searched open and closed issues and open, closed, and merged pull requests
for `ClusterService`, `updateClusterConfig`, cluster config persistence/update
failures, atomicity, repository failures, live-object mutation, and the
expected file names. I found no report or patch for this failure-atomicity
behavior.
Related work is distinct:
- #645 / the superseded #646 and #600 validate `flushDiskType` and stabilize
cluster identity/order; they do not handle repository exceptions or detach
config state.
- #678 / #679 add numeric request validation before the service is invoked.
- #680 / #681 fix frontend polling and mock-store update behavior.
- #457 introduced the cluster service and repository code.
--
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]
