DynamoDB state-layer limits: 400KB guard, CAS visibility, dedup key scoping - #88
Merged
Conversation
Four ways the DynamoDB state layer loses data or hides it: An oversized sketch fails every write non-retryably while Get keeps serving the last value that fit, as Present:true — a key that has silently stopped updating but reads healthy. Sketch size tracks key length, not just K, so a TopK lifting an unbounded wire field into its keys reaches 400KB on a couple of entries. casWrite now pre-flights the limit and returns ErrItemTooLarge / *ItemTooLargeError instead of an opaque wrapped ValidationException. CAS contention had a hardcoded ceiling of 8 retries, ~17s of backoff per record, then a dead letter — with no metric anywhere saying the writes were racing rather than failing. maxRetries and the backoff schedule are injectable now (WithCASRetries / WithCASBackoff), and every losing attempt lands under "<pipeline>:cas_conflict" via WithCASMetrics. The recently-interacted example wires it: that pipeline keys everything to "global", so its whole ingest rate lands on one row. The dedup claim key was the bare EventID, but EventIDs are only unique within a source. Two pipelines sharing one dedup table — the layout doc/design.md §13.4 recommends, and which §13.4 already describes as carrying the pipeline name — starve each other: whichever claims "evt-007" first makes the other skip a merge that never ran. Claims are now keyed "<pipeline>#<EventID>". The claim row carried no writer identity, so a claim DDB committed whose response was lost to a connection reset came back as a plain ConditionalCheckFailed on the SDK's retry, indistinguishable from a peer's claim, and dropped a first delivery. Each MarkSeen now stamps a claimant token and admits it in the condition; the SDK's retry middleware sits after serialization, so the replay carries the identical token and recognizes its own row. BREAKING: NewDeduper takes a pipeline name, and the on-disk dedup key format changes. An operator with an existing dedup table gets one window of re-processing as previously-claimed IDs re-claim under their namespaced keys; for non-idempotent monoids, drain in-flight records before deploying, or let the old rows age out via TTL first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
Kept out of the implementing commit so five parallel branches did not all conflict on the [Unreleased] heading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
pequalsnp
pushed a commit
that referenced
this pull request
Aug 28, 2026
NewDeduper gained a pipeline-name argument in #88 (claim keys are now "<pipeline>#<EventID>"). This test file was written on a parallel branch against the old three-argument signature. go build does not compile _test.go files, so only golangci-lint's typecheck caught it — worth noting for the next cross-branch rebase. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
pequalsnp
pushed a commit
that referenced
this pull request
Aug 28, 2026
#88 rewrote fakeTransport.RoundTrip on main and dropped the duplicate check this branch had added. Without it the fake accepts what real DynamoDB rejects, so a store that forwards duplicate keys passes the test and fails only in production — which is the whole failure this test exists to catch. Verified by mutation: replacing `uniq := dedupeKeys(ks)` with `uniq := ks` now fails both dedup tests with the real API error text, "Provided list of item keys contains duplicates (t/a/0)". Before this commit the mutation went unnoticed. Also reconciles the merged test file with main's newFakeClient signature, which gained a maxAttempts argument in #88. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
pequalsnp
pushed a commit
that referenced
this pull request
Aug 28, 2026
NewDeduper gained a pipeline-name argument in #88 (claim keys are now "<pipeline>#<EventID>"). This test file was written on a parallel branch against the old three-argument signature. go build does not compile _test.go files, so only golangci-lint's typecheck caught it — worth noting for the next cross-branch rebase. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
pequalsnp
pushed a commit
that referenced
this pull request
Aug 28, 2026
#88 rewrote fakeTransport.RoundTrip on main and dropped the duplicate check this branch had added. Without it the fake accepts what real DynamoDB rejects, so a store that forwards duplicate keys passes the test and fails only in production — which is the whole failure this test exists to catch. Verified by mutation: replacing `uniq := dedupeKeys(ks)` with `uniq := ks` now fails both dedup tests with the real API error text, "Provided list of item keys contains duplicates (t/a/0)". Before this commit the mutation went unnoticed. Also reconciles the merged test file with main's newFakeClient signature, which gained a maxAttempts argument in #88. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
pequalsnp
pushed a commit
that referenced
this pull request
Aug 28, 2026
NewDeduper gained a pipeline-name argument in #88 (claim keys are now "<pipeline>#<EventID>"). This test file was written on a parallel branch against the old three-argument signature. go build does not compile _test.go files, so only golangci-lint's typecheck caught it — worth noting for the next cross-branch rebase. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
pequalsnp
pushed a commit
that referenced
this pull request
Aug 28, 2026
#88 rewrote fakeTransport.RoundTrip on main and dropped the duplicate check this branch had added. Without it the fake accepts what real DynamoDB rejects, so a store that forwards duplicate keys passes the test and fails only in production — which is the whole failure this test exists to catch. Verified by mutation: replacing `uniq := dedupeKeys(ks)` with `uniq := ks` now fails both dedup tests with the real API error text, "Provided list of item keys contains duplicates (t/a/0)". Before this commit the mutation went unnoticed. Also reconciles the merged test file with main's newFakeClient signature, which gained a maxAttempts argument in #88. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
pequalsnp
added a commit
that referenced
this pull request
Aug 28, 2026
…he bug (#91) * Report the sequence number Lambda checkpoints on, not the eventID A DDB Streams BatchItemFailure is resolved against the shard's sequence numbers. Both handlers reported the record's eventID there, which names nothing Lambda can find: the batch is redelivered whole (duplicate merges, and dedup defaults to off), left on a stalled iterator, or the failure is discarded. kinesis.go already had this right. The eventID keeps its real job — it is what feeds the Deduper. The fixtures now carry an eventID and a realistic numeric SequenceNumber that share no characters, so an assertion on the wrong identifier cannot pass by coincidence. Alongside, four claims the code never honored: - windowed.Config.EventTimeField was read by nothing. Event time comes from source.Record.EventTime and always has. Deleted (breaking). - replay.WithDedup had no coverage at any level and promised idempotent re-runs without qualification. Two tests pin the contract: identical archives fold to one, and the same archive past the deduper's TTL merges twice — 200, not 100 — because an evicted claim is indistinguishable from a record never seen. - The recently-interacted example documented K=32 while Build() resolved a zero K to topk.DefaultK (10), under a query server hard-coded to 32. Mismatched-K sketches refuse to merge, so that lands as an empty Top-N rather than an error. K now resolves once, through Config.ResolveK(), for every binary; the example's default is the 32 its deployment has always run. - WithBatchWindow's doc said a crash loses at most a window of records because "dedup catches the redelivery". It does not: those records never reached the store, so the redelivery is their first apply, and a claim taken before the flush would suppress it. Prose only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S * Stop the docs teaching the eventID bug this branch fixed The code fix landed but every prose copy of it survived, so anyone reading the design doc or copy-pasting the search-integration handler reproduced the defect verbatim. - doc/design.md 6.2: replaced "DDB Streams uses EventID" with a per-source table of what Lambda actually checkpoints on, and spelled out why eventID is the wrong field even though it is the record's unique name. The dedup-EventID bullet now says explicitly that it is the only place EventID is used. - doc/search-integration.md: the Pattern B handler sample reported rec.EventID. It reports rec.Change.SequenceNumber now, and skips the entry when there isn't one. Guard the empty SequenceNumber. EventID was always non-empty, so swapping in Change.SequenceNumber introduced a shape the handler couldn't produce before: a hand-constructed or synthetic record with no cursor. Lambda treats a null/empty itemIdentifier as a malformed response and redelivers the WHOLE batch, re-merging every record that already succeeded — strictly worse than losing the one entry. Both the handler and the projector example now drop the unreportable entry and make the drop loud (metrics.RecordError plus a "<name>:unreportable_failure" event; Stats.Unreportable in the example). Correct the WithBatchWindow crash-safety prose. It asserted safety on a precondition that is false in this tree: aggregator.accept claims the EventID when the record ENTERS the accumulator, not at flush. So with WithDedup the claim outlives a crash, the redelivery is dedup-skipped, and the in-flight batch is lost — the opposite of the unbatched path, where MergeOne releases the claim on a failed merge. Fixed the three places that said otherwise (runtime.go's WithBatchWindow note, design 5.4, design 14.1 + its failure diagram) and left coalesce.go / aggregator.go untouched. Two tests that could not fail: - examples/recently-interacted-topk/multisource_test.go carried `const k uint32 = 10` and a hand-rolled buildPipeline claiming to mirror Build, while every deployed binary uses K=32. It calls the real example.Build now and substitutes only the Store, with assertions tying the built sketch's K to Config.ResolveK(). - TestReplay_RerunAfterClaimExpiryDoubleCounts mostly tested its own fake: ttlDeduper hand-rolled an expiry nothing in murmur implements (DynamoDB's TTL does). Deleted it, along with the fake clock, and reasserted the claim against the real dynamodb.Deduper in test/e2e/replay_dedup_ttl_test.go behind the DDB-local gate — evicting the claim rows the way TTL does, through the raw client rather than the type under test. The surviving unit test keeps a plain claim-once deduper and covers only the runtime's half of the contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S * 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S * Scope the replay e2e deduper to its pipeline NewDeduper gained a pipeline-name argument in #88 (claim keys are now "<pipeline>#<EventID>"). This test file was written on a parallel branch against the old three-argument signature. go build does not compile _test.go files, so only golangci-lint's typecheck caught it — worth noting for the next cross-branch rebase. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S --------- Co-authored-by: Kyle Galloway <kyle@galloway.software> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
pequalsnp
pushed a commit
that referenced
this pull request
Aug 28, 2026
#88 rewrote fakeTransport.RoundTrip on main and dropped the duplicate check this branch had added. Without it the fake accepts what real DynamoDB rejects, so a store that forwards duplicate keys passes the test and fails only in production — which is the whole failure this test exists to catch. Verified by mutation: replacing `uniq := dedupeKeys(ks)` with `uniq := ks` now fails both dedup tests with the real API error text, "Provided list of item keys contains duplicates (t/a/0)". Before this commit the mutation went unnoticed. Also reconciles the merged test file with main's newFakeClient signature, which gained a maxAttempts argument in #88. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
pequalsnp
added a commit
that referenced
this pull request
Aug 28, 2026
…answer (#89) * Stop the query layer answering broken requests with a confident zero Eight defects on the read path, all of which surfaced as plausible-looking data rather than as errors. Get/GetMany on a windowed pipeline could never return anything: they read bucket 0, which is simultaneously the all-time sentinel and the epoch bucket, so they reported "absent" no matter how much the pipeline had counted. Four shipped runbooks pointed operators at them; the soak runbook author was caught by it, and the integration test that "verified" the absent-entity shape would have passed for an entity with a million views. Now FAILED_PRECONDITION naming GetWindow, which fresh_read does not bypass. Degenerate time bounds were answered with a fabricated Present:true zero: start after end, bounds left at the proto3 zero, a negative duration, and the two instants whose UnixNano wraps — end_unix=253402300799 and the zero time.Time that pkg/query/typed puts on the wire as -62135596800, both of which landed on arbitrary negative buckets. All INVALID_ARGUMENT now, mirroring the check pkg/admin already had. Retention was advisory on the read path. Asking for 90 days against a 7-day retention read 83 TTL-evicted buckets, folded the holes in as the monoid identity, and labelled the result a full 90-day window. Nothing bounded the bucket fan-out. windowed.Minute(24h) with GetRange(entity, Unix(0,0), now) built 29,797,201 keys — about 715MB of state.Key — of which all but 1,440 addressed buckets TTL had already evicted. windowed.Config gains MaxBuckets, defaulting to ceil(Retention/Granularity), enforced on every path that fans a read over buckets. The singleflight coalesce key was ambiguous. Entities were joined with '|' after sorting, so ["a|b"] shared a group with ["a","b"] and ["a|b","c"] with ["a","b|c"] — a length check catches neither — and the shipped codegen key_template puts a literal '|' inside entity keys. Sorting also merged ["a","b"] with ["b","a"], and since responses are positional the second caller got the first caller's values against the wrong entities. The key is now length-prefixed and order-preserving; permutation coalescing is deliberately given up. The coalesced work also ran on the context of whichever caller led the group, so one client hanging up failed every peer with CodeInternal — under exactly the load coalescing exists to serve. It now runs detached under a server-side CoalesceTimeout, with each waiter selecting on its own context; a plain WithoutCancel would have dropped the client deadline and let a wedged store pin the group. Duplicate keys reached BatchGetItem verbatim, which DynamoDB rejects outright ("Provided list of item keys contains duplicates"), failing the whole RPC — and only when the two copies happened to land in the same 100-key chunk, so it read as a candidate-set-size flake. Both DDB stores now chunk a de-duplicated key set and scatter results back through the (entity, bucket) map they already keep. Finally, typed.SumClient sized its output slice from the entity list but wrote out[i] for every value the SERVER returned: a remotely triggerable index-out-of-range panic inside the calling application. The sketch clients' `if i >= len(out) { break }` turned the same bug into silent truncation. All batched clients now reject a value/entity count mismatch, since the wire contract is positional and a mismatch means the values cannot be attributed at all. BREAKING (pre-1.0): Get/GetMany now fail on windowed pipelines; malformed windows and ranges now error instead of returning zero; pkg/query's window helpers return errors matching query.ErrInvalidQuery; typed clients error on a value-count mismatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S * Stop the fan-out cap rejecting the exact-retention range The bucket-span cap was one short. BucketRange is inclusive at both ends, so an absolute [t, t+Retention] touches Retention/Granularity + 1 buckets, but MaxBucketSpan derived ceil(Retention/Granularity). GetRange and GetRangeMany over exactly the retention window — the most natural range a caller writes — came back InvalidArgument. The default is now ceil(Retention/Granularity) + 1; an explicit MaxBuckets is still used verbatim, since that is a count the operator wrote down. Four more holes around the same code: - A Config with a Granularity but neither MaxBuckets nor Retention got MaxBucketSpan()==0, i.e. no cap at all, so GetRange(epoch, now) at minute granularity still built ~30 million keys. It now falls back to DefaultMaxBucketSpan (100,000) — a backstop against unbounded, far above any deliberate query. - checkRetention never ran on the absolute-range path, so raising MaxBuckets above Retention bought reads straight past TTL: a year against a 7-day Retention fanned out over 366 buckets, found 359 evicted, folded the holes in as Identity and labelled the week's total as a year. Ranges are now held to Retention too. The bound is on a range's WIDTH, not its age, and checkRetention says why: rangeBuckets has no `now`, and LambdaQuery.GetRange reads a batch view that outlives the streaming table's TTL by design. - Coalesced work dropped the caller's deadline along with its cancellation. WithoutCancel plus a flat CoalesceTimeout meant a burst of abandoned requests each kept up to 10s of store fan-out alive with nobody left to read it, where before coalescing a hangup shed that work at once. The shared context now derives its deadline from the leader's, capped by CoalesceTimeout. - TestQueryServer_CoalescedPeerSurvivesCallerCancellation was racing a sleep and failed only 16 runs in 20 against the unfixed server. It is now gated end to end: the leader is provably inside the store call, the peer is provably a waiter on its group (via a context that signals when coalesce evaluates Done(), which only happens after DoChan has registered it), and blockingStore resolves cancel-vs-release by checking ctx.Err() instead of letting select flip a coin. 20/20 pre-fix failures now. TestGetRange_RejectsBucketSpanBeyondCap and its Many counterpart were retargeted onto a config where MaxBuckets, not Retention, is the binding limit — otherwise the new retention check refuses those ranges a step earlier and neither test would reach the span cap it names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S * 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S * Assert Connect's real status for FailedPrecondition (400, not 412) The integration test expected HTTP 412. Connect maps CodeFailedPrecondition to 400 — verified in connectCodeToHTTP in connectrpc.com/connect v1.20.0. The server behaviour was correct; only the test's expectation was wrong. The Connect error code assertion just below is the real check; the status is asserted only so a plain 200 with a zero value cannot pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S * Restore the fake transport's duplicate-key rejection after the rebase #88 rewrote fakeTransport.RoundTrip on main and dropped the duplicate check this branch had added. Without it the fake accepts what real DynamoDB rejects, so a store that forwards duplicate keys passes the test and fails only in production — which is the whole failure this test exists to catch. Verified by mutation: replacing `uniq := dedupeKeys(ks)` with `uniq := ks` now fails both dedup tests with the real API error text, "Provided list of item keys contains duplicates (t/a/0)". Before this commit the mutation went unnoticed. Also reconciles the merged test file with main's newFakeClient signature, which gained a maxAttempts argument in #88. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S --------- Co-authored-by: Kyle Galloway <kyle@galloway.software> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four verified defects from the edge-case hunt. Reviewed independently and returned ship-with-notes; I additionally spot-checked the dedup scoping myself — neutralising
claimKeyback to a bare EventID fails both new tests.Silent freeze on an oversized item
BytesStorehad no pre-flight against DynamoDB's hard 400 KB item limit. An oversized sketch failed every write non-retryably, whileGetkept serving the last value that fit asPresent:true. A key that had silently stopped updating read as perfectly healthy.The realistic trigger is key length, not K:
examples/recently-interacted-topk/pipeline.golifts an unbounded raw wire field into the sketch key, so two ~200 KB entity IDs overflow the row at the shipped K=32. (For calibration, I measured the live soak's actual rows at 624 B and 382 B — 0.16% of the limit — so this is a cliff you fall off, not a slope you notice.)Now returns a typed
ErrItemTooLargecarrying the key and measured size.CAS contention was invisible
8 retries and ~17s of backoff, hardcoded with no setter, then a dead-letter — with no metric anywhere to say the writes were racing rather than failing. Now configurable via
WithCASRetries/WithCASBackoff, counted under<pipeline>:cas_conflict, andErrMaxRetriesExceededcarries table, key and attempt count.Worth noting this is the metric that would tell you whether a hot key is contending — the thing the soak currently cannot measure.
Two pipelines sharing a dedup table starved each other
EventIDs are only unique within a source, and the claim key was the bare EventID. Two pipelines sharing one dedup table — the layout
doc/design.md§13.4 recommends — meant whichever claimed an ID first made the other skip a merge that never ran.The namespaced key
"<pipeline>#<EventID>"is the format §13.4 already described. The code just never wrote it.A lost response dropped a first delivery
A claim DynamoDB had committed but whose response was lost to a connection reset came back as a plain
ConditionalCheckFailedExceptionon the SDK's retry — indistinguishable from a peer's claim — and dropped a first delivery. EachMarkSeennow stamps a per-callclaimanttoken and admits it in the condition. This works because the SDK's retry middleware sits after serialization, so the replay carries the identical token.Rate is ~1e-5 of claims. Fixed for the invariant, not the magnitude.
Breaking, and what it means for the running soak
NewDeduper(client, table, ttl)→NewDeduper(client, table, pipeline, ttl)NewBytesStoretakes variadic options (existing 3-arg calls compile unchanged)The dedup key format change gives one window of re-processing as previously-claimed IDs re-claim under their namespaced keys. No schema change — same partition-key attribute, different contents. At the soak's 1 event/sec that window is a handful of records; at production rates, drain in-flight records first or let the old rows age out via TTL.
Test plan
TestBytesStore_MergeUpdate_RejectsOversizedItemBeforeWritingTestBytesStore_MergeUpdate_CASConflictExhaustsInjectedRetriesTestDeduper_PipelineScopesShareATableWithoutCollidingTestDeduper_SeparateInstancesShareATableWithoutColliding— the collision is in the table's key space, not the Go objectTestDeduper_LostClaimResponseIsNotADuplicate— via the existingfakeTransport, no smithy middleware neededgo build,go test -race,make test-unit,golangci-lint0 issuesReviewer notes carried forward, not blocking
The size guard is slightly less conservative than its comment claims (numbers counted as decimal text where DDB packs tighter), and
doc/design.md§7.4 still documents the old dedup schema. Both worth a follow-up.🤖 Generated with Claude Code
https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S