Query layer: reject the inputs that used to return a confident wrong answer - #89
Merged
Conversation
pequalsnp
force-pushed
the
fix/query-input-validation
branch
3 times, most recently
from
August 28, 2026 15:10
520e77c to
42846c3
Compare
Eight defects on the read path, all of which surfaced as plausible-looking
data rather than as errors.
Get/GetMany on a windowed pipeline could never return anything: they read
bucket 0, which is simultaneously the all-time sentinel and the epoch
bucket, so they reported "absent" no matter how much the pipeline had
counted. Four shipped runbooks pointed operators at them; the soak runbook
author was caught by it, and the integration test that "verified" the
absent-entity shape would have passed for an entity with a million views.
Now FAILED_PRECONDITION naming GetWindow, which fresh_read does not bypass.
Degenerate time bounds were answered with a fabricated Present:true zero:
start after end, bounds left at the proto3 zero, a negative duration, and
the two instants whose UnixNano wraps — end_unix=253402300799 and the zero
time.Time that pkg/query/typed puts on the wire as -62135596800, both of
which landed on arbitrary negative buckets. All INVALID_ARGUMENT now,
mirroring the check pkg/admin already had.
Retention was advisory on the read path. Asking for 90 days against a 7-day
retention read 83 TTL-evicted buckets, folded the holes in as the monoid
identity, and labelled the result a full 90-day window.
Nothing bounded the bucket fan-out. windowed.Minute(24h) with
GetRange(entity, Unix(0,0), now) built 29,797,201 keys — about 715MB of
state.Key — of which all but 1,440 addressed buckets TTL had already
evicted. windowed.Config gains MaxBuckets, defaulting to
ceil(Retention/Granularity), enforced on every path that fans a read over
buckets.
The singleflight coalesce key was ambiguous. Entities were joined with '|'
after sorting, so ["a|b"] shared a group with ["a","b"] and ["a|b","c"]
with ["a","b|c"] — a length check catches neither — and the shipped codegen
key_template puts a literal '|' inside entity keys. Sorting also merged
["a","b"] with ["b","a"], and since responses are positional the second
caller got the first caller's values against the wrong entities. The key is
now length-prefixed and order-preserving; permutation coalescing is
deliberately given up.
The coalesced work also ran on the context of whichever caller led the
group, so one client hanging up failed every peer with CodeInternal — under
exactly the load coalescing exists to serve. It now runs detached under a
server-side CoalesceTimeout, with each waiter selecting on its own context;
a plain WithoutCancel would have dropped the client deadline and let a
wedged store pin the group.
Duplicate keys reached BatchGetItem verbatim, which DynamoDB rejects
outright ("Provided list of item keys contains duplicates"), failing the
whole RPC — and only when the two copies happened to land in the same
100-key chunk, so it read as a candidate-set-size flake. Both DDB stores
now chunk a de-duplicated key set and scatter results back through the
(entity, bucket) map they already keep.
Finally, typed.SumClient sized its output slice from the entity list but
wrote out[i] for every value the SERVER returned: a remotely triggerable
index-out-of-range panic inside the calling application. The sketch clients'
`if i >= len(out) { break }` turned the same bug into silent truncation.
All batched clients now reject a value/entity count mismatch, since the
wire contract is positional and a mismatch means the values cannot be
attributed at all.
BREAKING (pre-1.0): Get/GetMany now fail on windowed pipelines; malformed
windows and ranges now error instead of returning zero; pkg/query's window
helpers return errors matching query.ErrInvalidQuery; typed clients error
on a value-count mismatch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
The bucket-span cap was one short. BucketRange is inclusive at both ends, so an absolute [t, t+Retention] touches Retention/Granularity + 1 buckets, but MaxBucketSpan derived ceil(Retention/Granularity). GetRange and GetRangeMany over exactly the retention window — the most natural range a caller writes — came back InvalidArgument. The default is now ceil(Retention/Granularity) + 1; an explicit MaxBuckets is still used verbatim, since that is a count the operator wrote down. Four more holes around the same code: - A Config with a Granularity but neither MaxBuckets nor Retention got MaxBucketSpan()==0, i.e. no cap at all, so GetRange(epoch, now) at minute granularity still built ~30 million keys. It now falls back to DefaultMaxBucketSpan (100,000) — a backstop against unbounded, far above any deliberate query. - checkRetention never ran on the absolute-range path, so raising MaxBuckets above Retention bought reads straight past TTL: a year against a 7-day Retention fanned out over 366 buckets, found 359 evicted, folded the holes in as Identity and labelled the week's total as a year. Ranges are now held to Retention too. The bound is on a range's WIDTH, not its age, and checkRetention says why: rangeBuckets has no `now`, and LambdaQuery.GetRange reads a batch view that outlives the streaming table's TTL by design. - Coalesced work dropped the caller's deadline along with its cancellation. WithoutCancel plus a flat CoalesceTimeout meant a burst of abandoned requests each kept up to 10s of store fan-out alive with nobody left to read it, where before coalescing a hangup shed that work at once. The shared context now derives its deadline from the leader's, capped by CoalesceTimeout. - TestQueryServer_CoalescedPeerSurvivesCallerCancellation was racing a sleep and failed only 16 runs in 20 against the unfixed server. It is now gated end to end: the leader is provably inside the store call, the peer is provably a waiter on its group (via a context that signals when coalesce evaluates Done(), which only happens after DoChan has registered it), and blockingStore resolves cancel-vs-release by checking ctx.Err() instead of letting select flip a coin. 20/20 pre-fix failures now. TestGetRange_RejectsBucketSpanBeyondCap and its Many counterpart were retargeted onto a config where MaxBuckets, not Retention, is the binding limit — otherwise the new retention check refuses those ranges a step earlier and neither test would reach the span cap it names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
Kept out of the implementing commits so parallel branches did not all conflict on the [Unreleased] heading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
The integration test expected HTTP 412. Connect maps CodeFailedPrecondition to 400 — verified in connectCodeToHTTP in connectrpc.com/connect v1.20.0. The server behaviour was correct; only the test's expectation was wrong. The Connect error code assertion just below is the real check; the status is asserted only so a plain 200 with a zero value cannot pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
#88 rewrote fakeTransport.RoundTrip on main and dropped the duplicate check this branch had added. Without it the fake accepts what real DynamoDB rejects, so a store that forwards duplicate keys passes the test and fails only in production — which is the whole failure this test exists to catch. Verified by mutation: replacing `uniq := dedupeKeys(ks)` with `uniq := ks` now fails both dedup tests with the real API error text, "Provided list of item keys contains duplicates (t/a/0)". Before this commit the mutation went unnoticed. Also reconciles the merged test file with main's newFakeClient signature, which gained a maxAttempts argument in #88. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
pequalsnp
force-pushed
the
fix/query-input-validation
branch
from
August 28, 2026 15:17
42846c3 to
82e59b1
Compare
This was referenced Aug 28, 2026
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
1 participant
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.
Eight verified defects from the edge-case hunt. Went through one round of adversarial review that returned needs-work; the blocking off-by-one and four concerns are fixed and re-verified.
The read side was answering questions it could not answer
GetandGetManyon a windowed pipeline could never return data. Bucket 0 is both the all-time sentinel and the epoch bucket, so a windowed row is never at bucket 0. Four shipped docs point operators at these RPCs — and it already fooled the author of the soak runbook, whose smoke test usedGetand would have reported an empty result against a perfectly healthy stack. NowCodeFailedPreconditionnamingGetWindow.test/integration/page_view_counters_test.goassertedpresent==falseon a Daily-windowed server and was therefore vacuous; fixed too.Degenerate time bounds returned a fabricated
Present:truezero.start>end, both bounds unset (proto3 zero), negative duration,end=253402300799(what JSnew Date('9999-12-31')sends), andstart=time.Time{}— whichtyped.go:185sends as Unix-62135596800, wrappingUnixNanooutside[1677, 2262]onto a negative bucket. All nowCodeInvalidArgument, mirroringpkg/admin/server.go:255which already validated this on the control plane.Retention was advisory. A query longer than
Retentionfolded TTL-evicted buckets in asIdentityand labelled the result a full window withPresent:true— a week's total returned as a year's.Unbounded fan-out
GetRangehad no cap. Withwindowed.Minute(24h),GetRange(entity, epoch, now)computed 29,797,201 keys (~715 MB) before the first store call, of which all but 1,440 were past retention and could never hold data.Capped via
MaxBucketSpan, enforced at all four call sites. The review caught an off-by-one in the first attempt: bucket ranges are inclusive at both ends, so a range of exactlyRetentiontouchesRetention/Granularity + 1buckets and was being rejected — the most natural query a user writes. Fixed, with a property test over unaligned start offsets.It also caught that the cap only applied when
Retention > 0, leavingConfig{Granularity: time.Minute}uncapped; there's now aDefaultMaxBucketSpanbackstop.Concurrent callers could get each other's values
The singleflight coalesce key was
sortedJoin, which is ambiguous:["a|b"]vs["a","b"], and["a|b","c"]vs["a","b|c"]— same length, different entities. Reachable, because the shipped codegenkey_templateputs a literal|in entity keys. Two concurrent callers with distinct lists shared one result and got per-entity values transposed.Now a count-prefixed, per-entry length-prefixed, order-preserving encoding. Permutation coalescing is given up deliberately.
Separately, coalesced closures captured the first caller's context, so one client disconnect failed every coalesced peer with
CodeInternal. Nowsf.DoChanwith each waiter selecting on its ownctx.Done(). The review caught that the first fix usedWithoutCanceland dropped the caller's deadline along with its cancellation — so a burst of abandoned requests kept store work alive. Now the shared deadline is derived from the leader's, capped byCoalesceTimeout.A remotely triggerable panic in the caller
typed.SumClienthadif i < len(values)where it neededif i < len(out)— a server returning more values than entities writes past the output slice, panicking the calling application. Same bug at:133and inexamples/search-rerank.Note the sketch clients' existing
if i >= len(out) { break }is not the fix — it converts the panic into silent truncation. All eight batched loops now error on a count mismatch.Duplicate entities broke the whole request
Duplicate keys reached
BatchGetItemverbatim, which DynamoDB rejects (Provided list of item keys contains duplicates), failing the entire RPC. The shipped rerank example swallowed it into an all-zero feature vector. The failure is chunk-boundary dependent over 100-key chunks, so it presented as a request-shape-specific outage.Test plan
pkg/query,pkg/query/grpc,pkg/query/typed,pkg/state/dynamodb<-releaseagainst<-ctx.Done()in oneselect, so Go picked at random. Now deterministically gated, 20/20make test-unit,golangci-lint0 issuesBreaking (pre-1.0):
Get/GetManynow reject windowed pipelines; degenerate ranges now error instead of returning zero;MaxBucketSpan()counts both ends.Known scope limit, documented not fixed: the retention bound is on a range's width, not its age —
GetRange(now-90d, now-83d)against a 7-day retention is 7 days wide and still passes. Closing it needs anowthreaded through, which would breakLambdaQuery.GetRangesemantically, since its View store outlives the streaming TTL by design.🤖 Generated with Claude Code
https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S