From 34d5a0ff0ca9953b621b68235cf5e7063d63741e Mon Sep 17 00:00:00 2001 From: Kyle Galloway Date: Fri, 28 Aug 2026 11:01:51 -0300 Subject: [PATCH 1/5] Stop the query layer answering broken requests with a confident zero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S --- STABILITY.md | 6 +- doc/design.md | 24 +- examples/page-view-counters/README.md | 9 +- examples/page-view-counters/cmd/query/main.go | 16 +- examples/recently-interacted-topk/README.md | 14 +- .../cmd/query/main.go | 13 +- .../terraform/README.md | 9 +- examples/search-rerank/rerank.go | 17 +- pkg/monoid/windowed/windowed.go | 39 ++ pkg/query/grpc/server.go | 369 +++++++++++----- pkg/query/grpc/validation_test.go | 407 ++++++++++++++++++ pkg/query/lambda.go | 10 +- pkg/query/typed/typed.go | 64 ++- pkg/query/typed/valuecount_test.go | 125 ++++++ pkg/query/validate.go | 135 ++++++ pkg/query/validate_test.go | 218 ++++++++++ pkg/query/warmup.go | 30 +- pkg/query/window.go | 43 +- pkg/state/dynamodb/bytesstore.go | 13 +- pkg/state/dynamodb/store.go | 34 +- pkg/state/dynamodb/store_test.go | 153 +++++++ .../v1/murmurv1connect/query.connect.go | 34 +- proto/gen/murmur/v1/query_grpc.pb.go | 34 +- proto/murmur/v1/query.proto | 17 +- test/integration/page_view_counters_test.go | 64 ++- 25 files changed, 1673 insertions(+), 224 deletions(-) create mode 100644 pkg/query/grpc/validation_test.go create mode 100644 pkg/query/typed/valuecount_test.go create mode 100644 pkg/query/validate.go create mode 100644 pkg/query/validate_test.go diff --git a/STABILITY.md b/STABILITY.md index 060b392..53464f3 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -34,9 +34,9 @@ edges callers should plan around. | `pkg/exec/lambda/kinesis` | experimental | `NewHandler` returns the Lambda Kinesis handler signature; partial-batch failures via BatchItemFailures; pair with `WithDedup` so adjacent-redelivered records fold idempotently | | `pkg/exec/lambda/dynamodbstreams` | experimental | DDB Streams Lambda handler; same retry/dedup/BatchItemFailures shape as the Kinesis variant. Decoder takes the whole change record so callers can branch on EventName / inspect OldImage. BatchItemFailures report the record's SequenceNumber (what Lambda checkpoints on); the eventID feeds dedup only. A failed record with an empty SequenceNumber gets no entry at all (an empty ItemIdentifier makes Lambda redeliver the whole batch) and is surfaced via metrics.RecordError plus a `:unreportable_failure` event | | `pkg/exec/lambda/sqs` | experimental | SQS Lambda handler; same shape as kinesis/dynamodbstreams. Default EventID is "/"; override via WithEventID for FIFO content-dedup or upstream-key dedup. Uses SQS SentTimestamp for windowed-bucket assignment so delayed deliveries land in the correct bucket | -| `pkg/query` | mostly stable | `Get` / `GetWindow` / `GetRange` / `LambdaQuery` are likely v1 surface | -| `pkg/query/grpc` | mostly stable | generic byte-encoded responses; `cmd/murmur-codegen-typed` emits per-service typed `.proto` + Go server stubs (sum / hll / topk / bloom; get_all_time / get_window / get_window_many / get_many / get_range) over `pkg/query/typed` clients. `HealthHandler` serves `grpc.health.v1.Health` and `HealthzHandler` serves `/healthz` (liveness, always 200) + `/readyz` (readiness, store round-trip, cached so probe traffic is not billed reads) | -| `pkg/query/typed` | mostly stable | typed-client wrappers over the generic QueryService — `SumClient`, `HLLClient`, `TopKClient`, `BloomClient`. All four expose `Get` / `GetMany` / `GetWindow` / `GetWindowMany` / `GetRange`. `GetMany` returns parallel value + present arrays so callers can distinguish "absent" from "present-and-empty"; `GetWindowMany` can't (the generic RPC merges before returning). The decoders + typed shape behind application-service typed-wrapper RPCs (see `examples/typed-wrapper`). Building block under `cmd/murmur-codegen-typed` | +| `pkg/query` | mostly stable | `Get` / `GetWindow` / `GetRange` / `LambdaQuery` are likely v1 surface. Windowed reads validate the request before fanning out: non-positive durations, swapped / unset / unrepresentable range bounds, windows longer than `Retention`, and bucket spans past `windowed.Config.MaxBuckets` all return an error matching `query.ErrInvalidQuery` instead of a monoid Identity that looks like a real zero | +| `pkg/query/grpc` | mostly stable | `Get` / `GetMany` return `FAILED_PRECONDITION` on a windowed pipeline (they read bucket 0, which windowed writes never populate) — route to `GetWindow` / `GetWindowMany`; `fresh_read` does not bypass it. Malformed windows and ranges return `INVALID_ARGUMENT`. Singleflight coalescing keys on a length-prefixed, order-preserving entity encoding and runs the shared call under `Config.CoalesceTimeout`, detached from the leading caller's context. generic byte-encoded responses; `cmd/murmur-codegen-typed` emits per-service typed `.proto` + Go server stubs (sum / hll / topk / bloom; get_all_time / get_window / get_window_many / get_many / get_range) over `pkg/query/typed` clients. `HealthHandler` serves `grpc.health.v1.Health` and `HealthzHandler` serves `/healthz` (liveness, always 200) + `/readyz` (readiness, store round-trip, cached so probe traffic is not billed reads) | +| `pkg/query/typed` | mostly stable | typed-client wrappers over the generic QueryService — `SumClient`, `HLLClient`, `TopKClient`, `BloomClient`. All four expose `Get` / `GetMany` / `GetWindow` / `GetWindowMany` / `GetRange`. `GetMany` returns parallel value + present arrays so callers can distinguish "absent" from "present-and-empty"; `GetWindowMany` can't (the generic RPC merges before returning). Every batched client errors when the response's value count doesn't match the requested entity count — the wire contract is positional, and the old handling either panicked in the caller or silently truncated. The decoders + typed shape behind application-service typed-wrapper RPCs (see `examples/typed-wrapper`). Building block under `cmd/murmur-codegen-typed` | | `pkg/admin` | mostly stable | CORS is closed by default; opt in via `WithAllowedOrigins`. Bearer-token (`WithAuthToken`, constant-time, multi-token for rotation) and JWT (`WithJWTVerifier`, BYO verifier) auth via a single middleware; auth is off by default — same-origin / network-isolated deploys keep the historical behavior. The `cmd/murmur-ui` binary exposes `--auth-token` + `MURMUR_ADMIN_TOKEN` env fallback | | `pkg/swap` | mostly stable | small surface; integrated into `deploy/terraform/modules/pipeline-counter` via opt-in `swap_enabled` (control table + IAM + seed + `SWAP_CONTROL_TABLE` / `SWAP_ALIAS` env vars in every task definition) | | `pkg/metrics/emf` | experimental | CloudWatch Embedded Metric Format `Recorder`. Aggregates in memory and emits one document per flush interval as counters plus EMF StatisticSets, so the hot path stays cheap and CloudWatch Logs ingestion does not scale with throughput. Needs no CloudWatch API permission or SDK client — Lambda and the ECS awslogs driver already ship stdout to Logs. Splits `pipeline:sub_event` names so `dedup_skip` / `dedup_release` become their own metrics rather than fragmenting the Pipeline dimension. Lambda callers should `Flush()` per invocation; the environment freezes on return and a background ticker may never fire | diff --git a/doc/design.md b/doc/design.md index c6597f1..f51be8e 100644 --- a/doc/design.md +++ b/doc/design.md @@ -1953,11 +1953,25 @@ flowchart LR ### 8.3 Singleflight coalescing -`pkg/query/grpc.Server` wraps the four read methods in -`golang.org/x/sync/singleflight`. The keying is the request shape: for -`Get` it's `"get:" + entity`; for `GetWindow` it's `"window:" + entity -+ ":" + duration`; for `GetRange` it's `"range:" + entity + ":" + -start + ":" + end`. +`pkg/query/grpc.Server` wraps the read methods in +`golang.org/x/sync/singleflight`. The key is the request shape: the RPC +name, the duration or absolute bounds, the bucketed "now" for windowed +reads, and the entity list. + +Two properties of the entity-list encoding are load-bearing. It is +**length-prefixed**, because a plain separator-joined key is not +injective over entity strings that may contain the separator — `["a|b"]` +and `["a","b"]` produced the same key, and so did `["a|b","c"]` and +`["a","b|c"]`, which a length check does not catch either. And it is +**order-preserving**, because responses are positional: sorting the list +to catch permutations made `["a","b"]` and `["b","a"]` one group, so the +second caller received the first caller's values attributed to the wrong +entities. Permutation coalescing is deliberately given up. + +The shared call runs on a context detached from whichever caller led the +group, under a server-side `CoalesceTimeout`, and each waiter selects on +its own context. A client hanging up therefore leaves without taking its +coalesced peers down with it, and a wedged store still frees the group. Concurrent requests for the same key resolve through one underlying fold. A thousand simultaneous feed renders asking for the same hot diff --git a/examples/page-view-counters/README.md b/examples/page-view-counters/README.md index 0a06307..5536836 100644 --- a/examples/page-view-counters/README.md +++ b/examples/page-view-counters/README.md @@ -57,9 +57,16 @@ Produce some events (using your tool of choice — `kcat`, a small Go producer, then query: ```sh -grpcurl -plaintext -d '{"entity": "page-A"}' localhost:50051 murmur.v1.QueryService/Get +# This pipeline is windowed (daily buckets, 90d retention), so GetWindow is +# the RPC to use. Get / GetMany read the all-time row at bucket 0, which a +# windowed pipeline never writes — the server answers them with +# FAILED_PRECONDITION rather than a misleading "not found". grpcurl -plaintext -d '{"entity": "page-A", "duration_seconds": 86400}' \ localhost:50051 murmur.v1.QueryService/GetWindow + +# Last 7 days: +grpcurl -plaintext -d '{"entity": "page-A", "duration_seconds": 604800}' \ + localhost:50051 murmur.v1.QueryService/GetWindow ``` ## Production deployment diff --git a/examples/page-view-counters/cmd/query/main.go b/examples/page-view-counters/cmd/query/main.go index df33b02..f771561 100644 --- a/examples/page-view-counters/cmd/query/main.go +++ b/examples/page-view-counters/cmd/query/main.go @@ -1,9 +1,14 @@ // Connect-RPC query server for the page-view-counters example. // -// Serves Get / GetMany / GetWindow / GetRange against the same DynamoDB table +// Serves GetWindow / GetWindowMany / GetRange against the same DynamoDB table // the streaming worker writes to. The endpoint speaks gRPC, gRPC-Web, and // Connect (HTTP+JSON) on the same port — pick whichever your client supports. // +// This pipeline is windowed (daily buckets), so GetWindow is the entry point. +// Get / GetMany address the all-time row at bucket 0, which a windowed +// pipeline never writes; the server rejects them with FAILED_PRECONDITION +// instead of reporting a phantom "not found". +// // In production this binary runs as a separate ECS Fargate service behind an // ALB (the Terraform pipeline-counter module's `query` service). Locally: // @@ -12,10 +17,11 @@ // // Then call it with grpcurl, buf curl, or plain curl: // -// grpcurl -plaintext -d '{"entity": "page-A"}' \ -// localhost:50051 murmur.v1.QueryService/Get -// curl -X POST http://localhost:50051/murmur.v1.QueryService/Get \ -// -H 'Content-Type: application/json' -d '{"entity": "page-A"}' +// grpcurl -plaintext -d '{"entity": "page-A", "duration_seconds": 86400}' \ +// localhost:50051 murmur.v1.QueryService/GetWindow +// curl -X POST http://localhost:50051/murmur.v1.QueryService/GetWindow \ +// -H 'Content-Type: application/json' \ +// -d '{"entity": "page-A", "duration_seconds": 86400}' package main import ( diff --git a/examples/recently-interacted-topk/README.md b/examples/recently-interacted-topk/README.md index 504fde7..a850f57 100644 --- a/examples/recently-interacted-topk/README.md +++ b/examples/recently-interacted-topk/README.md @@ -101,15 +101,21 @@ go run ./examples/recently-interacted-topk/cmd/query Then query the merged Top-N: ```sh -# All time (single bucket if non-windowed) -grpcurl -plaintext -d '{"entity":"global"}' \ - localhost:50051 murmur.v1.QueryService/Get - # Last 7 days (merges 7 daily Misra-Gries summaries) grpcurl -plaintext -d '{"entity":"global","duration_seconds":604800}' \ localhost:50051 murmur.v1.QueryService/GetWindow + +# Last 24 hours +grpcurl -plaintext -d '{"entity":"global","duration_seconds":86400}' \ + localhost:50051 murmur.v1.QueryService/GetWindow ``` +`cmd/query` configures daily windowing, so `Get` / `GetMany` are not the +right RPCs here: they read the all-time row at bucket 0, which the windowed +writers never touch. The server returns `FAILED_PRECONDITION` for them +rather than an empty result that looks like missing data. Run the pipeline +without a window if you want an all-time `Get`. + The response's `data` is a serialized Misra-Gries summary (raw bytes); decode via `pkg/monoid/sketch/topk.Decode`, or use the embedded admin UI which renders the items + counts directly. diff --git a/examples/recently-interacted-topk/cmd/query/main.go b/examples/recently-interacted-topk/cmd/query/main.go index 3be4322..c3ded89 100644 --- a/examples/recently-interacted-topk/cmd/query/main.go +++ b/examples/recently-interacted-topk/cmd/query/main.go @@ -1,11 +1,16 @@ // Connect-RPC query server for the recently-interacted-topk example. // -// Serves Get / GetMany / GetWindow / GetRange against the same DynamoDB +// Serves GetWindow / GetWindowMany / GetRange against the same DynamoDB // row both writers (Lambda + ECS Kafka worker) merge into. The byte-encoded // Misra-Gries summary is returned verbatim; clients decode via the // pkg/monoid/sketch/topk package's Decode helper, or via the embedded // admin UI which renders the sketch's items + counts directly. // +// This server configures daily windowing, so Get / GetMany are rejected with +// FAILED_PRECONDITION: they read the all-time row at bucket 0, which the +// windowed writers never populate, and an empty answer there is +// indistinguishable from a genuinely idle pipeline. +// // Run locally: // // export DDB_ENDPOINT=http://localhost:8000 @@ -13,9 +18,9 @@ // // Then call it: // -// # all-time top entities (when running non-windowed) -// grpcurl -plaintext -d '{"entity":"global"}' \ -// localhost:50051 murmur.v1.QueryService/Get +// # top entities over the last 24 hours +// grpcurl -plaintext -d '{"entity":"global","duration_seconds":86400}' \ +// localhost:50051 murmur.v1.QueryService/GetWindow // # top entities over the last 7 days // grpcurl -plaintext -d '{"entity":"global","duration_seconds":604800}' \ // localhost:50051 murmur.v1.QueryService/GetWindow diff --git a/examples/recently-interacted-topk/terraform/README.md b/examples/recently-interacted-topk/terraform/README.md index dd447ee..c74214a 100644 --- a/examples/recently-interacted-topk/terraform/README.md +++ b/examples/recently-interacted-topk/terraform/README.md @@ -113,8 +113,8 @@ inside the VPC (jump host, bastion, EC2 instance in the same VPC): ```sh ENDPOINT=$(terraform output -raw query_service_endpoint) -grpcurl -plaintext -d '{"entity":"global"}' \ - "$ENDPOINT" murmur.v1.QueryService/Get +grpcurl -plaintext -d '{"entity":"global","duration_seconds":86400}' \ + "$ENDPOINT" murmur.v1.QueryService/GetWindow # → Top-N with product-42 at count = 2 (one from each source). grpcurl -plaintext -d '{"entity":"global","duration_seconds":604800}' \ @@ -122,6 +122,11 @@ grpcurl -plaintext -d '{"entity":"global","duration_seconds":604800}' \ # → 7-day windowed Top-N (merged across daily Misra-Gries summaries). ``` +The deployed pipeline is windowed, so `GetWindow` is the RPC to reach for. +`Get` addresses the all-time row at bucket 0, which no windowed write ever +populates; the server answers it with `FAILED_PRECONDITION` so a soak run +can't mistake a routing error for zero traffic. + ### 6. Observability during the soak CloudWatch namespaces to watch: diff --git a/examples/search-rerank/rerank.go b/examples/search-rerank/rerank.go index f7f81da..daa82da 100644 --- a/examples/search-rerank/rerank.go +++ b/examples/search-rerank/rerank.go @@ -250,7 +250,15 @@ func (s *Service) fetchAllTime(ctx context.Context, ids []string) []int64 { s.stats.LikesMisses.Add(int64(len(ids))) return out } - for i, v := range resp.Msg.GetValues() { + values := resp.Msg.GetValues() + // The response is positional — values[i] is ids[i]. A count mismatch means the + // values can't be attributed to candidates at all, and indexing out[i] off the + // response length would panic the reranker on whatever the query server sent. + if len(values) != len(ids) { + s.stats.LikesMisses.Add(int64(len(ids))) + return out + } + for i, v := range values { if v.GetPresent() && len(v.GetData()) >= 8 { out[i] = decodeInt64LE(v.GetData()) s.stats.LikesAllHits.Add(1) @@ -274,7 +282,12 @@ func (s *Service) fetchWindowed(ctx context.Context, ids []string) []int64 { if err != nil { return out } - for i, v := range resp.Msg.GetValues() { + values := resp.Msg.GetValues() + // Positional response; see fetchAllTime. + if len(values) != len(ids) { + return out + } + for i, v := range values { if v.GetPresent() && len(v.GetData()) >= 8 { out[i] = decodeInt64LE(v.GetData()) s.stats.LikesWindowHits.Add(1) diff --git a/pkg/monoid/windowed/windowed.go b/pkg/monoid/windowed/windowed.go index f7dfd89..62d0215 100644 --- a/pkg/monoid/windowed/windowed.go +++ b/pkg/monoid/windowed/windowed.go @@ -24,6 +24,45 @@ type Config struct { // Retention is how long buckets are kept before TTL eviction. Sliding-window queries // can ask for any range up to Retention. Retention time.Duration + + // MaxBuckets caps how many buckets a single read may span. Leave it zero to take + // the default from Retention (see MaxBucketSpan). + // + // Without a cap, a caller who passes an open-ended range gets no error — just an + // enormous key list. GetRange(entity, Unix(0,0), now) against Minute granularity + // builds 29,797,201 keys (~715MB of state.Key) before the first store call, and + // all but the last Retention/Granularity of them address buckets that TTL evicted + // and can never hold data again. + MaxBuckets int +} + +// MaxBucketSpan reports the largest number of buckets one read may span. It is +// MaxBuckets when set, otherwise ceil(Retention / Granularity) — reading further back +// than Retention can only return TTL-evicted buckets. Zero means unbounded, which +// happens only when neither MaxBuckets nor Retention is configured. +func (c Config) MaxBucketSpan() int64 { + if c.MaxBuckets > 0 { + return int64(c.MaxBuckets) + } + if c.Granularity <= 0 || c.Retention <= 0 { + return 0 + } + n := int64(c.Retention / c.Granularity) + if c.Retention%c.Granularity != 0 { + n++ + } + return n +} + +// RetentionBuckets reports how many whole buckets fit inside Retention — the longest +// window a read can ask for and still be answered entirely from live buckets. Zero +// means Retention is unset (or shorter than a single bucket), in which case callers +// should not enforce a retention bound. +func (c Config) RetentionBuckets() int64 { + if c.Granularity <= 0 || c.Retention <= 0 { + return 0 + } + return int64(c.Retention / c.Granularity) } // Daily returns a Config with 24h granularity and the given retention. The most common diff --git a/pkg/query/grpc/server.go b/pkg/query/grpc/server.go index ffcb94f..e7130ec 100644 --- a/pkg/query/grpc/server.go +++ b/pkg/query/grpc/server.go @@ -26,8 +26,9 @@ import ( "context" "encoding/binary" "errors" + "fmt" + "math" "net/http" - "sort" "strconv" "strings" "time" @@ -72,13 +73,14 @@ func BytesIdentity() Encoder[[]byte] { // underlying store call. The dedup window is the lifetime of the in-flight // call — once the future resolves, the next request is fresh. type Server[V any] struct { - store state.Store[V] - mon monoid.Monoid[V] - window *windowed.Config - encode Encoder[V] - nowFn func() time.Time - recorder metrics.Recorder - pipeline string + store state.Store[V] + mon monoid.Monoid[V] + window *windowed.Config + encode Encoder[V] + nowFn func() time.Time + recorder metrics.Recorder + pipeline string + coalesceTimeout time.Duration // sf coalesces concurrent identical reads. Cheap when traffic is cold // (a no-op fastpath); huge wins on hot keys at feed-render time. @@ -111,8 +113,21 @@ type Config[V any] struct { // labels. Defaults to "query" when unset; set explicitly when one // process serves multiple pipelines. Pipeline string + + // CoalesceTimeout bounds the store call a singleflight group runs on behalf + // of its waiters. That call is deliberately detached from the context of + // whichever caller happened to lead the group — otherwise one client hanging + // up cancels the read out from under every peer coalesced onto it — so it + // needs a deadline of its own or a wedged store pins the group forever. + // Defaults to defaultCoalesceTimeout. + CoalesceTimeout time.Duration } +// defaultCoalesceTimeout bounds detached singleflight work when Config leaves +// CoalesceTimeout unset. Long enough for a multi-chunk BatchGetItem with retries, +// short enough that a wedged store frees the group inside one health-check interval. +const defaultCoalesceTimeout = 10 * time.Second + // NewServer constructs a query Server. func NewServer[V any](cfg Config[V]) *Server[V] { now := cfg.Now @@ -127,14 +142,19 @@ func NewServer[V any](cfg Config[V]) *Server[V] { if pipe == "" { pipe = "query" } + coalesceTimeout := cfg.CoalesceTimeout + if coalesceTimeout <= 0 { + coalesceTimeout = defaultCoalesceTimeout + } return &Server[V]{ - store: cfg.Store, - mon: cfg.Monoid, - window: cfg.Window, - encode: cfg.Encode, - nowFn: now, - recorder: rec, - pipeline: pipe, + store: cfg.Store, + mon: cfg.Monoid, + window: cfg.Window, + encode: cfg.Encode, + nowFn: now, + recorder: rec, + pipeline: pipe, + coalesceTimeout: coalesceTimeout, } } @@ -145,22 +165,122 @@ type coalescedResult[V any] struct { present bool } -// coalesceGet runs fn at most once per concurrent group keyed by `key`. The -// first caller does the actual work; everyone else awaits the same result. -func coalesceGet[V any](sf *singleflight.Group, key string, fn func() (V, bool, error)) (V, bool, error) { - out, err, _ := sf.Do(key, func() (any, error) { - v, ok, err := fn() +// coalesce runs fn at most once per concurrent group keyed by `key`; every other +// caller in the group awaits the same result. +// +// Two things the plain singleflight.Do version got wrong. The shared work ran on +// the context of whichever caller happened to arrive first, so a single client +// hanging up cancelled the store read out from under every peer and failed all of +// them with CodeInternal — a failure that only appears under exactly the concurrent +// load coalescing exists to serve. And detaching that context outright would drop +// the client deadline with it, letting a wedged store call hold the group open +// indefinitely, so the detached work carries a server-side bound instead. +// +// Each waiter selects on its OWN context, so a caller that goes away leaves without +// disturbing the group. +func coalesce[R any]( + ctx context.Context, + sf *singleflight.Group, + key string, + timeout time.Duration, + fn func(context.Context) (R, error), +) (R, error) { + var zero R + ch := sf.DoChan(key, func() (any, error) { + workCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), timeout) + defer cancel() + v, err := fn(workCtx) if err != nil { return nil, err } - return coalescedResult[V]{value: v, present: ok}, nil + return v, nil }) - if err != nil { - var zero V - return zero, false, err + select { + case <-ctx.Done(): + return zero, ctx.Err() + case res := <-ch: + if res.Err != nil { + return zero, res.Err + } + v, ok := res.Val.(R) + if !ok { + return zero, fmt.Errorf("query: coalesced result for %q has unexpected type %T", key, res.Val) + } + return v, nil + } +} + +// fail maps err onto a Connect status and records it against the pipeline — +// except for the request-shaped rejections, which are the caller's mistake. +// Counting a malformed range as a pipeline error is the same conflation that +// sends the next operator looking for a store outage. +func (s *Server[V]) fail(err error) error { + out := rpcError(err) + if connect.CodeOf(out) == connect.CodeInternal { + s.recorder.RecordError(s.pipeline, err) } - r := out.(coalescedResult[V]) - return r.value, r.present, nil + return out +} + +// rpcError maps a read-path error onto a Connect status code. Request-shaped +// rejections belong to the caller, not the store: reporting a swapped time range +// as CodeInternal sent operators hunting a DynamoDB outage that never happened. +func rpcError(err error) error { + var connErr *connect.Error + if errors.As(err, &connErr) { + return err + } + switch { + case errors.Is(err, query.ErrInvalidQuery): + return connect.NewError(connect.CodeInvalidArgument, err) + case errors.Is(err, context.Canceled): + return connect.NewError(connect.CodeCanceled, err) + case errors.Is(err, context.DeadlineExceeded): + return connect.NewError(connect.CodeDeadlineExceeded, err) + default: + return connect.NewError(connect.CodeInternal, err) + } +} + +// requireNonWindowed rejects an all-time read against a windowed pipeline. Get and +// GetMany read bucket 0, which on a windowed pipeline is both the all-time sentinel +// and the epoch bucket — so they can only ever report absent, however much data the +// pipeline has written. Four shipped runbooks pointed operators at Get for windowed +// counters before this became an error. A Granularity of zero legitimately writes +// bucket 0 even with a Window configured, so that shape stays allowed. +func (s *Server[V]) requireNonWindowed(alt string) error { + if s.window != nil && s.window.Granularity > 0 { + return connect.NewError(connect.CodeFailedPrecondition, + fmt.Errorf("pipeline is windowed; bucket 0 holds no data — use %s instead", alt)) + } + return nil +} + +// durationFromSeconds converts a request's duration_seconds into a time.Duration. +// duration_seconds above ~9.2e9 overflows the nanosecond representation and wraps +// NEGATIVE, which then read as a perfectly ordinary tiny window rather than an error. +func durationFromSeconds(sec int64) (time.Duration, error) { + const maxSeconds = int64(math.MaxInt64) / int64(time.Second) + if sec <= 0 { + return 0, connect.NewError(connect.CodeInvalidArgument, + fmt.Errorf("duration_seconds must be > 0, got %d", sec)) + } + if sec > maxSeconds { + return 0, connect.NewError(connect.CodeInvalidArgument, + fmt.Errorf("duration_seconds %d exceeds the representable maximum %d", sec, maxSeconds)) + } + return time.Duration(sec) * time.Second, nil +} + +// requireRangeBounds rejects an absolute range whose bounds were never set. proto3 +// scalars have no presence, so an omitted start_unix/end_unix arrives as the epoch +// — previously answered with a fabricated Present:true zero over bucket 0. +func requireRangeBounds(startUnix, endUnix int64) error { + if startUnix == 0 && endUnix == 0 { + return connect.NewError(connect.CodeInvalidArgument, + errors.New("start_unix and end_unix are required")) + } + return nil } // Handler returns the Connect HTTP handler and its mount path. Wire it into a @@ -181,6 +301,10 @@ func (s *Server[V]) Handler() (string, http.Handler) { // {present: false, data: nil}; clients should branch on `present` rather than // on len(data). // +// Returns CodeFailedPrecondition on a windowed pipeline — see +// requireNonWindowed. fresh_read does not bypass that check: a windowed +// pipeline has no all-time row to read freshly. +// // Concurrent identical Gets are coalesced via singleflight: under load on a // hot entity, one underlying store.Get serves N waiters. Set // `req.fresh_read = true` to bypass coalescing and force an authoritative @@ -193,32 +317,38 @@ func (s *Server[V]) Get(ctx context.Context, req *connect.Request[pb.GetRequest] }() s.recorder.RecordEvent(s.pipeline + ":query_get") + if err := s.requireNonWindowed("GetWindow"); err != nil { + return nil, err + } entity := req.Msg.GetEntity() - doGet := func() (V, bool, error) { return query.Get(ctx, s.store, entity) } + doGet := func(ctx context.Context) (coalescedResult[V], error) { + v, ok, err := query.Get(ctx, s.store, entity) + return coalescedResult[V]{value: v, present: ok}, err + } var ( - v V - ok bool + r coalescedResult[V] err error ) if req.Msg.GetFreshRead() { - v, ok, err = doGet() + r, err = doGet(ctx) } else { - v, ok, err = coalesceGet(&s.sf, "Get|"+entity, doGet) + r, err = coalesce(ctx, &s.sf, "Get|"+encodeEntities([]string{entity}), s.coalesceTimeout, doGet) } if err != nil { - return nil, connect.NewError(connect.CodeInternal, err) + return nil, s.fail(err) } val := &pb.Value{Present: false} - if ok { - val = &pb.Value{Present: true, Data: s.encode(v)} + if r.present { + val = &pb.Value{Present: true, Data: s.encode(r.value)} } return connect.NewResponse(&pb.GetResponse{Value: val}), nil } // GetMany implements murmur.v1.QueryService/GetMany. Same shape as Get but // for many entities in one round-trip; the response preserves request order -// so clients can zip without an extra index map. +// so clients can zip without an extra index map. Same windowed-pipeline +// precondition as Get. func (s *Server[V]) GetMany(ctx context.Context, req *connect.Request[pb.GetManyRequest]) (*connect.Response[pb.GetManyResponse], error) { start := time.Now() defer func() { @@ -226,14 +356,16 @@ func (s *Server[V]) GetMany(ctx context.Context, req *connect.Request[pb.GetMany }() s.recorder.RecordEvent(s.pipeline + ":query_get_many") + if err := s.requireNonWindowed("GetWindowMany"); err != nil { + return nil, err + } keys := make([]state.Key, len(req.Msg.GetEntities())) for i, e := range req.Msg.GetEntities() { keys[i] = state.Key{Entity: e} } vals, oks, err := s.store.GetMany(ctx, keys) if err != nil { - s.recorder.RecordError(s.pipeline, err) - return nil, connect.NewError(connect.CodeInternal, err) + return nil, s.fail(err) } out := &pb.GetManyResponse{Values: make([]*pb.Value, len(req.Msg.GetEntities()))} for i := range req.Msg.GetEntities() { @@ -281,28 +413,29 @@ func (s *Server[V]) windowedSingle(ctx context.Context, metric, coalescePrefix, if s.window == nil { return zero, connect.NewError(connect.CodeFailedPrecondition, errors.New("pipeline is not windowed; use Get instead")) } + d, err := durationFromSeconds(durationSeconds) + if err != nil { + return zero, err + } now := s.nowFn() - d := time.Duration(durationSeconds) * time.Second - doFetch := func() (V, bool, error) { - out, err := query.GetWindow(ctx, s.store, s.mon, *s.window, entity, d, now) - return out, true, err + doFetch := func(ctx context.Context) (V, error) { + return query.GetWindow(ctx, s.store, s.mon, *s.window, entity, d, now) } + var v V if freshRead { - v, _, err := doFetch() - if err != nil { - return zero, connect.NewError(connect.CodeInternal, err) - } - return v, nil + v, err = doFetch(ctx) + } else { + // Coalesce key: bucketed "now" means consecutive requests within the + // same bucket reuse a single store call; first request in a new bucket + // does the work. This bounds staleness to at most one bucket. + bucket := s.window.BucketID(now) + key := coalescePrefix + "|" + strconv.FormatInt(durationSeconds, 10) + "|" + + strconv.FormatInt(bucket, 10) + "|" + encodeEntities([]string{entity}) + v, err = coalesce(ctx, &s.sf, key, s.coalesceTimeout, doFetch) } - // Coalesce key: bucketed "now" means consecutive requests within the - // same bucket reuse a single store call; first request in a new bucket - // does the work. This bounds staleness to at most one bucket. - bucket := s.window.BucketID(now) - key := coalescePrefix + "|" + entity + "|" + strconv.FormatInt(durationSeconds, 10) + "|" + strconv.FormatInt(bucket, 10) - v, _, err := coalesceGet(&s.sf, key, doFetch) if err != nil { - return zero, connect.NewError(connect.CodeInternal, err) + return zero, s.fail(err) } return v, nil } @@ -326,12 +459,14 @@ func (s *Server[V]) GetRange(ctx context.Context, req *connect.Request[pb.GetRan } startUnix := req.Msg.GetStartUnix() endUnix := req.Msg.GetEndUnix() + if err := requireRangeBounds(startUnix, endUnix); err != nil { + return nil, err + } entity := req.Msg.GetEntity() - doFetch := func() (V, bool, error) { + doFetch := func(ctx context.Context) (V, error) { start := time.Unix(startUnix, 0).UTC() end := time.Unix(endUnix, 0).UTC() - out, err := query.GetRange(ctx, s.store, s.mon, *s.window, entity, start, end) - return out, true, err + return query.GetRange(ctx, s.store, s.mon, *s.window, entity, start, end) } var ( @@ -339,13 +474,14 @@ func (s *Server[V]) GetRange(ctx context.Context, req *connect.Request[pb.GetRan err error ) if req.Msg.GetFreshRead() { - v, _, err = doFetch() + v, err = doFetch(ctx) } else { - key := "GetRange|" + entity + "|" + strconv.FormatInt(startUnix, 10) + "|" + strconv.FormatInt(endUnix, 10) - v, _, err = coalesceGet(&s.sf, key, doFetch) + key := "GetRange|" + strconv.FormatInt(startUnix, 10) + "|" + + strconv.FormatInt(endUnix, 10) + "|" + encodeEntities([]string{entity}) + v, err = coalesce(ctx, &s.sf, key, s.coalesceTimeout, doFetch) } if err != nil { - return nil, connect.NewError(connect.CodeInternal, err) + return nil, s.fail(err) } return connect.NewResponse(&pb.GetRangeResponse{ Value: &pb.Value{Present: true, Data: s.encode(v)}, @@ -378,29 +514,29 @@ func (s *Server[V]) windowedMany(ctx context.Context, metric, coalescePrefix str if s.window == nil { return nil, connect.NewError(connect.CodeFailedPrecondition, errors.New("pipeline is not windowed; use GetMany instead")) } + d, err := durationFromSeconds(durationSeconds) + if err != nil { + return nil, err + } now := s.nowFn() - d := time.Duration(durationSeconds) * time.Second - doFetch := func() ([]V, bool, error) { - vs, err := query.GetWindowMany(ctx, s.store, s.mon, *s.window, entities, d, now) - return vs, true, err + doFetch := func(ctx context.Context) ([]V, error) { + return query.GetWindowMany(ctx, s.store, s.mon, *s.window, entities, d, now) } + var vs []V if freshRead { - vs, _, err := doFetch() - if err != nil { - return nil, connect.NewError(connect.CodeInternal, err) - } - return vs, nil - } - // Coalesce key: hash the (sorted) entity list + duration + bucket. - // Sorting normalizes equivalent permutations onto the same coalesce - // key. For typical query shapes (a fixed candidate set per query), - // concurrent identical reads collapse to one store fetch. - bucket := s.window.BucketID(now) - key := coalescePrefix + "|" + sortedJoin(entities) + "|" + strconv.FormatInt(durationSeconds, 10) + "|" + strconv.FormatInt(bucket, 10) - vs, _, err := coalesceGetSlice(&s.sf, key, doFetch) + vs, err = doFetch(ctx) + } else { + // Coalesce key: the entity list + duration + bucket. For typical query + // shapes (a fixed candidate set per query), concurrent identical reads + // collapse to one store fetch. + bucket := s.window.BucketID(now) + key := coalescePrefix + "|" + strconv.FormatInt(durationSeconds, 10) + "|" + + strconv.FormatInt(bucket, 10) + "|" + encodeEntities(entities) + vs, err = coalesce(ctx, &s.sf, key, s.coalesceTimeout, doFetch) + } if err != nil { - return nil, connect.NewError(connect.CodeInternal, err) + return nil, s.fail(err) } return vs, nil } @@ -430,12 +566,14 @@ func (s *Server[V]) GetRangeMany(ctx context.Context, req *connect.Request[pb.Ge entities := req.Msg.GetEntities() startUnix := req.Msg.GetStartUnix() endUnix := req.Msg.GetEndUnix() + if err := requireRangeBounds(startUnix, endUnix); err != nil { + return nil, err + } - doFetch := func() ([]V, bool, error) { + doFetch := func(ctx context.Context) ([]V, error) { start := time.Unix(startUnix, 0).UTC() end := time.Unix(endUnix, 0).UTC() - vs, err := query.GetRangeMany(ctx, s.store, s.mon, *s.window, entities, start, end) - return vs, true, err + return query.GetRangeMany(ctx, s.store, s.mon, *s.window, entities, start, end) } var ( @@ -443,13 +581,14 @@ func (s *Server[V]) GetRangeMany(ctx context.Context, req *connect.Request[pb.Ge err error ) if req.Msg.GetFreshRead() { - vs, _, err = doFetch() + vs, err = doFetch(ctx) } else { - key := "GetRangeMany|" + sortedJoin(entities) + "|" + strconv.FormatInt(startUnix, 10) + "|" + strconv.FormatInt(endUnix, 10) - vs, _, err = coalesceGetSlice(&s.sf, key, doFetch) + key := "GetRangeMany|" + strconv.FormatInt(startUnix, 10) + "|" + + strconv.FormatInt(endUnix, 10) + "|" + encodeEntities(entities) + vs, err = coalesce(ctx, &s.sf, key, s.coalesceTimeout, doFetch) } if err != nil { - return nil, connect.NewError(connect.CodeInternal, err) + return nil, s.fail(err) } return connect.NewResponse(&pb.GetRangeManyResponse{Values: s.encodeMany(vs)}), nil } @@ -482,37 +621,31 @@ func (s *Server[V]) GetTrailingMany(ctx context.Context, req *connect.Request[pb return connect.NewResponse(&pb.GetTrailingManyResponse{Values: s.encodeMany(vs)}), nil } -// coalesceGetSlice is the slice-result analog of coalesceGet. Used by the -// "Many" endpoints whose return shape is []V instead of (V, bool). -func coalesceGetSlice[V any](sf *singleflight.Group, key string, fn func() ([]V, bool, error)) ([]V, bool, error) { - out, err, _ := sf.Do(key, func() (any, error) { - v, ok, err := fn() - if err != nil { - return nil, err - } - return coalescedSliceResult[V]{values: v, present: ok}, nil - }) - if err != nil { - return nil, false, err - } - r := out.(coalescedSliceResult[V]) - return r.values, r.present, nil -} - -type coalescedSliceResult[V any] struct { - values []V - present bool -} - -// sortedJoin returns the entities sorted and joined by '|'. Used by the -// singleflight coalesce keys so two requests with the same set of entities -// in different orders collapse to the same key. -func sortedJoin(entities []string) string { - if len(entities) == 0 { - return "" - } - cp := make([]string, len(entities)) - copy(cp, entities) - sort.Strings(cp) - return strings.Join(cp, "|") +// encodeEntities builds the entity-list fragment of a singleflight coalesce key. +// +// Each entry is length-prefixed, and the list keeps its request order. Both parts +// are load-bearing: +// +// A plain '|' join is not injective over entity strings that may themselves +// contain '|' — and the shipped codegen key_template does exactly that +// (examples/typed-rpc-codegen/bot-interactions/pipeline-spec.yaml). ["a|b"] and +// ["a","b"] joined to the same key, as did ["a|b","c"] and ["a","b|c"]; a length +// check catches neither, since both pairs agree on total byte count. Two callers +// asking about different entities then shared one result slice. +// +// Order is preserved because responses are positional — value[i] belongs to +// entities[i]. Sorting made ["a","b"] and ["b","a"] one group, so the second +// caller received the first caller's slice with every value attributed to the +// wrong entity. Permutation coalescing is given up deliberately; silently +// transposed counters are far worse than a missed dedup. +func encodeEntities(entities []string) string { + var b strings.Builder + b.WriteString(strconv.Itoa(len(entities))) + for _, e := range entities { + b.WriteByte('|') + b.WriteString(strconv.Itoa(len(e))) + b.WriteByte(':') + b.WriteString(e) + } + return b.String() } diff --git a/pkg/query/grpc/validation_test.go b/pkg/query/grpc/validation_test.go new file mode 100644 index 0000000..add3c99 --- /dev/null +++ b/pkg/query/grpc/validation_test.go @@ -0,0 +1,407 @@ +package grpc_test + +import ( + "context" + "sync" + "testing" + "time" + + "connectrpc.com/connect" + + "github.com/gallowaysoftware/murmur/pkg/monoid/core" + "github.com/gallowaysoftware/murmur/pkg/monoid/windowed" + mgrpc "github.com/gallowaysoftware/murmur/pkg/query/grpc" + "github.com/gallowaysoftware/murmur/pkg/state" + pb "github.com/gallowaysoftware/murmur/proto/gen/murmur/v1" +) + +// TestQueryServer_GetRejectsWindowedPipeline pins the routing guard. On a windowed +// pipeline, Get and GetMany read bucket 0, which is simultaneously the all-time +// sentinel and the epoch bucket — so they always reported absent no matter how much +// the pipeline had counted, and four shipped runbooks told operators to call them. +func TestQueryServer_GetRejectsWindowedPipeline(t *testing.T) { + w := windowed.Daily(30 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + store := fakeStore{ + state.Key{Entity: "page-A", Bucket: w.BucketID(now)}: 42, + } + client, cleanup := startServer(t, mgrpc.Config[int64]{ + Store: store, Monoid: core.Sum[int64](), Window: &w, Encode: mgrpc.Int64LE(), + Now: func() time.Time { return now }, + }) + defer cleanup() + ctx := context.Background() + + // fresh_read must not route around the guard: a windowed pipeline has no + // all-time row to read freshly. + for _, fresh := range []bool{false, true} { + _, err := client.Get(ctx, connect.NewRequest(&pb.GetRequest{Entity: "page-A", FreshRead: fresh})) + if err == nil { + t.Fatalf("Get(fresh_read=%v) on a windowed pipeline: got nil error, want FailedPrecondition", fresh) + } + if code := connect.CodeOf(err); code != connect.CodeFailedPrecondition { + t.Errorf("Get(fresh_read=%v): got code %v (%v), want FailedPrecondition", fresh, code, err) + } + + _, err = client.GetMany(ctx, connect.NewRequest(&pb.GetManyRequest{ + Entities: []string{"page-A"}, FreshRead: fresh, + })) + if err == nil { + t.Fatalf("GetMany(fresh_read=%v) on a windowed pipeline: got nil error, want FailedPrecondition", fresh) + } + if code := connect.CodeOf(err); code != connect.CodeFailedPrecondition { + t.Errorf("GetMany(fresh_read=%v): got code %v (%v), want FailedPrecondition", fresh, code, err) + } + } + + // The RPC the guard points at still answers. + resp, err := client.GetWindow(ctx, connect.NewRequest(&pb.GetWindowRequest{ + Entity: "page-A", DurationSeconds: 86400, + })) + if err != nil { + t.Fatalf("GetWindow: %v", err) + } + if got := decodeInt64(resp.Msg.GetValue().GetData()); got != 42 { + t.Errorf("GetWindow: got %d, want 42", got) + } +} + +// TestQueryServer_GetAllowsZeroGranularityWindow keeps the escape hatch open: a +// Window whose Granularity is zero writes bucket 0 for real, so Get is the right +// RPC for it. +func TestQueryServer_GetAllowsZeroGranularityWindow(t *testing.T) { + w := windowed.Config{Retention: 30 * 24 * time.Hour} + store := fakeStore{state.Key{Entity: "page-A"}: 42} + client, cleanup := startServer(t, mgrpc.Config[int64]{ + Store: store, Monoid: core.Sum[int64](), Window: &w, Encode: mgrpc.Int64LE(), + }) + defer cleanup() + + resp, err := client.Get(context.Background(), connect.NewRequest(&pb.GetRequest{Entity: "page-A"})) + if err != nil { + t.Fatalf("Get with Granularity=0: %v", err) + } + if !resp.Msg.GetValue().GetPresent() { + t.Error("Get with Granularity=0: reported absent, want present") + } +} + +func TestQueryServer_GetRangeRejectsDegenerateBounds(t *testing.T) { + w := windowed.Daily(30 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + client, cleanup := startServer(t, mgrpc.Config[int64]{ + Store: fakeStore{}, Monoid: core.Sum[int64](), Window: &w, Encode: mgrpc.Int64LE(), + Now: func() time.Time { return now }, + }) + defer cleanup() + ctx := context.Background() + + cases := []struct { + name string + startUnix int64 + endUnix int64 + freshRead bool + wantErrSubstring string + }{ + {name: "start after end", startUnix: now.Unix(), endUnix: now.Add(-24 * time.Hour).Unix()}, + {name: "both bounds unset", startUnix: 0, endUnix: 0}, + {name: "both bounds unset, fresh_read", startUnix: 0, endUnix: 0, freshRead: true}, + // What pkg/query/typed puts on the wire for a zero time.Time. + {name: "zero time.Time start", startUnix: -62135596800, endUnix: now.Unix()}, + {name: "year 9999 end", startUnix: now.Add(-24 * time.Hour).Unix(), endUnix: 253402300799}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resp, err := client.GetRange(ctx, connect.NewRequest(&pb.GetRangeRequest{ + Entity: "page-A", StartUnix: tc.startUnix, EndUnix: tc.endUnix, FreshRead: tc.freshRead, + })) + if err == nil { + t.Fatalf("GetRange(%d, %d): got value{present=%v}, want InvalidArgument", + tc.startUnix, tc.endUnix, resp.Msg.GetValue().GetPresent()) + } + if code := connect.CodeOf(err); code != connect.CodeInvalidArgument { + t.Errorf("GetRange(%d, %d): got code %v (%v), want InvalidArgument", + tc.startUnix, tc.endUnix, code, err) + } + }) + } + + t.Run("GetRangeMany", func(t *testing.T) { + _, err := client.GetRangeMany(ctx, connect.NewRequest(&pb.GetRangeManyRequest{ + Entities: []string{"page-A"}, StartUnix: now.Unix(), EndUnix: now.Add(-24 * time.Hour).Unix(), + })) + if err == nil { + t.Fatal("GetRangeMany with start after end: got nil error, want InvalidArgument") + } + if code := connect.CodeOf(err); code != connect.CodeInvalidArgument { + t.Errorf("GetRangeMany: got code %v (%v), want InvalidArgument", code, err) + } + }) +} + +func TestQueryServer_GetWindowRejectsInvalidDuration(t *testing.T) { + // 7 days of retention: 7 daily buckets live, anything longer reads evicted + // buckets and folds the holes in as Identity. + w := windowed.Daily(7 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + client, cleanup := startServer(t, mgrpc.Config[int64]{ + Store: fakeStore{}, Monoid: core.Sum[int64](), Window: &w, Encode: mgrpc.Int64LE(), + Now: func() time.Time { return now }, + }) + defer cleanup() + ctx := context.Background() + + cases := []struct { + name string + durationSeconds int64 + }{ + {"unset (proto3 zero)", 0}, + {"negative", -3600}, + {"beyond retention", 90 * 86400}, + // Overflows the nanosecond Duration and used to wrap to a negative one. + {"nanosecond overflow", 10_000_000_000}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + for _, fresh := range []bool{false, true} { + _, err := client.GetWindow(ctx, connect.NewRequest(&pb.GetWindowRequest{ + Entity: "page-A", DurationSeconds: tc.durationSeconds, FreshRead: fresh, + })) + if err == nil { + t.Fatalf("GetWindow(duration_seconds=%d, fresh_read=%v): got nil error, want InvalidArgument", + tc.durationSeconds, fresh) + } + if code := connect.CodeOf(err); code != connect.CodeInvalidArgument { + t.Errorf("GetWindow(duration_seconds=%d, fresh_read=%v): got code %v (%v), want InvalidArgument", + tc.durationSeconds, fresh, code, err) + } + } + _, err := client.GetWindowMany(ctx, connect.NewRequest(&pb.GetWindowManyRequest{ + Entities: []string{"page-A"}, DurationSeconds: tc.durationSeconds, + })) + if err == nil { + t.Fatalf("GetWindowMany(duration_seconds=%d): got nil error, want InvalidArgument", tc.durationSeconds) + } + if code := connect.CodeOf(err); code != connect.CodeInvalidArgument { + t.Errorf("GetWindowMany(duration_seconds=%d): got code %v (%v), want InvalidArgument", + tc.durationSeconds, code, err) + } + }) + } + + if _, err := client.GetWindow(ctx, connect.NewRequest(&pb.GetWindowRequest{ + Entity: "page-A", DurationSeconds: 7 * 86400, + })); err != nil { + t.Errorf("GetWindow at exactly the retention window: %v", err) + } +} + +// blockingStore parks every GetMany until release is closed, so a test can hold a +// singleflight group open and observe what the other callers in it experience. It +// honors the context it is handed — that is the whole point: a leader-scoped +// context used to cancel this call for everyone. +type blockingStore struct { + values fakeStore + release chan struct{} + arrived chan []state.Key + + mu sync.Mutex + calls int +} + +func newBlockingStore(values fakeStore) *blockingStore { + return &blockingStore{ + values: values, + release: make(chan struct{}), + arrived: make(chan []state.Key, 8), + } +} + +func (s *blockingStore) Get(ctx context.Context, k state.Key) (int64, bool, error) { + vs, oks, err := s.GetMany(ctx, []state.Key{k}) + if err != nil { + return 0, false, err + } + return vs[0], oks[0], nil +} + +func (s *blockingStore) GetMany(ctx context.Context, ks []state.Key) ([]int64, []bool, error) { + s.mu.Lock() + s.calls++ + s.mu.Unlock() + s.arrived <- ks + select { + case <-s.release: + case <-ctx.Done(): + return nil, nil, ctx.Err() + } + return s.values.GetMany(ctx, ks) +} + +func (s *blockingStore) MergeUpdate(context.Context, state.Key, int64, time.Duration) error { + return nil +} +func (s *blockingStore) Close() error { return nil } + +func (s *blockingStore) callCount() int { + s.mu.Lock() + defer s.mu.Unlock() + return s.calls +} + +// TestQueryServer_CoalesceKeepsEntityListsDistinct drives two concurrent +// GetWindowMany requests whose entity lists collided under the old '|'-joined, +// sorted coalesce key. The second caller used to be handed the first caller's +// result slice — a different length, or the same length with every value +// attributed to the wrong entity. +func TestQueryServer_CoalesceKeepsEntityListsDistinct(t *testing.T) { + w := windowed.Daily(30 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + bucket := w.BucketID(now) + + cases := []struct { + name string + first []string + second []string + wantSec []int64 + }{ + // A literal '|' inside an entity key is not hypothetical: the shipped + // codegen key_template builds entity keys that contain one. + {"separator inside an entity", []string{"a|b"}, []string{"a", "b"}, []int64{1, 2}}, + {"separator straddling entities", []string{"a|b", "c"}, []string{"a", "b|c"}, []int64{1, 5}}, + // Sorting made these one group, so the values came back transposed. + {"permuted order", []string{"a", "b"}, []string{"b", "a"}, []int64{2, 1}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + store := newBlockingStore(fakeStore{ + state.Key{Entity: "a", Bucket: bucket}: 1, + state.Key{Entity: "b", Bucket: bucket}: 2, + state.Key{Entity: "c", Bucket: bucket}: 3, + state.Key{Entity: "a|b", Bucket: bucket}: 4, + state.Key{Entity: "b|c", Bucket: bucket}: 5, + }) + client, cleanup := startServer(t, mgrpc.Config[int64]{ + Store: store, Monoid: core.Sum[int64](), Window: &w, Encode: mgrpc.Int64LE(), + Now: func() time.Time { return now }, + }) + defer cleanup() + + type result struct { + values []int64 + err error + } + run := func(entities []string) <-chan result { + out := make(chan result, 1) + go func() { + resp, err := client.GetWindowMany(context.Background(), connect.NewRequest(&pb.GetWindowManyRequest{ + Entities: entities, DurationSeconds: 86400, + })) + if err != nil { + out <- result{err: err} + return + } + vals := make([]int64, 0, len(resp.Msg.GetValues())) + for _, v := range resp.Msg.GetValues() { + vals = append(vals, decodeInt64(v.GetData())) + } + out <- result{values: vals} + }() + return out + } + + firstDone := run(tc.first) + // Wait for the first request to reach the store: its group is now + // open, which is the only state in which the second can collide + // with it. + <-store.arrived + + secondDone := run(tc.second) + // The second request forms its own group and reaches the store too. + // Under the colliding key it never did, so fall through after a + // grace period and let the assertions below report what it got. + select { + case <-store.arrived: + case <-time.After(2 * time.Second): + } + close(store.release) + + <-firstDone + second := <-secondDone + if second.err != nil { + t.Fatalf("second GetWindowMany(%v): %v", tc.second, second.err) + } + if len(second.values) != len(tc.wantSec) { + t.Fatalf("second GetWindowMany(%v): got %d values %v, want %d (%v) — coalesced onto the first caller's result", + tc.second, len(second.values), second.values, len(tc.wantSec), tc.wantSec) + } + for i := range tc.wantSec { + if second.values[i] != tc.wantSec[i] { + t.Fatalf("second GetWindowMany(%v): got %v, want %v — values transposed by a shared coalesce key", + tc.second, second.values, tc.wantSec) + } + } + }) + } +} + +// TestQueryServer_CoalescedPeerSurvivesCallerCancellation covers the other half of +// the singleflight bug: the shared store call ran on the leader's context, so one +// client hanging up failed everybody coalesced behind it with CodeInternal. +func TestQueryServer_CoalescedPeerSurvivesCallerCancellation(t *testing.T) { + w := windowed.Daily(30 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + store := newBlockingStore(fakeStore{ + state.Key{Entity: "page-A", Bucket: w.BucketID(now)}: 42, + }) + client, cleanup := startServer(t, mgrpc.Config[int64]{ + Store: store, Monoid: core.Sum[int64](), Window: &w, Encode: mgrpc.Int64LE(), + Now: func() time.Time { return now }, + }) + defer cleanup() + + leaderCtx, cancelLeader := context.WithCancel(context.Background()) + leaderDone := make(chan struct{}) + go func() { + defer close(leaderDone) + _, _ = client.GetWindow(leaderCtx, connect.NewRequest(&pb.GetWindowRequest{ + Entity: "page-A", DurationSeconds: 86400, + })) + }() + // The leader is now parked inside the store with its group open. + <-store.arrived + + peerDone := make(chan error, 1) + peerValue := make(chan int64, 1) + go func() { + resp, err := client.GetWindow(context.Background(), connect.NewRequest(&pb.GetWindowRequest{ + Entity: "page-A", DurationSeconds: 86400, + })) + if err != nil { + peerDone <- err + return + } + peerValue <- decodeInt64(resp.Msg.GetValue().GetData()) + peerDone <- nil + }() + // Give the peer time to join the leader's group (or, if it forms its own, + // to reach the store) before the leader hangs up. + select { + case <-store.arrived: + case <-time.After(500 * time.Millisecond): + } + + cancelLeader() + <-leaderDone + close(store.release) + + if err := <-peerDone; err != nil { + t.Fatalf("peer GetWindow failed after the leading caller cancelled: %v (code %v)", err, connect.CodeOf(err)) + } + if got := <-peerValue; got != 42 { + t.Errorf("peer GetWindow: got %d, want 42", got) + } + if store.callCount() == 0 { + t.Error("store was never called") + } +} diff --git a/pkg/query/lambda.go b/pkg/query/lambda.go index 349a208..074c22c 100644 --- a/pkg/query/lambda.go +++ b/pkg/query/lambda.go @@ -67,7 +67,10 @@ func (q LambdaQuery[V]) GetWindow( duration time.Duration, now time.Time, ) (V, error) { - lo, hi := w.LastN(now, duration) + lo, hi, err := windowBuckets(w, duration, now) + if err != nil { + return q.Monoid.Identity(), err + } return q.mergeBuckets(ctx, entity, lo, hi) } @@ -78,7 +81,10 @@ func (q LambdaQuery[V]) GetRange( entity string, start, end time.Time, ) (V, error) { - lo, hi := w.BucketRange(start, end) + lo, hi, err := rangeBuckets(w, start, end) + if err != nil { + return q.Monoid.Identity(), err + } return q.mergeBuckets(ctx, entity, lo, hi) } diff --git a/pkg/query/typed/typed.go b/pkg/query/typed/typed.go index 8a5b719..61e43b4 100644 --- a/pkg/query/typed/typed.go +++ b/pkg/query/typed/typed.go @@ -77,6 +77,24 @@ import ( // Tests: a fake satisfying the same shape. type Inner = murmurv1connect.QueryServiceClient +// checkValueCount rejects a response whose value count doesn't match the entity +// count that was asked for. +// +// The batched RPCs are strictly positional — values[i] belongs to entities[i] — +// so a mismatch means the values can no longer be attributed to entities at all. +// The old handling was worse than an error in both directions: SumClient wrote +// straight into out[i] for every value the server sent, so a server returning +// more values than entities panicked with index-out-of-range inside the CALLING +// application, remotely triggerable by whatever the server did. The sketch +// clients' `if i >= len(out) { break }` avoided the panic but silently truncated, +// handing back a feature vector that was quietly short of ground truth. +func checkValueCount(rpc string, entities, values int) error { + if entities != values { + return fmt.Errorf("murmur/typed: %s returned %d values for %d entities", rpc, values, entities) + } + return nil +} + // ---------------------------------------------------------------------------- // Sum / Count / Min / Max — int64 wire shape // ---------------------------------------------------------------------------- @@ -128,6 +146,9 @@ func (c *SumClient) GetMany(ctx context.Context, entities []string, opts ...Opti return nil, nil, err } values := resp.Msg.GetValues() + if err := checkValueCount("GetMany", len(entities), len(values)); err != nil { + return nil, nil, err + } out := make([]int64, len(entities)) present := make([]bool, len(entities)) for i, v := range values { @@ -167,11 +188,12 @@ func (c *SumClient) GetWindowMany(ctx context.Context, entities []string, durati return nil, err } values := resp.Msg.GetValues() + if err := checkValueCount("GetWindowMany", len(entities), len(values)); err != nil { + return nil, err + } out := make([]int64, len(entities)) for i, v := range values { - if i < len(values) { - out[i] = DecodeInt64(v.GetData()) - } + out[i] = DecodeInt64(v.GetData()) } return out, nil } @@ -270,12 +292,12 @@ func (c *HLLClient) GetMany(ctx context.Context, entities []string, opts ...Opti return nil, nil, err } values := resp.Msg.GetValues() + if err := checkValueCount("GetMany", len(entities), len(values)); err != nil { + return nil, nil, err + } out := make([]HLLValue, len(entities)) present := make([]bool, len(entities)) for i, v := range values { - if i >= len(out) { - break - } if !v.GetPresent() { continue } @@ -332,11 +354,11 @@ func (c *HLLClient) GetWindowMany(ctx context.Context, entities []string, durati return nil, err } values := resp.Msg.GetValues() + if err := checkValueCount("GetWindowMany", len(entities), len(values)); err != nil { + return nil, err + } out := make([]HLLValue, len(entities)) for i, v := range values { - if i >= len(out) { - break - } data := v.GetData() if len(data) == 0 { continue @@ -439,12 +461,12 @@ func (c *TopKClient) GetMany(ctx context.Context, entities []string, opts ...Opt return nil, nil, err } values := resp.Msg.GetValues() + if err := checkValueCount("GetMany", len(entities), len(values)); err != nil { + return nil, nil, err + } out := make([][]TopKItem, len(entities)) present := make([]bool, len(entities)) for i, v := range values { - if i >= len(out) { - break - } if !v.GetPresent() { continue } @@ -508,11 +530,11 @@ func (c *TopKClient) GetWindowMany(ctx context.Context, entities []string, durat return nil, err } values := resp.Msg.GetValues() + if err := checkValueCount("GetWindowMany", len(entities), len(values)); err != nil { + return nil, err + } out := make([][]TopKItem, len(entities)) for i, v := range values { - if i >= len(out) { - break - } data := v.GetData() if len(data) == 0 { continue @@ -617,12 +639,12 @@ func (c *BloomClient) GetMany(ctx context.Context, entities []string, opts ...Op return nil, nil, err } values := resp.Msg.GetValues() + if err := checkValueCount("GetMany", len(entities), len(values)); err != nil { + return nil, nil, err + } out := make([]BloomValue, len(entities)) present := make([]bool, len(entities)) for i, v := range values { - if i >= len(out) { - break - } if !v.GetPresent() { continue } @@ -686,11 +708,11 @@ func (c *BloomClient) GetWindowMany(ctx context.Context, entities []string, dura return nil, err } values := resp.Msg.GetValues() + if err := checkValueCount("GetWindowMany", len(entities), len(values)); err != nil { + return nil, err + } out := make([]BloomValue, len(entities)) for i, v := range values { - if i >= len(out) { - break - } data := v.GetData() if len(data) == 0 { continue diff --git a/pkg/query/typed/valuecount_test.go b/pkg/query/typed/valuecount_test.go new file mode 100644 index 0000000..0630410 --- /dev/null +++ b/pkg/query/typed/valuecount_test.go @@ -0,0 +1,125 @@ +package typed_test + +import ( + "context" + "encoding/binary" + "testing" + "time" + + "connectrpc.com/connect" + + "github.com/gallowaysoftware/murmur/pkg/monoid/sketch/hll" + "github.com/gallowaysoftware/murmur/pkg/query/typed" + pb "github.com/gallowaysoftware/murmur/proto/gen/murmur/v1" +) + +// overcountingClient is a QueryService client that answers every batched RPC with +// `values` regardless of how many entities were asked about. A real server should +// never do this — which is exactly why the clients trusted it and indexed the +// caller's output slice off the response length. +type overcountingClient struct { + values []*pb.Value +} + +func int64Value(n int64) *pb.Value { + b := make([]byte, 8) + binary.LittleEndian.PutUint64(b, uint64(n)) + return &pb.Value{Present: true, Data: b} +} + +func sketchValue(t *testing.T) *pb.Value { + t.Helper() + return &pb.Value{Present: true, Data: hll.Single([]byte("x"))} +} + +func (c *overcountingClient) Get(context.Context, *connect.Request[pb.GetRequest]) (*connect.Response[pb.GetResponse], error) { + return connect.NewResponse(&pb.GetResponse{Value: c.values[0]}), nil +} + +func (c *overcountingClient) GetWindow(context.Context, *connect.Request[pb.GetWindowRequest]) (*connect.Response[pb.GetWindowResponse], error) { + return connect.NewResponse(&pb.GetWindowResponse{Value: c.values[0]}), nil +} + +func (c *overcountingClient) GetRange(context.Context, *connect.Request[pb.GetRangeRequest]) (*connect.Response[pb.GetRangeResponse], error) { + return connect.NewResponse(&pb.GetRangeResponse{Value: c.values[0]}), nil +} + +func (c *overcountingClient) GetMany(context.Context, *connect.Request[pb.GetManyRequest]) (*connect.Response[pb.GetManyResponse], error) { + return connect.NewResponse(&pb.GetManyResponse{Values: c.values}), nil +} + +func (c *overcountingClient) GetWindowMany(context.Context, *connect.Request[pb.GetWindowManyRequest]) (*connect.Response[pb.GetWindowManyResponse], error) { + return connect.NewResponse(&pb.GetWindowManyResponse{Values: c.values}), nil +} + +func (c *overcountingClient) GetRangeMany(context.Context, *connect.Request[pb.GetRangeManyRequest]) (*connect.Response[pb.GetRangeManyResponse], error) { + return connect.NewResponse(&pb.GetRangeManyResponse{Values: c.values}), nil +} + +func (c *overcountingClient) GetTrailing(context.Context, *connect.Request[pb.GetTrailingRequest]) (*connect.Response[pb.GetTrailingResponse], error) { + return connect.NewResponse(&pb.GetTrailingResponse{Value: c.values[0]}), nil +} + +func (c *overcountingClient) GetTrailingMany(context.Context, *connect.Request[pb.GetTrailingManyRequest]) (*connect.Response[pb.GetTrailingManyResponse], error) { + return connect.NewResponse(&pb.GetTrailingManyResponse{Values: c.values}), nil +} + +// TestSumClient_RejectsValueCountMismatch pins the fix for a remotely triggerable +// panic: SumClient sized `out` from the entity list but wrote out[i] for every +// value the SERVER returned, so three values for two entities panicked with +// index-out-of-range inside the calling application. +func TestSumClient_RejectsValueCountMismatch(t *testing.T) { + inner := &overcountingClient{values: []*pb.Value{int64Value(1), int64Value(2), int64Value(3)}} + c := typed.NewSumClient(inner) + entities := []string{"a", "b"} + ctx := context.Background() + + if _, _, err := c.GetMany(ctx, entities); err == nil { + t.Error("GetMany: got nil error for 3 values over 2 entities, want a rejection") + } + if _, err := c.GetWindowMany(ctx, entities, 24*time.Hour); err == nil { + t.Error("GetWindowMany: got nil error for 3 values over 2 entities, want a rejection") + } +} + +// TestHLLClient_RejectsValueCountMismatch covers the other half of the same +// defect. The sketch clients guarded the index with `if i >= len(out) { break }`, +// which stopped the panic but silently truncated instead — a short answer that +// still reported success. This asserts the mismatch is now surfaced. +func TestHLLClient_RejectsValueCountMismatch(t *testing.T) { + v := sketchValue(t) + ctx := context.Background() + + t.Run("too many values", func(t *testing.T) { + c := typed.NewHLLClient(&overcountingClient{values: []*pb.Value{v, v, v}}) + if _, _, err := c.GetMany(ctx, []string{"a", "b"}); err == nil { + t.Error("GetMany: got nil error for 3 values over 2 entities, want a rejection") + } + if _, err := c.GetWindowMany(ctx, []string{"a", "b"}, 24*time.Hour); err == nil { + t.Error("GetWindowMany: got nil error for 3 values over 2 entities, want a rejection") + } + }) + + t.Run("too few values", func(t *testing.T) { + c := typed.NewHLLClient(&overcountingClient{values: []*pb.Value{v}}) + if _, err := c.GetWindowMany(ctx, []string{"a", "b", "c"}, 24*time.Hour); err == nil { + t.Error("GetWindowMany: got nil error for 1 value over 3 entities, want a rejection") + } + }) +} + +func TestTopKClient_RejectsValueCountMismatch(t *testing.T) { + inner := &overcountingClient{values: []*pb.Value{{Present: true}, {Present: true}, {Present: true}}} + c := typed.NewTopKClient(inner) + if _, _, err := c.GetMany(context.Background(), []string{"a"}); err == nil { + t.Error("GetMany: got nil error for 3 values over 1 entity, want a rejection") + } +} + +func TestBloomClient_RejectsValueCountMismatch(t *testing.T) { + inner := &overcountingClient{values: []*pb.Value{{Present: true}, {Present: true}, {Present: true}}} + c := typed.NewBloomClient(inner) + if _, _, err := c.GetMany(context.Background(), []string{"a"}); err == nil { + t.Error("GetMany: got nil error for 3 values over 1 entity, want a rejection") + } +} diff --git a/pkg/query/validate.go b/pkg/query/validate.go new file mode 100644 index 0000000..9f82214 --- /dev/null +++ b/pkg/query/validate.go @@ -0,0 +1,135 @@ +// Read-path request validation. Every helper in this package used to answer a +// degenerate request with the monoid Identity and no error: a swapped range, a +// negative duration, an unset proto3 bound, or a window longer than Retention all +// produced a confident zero that the gRPC layer then labelled Present:true. An +// operator cannot tell that apart from "the counter really is zero", so the +// checks below turn every one of them into a caller-visible rejection. + +package query + +import ( + "errors" + "fmt" + "math" + "time" + + "github.com/gallowaysoftware/murmur/pkg/monoid/windowed" +) + +// ErrInvalidQuery marks a read rejected because of the request itself rather than +// anything wrong with the store. Test for it with errors.Is; pkg/query/grpc maps it +// onto connect.CodeInvalidArgument so callers stop reading query bugs as DDB outages. +var ErrInvalidQuery = errors.New("query: invalid request") + +// minQueryTime and maxQueryTime bound the instants whose UnixNano — and therefore +// windowed.Config.BucketID — is representable in int64. Outside them BucketID wraps +// silently: end_unix=253402300799 (year 9999) and the zero time.Time that +// pkg/query/typed sends as Unix -62135596800 both land on arbitrary, often negative, +// bucket IDs and merge a slice of state nobody asked for. +var ( + minQueryTime = time.Unix(0, math.MinInt64).UTC() + maxQueryTime = time.Unix(0, math.MaxInt64).UTC() +) + +// invalidQuery builds an ErrInvalidQuery-matching error carrying a message specific +// enough for an operator to fix the callsite from the log line alone. +func invalidQuery(format string, args ...any) error { + return &invalidQueryError{msg: "query: " + fmt.Sprintf(format, args...)} +} + +type invalidQueryError struct{ msg string } + +func (e *invalidQueryError) Error() string { return e.msg } + +// Is reports a match against ErrInvalidQuery so callers can branch on the class +// without depending on the concrete type. +func (e *invalidQueryError) Is(target error) bool { return target == ErrInvalidQuery } + +// windowBuckets validates a trailing-duration request and returns the inclusive +// bucket range it covers. Shared by GetWindow / GetWindowMany / LambdaQuery / +// WarmupWindowed so all of them reject the same shapes. +func windowBuckets(w windowed.Config, d time.Duration, now time.Time) (lo, hi int64, err error) { + if d <= 0 { + return 0, 0, invalidQuery("duration must be positive, got %s", d) + } + if now.Before(minQueryTime) || now.After(maxQueryTime) { + return 0, 0, invalidQuery("now %s is outside the representable bucket range [%s, %s]", + now.UTC().Format(time.RFC3339), minQueryTime.Format(time.RFC3339), maxQueryTime.Format(time.RFC3339)) + } + if err := checkRetention(w, d); err != nil { + return 0, 0, err + } + lo, hi = w.LastN(now, d) + if err := checkSpan(w, lo, hi); err != nil { + return 0, 0, err + } + return lo, hi, nil +} + +// rangeBuckets validates an absolute [start, end] request and returns the inclusive +// bucket range it covers. +func rangeBuckets(w windowed.Config, start, end time.Time) (lo, hi int64, err error) { + if err := checkRepresentable("start", start); err != nil { + return 0, 0, err + } + if err := checkRepresentable("end", end); err != nil { + return 0, 0, err + } + if end.Before(start) { + return 0, 0, invalidQuery("end %s precedes start %s", + end.UTC().Format(time.RFC3339), start.UTC().Format(time.RFC3339)) + } + lo, hi = w.BucketRange(start, end) + if err := checkSpan(w, lo, hi); err != nil { + return 0, 0, err + } + return lo, hi, nil +} + +func checkRepresentable(name string, t time.Time) error { + if t.Before(minQueryTime) || t.After(maxQueryTime) { + return invalidQuery("%s %s is outside the representable bucket range [%s, %s]", + name, t.UTC().Format(time.RFC3339), + minQueryTime.Format(time.RFC3339), maxQueryTime.Format(time.RFC3339)) + } + return nil +} + +// checkRetention rejects a window longer than the buckets still exist for. Retention +// was advisory on the read path: asking for 90 days against a 7-day Retention read 83 +// TTL-evicted buckets, folded them in as Identity, and returned the result labelled as +// a full 90-day window. A short window that happens to be missing buckets is normal +// and stays silent — this only catches the case where the bucket range itself reaches +// past what TTL keeps. +func checkRetention(w windowed.Config, d time.Duration) error { + limit := w.RetentionBuckets() + if limit <= 0 { + return nil + } + n := int64(d / w.Granularity) + if d%w.Granularity != 0 { + n++ + } + if n > limit { + return invalidQuery("duration %s spans %d buckets of %s, beyond the %s retention (%d buckets)", + d, n, w.Granularity, w.Retention, limit) + } + return nil +} + +// checkSpan enforces windowed.Config.MaxBucketSpan on an already-computed bucket +// range. This is the last line before the key slice is materialized, so it runs on +// every path that fans a read out over buckets. +func checkSpan(w windowed.Config, lo, hi int64) error { + limit := w.MaxBucketSpan() + if limit <= 0 { + return nil + } + span := hi - lo + // A negative span here means hi-lo overflowed int64, which a nanosecond-scale + // Granularity makes reachable from two representable instants. + if span < 0 || span >= limit { + return invalidQuery("range spans more than %d buckets (max_buckets); narrow the range or raise MaxBuckets", limit) + } + return nil +} diff --git a/pkg/query/validate_test.go b/pkg/query/validate_test.go new file mode 100644 index 0000000..b44b5f1 --- /dev/null +++ b/pkg/query/validate_test.go @@ -0,0 +1,218 @@ +package query_test + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/gallowaysoftware/murmur/pkg/monoid/core" + "github.com/gallowaysoftware/murmur/pkg/monoid/windowed" + "github.com/gallowaysoftware/murmur/pkg/query" + "github.com/gallowaysoftware/murmur/pkg/state" +) + +// keyProbeStore records the key slices it was asked for, so a test can assert +// that a rejected request never reached the store at all. +type keyProbeStore struct { + fakeStore + requested [][]state.Key +} + +func (s *keyProbeStore) GetMany(ctx context.Context, ks []state.Key) ([]int64, []bool, error) { + s.requested = append(s.requested, ks) + return s.fakeStore.GetMany(ctx, ks) +} + +func (s *keyProbeStore) keysRequested() int { + n := 0 + for _, ks := range s.requested { + n += len(ks) + } + return n +} + +func TestGetRange_RejectsDegenerateBounds(t *testing.T) { + w := windowed.Daily(30 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + + cases := []struct { + name string + start, end time.Time + }{ + {"swapped", now, now.Add(-24 * time.Hour)}, + // pkg/query/typed sends a zero time.Time as Unix -62135596800, whose + // UnixNano wraps and lands on an arbitrary negative bucket. + {"zero start time", time.Time{}, now}, + // The proto3 "max timestamp" an operator reaches for when they mean + // "forever"; UnixNano wraps here too. + {"year 9999 end", now.Add(-24 * time.Hour), time.Unix(253402300799, 0).UTC()}, + {"both ends unrepresentable", time.Time{}, time.Unix(253402300799, 0).UTC()}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + store := &keyProbeStore{fakeStore: fakeStore{}} + got, err := query.GetRange(context.Background(), store, core.Sum[int64](), w, "page-A", tc.start, tc.end) + if err == nil { + t.Fatalf("GetRange(%s, %s): got (%d, nil), want an error", + tc.start.Format(time.RFC3339), tc.end.Format(time.RFC3339), got) + } + if !errors.Is(err, query.ErrInvalidQuery) { + t.Errorf("error %v does not match ErrInvalidQuery", err) + } + if n := store.keysRequested(); n != 0 { + t.Errorf("rejected range still read %d keys from the store", n) + } + }) + } +} + +func TestGetRange_AcceptsOrdinaryBounds(t *testing.T) { + w := windowed.Daily(30 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + store := fakeStore{ + state.Key{Entity: "page-A", Bucket: w.BucketID(now)}: 5, + } + got, err := query.GetRange(context.Background(), store, core.Sum[int64](), w, "page-A", + now.Add(-48*time.Hour), now) + if err != nil { + t.Fatalf("GetRange: %v", err) + } + if got != 5 { + t.Errorf("GetRange: got %d, want 5", got) + } +} + +func TestGetWindow_RejectsNonPositiveDuration(t *testing.T) { + w := windowed.Daily(30 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + + for _, d := range []time.Duration{0, -time.Second, -30 * 24 * time.Hour} { + store := &keyProbeStore{fakeStore: fakeStore{}} + got, err := query.GetWindow(context.Background(), store, core.Sum[int64](), w, "page-A", d, now) + if err == nil { + t.Fatalf("GetWindow(duration=%s): got (%d, nil), want an error", d, got) + } + if !errors.Is(err, query.ErrInvalidQuery) { + t.Errorf("GetWindow(duration=%s): error %v does not match ErrInvalidQuery", d, err) + } + if n := store.keysRequested(); n != 0 { + t.Errorf("GetWindow(duration=%s): rejected window still read %d keys", d, n) + } + } +} + +func TestGetWindow_RejectsDurationBeyondRetention(t *testing.T) { + // Retention keeps 7 daily buckets. A 30-day window used to read 30 buckets, + // find 23 of them TTL-evicted, fold the gaps in as Identity, and hand back + // the 7-day total labelled as a 30-day answer. + w := windowed.Daily(7 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + store := &keyProbeStore{fakeStore: fakeStore{}} + for i := 0; i < 7; i++ { + store.fakeStore[state.Key{Entity: "page-A", Bucket: w.BucketID(now.Add(-time.Duration(i) * 24 * time.Hour))}] = 1 + } + + if _, err := query.GetWindow(context.Background(), store, core.Sum[int64](), w, "page-A", 30*24*time.Hour, now); err == nil { + t.Fatal("GetWindow(30d) against a 7d retention: got nil error, want a rejection") + } else if !errors.Is(err, query.ErrInvalidQuery) { + t.Errorf("error %v does not match ErrInvalidQuery", err) + } + + // The longest window retention can actually answer stays allowed. + got, err := query.GetWindow(context.Background(), store, core.Sum[int64](), w, "page-A", 7*24*time.Hour, now) + if err != nil { + t.Fatalf("GetWindow(7d): %v", err) + } + if got != 7 { + t.Errorf("GetWindow(7d): got %d, want 7", got) + } +} + +func TestGetRange_RejectsBucketSpanBeyondCap(t *testing.T) { + // Minute granularity with an hour of retention: 60 live buckets. A range + // reaching two days back used to build 2880 keys, of which at most 60 could + // ever hold data. At the shipped Minute(24h) preset the same shape reaches + // 29,797,201 keys for a range starting at the epoch. + w := windowed.Minute(time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + store := &keyProbeStore{fakeStore: fakeStore{}} + + _, err := query.GetRange(context.Background(), store, core.Sum[int64](), w, "page-A", + now.Add(-48*time.Hour), now) + if err == nil { + t.Fatal("GetRange over 2880 minute buckets with a 60-bucket cap: got nil error, want a rejection") + } + if !errors.Is(err, query.ErrInvalidQuery) { + t.Errorf("error %v does not match ErrInvalidQuery", err) + } + if n := store.keysRequested(); n != 0 { + t.Errorf("rejected range still fanned out over %d keys", n) + } + + // A range inside the cap is still served. + if _, err := query.GetRange(context.Background(), store, core.Sum[int64](), w, "page-A", + now.Add(-30*time.Minute), now); err != nil { + t.Errorf("GetRange over 31 minute buckets: %v", err) + } +} + +func TestGetRangeMany_RejectsBucketSpanPerEntity(t *testing.T) { + // The Many path multiplies the bucket span by the entity count, so the cap + // matters most here: 2880 buckets × 3 entities = 8640 keys in one request. + w := windowed.Minute(time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + store := &keyProbeStore{fakeStore: fakeStore{}} + entities := []string{"a", "b", "c"} + + vals, err := query.GetRangeMany(context.Background(), store, core.Sum[int64](), w, entities, + now.Add(-48*time.Hour), now) + if err == nil { + t.Fatal("GetRangeMany over 2880 minute buckets with a 60-bucket cap: got nil error, want a rejection") + } + if !errors.Is(err, query.ErrInvalidQuery) { + t.Errorf("error %v does not match ErrInvalidQuery", err) + } + if n := store.keysRequested(); n != 0 { + t.Errorf("rejected range still fanned out over %d keys", n) + } + // Callers that ignore the error must still get an indexable slice. + if len(vals) != len(entities) { + t.Errorf("rejected GetRangeMany returned %d values, want %d", len(vals), len(entities)) + } +} + +func TestGetWindowMany_RejectsDurationBeyondRetention(t *testing.T) { + w := windowed.Daily(7 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + store := &keyProbeStore{fakeStore: fakeStore{}} + entities := []string{"a", "b"} + + vals, err := query.GetWindowMany(context.Background(), store, core.Sum[int64](), w, entities, 90*24*time.Hour, now) + if err == nil { + t.Fatal("GetWindowMany(90d) against a 7d retention: got nil error, want a rejection") + } + if !errors.Is(err, query.ErrInvalidQuery) { + t.Errorf("error %v does not match ErrInvalidQuery", err) + } + if len(vals) != len(entities) { + t.Errorf("rejected GetWindowMany returned %d values, want %d", len(vals), len(entities)) + } + if n := store.keysRequested(); n != 0 { + t.Errorf("rejected window still read %d keys", n) + } +} + +func TestConfigMaxBuckets_OverridesRetentionDefault(t *testing.T) { + w := windowed.Daily(365 * 24 * time.Hour) + w.MaxBuckets = 7 + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + store := &keyProbeStore{fakeStore: fakeStore{}} + + if _, err := query.GetWindow(context.Background(), store, core.Sum[int64](), w, "page-A", 30*24*time.Hour, now); err == nil { + t.Fatal("GetWindow(30d) with MaxBuckets=7: got nil error, want a rejection") + } + if _, err := query.GetWindow(context.Background(), store, core.Sum[int64](), w, "page-A", 7*24*time.Hour, now); err != nil { + t.Errorf("GetWindow(7d) with MaxBuckets=7: %v", err) + } +} diff --git a/pkg/query/warmup.go b/pkg/query/warmup.go index 562e233..e575abd 100644 --- a/pkg/query/warmup.go +++ b/pkg/query/warmup.go @@ -26,8 +26,9 @@ import ( // few hundred milliseconds against typical DDB capacity and is a // straightforward step in the service-start path. // -// Returns the count of successfully warmed entries (entities × buckets -// that the store had data for) so callers can log meaningful progress. +// Returns the count of successfully warmed entries (distinct entities × +// buckets that the store had data for) so callers can log meaningful +// progress. Repeated entities are collapsed before the fetch. func WarmupWindowed[V any]( ctx context.Context, cache state.Cache[V], @@ -46,10 +47,16 @@ func WarmupWindowed[V any]( if len(entities) == 0 { return 0, nil } - lo, hi := w.LastN(now, duration) + lo, hi, err := windowBuckets(w, duration, now) + if err != nil { + return 0, fmt.Errorf("query.WarmupWindowed: %w", err) + } if hi < lo { return 0, nil } + // A repeated entity would put the same (entity, bucket) key in the list twice, + // which BatchGetItem rejects outright, and would double-count it in `warmed`. + entities = dedupeStrings(entities) bucketCount := int(hi - lo + 1) keys := make([]state.Key, 0, bucketCount*len(entities)) for _, e := range entities { @@ -98,6 +105,9 @@ func WarmupNonWindowed[V any]( if len(entities) == 0 { return 0, nil } + // Same reason as WarmupWindowed: duplicates are illegal in a BatchGetItem key + // list and inflate the warmed count. + entities = dedupeStrings(entities) keys := make([]state.Key, len(entities)) for i, e := range entities { keys[i] = state.Key{Entity: e} @@ -117,3 +127,17 @@ func WarmupNonWindowed[V any]( } return warmed, nil } + +// dedupeStrings returns entities with repeats removed, preserving first-seen order. +func dedupeStrings(entities []string) []string { + out := make([]string, 0, len(entities)) + seen := make(map[string]struct{}, len(entities)) + for _, e := range entities { + if _, dup := seen[e]; dup { + continue + } + seen[e] = struct{}{} + out = append(out, e) + } + return out +} diff --git a/pkg/query/window.go b/pkg/query/window.go index 3e55189..812ce2c 100644 --- a/pkg/query/window.go +++ b/pkg/query/window.go @@ -49,7 +49,10 @@ func GetWindow[V any]( duration time.Duration, now time.Time, ) (V, error) { - lo, hi := w.LastN(now, duration) + lo, hi, err := windowBuckets(w, duration, now) + if err != nil { + return m.Identity(), err + } return getRangeBuckets(ctx, store, m, entity, lo, hi) } @@ -63,7 +66,10 @@ func GetRange[V any]( entity string, start, end time.Time, ) (V, error) { - lo, hi := w.BucketRange(start, end) + lo, hi, err := rangeBuckets(w, start, end) + if err != nil { + return m.Identity(), err + } return getRangeBuckets(ctx, store, m, entity, lo, hi) } @@ -89,7 +95,10 @@ func GetWindowMany[V any]( duration time.Duration, now time.Time, ) ([]V, error) { - lo, hi := w.LastN(now, duration) + lo, hi, err := windowBuckets(w, duration, now) + if err != nil { + return identityFill(m, len(entities)), err + } return getRangeBucketsMany(ctx, store, m, entities, lo, hi) } @@ -102,7 +111,10 @@ func GetRangeMany[V any]( entities []string, start, end time.Time, ) ([]V, error) { - lo, hi := w.BucketRange(start, end) + lo, hi, err := rangeBuckets(w, start, end) + if err != nil { + return identityFill(m, len(entities)), err + } return getRangeBucketsMany(ctx, store, m, entities, lo, hi) } @@ -113,13 +125,10 @@ func getRangeBucketsMany[V any]( entities []string, lo, hi int64, ) ([]V, error) { - out := make([]V, len(entities)) if hi < lo || len(entities) == 0 { - for i := range out { - out[i] = m.Identity() - } - return out, nil + return identityFill(m, len(entities)), nil } + out := make([]V, len(entities)) bucketCount := int(hi - lo + 1) totalKeys := bucketCount * len(entities) @@ -136,10 +145,7 @@ func getRangeBucketsMany[V any]( vals, _, err := store.GetMany(ctx, keys) if err != nil { // Identity-fill on error so callers don't see a partial slice. - for i := range out { - out[i] = m.Identity() - } - return out, err + return identityFill(m, len(entities)), err } for i := range entities { @@ -150,6 +156,17 @@ func getRangeBucketsMany[V any]( return out, nil } +// identityFill returns a full-length slice of monoid identities. Every "Many" error +// path hands one back so a caller that ignores the error still gets a slice it can +// index by entity position rather than a short or nil one. +func identityFill[V any](m monoid.Monoid[V], n int) []V { + out := make([]V, n) + for i := range out { + out[i] = m.Identity() + } + return out +} + func getRangeBuckets[V any]( ctx context.Context, store state.Store[V], diff --git a/pkg/state/dynamodb/bytesstore.go b/pkg/state/dynamodb/bytesstore.go index 90e6392..96fe755 100644 --- a/pkg/state/dynamodb/bytesstore.go +++ b/pkg/state/dynamodb/bytesstore.go @@ -182,13 +182,18 @@ func (s *BytesStore) GetMany(ctx context.Context, ks []state.Key) ([][]byte, []b } byPair := make(map[pair][]byte, len(ks)) + // See Int64SumStore.GetMany: BatchGetItem rejects a duplicated key outright, + // so the request has to carry each (entity, bucket) once even when the caller + // asked for it twice. + uniq := dedupeKeys(ks) + const maxPerCall = 100 - for offset := 0; offset < len(ks); offset += maxPerCall { + for offset := 0; offset < len(uniq); offset += maxPerCall { end := offset + maxPerCall - if end > len(ks) { - end = len(ks) + if end > len(uniq) { + end = len(uniq) } - chunk := ks[offset:end] + chunk := uniq[offset:end] keys := make([]map[string]types.AttributeValue, len(chunk)) for i, k := range chunk { keys[i] = keyAttr(k) diff --git a/pkg/state/dynamodb/store.go b/pkg/state/dynamodb/store.go index 8b8f327..1549bd4 100644 --- a/pkg/state/dynamodb/store.go +++ b/pkg/state/dynamodb/store.go @@ -114,16 +114,23 @@ func (s *Int64SumStore) GetMany(ctx context.Context, ks []state.Key) ([]int64, [ } byPair := make(map[pair]int64, len(ks)) + // DynamoDB REJECTS a BatchGetItem whose key list repeats a key ("Provided list + // of item keys contains duplicates") and fails the whole request, so the same + // entity appearing twice in a candidate list took the entire RPC down with it. + // Whether that even reproduced depended on the two copies landing in the same + // 100-key chunk, which made it look like a size-dependent flake. + uniq := dedupeKeys(ks) + // Build the initial RequestItems; loop on UnprocessedKeys with bounded // exponential backoff. DDB caps BatchGetItem at 100 keys per request — if the // caller hands us more, chunk before issuing. const maxPerCall = 100 - for offset := 0; offset < len(ks); offset += maxPerCall { + for offset := 0; offset < len(uniq); offset += maxPerCall { end := offset + maxPerCall - if end > len(ks) { - end = len(ks) + if end > len(uniq) { + end = len(uniq) } - chunk := ks[offset:end] + chunk := uniq[offset:end] keys := make([]map[string]types.AttributeValue, len(chunk)) for i, k := range chunk { keys[i] = keyAttr(k) @@ -230,6 +237,25 @@ func CreateInt64Table(ctx context.Context, client *dynamodb.Client, table string return err } +// dedupeKeys returns ks with repeated (entity, bucket) pairs removed, preserving +// first-seen order. BatchGetItem rejects a request whose key list repeats a key, +// and a repeated entity in a batched read is ordinary caller input — a rerank +// candidate list that names the same item twice, a windowed fan-out over an entity +// list with a duplicate. Read results are scattered back through the (entity, +// bucket) map, so every copy of a duplicated key still gets its value. +func dedupeKeys(ks []state.Key) []state.Key { + uniq := make([]state.Key, 0, len(ks)) + seen := make(map[state.Key]struct{}, len(ks)) + for _, k := range ks { + if _, dup := seen[k]; dup { + continue + } + seen[k] = struct{}{} + uniq = append(uniq, k) + } + return uniq +} + func keyAttr(k state.Key) map[string]types.AttributeValue { return map[string]types.AttributeValue{ attrPK: &types.AttributeValueMemberS{Value: k.Entity}, diff --git a/pkg/state/dynamodb/store_test.go b/pkg/state/dynamodb/store_test.go index 6b59113..22c5875 100644 --- a/pkg/state/dynamodb/store_test.go +++ b/pkg/state/dynamodb/store_test.go @@ -3,6 +3,7 @@ package dynamodb_test import ( "bytes" "context" + "encoding/base64" "encoding/json" "fmt" "io" @@ -13,6 +14,8 @@ import ( "testing" "time" + "github.com/gallowaysoftware/murmur/pkg/monoid/sketch/hll" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/aws/retry" awsconfig "github.com/aws/aws-sdk-go-v2/config" @@ -494,3 +497,153 @@ func TestInt64SumStore_GetMany_EmptyInputNoCalls(t *testing.T) { t.Fatalf("BatchGetItem calls: got %d, want 0", got) } } + +// duplicateKey reports the first (pk, sk) pair that appears twice in a single +// BatchGetItem request, across all tables in it. +func duplicateKey(req ddbReq) (string, bool) { + for table, ka := range req.RequestItems { + seen := make(map[string]struct{}, len(ka.Keys)) + for _, k := range ka.Keys { + id := table + "/" + k["pk"]["S"] + "/" + k["sk"]["N"] + if _, dup := seen[id]; dup { + return id, true + } + seen[id] = struct{}{} + } + } + return "", false +} + +// validationException builds the 400 the DynamoDB API returns for a malformed +// request, in the JSON 1.0 shape the SDK decodes. +func validationException(message string) *http.Response { + body, _ := json.Marshal(map[string]string{ + "__type": "com.amazon.coral.validate#ValidationException", + "message": message, + }) + return &http.Response{ + StatusCode: 400, + Header: http.Header{"Content-Type": []string{"application/x-amz-json-1.0"}}, + Body: io.NopCloser(bytes.NewReader(body)), + } +} + +// echoHandler answers a BatchGetItem with every requested key, valued from the +// lookup map. Keys absent from the map are omitted from the response, which is +// how DynamoDB reports a miss. +func echoHandler(values map[string]int64) func(int, ddbReq) ddbResp { + return func(_ int, req ddbReq) ddbResp { + items := make([]map[string]map[string]string, 0, len(req.RequestItems["t"].Keys)) + for _, k := range req.RequestItems["t"].Keys { + entity := k["pk"]["S"] + bucket, _ := strconv.ParseInt(k["sk"]["N"], 10, 64) + v, ok := values[entity] + if !ok { + continue + } + items = append(items, ddbItem(entity, bucket, v)) + } + return ddbResp{Responses: map[string][]map[string]map[string]string{"t": items}} + } +} + +// TestInt64SumStore_GetManyDeduplicatesKeys pins the duplicate handling. +// DynamoDB rejects a BatchGetItem whose key list repeats a key and fails the +// whole request, so a batched read over a candidate list naming the same entity +// twice took the entire RPC down with it. +func TestInt64SumStore_GetManyDeduplicatesKeys(t *testing.T) { + ft := &fakeTransport{handle: echoHandler(map[string]int64{"a": 10, "b": 20})} + store := newFakeStore(t, ft) + + keys := []state.Key{ + {Entity: "a", Bucket: 0}, + {Entity: "b", Bucket: 0}, + {Entity: "a", Bucket: 0}, // same entity twice, same chunk + {Entity: "missing", Bucket: 0}, + {Entity: "b", Bucket: 0}, + } + vals, oks, err := store.GetMany(context.Background(), keys) + if err != nil { + t.Fatalf("GetMany with duplicate keys: %v", err) + } + want := []int64{10, 20, 10, 0, 20} + wantOK := []bool{true, true, true, false, true} + for i := range keys { + if vals[i] != want[i] || oks[i] != wantOK[i] { + t.Errorf("result[%d] (%s): got (%d,%v), want (%d,%v)", + i, keys[i].Entity, vals[i], oks[i], want[i], wantOK[i]) + } + } +} + +// TestInt64SumStore_GetManyDeduplicatesAcrossChunks covers the chunk-boundary +// dependence directly: 100 distinct keys followed by a repeat of the first one +// puts the duplicate in a different 100-key chunk, which is why the failure was +// intermittent on candidate-set size rather than reliable. +func TestInt64SumStore_GetManyDeduplicatesAcrossChunks(t *testing.T) { + keys := makeKeys(100) + values := make(map[string]int64, len(keys)) + for i, k := range keys { + values[k.Entity] = int64(i + 1) + } + keys = append(keys, keys[0]) + + ft := &fakeTransport{handle: echoHandler(values)} + store := newFakeStore(t, ft) + + vals, oks, err := store.GetMany(context.Background(), keys) + if err != nil { + t.Fatalf("GetMany with a cross-chunk duplicate: %v", err) + } + // 101 keys collapse to 100 distinct ones — a single BatchGetItem, not two. + if got := ft.batchGetCalls.Load(); got != 1 { + t.Errorf("BatchGetItem calls: got %d, want 1", got) + } + if !oks[0] || vals[0] != 1 { + t.Errorf("result[0]: got (%d,%v), want (1,true)", vals[0], oks[0]) + } + if !oks[100] || vals[100] != 1 { + t.Errorf("duplicate at result[100]: got (%d,%v), want (1,true)", vals[100], oks[100]) + } +} + +// TestBytesStore_GetManyDeduplicatesKeys is the sketch-state counterpart — +// BytesStore carries its own copy of the batch loop. +func TestBytesStore_GetManyDeduplicatesKeys(t *testing.T) { + ft := &fakeTransport{ + handle: func(_ int, req ddbReq) ddbResp { + items := make([]map[string]map[string]string, 0) + for _, k := range req.RequestItems["t"].Keys { + entity := k["pk"]["S"] + if entity != "a" { + continue + } + items = append(items, map[string]map[string]string{ + "pk": {"S": entity}, + "sk": {"N": k["sk"]["N"]}, + "v": {"B": base64.StdEncoding.EncodeToString([]byte("sketch"))}, + }) + } + return ddbResp{Responses: map[string][]map[string]map[string]string{"t": items}} + }, + } + store := dynamodb.NewBytesStore(newFakeClient(t, ft, 1), "t", hll.HLL()) + + keys := []state.Key{ + {Entity: "a", Bucket: 7}, + {Entity: "a", Bucket: 7}, + {Entity: "b", Bucket: 7}, + } + vals, oks, err := store.GetMany(context.Background(), keys) + if err != nil { + t.Fatalf("GetMany with duplicate keys: %v", err) + } + for _, i := range []int{0, 1} { + if !oks[i] || string(vals[i]) != "sketch" { + t.Errorf("result[%d]: got (%q,%v), want (\"sketch\", true)", i, vals[i], oks[i]) + } + } + if oks[2] { + t.Errorf("result[2] (b): reported present, want absent") + } +} diff --git a/proto/gen/murmur/v1/murmurv1connect/query.connect.go b/proto/gen/murmur/v1/murmurv1connect/query.connect.go index 63ec21b..661173d 100644 --- a/proto/gen/murmur/v1/murmurv1connect/query.connect.go +++ b/proto/gen/murmur/v1/murmurv1connect/query.connect.go @@ -58,16 +58,27 @@ const ( // QueryServiceClient is a client for the murmur.v1.QueryService service. type QueryServiceClient interface { // Get returns the all-time aggregation value for entity (non-windowed - // pipelines). + // pipelines). On a WINDOWED pipeline this returns FAILED_PRECONDITION: + // it addresses bucket 0, which is the all-time sentinel and which a + // windowed pipeline never writes, so it could only ever answer "absent". + // Use GetWindow there. fresh_read does not bypass that check. Get(context.Context, *connect.Request[v1.GetRequest]) (*connect.Response[v1.GetResponse], error) // GetWindow returns the aggregation merged across the bucket range // covering the most recent `duration_seconds`, ending at the server's "now". + // Returns INVALID_ARGUMENT when duration_seconds is not positive or when + // the window reaches further back than the pipeline's retention keeps + // buckets — a longer window can only fold in TTL-evicted holes as the + // monoid identity and report the result as a full window. GetWindow(context.Context, *connect.Request[v1.GetWindowRequest]) (*connect.Response[v1.GetWindowResponse], error) // GetRange returns the aggregation merged across the bucket range - // covering [start_unix, end_unix]. + // covering [start_unix, end_unix]. Returns INVALID_ARGUMENT when the + // bounds are unset (proto3 zero), reversed, outside the representable + // bucket range, or span more buckets than the pipeline's cap allows. GetRange(context.Context, *connect.Request[v1.GetRangeRequest]) (*connect.Response[v1.GetRangeResponse], error) // GetMany batches Get calls. Response order matches request order so - // callers can zip without an index map. + // callers can zip without an index map — the response ALWAYS carries + // exactly one value per requested entity. Same windowed-pipeline + // precondition as Get. GetMany(context.Context, *connect.Request[v1.GetManyRequest]) (*connect.Response[v1.GetManyResponse], error) // GetWindowMany batches GetWindow calls across many entities in a single // round-trip. ONE underlying store fetch over (N entities × M buckets) @@ -206,16 +217,27 @@ func (c *queryServiceClient) GetTrailingMany(ctx context.Context, req *connect.R // QueryServiceHandler is an implementation of the murmur.v1.QueryService service. type QueryServiceHandler interface { // Get returns the all-time aggregation value for entity (non-windowed - // pipelines). + // pipelines). On a WINDOWED pipeline this returns FAILED_PRECONDITION: + // it addresses bucket 0, which is the all-time sentinel and which a + // windowed pipeline never writes, so it could only ever answer "absent". + // Use GetWindow there. fresh_read does not bypass that check. Get(context.Context, *connect.Request[v1.GetRequest]) (*connect.Response[v1.GetResponse], error) // GetWindow returns the aggregation merged across the bucket range // covering the most recent `duration_seconds`, ending at the server's "now". + // Returns INVALID_ARGUMENT when duration_seconds is not positive or when + // the window reaches further back than the pipeline's retention keeps + // buckets — a longer window can only fold in TTL-evicted holes as the + // monoid identity and report the result as a full window. GetWindow(context.Context, *connect.Request[v1.GetWindowRequest]) (*connect.Response[v1.GetWindowResponse], error) // GetRange returns the aggregation merged across the bucket range - // covering [start_unix, end_unix]. + // covering [start_unix, end_unix]. Returns INVALID_ARGUMENT when the + // bounds are unset (proto3 zero), reversed, outside the representable + // bucket range, or span more buckets than the pipeline's cap allows. GetRange(context.Context, *connect.Request[v1.GetRangeRequest]) (*connect.Response[v1.GetRangeResponse], error) // GetMany batches Get calls. Response order matches request order so - // callers can zip without an index map. + // callers can zip without an index map — the response ALWAYS carries + // exactly one value per requested entity. Same windowed-pipeline + // precondition as Get. GetMany(context.Context, *connect.Request[v1.GetManyRequest]) (*connect.Response[v1.GetManyResponse], error) // GetWindowMany batches GetWindow calls across many entities in a single // round-trip. ONE underlying store fetch over (N entities × M buckets) diff --git a/proto/gen/murmur/v1/query_grpc.pb.go b/proto/gen/murmur/v1/query_grpc.pb.go index 6455696..f53f28c 100644 --- a/proto/gen/murmur/v1/query_grpc.pb.go +++ b/proto/gen/murmur/v1/query_grpc.pb.go @@ -51,16 +51,27 @@ const ( // others. type QueryServiceClient interface { // Get returns the all-time aggregation value for entity (non-windowed - // pipelines). + // pipelines). On a WINDOWED pipeline this returns FAILED_PRECONDITION: + // it addresses bucket 0, which is the all-time sentinel and which a + // windowed pipeline never writes, so it could only ever answer "absent". + // Use GetWindow there. fresh_read does not bypass that check. Get(ctx context.Context, in *GetRequest, opts ...grpc.CallOption) (*GetResponse, error) // GetWindow returns the aggregation merged across the bucket range // covering the most recent `duration_seconds`, ending at the server's "now". + // Returns INVALID_ARGUMENT when duration_seconds is not positive or when + // the window reaches further back than the pipeline's retention keeps + // buckets — a longer window can only fold in TTL-evicted holes as the + // monoid identity and report the result as a full window. GetWindow(ctx context.Context, in *GetWindowRequest, opts ...grpc.CallOption) (*GetWindowResponse, error) // GetRange returns the aggregation merged across the bucket range - // covering [start_unix, end_unix]. + // covering [start_unix, end_unix]. Returns INVALID_ARGUMENT when the + // bounds are unset (proto3 zero), reversed, outside the representable + // bucket range, or span more buckets than the pipeline's cap allows. GetRange(ctx context.Context, in *GetRangeRequest, opts ...grpc.CallOption) (*GetRangeResponse, error) // GetMany batches Get calls. Response order matches request order so - // callers can zip without an index map. + // callers can zip without an index map — the response ALWAYS carries + // exactly one value per requested entity. Same windowed-pipeline + // precondition as Get. GetMany(ctx context.Context, in *GetManyRequest, opts ...grpc.CallOption) (*GetManyResponse, error) // GetWindowMany batches GetWindow calls across many entities in a single // round-trip. ONE underlying store fetch over (N entities × M buckets) @@ -192,16 +203,27 @@ func (c *queryServiceClient) GetTrailingMany(ctx context.Context, in *GetTrailin // others. type QueryServiceServer interface { // Get returns the all-time aggregation value for entity (non-windowed - // pipelines). + // pipelines). On a WINDOWED pipeline this returns FAILED_PRECONDITION: + // it addresses bucket 0, which is the all-time sentinel and which a + // windowed pipeline never writes, so it could only ever answer "absent". + // Use GetWindow there. fresh_read does not bypass that check. Get(context.Context, *GetRequest) (*GetResponse, error) // GetWindow returns the aggregation merged across the bucket range // covering the most recent `duration_seconds`, ending at the server's "now". + // Returns INVALID_ARGUMENT when duration_seconds is not positive or when + // the window reaches further back than the pipeline's retention keeps + // buckets — a longer window can only fold in TTL-evicted holes as the + // monoid identity and report the result as a full window. GetWindow(context.Context, *GetWindowRequest) (*GetWindowResponse, error) // GetRange returns the aggregation merged across the bucket range - // covering [start_unix, end_unix]. + // covering [start_unix, end_unix]. Returns INVALID_ARGUMENT when the + // bounds are unset (proto3 zero), reversed, outside the representable + // bucket range, or span more buckets than the pipeline's cap allows. GetRange(context.Context, *GetRangeRequest) (*GetRangeResponse, error) // GetMany batches Get calls. Response order matches request order so - // callers can zip without an index map. + // callers can zip without an index map — the response ALWAYS carries + // exactly one value per requested entity. Same windowed-pipeline + // precondition as Get. GetMany(context.Context, *GetManyRequest) (*GetManyResponse, error) // GetWindowMany batches GetWindow calls across many entities in a single // round-trip. ONE underlying store fetch over (N entities × M buckets) diff --git a/proto/murmur/v1/query.proto b/proto/murmur/v1/query.proto index 5a70ae4..60f4993 100644 --- a/proto/murmur/v1/query.proto +++ b/proto/murmur/v1/query.proto @@ -22,19 +22,30 @@ package murmur.v1; // others. service QueryService { // Get returns the all-time aggregation value for entity (non-windowed - // pipelines). + // pipelines). On a WINDOWED pipeline this returns FAILED_PRECONDITION: + // it addresses bucket 0, which is the all-time sentinel and which a + // windowed pipeline never writes, so it could only ever answer "absent". + // Use GetWindow there. fresh_read does not bypass that check. rpc Get(GetRequest) returns (GetResponse); // GetWindow returns the aggregation merged across the bucket range // covering the most recent `duration_seconds`, ending at the server's "now". + // Returns INVALID_ARGUMENT when duration_seconds is not positive or when + // the window reaches further back than the pipeline's retention keeps + // buckets — a longer window can only fold in TTL-evicted holes as the + // monoid identity and report the result as a full window. rpc GetWindow(GetWindowRequest) returns (GetWindowResponse); // GetRange returns the aggregation merged across the bucket range - // covering [start_unix, end_unix]. + // covering [start_unix, end_unix]. Returns INVALID_ARGUMENT when the + // bounds are unset (proto3 zero), reversed, outside the representable + // bucket range, or span more buckets than the pipeline's cap allows. rpc GetRange(GetRangeRequest) returns (GetRangeResponse); // GetMany batches Get calls. Response order matches request order so - // callers can zip without an index map. + // callers can zip without an index map — the response ALWAYS carries + // exactly one value per requested entity. Same windowed-pipeline + // precondition as Get. rpc GetMany(GetManyRequest) returns (GetManyResponse); // GetWindowMany batches GetWindow calls across many entities in a single diff --git a/test/integration/page_view_counters_test.go b/test/integration/page_view_counters_test.go index 45c886b..212c849 100644 --- a/test/integration/page_view_counters_test.go +++ b/test/integration/page_view_counters_test.go @@ -283,35 +283,63 @@ func TestDeployed_PageViewCounters_QueryBootsAgainstDDB(t *testing.T) { host, _ := query.Host(ctx) port, _ := query.MappedPort(ctx, "50051/tcp") - url := fmt.Sprintf("http://%s:%s/murmur.v1.QueryService/Get", host, port.Port()) - + baseURL := fmt.Sprintf("http://%s:%s", host, port.Port()) httpc := &http.Client{Timeout: 5 * time.Second} - req, _ := http.NewRequestWithContext(ctx, http.MethodPost, url, - strings.NewReader(`{"entity":"page-never-seen"}`)) - req.Header.Set("Content-Type", "application/json") - resp, err := httpc.Do(req) - if err != nil { - t.Fatalf("query request: %v", err) + post := func(method, payload string) (int, []byte) { + t.Helper() + req, _ := http.NewRequestWithContext(ctx, http.MethodPost, + baseURL+"/murmur.v1.QueryService/"+method, strings.NewReader(payload)) + req.Header.Set("Content-Type", "application/json") + resp, err := httpc.Do(req) + if err != nil { + t.Fatalf("%s request: %v", method, err) + } + defer func() { _ = resp.Body.Close() }() + body, _ := io.ReadAll(resp.Body) + return resp.StatusCode, body } - defer func() { _ = resp.Body.Close() }() - body, _ := io.ReadAll(resp.Body) - if resp.StatusCode != http.StatusOK { - t.Fatalf("query response: status=%d body=%s", resp.StatusCode, body) + + // This pipeline is windowed (Daily, 90d retention), so Get is the wrong RPC + // for it: bucket 0 is the all-time sentinel and no windowed write ever lands + // there. It used to answer present=false, which this test then "confirmed" — + // an assertion that would have held just as well for an entity with a + // million views. The server now says so out loud. + status, body := post("Get", `{"entity":"page-never-seen"}`) + if status != http.StatusPreconditionFailed { + t.Fatalf("Get on a windowed pipeline: status=%d body=%s, want 412", status, body) + } + var connErr struct { + Code string `json:"code"` + Message string `json:"message"` + } + if err := json.Unmarshal(body, &connErr); err != nil { + t.Fatalf("Get error decode: %v (body: %s)", err, body) + } + if connErr.Code != "failed_precondition" { + t.Errorf("Get error code: got %q, want %q (body: %s)", connErr.Code, "failed_precondition", body) + } + + // GetWindow is the RPC the precondition points at, and it answers for an + // absent entity with the merged-empty value rather than an error. + status, body = post("GetWindow", `{"entity":"page-never-seen","duration_seconds":86400}`) + if status != http.StatusOK { + t.Fatalf("GetWindow: status=%d body=%s", status, body) } - // Absent-entity shape: { "value": { "present": false } } var env struct { Value struct { - Present bool `json:"present"` + Data []byte `json:"data"` } `json:"value"` } if err := json.Unmarshal(body, &env); err != nil { - t.Fatalf("query response decode: %v (body: %s)", err, body) + t.Fatalf("GetWindow response decode: %v (body: %s)", err, body) } - if env.Value.Present { - t.Errorf("absent entity reported present: %s", body) + if len(env.Value.Data) >= 8 { + if got := int64(binary.LittleEndian.Uint64(env.Value.Data)); got != 0 { + t.Errorf("GetWindow for an absent entity: got %d, want 0", got) + } } - t.Logf("query container served Connect-RPC Get against DDB-local; absent-entity round-trip clean") + t.Logf("query container served Connect-RPC against DDB-local; Get routed to GetWindow, absent-entity window clean") } // dumpContainerLogs prints a tail of the named container's combined From a72c381e1a2642dc5f27085a9050a1f42d061bf9 Mon Sep 17 00:00:00 2001 From: Kyle Galloway Date: Fri, 28 Aug 2026 11:43:21 -0300 Subject: [PATCH 2/5] Stop the fan-out cap rejecting the exact-retention range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S --- STABILITY.md | 6 +- doc/design.md | 12 +- pkg/monoid/windowed/windowed.go | 41 +++++- pkg/monoid/windowed/windowed_test.go | 75 ++++++++++ pkg/query/grpc/server.go | 59 +++++--- pkg/query/grpc/validation_test.go | 203 ++++++++++++++++++++++---- pkg/query/validate.go | 45 +++++- pkg/query/validate_test.go | 211 +++++++++++++++++++++++++-- 8 files changed, 571 insertions(+), 81 deletions(-) create mode 100644 pkg/monoid/windowed/windowed_test.go diff --git a/STABILITY.md b/STABILITY.md index 53464f3..c9a7334 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -13,7 +13,7 @@ edges callers should plan around. | `pkg/monoid/core` | mostly stable | `Min` / `Max` use `Bounded[V]` for a proper Identity; lift inputs via `core.NewBounded(v)`. `Monotonic[V](identity)` is the raw-V counterpart that pairs with conditional-update stores like `pkg/state/dynamodb.Int64MaxStore` for the SetCountIfGreater pattern (out-of-order absolute-value safety) | | `pkg/monoid/sketch/{hll,topk,bloom}` | experimental | On a decode error `Combine` returns the operand that decoded and discards the other — unavoidable while `Monoid.Combine` has no error return, but now reportable via `WithDecodeErrorHandler`; wire it to a `metrics.Recorder` or the loss stays invisible. For `bloom` the same hook now reports **shape problems**: two filters whose (m, k) differ cannot be OR'd (`Combine` keeps the left operand), and operands that agree with each other but not with the (n, p) passed to `NewWithCapacity` are merged but reported — without that report those parameters were decorative, since every filter carries its own shape on the wire. `bloom.Bloom` / `bloom.NewWithCapacity` Identity is now the empty slice, so it is an identity for an operand of ANY shape. **`topk` counts are Misra-Gries lower bounds and the wire format does not record the stream size**, so a summary that retained 29 of 45,932 events is indistinguishable from an exact answer over 29; carry `n` out of band to size the `n/(K+1)` error bar. Making that visible in the header is a staged wire-format change that has not landed. Cross-runtime encoding portability not yet proven | | `pkg/monoid/compose` | mostly stable | `MapMerge` / `Tuple2` / `DecayedSum`; FP-associativity caveats apply to `DecayedSum`. `DecayedSum` / `DecayedSumBytes` take one experimental option, `WithDecodeErrorHandler` (bytes only — a non-17-byte operand is now an error, not a value assembled from someone else's blob), and `DecodeDecayed` returns `(Decayed, error)`. A non-positive half-life means "no decay" in `Combine` as well as `EvaluateAt`, and `EvaluateAt` before the reference time returns the stored value rather than scaling it up. `Combine` is a pure function of its operands — it reads no clock, because `BytesStore.MergeUpdate` recomputes it on every CAS retry. A future-dated observation therefore still freezes a key, and the guard is `compose.ClampFuture(t, now, compose.DefaultSkewBound(halfLife))` applied at the **lift**, which is the last point where a clock reading is honest; `murmur.Trending` stamps at its own clock and is not exposed | -| `pkg/monoid/windowed` | mostly stable | bucket math is solid; minute-granularity has high read-amplification on long ranges | +| `pkg/monoid/windowed` | mostly stable | bucket math is solid; minute-granularity has high read-amplification on long ranges. `MaxBucketSpan()` counts both ends of an inclusive range, so the default derived from `Retention` is `ceil(Retention/Granularity) + 1` — a read over exactly the retention window fits. A `Config` with a `Granularity` but neither `MaxBuckets` nor `Retention` falls back to `DefaultMaxBucketSpan` (100,000) rather than going uncapped | | `pkg/state` (interfaces) | mostly stable | `Store` / `Cache` interfaces unlikely to change before v1. `state.NewInstrumented` / `state.NewInstrumentedCache` decorate any store/cache with metrics.Recorder hooks (store_get / store_get_many / store_merge_update / cache_get / cache_repopulate latencies + errors) | | `pkg/state/dynamodb` | mostly stable | `BatchGetItem` retries `UnprocessedKeys` with chunking + jittered backoff; CAS path retries CCF with the same backoff policy, tunable per store via `WithCASRetries` / `WithCASBackoff` and counted under `:cas_conflict` when `WithCASMetrics` is wired. `BytesStore` pre-flights DynamoDB's 400KB item limit and returns `ErrItemTooLarge` / `*ItemTooLargeError` — sketch size tracks key length, not just K, and an oversized row otherwise fails every write while `Get` keeps serving the last value that fit. `Deduper` claim keys are `"#"` (`NewDeduper` takes the pipeline name; `ForPipeline` derives a sibling scope) and carry a per-call `claimant` token so a claim whose response was lost is not mistaken for a peer's. `Int64MaxStore` ships the SetCountIfGreater pattern via DDB `UpdateItem` with conditional expression — out-of-order events with lower values are silently dropped | | `pkg/state/valkey` | mostly stable | `Int64Cache` (atomic INCRBY) + `BytesCache` (RMW with caller-supplied byte-monoid; works with HLL/TopK/Bloom/DecayedSumBytes) + `HLLCache` (Valkey-native PFADD/PFCOUNT/PFMERGE accelerator) + `BloomCache` (Valkey-native BF.ADD/BF.MADD/BF.EXISTS/BF.MEXISTS accelerator; requires the valkey-bloom or RedisBloom module loaded into the server). The sketch accelerators run side-by-side with the BytesStore-authoritative sketches — independent estimators, both within the monoid's error bound. No portable axiomhq↔HYLL or bits-and-blooms↔valkey-bloom byte conversion: on Valkey loss the accelerators can only be repopulated by re-feeding events | @@ -34,8 +34,8 @@ edges callers should plan around. | `pkg/exec/lambda/kinesis` | experimental | `NewHandler` returns the Lambda Kinesis handler signature; partial-batch failures via BatchItemFailures; pair with `WithDedup` so adjacent-redelivered records fold idempotently | | `pkg/exec/lambda/dynamodbstreams` | experimental | DDB Streams Lambda handler; same retry/dedup/BatchItemFailures shape as the Kinesis variant. Decoder takes the whole change record so callers can branch on EventName / inspect OldImage. BatchItemFailures report the record's SequenceNumber (what Lambda checkpoints on); the eventID feeds dedup only. A failed record with an empty SequenceNumber gets no entry at all (an empty ItemIdentifier makes Lambda redeliver the whole batch) and is surfaced via metrics.RecordError plus a `:unreportable_failure` event | | `pkg/exec/lambda/sqs` | experimental | SQS Lambda handler; same shape as kinesis/dynamodbstreams. Default EventID is "/"; override via WithEventID for FIFO content-dedup or upstream-key dedup. Uses SQS SentTimestamp for windowed-bucket assignment so delayed deliveries land in the correct bucket | -| `pkg/query` | mostly stable | `Get` / `GetWindow` / `GetRange` / `LambdaQuery` are likely v1 surface. Windowed reads validate the request before fanning out: non-positive durations, swapped / unset / unrepresentable range bounds, windows longer than `Retention`, and bucket spans past `windowed.Config.MaxBuckets` all return an error matching `query.ErrInvalidQuery` instead of a monoid Identity that looks like a real zero | -| `pkg/query/grpc` | mostly stable | `Get` / `GetMany` return `FAILED_PRECONDITION` on a windowed pipeline (they read bucket 0, which windowed writes never populate) — route to `GetWindow` / `GetWindowMany`; `fresh_read` does not bypass it. Malformed windows and ranges return `INVALID_ARGUMENT`. Singleflight coalescing keys on a length-prefixed, order-preserving entity encoding and runs the shared call under `Config.CoalesceTimeout`, detached from the leading caller's context. generic byte-encoded responses; `cmd/murmur-codegen-typed` emits per-service typed `.proto` + Go server stubs (sum / hll / topk / bloom; get_all_time / get_window / get_window_many / get_many / get_range) over `pkg/query/typed` clients. `HealthHandler` serves `grpc.health.v1.Health` and `HealthzHandler` serves `/healthz` (liveness, always 200) + `/readyz` (readiness, store round-trip, cached so probe traffic is not billed reads) | +| `pkg/query` | mostly stable | `Get` / `GetWindow` / `GetRange` / `LambdaQuery` are likely v1 surface. Windowed reads validate the request before fanning out: non-positive durations, swapped / unset / unrepresentable range bounds, windows longer than `Retention`, and bucket spans past `windowed.Config.MaxBuckets` all return an error matching `query.ErrInvalidQuery` instead of a monoid Identity that looks like a real zero. Absolute ranges (`GetRange` / `GetRangeMany` / `LambdaQuery.GetRange`) are held to `Retention` too, so raising `MaxBuckets` above it does not buy reads past TTL — but the bound is on a range's WIDTH, not its age: a within-retention-width range sitting entirely in the past is still answered from whatever buckets survive | +| `pkg/query/grpc` | mostly stable | `Get` / `GetMany` return `FAILED_PRECONDITION` on a windowed pipeline (they read bucket 0, which windowed writes never populate) — route to `GetWindow` / `GetWindowMany`; `fresh_read` does not bypass it. Malformed windows and ranges return `INVALID_ARGUMENT`. Singleflight coalescing keys on a length-prefixed, order-preserving entity encoding. The shared call is detached from the leading caller's CANCELLATION but keeps its DEADLINE, capped by `Config.CoalesceTimeout` — one client hanging up no longer fails its coalesced peers, and abandoned work still expires when the caller's own deadline would have. generic byte-encoded responses; `cmd/murmur-codegen-typed` emits per-service typed `.proto` + Go server stubs (sum / hll / topk / bloom; get_all_time / get_window / get_window_many / get_many / get_range) over `pkg/query/typed` clients. `HealthHandler` serves `grpc.health.v1.Health` and `HealthzHandler` serves `/healthz` (liveness, always 200) + `/readyz` (readiness, store round-trip, cached so probe traffic is not billed reads) | | `pkg/query/typed` | mostly stable | typed-client wrappers over the generic QueryService — `SumClient`, `HLLClient`, `TopKClient`, `BloomClient`. All four expose `Get` / `GetMany` / `GetWindow` / `GetWindowMany` / `GetRange`. `GetMany` returns parallel value + present arrays so callers can distinguish "absent" from "present-and-empty"; `GetWindowMany` can't (the generic RPC merges before returning). Every batched client errors when the response's value count doesn't match the requested entity count — the wire contract is positional, and the old handling either panicked in the caller or silently truncated. The decoders + typed shape behind application-service typed-wrapper RPCs (see `examples/typed-wrapper`). Building block under `cmd/murmur-codegen-typed` | | `pkg/admin` | mostly stable | CORS is closed by default; opt in via `WithAllowedOrigins`. Bearer-token (`WithAuthToken`, constant-time, multi-token for rotation) and JWT (`WithJWTVerifier`, BYO verifier) auth via a single middleware; auth is off by default — same-origin / network-isolated deploys keep the historical behavior. The `cmd/murmur-ui` binary exposes `--auth-token` + `MURMUR_ADMIN_TOKEN` env fallback | | `pkg/swap` | mostly stable | small surface; integrated into `deploy/terraform/modules/pipeline-counter` via opt-in `swap_enabled` (control table + IAM + seed + `SWAP_CONTROL_TABLE` / `SWAP_ALIAS` env vars in every task definition) | diff --git a/doc/design.md b/doc/design.md index f51be8e..48f28d3 100644 --- a/doc/design.md +++ b/doc/design.md @@ -1968,10 +1968,16 @@ to catch permutations made `["a","b"]` and `["b","a"]` one group, so the second caller received the first caller's values attributed to the wrong entities. Permutation coalescing is deliberately given up. -The shared call runs on a context detached from whichever caller led the -group, under a server-side `CoalesceTimeout`, and each waiter selects on +The shared call runs on a context derived from whichever caller led the +group, with that caller's **cancellation dropped** but its **deadline +kept**, capped by a server-side `CoalesceTimeout`; each waiter selects on its own context. A client hanging up therefore leaves without taking its -coalesced peers down with it, and a wedged store still frees the group. +coalesced peers down with it. Keeping the deadline matters as much as +dropping the cancellation: with both gone, a burst of abandoned requests +would each hold a full `CoalesceTimeout` of fan-out open with nobody left +to read it, where before coalescing a hangup shed that work at once. The +`CoalesceTimeout` ceiling then covers the remaining case — a leader with +no deadline at all, against a wedged store. Concurrent requests for the same key resolve through one underlying fold. A thousand simultaneous feed renders asking for the same hot diff --git a/pkg/monoid/windowed/windowed.go b/pkg/monoid/windowed/windowed.go index 62d0215..9c1fd58 100644 --- a/pkg/monoid/windowed/windowed.go +++ b/pkg/monoid/windowed/windowed.go @@ -25,8 +25,9 @@ type Config struct { // can ask for any range up to Retention. Retention time.Duration - // MaxBuckets caps how many buckets a single read may span. Leave it zero to take - // the default from Retention (see MaxBucketSpan). + // MaxBuckets caps how many buckets a single read may touch, counting both ends of + // an inclusive range. Leave it zero to take the default from Retention (see + // MaxBucketSpan). // // Without a cap, a caller who passes an open-ended range gets no error — just an // enormous key list. GetRange(entity, Unix(0,0), now) against Minute granularity @@ -36,22 +37,46 @@ type Config struct { MaxBuckets int } -// MaxBucketSpan reports the largest number of buckets one read may span. It is -// MaxBuckets when set, otherwise ceil(Retention / Granularity) — reading further back -// than Retention can only return TTL-evicted buckets. Zero means unbounded, which -// happens only when neither MaxBuckets nor Retention is configured. +// DefaultMaxBucketSpan caps the fan-out of a Config that sets a Granularity but +// neither MaxBuckets nor Retention. That shape used to get no cap at all, so a +// hand-built Config{Granularity: time.Minute} still let GetRange(epoch, now) build +// ~30 million keys before the first store call. +// +// It is a backstop against unbounded, not a tuning knob. 100,000 buckets sits far +// above any deliberate query — seven days at Minute granularity is 10,080 buckets, a +// year at Hourly is 8,760 — while holding the key slice to a couple of megabytes +// instead of hundreds. A pipeline that genuinely wants to read more sets MaxBuckets +// and says so. +const DefaultMaxBucketSpan = 100_000 + +// MaxBucketSpan reports the largest number of buckets one read may touch, counting +// both ends of an inclusive range. +// +// MaxBuckets wins when set: it is a bucket count the operator wrote down literally. +// Otherwise the cap is derived from Retention as ceil(Retention/Granularity) + 1, and +// that +1 is load-bearing. Buckets are tumbling and BucketRange is inclusive at both +// ends, so an absolute [t, t+Retention] touches Retention/Granularity + 1 of them. +// Deriving ceil(Retention/Granularity) rejected a read over exactly the retention +// window — the most natural range a caller writes. +// +// With neither field set the cap is DefaultMaxBucketSpan. Zero — genuinely unbounded — +// comes back only for a Config with no Granularity, which assigns everything to bucket +// 0 and so cannot fan out at all. func (c Config) MaxBucketSpan() int64 { if c.MaxBuckets > 0 { return int64(c.MaxBuckets) } - if c.Granularity <= 0 || c.Retention <= 0 { + if c.Granularity <= 0 { return 0 } + if c.Retention <= 0 { + return DefaultMaxBucketSpan + } n := int64(c.Retention / c.Granularity) if c.Retention%c.Granularity != 0 { n++ } - return n + return n + 1 } // RetentionBuckets reports how many whole buckets fit inside Retention — the longest diff --git a/pkg/monoid/windowed/windowed_test.go b/pkg/monoid/windowed/windowed_test.go new file mode 100644 index 0000000..047e304 --- /dev/null +++ b/pkg/monoid/windowed/windowed_test.go @@ -0,0 +1,75 @@ +package windowed_test + +import ( + "testing" + "time" + + "github.com/gallowaysoftware/murmur/pkg/monoid/windowed" +) + +// TestConfig_MaxBucketSpanCoversExactRetention pins the arithmetic against the shape +// it exists to admit. BucketRange is inclusive at both ends, so an absolute range of +// exactly Retention touches Retention/Granularity + 1 buckets; a cap derived as +// ceil(Retention/Granularity) was one short of that and turned the exact-retention +// read into an InvalidArgument. +func TestConfig_MaxBucketSpanCoversExactRetention(t *testing.T) { + cases := []struct { + name string + cfg windowed.Config + want int64 + }{ + {"daily, 30d retention", windowed.Daily(30 * 24 * time.Hour), 31}, + {"hourly, 7d retention", windowed.Hourly(7 * 24 * time.Hour), 169}, + {"minute, 1h retention", windowed.Minute(time.Hour), 61}, + // Retention that is not a whole number of buckets rounds up, then still + // gets the inclusive-endpoint bucket. + {"daily, 90h retention", windowed.Daily(90 * time.Hour), 5}, + // An explicit MaxBuckets is a count the operator wrote down; it is used + // verbatim, no +1. + {"explicit MaxBuckets wins", windowed.Config{ + Granularity: 24 * time.Hour, Retention: 365 * 24 * time.Hour, MaxBuckets: 7, + }, 7}, + // No Granularity means everything lands in bucket 0, so there is nothing + // to fan out over and nothing to cap. + {"no granularity is unbounded", windowed.Config{Retention: 30 * 24 * time.Hour}, 0}, + // The reason DefaultMaxBucketSpan exists: this Config used to report 0. + {"granularity without retention", windowed.Config{Granularity: time.Minute}, + windowed.DefaultMaxBucketSpan}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := tc.cfg.MaxBucketSpan(); got != tc.want { + t.Errorf("MaxBucketSpan() = %d, want %d", got, tc.want) + } + }) + } +} + +// TestConfig_BucketRangeOverRetentionIsWithinCap is the property the constant in +// MaxBucketSpan has to satisfy: whatever the alignment of the range, a read covering +// exactly Retention must fit inside the cap, and a read one bucket longer must not. +func TestConfig_BucketRangeOverRetentionIsWithinCap(t *testing.T) { + for _, cfg := range []windowed.Config{ + windowed.Daily(30 * 24 * time.Hour), + windowed.Hourly(7 * 24 * time.Hour), + windowed.Minute(time.Hour), + } { + // Offsets deliberately unaligned to the bucket grid — the natural query + // starts at whatever instant the caller happens to hold. + for _, offset := range []time.Duration{0, 1, 37 * time.Second, 12*time.Hour + 3*time.Minute} { + start := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC).Add(offset) + + lo, hi := cfg.BucketRange(start, start.Add(cfg.Retention)) + if touched := hi - lo + 1; touched > cfg.MaxBucketSpan() { + t.Errorf("%s granularity, offset %s: a range of exactly the %s retention touches %d buckets, over the %d cap", + cfg.Granularity, offset, cfg.Retention, touched, cfg.MaxBucketSpan()) + } + + lo, hi = cfg.BucketRange(start, start.Add(cfg.Retention+cfg.Granularity)) + if touched := hi - lo + 1; touched <= cfg.MaxBucketSpan() { + t.Errorf("%s granularity, offset %s: a range one bucket past the %s retention touches %d buckets, still inside the %d cap", + cfg.Granularity, offset, cfg.Retention, touched, cfg.MaxBucketSpan()) + } + } + } +} diff --git a/pkg/query/grpc/server.go b/pkg/query/grpc/server.go index e7130ec..caebe86 100644 --- a/pkg/query/grpc/server.go +++ b/pkg/query/grpc/server.go @@ -114,18 +114,19 @@ type Config[V any] struct { // process serves multiple pipelines. Pipeline string - // CoalesceTimeout bounds the store call a singleflight group runs on behalf - // of its waiters. That call is deliberately detached from the context of - // whichever caller happened to lead the group — otherwise one client hanging - // up cancels the read out from under every peer coalesced onto it — so it - // needs a deadline of its own or a wedged store pins the group forever. - // Defaults to defaultCoalesceTimeout. + // CoalesceTimeout is the CEILING on the store call a singleflight group runs on + // behalf of its waiters. That call keeps the leading caller's deadline but not + // its cancellation — otherwise one client hanging up cancels the read out from + // under every peer coalesced onto it — and CoalesceTimeout bounds the case where + // the leader had no deadline at all, so a wedged store cannot pin the group + // forever. Defaults to defaultCoalesceTimeout. CoalesceTimeout time.Duration } -// defaultCoalesceTimeout bounds detached singleflight work when Config leaves -// CoalesceTimeout unset. Long enough for a multi-chunk BatchGetItem with retries, -// short enough that a wedged store frees the group inside one health-check interval. +// defaultCoalesceTimeout bounds detached singleflight work whose leading caller set no +// deadline of its own, when Config leaves CoalesceTimeout unset. Long enough for a +// multi-chunk BatchGetItem with retries, short enough that a wedged store frees the +// group inside one health-check interval. const defaultCoalesceTimeout = 10 * time.Second // NewServer constructs a query Server. @@ -168,13 +169,12 @@ type coalescedResult[V any] struct { // coalesce runs fn at most once per concurrent group keyed by `key`; every other // caller in the group awaits the same result. // -// Two things the plain singleflight.Do version got wrong. The shared work ran on -// the context of whichever caller happened to arrive first, so a single client -// hanging up cancelled the store read out from under every peer and failed all of -// them with CodeInternal — a failure that only appears under exactly the concurrent -// load coalescing exists to serve. And detaching that context outright would drop -// the client deadline with it, letting a wedged store call hold the group open -// indefinitely, so the detached work carries a server-side bound instead. +// The plain singleflight.Do version ran the shared work on the context of whichever +// caller happened to arrive first, so a single client hanging up cancelled the store +// read out from under every peer and failed all of them with CodeInternal — a failure +// that only appears under exactly the concurrent load coalescing exists to serve. The +// shared work now runs on a context detached from the leader's cancellation but still +// carrying its deadline; see detachedWork. // // Each waiter selects on its OWN context, so a caller that goes away leaves without // disturbing the group. @@ -187,7 +187,7 @@ func coalesce[R any]( ) (R, error) { var zero R ch := sf.DoChan(key, func() (any, error) { - workCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), timeout) + workCtx, cancel := detachedWork(ctx, timeout) defer cancel() v, err := fn(workCtx) if err != nil { @@ -210,6 +210,31 @@ func coalesce[R any]( } } +// detachedWork derives the context a coalesced store call runs on from the context of +// the caller that led the group. +// +// It drops that caller's CANCELLATION and keeps its DEADLINE, capped by timeout. +// Dropping both — context.WithoutCancel plus a flat CoalesceTimeout — traded one load +// problem for another: before coalescing existed, a client hanging up shed the store +// work with it, and a burst of abandoned requests would instead have kept a full +// CoalesceTimeout of fan-out alive per group with nobody left to read it. The cap is +// still needed on its own, because a leader with no deadline at all would otherwise +// pin its group on a wedged store forever. +// +// A waiter whose own deadline is longer than the leader's is bounded by the leader's: +// the group runs one call, and it can only carry one deadline. That is the standing +// bargain of coalescing — the alternative is not sharing the call at all. In the corner +// where a leader arrives with a deadline that has already passed, its group gets a +// context that is already expired and its waiters see DeadlineExceeded rather than a +// result; they are free to retry, and the next request leads a fresh group. +func detachedWork(ctx context.Context, timeout time.Duration) (context.Context, context.CancelFunc) { + bound := time.Now().Add(timeout) + if dl, ok := ctx.Deadline(); ok && dl.Before(bound) { + bound = dl + } + return context.WithDeadline(context.WithoutCancel(ctx), bound) +} + // fail maps err onto a Connect status and records it against the pipeline — // except for the request-shaped rejections, which are the caller's mistake. // Counting a malformed range as a pipeline error is the same conflation that diff --git a/pkg/query/grpc/validation_test.go b/pkg/query/grpc/validation_test.go index add3c99..32087a9 100644 --- a/pkg/query/grpc/validation_test.go +++ b/pkg/query/grpc/validation_test.go @@ -200,13 +200,18 @@ func TestQueryServer_GetWindowRejectsInvalidDuration(t *testing.T) { // singleflight group open and observe what the other callers in it experience. It // honors the context it is handed — that is the whole point: a leader-scoped // context used to cancel this call for everyone. +// +// It also records the deadline each call's context carried, which is how the +// deadline-propagation test reads the answer off the context instead of waiting for +// a timer to fire. type blockingStore struct { values fakeStore release chan struct{} arrived chan []state.Key - mu sync.Mutex - calls int + mu sync.Mutex + calls int + deadlines []time.Time } func newBlockingStore(values fakeStore) *blockingStore { @@ -217,6 +222,17 @@ func newBlockingStore(values fakeStore) *blockingStore { } } +// deadlineAt returns the deadline the i'th store call's context carried, zero if it +// had none. +func (s *blockingStore) deadlineAt(i int) time.Time { + s.mu.Lock() + defer s.mu.Unlock() + if i >= len(s.deadlines) { + return time.Time{} + } + return s.deadlines[i] +} + func (s *blockingStore) Get(ctx context.Context, k state.Key) (int64, bool, error) { vs, oks, err := s.GetMany(ctx, []state.Key{k}) if err != nil { @@ -226,14 +242,24 @@ func (s *blockingStore) Get(ctx context.Context, k state.Key) (int64, bool, erro } func (s *blockingStore) GetMany(ctx context.Context, ks []state.Key) ([]int64, []bool, error) { + deadline, _ := ctx.Deadline() s.mu.Lock() s.calls++ + s.deadlines = append(s.deadlines, deadline) s.mu.Unlock() s.arrived <- ks select { case <-s.release: case <-ctx.Done(): - return nil, nil, ctx.Err() + } + // Cancellation is re-checked here rather than decided by the select above. With + // both channels ready select picks at random, and that coin flip is precisely what + // let the cancellation test below pass 4 runs in 20 against the unfixed server: + // half the time the release won the race and the store returned a value even + // though its context was already dead. Whether the coalesced call still holds a + // live context when its turn finally comes is the whole question, so answer it. + if err := ctx.Err(); err != nil { + return nil, nil, err } return s.values.GetMany(ctx, ks) } @@ -345,63 +371,176 @@ func TestQueryServer_CoalesceKeepsEntityListsDistinct(t *testing.T) { } } +// joinContext reports the moment its holder becomes a singleflight waiter. +// +// coalesce evaluates ctx.Done() only in the select it runs AFTER sf.DoChan returns, +// and DoChan has already appended the caller to the in-flight call's channel list by +// the time it returns. A signal here is therefore proof that the peer is parked on +// the leader's group — where a sleep was only ever a guess that it had got there. +type joinContext struct { + context.Context + once sync.Once + joined chan struct{} +} + +func newJoinContext(parent context.Context) *joinContext { + return &joinContext{Context: parent, joined: make(chan struct{})} +} + +func (c *joinContext) Done() <-chan struct{} { + c.once.Do(func() { close(c.joined) }) + return c.Context.Done() +} + // TestQueryServer_CoalescedPeerSurvivesCallerCancellation covers the other half of // the singleflight bug: the shared store call ran on the leader's context, so one // client hanging up failed everybody coalesced behind it with CodeInternal. +// +// Every step is gated on an explicit channel rather than a sleep, because the sleeping +// version of this test only failed 16 runs in 20 against the unfixed server and a +// regression test that passes a fifth of the time is not one. Two gates carry it: the +// leader is provably inside the store call before the cancel, and the peer is provably +// a waiter on the leader's group before it. The server is driven in-process rather than +// over HTTP so the cancellation reaches the store through nothing but context plumbing +// — the HTTP path stays covered by the coalesce-key test above. func TestQueryServer_CoalescedPeerSurvivesCallerCancellation(t *testing.T) { w := windowed.Daily(30 * 24 * time.Hour) now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) store := newBlockingStore(fakeStore{ state.Key{Entity: "page-A", Bucket: w.BucketID(now)}: 42, }) - client, cleanup := startServer(t, mgrpc.Config[int64]{ + srv := mgrpc.NewServer(mgrpc.Config[int64]{ Store: store, Monoid: core.Sum[int64](), Window: &w, Encode: mgrpc.Int64LE(), Now: func() time.Time { return now }, }) - defer cleanup() + req := func() *connect.Request[pb.GetWindowRequest] { + return connect.NewRequest(&pb.GetWindowRequest{Entity: "page-A", DurationSeconds: 86400}) + } leaderCtx, cancelLeader := context.WithCancel(context.Background()) - leaderDone := make(chan struct{}) + defer cancelLeader() + leaderErr := make(chan error, 1) go func() { - defer close(leaderDone) - _, _ = client.GetWindow(leaderCtx, connect.NewRequest(&pb.GetWindowRequest{ - Entity: "page-A", DurationSeconds: 86400, - })) + _, err := srv.GetWindow(leaderCtx, req()) + leaderErr <- err }() - // The leader is now parked inside the store with its group open. + // Gate 1: the leader is inside the store call, so its group is open. <-store.arrived - peerDone := make(chan error, 1) - peerValue := make(chan int64, 1) + peerCtx := newJoinContext(context.Background()) + type peerResult struct { + value int64 + err error + } + peerDone := make(chan peerResult, 1) go func() { - resp, err := client.GetWindow(context.Background(), connect.NewRequest(&pb.GetWindowRequest{ - Entity: "page-A", DurationSeconds: 86400, - })) + resp, err := srv.GetWindow(peerCtx, req()) if err != nil { - peerDone <- err + peerDone <- peerResult{err: err} return } - peerValue <- decodeInt64(resp.Msg.GetValue().GetData()) - peerDone <- nil + peerDone <- peerResult{value: decodeInt64(resp.Msg.GetValue().GetData())} }() - // Give the peer time to join the leader's group (or, if it forms its own, - // to reach the store) before the leader hangs up. - select { - case <-store.arrived: - case <-time.After(500 * time.Millisecond): - } + // Gate 2: the peer is a waiter on that group, not merely dispatched towards it. + <-peerCtx.joined cancelLeader() - <-leaderDone + if err := <-leaderErr; connect.CodeOf(err) != connect.CodeCanceled { + t.Fatalf("leader GetWindow after hanging up: got %v (code %v), want Canceled", + err, connect.CodeOf(err)) + } + // The leader is gone and its context is dead. The store call is still parked; + // whether it kept a live context of its own is what the peer now reports. close(store.release) - if err := <-peerDone; err != nil { - t.Fatalf("peer GetWindow failed after the leading caller cancelled: %v (code %v)", err, connect.CodeOf(err)) + got := <-peerDone + if got.err != nil { + t.Fatalf("peer GetWindow failed after the leading caller cancelled: %v (code %v)", + got.err, connect.CodeOf(got.err)) } - if got := <-peerValue; got != 42 { - t.Errorf("peer GetWindow: got %d, want 42", got) + if got.value != 42 { + t.Errorf("peer GetWindow: got %d, want 42", got.value) } - if store.callCount() == 0 { - t.Error("store was never called") + if n := store.callCount(); n != 1 { + t.Fatalf("store called %d times, want 1: the peer never coalesced onto the leader's group, so this run proved nothing", n) } } + +// TestQueryServer_CoalescedWorkKeepsCallerDeadline pins that detaching the shared store +// call from the leader's CANCELLATION did not also detach it from the leader's DEADLINE. +// +// context.WithoutCancel plus a flat CoalesceTimeout dropped both. Before coalescing +// existed, a client hanging up shed its store work immediately; with the deadline gone +// a burst of abandoned requests would instead keep a full CoalesceTimeout of fan-out +// alive per group with nobody left to read the answer. +// +// Both cases read the deadline straight off the context the store was handed, so +// neither waits for a timer. +func TestQueryServer_CoalescedWorkKeepsCallerDeadline(t *testing.T) { + w := windowed.Daily(30 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + // An hour of CoalesceTimeout, far longer than any deadline below, so what the + // assertions measure is which of the two bounds the store call inherited. + const coalesceTimeout = time.Hour + + newFixture := func(t *testing.T) (*mgrpc.Server[int64], *blockingStore) { + t.Helper() + store := newBlockingStore(fakeStore{ + state.Key{Entity: "page-A", Bucket: w.BucketID(now)}: 42, + }) + return mgrpc.NewServer(mgrpc.Config[int64]{ + Store: store, Monoid: core.Sum[int64](), Window: &w, Encode: mgrpc.Int64LE(), + Now: func() time.Time { return now }, CoalesceTimeout: coalesceTimeout, + }), store + } + req := connect.NewRequest(&pb.GetWindowRequest{Entity: "page-A", DurationSeconds: 86400}) + + t.Run("caller deadline is inherited", func(t *testing.T) { + srv, store := newFixture(t) + // Generous enough that it cannot expire mid-test; it is never waited out. + callerDeadline := time.Now().Add(30 * time.Second) + ctx, cancel := context.WithDeadline(context.Background(), callerDeadline) + defer cancel() + + done := make(chan struct{}) + go func() { + defer close(done) + _, _ = srv.GetWindow(ctx, req) + }() + <-store.arrived + close(store.release) + <-done + + got := store.deadlineAt(0) + if got.IsZero() { + t.Fatal("the coalesced store call ran with no deadline at all") + } + if got.After(callerDeadline) { + t.Errorf("coalesced store call carries a deadline %s past the caller's own; the caller's deadline was dropped for the flat %s CoalesceTimeout", + got.Sub(callerDeadline), coalesceTimeout) + } + }) + + t.Run("CoalesceTimeout bounds a caller with no deadline", func(t *testing.T) { + srv, store := newFixture(t) + before := time.Now() + + done := make(chan struct{}) + go func() { + defer close(done) + _, _ = srv.GetWindow(context.Background(), req) + }() + <-store.arrived + close(store.release) + <-done + + got := store.deadlineAt(0) + if got.IsZero() { + t.Fatal("a caller with no deadline left the coalesced store call unbounded; a wedged store would pin the group forever") + } + if got.Before(before) || got.After(before.Add(coalesceTimeout+time.Minute)) { + t.Errorf("coalesced store call deadline is %s from the call, want about the %s CoalesceTimeout", + got.Sub(before), coalesceTimeout) + } + }) +} diff --git a/pkg/query/validate.go b/pkg/query/validate.go index 9f82214..84cda1d 100644 --- a/pkg/query/validate.go +++ b/pkg/query/validate.go @@ -79,6 +79,19 @@ func rangeBuckets(w windowed.Config, start, end time.Time) (lo, hi int64, err er return 0, 0, invalidQuery("end %s precedes start %s", end.UTC().Format(time.RFC3339), start.UTC().Format(time.RFC3339)) } + // An absolute range answers to Retention just as a trailing window does. Only + // checkSpan ran here before, and its limit is MaxBuckets whenever MaxBuckets is + // set — so a Config with MaxBuckets raised above Retention read straight past TTL: + // GetRange over a year against a 7-day Retention fanned out over 366 buckets, found + // 359 of them evicted, folded the holes in as Identity, and returned one week's + // total labelled as a year. MaxBuckets is a cap, never a licence to outrun TTL. + // + // time.Time.Sub saturates instead of wrapping, so a range between the two extreme + // representable instants arrives here as the maximum Duration and is rejected + // rather than overflowing into a plausible-looking short one. + if err := checkRetention(w, end.Sub(start)); err != nil { + return 0, 0, err + } lo, hi = w.BucketRange(start, end) if err := checkSpan(w, lo, hi); err != nil { return 0, 0, err @@ -95,12 +108,21 @@ func checkRepresentable(name string, t time.Time) error { return nil } -// checkRetention rejects a window longer than the buckets still exist for. Retention -// was advisory on the read path: asking for 90 days against a 7-day Retention read 83 +// checkRetention rejects a read wider than the buckets still exist for. Retention was +// advisory on the read path: asking for 90 days against a 7-day Retention read 83 // TTL-evicted buckets, folded them in as Identity, and returned the result labelled as // a full 90-day window. A short window that happens to be missing buckets is normal -// and stays silent — this only catches the case where the bucket range itself reaches +// and stays silent — this only catches the case where the requested span itself reaches // past what TTL keeps. +// +// It bounds the WIDTH of a read, not its age. For a trailing window those are the same +// thing, because the window is anchored at now. For an absolute range they are not: +// GetRange(now-90d, now-83d) against a 7-day Retention is seven days wide and passes +// here even though every bucket in it is long evicted. Age is deliberately left +// unchecked. rangeBuckets has no `now` to measure against, and threading one in would +// break LambdaQuery.GetRange, whose View store holds history written by a bootstrap or +// replay job and outlives the streaming table's TTL by design — reading a year-old +// range out of a batch view is the feature, not the bug. func checkRetention(w windowed.Config, d time.Duration) error { limit := w.RetentionBuckets() if limit <= 0 { @@ -120,16 +142,25 @@ func checkRetention(w windowed.Config, d time.Duration) error { // checkSpan enforces windowed.Config.MaxBucketSpan on an already-computed bucket // range. This is the last line before the key slice is materialized, so it runs on // every path that fans a read out over buckets. +// +// [lo, hi] is inclusive at both ends, so it touches hi-lo+1 buckets. That count is +// what MaxBucketSpan caps, and the comparison is written as span >= limit rather than +// span+1 > limit so that a span of MaxInt64 cannot overflow the addition. func checkSpan(w windowed.Config, lo, hi int64) error { limit := w.MaxBucketSpan() if limit <= 0 { return nil } span := hi - lo - // A negative span here means hi-lo overflowed int64, which a nanosecond-scale - // Granularity makes reachable from two representable instants. - if span < 0 || span >= limit { - return invalidQuery("range spans more than %d buckets (max_buckets); narrow the range or raise MaxBuckets", limit) + // A negative span means hi-lo overflowed int64, which a nanosecond-scale + // Granularity makes reachable from two representable instants; MaxInt64 is the + // one non-negative span whose bucket count would overflow in turn. + if span < 0 || span == math.MaxInt64 { + return invalidQuery("range spans more buckets than int64 can count; narrow the range or coarsen Granularity") + } + if span >= limit { + return invalidQuery("range touches %d buckets of %s, more than the %d-bucket cap; narrow the range or raise MaxBuckets", + span+1, w.Granularity, limit) } return nil } diff --git a/pkg/query/validate_test.go b/pkg/query/validate_test.go index b44b5f1..6632445 100644 --- a/pkg/query/validate_test.go +++ b/pkg/query/validate_test.go @@ -3,6 +3,7 @@ package query_test import ( "context" "errors" + "strings" "testing" "time" @@ -129,23 +130,40 @@ func TestGetWindow_RejectsDurationBeyondRetention(t *testing.T) { } } +// spanCappedConfig is a Config whose binding read limit is MaxBucketSpan rather than +// Retention: a day of minute buckets kept, but no more than 60 of them read at once. +// +// The distinction is what makes the two tests below exercise checkSpan at all. A range +// wider than Retention is refused a step earlier now, so a config where Retention is +// the tighter of the two bounds can never reach the span check. +func spanCappedConfig() windowed.Config { + w := windowed.Minute(24 * time.Hour) + w.MaxBuckets = 60 + return w +} + func TestGetRange_RejectsBucketSpanBeyondCap(t *testing.T) { - // Minute granularity with an hour of retention: 60 live buckets. A range - // reaching two days back used to build 2880 keys, of which at most 60 could - // ever hold data. At the shipped Minute(24h) preset the same shape reaches - // 29,797,201 keys for a range starting at the epoch. - w := windowed.Minute(time.Hour) + // Two hours at minute granularity is 121 buckets, well inside the day of + // retention but twice the 60 the operator allowed per read. Uncapped, the same + // shape at the shipped Minute(24h) preset reaches 29,797,201 keys for a range + // starting at the epoch. + w := spanCappedConfig() now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) store := &keyProbeStore{fakeStore: fakeStore{}} _, err := query.GetRange(context.Background(), store, core.Sum[int64](), w, "page-A", - now.Add(-48*time.Hour), now) + now.Add(-2*time.Hour), now) if err == nil { - t.Fatal("GetRange over 2880 minute buckets with a 60-bucket cap: got nil error, want a rejection") + t.Fatal("GetRange over 121 minute buckets with a 60-bucket cap: got nil error, want a rejection") } if !errors.Is(err, query.ErrInvalidQuery) { t.Errorf("error %v does not match ErrInvalidQuery", err) } + // Naming the count and the cap pins that MaxBucketSpan is what refused this, not + // the retention check one step earlier. + if !strings.Contains(err.Error(), "121 buckets") || !strings.Contains(err.Error(), "60-bucket cap") { + t.Errorf("error %q does not report the bucket count against the cap", err) + } if n := store.keysRequested(); n != 0 { t.Errorf("rejected range still fanned out over %d keys", n) } @@ -159,20 +177,23 @@ func TestGetRange_RejectsBucketSpanBeyondCap(t *testing.T) { func TestGetRangeMany_RejectsBucketSpanPerEntity(t *testing.T) { // The Many path multiplies the bucket span by the entity count, so the cap - // matters most here: 2880 buckets × 3 entities = 8640 keys in one request. - w := windowed.Minute(time.Hour) + // matters most here: 121 buckets × 3 entities = 363 keys in one request. + w := spanCappedConfig() now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) store := &keyProbeStore{fakeStore: fakeStore{}} entities := []string{"a", "b", "c"} vals, err := query.GetRangeMany(context.Background(), store, core.Sum[int64](), w, entities, - now.Add(-48*time.Hour), now) + now.Add(-2*time.Hour), now) if err == nil { - t.Fatal("GetRangeMany over 2880 minute buckets with a 60-bucket cap: got nil error, want a rejection") + t.Fatal("GetRangeMany over 121 minute buckets with a 60-bucket cap: got nil error, want a rejection") } if !errors.Is(err, query.ErrInvalidQuery) { t.Errorf("error %v does not match ErrInvalidQuery", err) } + if !strings.Contains(err.Error(), "60-bucket cap") { + t.Errorf("error %q does not report the bucket cap", err) + } if n := store.keysRequested(); n != 0 { t.Errorf("rejected range still fanned out over %d keys", n) } @@ -203,6 +224,174 @@ func TestGetWindowMany_RejectsDurationBeyondRetention(t *testing.T) { } } +// TestGetRange_AcceptsExactlyRetention pins the boundary the fan-out cap has to get +// right. Buckets are tumbling and BucketRange is inclusive at both ends, so an absolute +// range of exactly Retention touches Retention/Granularity + 1 buckets. A cap derived as +// ceil(Retention/Granularity) was one short of that, so "give me the whole window I am +// allowed to ask for" — the most natural range a caller writes — came back +// InvalidArgument. +func TestGetRange_AcceptsExactlyRetention(t *testing.T) { + w := windowed.Daily(30 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + store := &keyProbeStore{fakeStore: fakeStore{}} + for i := 0; i <= 30; i++ { + store.fakeStore[state.Key{Entity: "page-A", Bucket: w.BucketID(now.Add(-time.Duration(i) * 24 * time.Hour))}] = 1 + } + + got, err := query.GetRange(context.Background(), store, core.Sum[int64](), w, "page-A", + now.Add(-30*24*time.Hour), now) + if err != nil { + t.Fatalf("GetRange over exactly the %s retention window: %v", w.Retention, err) + } + if got != 31 { + t.Errorf("GetRange over exactly the retention window: got %d, want 31", got) + } + if n := store.keysRequested(); n != 31 { + t.Errorf("GetRange over exactly the retention window read %d keys, want 31", n) + } + + // One bucket further back is genuinely past what TTL keeps, and is refused. + store = &keyProbeStore{fakeStore: fakeStore{}} + if _, err := query.GetRange(context.Background(), store, core.Sum[int64](), w, "page-A", + now.Add(-31*24*time.Hour), now); err == nil { + t.Fatal("GetRange one bucket past the retention window: got nil error, want a rejection") + } else if !errors.Is(err, query.ErrInvalidQuery) { + t.Errorf("error %v does not match ErrInvalidQuery", err) + } + if n := store.keysRequested(); n != 0 { + t.Errorf("rejected range still read %d keys", n) + } +} + +// TestGetRangeMany_AcceptsExactlyRetention is the fan-out counterpart: GetRangeMany +// shares rangeBuckets, so the same off-by-one rejected the exact-retention query for +// every entity in the batch at once. +func TestGetRangeMany_AcceptsExactlyRetention(t *testing.T) { + w := windowed.Hourly(7 * 24 * time.Hour) + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + entities := []string{"a", "b"} + store := &keyProbeStore{fakeStore: fakeStore{}} + for _, e := range entities { + store.fakeStore[state.Key{Entity: e, Bucket: w.BucketID(now)}] = 3 + } + + vals, err := query.GetRangeMany(context.Background(), store, core.Sum[int64](), w, entities, + now.Add(-7*24*time.Hour), now) + if err != nil { + t.Fatalf("GetRangeMany over exactly the %s retention window: %v", w.Retention, err) + } + for i, v := range vals { + if v != 3 { + t.Errorf("GetRangeMany[%d] = %d, want 3", i, v) + } + } + // 169 buckets per entity: 168 hours of retention plus the inclusive endpoint. + if n := store.keysRequested(); n != 169*len(entities) { + t.Errorf("GetRangeMany read %d keys, want %d", n, 169*len(entities)) + } + + if _, err := query.GetRangeMany(context.Background(), store, core.Sum[int64](), w, entities, + now.Add(-7*24*time.Hour-time.Hour), now); err == nil { + t.Error("GetRangeMany one bucket past the retention window: got nil error, want a rejection") + } +} + +// TestGetRange_CapsFanOutWithoutRetention covers the Config the cap used to miss +// entirely. MaxBucketSpan only reported a limit when Retention or MaxBuckets was set, +// so a hand-built Config{Granularity: time.Minute} got no cap at all and GetRange from +// the epoch still built ~30 million keys before the first store call. +func TestGetRange_CapsFanOutWithoutRetention(t *testing.T) { + w := windowed.Config{Granularity: time.Minute} // no Retention, no MaxBuckets + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + store := &keyProbeStore{fakeStore: fakeStore{}} + + _, err := query.GetRange(context.Background(), store, core.Sum[int64](), w, "page-A", + time.Unix(0, 0).UTC(), now) + if err == nil { + t.Fatal("GetRange(epoch, now) at minute granularity with no Retention: got nil error, want a rejection") + } + if !errors.Is(err, query.ErrInvalidQuery) { + t.Errorf("error %v does not match ErrInvalidQuery", err) + } + if n := store.keysRequested(); n != 0 { + t.Errorf("rejected range still fanned out over %d keys", n) + } + + // A read inside the default cap is still served: the cap is a backstop against + // unbounded, not a policy on how far back an unretained pipeline may look. + store = &keyProbeStore{fakeStore: fakeStore{ + state.Key{Entity: "page-A", Bucket: w.BucketID(now)}: 9, + }} + got, err := query.GetRange(context.Background(), store, core.Sum[int64](), w, "page-A", + now.Add(-24*time.Hour), now) + if err != nil { + t.Fatalf("GetRange over 1441 minute buckets, inside the %d-bucket default: %v", + windowed.DefaultMaxBucketSpan, err) + } + if got != 9 { + t.Errorf("GetRange: got %d, want 9", got) + } +} + +// TestGetRange_MaxBucketsCannotOutrunRetention pins that MaxBuckets is a cap and never +// a licence. Only checkSpan ran on the absolute-range path, and its limit is MaxBuckets +// whenever MaxBuckets is set — so raising MaxBuckets above Retention let GetRange read +// straight past TTL and report the holes as Identity. +func TestGetRange_MaxBucketsCannotOutrunRetention(t *testing.T) { + w := windowed.Daily(7 * 24 * time.Hour) + w.MaxBuckets = 400 + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + store := &keyProbeStore{fakeStore: fakeStore{}} + for i := 0; i < 7; i++ { + store.fakeStore[state.Key{Entity: "page-A", Bucket: w.BucketID(now.Add(-time.Duration(i) * 24 * time.Hour))}] = 1 + } + + // A year against a 7-day retention: 359 of the 366 buckets are long evicted, and + // the answer used to come back as 7 labelled as a year's worth. + got, err := query.GetRange(context.Background(), store, core.Sum[int64](), w, "page-A", + now.Add(-365*24*time.Hour), now) + if err == nil { + t.Fatalf("GetRange(365d) with Retention=7d, MaxBuckets=400: got (%d, nil), want a rejection", got) + } + if !errors.Is(err, query.ErrInvalidQuery) { + t.Errorf("error %v does not match ErrInvalidQuery", err) + } + if n := store.keysRequested(); n != 0 { + t.Errorf("rejected range still fanned out over %d keys", n) + } + + // What retention can actually answer is unaffected. + if _, err := query.GetRange(context.Background(), store, core.Sum[int64](), w, "page-A", + now.Add(-7*24*time.Hour), now); err != nil { + t.Errorf("GetRange over exactly the retention window with MaxBuckets=400: %v", err) + } +} + +// TestLambdaQueryGetRange_MaxBucketsCannotOutrunRetention covers the third caller of +// rangeBuckets. LambdaQuery fans the same key list out over TWO stores, so an +// unretained range costs double. +func TestLambdaQueryGetRange_MaxBucketsCannotOutrunRetention(t *testing.T) { + w := windowed.Daily(7 * 24 * time.Hour) + w.MaxBuckets = 400 + now := time.Date(2026, 5, 7, 12, 0, 0, 0, time.UTC) + view := &keyProbeStore{fakeStore: fakeStore{}} + delta := &keyProbeStore{fakeStore: fakeStore{}} + q := query.LambdaQuery[int64]{View: view, Delta: delta, Monoid: core.Sum[int64]()} + + if _, err := q.GetRange(context.Background(), w, "page-A", now.Add(-365*24*time.Hour), now); err == nil { + t.Fatal("LambdaQuery.GetRange(365d) with Retention=7d, MaxBuckets=400: got nil error, want a rejection") + } else if !errors.Is(err, query.ErrInvalidQuery) { + t.Errorf("error %v does not match ErrInvalidQuery", err) + } + if n := view.keysRequested() + delta.keysRequested(); n != 0 { + t.Errorf("rejected range still fanned out over %d keys across the two stores", n) + } + + if _, err := q.GetRange(context.Background(), w, "page-A", now.Add(-7*24*time.Hour), now); err != nil { + t.Errorf("LambdaQuery.GetRange over exactly the retention window: %v", err) + } +} + func TestConfigMaxBuckets_OverridesRetentionDefault(t *testing.T) { w := windowed.Daily(365 * 24 * time.Hour) w.MaxBuckets = 7 From c4234d1a32764f331c331c1b70d27f113bb18839 Mon Sep 17 00:00:00 2001 From: Kyle Galloway Date: Fri, 28 Aug 2026 11:46:12 -0300 Subject: [PATCH 3/5] CHANGELOG for this branch 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) Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S --- CHANGELOG.md | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9c9fc1..3a70ac2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -262,6 +262,61 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - `examples/recently-interacted-topk/multisource_test.go` drives the example's real `Build()` instead of a hand-rolled copy that had drifted to `K=10` against the deployment's `K=32`, and asserts the built sketch's K against `Config.ResolveK()`. - The replay dedup-TTL contract moved from a unit test asserting against its own hand-rolled expiring fake to `test/e2e/replay_dedup_ttl_test.go`, which exercises the real `pkg/state/dynamodb.Deduper` behind the `DDB_LOCAL_ENDPOINT` gate. +- **Query layer: `Get` / `GetMany` on a windowed pipeline could never return data.** Both RPCs address bucket 0, which is simultaneously the all-time sentinel and the epoch bucket, so on a windowed pipeline they reported `present: false` no matter how much the pipeline had counted. Four shipped runbooks pointed operators at them. They now return `FAILED_PRECONDITION` naming `GetWindow` / `GetWindowMany`; `fresh_read` does not bypass the check. A `Granularity` of zero still legitimately writes bucket 0 and stays allowed. +- **Query layer: degenerate time bounds returned a fabricated `present: true` zero.** `start_unix > end_unix`, bounds left at the proto3 zero, a non-positive `duration_seconds`, a `duration_seconds` large enough to overflow the nanosecond `time.Duration`, `end_unix = 253402300799`, and the zero `time.Time` that `pkg/query/typed` sends as `-62135596800` — the last two wrap `UnixNano` and landed on arbitrary negative buckets. All now return `INVALID_ARGUMENT`, mirroring the check `pkg/admin` already had. +- **Query layer: retention was advisory on the read path.** A window longer than `Retention` read TTL-evicted buckets, folded the holes in as the monoid identity, and returned the result labelled as a full window. Windowed reads now reject a duration that reaches past what retention keeps. +- **Query layer: bucket fan-out was unbounded.** With `windowed.Minute(24h)`, `GetRange(entity, Unix(0,0), now)` built 29,797,201 keys (~715MB) 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 across buckets (`GetWindow`/`GetRange` and their `Many` forms, `LambdaQuery`, `WarmupWindowed`). +- **Query layer: the singleflight coalesce key was ambiguous.** Entities were sorted and joined with `|`, so `["a|b"]` shared a group with `["a","b"]` and `["a|b","c"]` with `["a","b|c"]` — 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 received the first caller's values against the wrong entities. The key is now length-prefixed and order-preserving; permutation coalescing is deliberately given up. +- **Query layer: one client disconnect failed every coalesced peer.** The shared store call ran on the context of whichever caller happened to lead the singleflight group, so a hang-up cancelled the read for everyone with `CodeInternal` — under exactly the concurrent load coalescing exists to serve. The shared work now runs detached under a server-side `Config.CoalesceTimeout` (default 10s) with each waiter selecting on its own context. +- **DynamoDB stores: a duplicate key in a batched read failed the whole RPC.** `BatchGetItem` rejects a repeated key (`Provided list of item keys contains duplicates`), and whether it reproduced depended on the two copies landing in the same 100-key chunk. `Int64SumStore.GetMany` and `BytesStore.GetMany` now chunk a de-duplicated key set and scatter results back through the `(entity, bucket)` map they already keep; `Int64MaxStore` inherits the fix. `query.WarmupWindowed` / `WarmupNonWindowed` collapse repeated entities before fetching (their reported "warmed" count is now distinct entities × buckets). +- **`pkg/query/typed`: `SumClient` wrote past its output slice.** It sized the slice from the entity list but indexed it by the server's value count, so a server returning more values than entities caused an index-out-of-range panic inside the calling application. The sketch clients' `if i >= len(out) { break }` guard turned the same bug into silent truncation. Every batched typed client now rejects a value/entity count mismatch — the wire contract is positional, so a mismatch means the values cannot be attributed at all. `examples/search-rerank` was fixed the same way. + +### Changed (breaking, pre-1.0) + +- `QueryService.Get` / `GetMany` now fail with `FAILED_PRECONDITION` on windowed pipelines instead of reporting `present: false`. +- Malformed windows and ranges now return `INVALID_ARGUMENT` instead of a monoid-identity value; `duration_seconds` must be positive, and absolute ranges must set at least one bound. +- `pkg/query`'s `GetWindow` / `GetRange` / `GetWindowMany` / `GetRangeMany` / `LambdaQuery` / `WarmupWindowed` return errors matching the new `query.ErrInvalidQuery` for these shapes. +- `windowed.Config` gains `MaxBuckets`; `grpc.Config` gains `CoalesceTimeout`. Both default from existing fields, so existing configs keep working. +- `pkg/query/typed`'s batched clients (`GetMany`, `GetWindowMany` on all four clients) now return an error when the response's value count does not match the requested entity count. + +### Fixed + +- `pkg/query`: a `GetRange` / `GetRangeMany` over exactly the retention + window is accepted again. `windowed.Config.MaxBucketSpan` derived + `ceil(Retention/Granularity)`, but bucket ranges are inclusive at both + ends, so a range of duration `Retention` touches + `Retention/Granularity + 1` buckets and was rejected as + `InvalidArgument`. +- `pkg/query`: absolute ranges are now held to `Retention` as trailing + windows already were. Setting `windowed.Config.MaxBuckets` higher than + `Retention` let `GetRange`, `GetRangeMany` and `LambdaQuery.GetRange` + read past TTL — a year against a 7-day `Retention` fanned out over 366 + buckets, folded 359 evicted ones in as `Identity`, and returned the + week's total labelled as a year. The bound is on a range's width, not + its age; `checkRetention` documents why. +- `pkg/query/grpc`: coalesced store work keeps the leading caller's + deadline instead of only a flat `CoalesceTimeout`. Detaching the shared + call from the caller's context dropped the deadline with the + cancellation, so a burst of abandoned requests each held up to + `CoalesceTimeout` of fan-out open with nobody left to read it; before + coalescing existed, a client hangup shed that work immediately. The + shared context's deadline is now derived from the leader's, capped by + `CoalesceTimeout`. + +### Changed + +- `pkg/monoid/windowed`: **breaking.** `Config.MaxBucketSpan()` counts + both ends of an inclusive range, so the default derived from + `Retention` is now `ceil(Retention/Granularity) + 1`. An explicit + `MaxBuckets` is unchanged and still used verbatim. +- `pkg/monoid/windowed`: **breaking.** A `Config` with a `Granularity` + but neither `MaxBuckets` nor `Retention` now caps reads at the new + `DefaultMaxBucketSpan` (100,000 buckets) instead of being unbounded. + `Config{Granularity: time.Minute}` previously let + `GetRange(epoch, now)` build ~30 million keys (~715MB of `state.Key`) + before the first store call. A `Config` with no `Granularity` still + reports 0 — it assigns everything to bucket 0 and cannot fan out. + ### Fixed — graceful shutdown silently lost every in-flight record `streaming.Run` treated a cancelled context as a poison record. The comment at From 83d1ecc84386a4da430b5123a845e60d3b3fe6d2 Mon Sep 17 00:00:00 2001 From: Kyle Galloway Date: Fri, 28 Aug 2026 11:53:15 -0300 Subject: [PATCH 4/5] Assert Connect's real status for FailedPrecondition (400, not 412) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S --- test/integration/page_view_counters_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/integration/page_view_counters_test.go b/test/integration/page_view_counters_test.go index 212c849..4c65284 100644 --- a/test/integration/page_view_counters_test.go +++ b/test/integration/page_view_counters_test.go @@ -305,9 +305,13 @@ func TestDeployed_PageViewCounters_QueryBootsAgainstDDB(t *testing.T) { // there. It used to answer present=false, which this test then "confirmed" — // an assertion that would have held just as well for an entity with a // million views. The server now says so out loud. + // Connect maps CodeFailedPrecondition to HTTP 400, not 412 — see + // connectCodeToHTTP in connectrpc.com/connect. The Connect error CODE below + // is the real assertion; the status is checked only so a plain 200 with a + // zero value cannot pass. status, body := post("Get", `{"entity":"page-never-seen"}`) - if status != http.StatusPreconditionFailed { - t.Fatalf("Get on a windowed pipeline: status=%d body=%s, want 412", status, body) + if status != http.StatusBadRequest { + t.Fatalf("Get on a windowed pipeline: status=%d body=%s, want 400", status, body) } var connErr struct { Code string `json:"code"` From 82e59b1028fb9e0ced169f5c488a3026ddd79e53 Mon Sep 17 00:00:00 2001 From: Kyle Galloway Date: Fri, 28 Aug 2026 12:01:29 -0300 Subject: [PATCH 5/5] Restore the fake transport's duplicate-key rejection after the rebase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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) Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S --- pkg/state/dynamodb/store_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/state/dynamodb/store_test.go b/pkg/state/dynamodb/store_test.go index 22c5875..dead959 100644 --- a/pkg/state/dynamodb/store_test.go +++ b/pkg/state/dynamodb/store_test.go @@ -207,6 +207,13 @@ func (f *fakeTransport) RoundTrip(r *http.Request) (*http.Response, error) { if err := json.Unmarshal(body, &req); err != nil { return nil, fmt.Errorf("fakeTransport: decode BatchGetItem body: %w", err) } + // Real DynamoDB rejects the whole request when the key list repeats a + // key, so the fake has to as well — otherwise a store that forwards + // duplicates looks fine here and fails only in production. + if dup, ok := duplicateKey(req); ok { + return validationException(fmt.Sprintf( + "Provided list of item keys contains duplicates (%s)", dup)), nil + } resp := f.handle(inv, req) buf, err := json.Marshal(resp) if err != nil {