fix: release blob file handles on resolve error paths - #810
jackylee-ch wants to merge 3 commits into
Conversation
resolveBatch closed each BlobFile with try-with-resources inside the loop, so the count-mismatch throw abandoned the whole returned list, the null check abandoned everything after the current index, and a read() failure abandoned the tail. BlobFile holds a native handle with no cleaner, so an abandoned one lives until the JVM exits, and Spark retries the task in the same JVM by default. Close the handles once each in a finally instead. resolve() had the same shape for a multi-element return, so it gets the same treatment.
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-approved
Exercise successful reads, count mismatches, null blobs and read failures with real Lance blob v2 datasets. Capture the returned handles through a shared package-private takeBlobs helper and verify their native owners are released before the resolver closes. The old per-item cleanup fails all three batch error regressions. Blob/Join suites pass on Spark 3.5/Scala 2.12 and Spark 4.1/Scala 2.13 (149 tests each, no skips); Checkstyle and Spotless pass.
|
cc @LuciferYang |
lance-gatekeeper
Bot
removed
the
K-approved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-approved
|
Missed this ping, will check it out. |
lance-gatekeeper
Bot
removed
the
K-approved
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The merge from main leaves this pull request’s two-file patch unchanged, so the prior assessment still holds: the real-dataset regressions verify native-handle release after successful reads, count mismatches, null results, and read failures, while the narrowly scoped takeBlobs seam leaves production resolution semantics unchanged.
lance-gatekeeper
Bot
added
the
K-approved
LuciferYang
left a comment
LuciferYang
left a comment
There was a problem hiding this comment.
The resolveBatch fix looks correct: every handle takeBlobs returned is released once on all error paths (empty, count-mismatch, null-blob, read failure), with no double-close, and LargeBinaryWriter reads the copied bytes so closing before return doesn't affect callers. Nothing blocking.
A few non-blocking notes inline. The main one: singleReadFailureReleasesHandle is effectively a smoke test — the single-address resolve change is behavior-neutral (the old try-with-resources also closed that one blob on a read failure), so the test wouldn't catch a resolve regression; the real release semantics are pinned by the resolveBatch tests. The rest are optional: resolve lacks the null-blob guard resolveBatch has, the read-failure tests couple to lance-core's "Not found" text, and the release loop is duplicated across the two methods.
| } | ||
|
|
||
| @Test | ||
| def singleReadFailureReleasesHandle(): Unit = withSource() { (uri, refs) => |
There was a problem hiding this comment.
This test doesn't actually pin the resolve change. resolve requests a single address, so takeBlobs returns one blob, and the old try (BlobFile blob = blobs.get(0)) closed it just as well when read() threw: revert the try/finally to the old form and this test still passes. For a single-address request the old and new code are equivalent, so resolve's change is close to behavior-neutral.
The case that would show the new code's value (takeBlobs returning more handles than requested, where the old code closed only blobs.get(0) and leaked the rest) can't be constructed from a single-address request. So this is really a smoke test for resolve; the release semantics are pinned by the resolveBatch tests. If resolve were broken, this test wouldn't catch it.
| if (blobs.isEmpty()) { | ||
| return new byte[0]; | ||
| } | ||
| return blobs.get(0).read(); |
There was a problem hiding this comment.
resolve calls blobs.get(0).read() with no null check, unlike resolveBatch, which throws a descriptive IOException for a null blob. takeBlobs returns null for a null-descriptor row, so resolve throws a bare NPE where resolveBatch reports column/dataset context. This isn't a leak (the finally's closeQuietly(null) is a no-op) and the behavior is pre-existing; also resolve/resolveIfNeeded have no production caller today, only tests.
Since this PR already rewrote resolve's body, adding a null guard matching resolveBatch would make the two paths report errors consistently. Optional, can be deferred.
| val error = assertThrows( | ||
| classOf[IOException], | ||
| () => resolver.resolveBatch(indices(refs), refs)) | ||
| assertTrue(error.getMessage.contains("Not found"), error.getMessage) |
There was a problem hiding this comment.
The two read-failure tests (this one and line 187 in singleReadFailureReleasesHandle) force read() to fail by deleting the data files, then assert getMessage.contains("Not found"). That leans on two lance-core behaviors: the read must fail after takeBlobs returns (which assumes Lance reads lazily and holds no fd / no mmap at take time), and the error text stays exactly "Not found". A lance-core message change, or a platform where the file is opened at take time, turns these red.
The direction is fail-fast (the test breaks loudly, it won't miss a real leak), so it's not a merge blocker. Asserting only the IOException type rather than the literal text would be more robust.
| for (int vectorIndex : group.indicesByAddress.get(addresses.get(i))) { | ||
| resolved.put(vectorIndex, data); | ||
| } finally { | ||
| for (BlobFile blob : blobs) { |
There was a problem hiding this comment.
The blob-release loop is identical in resolve (line 88) and here in resolveBatch. A future change to release semantics has to touch both, which is easy to miss. Since the PR already extracted a takeBlobs method, extracting a closeAll(List<BlobFile>) helper alongside it would read more cleanly. Pure cleanup, can be deferred.
2 participants
Summary
resolveBatchabandoned everyBlobFiletakeBlobshad returned when the count check or thenull check threw, and the tail of the list when
read()threw.BlobFilewraps a native handlewith no cleaner, and Spark retries the task in the same JVM. Release all of them in a
finally,and do the same in
resolveTest plan
*Blob*/*Join*suites on Spark 4.1/Scala 2.13 and 3.5/Scala 2.12 (144 each);make lint🤖 Generated with Claude Code