[bugfix] Sequence.containsReference: recurse into nested map/array items

[joewiz]

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

Summary

A file-backed binary value (BinaryValueFromFile, e.g. from request:get-uploaded-file-data or file:read-binary) that is nested inside a map (or array) held by a sequence could have its file channel closed while it was still referenced — after which any read failed with "Underlying channel has been closed". The most visible symptom: a multipart upload stored through a routing framework (Roaster / existdb-openapi POST /api/db/.../install) failed at xmldb:store with:

error while obtaining length of binary value <filename>

Root cause

The general-purpose value sequences checked only direct reference equality and did not recurse into items that are themselves containers — unlike MapType.containsReference and ArrayType.containsReference, which both do value == item || value.containsReference(item):

// e.g. ValueSequence (before)
public boolean containsReference(final Item item) {
    for (int i = 0; i <= size; i++) {
        if (values[i] == item) {       // direct items only
            return true;
        }
    }
    return false;
}

containsReference is the guard eXist uses to avoid destroying a value that is part of a result. When a let/function scope pops, XQueryContext.popLocalVariables → VariableImpl.destroy → <sequence>.destroy → <value>.destroy(context, contextSequence), and BinaryValueFromFile.destroy only skips closing when contextSequence.containsReference(this). For a binary returned nested in a map inside a sequence, that check returned false, so the channel was closed even though the value was still reachable through the returned map.

Five general-purpose sequence types had this gap: ValueSequence, ArrayListValueSequence, OrderedValueSequence, PreorderedValueSequence, and SubSequence — any of which can be the contextSequence while holding a map/array. (MapType/ArrayType already recurse correctly; RangeSequence holds only integers and every NodeSet holds only nodes, so none of those can nest a map/array.)

This is precisely the path the Roaster / existdb-openapi multipart upload tripped: request:get-uploaded-file-data is carried as $request?body?file?data (Roaster's form-data binary shape, map { name, data, size } produced by for ... return map { "data": $data[$index] }), and the deferred handler's xmldb:store then read a value whose channel had already been closed by the enclosing function's variable cleanup.

Fix

Make each affected sequence's containsReference recurse into container items, mirroring MapType/ArrayType:

if (i == item || (i instanceof Sequence seq && seq.containsReference(item))) {

How it was found

Reproduced end-to-end against the real Roaster body:parse deployed into a local eXist, instrumenting BinaryValueFromFile to capture the close-site. The close fired from popLocalVariables → destroy with containsReference returning false on a ValueSequence of maps; the fix flips it to true and the upload stores cleanly. Reduced to a pure-XQuery / direct unit-test case, then a containsReference audit of every Sequence implementation found the four sibling types with the identical gap.

What changed

• exist-core/.../xquery/value/{ValueSequence, ArrayListValueSequence, OrderedValueSequence, PreorderedValueSequence, SubSequence}.java — containsReference recurses into nested container items.

Test plan

• ContainsReferenceNestedTest (unit): asserts ValueSequence, ArrayListValueSequence and SubSequence detect an item nested in a map (and don't report an unrelated item). Each fails without the fix. OrderedValueSequence/PreorderedValueSequence take the identical one-line change (their OrderSpec construction is not exercised directly).
• FileBinaryNestedInMapTest (file module): a file:read-binary value returned nested in a map inside a sequence from a function, then read. Fails with "Underlying channel has been closed" before the fix, passes after.
• Verified end-to-end against the real Roaster multipart route (stores byte-identical, no error).
• Codacy/PMD clean on the changed methods (pre-existing findings elsewhere in those files are untouched).

Notes

• Sibling of [bugfix] BinaryValueFromFile: honor shared-reference reference counting #6506 (BinaryValueFromFile shared-reference refcounting, the enclosed-expression premature-close path). Both are "file-backed binary closed while still referenced", but distinct root causes in different files; this one is the multipart/xmldb:store symptom. Neither fixes the other; both are needed.

[line-o]

Can this be rewritten to

final ValueSequence vs = withMapNesting() (map, nested, notNested) -> {
    final ValueSequence vs = new ValueSequence();
    vs.add(map);
    return vs;
});

assertTrue(vs.containsReference(nested));
assertFalse(vs.containsReference(notNested));

[line-o]

Or a custom assertion maybe?

[line-o]

I have to revise my earlier review, the tests use some "clever" construct for testing that is hard to follow.
Preferrably, there is a simpler more straightforward solution. This can be more verbose and duplicate more code as long as it is easy to follow.

[line-o]

I would like to see Codacy to like the new junit tests. But this is not a blocker for me.

[joewiz]

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

Thanks @line-o. Both addressed in be7e255:

• Simpler test. Dropped the withMapNesting(...) functional-interface helper and inlined the setup in each test method. It's a bit more verbose and duplicates the map { "data": $nested } construction three times, but each test now reads top to bottom with no indirection.
• Codacy. PMD is clean on the rewritten ContainsReferenceNestedTest. All three tests still pass (Tests run: 3, Failures: 0).

[line-o]

Lines 64 and 65 should be in a setup function.

[joewiz]

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

Done in 9a11664 — fixture creation now lives in a %test:setUp function, symmetric with the %test:tearDown. One wrinkle worth noting: helper:get-test-directory embeds a fresh util:uuid() on every call, so computing the directory independently in setUp and in the test would put the fixture and the read in different directories. I compute it once in a module variable ($bnm:directory) that both share. FileTests stays green.