Lambda partial-batch identifiers, dead config, and docs that taught the bug - #91
Merged
Merged
Conversation
pequalsnp
force-pushed
the
fix/lambda-failures-and-doc-drift
branch
2 times, most recently
from
August 28, 2026 15:02
f64830d to
c0b6885
Compare
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
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
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
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
force-pushed
the
fix/lambda-failures-and-doc-drift
branch
from
August 28, 2026 15:09
c0b6885 to
0c6804e
Compare
This was referenced Aug 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seven verified defects. One round of review returned needs-work — the code was fixed but the docs still taught the defect — and those are now fixed too.
AWS was being told the wrong record failed
The DynamoDB Streams handler reported
EventIDas theBatchItemFailuresItemIdentifier, where AWS requires theSequenceNumber.kinesis.go:245gets this right; this didn't.Outcome is one of: whole-batch redelivery (duplicate merges — and dedup defaults to off), a stalled iterator, or a silently discarded failure. All three are bad, and no test pinned any of them. Same bug in
examples/search-projector.The test now uses deliberately different values for
EventID("ev-doom") andSequenceNumber(a realistic"49590338271490256608..."), so it cannot pass by coincidence.The docs still taught it
Review's blocking finding, and the more important half — the code was fixed while three documents still instructed readers to reproduce it:
doc/design.md§6.2: "DDB Streams uses EventID" — stated twice.doc/search-integration.md:830: a copy-pasteable Lambda handler doingItemIdentifier: rec.EventID. Anyone copying it reproduced the defect verbatim.Both corrected, with a per-source table and prose on why
EventIDis the wrong field despite being the record's unique name.A crash-safety claim that was false
runtime.goanddoc/design.mdclaimed that underWithBatchWindow"a worker crash loses at most a window-worth of records (dedup catches the redelivery)". That is false in this tree: the aggregator claims the EventID when the record enters the accumulator, so withWithDedupthe claim outlives the crash and the redelivery is dedup-skipped — the batch is lost, not recovered.The prose now states the actual guarantee, both with and without a deduper. The underlying repair is #90's.
Also
SequenceNumberhad no guard.EventIDwas always non-empty; a hand-constructed record'sSequenceNumberneed not be. Lambda treats a null/emptyitemIdentifieras a malformed response and retries the whole batch — strictly worse than dropping one entry. Now guarded and counted.windowed.Config.EventTimeFielddeleted — documented as honoured by backends, read by nothing. Grep yielded exactly two hits: the declaration and its own comment. Event time comes solely fromsource.Record.EventTime.doc/use-cases.mdand the example documented K=32;topk.DefaultKis 10 andpreset.goused it — so a user buildingConfigwithout K got a K=10 store under a K=32 query server. Resolved by moving the example's default, deliberately nottopk.DefaultK, since raising that would silently resize every pipeline taking the default.replay.WithDeduphad zero coverage at any level. Now has an idempotency test, plus a TTL-horizon test rewritten against the realdynamodb.Deduperbehind the existing DDB-local gate — the first version tested its own hand-rolled fake, which is not a behavioural test.Note on the security flag
The workflow that produced this branch raised a classifier warning. I audited the full diff: the only hits are
credentials.NewStaticCredentialsProvider("test","test","")andhttp://127.0.0.1:8000in a dynamodb-local test — dummy credentials used specifically so the SDK's credential chain cannot reach real ones. No account IDs, no profiles, no outbound endpoints. Assessed as a false positive; recording the disposition rather than leaving it silent.Test plan
SequenceNumberassertions cannot pass by coincidencemake test-unit,golangci-lint0 issuesBreaking (pre-1.0):
windowed.Config.EventTimeFielddeleted.🤖 Generated with Claude Code
https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S