[This issue was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]
Why this issue exists
#6690 is a silent wrong-result bug: an axis step with a name test returns the correct result once and an empty sequence on every evaluation after that, within a single query, whenever the context node carries a predicate context item. Root cause and reproduction are in that issue.
There are two reasonable places to fix it, and they embody different judgments about a piece of the engine that predates all of us. I have implemented both, and both are green — so the choice is not "which one works" but "which one do we want to live with". That is a call for people who know this code's history, which is why this is an issue rather than one PR with a preferred option.
The two candidates — A, resetting at the call site, and B, moving the state off the shared proxies — are both open as pull requests, linked in a comment below.
The mechanism, in one paragraph
LocationStep caches its structural-index lookup in currentSet for the whole query execution (resetState() clears it only between executions). NewArrayNodeSet.selectPrecedingSiblings, selectFollowingSiblings, selectPreceding and selectFollowing then stamp those cached NodeProxy objects in place with the matching reference node's context, and a duplicate-suppression guard added in 2019 reads those stamps back to decide whether it has already handled a node. The guard is correct about the current evaluation and blind to the fact that the stamps it is reading may be left over from the previous one.
The guard still does necessary work: removing it fixes #6690 but breaks ten existing tests across axes.xql, npt.xqm and positional-nested.xql. So "delete the guard" is not on the table.
Candidate A — reset the cached set when it is reused
LocationStep clears predicate contexts on currentSet when it reuses a cached set rather than building a fresh one, so the guard only ever sees stamps from the current evaluation.
For: Smallest possible change, confined to two call sites in one method each. Touches nothing in the node-set algorithms or in any hot path. The invariant is easy to state: a cached set enters each evaluation looking the way a freshly built one would.
Against: It leaves the underlying design in place — these methods still mutate objects they share with a cache. And it fixes the symptom one caller at a time: any future code that reuses currentSet and relies on the guard has to remember to call the reset. Most significantly, the previous evaluation's result set holds the same NodeProxy instances, so clearing at the start of evaluation n retroactively mutates the result of evaluation n-1. That is harmless under count() and harmless in every test we have, but it is observable in principle if a consumer holds a result across evaluations.
Candidate B — keep the duplicate-suppression state per call
The four select methods stop reading the stamp back off the NodeProxy and instead record, per call, which nodes they have already stamped and with which context id. Within one call the behavior is identical to today; across calls there is nothing to leak.
For: Fixes the cause rather than the symptom — the guard can no longer see another evaluation's state, whoever the caller is and whatever it does with the cache. Applies uniformly to all four axes. No retroactive mutation of anything. It makes an implicit assumption explicit: this state was always meant to be per-call, and only happened to be stored somewhere that outlives the call.
Against: Touches four methods in a hot path rather than two call sites. Allocates a small per-call map when a context id is in play (lazily bounded by result size, not set size, but it is still an allocation where there was none). And it leaves the proxies shared and mutable — it stops the guard depending on that, but stamping still writes to objects the cache owns, so it narrows the design problem without removing it.
What I would like a decision on
-
Which candidate should land? Both are green, so this is a judgment about maintainability and about how much of the underlying design we want to disturb in a bugfix.
-
Is there a third option that is actually the right one? The honest answer is that both candidates work around the real problem: LocationStep hands out NodeProxy instances that it also caches and that callees mutate. Making these methods copy before stamping would remove that entirely — but it changes what the 2019 guard is protecting against, since that guard relies on shared mutation to see duplicates at all, and I did not want to redesign it blind. If someone remembers what those ten tests are really pinning, that would change the calculus.
-
Does the retroactive-mutation concern in A matter in practice? I could not construct a case where it does, but "I could not construct one" is weaker than "someone who knows the evaluation model says it cannot happen."
Testing
Both candidates are verified identically:
- A new regression test evaluates each affected axis four times over the same node and asserts stability, with the wildcard form, a node reached without a predicate, an unaffected axis, and a non-matching name pinned alongside. It fails on unfixed
develop and passes on both candidates.
xquery.CoreTests — the whole src/test/xquery corpus, including the ten tests that naive guard removal breaks — is green on both.- Full
exist-core suite green on both.
Happy to take either forward, combine them, or throw both away for a better third option.
[This issue was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]
Why this issue exists
#6690 is a silent wrong-result bug: an axis step with a name test returns the correct result once and an empty sequence on every evaluation after that, within a single query, whenever the context node carries a predicate context item. Root cause and reproduction are in that issue.
There are two reasonable places to fix it, and they embody different judgments about a piece of the engine that predates all of us. I have implemented both, and both are green — so the choice is not "which one works" but "which one do we want to live with". That is a call for people who know this code's history, which is why this is an issue rather than one PR with a preferred option.
The two candidates — A, resetting at the call site, and B, moving the state off the shared proxies — are both open as pull requests, linked in a comment below.
The mechanism, in one paragraph
LocationStepcaches its structural-index lookup incurrentSetfor the whole query execution (resetState()clears it only between executions).NewArrayNodeSet.selectPrecedingSiblings,selectFollowingSiblings,selectPrecedingandselectFollowingthen stamp those cachedNodeProxyobjects in place with the matching reference node's context, and a duplicate-suppression guard added in 2019 reads those stamps back to decide whether it has already handled a node. The guard is correct about the current evaluation and blind to the fact that the stamps it is reading may be left over from the previous one.The guard still does necessary work: removing it fixes #6690 but breaks ten existing tests across
axes.xql,npt.xqmandpositional-nested.xql. So "delete the guard" is not on the table.Candidate A — reset the cached set when it is reused
LocationStepclears predicate contexts oncurrentSetwhen it reuses a cached set rather than building a fresh one, so the guard only ever sees stamps from the current evaluation.For: Smallest possible change, confined to two call sites in one method each. Touches nothing in the node-set algorithms or in any hot path. The invariant is easy to state: a cached set enters each evaluation looking the way a freshly built one would.
Against: It leaves the underlying design in place — these methods still mutate objects they share with a cache. And it fixes the symptom one caller at a time: any future code that reuses
currentSetand relies on the guard has to remember to call the reset. Most significantly, the previous evaluation's result set holds the sameNodeProxyinstances, so clearing at the start of evaluation n retroactively mutates the result of evaluation n-1. That is harmless undercount()and harmless in every test we have, but it is observable in principle if a consumer holds a result across evaluations.Candidate B — keep the duplicate-suppression state per call
The four select methods stop reading the stamp back off the
NodeProxyand instead record, per call, which nodes they have already stamped and with which context id. Within one call the behavior is identical to today; across calls there is nothing to leak.For: Fixes the cause rather than the symptom — the guard can no longer see another evaluation's state, whoever the caller is and whatever it does with the cache. Applies uniformly to all four axes. No retroactive mutation of anything. It makes an implicit assumption explicit: this state was always meant to be per-call, and only happened to be stored somewhere that outlives the call.
Against: Touches four methods in a hot path rather than two call sites. Allocates a small per-call map when a context id is in play (lazily bounded by result size, not set size, but it is still an allocation where there was none). And it leaves the proxies shared and mutable — it stops the guard depending on that, but stamping still writes to objects the cache owns, so it narrows the design problem without removing it.
What I would like a decision on
Which candidate should land? Both are green, so this is a judgment about maintainability and about how much of the underlying design we want to disturb in a bugfix.
Is there a third option that is actually the right one? The honest answer is that both candidates work around the real problem:
LocationStephands outNodeProxyinstances that it also caches and that callees mutate. Making these methods copy before stamping would remove that entirely — but it changes what the 2019 guard is protecting against, since that guard relies on shared mutation to see duplicates at all, and I did not want to redesign it blind. If someone remembers what those ten tests are really pinning, that would change the calculus.Does the retroactive-mutation concern in A matter in practice? I could not construct a case where it does, but "I could not construct one" is weaker than "someone who knows the evaluation model says it cannot happen."
Testing
Both candidates are verified identically:
developand passes on both candidates.xquery.CoreTests— the wholesrc/test/xquerycorpus, including the ten tests that naive guard removal breaks — is green on both.exist-coresuite green on both.Happy to take either forward, combine them, or throw both away for a better third option.