fix(bulk): key per-item idempotency on content, not on position - #1283
Conversation
|
@claude please review at head This changes the derivation of a value that appears in API responses (
Deliberately out of scope: H-07 (>100 facts → 500) in the same function. Its fix is batching, which under positional keys would have reintroduced this bug one layer up. |
This confirms the fix is well-designed and the comments accurately reflect the code's actual behavior. This is purely a docstring/comment-clarity and bugfix PR (with accompanying test) — the core logic change ( Claude Code Review ✅ No issues found. Reviewed by |
|
Checking my own question 1 rather than leaving it open, so you can falsify the reasoning instead of re-deriving it. The claim: no two entries in Why it holds. There is exactly one if ch in seen_hashes:
... # duplicate_content
continue
seen_hashes[ch] = i
The stronger half: the key is The residual risk I can't close by reading is a truncation collision: two different contents whose hashes agree in the first 16 hex chars. 64 bits over ≤100 items is ~2⁻⁵¹, and the consequence would be one item resolving to the other's row. If you think that's the wrong place to spend a probability budget, say so and I'll widen it — the column is |
Claude Code Review ✅ No issues found. The core functional change (deriving Reviewed by |
eec78ed to
ec459d6
Compare
create_memories_bulk derived each item's client_request_id as
f"{bulk_attempt_id}:{i}", where i is the index within THIS request's body.
That is stable only under a precondition the caller had to uphold and could
not see: same body + same attempt id => same per-item id. A retry that carries
only the items which did not succeed is a DIFFERENT body, and then every
surviving item shifts down onto an index another item's row already claimed.
The failure is silent and total:
1. the survivor's content is new, so it clears the content-hash dedup and
goes to the write path;
2. storage's ON CONFLICT DO NOTHING on ix_memories_attempt_unique sees the
client_request_id already taken and SKIPS the insert;
3. the follow-up re-query resolves that id to the FOREIGN row, which comes
back was_inserted=False;
4. that is reported as duplicate_attempt carrying the foreign row's id, so
the response reads created=0, errors=0 — a fully successful retry — while
the resent content was never written, and never will be on any number of
further retries.
This is not an exotic caller mistake. The bulk route answers 207 with per-item
results naming exactly which items failed, and tells clients that a retry of
the same logical batch reuses the same attempt id; trimming the succeeded items
is the obvious reading of that pair. ingest_commit does it structurally — its
pre-loop dedup removes already-created facts before building the body, so its
documented same-run_id retry path shrinks the body every time.
The fix keys each item on its content hash instead:
f"{bulk_attempt_id}:{content_hash[:16]}". Content is what the key was always
trying to name — "this logical row within this attempt" — so keying on it
removes the precondition rather than documenting it harder. A partial retry
becomes just a smaller batch.
Cost is nil: hashes are already computed above for the dedup gate, so the two
now key on the same value. 16 hex chars is 64 bits over a batch capped at 100
items, and identical content within one batch never reaches the write path
anyway (seen_hashes collapses it first), so the only collisions this has to
rule out are accidental.
Reproduced end-to-end against Postgres before fixing. Two items committed under
an attempt id, then two DIFFERENT items sent under the same one:
{'created': 0, 'duplicates': 2, 'errors': 0, 'results': [
{'index': 0, 'client_request_id': 'partial-dcebaf3c:0',
'status': 'duplicate_attempt', 'id': '604ef8e9-...'},
{'index': 1, 'client_request_id': 'partial-dcebaf3c:1',
'status': 'duplicate_attempt', 'id': 'cb8034b6-...'}]}
Neither resent fact exists; both ids belong to the first attempt's rows.
The new test asserts persistence, not labelling: it reads the rows back by id
and checks their CONTENT, so a relabelling that still wrote nothing would not
pass it. It also asserts the returned ids are disjoint from the first attempt's,
which is the "mapped onto another fact's row" half of the defect.
Two existing assertions pinned the positional shape via
endswith(f":{index}"). Their stated intent was "every result carries its
server-derived per-item attempt id — callers can use this to correlate with
retries", and content keying serves that better than position did, so they now
check the property (one distinct token per item, scoped to the attempt) rather
than the derivation. Pinning the shape would have pinned the defect.
Docstrings that stated the positional form are corrected in all four places
that carried it: create_memories_bulk, the bulk route, memory_add_all, and
_auto_chunk_request_id.
core-api/openapi.broker.json is regenerated because of that. The bulk route's
docstring is not internal: POST /api/v1/memories/bulk is one of the eight
frozen-v1 broker operations caura-daemon calls, and the docstring is published
as that operation's OpenAPI description. CI caught it (the baseline-staleness
gate) after local ruff, mypy, both test roots and all three naming gates passed
clean — none of them look at the generated contract. The diff is one line of
description text, no schema change, so the companion oasdiff breaking-change
gate is unaffected.
Not fixed here: H-07, the >100-fact 500 in the same function. Its fix is to
chunk survivors into batches, which under positional keys would have
reintroduced exactly this bug one layer up — batch 2's item 0 taking batch 1's
item 0's key. Content keying is what makes that fix safe, so it is the natural
follow-up rather than part of this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Eldad Caura <eldad@caura.ai>
ec459d6 to
bf334bb
Compare
|
Both review passes came back clean — thank you, and the second one added a detail I had not stated: masking happens before the hash is computed, so the key is derived from the post-governance content, which is the content actually written. That is the right property and I had not checked it. CI failed after those reviews, on something neither of us was looking at. The bulk route's docstring is not internal: Worth recording because of what caught it. Locally I had run ruff, mypy, both test roots and all three naming gates, all clean — none of them look at the generated contract. The only signal was CI. Regenerated and committed at Also rebased onto Nothing about the fix itself changed since your review — the delta is the regenerated baseline and the rebase. |
|
@erni-a this is ready for your review — H-08, the silent data loss on a partial bulk retry. State at head
Worth your attention in particular, since it is the part with the widest blast radius: The per-item idempotency key that appears in bulk responses changes shape, from Not merging — that is Eldad's. |
🤖 I have created a release *beep* *boop* --- <details><summary>backend: 2.47.2</summary> ## [2.47.2](backend-v2.47.1...backend-v2.47.2) (2026-09-04) ### Bug Fixes * **autochunk:** degrade a failed child embed instead of 500ing a persisted write ([#1278](#1278)) ([2342f86](2342f86)) * **bulk:** key per-item idempotency on content, not on position ([#1283](#1283)) ([fa779e5](fa779e5)) * **core-api:** stamp provenance on bulk re-embed fallbacks ([#1281](#1281)) ([46eb44a](46eb44a)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: release-please[bot] <release-please[bot]@users.noreply.github.com> Co-authored-by: caura-deploy-bot[bot] <265395343+caura-deploy-bot[bot]@users.noreply.github.com>
…00ing ingest_commit packed every surviving fact into a single BulkMemoryCreate, whose items field carries max_length=BULK_MAX_ITEMS (100). Nothing upstream caps the fact count: IngestCommitRequest.facts has no max_length, and preview caps nothing either — it accepts documents up to 100k tokens, sections them at ~2k tokens, and asks the extractor for 5-20 facts per section, so a ~40k-token document routinely clears 100 facts. The model was constructed OUTSIDE the try below it, which catches only HTTPException in any case, and core-api registers no handler for a raw pydantic.ValidationError — RequestValidationError is FastAPI's request-body wrapper and a hand-built model does not raise it. So an over-100 commit reached the global exception handler as an opaque 500 with ZERO facts persisted, and the retry re-extracted the same set and failed identically. The user's only recourse was to feed in a smaller document. Reproduced before fixing, straight out of the pre-fix path: ingest_service.py:1070: pydantic_core._pydantic_core.ValidationError: 1 validation error for BulkMemoryCreate Survivors are now chunked into batches of BULK_MAX_ITEMS and committed sequentially. Every batch shares ONE bulk_attempt_id — run_id, unchanged — and that is the part worth reading twice. It is correct only because H-08 (#1283) made the per-item idempotency key content-derived, so batch boundaries no longer enter the key. The audit's suggested f"{run_id}:batch{n}" would have been actively harmful: the pre-loop dedup shrinks the survivor list between attempts, so a retry re-cuts the boundaries and the same fact lands in a different batch — computing a different key each time and losing the duplicate_attempt resolution that reusing run_id exists to provide. A test pins the shared id so a later change cannot quietly reintroduce per-batch keys. Sequential rather than gathered: each batch already fans out its own embed/enrich internally and takes a per-tenant storage slot, so concurrency here would multiply pressure on the same bulkhead, and it keeps the abort-on-failure semantics honest. A batch failing with HTTPException now stops the run and keeps the counts the earlier batches earned. It previously zeroed them and logged "0 facts persisted on this attempt" — true while there could only ever be one batch, and a false statement to an operator once there can be several, at exactly the moment they are deciding whether to clean up. The message now names how many landed, how many were not attempted, and which fact index the failing batch started at. Stopping rather than continuing is deliberate: these failures are overwhelmingly systemic, so the remaining batches would queue behind the same wall and turn one failure into N. The per-item "fact[N]" warning offsets item.index by the batch start. That format is what the P1.C-lite runbook and operator greps key on; without the offset every batch would restart at fact[0] and point an operator at the wrong fact. The frame of reference is unchanged — it counts within the survivor list, as it always has, not within the caller's original fact list. Tests: three, all confirmed to fail without the fix (the first two with the ValidationError above). The over-100 commit persists every fact — asserted by comparing the full written-content list against the input, not just the batch count, so a chunking bug that dropped or duplicated a fact at a boundary cannot pass. The attempt id is shared across batches. And the mid-run failure keeps the earlier count, stops rather than continuing, and does not log the now-false "0 facts persisted". The captured fixture gains two knobs for this: bulk_attempt_ids records the id each call received, and raise_http_on_batch injects a whole-call failure keyed on batch number — distinct from the existing write_raise_for, which produces per-item error results rather than an HTTPException from the call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Eldad Caura <eldad@caura.ai>
…00ing ingest_commit packed every surviving fact into a single BulkMemoryCreate, whose items field carries max_length=BULK_MAX_ITEMS (100). Nothing upstream caps the fact count: IngestCommitRequest.facts has no max_length, and preview caps nothing either — it accepts documents up to 100k tokens, sections them at ~2k tokens, and asks the extractor for 5-20 facts per section, so a ~40k-token document routinely clears 100 facts. The model was constructed OUTSIDE the try below it, which catches only HTTPException in any case, and core-api registers no handler for a raw pydantic.ValidationError — RequestValidationError is FastAPI's request-body wrapper and a hand-built model does not raise it. So an over-100 commit reached the global exception handler as an opaque 500 with ZERO facts persisted, and the retry re-extracted the same set and failed identically. The user's only recourse was to feed in a smaller document. Reproduced before fixing, straight out of the pre-fix path: ingest_service.py:1070: pydantic_core._pydantic_core.ValidationError: 1 validation error for BulkMemoryCreate Survivors are now chunked into batches of BULK_MAX_ITEMS and committed sequentially. Every batch shares ONE bulk_attempt_id — run_id, unchanged — and that is the part worth reading twice. It is correct only because H-08 (#1283) made the per-item idempotency key content-derived, so batch boundaries no longer enter the key. The audit's suggested f"{run_id}:batch{n}" would have been actively harmful: the pre-loop dedup shrinks the survivor list between attempts, so a retry re-cuts the boundaries and the same fact lands in a different batch — computing a different key each time and losing the duplicate_attempt resolution that reusing run_id exists to provide. A test pins the shared id so a later change cannot quietly reintroduce per-batch keys. Sequential rather than gathered: each batch already fans out its own embed/enrich internally and takes a per-tenant storage slot, so concurrency here would multiply pressure on the same bulkhead, and it keeps the abort-on-failure semantics honest. A batch failing with HTTPException now stops the run and keeps the counts the earlier batches earned. It previously zeroed them and logged "0 facts persisted on this attempt" — true while there could only ever be one batch, and a false statement to an operator once there can be several, at exactly the moment they are deciding whether to clean up. The message now names how many landed, how many were not attempted, and which fact index the failing batch started at. Stopping rather than continuing is deliberate: these failures are overwhelmingly systemic, so the remaining batches would queue behind the same wall and turn one failure into N. The per-item "fact[N]" warning offsets item.index by the batch start. That format is what the P1.C-lite runbook and operator greps key on; without the offset every batch would restart at fact[0] and point an operator at the wrong fact. The frame of reference is unchanged — it counts within the survivor list, as it always has, not within the caller's original fact list. Tests: three, all confirmed to fail without the fix (the first two with the ValidationError above). The over-100 commit persists every fact — asserted by comparing the full written-content list against the input, not just the batch count, so a chunking bug that dropped or duplicated a fact at a boundary cannot pass. The attempt id is shared across batches. And the mid-run failure keeps the earlier count, stops rather than continuing, and does not log the now-false "0 facts persisted". The captured fixture gains two knobs for this: bulk_attempt_ids records the id each call received, and raise_http_on_batch injects a whole-call failure keyed on batch number — distinct from the existing write_raise_for, which produces per-item error results rather than an HTTPException from the call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Eldad Caura <eldad@caura.ai>
…00ing (#1286) Closes audit finding **H-07**. Depends on the content-keyed idempotency from #1283 — see *Why one attempt id* below. ## The defect `ingest_commit` packed every surviving fact into a single `BulkMemoryCreate`, whose `items` field carries `max_length=BULK_MAX_ITEMS` (**100**). Nothing upstream caps the fact count: - `IngestCommitRequest.facts` has no `max_length`. - Preview caps nothing either — it accepts documents up to **100k tokens**, sections them at ~2k, and asks the extractor for **5-20 facts per section**. A ~40k-token document routinely clears 100 facts. The model was built **outside** the `try` below it — which catches only `HTTPException` in any case — and core-api registers no handler for a raw `pydantic.ValidationError` (`RequestValidationError` is FastAPI's *request-body* wrapper, and a hand-built model doesn't raise it). So it reached the global handler as an **opaque 500 with zero facts persisted**, and the retry re-extracted the same set and failed identically. The user's only recourse was a smaller document. ## Reproduced before fixing Straight out of the pre-fix path: ``` ingest_service.py:1070: pydantic_core._pydantic_core.ValidationError: 1 validation error for BulkMemoryCreate ``` ## The fix Survivors are chunked into batches of `BULK_MAX_ITEMS` and committed sequentially. ### Why one attempt id, not one per batch Every batch shares **`run_id`, unchanged** — and this is the part worth reading twice. It is correct **only because #1283 made the per-item idempotency key content-derived**, so batch boundaries no longer enter the key at all. The audit's suggested `f"{run_id}:batch{n}"` would have been *actively harmful*: the pre-loop dedup shrinks the survivor list between attempts, so a retry re-cuts the boundaries and the same fact lands in a different batch — computing a different key each time and losing the `duplicate_attempt` resolution that reusing `run_id` exists to provide. **A test pins the shared id** so a later change cannot quietly reintroduce per-batch keys. ### Sequential, not gathered Each batch already fans out its own embed/enrich internally and takes a per-tenant storage slot, so concurrency here would multiply pressure on the same bulkhead — and it keeps the abort-on-failure semantics below honest. ### A mid-run failure no longer lies about what landed A batch failing with `HTTPException` now **stops the run and keeps the counts earlier batches earned**. It previously zeroed them and logged *"0 facts persisted on this attempt"* — true while there could only ever be one batch, and a **false statement to an operator** once there can be several, at exactly the moment they are deciding whether to clean up. The message now names how many landed, how many were not attempted, and which fact index the failing batch started at. Stopping rather than continuing is deliberate: these failures are overwhelmingly systemic (storage down, budget burned), so the remaining batches would queue behind the same wall and turn one failure into N. ### The `fact[N]` log format The per-item warning offsets `item.index` by the batch start. That format is what the P1.C-lite runbook and operator greps key on; without the offset every batch would restart at `fact[0]` and point an operator at the wrong fact. The frame of reference is unchanged — it counts within the **survivor** list, as it always has, not within the caller's original fact list. ## Tests Three, all confirmed to fail without the fix (the first two with the `ValidationError` above): 1. **The over-100 commit persists every fact.** Asserted by comparing the full written-content list against the input — not just the batch count — so a chunking bug that dropped, duplicated or reordered a fact at a boundary cannot pass. 2. **The attempt id is shared** across batches. 3. **A mid-run failure** keeps the earlier count, stops rather than continuing, and does not log the now-false *"0 facts persisted"*. The `captured` fixture gains two knobs: `bulk_attempt_ids` records the id each call received, and `raise_http_on_batch` injects a whole-call failure keyed on batch number — distinct from the existing `write_raise_for`, which produces per-item error *results* rather than an `HTTPException` from the call. ## Verification - Full root suite: **6078 passed, 5 skipped, 1 xfailed, 0 failed**. - `ruff check` and `ruff format --check` run **separately** at CI's exact scopes — clean. - `mypy` clean apart from 2 pre-existing `types-python-dateutil` stub errors in an untouched file. - **Broker OpenAPI baseline checked** — current. (Added to my routine after #1283, where a route docstring turned out to be a published OpenAPI description and only CI caught it. Nothing here touches a route, but the check is cheap.) - `legacy_name_ratchet.py` → *No new lines.* · `do_not_touch_sentinel.py` → *All 39 protected strings survive.* · `tenant_scope_gate.py` → exit 0. All after `git add`. - Checked for an open PR on this subsystem before starting; only #524 touches ingest commit and does not overlap. - Branched from `origin/main`, rebased onto `c2ac9e87`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Eldad Caura <eldad@caura.ai> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
Closes audit finding H-08.
The defect
create_memories_bulkderived each item'sclient_request_idpositionally:Stable only under a precondition the caller had to uphold and could not see — same body + same attempt id ⇒ same per-item id. A retry carrying only the items that did not succeed is a different body, so every survivor shifts down onto an index another item's row already claimed.
Why the loss is silent and total
ON CONFLICT DO NOTHINGonix_memories_attempt_uniquesees the id already taken and skips the insert.was_inserted=False.duplicate_attemptcarrying the foreign row's id — so the response sayscreated=0, errors=0, indistinguishable from a clean retry, while the resent content was never written and never will be on any number of further retries.Reproduced against Postgres before fixing
Two items committed under an attempt id, then two different items sent under the same one:
Neither resent fact exists. Both ids belong to the first attempt's rows.
This is not an exotic caller mistake
ingest_commitdoes it structurally. Its pre-loop dedup removes already-created facts before building the body, so its own documented same-run_idretry path shrinks the body every time. The pre-dedup is what makes retries cheap and it is also what shifts the indices.The fix
Content is what the key was always trying to name — "this logical row within this attempt" — so keying on it removes the precondition rather than documenting it harder. A partial retry becomes just a smaller batch.
Cost is nil.
hashesis already computed above for the dedup gate, so the key and the dedup now agree on the same value. 16 hex chars is 64 bits over a batch capped at 100 items, and identical content within one batch never reaches the write path anyway (seen_hashescollapses it first) — so the only collisions this has to rule out are accidental ones.Tests
One new test, and it asserts persistence rather than labelling: it reads the rows back by id and checks their content, so a relabelling that still wrote nothing would not pass. It also asserts the returned ids are disjoint from the first attempt's — the "mapped onto another fact's row" half of the defect. Pairing goes through each result's own
index, not list order, so it checks the mapping the response claims.It runs over HTTP against a real Postgres, so the
ON CONFLICTbehaviour is the real one rather than a fake's.Two existing assertions pinned the positional shape via
endswith(f":{index}"). Their stated intent was "every result carries its server-derived per-item attempt id — callers can use this to correlate with retries", and content keying serves that better than position did. They now check the property — one distinct token per item, scoped to the attempt — instead of the derivation. Pinning the shape would have pinned the defect.Docstrings — and one that turned out to be wire-visible
All four places that stated the positional form are corrected:
create_memories_bulk, the bulk route,memory_add_all, and_auto_chunk_request_id.The bulk route's docstring is not internal.
POST /api/v1/memories/bulkis one of the eight frozen-v1 broker operationscaura-daemoncalls, and its docstring is published as that operation's OpenAPI description — so editing it madecore-api/openapi.broker.jsonstale and CI failed.Worth recording because of what caught it: ruff, mypy, both test roots and all three naming gates passed clean locally, and none of them look at the generated contract. Regenerated and committed; the diff is one line of description text with no schema change, so the companion
oasdiffbreaking-change gate is unaffected.Deliberately not here: H-07
H-07 is the >100-fact 500 in the same function, and its fix is to chunk survivors into batches. Under positional keys that would have reintroduced this exact bug one layer up — batch 2's item 0 taking batch 1's item 0's key. Content keying is what makes that fix safe, so H-07 is the natural follow-up rather than part of this change.
Verification
core-storage-api/tests/: 328 passed, on its own scratch database (the suite refuses to share the root suite's, by design).ruff checkandruff format --checkrun separately at CI's exact scopes — clean.mypyclean oncore-storage-api/src/;core-api/src/clean apart from 2 pre-existingtypes-python-dateutilstub errors in an untouched file.legacy_name_ratchet.py→ No new lines. ·do_not_touch_sentinel.py→ All 39 protected strings survive. ·tenant_scope_gate.py→ exit 0. All aftergit add.origin/main, rebased onto46eb44abas main moved under it (fix(autochunk): degrade a failed child embed instead of 500ing a persisted write #1278, release(clients): bump caura-client to 1.0.2, caura to 1.0.1 #1280, fix(core-api): stamp provenance on bulk re-embed fallbacks #1281). Each time the branch was rebased remotely I confirmed by patch-id that the remote head was my own commit before superseding it.🤖 Generated with Claude Code