fix(retain): preserve oversized document deltas

[Korayem]

Transparency note: This pull request was implemented by an AI coding agent under direct user supervision. The user supplied the production use case, approved the intended behavior, and supervised the investigation, implementation, and verification.

Summary

Preserve Delta Retain's changed-chunk-only extraction for oversized full-document replacements.

This matters for append-only documents such as chat transcripts: callers can safely keep sending the canonical full transcript with update_mode=replace, while unchanged history is excluded from LLM extraction whenever the actual delta fits the configured retain bounds.

Failure mode

Assume the stored transcript has logical retain chunks:

A B C D E

The next scheduled retain submits the canonical replacement document:

A B C D E + new tail

When that document exceeded retain_batch_tokens, the old flow split it into transport-sized fragments before running Delta Retain. Those transport boundaries do not necessarily align with the logical chunks stored for the document, so unchanged history could no longer be matched reliably. The result was that an incremental update could re-enter LLM extraction as if it were new content, defeating Delta Retain's cost-saving behavior.

Root cause

MemoryEngine.retain_batch_async applied transport/OOM splitting before orchestrator._try_delta_retain.

Delta Retain therefore compared partial transport slices instead of comparing the complete canonical replacement document with the previously stored logical document chunks.

Why this required a deeper fix

The motivating workload is a periodically retained chat transcript. The caller intentionally resubmits the full canonical transcript with update_mode=replace so retries are idempotent and the stored document remains correct. Only the newly changed logical chunks should incur extraction cost. Once a transcript crossed the transport batch limit, however, the order of operations silently broke that cost guarantee.

Fixing this safely was more involved than moving Delta Retain ahead of the splitter. That change crosses several retain invariants at once:

• Delta work must remain bounded by both token and chunk-count limits.
• A genuine full rewrite must retain the existing streaming and OOM fallback behavior.
• One submitted document must still produce one public result slot, even when its delta spans several extraction chunks.
• Memory Defense must screen once rather than emit duplicate work or events during fallback.
• A concurrent document update between comparison and commit must not be mistaken for a successful delta retain.
• Transactional outbox completion must still fire exactly once on every successful path, including exact batch-boundary cases.

The implementation therefore required tracing the full retain pipeline, reproducing the oversized replacement behavior with realistic multi-chunk documents, defining bounded eligibility for the pre-split delta path, and adding regression tests around every affected fallback and completion contract. The result preserves the existing resilience mechanisms while restoring Delta Retain's intended LLM-cost behavior for growing documents.

How this fixes it

For a single oversized update_mode=replace item with a document_id, the retain path now:

1. Runs Delta Retain against the complete replacement document before transport splitting.
2. Computes the changed/new logical chunks and their required context.
3. Uses the delta path only when both the changed token count fits retain_batch_tokens and the changed chunk count fits retain_chunk_batch_size.
4. Falls back to the existing bounded streaming/OOM-safe splitter for a genuine large rewrite or oversized delta.

Additional correctness fixes included in the same path:

• Multi-chunk delta unit IDs are flattened into the single public result slot associated with the submitted document.
• Memory Defense screens the replacement once; fallback paths do not duplicate screening or related events.
• A post-extraction document-hash race now propagates to bounded fallback instead of being reported as a successful delta commit.
• A full streaming batch is marked final when the total chunk count is exactly divisible by retain_chunk_batch_size, ensuring the transactional outbox callback is not skipped.

Safety properties

• Unchanged logical chunks do not enter extraction on a bounded delta.
• processed_content_tokens reflects changed work rather than the full replacement document.
• The stored document body remains the complete canonical replacement, not only the delta.
• Large rewrites retain the existing bounded streaming and OOM protection.
• update_mode=append behavior is unchanged.
• No API, schema, or migration changes are required.

Verification

Added regression coverage proving that:

• A one-chunk changed tail skips unchanged transcript history.
• A bounded delta spanning multiple logical chunks still skips unchanged history.
• Multi-chunk delta extraction preserves one result slot per submitted document.
• Token-count and chunk-count limits independently force bounded fallback.
• The outbox callback runs exactly once after delta success and after either fallback, including an exact chunk-batch boundary.
• The committed document remains the full replacement.
• Existing oversized Memory Defense redaction, async completion, webhook, and batch chunking behavior remains green.

Validation completed locally:

• 49 targeted tests passed.
• Ruff lint and format checks passed.
• ty check hindsight_api/ passed.
• Repository lint hook and pre-commit hooks passed.

[nicoloboschi]

Closing in favor of a focused replacement.

After tracing the retain pipeline on main, the diagnosis here doesn't hold up and the fix is aimed at a mechanism that isn't the one failing:

The stated failure mode is not what main does. The claim is that transport-sized splitting fragments the document before delta retain, so an append-only transcript re-extracts all unchanged history on every retain. But sub-batches 2..N never reach _try_delta_retain at all — the call is gated on is_first_batch (orchestrator.py:903). In the steady-state append case (slice 1 byte-identical), slice 1 diffs clean → _delta_metadata_only writes the full-body content_hash, so sub-batches 2..N compute a matching hash, take the is_recovery=True path, and skip already-stored chunks by set membership. Unchanged history is not re-extracted. A pure append with a stable prefix is already efficient on main.

The real cost blowup is narrower: it's a one-time full re-extraction when slice 1 itself differs (a head edit, or the first append that crosses the threshold), caused by the removed_indices deletion in the delta write path (orchestrator.py:2139) — not by transport/logical boundary misalignment. That's a cost issue, not a correctness one (final stored state is correct), and it's much smaller in scope than this PR.

The is_last outbox fix uses the wrong denominator. The dropped-outbox bug is real, but the fix here compares against total_chunks, whereas the consumer drains queued chunks (total minus skipped_total, orchestrator.py:1282-1289). On the is_recovery path — the normal path for oversized/append documents — skipped chunks mean the condition never trips, so the fix doesn't fire in the scenario it exists to serve. It also doesn't cover the zero-fact-final-batch case, where _process_db_batch returns before the callback site.

What we'll take forward in a focused PR:

• The genuinely-real bug: the transactional outbox / retain.completed webhook is silently dropped when the committed chunk count is an exact multiple of chunk_batch_size, or when the final batch extracts zero facts. Fix counted against queued chunks, plus the zero-fact path, with a red test.
• A test that actually measures extraction cost at production-realistic ratios (the existing suite asserts stored-state correctness, which is why this class of regression stays green) — including removing the if second is None: return escape hatch in test_delta_retain.py.

Thanks for the thorough write-up and repro scaffolding — the multi-chunk extraction-recording test harness here is reusable and we'll carry the idea over.