fix(core): keep materialized views current after concurrent invalidation - #7330

jovfer · 2026-06-25T22:12:57Z

Problem

On a primary, a materialized view could continue to report valid while serving stale rows. An apply-time INVALIDATE could publish while another refresh worker held the same view latch. The losing worker queued a retry, but the pending-marker guard then consumed that retry without minting durable invalid state or arranging another wake-up.

The original freeze required at least two refresh workers and a base operation such as TRUNCATE, a rows-affected UPDATE, or a structural change. #7261 introduced that master-only regression; no release includes it.

The same state machine had related paths with the same user-visible outcome, an unnecessary invalid result, or an unbounded worker loop:

a full refresh could clear an invalidation newer than its fixed base snapshot;
write suspension and full-refresh terminal exits could consume pending work;
queue allocation failure could lose the only invalidation wake-up;
reason-blind finalization could invalidate a view that had already refreshed through the triggering base transaction;
every lock contender could add a redundant retry to the shared queue;
a declined invalidation could still cascade to dependent views even though the parent remained valid;
an authorization refusal from the read-only writer gate (a replica demote racing a refresh) could trap the worker in a requeue loop: a refused invalidation re-queued itself once per drain pass, and a refused full refresh re-queued itself twice per pass (retry path plus finalize wake), growing the queue without bound while the refusal persisted.

Changes

Coordinate invalidation and full-refresh ownership

MatViewState now keeps one atomic pending marker with independent invalidation and full-refresh facets. The marker carries the invalidation reason, base table token, base transaction, force flag, and a distinct full-refresh owner. Facet publications merge keep-strongest: force flags OR, same-token transactions keep the maximum, and unknown or mismatched provenance collapses to conservative.

An invalidator publishes that marker before it attempts the view latch. It either acquires the latch and handles the marker itself, or the completing latch holder observes the publication after unlock and queues one authoritative wake-up. Lock losers no longer publish their own duplicate retries, so one contention episode produces bounded queue traffic.

Every operational latch holder routes release through finalizeAndUnlock, including incremental, range, interval, full, invalidation, and synchronous REFRESH ... STATS paths. The helper always releases the latch and runs close/drop cleanup before it publishes follow-up work; fullRefresh nests its owner-facet cleanup so the latch releases even when the marker-replacement allocation inside that cleanup throws.

If queue growth throws, the state store records allocation-free retry flags on the view state. A later refresh-job tick claims those flags and retries only the facets whose publication failed. The marker remains authoritative until the operation reaches a terminal state.

Make full refresh snapshot-aware

Apply-WAL invalidations now carry the base-table token and the latest sequence transaction covered by the apply batch. A successful full refresh records its fixed reader snapshot and clears an invalidation only when the token matches and the snapshot transaction covers the invalidation transaction. A successful full refresh also clears, by object identity, the exact marker that was published before its fixed reader snapshot -- a provenance-free invalidation covered by construction no longer re-invalidates the view the rebuild just repaired.

A parked provenance-free cascade marker ("base materialized view is invalidated") is likewise consumed by a child's covering FULL rebuild; this is intended — neither master nor this branch gates FULL on the parent's invalid status, so the resulting child-valid-on-stale-parent state matches what a user REFRESH FULL already produces on master, and the chain self-heals when the repaired parent's next commit re-notifies the child.

Unknown provenance, a different table epoch, or a newer transaction keeps the invalidation pending. A separate full-refresh owner also prevents an invalidation from consuming an independently requested rebuild. Blocked, suspended, missing-base, retry, and lock-loss exits now clear or transfer that owner instead of leaving a permanent sentinel.

Defer instead of spinning on read-only refusal

An authorization refusal from the read-only writer gate is a transient role condition, not a refresh failure, so neither facet invalidates the view or retries in a loop. The refused holder keeps its facet pending on the marker, re-queues nothing, and hands finalizeAndUnlock the exact publication it consumed; the post-release handoff suppresses only that publication's wake, keyed on object identity. Publications mint fresh marker and owner objects, so identity proves no newer request arrived during the hold — anything newer still wakes exactly once, and convergence stays bounded by real publications.

The retained facets wait for out-of-band redelivery: a later latch holder's handoff, RESUME WAL, a fresh request, or promote-time queue rebuild. The refresh and WAL-purge gates key on the marker's reason facet only, so a retained full-refresh owner does not freeze a valid view; the next ordinary refresh holder's finalize redelivers it.

Preserve intent across suspension and non-mint exits

Suspended views retain pending invalidation and full-refresh facets. A successful RESUME WAL republishes those facets after writes reopen. A REFRESH FULL issued while the view is suspended now parks its owner facet before the suspended exit, so RESUME WAL redelivers it instead of silently dropping the request; RESUME WAL itself reports success even when the post-resume redelivery enqueue fails, leaving recovery to the store's retry flags.

invalidateView now cascades to dependent views only after it persists an invalid state. A force=false decline on a never-incrementally-refreshed parent therefore leaves both the parent and its children valid. The String marker API also rejects a null invalidation reason; only the no-argument full-refresh API can create a reasonless facet.

Hardening from external review (r19)

A further review pass on the finalized state machine surfaced two behavioral gaps and confirmed that one remaining simplification request targets an unreachable state; five new regression tests close the gaps and pin the invariant.

A view rename landing between a FULL task's enqueue and its writer acquire made the pass fail with table-does-not-exist; the rename branch re-enqueued the task's owner under the updated token while the finalize owner wake, which had no suppression handed to it on this branch, enqueued the same owner again under the stale token - two tasks for one request. fullRefresh now hands that owner to finalizeAndUnlock0's existing identity-keyed suppression, so the rename re-enqueue is the single redelivery.

The remaining review recipe asked for a test that reaches a FULL refresh with no rows and an unchanged watermark; that state is unreachable. The FULL pump unconditionally resets the watermark to -1 before it runs, and findRefreshIntervals stamps toBaseTxn with the reader's seqTxn, which is never negative for a WAL table, so insertAsSelect's no-interval branch always advances the watermark and reports success. The dead || intervalIterator == null sub-clause is replaced by if (didRefresh) plus an assertion that pins the invariant, and the recipe is recorded as refuted rather than implemented.

New tests close coverage gaps this and the prior review round left open: a rename racing a full refresh's finalize redelivers the owner exactly once; RESUME WAL redelivery that swallows an enqueue failure on one facet still recovers both the invalidation and full-refresh halves through the store's retry flags; the refresh timer schedules a run despite an owner-only marker with no invalidation reason; the WAL-purge gate treats an owner-only marker as a valid view rather than freezing purge; and a put-back failure under the promote gate recovers through the same retry-flag mechanism. A further pass consolidates duplicated helper methods across the trap test suite.

Hardening from external review (r20)

A from-scratch review pass found no production defects but blocked on one coverage row: the retry-heap gate flip in MatViewTimerJob.processRefreshRetries — per its javadoc the only path that wakes immediate views — had no test, so reverting it to the marker-any predicate left the whole tree green. A new test drives a due busy-retry entry with an owner-only marker parked and pins the reason-only gate.

Ten further tests close the round's remaining coverage rows. A @TestOnly append seam in MatViewStateStoreImpl, fired before both task-queue append sites, makes the store's real recovery catches reachable for the first time: tests now drive the owner-overload, enqueueInvalidate0, and put-back catches genuinely instead of modeling them at the engine wrapper, and a mid-scan failure test pins the per-state catch's unique duty of restoring retry flags for facets whose enqueue was never attempted. The rest cover the finalize catch's both-facets arm, suspension landing between the up-front gate and the writer acquire in both the FULL and invalidation paths, the generic terminal failure clearing the owner sentinel, and a stale handoff owner delivering as a no-op.

REFRESH ... STATS now swallows a wake-enqueue failure inside its post-release finalize instead of failing the statement after the stats reset durably succeeded, mirroring ALTER TABLE RESUME WAL; the finalize's own catch has armed the store's retry flags before the swallow sees the throw.

Effects and tradeoffs

The state machine prevents the reviewed paths from leaving a stale view reporting valid, and it avoids invalidating a view when a full snapshot can prove that it covered the triggering base transaction.
The marker now stores more metadata and allocates a small ownership object per invalidation or full-refresh publication. CAS loops and additional volatile reads run on refresh-task boundaries, not per row, but they add complexity to an already concurrent control path.
Invalidations without exact base-token and transaction provenance remain conservative. The engine may report a view invalid and require REFRESH ... FULL even when a refresh happened to include the change, because suppressing an unproven invalidation could hide stale data.
A suspended view keeps its marker and delays invalid-state minting until RESUME WAL. This avoids queue spin while writes remain unavailable, but the catalogue continues to show the last durable state during suspension.
A read-only refusal now defers rather than hot-retries. In the narrow window where the writer gate refuses while the engine still reports writable, the refused request waits for out-of-band redelivery instead of retrying until the window closes; with both facets pending in that window, a bounded one-task rotation replaces the previous unbounded queue growth. A view skips incremental and timer refreshes only while an invalidation reason is pending; a deferred full-refresh owner no longer freezes the view, at the cost of possible wasted incremental work while a full rebuild is queued.
Sustained allocation failure can keep retry publication from succeeding. The engine retains the marker and retries on later job ticks instead of silently dropping intent, so repeated failures remain visible to worker error handling.
The retry flags recover re-publication of an already-published marker only. A fresh invalidation whose first enqueue throws keeps the pre-existing behavior: the throw reaches the caller, and durable causes (truncate, missing base) are re-derived by hydration or the next refresh failure, while a mid-loop failure in the dependent-view cascade is not re-driven.
A wake-enqueue failure inside the finalize that follows a successful full refresh also skips the immediate incremental kickstart for IMMEDIATE views; later job ticks recover it.
REFRESH ... STATS no longer surfaces a wake-enqueue failure from its post-release finalize; the statement logs the error and leaves redelivery to the store's retry flags. The caller loses direct visibility of that failure, in exchange for the statement no longer failing after its stats reset durably succeeded.

Out of scope (follow-ups)

A demote can discard the in-memory dependent-view cascade before a child processes it, leaving a chained view stale across promote and restart. fix(core): fix chained materialized views left stale after a parent view is invalidated #7337 (stacked on this PR) fixes that on the durable cold-load path, which also backstops the cascade-loss case in the last tradeoff above on the next boot or promote.
Publishing the invalidation marker before the first enqueue would extend retry-flag recovery to fresh invalidations and the dependent-view cascade. It changes marker-identity dynamics and needs its own design pass.
On the enterprise side, the heal path of an aborted role switch keeps the state store alive but does not redeliver surviving pending facets; wiring reenqueuePendingOnResume into that path closes the gap for both facets.
The enterprise switch-invariants suite pins the no-spin deferral contract for the invalidation facet; a sibling pin for the full-refresh facet belongs there as well.
Three enterprise cross-repo checks came back clean: no enterprise class implements MatViewStateStore directly, so the interface additions cannot break enterprise compilation; the inherited alterTableResume redelivery tail is untouched by the enterprise SQL compiler, so it fires for enterprise RESUME WAL too; and no MatViewState survives a role switch, since demote frees the whole store and promote hydrates a fresh one, so stale-marker reuse across roles cannot arise. Only the routine OSS submodule pin bump remains on the enterprise side.
A view that has only ever range-refreshed declines a non-forced truncate invalidation and keeps serving pre-truncate rows (master-identical; the decline gate never sees a range-refresh watermark).
Extending MatViewFuzzTest with a rows-affected UPDATE mix plus occasional REFRESH FULL, asserting post-drain view_status consistency, is the highest-value randomized check for the provenance and covering-FULL machinery; the current fuzz surface is truncate-driven and never stresses it.

Test plan

mvn -pl core -Dtest=io.questdb.test.cairo.mv.MatViewPendingInvalidationTrapTest test
- 73 tests pass with 0 failures, 0 errors, and 0 skips.
Deterministic latch and marker seams cover the marker-read-to-unlock handoff, fixed-snapshot coverage, a publisher acquiring the latch after full-refresh consumption, and concurrent invalidation/full-owner CAS collisions.
Queue tests cover one wake-up per facet under many contenders and retry after injected allocation failure, including the split where the invalidation wake succeeds and the full-refresh wake fails.
Read-only refusal tests pin the deferral contract for both facets: no self-feed on a sticky refusal (acquire and commit-fence faces), newer publications wake exactly once, a full-refresh owner is never stranded, the both-facet rotation stays bounded, and redelivery completes after the refusal clears.
Characterization tests pin the marker merge semantics: force OR, maximum-txn frontier, conservative provenance collapse, and facet combination in both orders.
Full-refresh tests cover blocked, suspended, missing-base, lock-loss, retry, covered, and newer-than-snapshot outcomes, plus latch release when the owner-facet cleanup itself throws, and a rename that lands between a FULL task's enqueue and its writer acquire redelivers the full-refresh owner exactly once.
Gate tests confirm the refresh timer schedules a run despite an owner-only marker with no invalidation reason, and that the WAL-purge gate treats an owner-only marker as a valid view rather than freezing purge.
Suspension tests cover direct and finalized invalidation delivery through SUSPEND WAL and RESUME WAL, and a RESUME WAL that swallows a redelivery failure still recovers both facets on a later job tick via the store's retry flags.
A promote-window put-back failure test, reached through an injected engine override since isMatViewRefreshSuspended() is constant false in OSS, confirms recovery via the same retry-flag mechanism for both facets.
Cleanup tests cover failed refreshes, dropped and closed states, read-only transitions, bounded child-thread termination, and latch release on every tested exit, including test-side rescue of manually held latches.
Regression tests pin the FULL-repair path (a provenance-free marker covered by the rebuild's snapshot is consumed; the repaired view stays valid), owner-freeze recovery through an ordinary refresh holder's finalize, suspended-FULL owner parking with resume redelivery, resume-path early returns, the single-runner reenqueue scan, and the batch-end txn frontier stamp.
Retry-heap gate: a due busy-retry entry re-drives an incremental refresh despite an owner-only marker, and the consumed retry clears the backoff deadline.
Store-level failure tests drive the real queue-append recovery catches through a @TestOnly append seam: the owner-carrying full-refresh enqueue, the invalidation enqueue, put-back for both facets, and a mid-scan failure that must restore unclaimed retry flags for the second scan to redeliver.
Finalize-catch tests cover the both-facets arm (an invalidation wake failure with a co-pending owner arms both retry flags, observed by inspecting what one retry scan redelivers) and the STATS holder swallowing a wake failure while recovery rides the retry flags.
Suspend-window tests cover a suspension landing between the up-front gate and the writer acquire in both the FULL and invalidation paths, a generic terminal failure clearing the owner sentinel, and a stale handoff owner delivering as a no-op.

@testonly

When an apply-time INVALIDATE deferred because a concurrent refresh held the view lock, only the incremental-refresh holders (refreshIncremental and refreshDependentViewsIncremental) finalized the pending marker on completion. The range-refresh and interval-update holders did not, leaving the view stuck as valid-in-memory with stale rows until a restart, REFRESH FULL, or role-switch rebuilt the store. Extend finalizeDeferredInvalidation to the finally blocks of rangeRefresh and updateRefreshIntervals. Both holders now call the helper as the first statement of their finally block, mirroring the incremental pattern: while the lock is still held, clear the pending marker and re-enqueue a fresh INVALIDATE so the guard in invalidateView no longer swallows the re-delivered task. Fix the write-ordering in markAsPendingInvalidation: publish the reason before the flag so a reader on a weak-memory arch cannot observe pending==true with a null reason. Wire the @testonly seam into refreshIncremental0 (after the isLocked assert), rangeRefresh (after tryLock), and updateRefreshIntervals (after tryLock) so all three holders can be driven deterministically in tests. Add testRangeRefreshHoldingLockFinalizesDeferredInvalidation and testUpdateRefreshIntervalsHoldingLockFinalizesDeferredInvalidation to MatViewPendingInvalidationTrapTest; each was witnessed RED without its respective finally call and GREEN with it. Also refine two comments: the invalidateView guard comment now correctly notes that a fullRefresh-holder deferral only self-resolves for an invalidation set before the full refresh's reader snapshot (fullRefresh does not call finalizeDeferredInvalidation); and the finalizeDeferredInvalidation guard comment now explains that the getLastRefreshBaseTxn()==-1 skip mirrors invalidateView's own force||getLastRefreshBaseTxn()!=-1 gate rather than claiming the view has no stale rows.

coderabbitai · 2026-06-25T22:13:07Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8bf2b7b4-e91c-41bd-8502-2979c67a06b3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly matches the main fix: keeping materialized views current after a concurrent invalidation.
Description check	✅ Passed	The description is directly about the same materialized-view invalidation bug and the corresponding fix and tests.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch sm_matview_pending_invalidation_finalize

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

The first cut of finalizeDeferredInvalidation early-returned when getLastRefreshBaseTxn() == -1, leaving a deferred invalidation's pending marker set forever. A range-only-populated view (a MANUAL view backfilled with REFRESH ... RANGE, whose rangeRefreshSuccess never advances lastRefreshBaseTxn) that received a base-cascade INVALIDATE while one of its range refreshes held the lock was left frozen: every refresh path skips a pending view, the re-enqueued INVALIDATE is swallowed by the top guard, and getViewStatus reads the persisted invalid flag (never the in-memory marker), so the view reported valid while serving stale rows with no auto-recovery. finalize now clears the marker and re-enqueues regardless of lastRefreshBaseTxn. The re-enqueued view task re-delivers force=true, which mints regardless of lastRefreshBaseTxn, so such a view ends cleanly invalid -- visible and recoverable via REFRESH ... FULL -- instead of frozen. This is deliberately stricter than the no-contention path (which leaves a never-incrementally-refreshed view valid); the comment that claimed finalize matched no-contention is corrected to state the real behavior and why matching it is not reachable once the deferral has queued a force=true retry. fullRefresh was the lone lock-holding path that never finalized: an INVALIDATE deferring during its pump (the lock is held for the whole insert-as-select, far wider than a tight race) left the view frozen stale-but-valid. Add finalizeDeferredInvalidation to its finally, plus a test-only seam after resetInvalidState to model the deferral. Other changes: - Add MatViewState.clearPendingInvalidation(), separating clearing the marker from the invalid flag; finalize uses it instead of the broader markAsValid(), which could resurrect an invalid view. - Correct the invalidateView residuals comment: fold full refresh into the finalizing holders and state that the finalize-to-unlock window, when hit, is a permanent silent freeze, not self-healing. - Refresh the stale MatViewState class javadoc to describe the new transient pendingInvalidationReason. Tests pin each path: the range-only (== -1) frozen branch, the null-reason full-refresh marker (asserting no spurious invalidation), and the fullRefresh finalize path. Each fails when its fix is reverted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fd3dGESij17765qqbDTSWF

Round-2/3 review of PR #7330 surfaced an un-finalized sixth lock-holder and several robustness items. M1: invalidateView is itself a lock-holder -- its force=false decline on a never-incrementally-refreshed view holds the lock without minting, so a concurrent INVALIDATE deferring during that hold stranded pending and froze the view (silently stale while it reports valid). Its finally now finalizes too. A new test pins it and fails without the fix. The auth-rollback defer is handled: it is read-only when finalize runs (no-op), or re-delivers the invalidation it already queued if the role flipped back to writable. M2 (accepted, documented): the fullRefresh finalize can flip a view that the full refresh just rebuilt correctly to invalid under a lock race. Kept -- it is visible and recoverable, and dropping it would re-expose the silent frozen-pending freeze across the whole pump. finalize is reason-blind; txn-gating is left as a follow-up. Minor hardening: - finalizeAndUnlock wraps finalize so an OutOfMemoryError cannot skip unlock and leak the latch/cursorFactory; finalize stays under the latch to keep the finalize->unlock race narrow. - assert non-null reason at the three defer sites (null is reserved for the full-refresh reschedule marker; document the torn-write route into the residual freeze). - test seam made volatile, renamed to the *ForTesting idiom, read via a local before the null-check. - new tests: invalidateView finalize, single-view incremental finalize, multi-dependent fan-out; trivia (license year, dangling javadoc pointer, text blocks). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fd3dGESij17765qqbDTSWF

Pull in #7275 (retry mat view refresh on transient errors instead of invalidating, plus the refresh block list). It rewrites the same MatViewRefreshJob refresh/invalidate paths this branch touches. Conflict: MatViewState.markAsValid() reset both this branch's pendingInvalidationReason and #7275's refreshRetryAfterMicros and refreshRetryCount. Resolved by resetting all three. MatViewRefreshJob auto-merged textually; audited every overlapping method. #7275's tryScheduleRetry and block-list guards live in the catch blocks and the pre-tryLock gates; this branch's finalizeAndUnlock lives in the finally blocks. They compose: a scheduled retry still runs the finally, where finalizeDeferredInvalidation is a no-op unless a concurrent INVALIDATE actually deferred during the lock hold. Verified: core main and tests compile; the trap suite (10), busy-retry (19), block-list (5) and cancel (2) suites pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fd3dGESij17765qqbDTSWF

jovfer · 2026-06-26T12:23:01Z

/azp run macwin

azure-pipelines · 2026-06-26T12:23:11Z

Azure Pipelines successfully started running 1 pipeline(s).

invalidateView's finally finalized its own auth-rollback deferral: finalizeDeferredInvalidation cleared the pending marker the catch had just set and re-enqueued the INVALIDATE. When the WAL writer refusal is sticky (getWalWriter keeps throwing an authorization error until a re-promote), clearing the marker lets the retry back past the top guard on every drain pass, so the refresh worker busy-spins instead of deferring once. finalize's isReadOnlyMode skip did not cover this: getWalWriter can refuse while the node still reports writable (the TOCTOU the auth-rollback branch exists for), so isReadOnlyMode is false and finalize proceeds to clear and re-enqueue. Track the self-deferral with a selfDeferred flag and skip finalize on that path -- just unlock. The marker stays set, the top guard stops the next pass, and a re-promote rebuilds the view from disk. The decline path still finalizes a deferral a concurrent invalidate left while this invalidate held the lock. Caught by the enterprise MatViewSwitchInvariantsTest method invalidateInnerLoopDefersOnReadOnlyRefusalAndDoesNotSpin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fd3dGESij17765qqbDTSWF

…invalidation_finalize

jovfer · 2026-06-26T15:43:58Z

Code Review — PR #7330: `fix(core): fix materialized views left stale after a concurrent invalidation`

Level 3 (full mission-critical pass — all 11 review dimensions, per-finding source verification). Reviewed from scratch.

Branch sm_matview_pending_invalidation_finalize → master. Diff: 3 files,
MatViewRefreshJob.java (+138/-30), MatViewState.java (+29/-2),
MatViewPendingInvalidationTrapTest.java (new, +617).

What the PR does

An apply-time INVALIDATE for view V that defers (a second refresh-pool worker holds
V's lock, or the node is read-only) marks pendingInvalidation=true and re-enqueues an
INVALIDATE. The re-enqueued task is then swallowed by invalidateView's
!isPendingInvalidation() top guard, so nothing finalizes it and V stays valid on disk
with stale rows (a silent, non-self-healing freeze). The fix: every lock-holder, on
completion, calls finalizeDeferredInvalidation, which clears the marker and re-enqueues a
fresh force=true INVALIDATE so the re-delivery mints invalid. A selfDeferred flag
suppresses finalize on invalidateView's own auth-rollback deferral to avoid a busy-spin on
a sticky writer refusal.

The core fix is correct. I traced all 6 in-diff lock-holders, the selfDeferred
auth-rollback path, the two new volatile fields' memory ordering, every INVALIDATE producer,
and every reader of isPendingInvalidation(). Post-fix freezes are a strict subset of
pre-fix freezes. The findings below are one out-of-diff gap that contradicts the PR's own
completeness claim, a PR-body/doc gap, and a set of minor/test-infra items.

Critical

None. No correctness, concurrency, or resource defect is introduced by this diff. (The
out-of-diff gap below is a real silent-freeze window but is pre-existing and microscopically
narrow; it is filed as Moderate because the PR newly asserts an invariant that it covers it.)

Moderate

M1 — A 7th lock-holder (`REFRESH ... STATS`) is not finalized; the PR's "every lock-holder finalizes" claim is inaccurate (out-of-diff)

core/src/main/java/io/questdb/griffin/SqlCompilerImpl.java:3586-3591 — the
REFRESH MATERIALIZED VIEW <name> STATS branch is the only MatViewState latch holder
outside MatViewRefreshJob, and it does a plain unlock() with no finalize:

if (viewState.tryLock()) {
    try {
        viewState.refreshStats();          // resets 3 EMA longs; never touches pending/invalid
    } finally {
        viewState.unlock();                // <-- no finalizeDeferredInvalidation
    }
}

I grepped every .tryLock() on a MatViewState across questdb/core/src/main and
questdb-ent/src/main: there are exactly 7 holders — the 6 in MatViewRefreshJob
(830/1583/1764/1935/2084/2307) that the PR routes through finalizeAndUnlock, plus this one.
refreshStats() (MatViewState.java:711-716) only zeroes avgCommitNanos /
avgScanSampleNanos / avgScanRangeTsUnits, so it never clears a pending marker.

Trigger path (production-reachable): a user runs REFRESH MATERIALIZED VIEW v STATS
(runs synchronously on the SQL/pg-wire/HTTP thread, wins the latch). Concurrently a base op's
INVALIDATE (TRUNCATE / rows-affected UPDATE / structural change, via
ApplyWal2TableJob) is processed by a refresh-pool worker → invalidateView →
tryLock() fails → markAsPendingInvalidation(reason) + enqueueInvalidate + return. The
STATS unlock() runs without finalizing → the re-enqueued INVALIDATE is swallowed at
MatViewRefreshJob.java:1561, and every later refresh is also swallowed by the pending
top-guards (1739/1904/2063/2299) → terminal silent freeze: valid on disk, stale rows, only
recoverable by REFRESH ... FULL / restart / role switch (and for an UPDATE-caused
invalidation, not even by restart — see Residuals).

Why this is filed even though it's pre-existing: the new in-code comment at
MatViewRefreshJob.java:1551-1553 asserts "Every lock-holder finalizes a deferral on
completion" and enumerates only the 6 refresh-job holders, and the PR body says the fix is
"Applied to every lock-holder". Both are now factually incomplete — this holder is
unaccounted for. The window is far narrower than the pump windows the PR closes
(refreshStats is ~3 field writes), but it is the identical bug class at an uncovered
callsite.

Fix (recommend bundling into this PR, per the repo's bundle-related-fixes convention):
route the STATS path through the same finalize, or — since finalizeDeferredInvalidation is
private to MatViewRefreshJob — at minimum correct the invalidateView comment and the PR
body to list REFRESH ... STATS as a known residual alongside the documented ones. Making it
true (covering STATS) is preferable to documenting a fourth residual.

M2 — PR body omits the `selfDeferred` busy-spin fix (commit `3cc2e1285f`); the squashed commit message will be incomplete (in-diff / metadata)

The body's Fix section says only that invalidateView's finally "now finalizes too." But
the shipped behavior is not unconditional finalize: commit 3cc2e1285f added
selfDeferred, which skips finalize on invalidateView's own auth-rollback deferral
because, with a sticky writer refusal (a read-only TOCTOU where getWalWriter refuses while
isReadOnlyMode() still reads false), re-clearing the marker lets the retry past the top
guard every drain pass → the refresh worker busy-spins. The body never mentions
selfDeferred, the sticky refusal, the spin, or the enterprise regression test
MatViewSwitchInvariantsTest#invalidateInnerLoopDefersOnReadOnlyRefusalAndDoesNotSpin that
pins it. Since PRs are squash-merged, the body is the commit message that lands on master,
so it should describe the whole change.

Fix: add a Fix paragraph and a Test-plan bullet covering the selfDeferred /
no-busy-spin behavior and the enterprise test.

Minor

m1 — Test seam can leak the view latch on three paths (in-diff, test-infra only)

In rangeRefresh (1774-1777), updateRefreshIntervals (2313-2316), and invalidateView
(1593-1596), seam.run() executes after tryLock() but outside the try whose
finally unlocks. A throwing seam would leak the latch → wedge the view and leak the parked
cursorFactory at teardown. Unreachable in production (onHoldingLockForTesting is null;
cloneInstance() doesn't copy it; current test seams only do compareAndSet + volatile
writes), but it is the one structural asymmetry vs. fullRefresh (869) and
refreshIncremental0 (2172), which place the seam inside the unlock-protected region.
Cheap hardening: move each of the three seams to the first statement inside its unlocking
try. Flagged independently by 4 of the review agents.

m2 — Tests model the deferral via the seam but never enqueue the competing INVALIDATE, so the guard-swallow is not exercised end-to-end (in-diff, test fidelity)

Every seam lambda calls only state.markAsPendingInvalidation(...) — never the paired
stateStore.enqueueInvalidate(...) that a real losing invalidateView issues
(MatViewRefreshJob.java:1586-1587). So the actual cause of the bug — a concurrent
INVALIDATE losing the lock and having its re-delivered task swallowed at line 1561 — is not
reproduced in a single end-to-end flow by any of the 10 tests; they prove only that finalize,
given a pending marker, re-enqueues and mints. The tests are still genuine regression
tests (testInvalidateView... and testRangeOnlyPopulated... were verified to fail pre-fix,
and testLockContendedInvalidationDefersWithReason exercises the real defer path), but the
seam is a simplification, not the true two-worker interleaving the comments claim
("exactly as a losing concurrent invalidateView would"). Consider one test that enqueues a
real second INVALIDATE and drains, or soften the comments.

m3 — Boolean naming (in-diff)

finalizeAndUnlock(..., boolean incrementRefreshSeq) (496): the param reads as an
imperative verb identical to the method it gates (viewState.incrementRefreshSeq()), so
call sites are bare true/false against a verb. Rename to shouldIncrementRefreshSeq
per the is.../has... (here should...) convention.
boolean selfDeferred (1599): lacks the is/has prefix; isSelfDeferred is more
consistent (though it matches the file's pervasive bare-boolean-local style).

m4 — Missing OSS test for finalize's `isInvalid()` early-return (in-diff, coverage)

finalizeDeferredInvalidation's || viewState.isInvalid() guard is OSS-reachable (a refresh
that fails → refreshFailState → markAsInvalid, with a deferral also pending) but untested.
Worth a test: arm the seam to mark pending, force the refresh to fail, assert finalize does
NOT re-enqueue and the view carries the fail reason, not the deferral reason. (The
read-only early-return and the selfDeferred branch are intentionally OSS-uncovered and are
pinned by enterprise tests — see Downgraded D6/D7.)

m5 — Seam idiom duplicated at 5 sites; branch commit titles >50 chars (in-diff, trivial)

The 4-line final Runnable seam = onHoldingLockForTesting; if (seam != null) seam.run();
idiom is copy-pasted at lines 869/1593/1774/2172/2313 — could be a one-line private helper
(minor; the inline form is clear). Two branch commit titles exceed the 50-char guideline
("Finalize deferred mat-view invalidation on refresh completion" = 61; "Fix frozen-pending
mat views on range-only and full refresh" = 59); throwaway under squash-merge.

Documented residuals (verified real, accurately scoped, intentionally accepted)

Not new findings — the PR documents all three and they are strictly narrower than the pre-fix
always-lost deferral. Recorded so they stay tracked:

Full-refresh valid→invalid flip under a lock race the full refresh wins: finalize
is reason-blind, so a full refresh that already rebuilt the view correctly can still be
flipped invalid (cascading to dependents). The same divergence applies to range /
interval-update on a never-incrementally-refreshed view (lastRefreshBaseTxn == -1), where
the no-race path declines (force=false) and leaves the view valid but finalize re-mints
force=true → invalid. Conservatively safe (invalid is visible, REFRESH ... FULL
recovers), documented at finalizeDeferredInvalidation (525-533) and fullRefresh
(915-920), and testRangeOnlyPopulatedView... asserts the range-only case as intended.
Note: MANUAL/range-only views do receive background lock-holders (timer-driven
UPDATE_REFRESH_INTERVALS), so this flip is reachable in normal operation, not only under
REFRESH ... FULL.
finalize→unlock window: finalize clears+enqueues under the latch, then unlocks; a second
worker can dequeue the re-enqueued task, fail tryLock, re-defer, and the re-deferred task
is then swallowed for good. Verified the worst case is the documented silent freeze and
nothing worse (no lost/double-freed factory, no half-mint, no spin).
Torn (pending, reason) write: the setter writes reason→flag; clearPendingInvalidation
/ markAsValid write flag→reason, off-latch vs under-latch, so finalize can observe
(pending=true, reason=null) and route a real deferral into the null-reason no-op branch.
The concurrency pass mapped all 6 overlapping interleavings and confirmed this is exactly
the documented window — finalize never reads a stale non-null reason intra-read (its
under-latch read only races the reason-before-flag setter), so it never enqueues a
wrong-but-non-null reason.

Scope note (sharpening, not a new defect): for an UPDATE / DROP PARTITION-caused
invalidation lost in residual #2/#3, recovery is genuinely permanent-until-REFRESH ... FULL
— WalTxnRangeLoader flags only TRUNCATE as a barrier (hasTruncate) and gap-skips other
non-data txns, so a later incremental refresh advances lastRefreshBaseTxn past the UPDATE
without re-detecting it. The PR's "terminal silent freeze ... not self-healing" comment is
accurate. This is a residual, not a regression.

Downgraded (draft findings dismissed after source verification)

D1 — "finalize under the latch can deadlock (isReadOnlyMode / enqueueInvalidate)." False.
engine.isReadOnlyMode() is a lock-free flag read on OSS (configuration.isReadOnlyInstance())
and enterprise (super || isReadOnlyReplica()); the task queue is an unbounded,
non-blocking ConcurrentQueue (enqueue never blocks on full); neither re-acquires the same
view latch. The WalWriter try-with-resources closes before the outer finally, so finalize
holds only the view latch.
D2 — "the three assert reason != null defer sites can fire." False. Every INVALIDATE
producer passes a non-null reason: CairoEngine (6 literals), ApplyWal2TableJob
(operation is flipped to INVALIDATE only inside if (reason != null) guards, or the non-null
UpdateOperation.MAT_VIEW_INVALIDATION_REASON). The only null marker is fullRefresh's
markAsPendingInvalidation(), which enqueues a FULL_REFRESH, never an INVALIDATE.
D3 — "the selfDeferred auth-rollback branch has no test." False. The enterprise test
MatViewSwitchInvariantsTest#invalidateInnerLoopDefersOnReadOnlyRefusalAndDoesNotSpin
(line 510) arms getWalWriter to throw authorization() while isReadOnlyMode()==false,
drives one INVALIDATE pass on a side thread, and asserts the thread returns within 15s — i.e.
the busy-spin the selfDeferred flag prevents. invalidateCommitFenceRefusalLeavesNoHalfApplied State covers the commit-fence face. (Coverage exists; it is just not mentioned in the PR
body — see M2.)
D4 — "performance: redundant volatile reads / per-row cost / per-task allocation." False.
finalize runs once per refresh task (never per row); the common no-deferral path
short-circuits to a single volatile read (!isPendingInvalidation()); the three guard terms
read three distinct fields (cannot be coalesced); seam is a local field-read, not a
capturing lambda; no autoboxing, no java.util.*. The two reviewers who ran adversarial
perf passes found nothing to change.
D5 — "selfDeferred skip strands a concurrent deferral with no recovery." False. An
authorization error in this path can only come from the read-only gate (getWalWriter /
commit fence), so selfDeferred ⟹ read-only ⟹ promote-from-disk rebuild. It is never true
on a non-read-only node with no promote coming.
D6 / D7 — "finalize's read-only early-return and invalidateView's top read-only defer are
untested." Correct but intended: OSS cannot produce isReadOnlyMode()==true under the
write-heavy setup; both are covered by the enterprise demote/promote suite
(MatViewInvalidateRepromoteLosslessTest, MatViewSwitchInvariantsTest). The new test file's
own javadoc states this.
D8 — "member-ordering / LOG-chain / ASCII / dead-code violations." None found. All new
members slot into the correct alphabetical position for their kind; the PR adds no LOG
statements (the one in-context chain is properly .I$()-closed); comments are ASCII-only;
the @TestOnly import is used; the new pendingInvalidationReason is transient/in-memory and
not persisted (no on-disk format change).
D9 — "enqueueInvalidateDependentViews cascades on the force=false no-mint decline."
Pre-existing (unchanged context line 1667), not introduced here; idempotent.

Summary

Verdict: approve with changes requested — not blocking on correctness of the diff itself.

The core concurrency fix is sound and well-tested; the two flagship tests are genuine
pre-fix-failing regressions; there are no resource leaks, no new deadlocks, no measurable
performance cost, and no new correctness bug introduced by the diff. The documented residuals
are real, accurately scoped, and strictly better than the pre-fix behavior.

The one finding worth acting on before merge is M1: the PR's in-code comment and body
claim the fix covers "every lock-holder," but REFRESH MATERIALIZED VIEW ... STATS
(SqlCompilerImpl.java:3586) is a 7th holder that still does a plain unlock() and can strand
a deferral — the identical silent-freeze bug class, at a callsite outside the diff. It is
pre-existing and extremely narrow, but the PR newly asserts an invariant it does not establish,
so either cover STATS (preferred, bundle it here) or correct the claim. M2 (PR body omits
the selfDeferred spin fix) should be fixed so the squashed master commit is accurate.
Everything else is Minor (test-infra latch-leak hardening, test fidelity, naming, a coverage
gap).

Draft findings: 9 verified, 9 dropped as false positives / non-issues (D1-D9).
In-diff vs out-of-diff: 7 in-diff (1 Moderate + 6 Minor), 1 out-of-diff (M1, Moderate).
The cross-context pass surfaced the highest-value finding (M1), as expected for a small diff
to a widely-reachable subsystem.
Tradeoffs/regressions: the documented valid→invalid flip is a real, timing-dependent
behavior change for never-refreshed (incl. MANUAL) views under contention — conservatively
safe and REFRESH ... FULL-recoverable, but operators of manually-managed views should be
aware. Lost deferrals in the residual windows remain permanently stale until REFRESH ... FULL
for UPDATE/DROP-PARTITION causes.

jovfer · 2026-06-26T15:44:58Z

/azp run macwin

azure-pipelines · 2026-06-26T15:45:09Z

Azure Pipelines successfully started running 1 pipeline(s).

…latile markAsPendingInvalidation() and clearPendingInvalidation() wrote the (pendingInvalidation, pendingInvalidationReason) pair as two independent volatiles in opposite orders. An off-latch deferral racing a holder's under-latch finalize-clear could tear the pair to (pending=true, reason=null). That state is terminal: every refresh path and the invalidateView top guard skip a pending view, while finalize reads the null reason as a full-refresh marker and returns without clearing -- yet no full refresh is queued, so the view sits valid+stale with no self-heal (only REFRESH ... FULL or a restart/role-switch recovers it). Collapse both fields into a single volatile Object marker: null -> not pending sentinel -> pending, no reason (full-refresh reschedule) String -> pending, with that reason Every transition is now one atomic write/read, so the torn intermediate cannot exist. The sentinel (reasonless) marker is only ever written alongside a queued full refresh, so finalize leaving it is safe. Closes M1 (the torn composite). The narrower M2 lost-update/swallow window is unchanged and stays documented in invalidateView. Adds two regression tests covering the marker state machine and a concurrent defer-vs-clear guard.

bluestreak01

@jovfer — Level 3 review (full mission-critical pass). I ran all review dimensions (correctness, concurrency, performance, resources, tests, quality, metadata) against the source, with per-finding source verification.

What I verified (the core fix is sound)

Re-enqueue re-delivers force=true. finalizeDeferredInvalidation -> stateStore.enqueueInvalidate(viewToken, reason) enqueues a view-scoped task; invalidate() (MatViewRefreshJob.java:1520) dispatches it as invalidateView(token, reason, /*force*/ true). So a range-only / never-refreshed view mints invalid on re-delivery.
All 6 in-diff lock-holders route through finalizeAndUnlock (830/1583/1764/1935/2084/2307), and finalize correctly no-ops on the common no-deferral path (single volatile read !isPendingInvalidation()).
The single-volatile marker eliminates the torn (pending=true, reason=null) write. Setter writes one reference; clearPendingInvalidation/markAsValid write null; getPendingInvalidationReason() is one read.
The no-reason sentinel never reaches an INVALIDATE path. fullRefresh's tryLock-fail marks pending with no reason and enqueues a FULL_REFRESH (MatViewRefreshJob.java:833-834), not an INVALIDATE; every INVALIDATE producer (CairoEngine.java:2596-2641, ApplyWal2TableJob.java:891) passes a non-null reason, so the three assert invalidationReason != null guards can't fire.
refreshSuccess does not clear the marker (MatViewState.java:739), so a deferral that lands mid-incremental-refresh survives into finalize — the tests' asserted invalid end-state is real, not accidental.
No new deadlock / leak / infinite re-enqueue. finalizeAndUnlock's inner-try keeps unlock() running past an OOM in finalize; double-enqueued INVALIDATEs are idempotent (second is swallowed by !isInvalid()); selfDeferred skips finalize on the auth-rollback deferral to avoid a busy-spin.

Post-fix silent freezes are a strict subset of pre-fix. No correctness, concurrency, or resource defect is introduced by the diff.

Critical

None.

Moderate

M1 — A 7th lock-holder (`REFRESH ... STATS`) is not finalized; the PR's "every lock-holder" claim is inaccurate (out-of-diff, pre-existing)

invalidateView's new comment (MatViewRefreshJob.java:1551-1553) asserts "Every lock-holder finalizes a deferral on completion" and the PR body says "Applied to every lock-holder." Both are factually wrong. I grepped every MatViewState.tryLock() in the repo: there are 7 holders — the 6 in MatViewRefreshJob the PR routes through finalizeAndUnlock, plus SqlCompilerImpl.java:3586:

if (viewState.tryLock()) {
    try {
        viewState.refreshStats();   // MatViewState.java:727 - zeroes 3 EMA longs only; never touches the marker
    } finally {
        viewState.unlock();          // plain unlock, no finalizeDeferredInvalidation
    }
}

refreshStats() never clears a pending marker. Trigger: a user runs REFRESH MATERIALIZED VIEW v STATS (synchronous, on the SQL thread, wins the latch); concurrently a base op's INVALIDATE is processed by a refresh-pool worker -> invalidateView -> tryLock() fails -> markAsPendingInvalidation(reason) + enqueueInvalidate + return. The STATS unlock() runs without finalizing -> the re-enqueued task is swallowed at the !isPendingInvalidation() top guard -> the same terminal silent freeze the PR is closing everywhere else.

Pre-existing (the PR doesn't touch this path) and the window is microscopic (~3 field writes), but the PR newly asserts an invariant it does not fully establish. Per the bundle-related-fixes convention, prefer routing STATS through the same finalize; at minimum correct the in-code comment and PR body to list STATS as a fourth residual.

M2 — PR body omits the `selfDeferred` busy-spin fix; the squashed commit message will be incomplete (metadata)

The body's Fix section says only that invalidateView's finally "now finalizes too." The shipped behavior is conditional: commit 3cc2e1285f added selfDeferred (MatViewRefreshJob.java:1600,1657-1668) which skips finalize on invalidateView's own auth-rollback deferral, because with a sticky read-only writer refusal, re-clearing the marker would let the retry past the top guard every drain pass — a busy-spin. The body never mentions selfDeferred, the sticky refusal, or the spin. PRs here are squash-merged, so the body is the message that lands on master; it should describe the whole change. Add a Fix paragraph and a test-plan bullet.

Note: the enterprise module (questdb-ent) is not present in my workspace, so I could not verify the enterprise regression tests the branch presumably relies on for the read-only selfDeferred/no-spin and demote/promote paths (MatViewSwitchInvariantsTest, MatViewInvalidateRepromoteLosslessTest). Those coverage claims are unverifiable here and should be confirmed against the enterprise CI run.

Minor

m1 (test-infra latch-leak asymmetry). In rangeRefresh (1774-1777), updateRefreshIntervals (2313-2316), and invalidateView (1595-1598), the seam.run() executes after tryLock() but outside the try whose finally unlocks. A throwing seam leaks the latch (wedges the view + leaks the parked cursorFactory at teardown). fullRefresh (867) and refreshIncremental0 (2175) place the seam inside the unlock-protected region. Unreachable in production (onHoldingLockForTesting is null, not copied by cloneInstance), but it's a real structural inconsistency — move each of the three seams to the first statement inside the unlocking try.
m2 (member ordering, in-diff). The new private static final Object PENDING_INVALIDATION_NO_REASON (MatViewState.java:88) is inserted after REFRESH_RETRY_AFTER_UPDATER; alphabetically P < R, so it should precede it within the private-static-final group. (The isPendingInvalidation() mis-ordering at line 562 is pre-existing on master — not this PR.)
m3 (naming, in-diff). finalizeAndUnlock(..., boolean incrementRefreshSeq) — the param is a bare verb identical to the method it gates; call sites read as bare true/false. Prefer shouldIncrementRefreshSeq. boolean selfDeferred (1600) lacks the is/has prefix — isSelfDeferred is more consistent (minor).
m4 (test fidelity, in-diff). Every seam lambda calls only state.markAsPendingInvalidation(...) — never the paired stateStore.enqueueInvalidate(...) a real losing invalidateView issues. So no single test reproduces the actual end-to-end bug (concurrent INVALIDATE loses the lock -> re-delivered task swallowed at 1561 -> finalize recovers) in one flow; the 10 tests prove finalize given a marker re-enqueues and mints. testLockContendedInvalidationDefersWithReason does exercise the real defer site, and two flagship tests were confirmed to fail pre-fix, so these are genuine regression tests — but the comments' "exactly as a losing concurrent invalidateView would" overstate the seam. Consider one test that enqueues a real second INVALIDATE and drains, or soften the comments.
m5 (coverage gap, in-diff). finalizeDeferredInvalidation's || viewState.isInvalid() early-return is OSS-reachable (a refresh that fails -> markAsInvalid with a deferral also pending) but untested. Worth a test that fails the refresh with a pending marker armed and asserts finalize does not re-enqueue and the view carries the fail reason.

Tradeoffs / regressions to weigh (documented, accepted)

These are real behavior changes the PR documents; they are strictly narrower than the pre-fix always-lost freeze, but reviewers/operators should be aware:

valid->invalid flip under a lock race the holder wins. finalize is reason-blind, not txn-aware, so a full refresh (or a force=false range/interval-update on a never-incrementally-refreshed view) that already rebuilt the view correctly can still flip it invalid and cascade to dependents when a concurrent deferral lands mid-hold — a spurious-looking flip for the "UPDATE then REFRESH FULL" flow. Verified conservatively safe (invalid is visible; REFRESH ... FULL recovers) and better than the alternative (an eventually-stale frozen-pending view that also blocks future refreshes via the pending guard). Reachable in normal operation for MANUAL/range-only views because timer-driven UPDATE_REFRESH_INTERVALS is a background lock-holder, not only under manual REFRESH ... FULL.
finalize->unlock and torn-write windows. A deferral landing between a holder's finalize-clear and its unlock is still lost (terminal silent freeze). Verified the worst case is exactly the documented freeze — no lost/double-freed factory, no half-mint, no spin. For an UPDATE/DROP-PARTITION-caused loss, recovery is REFRESH ... FULL-only (a later incremental advances lastRefreshBaseTxn past the non-TRUNCATE txn without re-detecting it).

Downgraded (draft findings dismissed after source verification)

"finalize under the latch can deadlock (isReadOnlyMode/enqueueInvalidate)." False — isReadOnlyMode() is a lock-free flag read; the task queue is unbounded/non-blocking; neither re-acquires the view latch; the WalWriter try-with-resources closes before the outer finally.
"the three assert reason != null sites can fire." False — verified every INVALIDATE producer passes a non-null reason; the only null marker enqueues FULL_REFRESH, never INVALIDATE.
"finalize double-enqueues cause an infinite INVALIDATE loop." False — the second re-delivery hits !isInvalid() and is swallowed; monotonic toward invalid.
"perf: redundant volatile reads / per-row cost / capturing lambda." False — finalize runs once per task (never per row); no-deferral path is one volatile read; seam is a plain field read, not a capturing lambda; no autoboxing, no java.util.*.
isPendingInvalidation() alphabetical mis-ordering. Pre-existing on master; the PR modifies the body only, doesn't move it — not a finding for this PR.
assertMemoryLeak(...) { assertQuery(...).noLeakCheck() } anti-pattern. False — each lambda holds many statements (DDL, inserts, job lifecycle, WAL drains, multiple assertions) that must share one leak scope; the .noLeakCheck() is correct there. Tests use .returns(...) (not .returnsOnce) throughout.

Summary

Verdict: request changes — not blocking on the correctness of the diff itself.

The core concurrency fix is sound: I traced all 6 in-diff lock-holders, the selfDeferred auth-rollback path, the single-volatile marker state machine, every INVALIDATE producer, and the force=true re-delivery, and post-fix freezes are a strict subset of pre-fix. No new correctness bug, deadlock, leak, or measurable perf cost. The two flagship tests are genuine pre-fix-failing regressions.

The one item worth acting on before merge is M1: the in-code comment and PR body claim the fix covers "every lock-holder," but REFRESH ... STATS (SqlCompilerImpl.java:3586) is a 7th holder that still does a plain unlock() and can strand the identical deferral — either cover it (preferred, bundle here) or correct the claim. M2 (body omits the selfDeferred spin fix) should be fixed so the squashed master commit is accurate. Everything else is Minor.

Findings: 5 verified as actionable (2 Moderate + 3 substantive Minor) + 2 trivial Minor; 6 draft findings dropped as false positives.
In-diff vs out-of-diff: 6 in-diff (M2 + m1-m5), 1 out-of-diff (M1).
Caveat: the enterprise questdb-ent module is absent from my workspace, so the read-only/demote-promote/no-spin enterprise test coverage the branch relies on could not be verified here.

@jovfer

Review follow-up (PR #7330, @jovfer): The REFRESH MATERIALIZED VIEW ... STATS reset in SqlCompilerImpl takes the same per-view latch as the refresh paths, synchronously on the SQL thread, but unlocked without finalizing -- a concurrent INVALIDATE deferring during that hold stayed swallowed by invalidateView's pending guard, the same terminal freeze the branch closes elsewhere. The stats path now routes its unlock through MatViewRefreshJob.finalizeAndUnlock, exposed as a public static helper shared with the job's own six lock-holders (a private instance wrapper keeps the in-class call sites unchanged). The static also runs tryCloseIfDropped/tryCloseIfClosed after the unlock, so a teardown that raced the stats hold now frees the parked cursor factory instead of leaking it. Also addressed from the review: - Move the rangeRefresh, invalidateView and updateRefreshIntervals test seams inside the unlock-protected try, matching fullRefresh and refreshIncremental0: a throwing seam can no longer wedge the latch. - Rename the finalizeAndUnlock parameter to shouldIncrementRefreshSeq and the invalidateView local to isSelfDeferred, per the boolean naming convention. - Move PENDING_INVALIDATION_NO_REASON to its alphabetical slot ahead of REFRESH_RETRY_AFTER_UPDATER. - Update the invalidateView holder-list comment to name the stats reset and note MatViewState's teardown-only tryLock holders. New tests in MatViewPendingInvalidationTrapTest (15 total now): - testStatsResetFinalizesDeferredInvalidation arms a swallowed deferral and runs REFRESH ... STATS; confirmed to fail with a plain unlock. - testRefreshHoldingLockFinalizesDeferredInvalidationWithQueuedTask drives the complete defer-site pair (marker plus the re-enqueued task the guard swallows) through a real refresh finalize; confirmed to fail with finalizeDeferredInvalidation no-op'd. - testFailedRefreshLeavesDeferredInvalidationUntouched pins finalize's isInvalid early-return: a refresh that fails with a deferral pending keeps the failure reason and leaves the marker untouched. - Seam comments now say they model the marker half of a losing invalidateView's defer instead of claiming exact equivalence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…invalidation_finalize

Review follow-up (PR #7330, jovfer's June 26 pass, item m5): the 4-line "read the volatile seam field, run if armed" idiom was copy-pasted at five lock-holder sites. runHoldingLockSeamForTesting() now carries the idiom once, with the latch-safety contract (callers invoke it inside the unlocking try) documented on the helper instead of repeated at each call site. No behavior change; the trap test class passes unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Review follow-up (level-3 pass, 2026-07-06). Comment-accuracy and style fixes only; no behavior change. - invalidateView's residuals comment now scopes the lost-update window correctly: it opens at finalize's marker read (not its clear), and it strands a first-and-only deferral through finalize's early-return path too, not only a second deferral after a clear. The old "unlike it, not self-healing" contrast read as if the pre-fix loss self-healed; the new wording states that finalize now recovers the pre-fix class while nothing recovers a deferral lost in this window. - finalizeAndUnlock's javadoc names the one deliberate exception to "every lock-holder routes through here": invalidateView's auth-rollback self-deferral unlocks inline to avoid the busy spin against a sticky read-only writer refusal. - finalizeAndUnlock's OOM comment discloses the clear-then-enqueue non-atomicity: an OOM between the two drops the deferral outright (the view reads healthy while stale), and enqueue-before-clear would be no better because a second worker could swallow the task against the still-set marker. - rangeRefresh's finally cross-references the fullRefresh tradeoff: a deferral landing mid-hold flips the view invalid even when the range refresh just recomputed the affected rows. - getPendingInvalidationReason uses an instanceof pattern variable. - The trap test class lists its two marker state-machine tests in alphabetical position. MatViewPendingInvalidationTrapTest passes 15/15 locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Round-3 review follow-up (PR #7330). M1: the no-arg markAsPendingInvalidation() now CASes the marker from null to the reschedule sentinel (AtomicReferenceFieldUpdater), so a full refresh that loses the lock race can no longer demote a concurrent reason-bearing deferral to a marker that only the re-queued full refresh clears. invalidateView's residuals comment stops claiming the sentinel "is only ever written alongside a queued full refresh that recovers it": the re-queued full refresh's short-circuit exits (refresh block list, write suspension, missing base table) return without finalizing, so a stranded sentinel is now documented as a third residual -- the same freeze the pre-fix boolean produced on those paths, cleared by a restart (the marker is in-memory) or a successful REFRESH ... FULL. M2: the defer-vs-clear hammer no longer claims "it can only fail if the torn composite returns" -- its resting-state check catches a revert whose write orders leave the torn pair at rest, and the comment now explains why transient torn states are not black-box observable (the two-getter API yields the same (true, null) observation on correct code when a clear lands between a reader's two reads). The hammer shrinks to 50 rounds. The new testConcurrentSentinelMarkNeverDemotesReasonDeferral pins the keep-strongest CAS with a single-read reader; it and the extended state-machine test both fail against a plain last-write-wins sentinel write (verified by temporarily reverting the CAS). Minors from the review: - finalizeDeferredInvalidation folds its guard into one marker read. - finalizeAndUnlock's javadoc documents shouldIncrementRefreshSeq, and its window comment names the self-fed variant (finalize's under-latch enqueue feeds sibling pollers) plus the enqueue-after-unlock follow-up shape. - unlockAndTryClose dedupes the unlock tail shared by finalizeAndUnlock and the isSelfDeferred branch; the deferral-reason assert message moves to a constant. - New testDroppedViewLeavesDeferredInvalidationUntouched pins finalize's isDropped early-return and the tryCloseIfDropped leak path for a view dropped mid-hold. - The trap test class javadoc lists both OSS-uncovered branches (the read-only early-return and the isSelfDeferred skip) and names the enterprise tests that cover them. - SqlCompilerImpl's stats-reset comment shrinks to a cross-reference. MatViewPendingInvalidationTrapTest passes 17/17; the mat view suites pass 379 tests (1 pre-existing timestamp-type Assume skip). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Two new tests drive fullRefresh's real tryLock-fail branch -- the only production caller of the no-arg sentinel mark -- end-to-end: a drainer thread spins on the republished FULL_REFRESH task while the test thread holds the latch. testFullRefreshLosingLockArmsSentinelAndRecovers pins the arm/recover lifecycle (sentinel armed with a null reason, the re-queued full refresh clears it, the view ends valid). testFullRefreshLosingLockCannotDemoteReasonDeferral pins the keep-strongest CAS in integration form: the live losing branch cannot demote a reason-bearing deferral. It fails within half a second against a plain last-write-wins sentinel write (verified by a temporary revert). The invalidateView guard comment now enumerates the residuals as a list and adds three verified variants: timer-driven refreshes re-running the finalize->unlock window once finalize clears the marker, a sentinel CASed into the just-cleared marker swallowing finalize's own re-enqueue, and the start-of-fullRefresh window where resetInvalidState wipes a deferral whose queued task a sibling already swallowed. The rewrite drops the tear/CAS mechanics that duplicated the MatViewState field comment. finalizeDeferredInvalidation's header now names the String overload the defer sites actually call, and both "tracked follow-up" claims soften to "a possible follow-up" (no issue is filed yet). Smaller test changes: the marker state machine covers the (String) null routing into the sentinel CAS; the incremental-holder test asserts the refresh seq bumps through finalizeAndUnlock; the demotion hammer's reader spins with Thread.onSpinWait(); the contended-defer test asserts against UpdateOperation.MAT_VIEW_INVALIDATION_REASON instead of a literal. unlockAndTryClose moves after finalizeDeferredInvalidation to restore alphabetical order. All eight mat view suites pass: 381 tests, one pre-existing Assume-based timestamp-type skip in MatViewTest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The comment claimed the counter tracks every FULL enqueue reaching the engine-installed store across all callers. It does not: only InstrumentedStateStore's 2-arg enqueueFullRefresh(TableToken, Object) override increments it. ForwardingMatViewStateStore's 1-arg overload forwards straight to the delegate without routing through the 2-arg method, and impl-internal redeliveries push onto the store's private task queue directly, bypassing the wrapper entirely. Reword the comment to state that narrower, accurate scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…invalidation_finalize

MatViewTimerJob.processRefreshRetries keys the retry-heap gate on hasPendingInvalidationReason(). Nothing drove that path with a pending marker parked, so reverting the gate to isPendingInvalidation() left the whole tree green: the retry heap is the only path that wakes up immediate views, which have no timer of their own, so a stuck owner-only marker would silently freeze a busy-backoff view until the next base commit. Add testRetryHeapRedrivesRefreshDespiteOwnerOnlyMarker, the retry-heap analog of testTimerSchedulesRefreshDespiteOwnerOnlyMarker. It arms a backoff deadline via scheduleRefreshRetry plus a matching RETRY timer entry, parks an owner-only marker, and asserts the due retry still re-drives the incremental refresh and clears the backoff deadline. Add engineIncrementalRefreshEnqueues, a counter on InstrumentedStateStore that tracks enqueueIncrementalRefresh calls reaching the engine-installed store wrapper, following the same pattern as the existing engineFullRefreshEnqueues counter.

finalizeAndUnlock0's invalidation-wake catch arms the FULL retry flag via requestPendingFullRefreshReenqueue for a co-pending owner, in addition to arming the invalidation retry flag, before re-throwing. No existing test reached those lines: the only both-facets finalize test throws from the FULL enqueue rather than the INVALIDATE one, so the FULL arm inside the INVALIDATE catch was untested. Add testFinalizeBothFacetsInvalidationWakeFailureArmsBothRetryFlags, which finalizes through a store whose INVALIDATE wake throws first while a FULL owner is also pending. It then drives the retry scan directly on MatViewStateStoreImpl and inspects the drained queue: one scan must redeliver both the INVALIDATE and the FULL_REFRESH task with its owner. Without the arm, only the INVALIDATE facet comes back, which the added assertion isolates.

@testonly

MatViewStateStoreImpl's enqueueFullRefresh owner overload, enqueueInvalidate0, reenqueueRefreshTask, and the per-state catch inside reenqueueFailedPendingTasks each catch a queue-append failure and arm a retry flag so a later scan redelivers the facet -- but the private task queue never actually throws, so no test could reach any of the four catches. Add a @testonly Runnable seam that MatViewStateStoreImpl fires immediately before both private queue-append sites: the shared enqueueMatViewTask tail (which the owner overload and enqueueInvalidate0 funnel through) and reenqueueRefreshTask's direct append, which bypasses enqueueMatViewTask entirely. A single-site seam would leave the put-back catch permanently unreachable, so the seam fires at both. Four new tests drive each catch through a genuine queue-append failure and red-verify it: the owner-overload catch, the invalidate catch, both arms of the put-back catch, and the per-state scan catch. The scan-catch test arms both facets and inspects the raw queue directly, rather than through a job drain -- a job drain's finalize-unlock handoff independently re-wakes whatever the marker still holds, which would mask the very recovery path the test needs to isolate. Rewrite testFinalizeBothFacetsInvalidationWakeFailureArmsBothRetryFlags to call the new resolveStateStoreImpl() helper instead of its inline unwrap cast, and correct InstrumentedStateStore's docstring now that a genuine append failure can be injected through the real store.

Pin four previously uncovered branches in MatViewRefreshJob with two new one-shot getWalWriter failure hooks (walSuspendedRefusalToken, walGenericFailureToken) added to the trap test's engine factory. - fullRefresh's outer catch defers a suspension landing between the isViewWriteSuspended gate and the writer acquire: it retains the owner instead of failing, and the finalize wakes it exactly once so the redelivered attempt (finding the one-shot refusal consumed) rebuilds and leaves the view valid. - invalidateView's mint-loop catch retains the pending marker on the same suspend-window race instead of letting the CairoException escape run(); the finally's finalize wakes it and the redelivery mints the invalid state with the retained reason. - fullRefresh's generic terminal else-branch (non-suspend, non-auth, non-rename writer failure) fails the view state and clears the pending-full-refresh owner, so a stranded sentinel cannot wake into the same failure forever. - A stale full-refresh handoff task, whose owner terminal cleanup already consumed before delivery, is a no-op: the early !isPendingFullRefreshOwner return skips the lock, truncate, and rebuild entirely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

REFRESH ... STATS takes the per-view latch like any refresh, so its unlock must finalize a deferred invalidation through MatViewRefreshJob#finalizeAndUnlock. That finalize releases the latch before attempting any wake enqueue, so by the time a wake can fail, the stats reset has already durably succeeded. Previously a wake enqueue failure propagated out of the statement anyway, turning an already-successful stats reset into a reported failure. Wrap the finalize call in a try/catch that logs and swallows, mirroring the identical contract in alterTableResume for ALTER TABLE RESUME WAL (SqlCompilerImpl.java:1661-1672): the finalize's own catch in MatViewRefreshJob#finalizeAndUnlock0 arms the invalidation retry flag on the engine's canonical store before rethrowing, so recovery rides those retry flags on the next refresh-job tick. Adds a trap test to MatViewPendingInvalidationTrapTest that arms an injected wake-enqueue failure, confirms the STATS statement still succeeds and releases the latch, then drains the queue to confirm the retry flag redelivers the invalidation. A new failure-surface entry for this contract lands in the PR body in a later task. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Four independent, behavior-preserving refactors to MatViewPendingInvalidationTrapTest, collapsing duplication that accumulated across review rounds: - Add assertPriceViewValid() and replace all 22 exact-match sites of the 3-column "valid" baseline query block with a single call. - Add assertHolderFinalizesDeferredInvalidation(reason, trigger) and collapse the four holding-lock tests that only differ by trigger and reason (FULL, RANGE, single-view INCREMENTAL, UPDATE REFRESH INTERVALS) into one-line calls. - Replace the anonymous counting ForwardingMatViewStateStore in testStickyWriterRefusalStillWakesFullRefreshFacet with the existing CountingStateStore, verified to have identical delegate/bound semantics. - Replace three hand-written base_price CREATE TABLE blocks with the existing createBasePriceTable() helper. Two deliberate non-changes: testRefreshHoldingLockFinalizesDeferredInvalidation stays standalone (insert-driven, reordered drains, extra refreshSeq oracle -- it deviates from the four-way template). The splittingStore in testFinalizePartialFullWakeFailureRedrives stays anonymous (its FULL facet throws unconditionally, a combination CountingStateStore doesn't support). Full trap suite: 73/73 tests pass, same count as before this change.

Four mechanical, behavior-preserving changes flagged in r20 review: - Reorder MatViewState's two markAsPendingInvalidationAndGetMarker overloads to sit before markAsPendingInvalidationForTesting, matching alphabetical member order ("AndGetMarker" sorts before "ForTesting"). - Rename the local didRefresh to refreshed in MatViewRefreshJob.fullRefresh, matching the file's established name for the insertAsSelect result and the refresh accumulators. - Rename the AtomicBoolean latchRescued to isLatchRescued in the three trap tests that use it, applying the is-prefix boolean convention. - Drop the shadowed delegate field in InstrumentedStateStore and route all six internal delegate. calls through the inherited getDelegate(); the store never delegate-swaps, so the volatile read is equivalent. Also replace the two remaining inline (MatViewStateStoreImpl) ((ForwardingMatViewStateStore) engine.getMatViewStateStore()).getDelegate() casts in testEnqueueOomWhileRedrivingRestoresSignal and testPendingTaskReenqueueScanIsSingleRunner with the existing resolveStateStoreImpl() helper, so every site now unwraps the same way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…invalidation_finalize Master renamed MatViewGraph to DependentViewGraph and switched the accessor to CairoEngine.getDependentViewGraph() as part of the live-view feature, which collided with the branch's test-only MatViewRefreshJob constructor that injects a MatViewStateStore. The resolution keeps both: the renamed graph accessor from master and the injected state store from the branch. Everything else auto-merged. Master's mat-view edits in this window are the rename plus an idempotent MatViewStateStoreImpl.close(); its ApplyWal2TableJob, WalPurgeJob and SqlCompilerImpl changes add live-view handling in the new io.questdb.cairo.lv package, which carries its own state store and refresh job and does not touch the mat-view invalidation state machine.

jovfer · 2026-08-19T13:03:08Z

Review — level 3 (full PR net diff)

Reviewed the complete branch net diff (base 714b7500 → head 3fd8056d, all 54 commits collapsed as they will squash-merge; 12 files, +5,209/−92, 74 new tests), including the r19/r20 hardening rounds. Adversarial falsification across correctness/state-machine, concurrency, cross-context caller impact, a fresh-eyes diff-only pass, and test efficacy.

Verdict: approve. Both gates pass.

Findings

No Critical, no Minor. One Moderate coverage gap (non-blocking):

M1 — the base-table token-mismatch arm of the FULL-refresh coverage check is untested.
clearPendingInvalidationIfCoveredByLastFullRefresh() (MatViewState.java:450) declines to clear a pending invalidation when !coverage.baseTableToken.equals(pending.invalidationBaseTableToken) with both tokens non-null and different — the guard that stops a FULL covered under base-table epoch A from clearing an invalidation stamped under a different epoch B (reachable via a base-table drop+recreate that changes the token between the coverage record and the pending mark). The two sibling arms — null/unknown-token collapse (line 448) and txn-newer (line 451) — are each covered, but this cross-epoch arm is not driven by any of the 74 tests. The code is correct; this is a coverage gap, not a defect. Adding a test needs either a real cross-epoch base swap in the fixture or a small @TestOnly seam over recordFullRefreshSuccess (package-private, so the current suite reaches coverage only through full-refresh flows that never produce a token mismatch).

Suggested test: record FULL coverage under base token A, publish a pending invalidation carrying a distinct non-null base token B with txn <= coverageTxn, assert the marker is not cleared.

What was verified (in-diff and out-of-diff)

Two-sided handoff memory safety (the core property): holds by the JMM synchronization order through the shared latch — a publisher whose tryLock() fails (observing the latch held) forces publisher-marker-write →ˢᵒ tryLock-fail →ˢᵒ holder-unlock →ˢᵒ holder-marker-read, so the holder's post-release read is guaranteed to observe the publish. No lost wake. Cross-checked with an independent proof.
FULL coverage soundness: toBaseTxn is baseTableReader.getSeqTxn() (the local at findRefreshIntervals:729, not the lastRefreshTxn parameter), so a snapshot at seqTxn S provably contains every base change at txn ≤ S; the merge is conservative (max-txn = less coverage claimed, unknown/mismatched provenance collapses to non-clearable). Cannot clear an uncovered invalidation.
Cascade-only-on-mint is a bug fix (base over-invalidated children on a decline), not a regression.
Gate flips isPendingInvalidation() → hasPendingInvalidationReason(): zero remaining production callers of the old method; a parked full-refresh owner never freezes refresh/purge and is redelivered by the next holder's finalize; WalPurgeJob is strictly more conservative and its first-refresh protection is intact via the independent getLastRefreshBaseTxn() == -1 term (a FULL reads a fixed reader snapshot, not WAL segments).
Provenance flow (ApplyWal2TableJob stamp → copyTo → queue → invalidateView): no stale leak — mvRefreshTask.clear() resets provenance each batch, it is stamped only on a genuine INVALIDATE, and the batch-end frontier deliberately matches the pending.txn ≤ coverage.txn rule.
Interface additions are overridden by all three OSS implementers and no-oped/forwarded correctly; external callers of the compat shims (CairoEngine hydration invalidations, user REFRESH FULL) route to the correct forced / owner-minting behavior.
Worker safety: the deliberate rethrow from finalizeAndUnlock0 on enqueue failure does not crash the pool — matViewRefreshWorkerHaltOnError defaults false, and retry flags are armed before the throw.
Standards: member/field ordering cleanly alphabetized within kind/visibility groups; all new LOG chains terminate; no dead code, reflection, or debugging residue on changed lines.

Test suite

The new MatViewPendingInvalidationTrapTest (74 tests) is a strong verification backbone: deterministic seam-driven interleavings on a single drain thread (no Thread.sleep), the two thread-spawning tests route spawned-thread throwables through AtomicReference + join(30s), assertMemoryLeak wraps every allocating test, and the core regression is redundantly pinned with real view_status = invalid oracles on materialized_views. Zero banned SQL-assertion patterns.

Tradeoffs (author-acknowledged, confirmed accurate)

Per-publication ownership-object allocation on refresh-task boundaries (not per row); conservative over-invalidation when base-token/txn provenance is unknown (may require REFRESH ... FULL even when a refresh happened to include the change); STATS / RESUME WAL swallow a post-release wake-enqueue failure (recovered via retry flags on a later tick). All reasonable and clearly documented.

No submodule pointer moved; the whole change is in-scope in core. No out-of-diff breakage found. Recommend merge; optionally add the M1 test as a follow-up.

jovfer · 2026-08-19T16:20:19Z

/azp run macwin

azure-pipelines · 2026-08-19T16:20:33Z

Azure Pipelines successfully started running 1 pipeline(s).

…invalidation_finalize

bluestreak01

Review — PR #7330, level 3

fix(core): keep materialized views current after concurrent invalidation — branch sm_matview_pending_invalidation_finalize, head 3b7c7a30, base 271ec07c. Full net diff (12 files, +5209/−92, 74 new tests) reviewed as it will squash-merge.

Step 2.4 — submodule provenance: no submodule pointer moved in this diff (git diff 271ec07c...HEAD --submodule=short returns no Subproject commit lines; git status --porcelain is clean). The whole change is in-scope core.

Step 2 — PR metadata: title is Conventional Commits with the verb repeated and end-user impact; body is level-headed, states tradeoffs and regressions with equal weight, has a plain-bullet test plan and an explicit out-of-scope list; labels (Bug, SQL, Core, WAL, regression, Materialized View) match the scope. No Fixes #NNN needed — this fixes a master-only regression from #7261, not a filed issue. No findings.

Critical

None.

Moderate

M1 — the cross-epoch arm of the FULL coverage check is unpinned

Problem: Token-mismatch guard in the coverage check has no test.
Net impact: Silent removal would let a FULL clear a foreign-epoch invalidation.
Evidence: rg over all of core/src/test at 3b7c7a30 finds one non-base TableToken (MatViewPendingInvalidationTrapTest.java:1917), used only at :1937 in the merge test, which asserts provenance collapses to null; recordFullRefreshSuccess is package-private and no test lives in io.questdb.cairo.mv.

core/src/main/java/io/questdb/cairo/mv/MatViewState.java:450:

|| !coverage.baseTableToken.equals(pending.invalidationBaseTableToken)

This is the only clause that stops a FULL rebuild recorded under base-table token A from consuming a pending invalidation stamped under a different token B. Base txn numbering is per-table and incomparable across a drop+recreate, so without it a low-txn marker from epoch B compares <= a high-txn coverage from epoch A and is cleared.

The three sibling arms are each pinned (coverage == null and invalidationBaseTableToken == null by testFullRefreshConsumesProvenanceFreeMarkerCoveredByItsSnapshot; invalidationBaseTxn > coverage.baseTableTxn by testFullRefreshKeepsInvalidationNewerThanFixedSnapshot). This one is not.

Regression consequence: invalidateView returns early at MatViewRefreshJob.java:1873, so materialized_views.view_status stays valid and invalidation_reason stays empty while the view's rows do not reflect the change. No error, no log line, no operator signal.
Reachability / population: operators running mat views (cairo.mat.view.enabled defaults true) who do a base-table identity swap (RENAME TABLE base TO tmp; RENAME TABLE other TO base; or DROP TABLE base; CREATE TABLE base ... WAL;) while a provenance-bearing INVALIDATE from a base UPDATE/TRUNCATE is in flight. This is not a scriptable DDL sequence: the swap's own cascade publishes a provenance-free forced marker whose merge collapses the token to null (MatViewState.java:823-830), so the mismatch arm needs the swap to land inside the window between the UPDATE's marker publication and its consumption — a multi-worker interleaving. That narrowness is why this is Moderate, not Critical.
Change risk: new method, new field, no compile-time or lint safeguard (pom.xml/core/pom.xml/.github have no -Werror, checkstyle, spotbugs, or errorprone), and the mutant compiles because coverage stays used on the next line.
Least-fragile test, no new seam required: after a successful REFRESH MATERIALIZED VIEW price_1h FULL (coverage txn C, lastRefreshBaseTxn != -1), call the already-public 5-arg MatViewStateStore.enqueueInvalidate(viewToken, "update operation", foreignBaseToken, 1, false) with the synthetic foreignBaseToken at :1917, drain, and assert view_status = 'invalid' / invalidation_reason = 'update operation'. At head the mismatch arm declines and the mint gate's third disjunct mints; with the clause deleted (1 <= C) the view stays valid.

M2 — the `isInvalidViewRecovery` disjunct in the mint gate is masked in every existing test

Problem: Mint-gate recovery disjunct never decides an assertion.
Net impact: Silent removal drops a cascade and reports a stale invalidation reason.
Evidence: MatViewRefreshJob.java:1886; the four conditions are never simultaneously satisfied — the closest test (testFullRefreshKeepsInvalidationNewerThanFixedSnapshotWhenRecoveringInvalidView:1099-1145) fires its seam while fullRefresh holds the latch, so invalidateView returns at the tryLock at :1857 and never reaches :1886; every other owner-parking test publishes through the 1-arg/2-arg enqueueInvalidate, which defaults isInvalidationForced = true and satisfies the first disjunct on its own.

if (isPendingInvalidationForced || isInvalidViewRecovery || viewState.getLastRefreshBaseTxn() != -1) {

The entry gate's isInvalidViewRecovery term (:1829) is tested and is what prevents the silent-stale class. The mint-gate term is a separate branch reached when the invalidate wins the free latch: isForced == false (a base cascade) and getLastRefreshBaseTxn() == -1 (never successfully refreshed) and isInvalid() and an owner parked by a REFRESH ... FULL that lost the latch.

Regression consequence: the invalidation is declined at :1949 instead of minted — the view stays invalid either way, so this is not a data-correctness gap, but materialized_views.invalidation_reason keeps the older reason and enqueueInvalidateDependentViews(...) at :1960-1961 never fires, so chained views are not re-invalidated by that event. No later path re-derives the missed cascade (enqueueInvalidateDependentViews call sites: :681, :1961, :2341, :2765).
Least-fragile test: createUnseededAutoPriceViewFixture() → enqueueInvalidate(viewToken, "seed invalidation") + drain → state.markAsPendingFullRefreshForTesting() → enqueueInvalidate(viewToken, "update operation", baseToken, 1, false) + drain → assert invalidation_reason = 'update operation'. Uses only existing @TestOnly public seams.

Minor

None.

Coverage map

Test gate: PASS. Two admitted gaps, both Moderate (M1, M2); zero admitted Critical gaps. Rendered above with their recorded searches and failure links. Remaining rows are COVERED, ACCEPTED, or EXEMPT and stay private.

Independent runtime evidence at the reviewed revision: all completed Azure jobs are green — Cairo A/B, Griffin, Other A/B, and the coverage/fuzz runs across linux-arm64, linux-x64-zfs, and linux-x86-graal (build 263460); GitHub build and gitleaks pass. The coverage bot reports 454/469 new lines covered (96.80%).

Scope of verification (level 3)

Callsite analysis walked outward from the diff; zero out-of-diff breakage found, and both admitted items are in-diff.

Two-sided handoff. Exhaustive latch inventory: production tryLock() sites are MatViewRefreshJob:565, 992, 1856, 2061, 2233, 2382, 2640, SqlCompilerImpl:3867, and MatViewState:489, 1161, 1171. The first eight all release through finalizeAndUnlock*; the three MatViewState ones acquire only when closed/dropped, exactly the states finalizeAndUnlock0:470-474 short-circuits on. The only production MatViewState.unlock() is unlockAndTryClose (:540). The publish→failed-tryLock→unlock→post-read ordering is a correct volatile handoff for both facets.
Coverage soundness. ApplyWal2TableJob:658 stamps the batch-end writer.getSeqTxn() (an upper bound, making coverage strictly harder to claim); findRefreshIntervals:813 stamps toBaseTxn from the same numbering. Every provenance-free publisher (CairoEngine:4290/4299/4334/4345) collapses to a null token, which the coverage check rejects unconditionally, so a forced/hydration invalidation can never be swallowed.
Gate flips. WalPurgeJob:541 is neutral-or-more-conservative: identical for a reason-bearing marker, and for an owner-only marker it pins the retention floor where base released it. First-refresh protection via getLastRefreshBaseTxn() == -1 is intact. MatViewTimerJob:252/318 and the four MatViewRefreshJob refresh gates are pinned by dedicated tests.
Suspension/resume. ALTER ... RESUME WAL → reenqueuePendingOnResume redelivers both facets; base dropped the invalidation permanently for the same trigger. The residual operator hazard (lifting a config-list suspension without running RESUME WAL) is covered by the documented procedure on CairoEngine.isWalApplySuspended and is recoverable via REFRESH ... FULL, REFRESH ... STATS, or restart.
Standards on changed lines. All new LOG chains terminate with .I$(); no .put() in a chain; no throw-capable expression inside a chain. Members are ordered per the file's alphabetical-within-kind convention. No java.util on a data path, no TODO/FIXME, no non-ASCII in log text. Test file: no assertSql(, no returnsOnce(, no reflection, no Thread.sleep, no debugging residue; both threaded tests route spawned-thread Throwable through an AtomicReference with a bounded join.
Tradeoffs confirmed accurate. Per-publication PendingInvalidation/owner allocation is on refresh-task boundaries, not per row. The isInvalidViewRecovery widening does add a redundant setInvalidState WAL transaction and a cascade on an already-invalid view — bounded to one per base-invalidation event inside a latch-contention window, non-amplifying (each iteration consumes one INVALIDATE and adds one no-op FULL), and the child cascade short-circuits at the child's own !isInvalid gate. The finalizeAndUnlock0 rethrow escaping a finally is OOM-gated and does not halt the pool (mat.view.refresh.worker.haltOnError defaults false, PropServerConfiguration:1591; Worker:422-437 logs and continues).

Summary

Verdict: approve with comments. Both gates pass. I would like M1 landed on this branch — it is the one unpinned guard whose silent removal produces stale data reported as valid, and the test costs one existing-API call. M2 is optional.

Correctness gate: PASS — no admitted Critical finding.
Test gate: PASS — no admitted Critical coverage gap; 2 Moderate gaps.
Severity distribution: 0 Critical, 2 Moderate, 0 Minor. Admitted split: 2 in-diff, 0 out-of-diff-breakage.
Submodule provenance: no pointer moved — entire change in scope.
Regressions/tradeoffs carried by this change, all disclosed in the PR body and confirmed accurate: bounded redundant invalid-state WAL churn on an already-invalid view during a full-refresh latch window; conservative over-invalidation when base-token/txn provenance is unknown; REFRESH ... STATS and ALTER ... RESUME WAL now swallow a post-release wake-enqueue failure and rely on the store's retry flags.
Cross-repo note (not a finding, not in the verdict): the six MatViewStateStore additions have no default bodies, and MatViewState.isPendingInvalidation() kept its name while changing meaning (it now also returns true for an owner-only marker). Any enterprise implementer or caller needs the paired change; the PR body already schedules the pin bump.

bluestreak01 · 2026-08-22T10:21:41Z

/azp run macwin

azure-pipelines · 2026-08-22T10:21:51Z

Azure Pipelines successfully started running 1 pipeline(s).

ideoma · 2026-08-22T11:03:22Z

[PR Coverage check]

😍 pass : 454 / 469 (96.80%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/cairo/mv/NoOpMatViewStateStore.java	1	6	16.67%
🔵	io/questdb/cairo/mv/MatViewState.java	132	139	94.96%
🔵	io/questdb/cairo/mv/MatViewStateStoreImpl.java	120	122	98.36%
🔵	io/questdb/cairo/mv/MatViewRefreshJob.java	159	160	99.38%
🔵	io/questdb/cairo/wal/WalPurgeJob.java	1	1	100.00%
🔵	io/questdb/cairo/mv/MatViewRefreshTask.java	11	11	100.00%
🔵	io/questdb/griffin/SqlCompilerImpl.java	13	13	100.00%
🔵	io/questdb/cairo/mv/ForwardingMatViewStateStore.java	12	12	100.00%
🔵	io/questdb/cairo/mv/MatViewTimerJob.java	2	2	100.00%
🔵	io/questdb/cairo/wal/ApplyWal2TableJob.java	3	3	100.00%

jovfer added Bug Incorrect or unexpected behavior Materialized View labels Jun 25, 2026

jovfer added the regression label Jun 26, 2026

jovfer and others added 2 commits June 26, 2026 02:39

jovfer added the Core Related to storage, data type, etc. label Jun 26, 2026

jovfer mentioned this pull request Jun 26, 2026

fix(core): fix chained materialized views left stale after a parent view is invalidated #7337

Draft

jovfer and others added 2 commits June 26, 2026 16:35

Merge remote-tracking branch 'origin/master' into sm_matview_pending_…

a61d99d

…invalidation_finalize

jovfer added the READY PR is ready for the final review label Jun 26, 2026

bluestreak01 requested changes Jul 6, 2026

View reviewed changes

jovfer removed the READY PR is ready for the final review label Jul 6, 2026

jovfer and others added 4 commits July 6, 2026 16:21

Merge remote-tracking branch 'origin/master' into sm_matview_pending_…

ba558d5

…invalidation_finalize

jovfer force-pushed the sm_matview_pending_invalidation_finalize branch from a2bdb42 to a746808 Compare July 6, 2026 17:48

jovfer changed the title ~~fix(core): fix materialized views left stale after a concurrent invalidation~~ fix(mv): fix materialized views left stale after a concurrent invalidation Jul 6, 2026

jovfer changed the title ~~fix(mv): fix materialized views left stale after a concurrent invalidation~~ fix(core): fix materialized views left stale after a concurrent invalidation Jul 7, 2026

jovfer and others added 10 commits July 17, 2026 15:22

Merge remote-tracking branch 'origin/master' into sm_matview_pending_…

538c9c5

…invalidation_finalize

Merge remote-tracking branch 'origin/master' into sm_matview_pending_…

3bbff9c

…invalidation_finalize

nwoolmer assigned jovfer Jul 21, 2026

jovfer enabled auto-merge (squash) July 27, 2026 16:18

jovfer disabled auto-merge July 27, 2026 16:18

questdb deleted a comment from mtopolnik Aug 19, 2026

jovfer added READY PR is ready for the final review and removed READY PR is ready for the final review labels Aug 19, 2026

jovfer added the READY PR is ready for the final review label Aug 20, 2026

Merge remote-tracking branch 'origin/master' into sm_matview_pending_…

3b7c7a3

…invalidation_finalize

bluestreak01 approved these changes Aug 21, 2026

View reviewed changes

bluestreak01 added the QUEUED FOR MERGE Approved PR in the merge queue. Do not merge master into this PR. label Aug 21, 2026

Merge branch 'master' into sm_matview_pending_invalidation_finalize

cb662aa

bluestreak01 merged commit d55ccfd into master Aug 23, 2026
56 checks passed

bluestreak01 deleted the sm_matview_pending_invalidation_finalize branch August 23, 2026 13:53

Conversation

jovfer commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

Coordinate invalidation and full-refresh ownership

Make full refresh snapshot-aware

Defer instead of spinning on read-only refusal

Preserve intent across suspension and non-mint exits

Hardening from external review (r19)

Hardening from external review (r20)

Effects and tradeoffs

Out of scope (follow-ups)

Test plan

Uh oh!

coderabbitai Bot commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

❌ Failed checks (1 warning)

Uh oh!

jovfer commented Jun 26, 2026

Uh oh!

azure-pipelines Bot commented Jun 26, 2026

Uh oh!

jovfer commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review — PR #7330: fix(core): fix materialized views left stale after a concurrent invalidation

What the PR does

Critical

Moderate

M1 — A 7th lock-holder (REFRESH ... STATS) is not finalized; the PR's "every lock-holder finalizes" claim is inaccurate (out-of-diff)

M2 — PR body omits the selfDeferred busy-spin fix (commit 3cc2e1285f); the squashed commit message will be incomplete (in-diff / metadata)

Minor

m1 — Test seam can leak the view latch on three paths (in-diff, test-infra only)

m2 — Tests model the deferral via the seam but never enqueue the competing INVALIDATE, so the guard-swallow is not exercised end-to-end (in-diff, test fidelity)

m3 — Boolean naming (in-diff)

m4 — Missing OSS test for finalize's isInvalid() early-return (in-diff, coverage)

m5 — Seam idiom duplicated at 5 sites; branch commit titles >50 chars (in-diff, trivial)

Documented residuals (verified real, accurately scoped, intentionally accepted)

Downgraded (draft findings dismissed after source verification)

Summary

Uh oh!

jovfer commented Jun 26, 2026

Uh oh!

azure-pipelines Bot commented Jun 26, 2026

Uh oh!

bluestreak01 left a comment

Choose a reason for hiding this comment

What I verified (the core fix is sound)

Critical

Moderate

M1 — A 7th lock-holder (REFRESH ... STATS) is not finalized; the PR's "every lock-holder" claim is inaccurate (out-of-diff, pre-existing)

M2 — PR body omits the selfDeferred busy-spin fix; the squashed commit message will be incomplete (metadata)

Minor

Tradeoffs / regressions to weigh (documented, accepted)

Downgraded (draft findings dismissed after source verification)

Summary

Uh oh!

jovfer commented Aug 19, 2026

Review — level 3 (full PR net diff)

Findings

What was verified (in-diff and out-of-diff)

Test suite

Tradeoffs (author-acknowledged, confirmed accurate)

Uh oh!

jovfer commented Aug 19, 2026

Uh oh!

azure-pipelines Bot commented Aug 19, 2026

Uh oh!

bluestreak01 left a comment

Choose a reason for hiding this comment

Review — PR #7330, level 3

Critical

Moderate

M1 — the cross-epoch arm of the FULL coverage check is unpinned

M2 — the isInvalidViewRecovery disjunct in the mint gate is masked in every existing test

Minor

Coverage map

Scope of verification (level 3)

Summary

jovfer commented Jun 25, 2026 •

edited

Loading

coderabbitai Bot commented Jun 25, 2026 •

edited

Loading

jovfer commented Jun 26, 2026 •

edited

Loading

Code Review — PR #7330: `fix(core): fix materialized views left stale after a concurrent invalidation`

M1 — A 7th lock-holder (`REFRESH ... STATS`) is not finalized; the PR's "every lock-holder finalizes" claim is inaccurate (out-of-diff)

M2 — PR body omits the `selfDeferred` busy-spin fix (commit `3cc2e1285f`); the squashed commit message will be incomplete (in-diff / metadata)

m4 — Missing OSS test for finalize's `isInvalid()` early-return (in-diff, coverage)

M1 — A 7th lock-holder (`REFRESH ... STATS`) is not finalized; the PR's "every lock-holder" claim is inaccurate (out-of-diff, pre-existing)

M2 — PR body omits the `selfDeferred` busy-spin fix; the squashed commit message will be incomplete (metadata)

M2 — the `isInvalidViewRecovery` disjunct in the mint gate is masked in every existing test