[kafka1991]

QwpWebSocketSender.flushAndGetSequence() publishes the frame and only then checks the I/O loop for a latched error, so against a server that rejects the frame at once, flush() can throw the terminal error that frame caused. Five tests called flush() unguarded against such a server and failed intermittently with LineSenderServerException: server rejected batch: PARSE_ERROR: two in SymbolDictRecycleFsnContinuityTest, one in ConnectionHealthTest and the two poison-escalation tests in ServerErrorAckTerminalTest. IoThreadErrorSurfacedOnRowApiTest did not fail on that race, but its outer catch swallowed the early throw and the test passed without asserting anything.

Each test now catches the early throw at flush(), as ServerErrorAckTerminalTest already does since #111. The error is sticky, so the assertions that follow still observe it. The change is test-only.

[kafka1991]

Review of PR #119 (level 3): no findings, verdict approve

I reviewed head 72174f90 against base 28c84830. This is the full review level.

Scope:

• Test-only: 4 test files and 6 test methods change. QwpWebSocketSender.java is byte-identical at base and head.
• Submodules: no submodule pointer moves, so nothing outside this repo is in scope.
• Tandem PRs: none needed. The behaviour can be checked with unit tests and a loopback server; nothing changes against a real server or HA setup.
• PR title and description: the Conventional Commits title is fine. The description's claims are checked below.

The PR's claims, tested against the code

1. The race exists. flushAndGetSequence() sends the frame (sealAndSwapBuffer(), around line 2780), then checks for an error the I/O thread has recorded (cursorSendLoop.checkError(), line 2789). A server that rejects the frame immediately can therefore make flush() throw the error that frame caused.

2. The error is sticky. The recorded error (CursorWebSocketSendLoop.terminalError) is written once and never cleared, and checkError() rethrows that same object on every call. So the later table(), flush() and awaitAckedFsn() calls in these tests still throw.

   • close() does not throw it a second time once a caller has caught it (close0 drops an error identical to the one the user already caught).
   • The async error handler still receives it: dispatchError() runs on the I/O thread whether or not the user thread already caught the error.

3. Forced race reproduced. In scratch worktrees I added a 1 s sleep (behind a system property) just before that post-send checkError() call, then ran the tests on JDK 11 at both revisions:

   Run	Result
   Head, unmodified	8 of 8 green
   Head, forced race	7 of 7 green
   Base, forced race	5 errors, each LineSenderServerException thrown from the first flush(); the 2 other methods stayed green

   • The 5 base errors: ConnectionHealthTest failed with PARSE_ERROR, both poison-escalation tests with PROTOCOL_VIOLATION after 2 and 4 rejections, and both SymbolDictRecycleFsnContinuityTest methods with PARSE_ERROR.
   • The other 2: IoThreadErrorSurfacedOnRowApiTest and the test(qwp): tolerate early terminal error ACK in regression test #111 test, which already had the catch.

   These five match the PR's list exactly.

4. The vacuous pass in IoThreadErrorSurfacedOnRowApiTest. I added a deliberate bug: table() no longer surfaces the recorded error. Then I ran the test with the race forced.

   • Base: the test passes. Its outer catch swallows the early throw, so no assertion runs.
   • Head: the test fails on "table() must surface the I/O thread terminal failure…".

   So the change turns a test that couldn't fail into one that catches the bug it is named for.

Findings

• Critical: none.
• Moderate: none.
• Minor: none.

Tests

• No coverage gaps, because no production behaviour changes.
• Each catch is followed by checks that require the recorded error. They wait on getLastTerminalError() or the error handler's count, and assert the exact replay count, the "poisoned frame" message, the exact FSN span, or that table()/awaitAckedFsn() throws. So swallowing the early throw cannot make a test pass without asserting anything.
• The catches are inline, with no shared helper. That matches the test(qwp): tolerate early terminal error ACK in regression test #111 precedent and PrReviewRedTestsE2e, and no existing helper covers this.

Summary

• Verdict: approve. No open Critical findings and no coverage gaps.
• CI: JDK 8 build, test and javadoc pass, as do the JDK 25 and 26 compile smoke jobs. The three Azure build-and-test jobs (linux-x64, mac-aarch64, windows) were still pending at review time.
• Limits: no JDK 8 was available locally, so I ran the tests on JDK 11 and relied on CI for JDK 8. I couldn't find a CI log showing the original intermittent failures (the GitHub runs I checked don't include them; I didn't look at Azure history). The forced-race runs reproduce the mechanism directly instead.