[eugenels]

The sender can request durable acknowledgements by tier instead of a single
boolean. request_durable_ack accepts on, off, local, replicated and
local,replicated.

• local: the server emits STATUS_LOCAL_DURABLE_ACK (0x0E) frames once
  commits are fdatasync-durable on its disk. The sender trims its
  store-and-forward copy on them. Local durability survives power loss, not
  the loss of the server's disk.
• replicated: unchanged shipped behaviour — the server emits
  STATUS_DURABLE_ACK frames once commits reach the object store, and the
  sender trims on them.
• local,replicated: both streams; the sender trims on the replicated ack
  (the strongest requested guarantee) and receives local acks as early
  progress signals, exposed per table via
  CursorWebSocketSendLoop.getLocalDurableTableWatermark.

on keeps its shipped meaning as the legacy alias for the replicated tier: it
still travels as the literal true header value and expects the historical
enabled confirmation, so nothing changes on the wire for existing
configurations.

The grant is all-or-nothing. The server echoes the granted set verbatim (or
enabled for a legacy request); a partial or missing echo counts as a denial
and fails the connect with the mismatch error, which names the requested tier set
and what the server echoed. This is a behaviour
change worth noting: requesting a tier the server cannot serve now fails
at connect instead of silently proceeding without durable acks —
including explicit tokens sent to servers that predate them.

Binary compatibility is preserved: the nine public
QwpWebSocketSender.connect overloads that took the boolean flag remain as
delegating overloads (verified by ExportedApiCompatibilityTest), and
Sender.LineSenderBuilder.requestDurableAck(boolean) is unchanged. A new
requestDurableAck(CharSequence) overload accepts the tier tokens.

Server support for the local tier lands via questdb/questdb#7411 (branch
nw_adaptive_commit), which carries end-to-end tests driving this client
(QwpLocalDurableAckTest, QwpIngressServerRestartFuzzTest) and power-loss
tests at the server's processor level showing that the local ack never covers
a transaction the server's disk does not hold. The Enterprise tandem of that
branch is questdb/questdb-enterprise#1147, which composes the local and
replicated tiers on an eligible adaptive primary.

Test plan:

• unit coverage for the tier parsing, header/echo round-trip and trim
  routing (CursorWebSocketSendLoopDurableAckTest and siblings)
• WsSenderConfigHonoredTest covers the new config values
• ExportedApiCompatibilityTest verifies the restored boolean overloads
• the full client suite
• server-side integration tests exercise this client from the questdb/questdb
  branch

[eugenels]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mtopolnik]

[PR Coverage check]

😍 pass : 129 / 191 (67.54%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java	10	48	20.83%
🔵	io/questdb/client/Sender.java	11	19	57.89%
🔵	io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java	30	46	65.22%
🔵	io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java	2	2	100.00%
🔵	io/questdb/client/cutlass/qwp/client/QwpDurableAckMismatchException.java	12	12	100.00%
🔵	io/questdb/client/cutlass/http/client/WebSocketClient.java	13	13	100.00%
🔵	io/questdb/client/cutlass/qwp/client/DurableAckTiers.java	35	35	100.00%
🔵	io/questdb/client/cutlass/qwp/client/WebSocketResponse.java	16	16	100.00%

[eugenels]

Verdict: approve with comments. Level 3 review of this PR, client only. There are no Critical findings: 4 Moderate and 2 Minor, all from code or runs I checked myself.

I found no defect in the trim logic:

• With local,replicated, a local ack never trims.
• With local alone, the sender trims on local acks, and also on replicated acks, which give the stronger guarantee.
• A reconnect clears both watermark maps.
• on is byte-for-byte the same on the wire as before.
• The client's request and confirm tokens match what the server branch sends.

The PR title is fine. Only the description has problems (item 4).

Critical

None.

Moderate

1. The on setting is never tested inside the send loop or the builder

• Problem: no test runs the send loop with the value on produces.
• Net impact: a future slip could trim on users' data before upload, and nothing would catch it.
• Evidence: the full suite (3,749 tests) stays green with on broken, at 9d9aad51.

How the value travels:

• request_durable_ack=on and requestDurableAck(true) become the bitmask REPLICATED | LEGACY_TRUE (value 6).
• That value reaches the send loop and the orphan drainer (QwpWebSocketSender.java:5512 and :4163).
• The loop then sets durableAckMode = tiers != NONE (CursorWebSocketSendLoop.java:1074).

What the tests do:

• Every send-loop and drainer test builds with plain REPLICATED (value 2).
• The end-to-end on tests only check that close() or awaitAckedFsn finishes. Trimming on the ordinary commit ack would let them finish even sooner.

Two deliberate breaks, each run against the full suite, each left it at 0 failures:

• Line 1074 changed so the value 6 means "no durable ack". A probe confirmed that value 6 reached the loop in 8 test methods and in 3 orphan-drainer threads.
• Sender.java:2710 changed so requestDurableAck(true) loses the legacy bit. No test calls requestDurableAck(boolean) at all.

At base, the loop tests passed true, the same value production passed, so the real value was covered. The new integer encoding split the two.

This is Moderate, not Critical. Today's code is correct, and breaking it would take a future check like tiers == REPLICATED. That slip is plausible: this PR's own loop constructors map true to the plain REPLICATED value.

Fix:

• Build the durable-mode test loop with REPLICATED | LEGACY_TRUE, or run those tests with both values.
• Add a builder test: requestDurableAck(true) must send the header true, and requestDurableAck("local") must configure local.

2. Flaky both-tier integration test

• Problem: the test asserts on a value that is written after the one it waits for.
• Net impact: occasional false CI failures. Users are not affected.
• Evidence: a 200 ms pause in that gap makes it fail at 9d9aad51. It passes without the pause.

The response handler increments totalLocalDurableAcks (CursorWebSocketSendLoop.java:4408) before it writes the watermark (:4417, which writes at :2056). testEndToEndBothTiersLocalAckIsProgressOnly waits on the counter, then asserts getLocalDurableTableWatermark("trades") >= 0 (DurableAckIntegrationTest.java:312-322). If the I/O thread is paused between those two statements, the test fails with "local watermark tracks the fsync frontier". Base has neither this code nor this test.

Fix:

• Either increment the counter after the map update, or make the test wait on the watermark instead.
• Keep the extra rows the test sends after the local ack. With a bug that leaks local acks into the trim map, this test failed 5 runs out of 5 and the unit tests did not fail.
• A unit test would remove that reliance: both-tier loop, local ack, then commit ack, expect no trim.

3. Boolean overloads the project rule says not to add

• Problem: 18 unused boolean overloads were re-added to internal factories.
• Net impact: about 550 lines to maintain, with no effect on users.
• Evidence: a bytecode call inventory at 9d9aad51, plus the project rule in commit 09b61b60.

Where they are:

• 12 in QwpWebSocketSender.java, at :1126-1484 and :1598-1712.
• 6 in CursorWebSocketSendLoop.java:643-733.

Who calls them:

• Only two have callers, and both callers are tests.
• The 9-argument connect is called from SymbolDictRecycleSlotOwnershipTest and the new pin test.
• The 8-argument loop constructor is called from three test classes.
• Nothing in OSS or Enterprise, on any branch, calls any of them.

Client #109, which is part of the base, records the rule: "Only Sender and its builder are public API … widen the existing factory in place instead of adding an overload". design/entra-id-qwp-auth.md:186 says the same. The 26-argument boolean connectWithCredentialSupplier at :1670 brings back the exact signature #109 removed.

The set doesn't even achieve what it claims. These base signatures are gone anyway:

• the 27-argument boolean connectWithCredentialSupplier
• the three boolean BackgroundDrainer constructors
• WebSocketClient.setQwpRequestDurableAck

Fix:

• Delete the 12 overloads and the 6 constructors.
• Switch the test calls in the four affected classes to DurableAckTiers.NONE.
• Drop testBooleanConnectOverloadRequestsLegacyTier; testRequestHeaderCarriesConfiguredTierSet already checks the on token on the wire.

4. Inaccurate PR description

• Problem: four statements in the PR description are not true.
• Net impact: the squash commit would record false verification claims.
• Evidence: git history at 82b2e144 and 9d9aad51, plus a base test run.

The four statements:

• "Verified by ExportedApiCompatibilityTest". The same claim is in the test plan and in a comment at DurableAckIntegrationTest.java:141-142. feat(qwp): keep idle WebSocket connections alive with PINGs #109 deleted that test before this branch's base; the reference went stale when the branch merged main.
• "The nine public connect overloads". Base has ten.
• "Binary compatibility is preserved". The five base signatures listed in item 3 are gone. The project rule allows that, but the sentence says otherwise.
• "Now fails at connect instead of silently proceeding". Base already failed at connect when on was not confirmed: base testConnectStringOnRequiresServerSupport passes on base. The new values did not even parse at base. Nothing changed for existing configurations.

Fix:

• Say the Sender builder API is unchanged and the internal factories were retyped in place.
• Keep the sentence that on is unchanged on the wire.
• Replace the behaviour-change sentence with: the new explicit values are refused by servers that predate tiers, and on still works with them.
• Remove the references to the deleted test.

Minor

5. Stale docs

• Problem: public javadoc still describes durable ack as on only.
• Net impact: misleads readers who configure local. Behaviour is unaffected.
• Evidence: Sender.java at 9d9aad51.

The stale text:

• durableAckKeepaliveIntervalMillis (Sender.java:1949-1962). It says the keepalive is "effective only when request_durable_ack=on", but the ping runs for every tier set.
• awaitAckedFsn (Sender.java:283-298). It describes only on. With local, the target advances on the local fsync ack.
• Internal comments. These still say trimming happens only on STATUS_DURABLE_ACK:
  • BackgroundDrainerListener.java:34, :65
  • BackgroundDrainer.java:389
  • CursorWebSocketSendLoop.java:130-139, :290-296

6. Cosmetics

• Problem: a misplaced javadoc, plus imports and members out of order.
• Net impact: none.
• Evidence: source at 9d9aad51.

Where:

• TestWebSocketServer.java:393-422. The javadoc of setSuppressDurableAckHeader now sits on top of another method's javadoc, and setSuppressDurableAckHeader has none.
• Imports. New imports break sorted import blocks in CursorWebSocketSendLoop.java:37, CursorWebSocketSendLoopDurableAckTest.java:27 and CursorWebSocketSendLoopDurableAckFuzzTest.java:27.
• Constants and fields. STATUS_LOCAL_DURABLE_ACK (WebSocketResponse.java:90) breaks the sorted block of status constants. totalLocalDurableAcks (CursorWebSocketSendLoop.java:345) splits a sorted pair of counters.
• Test order. Four of the new tests and the new helpers in CursorWebSocketSendLoopDurableAckTest are out of alphabetical order.

Coverage map

The test gate passes. There is one coverage gap, item 1, and it is Moderate:

Change	Search	Failure link
The send loop's durable-ack mode for the on value (CursorWebSocketSendLoop.java:1074), and the requestDurableAck(true) mapping (Sender.java:2710)	grep of the test tree, plus a probe in the loop constructor over the full suite	none: the full suite stays green with either one broken

Summary

• Verdict: approve with comments. Please address the coverage gap (item 1), the flaky test (item 2) and the PR description (item 4). Removing the overloads (item 3) is recommended.
• Scope: the client diff moves no submodule pointer, so all of it was in scope. The server branch was read only to confirm what it sends on the wire.
• In-diff vs out-of-diff: everything is in the diff except the stale docs (item 5), which are unchanged lines made stale by the new tiers.
• Callers: every changed symbol was checked across the client, OSS master and its nw_adaptive_commit branch, and Enterprise main and its nw_adaptive_commit branch. No caller breaks.
• How the findings were checked: each runtime finding has a run at 9d9aad51 behind it: a forced pause for the flaky test, and full-suite runs with deliberate breaks for the coverage gap. The checkers got only neutral statements to test, with no severity or suggested fix.