Skip to content

Commit 9cc550f

Browse files
glasstigerclaude
andcommitted
Surface startup endpoint-policy failures
endpointPolicyFailureIsTerminal() latched a terminal only for an ORPHAN drainer, so a FOREGROUND sender retried auth, non-421 upgrade and durable-ack capability failures forever -- including before it had ever reached the server. An operator whose credentials are wrong then watched a mute sender buffer into store-and-forward until it filled and misreported the cause as "out of space". Two pre-existing tests pin that startup contract and went red: QuestDBFacadeCallbacksTest#testFacadeErrorHandlerReceivesAsyncIngestError asserts the errorHandler receives the async auth terminal, and QuestDBFacadeDrainerListenerTest asserts close() rethrows the foreground's latched capability-gap terminal. Key the terminal off hasEverConnected as well. Connectivity problems are the caller's problem during startup: SYNC/OFF startup reports them by throwing from build(); ASYNC has no caller left to throw at, so the latched terminal reaches the user through SenderErrorHandler and the close() rethrow. The constructor already seeds hasEverConnected = client != null, so SYNC/OFF arrives past initialization and never latches, and swapClient makes the flag sticky: once the wire has been up, store-and-forward owns the buffered data and every endpoint-policy failure is a transient to ride out (Invariant B), unchanged. A transport failure (a dead port) never consulted this predicate and still retries forever. Rewrite the three tests that asserted the opposite so they pin the restored contract, and cross-link testAsyncNoServerRetriesForeverNoTerminal as its transport-half twin. Reverting the predicate fails both pre-existing tests plus the two rewritten async-initial ones, while the post-start tests stay green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 0428fd8 commit 9cc550f

3 files changed

Lines changed: 93 additions & 38 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1772,8 +1772,31 @@ private void connectLoop(Throwable initial, String phase, long paceFirstAttemptM
17721772
phase, elapsedMs, attempts, lastMsg);
17731773
}
17741774

1775+
/**
1776+
* Whether an endpoint-policy failure (auth, non-421 upgrade, durable-ack
1777+
* capability gap) latches a terminal instead of retrying under Invariant B.
1778+
* <p>
1779+
* Two callers own a terminal, for different reasons:
1780+
* <ul>
1781+
* <li>An ORPHAN drainer: a sanctioned terminal that hands the slot back to
1782+
* its quarantine owner.</li>
1783+
* <li>A sender still INITIALIZING ({@code !hasEverConnected}): connectivity
1784+
* problems are the caller's problem during startup and must reach it, so
1785+
* an operator learns their credentials are wrong instead of watching a
1786+
* silent sender buffer forever. SYNC/OFF startup reports them by throwing
1787+
* from {@code build()}; ASYNC startup has no caller left to throw at, so
1788+
* the latched terminal reaches the user through {@code SenderErrorHandler}
1789+
* and a {@code close()} rethrow. The constructor seeds
1790+
* {@code hasEverConnected = client != null}, so SYNC/OFF (which is handed a
1791+
* live client) is already past initialization here and never latches.</li>
1792+
* </ul>
1793+
* Once a FOREGROUND sender has reached the server even once, initialization is
1794+
* over and store-and-forward owns the buffered data: every endpoint-policy
1795+
* failure is then a transient the drainer must ride out, never a producer-fatal
1796+
* terminal (Invariant B).
1797+
*/
17751798
private boolean endpointPolicyFailureIsTerminal() {
1776-
return reconnectPolicy == ReconnectPolicy.ORPHAN;
1799+
return reconnectPolicy == ReconnectPolicy.ORPHAN || !hasEverConnected;
17771800
}
17781801

17791802
/**
@@ -2287,7 +2310,10 @@ private void swapClient(WebSocketClient newClient) {
22872310
this.client = newClient;
22882311
// Sticky: once the wire is up, we've reached the server at least once
22892312
// for this sender's lifetime. Exposed to the owning sender for
2290-
// connection-state observability; reconnect policy does not depend on it.
2313+
// connection-state observability, and it ends initialization: from here
2314+
// on endpointPolicyFailureIsTerminal() stops latching endpoint-policy
2315+
// failures on a foreground sender and rides them out instead, because
2316+
// store-and-forward now owns the buffered data (Invariant B).
22912317
this.hasEverConnected = true;
22922318
if (old != null) {
22932319
try {

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/InitialConnectAsyncTest.java

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,21 @@
5858
public class InitialConnectAsyncTest {
5959

6060
@Test
61-
public void testAsyncAuthFailureRetriesForeverNoTerminal() throws Exception {
62-
// Server returns HTTP 401 on every upgrade attempt. Once ASYNC build()
63-
// has returned, store-and-forward owns the buffered rows: the I/O thread
64-
// must keep retrying rather than terminalizing the producer.
61+
public void testAsyncAuthFailureSurfacesTerminal() throws Exception {
62+
// Server returns HTTP 401 on every upgrade attempt. A rejection by ENDPOINT
63+
// POLICY before the sender has ever reached the server is a startup problem,
64+
// not a transient, so it must reach the caller: an operator with the wrong
65+
// credentials has to learn that, rather than watch a mute sender buffer into
66+
// SF until it fills and misreports the cause as "out of space". SYNC/OFF
67+
// startup reports it by throwing from build(); ASYNC has no caller left to
68+
// throw at, so it arrives on the SenderErrorHandler instead.
69+
//
70+
// Contrast testAsyncNoServerRetriesForeverNoTerminal: a dead port is a
71+
// TRANSPORT failure -- genuinely transient -- and retries forever even during
72+
// startup. And once the wire has been up even once, initialization is over
73+
// and store-and-forward owns the data, so the same 401 becomes a transient to
74+
// ride out (CursorWebSocketSendLoopForegroundReconnectPolicyTest
75+
// #testPostStartAuthFailureRetriesUntilCredentialsRecover).
6576
try (Always401Fixture fixture = new Always401Fixture()) {
6677
fixture.start();
6778
int port = fixture.getPort();
@@ -79,16 +90,17 @@ public void testAsyncAuthFailureRetriesForeverNoTerminal() throws Exception {
7990
QwpWebSocketSender wss = (QwpWebSocketSender) sender;
8091
awaitAtLeastOneConnectAttempt(wss);
8192

82-
sender.table("foo").longColumn("v", 1L).atNow();
83-
sender.flush();
84-
85-
Assert.assertFalse(
86-
"async 401 rejection must stay inside the retry loop",
87-
inbox.await(1_500, TimeUnit.MILLISECONDS));
88-
Assert.assertNull("no terminal SenderError may be delivered for an async 401",
89-
inbox.get());
93+
Assert.assertTrue(
94+
"an async 401 must surface a terminal to the errorHandler",
95+
inbox.await(5, TimeUnit.SECONDS));
96+
SenderError err = inbox.get();
97+
Assert.assertNotNull("a SenderError must be delivered for an async 401", err);
98+
Assert.assertEquals(SenderError.Policy.TERMINAL, err.getAppliedPolicy());
99+
Assert.assertEquals(SenderError.Category.SECURITY_ERROR, err.getCategory());
100+
Assert.assertTrue("the terminal must name the upgrade rejection, got: "
101+
+ err.getServerMessage(),
102+
err.getServerMessage().contains("ws-upgrade-failed"));
90103
Assert.assertFalse("no upgrade has succeeded yet", wss.wasEverConnected());
91-
awaitReconnectAttemptsAdvance(wss);
92104
} finally {
93105
closeQuietly(sender);
94106
}
@@ -102,8 +114,9 @@ public void testAsyncNoServerRetriesForeverNoTerminal() throws Exception {
102114
// (it may appear; the data is safe in SF), so the I/O thread retries
103115
// forever. reconnect_max_duration_millis is IGNORED as a give-up deadline:
104116
// no SenderError lands, the sender stays usable, and wasEverConnected()
105-
// stays false. Endpoint-policy failures are covered by
106-
// testAsyncAuthFailureRetriesForeverNoTerminal.
117+
// stays false. This is the TRANSPORT half of the startup contract; the
118+
// endpoint-POLICY half, which does surface, is
119+
// testAsyncAuthFailureSurfacesTerminal.
107120
int port = TestPorts.findUnusedPort();
108121
ErrorInbox inbox = new ErrorInbox();
109122
String cfg = "ws::addr=localhost:" + port

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopForegroundReconnectPolicyTest.java

Lines changed: 37 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -55,15 +55,17 @@ public class CursorWebSocketSendLoopForegroundReconnectPolicyTest {
5555
public final TemporaryFolder sfDir = TemporaryFolder.builder().assureDeletion().build();
5656

5757
@Test
58-
public void testAsyncInitialAuthFailureRetriesUntilCredentialsRecover() throws Exception {
59-
assertAsyncInitialForegroundRecovers(false,
60-
() -> new QwpAuthFailedException(401, "localhost", 1));
58+
public void testAsyncInitialAuthFailureSurfacesTerminalToTheCaller() throws Exception {
59+
assertAsyncInitialForegroundSurfacesTerminal(false,
60+
() -> new QwpAuthFailedException(401, "localhost", 1),
61+
"ws-upgrade-failed");
6162
}
6263

6364
@Test
64-
public void testAsyncInitialDurableAckMismatchRetriesUntilCapabilityRecovers() throws Exception {
65-
assertAsyncInitialForegroundRecovers(true,
66-
() -> new QwpDurableAckMismatchException("localhost", 1, "primary"));
65+
public void testAsyncInitialDurableAckMismatchSurfacesTerminalToTheCaller() throws Exception {
66+
assertAsyncInitialForegroundSurfacesTerminal(true,
67+
() -> new QwpDurableAckMismatchException("localhost", 1, "primary"),
68+
"durable-ack-mismatch");
6769
}
6870

6971
@Test
@@ -78,9 +80,18 @@ public void testPostStartDurableAckMismatchRetriesUntilCapabilityRecovers() thro
7880
() -> new QwpDurableAckMismatchException("localhost", 1, "primary"));
7981
}
8082

81-
private void assertAsyncInitialForegroundRecovers(
83+
/**
84+
* An endpoint-policy failure before the sender has EVER reached the server is a
85+
* startup problem, so it must reach the caller rather than retry silently: an
86+
* operator with wrong credentials has to learn that, not watch a mute sender
87+
* buffer forever. ASYNC startup has no caller left to throw at, so the terminal
88+
* is latched for {@code SenderErrorHandler} delivery and the {@code close()}
89+
* rethrow. Post-start the opposite holds -- see {@link #assertForegroundRecovers}.
90+
*/
91+
private void assertAsyncInitialForegroundSurfacesTerminal(
8292
boolean durableAck,
83-
FailureSupplier failureSupplier
93+
FailureSupplier failureSupplier,
94+
String expectedDetail
8495
) throws Exception {
8596
TestUtils.assertMemoryLeak(() -> {
8697
DropFirstConnectionHandler handler = new DropFirstConnectionHandler(durableAck, false);
@@ -111,20 +122,18 @@ private void assertAsyncInitialForegroundRecovers(
111122
appendFrame(engine, (byte) 1);
112123
loop.start();
113124

114-
await(() -> factory.attempts() >= 2 || loop.getTerminalError() != null,
115-
"async foreground did not retry the endpoint-policy failure");
116-
Assert.assertNull(
117-
"async initial endpoint-policy failures must not stop the producer",
118-
loop.getTerminalError());
125+
await(() -> loop.getTerminalError() != null,
126+
"an async initial endpoint-policy failure must latch a terminal");
127+
Assert.assertTrue("the terminal must name the endpoint-policy failure, got: "
128+
+ loop.getTerminalError().getMessage(),
129+
loop.getTerminalError().getMessage().contains(expectedDetail));
130+
// Never reached the server, so the terminal is a startup verdict.
119131
Assert.assertFalse(loop.hasEverConnected());
120-
121-
long target = appendFrame(engine, (byte) 2);
122-
factory.allowConnect();
123-
await(() -> engine.ackedFsn() >= target || loop.getTerminalError() != null,
124-
"async foreground did not deliver after endpoint policy recovered");
125-
Assert.assertNull(loop.getTerminalError());
126-
Assert.assertTrue(loop.hasEverConnected());
127-
Assert.assertEquals(target, engine.ackedFsn());
132+
// Latched on the FIRST rejection: retrying an endpoint-policy
133+
// failure at startup is exactly the silent-buffering regression
134+
// this pins. Attempts cannot climb once running flips false.
135+
Assert.assertEquals("a startup endpoint-policy failure must not be retried",
136+
1, factory.attempts());
128137
} finally {
129138
factory.allowConnect();
130139
loop.close();
@@ -133,6 +142,13 @@ private void assertAsyncInitialForegroundRecovers(
133142
});
134143
}
135144

145+
/**
146+
* The post-start twin of {@link #assertAsyncInitialForegroundSurfacesTerminal}:
147+
* handing the loop a live client seeds {@code hasEverConnected}, so the sender is
148+
* past initialization and store-and-forward owns the buffered data. Every
149+
* endpoint-policy failure is then a transient to ride out, never a terminal
150+
* (Invariant B).
151+
*/
136152
private void assertForegroundRecovers(boolean durableAck, FailureSupplier failureSupplier) throws Exception {
137153
TestUtils.assertMemoryLeak(() -> {
138154
DropFirstConnectionHandler handler = new DropFirstConnectionHandler(durableAck, true);

0 commit comments

Comments
 (0)