fix(qwp): repair sender state after rollback, clear, and flush

[jerrinot]

After rejecting an oversized timestamped row, a QWP sender could fail subsequent rows with a NullPointerException. This fix keeps the sender usable after rollback and ensures auto-flush and UDP size calculations remain accurate after buffers are cleared or flushed.

[jerrinot]

All four review agents plus my own trace converge: no correctness, concurrency, resource, performance, or accounting defect. The fresh-context adversarial agent independently ran the suites (108 tests) plus a custom probe of the one untested path and found nothing. Here is the verified report.

---

Review: PR #77 — fix(qwp): drop cached timestamp columns and repair accounting on buffer clear/rollback

Level 3 (full pass: 13 agent roles, per-finding source verification). No committed binaries. Both changed test suites pass locally (QwpWebSocketSenderTest 48/48, QwpUdpSenderTest 60/60, 0 skipped).

Critical

None.

Moderate

None. One latent, non-reachable observation is recorded under Minor.

Minor

1. Test method ordering violates the alphabetical convention (in-diff).

   • QwpWebSocketSenderTest.java:170,175 — testAtMicros/AtNanosRecoversAfterTableBufferClear are placed after testAtNowAfterCloseThrows:155; alphabetically both belong before it. :313,318 — the two testAt*RecoversAfterOversizeRowRollback sit after the testClearOf* block (before testBinaryColumnAfterCloseThrows:323), out of order.
   • QwpUdpSenderTest.java:283 — testTableBufferClearInvalidatesHeadroomBaseEstimate is wedged between testAt* methods.
   • Fix: move the six new testAt* methods into the testAt* run and keep the testClearOf* group together and internally sorted. Pure cosmetics; the skill's test-code rules do apply member ordering to tests.

2. firstPendingRowTimeNanos can point at removed rows after clearing a non-current buffer (QwpWebSocketSender.java:2274-2276). onTableBufferClear only resets the timer when pendingRowCount reaches exactly 0. If the cleared buffer held the oldest pending rows while other tables still have pending rows, the interval auto-flush may fire slightly early — strictly safe-side, never late, never data loss. Not reachable in production (production clear() runs only at sender close(); between-batch reuse goes through reset()), so this is a hardening note, not a bug. Would become live only if a future change invoked clear() on a live sender.

3. Missing Fixes #NNN in the PR body. CLAUDE.md wants it at the top when an issue exists; fine if none was filed.

4. Optional coverage gaps (correct by construction, untested): (a) WS oversize rollback when committed rows already exist and the rejected row introduces a new non-timestamp column (the surviving-ts, cache-relookup path); (b) clear() of the current buffer holding committed and in-progress rows simultaneously.

Downgraded (candidate concerns verified false)

• pendingBytes/pendingRowCount drift or negativity after clear/reset/rollback interleavings — false. pendingBytes == Σ(committedBytes − baselineBytes) telescopes from the same anchor sendRow() uses; committedBytes ≥ baselineBytes always; clear() is idempotent. Verified across double-clear, current-buffer-with-in-progress-row, post-flush, and switch-away-and-back sequences.
• baselineBytes goes stale when a column is created after reset() — false. The delta accounting only depends on current − baseline; rolled-back columns are always post-reset (index ≥ committedColumnCount) so never part of baseline.
• Dangling cached ColumnBuffer after rollback (regular columns / UDP in-progress state) — false. WS caches only the two timestamp columns (both nulled); regular columns are looked up per value; UDP InProgressColumnState.clear() nulls column (QwpUdpSender.java:1462).
• Constructor overload ambiguity / symbol-dict regression — false. (String, QwpWebSocketSender) binds for WS (global dict), (String, Owner) for UDP (local dict, identical to the pre-PR sender=null); the (QwpWebSocketSender) null cast disambiguates the 1-arg ctor.
• clear() reachable on a live sender / callback stale reads — false. Only reached via close(); callback fires before any state is released.
• Perf regression from the extra getBufferedBytes() walks in reset()/callback — false. Off the per-row path; O(columns) lost in the flush's existing O(columns×rows) encode.

Summary

Approve. This is a correct, tightly-scoped fix for a genuine native use-after-free: an oversized row that introduces a table's designated-timestamp column had that column closed by rollbackUncommittedColumns(), leaving cachedTimestampColumn pointing at freed off-heap memory so the next at() would addLong() into it. rollbackRow()/cancelRow() now null the caches, and the new Owner callback keeps byte/row accounting consistent when a buffer is cleared. The resetTableBuffersAfterFlush snapshot re-anchor (:3870) is an incidental but valid fix for a pre-existing over-count of var-width seed bytes in the auto-flush byte threshold.

• Draft findings: 6 candidate concerns verified and dropped as false positives; 4 minor items kept (1 style, 1 latent/unreachable, 1 metadata, 1 optional coverage). 0 Critical, 0 Moderate.
• In-diff vs out-of-diff: all kept findings are in-diff. The cross-context pass (Agent 9) walked every callsite of the changed symbols (clear, the constructors, rollbackRow, getTableBuffer, onTableBufferClear) and confirmed production clear() is close-only, so zero out-of-diff findings is correct here, not an under-run.
• Regression tests are genuine: all three helpers were traced to fail (NPE or wrong count/estimate) if the fix is reverted.

[mtopolnik]

[PR Coverage check]

😍 pass : 47 / 48 (97.92%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java	20	21	95.24%
🔵	io/questdb/client/cutlass/qwp/client/QwpUdpSender.java	15	15	100.00%
🔵	io/questdb/client/cutlass/qwp/protocol/QwpTableBuffer.java	12	12	100.00%

[jerrinot]

Review: PR #77 — fix(qwp): repair sender state after rollback, clear, and flush

Reviewing at level 3 (full pass: cross-context, fresh-context adversarial, and test-code agents, plus per-finding source verification). No committed binaries — all 5 changed files are text (3 production, 2 test).

What the PR fixes (verified real)

A genuine native use-after-free: when the first row on a table (or first after reset/clear/flush) sets a designated timestamp, atMicros()/atNanos() caches the newly-created timestamp ColumnBuffer. If sendRow()→nextRow() then rejects the row as oversize (throws before commit), at()'s catch calls rollbackRow()→rollbackUncommittedColumns(), which close()s (frees the off-heap memory of) that uncommitted column. Pre-fix, cachedTimestampColumn still pointed at freed memory, so the next at() did addLong() into it → NPE/corruption. The fix nulls the caches in rollbackRow()/cancelRow(), and adds the Owner.onTableBufferClear callback so byte/row accounting and UDP datagram/headroom estimates stay consistent when a buffer is cleared. I traced the full path (at():882 → atMicros:2970 → sendRow:4984 → nextRow; rollbackUncommittedColumns:414; getOrCreateDesignatedTimestampColumn) and confirm the bug and the fix.

Critical

None.

Moderate

None. One latent item is recorded below under Minor with its unreachability proof.

Minor

1. firstPendingRowTimeNanos can point at removed rows after clearing a non-current buffer that held the oldest pending rows — QwpWebSocketSender.java:2441-2443. onTableBufferClear only resets the timer when pendingRowCount hits exactly 0; if other tables still have pending rows, the interval auto-flush may fire slightly early. Strictly safe-side (never late, never data loss). Not reachable in production: production clear() runs only via close() (:1306); mid-session buffer reuse goes through reset(), which does not fire the callback. Only the @TestOnly getTableBuffer/currentTableBufferForTest hooks reach a live clear(). Would become live if a future change invoked clear() on a running sender. Hardening note, not a bug.

2. Test method ordering violates the alphabetical convention (in-diff). The file otherwise follows it strictly. QwpWebSocketSenderTest.java: the whole new block (171–374) was inserted contiguously between testAtNowAfterCloseThrows (156) and testBinaryColumnAfterCloseThrows (377); testAtMicros/AtNanosRecoversAfterTableBufferClear (171/176) sort before testAtNow*, the testClearOf* group belongs after testCancel*/before testClose* (~490), testFlushReanchors* belongs near testFlushAppendFailure (542), and testAtMicros/AtNanosRecoversAfterOversizeRowRollback (367/372) belong up in the testAt* run. The two private helpers are also swapped. QwpUdpSenderTest.java:278,283 likewise wedged into the testAt* block. Cosmetic; the skill's member-ordering rule does apply to tests.

3. Weak regression test — QwpWebSocketSenderTest.testClearOfEmptyVarWidthTableKeepsPendingBytesIntact (216). It clears a rowless non-current var-width buffer and asserts pendingBytes unchanged / pendingRowCount==1. With the callback present the subtraction is exactly baseline − baseline = 0; with the fix reverted it's a no-op — identical result either way, so it cannot fail on a behavioral revert and is redundant with testClearOfUnrelatedBufferIsLocalToThatBuffer. Either drop it or give table a a committed row so its clear must actually move the totals. Related: the b.clear() assertions in testClearOfUnrelatedBuffer... (204–205) are locality checks, not fix-guards (also subtract 0) — a one-line comment saying so would stop a future reader over-trusting them.

4. Missing Fixes #NNN in the PR body — CLAUDE.md wants it at the top when an issue exists. Fine if none was filed. Also consider adding an ILP/ingestion label alongside bug.

5. getTableBuffer(String) is a side-effectful @TestOnly "getter" (:1976) — on a non-current name it flips currentTableBuffer, re-snaps bytes, and bypasses table()'s in-progress-row guard. Safe for the current tests but a footgun if reused. Pre-existing; the PR leans on it more. A non-mutating peek would read cleaner.

Downgraded (candidate concerns verified false)

• pendingBytes/pendingRowCount drift or going negative across clear/reset/rollback/switch interleavings — false. Invariant pendingBytes == Σ(committedBytes − baselineBytes) telescopes from the same anchor sendRow() uses; committedBytes ≥ baselineBytes always; a non-current buffer can't hold an in-progress row (table() guard :2790); clear() is idempotent. Checked double-clear, current-buffer-with-in-progress-row, post-flush, switch-away-and-back.
• Cached ColumnBuffer dangling after clear/rollback — false. WS caches only the two timestamp columns (nulled before the free on every path); regular columns are re-looked-up per value; UDP already nulled its caches on rollback pre-PR and InProgressColumnState.clear() nulls its column ref (:1461).
• Constructor overload ambiguity / lost global symbol dictionary — false. (String, QwpWebSocketSender) binds for WS (global dict retained), (String, Owner) for UDP (this is not a QwpWebSocketSender → sender=null, identical to pre-PR); the (QwpWebSocketSender) null cast disambiguates the 1-arg ctor.
• UDP datagram under-estimate after clear — false. The headroom reset only drops caches (recompute is conservative), and the hard per-row commitCurrentRow size guard still runs, so an oversize datagram can't ship.
• resetTableBuffersAfterFlush re-anchor breaks delta accounting — false. Anchoring at getBufferedBytes() (= the reset baseline) matches the empty-flush path and fixes a pre-existing over-count of var-width seed bytes in the byte auto-flush threshold.
• Callback stale reads / throw stranding column frees, or reachable on a live sender — false. Callback runs before any state is released, reads only simple getters (can't throw), and production clear() is close-only.

Summary

Approve. Correct, tightly-scoped fix for a real native use-after-free, plus a valid incidental correction to the post-flush byte anchor. Three independent review agents (cross-context, fresh-context adversarial, test-efficacy) and my own trace all converge on zero correctness/concurrency/resource/accounting defects; the adversarial agent ran all 12 new tests and they pass. Regression coverage is genuine — the timestamp-cache and accounting tests fail (NPE or wrong count/bytes) if the fix is reverted.

• Draft findings: 6 candidate concerns verified and dropped as false positives; 5 minor items kept (1 latent/unreachable, 1 ordering, 1 weak-test, 1 metadata, 1 footgun). 0 Critical, 0 Moderate.
• All kept findings are in-diff. The cross-context pass walked every callsite of clear()/close(), the constructors, rollbackRow/cancelRow, getTableBuffer, and both onTableBufferClear implementations, and proved production clear() is close-only — so zero out-of-diff findings is correct here, not an under-run.