fix(zqlite): reject compound-correlation scalar hints that silently misbind - #6431
Draft
Karavil wants to merge 1 commit into
Draft
fix(zqlite): reject compound-correlation scalar hints that silently misbind#6431Karavil wants to merge 1 commit into
Karavil wants to merge 1 commit into
Conversation
|
@Karavil is attempting to deploy a commit to the Rocicorp Team on Vercel. A member of the Team first needs to authorize it. |
`resolveScalarSubquery` replaces a `{scalar: true}` gate with one comparison
built from `correlation.parentField[0]` and `correlation.childField[0]`, but
nothing checks the correlation's arity. A compound relationship therefore keeps
its first pair and silently drops the rest, admitting parent rows the EXISTS
excludes.
The shape is reachable through the public builder: the scalar overload's type
gate checks that the callback covers a unique key of the destination, not the
relationship's arity, so a one-hop `one({sourceField: ['a','b'], destField:
['x','y']})` with a callback pinning the destination's primary key compiles.
With the child row `{x: 'x1', y: 'y1'}` the gate means `a = 'x1' AND b = 'y1'`,
and the rewrite emits only `a = 'x1'`.
The live path compounds it. The companion watches the single stored
`childField`, so editing the second correlated column changes no resolved value,
raises no `ResetPipelinesSignal`, and leaves the wrong parent set in place for
the life of the query.
Refuse the hint whenever either correlation array has more than one column and
report it through the existing ignored-hint channel, so the gate degrades to
the plain EXISTS it should have been. This restores correctness rather than
breaking anything: every affected query returns wrong rows today.
`IgnoredScalarHint` gains a `reason` so the warning can name the real cause —
the existing wording tells the author to pin a unique key, which someone in the
compound case has already done. The `unpinned` wording is unchanged.
z2s needs no change: it emits every correlation pair and ignores the hint.
Karavil
force-pushed
the
capy/scalar-compound-guard
branch
from
August 28, 2026 17:39
89d8002 to
a08b625
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
whereExists(rel, cb, {scalar: true})gate over a compound relationship returns wrong rows on the zqlite / zero-cache read path: parent rows that match only the first column of the correlation are served to the client, and the live-update path can then hold the wrong answer in place indefinitely. The shape is reachable through the public query builder, compiles without complaint, and produces no warning at runtime.Repro
A one-hop compound relationship whose destination has a unique key the callback pins is enough. Nothing here is exotic or hand-written AST:
The static gate on the scalar overload checks that the callback covers some unique key of the destination. It does not check the relationship's arity, so this compiles. The resulting AST carries
correlation: {parentField: ['a', 'b'], childField: ['x', 'y']}together withscalar: true.With the fixture
child{id: 'c1', x: 'x1', y: 'y1'}parentp1 {a: 'x1', b: 'y1'},p2 {a: 'x1', b: 'y2'},p3 {a: 'x9', b: 'y1'}the gate means
a = 'x1' AND b = 'y1', so onlyp1qualifies.At the base commit (
7fb2c78bb) the query returnsp1andp2.resolveSimpleScalarSubquerieshonors the{scalar: true}hint whenever the subquery is provably limited to one row, then replaces the correlated EXISTS with a single comparison built fromcorrelation.parentField[0]andcorrelation.childField[0]. It never checks how many columns the correlation actually has. Here it emitteda = 'x1'and droppedb = 'y1', sop2-- which matches the first correlated column and not the second -- is served to the client. The resolvedwhereis a plainsimplecondition, and no warning is logged, because from the resolver's point of view the hint was honored.The live path makes it worse
The companion pipeline stores a single
childField(childField[0]) and raisesResetPipelinesSignalonly when the value read from that one column changes. Editing the child's second correlated column leaves the first one alone, so nothing invalidates.Updating
child.c1from{x: 'x1', y: 'y1'}to{x: 'x1', y: 'y2'}should move the parent set to['p2']. Measured at the base commit:No reset, no row changes, and the already-wrong set persists -- now wrong in a second way, since
p1no longer qualifies at all. A client can hold rows it was never entitled to for the lifetime of the query.z2sis unaffected: its correlated SQL emits every pair and deliberately ignores the hint, so the same AST is correct when compiled for Postgres. The hazard is specific to zqlite's scalar pre-resolution.The fix
Refuse the hint when either correlation array has more than one column, and report it through the existing ignored-hint channel so the gate degrades to the plain EXISTS it should always have been:
This is correctness-restoring, not a breaking change: every query it affects is returning wrong rows today, and after the fix it returns the rows its own
EXISTSsemantics always specified. The cost is that such a gate no longer gets the single-lookup plan it asked for -- which it was never entitled to.IgnoredScalarHintgains areasonso the warning can say what actually went wrong. The existing message tells the author to constrain every column of a unique key to a literal; someone hitting the compound case has already done exactly that, and would have no way to act on the advice. The two callers that format the warning pick the matching clause; the wording of the pre-existingunpinnedcase is unchanged, including its inline snapshot.Follow-up upstream may prefer
Full compound support rather than a refusal: emit one comparison per correlation pair (
a = row.x AND b = row.y), and widen the companion contract to watch everychildFieldso any of them changing invalidates. That is a larger change --CompanionSubquery.childFieldbecomes a list,ScalarExecutorreturns a tuple, andscalarValuesEqualcompares vectors -- and it changes the shape of data the driver keeps per query. It should be faster wherever compound-correlation gates are hot (unmeasured -- no such gate exists in the suites today) and is strictly larger in risk, so it seemed wrong to bundle with a bug fix that should land quickly. The guard and the full implementation are compatible: the guard becomes dead code the day the vector rewrite lands.Related: #6434 widens scalar-hint eligibility on the same resolver (parent-literal propagation). The two branches are independent, and #6434 deliberately keeps compound correlations ineligible for the same reason this branch refuses them.
Tests
packages/zqlite/src/resolve-scalar-subqueries.test.tsadds five cases: the repro above degrades even thoughisSimpleSubqueryaccepts its subquery (the test asserts that acceptance explicitly, since it is what made the shape dangerous), the same forNOT EXISTS, the same for a compound gate nested inside another subquery, a single-pair correlation still resolves, and an unpinned subquery is still reported asunpinnedrather than as compound.packages/zero-cache/src/services/view-syncer/pipeline-driver.compound-scalar.test.tsruns the repro on a real replica: hydration admitsp1only, the gate survives as a realcorrelatedSubquerywith the warning logged, editing the second correlated column streamsremove p1/add p2, and the compound gate agrees with the same query written without the hint. Copied onto the base commit, all four fail:Validation
The
zql-integration-testschinook zero-cache fuzzer's churn case times out under heavy parallel load on a small machine; it passes at file level (12/12, including the 5,091-case replica/PostgreSQL equivalence run) and in a full run with nothing else competing. That suite's scalar property -- marking gatesscalarmust not change results -- is exactly what this fix restores.