fix: DH-23494: Close some rowset leaks.

[cpwright]

No description provided.

[github-actions]

No docs changes detected for be79f1a

[Copilot]

Pull request overview

Closes leaked RowSet, iterator, chunk, and filter-execution resources across the table engine.

Changes:

• Adds deterministic resource cleanup and ownership handling.
• Closes unused update row sets.
• Improves partial-construction cleanup for formula contexts.

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
LeaderTableFilter.java	Closes temporary and unconsumed row sets.
WhereListener.java	Defines refilter input ownership.
SyncTableFilter.java	Closes temporary and empty-update row sets.
TimeTable.java	Avoids allocating unnotified update ranges.
TableUpdateValidator.java	Closes validation row sets.
SymbolTableCombiner.java	Manages verification resources.
StaticChunkedCrossJoinStateManager.java	Manages verification resources.
ShortSparseArraySource.java	Closes shift iterator.
ObjectSparseArraySource.java	Closes shift iterator.
LongSparseArraySource.java	Closes shift iterator.
IntegerSparseArraySource.java	Closes shift iterator.
FloatSparseArraySource.java	Closes shift iterator.
DoubleSparseArraySource.java	Closes shift iterator.
CharacterSparseArraySource.java	Closes shift iterator.
ByteSparseArraySource.java	Closes shift iterator.
BooleanSparseArraySource.java	Closes shift iterator.
SnapshotIncrementalListener.java	Closes copy-only row set.
SliceLikeOperation.java	Closes slice temporaries.
FormulaKernelAdapter.java	Cleans partial context construction.
RightIncrementalChunkedCrossJoinStateManager.java	Manages verification resources.
QueryTable.java	Consumes filter result resources.
RightIncrementalNaturalJoinStateManagerTypedBase.java	Closes freed duplicate row sets.
IncrementalNaturalJoinStateManagerTypedBase.java	Closes freed duplicate row sets.
LeftOnlyIncrementalChunkedCrossJoinStateManager.java	Manages verification resources.
InitialFilterExecution.java	Transfers initial input ownership.
CrossJoinModifiedSlotTracker.java	Closes per-slot row sets.
AsOfJoinHelper.java	Closes built addition row sets.
AbstractFilterExecution.java	Adds filter resource lifecycle handling.

Suppressed comments (1)

engine/table/src/main/java/io/deephaven/engine/table/impl/AbstractFilterExecution.java:718

• The completion callback has already taken ownership of localInput's result. If that callback throws (for example while notifying downstream), iterateSerial routes the exception here and this closes the handed-off row set again, potentially masking the original exception with a WritableRowSet.close() NPE or invalidating an update. Clear localInput immediately before invoking the callback so this failure cleanup only closes results that were never handed off.

                    // On success localInput's value is given to the completion callback; on failure nothing else owns
                    // the partially filtered result.
                    try (final SafeCloseable ignored = localInput.get()) {
                        errorAndRelease.accept(exception);

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java:1280

• This callback likewise receives ownership before constructing previouslyMatched. If that construction or either row-set mutation fails, completeRefilterUpdate is never reached and the scheduler cannot reclaim adds or mods because it cleared localInput before invoking the callback. Guard both outputs for the callback's entire body.

                filterExecution.scheduleCompletion((adds, mods) -> {

[cpwright]

Correct, fixed in 729af9c. Worth noting this one predates the PR rather than being introduced by it — but it is in a method the PR already touches, so it belongs here.

Verified the ownership model before changing anything: PushdownResult implements SafeCloseable and its close() does SafeCloseable.closeAll(match, maybeMatch), and the other two paths out of onPushdownComplete do account for it — the bubbled-up-filter path transfers via sf.pushdownResult = pushdownResult to StatelessFilter.close(), and the final-filter path closes it in a try-with-resources. Only the fully-resolved branch dropped it, which is the common success case.

if (pushdownResult.maybeMatch().isEmpty()) {
    // The filter is fully resolved, so nothing downstream needs the result; localInput takes a copy of
    // its match.
    try (final PushdownResult ignored = pushdownResult) {
        replace(localInput, pushdownResult.match().copy());
    }
    scheduleAndSortCostEstimates(...);
    return;
}

Closing before the scheduling call rather than after, so a failure in scheduleAndSortCostEstimates cannot orphan it either. That is safe because scheduleAndSortCostEstimates only borrows localInput.get(), which is the copy.

[cpwright]

Right on both lines, fixed in 729af9c. And this window is one the previous round opened: adding localInput.setValue(null) to the success terminal callback is exactly what stopped the scheduler's error path from closing these. Before that change the same code double-closed instead, so the two rounds traded one failure mode for the other.

Rather than repeat the guard at four call sites, I gave completeRefilterUpdate an optional pre-work consumer that runs inside the existing ownership guard, and kept a 5-arg overload for the two branches that need no pre-work:

private void completeRefilterUpdate(..., final RowSet newMapping, final RowSet mods,
        @Nullable final Consumer<WritableRowSet> prepareMapping) {
    try (final SafeCloseable ignored = newMapping; final SafeCloseable ignored2 = mods) {
        if (prepareMapping != null) {
            prepareMapping.accept(newMapping.writableCast());
        }
        completeRefilterUpdateInternal(listener, upstream, update, newMapping);
    }
}

Both flagged branches now pass their "add back what we previously matched" block as prepareMapping. The refilterRequestedRowset branch reads that row set inside the pre-work, which is still valid: it is the execution's owned input, and the cleanup-slot release from the previous round runs only after the completion callback returns.

I also applied the same reasoning to two callbacks the review did not flag, so the FilterComplete contract holds at all five call sites instead of three:

• WhereListener.completeUpdate — addFilterResult was unguarded from entry until update.added = addFilterResult, so a throw in currentMapping.extract(...) or shifted().apply(...) leaked it. A plain try-with-resources would have been wrong here, since notifyListeners(update) gives the update away and update owns added by then. Instead the hand-off moved to the top of the method and a finally releases the whole update if it was never handed off, which covers removed and modified too.
• The initial-filter completion — mods was guarded, adds was not, so a throw in toTracking() leaked it.

New test in TestFilterExecutionOwnership covers the exceptional path directly, which is the gap I flagged in the previous thread. It drives InitialFilterExecution.scheduleCompletion with a throwing completion callback and asserts the owned input is released exactly once — detectable without instrumenting the row set, since a second close NPEs and the scheduler escalates that to the fatal error reporter, which throws under test. Confirmed it is a real regression test by restoring the previous release shape and watching only that case fail with FakeFatalException: Unexpected iteration job error.

Local runs: 270 tests across the where/where-in/special-cases/pushdown/QueryTableTest suites plus the new one. The 3 failures are all FileSystemException: Operation not permitted on the system temp dir — a sandbox limitation here, not assertions — so the disk-backed cases (testFilterType, testWhereInGrouped) still need CI.

[Copilot]

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

engine/table/src/test/java/io/deephaven/engine/table/impl/TestFilterExecutionOwnership.java:51

• This helper accepts action, not execution, so the documentation names a nonexistent value.

     * Run {@code execution} with the immediate job scheduler, so that the terminal callbacks and any escalated error
     * happen on this thread.

[Copilot]

🟢 Approval recommended

The ownership transfers and resource lifetimes are consistent with the surrounding APIs, with no unresolved correctness issues found.

Review details

• Files reviewed: 24/24 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[lbooker42]

This looks sketchy (handing contexts to AdapterContext that will get auto-closed) but is correct. A comment might help, something like:

// Successfully transferred ownership to adapter context, empty the the auto-close list

[lbooker42]

Could just use prev() and skip the copy entirely (since not modifying the rowset).

Also, another "Index" sighting when we have adopted "RowSet" in Core (validateIndexesEqual())