[bugfix] Fix premature closing of still-referenced binary values

[joewiz]

[This PR was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

This PR now fixes two distinct premature-close bugs in the binary-value lifecycle. Per @duncdrum's request in #6552 (comment), the fix for #6552 has been folded into this PR as a second commit, so this one PR closes that issue.

Closes #6552

Bug 1 — enclosed expressions close a file-backed binary (commit 1)

A file-backed binary value (BinaryValueFromFile, e.g. from file:read-binary or request:get-uploaded-file-data) that is used in an element or document constructor (an enclosed expression) and then read again could have its file channel closed underneath it, after which the second read failed with "Underlying channel has been closed":

let $b := file:read-binary($path)
let $w := <a>{$b}</a>
return (count($w), $b)[2]   (: count($w) forces the constructor; -> "Underlying channel has been closed" without the fix :)

Root cause

BinaryValueFromFile did not honor the shared-reference reference counting that XQueryContext.enterEnclosedExpr() / exitEnclosedExpr() rely on:

• incrementSharedReferences() was a no-op ("we don't need reference counting…"), and
• close() released the FileChannel/RandomAccessFile unconditionally.

enterEnclosedExpr() increments the shared-reference count of each live binary value so a value that escapes an enclosed expression survives; the matching exitEnclosedExpr() then close()s each, intending only to decrement a reference. For BinaryValueFromFile the increment did nothing and the close() actually tore down the channel — so a value used in a constructor was closed immediately after the constructor, while it was still referenced. BinaryValueFromInputStream was unaffected because it already reference-counts through AbstractFilterInputStreamCache.

Fix

Give BinaryValueFromFile the same reference counting as its input-stream sibling: a counter starting at 1, incremented by incrementSharedReferences() and decremented by close(), releasing the channel/file handle only when it reaches zero. Eager cleanup of values not shared out of a scope is preserved (their count goes 1 → 0 on cleanup).

Bug 2 — xmldb:store closes the caller's binary value (commit 2; #6552)

The regression reported in #6552: since #6467, storing a binary value with xmldb:store and then reading it again in the same query fails. Root-cause analysis is in #6552 (comment).

let $file := request:get-uploaded-file-data("file")
return (xmldb:store($collection, $filename, $file), util:binary-to-string($file))[2]
(: -> "Underlying channel has been closed" without the fix :)

Root cause

XMLDBStore.evalWithCollection() wraps its Resource in try-with-resources. Since #6467 the xs:base64Binary branch passes the caller's actual BinaryValue to LocalBinaryResource.setContent() (before #6467 it was copied to a byte[]), and on try-exit AbstractEXistResource.close() → LocalBinaryResource.doClose() calls binaryValue.close() — closing the caller's value, which the query may still need. This breaks file-backed and database-backed (util:binary-doc) values, and also breaks storing the same value twice.

Fix

XMLDBStore now takes a shared reference on the value's behalf (incrementSharedReferences()) before lending it to the resource, so the resource's close() releases only that reference and the value remains readable by the caller. The reference is taken by the lender rather than inside LocalBinaryResource.setContent() because setContent has two kinds of callers with opposite ownership semantics: a query-result resource owns its value — closing the resource must release it, the contract pinned by FilterInputStreamCacheMonitorTest.binaryResult — while xmldb:store only lends the caller's value. For file-backed values this depends on Bug 1's reference counting — which is why the two fixes belong in one PR: commit 2 without commit 1 would still fail for file-backed values.

Verification matrix from the #6552 investigation (three reproducers, per build):

Build	file-backed reuse	db-backed reuse	stored twice
develop	fail	fail	fail
+ commit 1 only	fail	fail	fail
+ commit 2 only	fail	pass	fail
+ both (this PR)	pass	pass	pass

What changed

• exist-core/.../xquery/value/BinaryValueFromFile.java — add the sharedReferences counter; incrementSharedReferences() increments it; close() decrements and releases the channel only at zero (idempotent once released).
• exist-core/.../xquery/functions/xmldb/XMLDBStore.java — takes a shared reference on the binary value before lending it to the resource, so the resource's close() no longer closes the caller's value.
• exist-core/.../xquery/value/BinaryValueFromFileSharedReferenceTest.java — new unit test for the reference-counting contract.
• extensions/modules/file/.../AbstractBinariesTest.java — four new regression tests (see below), inherited by the embedded, XML:DB (local + remote), and REST harnesses.

Test plan

• BinaryValueFromFileSharedReferenceTest (unit): models enterEnclosedExpr()/exitEnclosedExpr() on a shared file-backed value; it must stay open and readable, then be released by the final close. Fails before the fix, passes after.
• AbstractBinariesTest#readBinaryUsedInElementConstructorThenReadAgain: the Bug 1 query. Fails with "Underlying channel has been closed" before commit 1, passes after.
• AbstractBinariesTest#readBinaryStoreThenReadAgain: Bug 2, file-backed (file:read-binary → xmldb:store → read again).
• AbstractBinariesTest#binaryDocStoreThenReadAgain: Bug 2, database-backed (util:binary-doc → xmldb:store → read again; failed with "The underlying InputStream has been closed").
• AbstractBinariesTest#readBinaryStoredTwice: Bug 2, same value stored twice (failed with "error while obtaining length of binary value" — the failure signature from the CI of eXist-db/public-repo, which currently carries a workaround for this).
• All AbstractBinariesTest tests were run against a build without the XMLDBStore change first: 12 failures with exactly the [BUG] Regression with binary streams closing #6552 error signatures across all harnesses (embedded, XML:DB local + remote, REST); with the change, all 26 pass.
• FilterInputStreamCacheMonitorTest passes (3/3) — in particular binaryResult, which pins the query-result resource-ownership contract that ruled out placing the reference-taking inside LocalBinaryResource.setContent().
• Codacy/PMD clean on the changed files (two pre-existing findings in XMLDBStore.java — method NPath complexity and a parameter reassignment in an untouched method — left as-is to keep the diff minimal).

The regression tests run main-module queries (not XQSuite test functions) on purpose: inside a module function Bug 1 is masked, because ModuleContext.registerBinaryValueInstance() delegates registration to the parent/root context, while enterEnclosedExpr()/exitEnclosedExpr() operate on the ModuleContext's own (empty) deque — so the constructor's exitEnclosedExpr() never sees the binary there and the close happens later via popLocalVariables (after the read), harmlessly. The bug surfaces only where the binary and the enclosed expression share a context, i.e., in a main-module query. (That asymmetry in ModuleContext is pre-existing and merely masks this bug; this PR does not change it.)

Notes

• This PR was originally described as fixing a multipart-upload bug. Upon further research, that claim was misplaced. The multipart xmldb:store failure actually has a distinct root cause (Sequence.containsReference not recursing into nested map/array items, across five sequence types), now fixed separately in [bugfix] Sequence.containsReference: recurse into nested map/array items #6507. This PR fixes two sibling "binary value closed while still referenced" bugs: the enclosed-expression path (Bug 1) and the xmldb:store resource-close path (Bug 2, [BUG] Regression with binary streams closing #6552).
• Rebased onto current develop per @duncdrum's request (the single original commit was reapplied unchanged).

[duncdrum]

@joewiz can you rebase this please. Should clear out ci flake

[line-o]

LGTM, but I assume we are waiting for the follow up.

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Done on both counts:

• Rebased onto current develop per @duncdrum's request. The original commit was reapplied unchanged.
• Folded in the fix for [BUG] Regression with binary streams closing #6552 as a second commit, per Duncan's request in [BUG] Regression with binary streams closing #6552 (comment) — so this PR now closes that issue (and @line-o, this is the follow-up you were waiting for). It is the one-line borrow described in the root-cause analysis: LocalBinaryResource.setContent(BinaryValue) takes a shared reference so that xmldb:store's try-with-resources close releases only the resource's reference instead of closing the caller's value.

The second commit adds three regression tests to AbstractBinariesTest, inherited by the embedded, XML:DB (local + remote), and REST harnesses: file-backed reuse after xmldb:store (Duncan's case from #6552), database-backed (util:binary-doc) reuse, and storing the same value twice (the pattern eXist-db/public-repo currently works around). Verified locally that the tests bite: against a build without the LocalBinaryResource change they produce 12 failures with exactly the #6552 error signatures; with it, all 26 tests in the three harnesses pass.

I've updated the PR description to cover both bugs and how they interlock (the xmldb:store fix depends on commit 1's reference counting for file-backed values, which is why one PR carries both).

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

CI caught a real problem with my first placement of the #6552 fix, now corrected in f4bdf72 (the second commit was amended in place).

FilterInputStreamCacheMonitorTest.binaryResult failed because it pins an ownership contract I had violated: a query-result LocalBinaryResource owns its binary value, and closing the resource must release it. Taking the shared reference inside LocalBinaryResource.setContent() broke that path — the resource's close no longer released the value, and the monitor saw a leaked active binary. setContent(BinaryValue) turns out to have two kinds of callers with opposite ownership semantics: query-result wrapping transfers ownership, while xmldb:store only lends the caller's value.

So the reference is now taken by the lender: XMLDBStore calls incrementSharedReferences() on the value before handing it to the resource, and the resource's close consumes that reference instead of the caller's. LocalBinaryResource is untouched. Locally green: FilterInputStreamCacheMonitorTest 3/3, the three new #6552 regression tests across all harnesses (26/26 in the binaries suites), and BinaryValueFromFileSharedReferenceTest 2/2. The PR description has been updated to match.

[line-o]

LGTM
Nitpick for the future: Verbose commentary explaining the concept of reference counting. A reference to an existing explanation outside the codebase might be all that is needed.

[joewiz]

@line-o Thanks! Based on this and your reviews today in existdb-openapi, Claude wrote:

Convention feedback — saved ✅

Juri's verbosity note recurs in all three reviews (#6506 reference-counting essay, eXist-db/existdb-openapi#61's mapping text repeated in 3 places, eXist-db/existdb-openapi#59). I saved it to memory as feedback_concise-comments-no-duplication: keep one concise canonical explanation, reference (don't restate) it, prefer linking an external concept doc over an in-source essay, never duplicate the same explanation across api.json/Java-doc/inline.