zhang-arvin commented on PR #6997: URL: https://github.com/apache/shenyu/pull/6997#issuecomment-6051673919
Thanks @Aias00 @sunnysabor — the regression test now drives the close path, on head `5eb356b2d`. **Blocking item fixed: the test now fails without the fix.** `testInitializeRegistersSessionUnderReturnedSessionId` no longer stops at the map-key assertions (which, as you noted, pass on master too). It now fetches the stored `McpServerTransport` from `sessionTransports` by the returned ID, calls `closeGracefully().block()`, and asserts that `sessions`, `sessionTransports` and `ShenyuMcpExchangeHolder` are all cleared for that ID. Verified non-vacuous on this head: commenting out `transport.setSessionId(newSessionId)` (line 335) makes the test fail at `close() must remove the session from the sessions map` (`expected: <true> but was: <false>`); with the line present it passes. Local run: `ShenyuStreamableHttpServerTransportProviderTest` → 11 tests, 0 failures; Checkstyle clean. @sunnysabor thanks for running the close-path probe independently — that is the assertion I committed, so it should now serve as real regression coverage. On the non-blocking notes: I've left `cleanupInvalidSession`'s `ShenyuMcpExchangeHolder` leak and the other two creation paths (`createTemporarySessionAndProcess`, `createSessionAndRestoreId`) as-is to keep this PR scoped to the #6833 initialize-path fix; happy to take them in a follow-up. I've also changed the description to `Partially fixes #6833` so the TTL/eviction half of the issue is not lost on merge. -- 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]
