Conversation
Add optional request-level source_doc_id and ts_valid_start to IngestCommitRequest. One commit distills one source document, so both apply to every fact in the batch: source_doc_id lands in each memory's metadata (and on the parent ingest-sources Document) so a later re-sync can find everything derived from that document; ts_valid_start carries the source's last-edited time onto each memory's temporal-validity column so imported facts inherit the age of the page they came from. Both fields are optional — existing callers see the exact prior write shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: erni <erni@caura.ai>
Contributor
|
Claude Code Review — skipped: PR author 'erni-a' is not a public member of the 'caura-ai' org |
Fact-dense documents can extract more than 100 facts (first hit importing real wiki pages: 115-194 facts per page). ingest_commit passed all survivors in a single BulkMemoryCreate, whose items list caps at BULK_MAX_ITEMS — the request 500'd on the model's own validator before reaching the bulk endpoint. Survivors now commit in <=BULK_MAX_ITEMS chunks. Attempt-id scheme: chunk 0 keeps the bare run_id (byte-identical to the pre-split contract, so a retry that straddles a deploy still dedups); later chunks append #<n>, deterministic so a retry of the same run re-derives the same ids and sees duplicate_attempt. A chunk-level HTTP failure no longer aborts the whole commit — later chunks are independent, the failed chunk's facts count as errored, and a retry re-attempts only what failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: erni <erni@caura.ai>
Contributor
|
Claude Code Review — skipped: PR author 'erni-a' is not a public member of the 'caura-ai' org |
Both are LLM-bound operator-plane calls that scale with document size (preview: one extraction round per ~3k-token section; commit: strong-mode enrichment across up-to-BULK_MAX_ITEMS chunks). Importing a real fact-dense wiki page blew the 45s blanket budget mid-commit and 504'd AFTER earlier chunks had already persisted — the same partial-write-then-cancel shape that motivated the /memories/bulk opt-out (CAURA-602). Both routes are bounded by the per-call LLM timeouts and the caller's own client timeout instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: erni <erni@caura.ai>
Contributor
|
Claude Code Review — skipped: PR author 'erni-a' is not a public member of the 'caura-ai' org |
The scored-search status filter only excluded outdated/conflicted, so memories archived by the source-deletion tombstone flow (and cancelled rows) kept ranking in /api/v1/search — archival removed nothing from recall. Add both statuses to the exclusion; the explicit status_filter override still allows inspection. Integration tests cover archived, cancelled, and the active-sibling-still-returned case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: erni <erni@caura.ai>
Contributor
|
Claude Code Review — skipped: PR author 'erni-a' is not a public member of the 'caura-ai' org |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: erni <erni@caura.ai>
Contributor
|
Claude Code Review — skipped: PR author 'erni-a' is not a public member of the 'caura-ai' org |
This was referenced Jul 30, 2026
Eldad-Caura
added a commit
that referenced
this pull request
Sep 4, 2026
Closes audit finding **H-08**.
## The defect
`create_memories_bulk` derived each item's `client_request_id`
positionally:
```python
item_request_id = f"{bulk_attempt_id}:{i}" # i = index within THIS body
```
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
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 id already taken and **skips the insert**.
3. The follow-up re-query resolves that id to the **foreign row**,
returned `was_inserted=False`.
4. That reads as `duplicate_attempt` carrying the foreign row's id — so
the response says `created=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:
```
{'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.
## This is not an exotic caller mistake
- **The route invites it.** It answers **207** with per-item results
naming exactly which items failed, and tells clients a retry of the same
logical batch reuses the same attempt id. Resending just the failures 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 own documented
same-`run_id` retry path shrinks the body every time. The pre-dedup is
what makes retries cheap and it is also what shifts the indices.
## The fix
```python
item_request_id = f"{bulk_attempt_id}:{hashes[i][: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` is 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_hashes` collapses 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 CONFLICT`
behaviour 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/bulk` is one of the eight frozen-v1 broker operations
`caura-daemon` calls, and its docstring is published as that operation's
OpenAPI **description** — so editing it made
`core-api/openapi.broker.json` stale 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
`oasdiff` breaking-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
- Full root suite: **6075 passed, 5 skipped, 1 xfailed, 0 failed**.
- `core-storage-api/tests/`: **328 passed**, on its own scratch database
(the suite refuses to share the root suite's, by design).
- `ruff check` and `ruff format --check` run **separately** at CI's
exact scopes — clean.
- `mypy` clean on `core-storage-api/src/`; `core-api/src/` clean apart
from 2 pre-existing `types-python-dateutil` stub 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 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 `46eb44ab` as main moved
under it (#1278, #1280, #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](https://claude.com/claude-code)
Signed-off-by: Eldad Caura <eldad@caura.ai>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Eldad-Caura
added a commit
that referenced
this pull request
Sep 4, 2026
…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>
This branch has not been deployed
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.
What
Adds optional request-level
source_doc_idandts_valid_starttoIngestCommitRequestand stamps them through the commit path.One commit distills one source document, so both fields apply to every fact in the batch:
source_doc_id— the stable id in the source system (e.g. a wiki page id). Stamped into each memory'smetadataand onto the parentingest-sourcesDocument, so a later re-sync can find everything derived from that document.ts_valid_start— the source's last-edited time. Stamped onto each memory's temporal-validity column so imported facts inherit the age of the page they came from instead of landing as brand-new knowledge. Explicit value wins over enrichment's content-inferred date (existing bulk-path semantics).Why
Foundation for importing external knowledge sources through the existing ingest pipeline with full provenance — every derived memory should point back at the exact source document and carry its timestamp.
Compatibility
Both fields are optional. Callers that omit them get the exact prior write shape (covered by test).
Testing
tests/test_ingest_commit.py(stamping on every item, absent-when-omitted back-compat, parent-Document provenance)🤖 Generated with Claude Code
Addendum (2026-07-12) — follow-up commits since the original description
4c844f2— ingest commit splits bulk writes atBULK_MAX_ITEMS, so a large preview no longer 413s/times out the storage layer in one oversized batch.b67594c— ingest preview/commit opted out of the request-wide timeout (LLM-bound extraction needs the LLM budget, not the default request budget).0219ed4— scored search now excludesarchivedandcancelledmemories (the archival contract was previously a no-op in recall); exact-lexical-match escape hatch forconflictedrows unchanged. Red/green tested plus a live re-check.3c31872— ruff-format only.