fix(ingest): batch a commit larger than one bulk request instead of 500ing

[Eldad-Caura]

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 fix(bulk): key per-item idempotency on content, not on position #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 feat(core-api): source-document provenance on ingest commit #524 touches ingest commit and does not overlap.
• Branched from origin/main, rebased onto c2ac9e87.

🤖 Generated with Claude Code

[Eldad-Caura]

@claude please review at head d9a25d15.

Four things I'd like attacked specifically:

1. The shared attempt id is the load-bearing decision. Every batch reuses run_id, which is only safe because fix(bulk): key per-item idempotency on content, not on position #1283 made the per-item key content-derived. I argue per-batch ids would be worse, not merely unnecessary — the pre-dedup re-cuts batch boundaries between attempts, so the same fact would compute a different key on every retry. Check that reasoning against the code rather than against my summary of it; if per-batch ids are in fact harmless, the test I wrote to forbid them is over-constraining.

2. Stopping on the first failed batch. I chose abort-and-report over continue-with-the-rest, on the grounds that these failures are systemic. The cost is that a single transient 504 on batch 2 of 5 leaves batches 3-5 unwritten when they might have succeeded. Tell me if you think that trade is wrong — the retry re-sends everything and resolves the committed rows as duplicates, which is what makes me comfortable with it, but I'd like that checked.

3. The fact[N] offset. I offset item.index by the batch start so the format keeps counting in the survivor list. Please confirm that is the frame the pre-existing behaviour used — I read it as survivor-relative because the pre-dedup runs before the bulk items are built, but the runbook this format serves may assume otherwise, and if so the offset makes an existing inconsistency worse rather than better.

4. Partial-batch accounting. On a mid-run failure I report errored = un-attempted + the failed batch and keep created from earlier batches. Check the arithmetic covers every exit — in particular that nothing is double-counted between errored, skipped_in_loop and pre_dedup_skipped when the run stops early.

Not in scope: capping fact count at the route with a 422. The audit offered that as an alternative and I rejected it — preview has already shown the user those facts, so refusing to commit them is a worse answer than writing them in batches.

[Eldad-Caura]

Working my own question 4 rather than leaving it open, so you can falsify the reasoning instead of re-deriving it.

Claim: on every exit, created + skipped_in_loop + errored == len(survivors), with no double counting.

bulk_items is built by an unconditional append per survivor, so len(bulk_items) == len(survivors).

Success path. Each batch's created + duplicates + errors equals its item count (that is create_memories_bulk's own per-item contract — one result per input item). Summing over batches gives len(bulk_items).

Failure path at batch_start. Batches 0..k-1 completed, contributing exactly batch_start items across the three counters. The failing batch raised before returning, so it contributed nothing. Then errored += len(bulk_items) - batch_start, which counts the failing batch and every un-attempted one. Total: batch_start + (len(bulk_items) - batch_start) = len(bulk_items). No item is counted twice and none is dropped.

skipped is a separate bucket. skipped = pre_dedup_skipped + skipped_in_loop, and pre_dedup_skipped counts facts that never became survivors — disjoint from everything above by construction.

Checked against the test's numbers: 210 facts, batch 1 of 3 fails → created=100, errored = 210 - 100 = 110, sum 210. That is what the test asserts and what it observes.

The one thing I could not settle by reading is question 3, the fact[N] frame of reference — that depends on what the P1.C-lite runbook assumes, which is not in this repo. I read the pre-existing behaviour as survivor-relative because the pre-dedup runs before bulk_items is built, so I preserved that frame rather than switching to caller-fact indices. If the runbook assumes caller indices, then the format was already off by the pre-dedup count before this PR and my offset neither fixes nor worsens it — but someone who knows that runbook should say so.

[github-actions]

🤖 Review by Claude Code

Claude Code Review ✅ No issues found.

The batching logic correctly slices bulk_items into BULK_MAX_ITEMS-sized chunks, accumulates counts across batches, offsets per-item log indices by batch_start, and preserves already-persisted counts when a batch fails mid-run (verified the created + errored accounting invariant holds in both the success and failure paths). The design decisions around sequential processing and reusing a single bulk_attempt_id across batches are explicitly justified in the comments and covered by dedicated tests. No correctness, security, or logic issues found in the changed source lines.

---

Reviewed by claude-sonnet-5 · cost $0.29325775

[github-actions]

🤖 Review by Claude Code

Claude Code Review ✅ No issues found.

The batching logic correctly preserves ordering, accumulates counts across successful batches, and the shared run_id reuse is safe because create_memories_bulk keys each item's idempotency id on content-hash rather than position (verified in memory_service.py's item_request_id = f"{bulk_attempt_id}:{hashes[i][:16]}"), so cross-batch retries and same-run duplicate content resolve correctly. The batch_start + item.index offset for error logging, the unattempted calculation on abort, and the sequential (non-concurrent) execution to respect the per-tenant storage bulkhead are all internally consistent with the accompanying comments and covered by the added tests.

---

Reviewed by claude-sonnet-5 · cost $0.6768084999999999

[Eldad-Caura]

@erni-a ready for your review — H-07, the opaque 500 that lost every fact when a commit exceeded 100.

State at head d9a25d15:

• Every check green, verified against the head SHA. claude-review ran on this head (not skipped), so the verdict covers the SHA that is actually up.
• Two review passes, both clean. The second independently checked the load-bearing decision by going to memory_service.py and confirming the content-derived key, rather than taking my word for it.
• Full root suite 6078 passed / 0 failed; ruff, mypy, ratchet, sentinel, tenant-scope gate and the broker OpenAPI baseline all clean after git add.

Two things worth your eye:

1. The shared attempt id. Every batch reuses run_id. That is safe only because #1283 made the per-item key content-derived — batch boundaries no longer enter the key. The audit's suggested f"{run_id}:batch{n}" would have been worse than unnecessary: the pre-dedup re-cuts boundaries between attempts, so the same fact would compute a different key on each retry and lose the duplicate_attempt resolution. There is a test forbidding per-batch keys; if you disagree with that call, it is the test to delete.

2. One question neither review could settle, and I am not reading silence as agreement. The per-item fact[N] warning counts within the survivor list — after pre-dedup — because that is how I read the pre-existing behaviour. I preserved that frame and offset by the batch start. But the P1.C-lite runbook that this format serves is not in this repo, so if it assumes caller fact indices, the format was already off by the pre-dedup count before this PR. My change neither fixes nor worsens that, but someone who knows the runbook should confirm which frame it expects.

Also flagging a deliberate rejection: the audit offered "cap fact count at the route with a 422" as an alternative. I did not take it — preview has already shown the user those facts, so refusing to commit them is a worse answer than writing them in batches. Say so if you'd rather have the cap.

Not merging — that's Eldad's.