[PR Coverage check]

😍 pass : 135 / 138 (97.83%)

file detail

M2/M3 confirmed against the diff source (setDesignatedTimestampName: the else if (!Chars.equals(...)) path skips the rowCount != 0 throw, so a same-name re-set while rows are buffered is a silent no-op — contradicting the javadoc's "any set while rows are buffered throws"). Here's the review.

Code Review: PR #76 — feat(qwp): add table options API to name the designated timestamp column

Reviewed at level 3 (full mission-critical pass): change-surface map, self-verification of all load-bearing wire-format/estimate logic, plus three parallel bug-hunters (cross-context callers, concurrency/resource/pool, fresh-context adversarial — the last cross-checked the client bytes against the parent-repo server decoder QwpMessageCursor.parseTableOptions).

Committed-binary gate: clean — no binary files in the diff.

This is a careful, well-tested PR. No critical or blocking defect. Every high-risk vector — trailer byte arithmetic, the UDP datagram estimate, store-and-forward replay, flag-bit allocation, header parsing, concurrency — holds up under byte-level verification against the golden-byte tests.

Moderate

M1 — The client emits FLAG_TABLE_OPTIONS + trailer unconditionally, even to servers that never advertised support. Confirm the server contract before merging ahead of the tandem PR. (in-diff, by-design, external dependency)
QwpUdpSender.sendEncodedPayload/appendTableOptionsForUdp and QwpWebSocketEncoder.finishMessage set the flag and append the trailer whenever a name is declared, without gating on the negotiated capability — warnIfTableOptionsUnsupported only logs. This is deliberate and correct for the client: store-and-forward frames must replay to arbitrary servers, so gating on a per-connection handshake is wrong. QwpConstants.java:54-64 documents the reliance on the server forward-compat guarantee: pre-support servers ignore unknown flag bit 0x20 and never read past the last table body (the trailer rides inside the existing payloadLength envelope). The fresh-context agent verified the current server decoder agrees byte-for-byte; the unverifiable part is a genuinely old server. Nothing to change here — just don't merge the client ahead of a verified server contract, and consider a mixed-version e2e test. If the guarantee holds, an old server silently creates timestamp and the log warning is the only signal; if it doesn't, frames get rejected.

Minor

M2 — Re-declaring the same name while rows are buffered is a silent no-op, contradicting the javadoc. (in-diff)
QwpTableBuffer.setDesignatedTimestampName: the else if (!Chars.equals(designatedTimestampName, columnName)) guard means an identical-name re-set skips both the rowCount != 0 check and the throw. Both the method javadoc and Sender.TableOptions.designatedTimestamp state "setting it while rows are buffered throws." Behavior is benign (idempotent; trailer size unchanged; the UDP base-estimate cache is only read at rowCount == 0), but the doc/behavior mismatch is real and untested. Fix: move the rowCount != 0 guard ahead of the equals-check, or soften the javadoc to "changing it while rows are buffered throws." One-line fix.

M3 — Stale explanatory comment in QwpUdpSender.TableOptionsImpl.designatedTimestamp. (in-diff)
The comment claims "Any accepted set requires rowCount == 0" — false for the M2 same-name path (accepted with rowCount > 0). The conclusion (no committedDatagramEstimate adjustment needed) still holds, but because a same-name set can't change the trailer size, not for the stated reason. Fix alongside M2.

Downgraded (verified false positives)

• UTF-8 length mismatch (validation vs encoding) — Utf8s.utf8Bytes (validation) and NativeBufferWriter.utf8Length are the same function, also what putString writes. Byte-exact.
• reset() wiping the sticky name — the null-assignment is in clear(), not reset(); sticky-across-flush contract holds (verified directly).
• FLAG_TABLE_OPTIONS = 0x20 collision — free bit (existing 0x01, 0x04, 0x08, 0x10).
• Header parser matching "10"/"21" as tag 1 — exact "1".equals(token.trim()), not a substring test; no infinite loop / OOB; reset to false before each upgrade.
• payloadStart promoted to a field — encoder is single-threaded, each entry point re-stamps it; pinned by the byte-identical golden test.
• UDP estimate under-counting the trailer — getSingleTableOptionsTrailerSize is byte-exact vs writeTableOptions+putInt, folded into the base estimate and invalidated on every rename; estimate >= actual holds.
• Concurrency / pool staleness / leak — capability volatiles gate only logging, never wire bytes; PooledTableOptions guards generation on every call; flyweights hold no native memory.
• Missing tableOptions() override — all three QWP senders override; ILP (AbstractLineHttpSender) inherits the throwing default; PooledSender delegates with a generation guard.

Test coverage

Strong. Golden-byte assertions on both transports, sticky/reset semantics, stale/retained-reference retargeting, estimate invalidation on rename, ILP rejection, header parser (present/absent/other-tag), mixed-presence and consecutive-split-message trailers, pool generation guard. The reflection in WebSocketClientTest/SenderLeaseGenerationTest reuses the file's existing pattern for package-private internals — acceptable. Only gap: M2's no-op path is untested (which is why the doc drift slipped through).

Summary

• Verdict: approve. No blocking issues. M2/M3 are one-line doc/comment fixes worth folding into this branch; M1 is a cross-repo release-ordering check, not a code change.
• 3 findings verified, 8 dropped as false positives after source verification.
• Split: 3 in-diff, 0 out-of-diff. Zero out-of-diff is a genuine result, not an underrun — the feature is additive and the trailer travels inside the existing payload-length envelope; every encoder/UDP-send caller (flushPendingRows, flushPendingRowsSplit, sendCommitMessage, all UDP paths) was traced and is safe.