X-LightYear opened a new issue, #4552: URL: https://github.com/apache/rocketmq-dashboard/issues/4552
### Before Creating the Bug Report - [x] I have searched the [open issues](https://github.com/apache/rocketmq-dashboard/issues) of this repository and believe that this is not a duplicate. - [x] This is a defect in RocketMQ Studio, not a usage question and not a defect in another Apache RocketMQ repository. - [x] I can reproduce this on the current `master` branch, or I have stated the exact version I am running below. ### Studio Version branch: master git commit id: d50ffecc9d7e8f8f46da64198831bd7952e6974e deployed as: docker compose ### Runtime Environment OS: Windows 11 MySQL: not required for the deterministic regression test browser: Google Chrome 127.0.6533.120 (64-bit) on Windows 11 ### Connected RocketMQ Cluster RocketMQ version: 5.5.0 access mode: not transport-specific; the defect occurs after the tool handler has completed deployment: docker compose from deploy/rocketmq ### Build Toolchain JDK: JetBrains Runtime OpenJDK 21.0.7 Maven: 3.9.10 ### Describe the Bug A successful MCP mutation can be reported as failed when the post-execution audit write throws an exception. `ToolAuditFilter` first allows the mutation handler to complete successfully and then records a `SUCCESS` audit entry. If `AuditService.record` throws at that point, the exception escapes from the filter. `ToolExecutionService` translates the audit exception into `UNEXPECTED_EXECUTION_FAILURE`, and `McpToolRegistrar` returns an MCP `CallToolResult` with `isError=true`. At this point the underlying mutation has already completed, so the returned error no longer reflects the actual operation state. A caller may retry an operation that has already succeeded. ### Steps to Reproduce 1. Execute an MCP mutation such as `rmq.topic.update` with a valid `confirm_token`. 2. Allow the mutation handler to complete successfully. 3. Make the post-execution audit persistence call throw, for example because the audit store is unavailable. 4. Observe the MCP result. The issue can also be reproduced deterministically without external services using a real `ToolFilterChain`, `ToolExecutionService`, and `McpToolRegistrar`, together with: - a counting mutation handler; - an `AuditService` mock that throws while recording the successful result. Regression test: `McpToolRegistrarTest.returnsSuccessfulMcpResultWhenMutationCompletesBeforeAuditPersistenceFailsTest` Command: `mvn "-Dtest=McpToolRegistrarTest#returnsSuccessfulMcpResultWhenMutationCompletesBeforeAuditPersistenceFailsTest" test` ### What Did You Expect to See? Once the mutation handler has completed successfully, the MCP result should report success and return the completed mutation result. Audit persistence is observational in the existing Studio architecture. A failure while recording the audit entry should not convert an operation that has already completed into an API/tool failure. If fail-closed auditing is required, that decision would need to happen before executing the mutation rather than after the side effect has completed. ### What Did You See Instead? Once the mutation handler has completed successfully, the MCP result should report success and return the completed mutation result. Audit persistence is observational in the existing Studio architecture. A failure while recording the audit entry should not convert an operation that has already completed into an API/tool failure. If fail-closed auditing is required, that decision would need to happen before executing the mutation rather than after the side effect has completed. ### Additional Context The failure path is: `McpToolRegistrar.callTool` → `ToolExecutionService.executeInternal` → `ToolAuditFilter.filter` → `ToolMutationFilter` → mutation handler succeeds → `SUCCESS` audit recording throws → `UNEXPECTED_EXECUTION_FAILURE` → MCP `isError=true` The repository already establishes best-effort audit semantics in `OperationAuditService`: > Audit is observational. A failed sink must not turn an operation that already completed at a broker or provider into an API failure that callers may retry. Related historical fixes apply the same principle in other modules: - #1134 / #1135: metadata and Broker configuration mutations - #1457 / #1458: saved general settings Those changes do not cover the MCP `ToolAuditFilter` execution path. A deterministic regression test was added locally: `server/src/test/java/org/apache/rocketmq/studio/ops/ai/mcp/McpToolRegistrarTest.java` Test: `returnsSuccessfulMcpResultWhenMutationCompletesBeforeAuditPersistenceFailsTest` Related existing MCP/tool tests remain green: - MCP authentication/result mapping - mutation filtering - executor invocation - instance routing 18 related tests pass with Checkstyle reporting 0 violations. The expected fix scope is limited to MCP tool audit handling and regression coverage. No RocketMQ protocol change is required. ### Are You Willing to Submit a Pull Request? - [x] Yes, I am willing to submit a pull request. -- 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]
