fix(backend): fence deferred conversation enrichment ownership

[aryanorastar]

Summary

• make deferred=true the compare-and-swap ownership token for lazy conversation enrichment, so concurrent first opens cannot both launch LLM processing
• atomically publish failed enrichment as completed + deferred=true, allowing the UI to stop polling and a later open to retry
• keep both paired transitions behind the conversation lifecycle owner and extend the static lifecycle-write tripwire to cover deferred
• document the deferred desktop lifecycle contract

Root cause

The first-open transaction checked only status=processing. Two requests could both read the deferred row, then each transaction could succeed even after the first had cleared deferred, duplicating summaries, tasks, memories, app results, and LLM cost.

Failure recovery was also split between a raw flag update and a separate status transition. Besides permitting a partial write to strand a row in processing, it produced completed + deferred=true even though reacquisition rejected every completed row, so the documented retry path could never run.

Fix

The storage transaction now claims only an explicit deferred row and writes status=processing, deferred=false, and the renewed admission lease together. It also accepts the explicit completed + deferred=true retry terminal. A second transaction owned by the lifecycle service atomically re-arms a failed attempt as completed + deferred=true; the router no longer performs lifecycle writes directly and no longer silently drops recovery errors.

Proof

Regression-first

The new contracts failed against current main with four failures:

• a second caller could reacquire processing + deferred=false
• completed + deferred=true could not be retried
• no atomic failure-recovery primitive existed
• the router still bypassed the lifecycle owner

Automated

• cd backend && .venv/bin/python -m pytest tests/unit/test_conversation_finalization_jobs.py tests/routers/test_conversations_processing_rollback.py tests/unit/test_check_conversation_lifecycle_writes.py -q — 64 passed
• python3 backend/scripts/check_conversation_lifecycle_writes.py — passed
• Firestore emulator: pytest backend/tests/unit/test_stale_processing_emulator_concurrency.py -k deferred_reacquire -q — 2 passed, including a real two-open contention race with exactly one winner
• OMI_PR_BODY_FILE=/tmp/omi-pr-12286.md make preflight — 26 checks passed

Full-suite baseline audit

backend/test.sh ran all 935 selected unit-test files. It reported 13 unrelated failing files under concurrent load. Twelve reproduced on a detached clean origin/main worktree with the same runner (module-stub/import drift, STT expectation drift, BYOK expectation drift, and the runtime-manifest CPU timing guard). The thirteenth, test_share_email_routes.py, passed all 21 tests when rerun alone on this branch, confirming a load-sensitive suite failure. None of those files or their production surfaces are changed here.

Scope

Backend and lifecycle documentation only; no iOS or Android code changes. This repairs the backend lifecycle root associated with stuck-processing reports in #11295 and complements the UI timeout treatment in #11856.

Fixes #12286

Failure-Class: FC-split-mutation-authority

Product-Invariants: none

Line-Count-Exception: backend/database/conversation_finalization_jobs.py | 2081 -> 2112 | keep the paired Firestore transaction primitives beside the existing deferred ownership transition

Line-Count-Exception: backend/routers/conversations.py | 1917 -> 1926 | surface atomic recovery loss and exceptions at the existing deferred enrichment call site

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aryanorastar]

Verification evidence:

• Regression-first contracts produced 4 failures on the old behavior: duplicate reacquisition, non-retryable completed/deferred state, missing atomic recovery, and a router lifecycle bypass.
• Focused unit/static suite: 64 passed.
• Real Firestore-emulator contention: 2 passed; two simultaneous first-open claims produced exactly one enrichment owner.
• Repository preflight: 26 checks passed.
• The bounded pre-push gate also reran the four changed backend test files, formatting, lifecycle guards, OpenAPI compatibility, and runtime-image contracts successfully.

The complete backend suite was additionally audited against a clean origin/main worktree. Twelve of its 13 reported failing files reproduced unchanged on the base branch; the remaining load-sensitive email-route file passed 21/21 alone on this branch. No affected production surface is part of this change.

No physical-device verification is required: this PR changes backend Firestore lifecycle ownership only and contains no iOS or Android code.

[aryanorastar]

The Backend unit suite red here is not this diff — this PR touches conversation enrichment and no byok or chat code.

Six unit files fail, all inherited from main, and they split into two different problems:

Five are mechanical, and #12292 fixes them. Hand-written module stubs that never gained names the production code started importing:

cannot import name 'get_byok_uid' from 'utils.byok' (unknown location)
cannot import name 'get_current_context' from 'utils.llm.usage_tracker' (unknown location)
cannot import name 'CHAT_AGENT_ROUTE_DIRECT' from 'utils.llm.gateway_client' (unknown location)
No module named 'utils.llm.gateway_client'; 'utils.llm' is not a package

(unknown location) is the giveaway — the module in sys.modules is a stub with no spec origin, so the name is missing from the stand-in rather than the package. 108 tests pass after adding what the real modules already export.

One is not. test_byok_security.py (8 failures) patches utils.subscription.get_byok_keys, which request_has_llm_byok_key no longer calls — it moved to get_byok_uid / get_cached_byok_state / get_byok_key. Repointing would bind the patch to a function the code under test doesn't use, so those need new assertions against the current contract. Traced in #12289; I'm not guessing at BYOK semantics.

So this PR goes green once #12292 lands, except for test_byok_security, which needs its owner.

[Git-on-my-level]

Reviewed the full diff at ecb75b6 — this is a well-constructed fix. The ownership-fencing analysis is right, and the tests are the kind this lifecycle deserves. File by file:

• backend/database/conversation_finalization_jobs.py — _reacquire_deferred_processing_txn now requires deferred is True (a real compare-and-swap token) and writes status/deferred/processing_admitted_at in one transaction, so the second of two concurrent first opens fails closed instead of double-launching enrichment. The new _recover_deferred_processing_failure_txn publishing completed + deferred=true atomically closes the stranding window the old two-write router recovery had. Both fail closed on discarded.
• backend/routers/conversations.py — the failure path now routes through lifecycle_service.recover_deferred_processing_failure and logs lost ownership / recovery exceptions instead of the old raw update_conversation({'deferred': True}) + complete() + silent pass. Correct per the Epic: durable listen→conversation lifecycle — single mutation owner + durable recording sessions (parent of #9516, #9351) #9687 single-writer rule.
• backend/utils/conversations/lifecycle.py — the new recover_deferred_processing_failure wrapper keeps the lifecycle service the single authority for the paired status/flag transition; docstrings match the implemented semantics.
• backend/scripts/check_conversation_lifecycle_writes.py — adding deferred to LIFECYCLE_FIELDS closes the exact loophole the old router code used. Hygiene passes on this head, so no other writer trips the extended tripwire.
• backend/tests/routers/test_conversations_processing_rollback.py — the new test asserts recovery runs through the lifecycle owner and that no raw update_conversation fires; good regression-first shape.
• backend/tests/unit/test_check_conversation_lifecycle_writes.py — encodes the split-write rejection so the tripwire extension can't silently regress.
• backend/tests/unit/test_conversation_finalization_jobs.py — covers claim-from-processing, reopen-from-completed-retry-terminal, discarded/failed fail-closed, and the recovery matrix; asserting exact update payloads is the right level of strictness.
• backend/tests/unit/test_stale_processing_emulator_concurrency.py — real emulator contention with two barrier-synchronized claims asserting sorted(results) == [False, True]; this is the proof the CAS actually fences under Firestore transaction retry.

On the red Backend unit suite: verified it is not from this diff. Every failure on this head is in chat/byok/quota test files this PR doesn't touch (test_byok_security, test_chat_file_upload_unsupported, test_chat_generate_reply_stateless, test_chat_quota*, test_chat_stream_error_fallback, test_chat_first_proactive_router), caused by stale hand-written module stubs missing names the real modules export (get_current_context in utils.llm.usage_tracker, CHAT_AGENT_ROUTE_DIRECT in utils.llm.gateway_client, get_byok_keys in utils.subscription — all present in main's production code). #12292 addresses the mechanical subset, and its run confirms those import errors clear. None of this PR's own tests fail, and the Hermetic Merge Gate, Hermetic Backend E2E, and Listen Pusher Stack Gauntlet are green on this head.

One semantics question for a maintainer (non-blocking): _recover_deferred_processing_failure_txn also accepts completed + deferred=false and re-arms it as retryable. As far as I can trace, that state should be unreachable while a lazy-row owner is failing (the admitted-at generation fence keeps the bare-processing sweep off a live-leased row), and re-arming an externally-completed row would make the next open re-run enrichment. The unit test encodes this as intended — if "retry even if someone else completed it" is the desired behavior, this is fine; just confirming it's a deliberate choice.

Docs/agent-guidance note: docs/doc/developer/backend/listen_pusher_pipeline.mdx adds the deferred lifecycle contract, including "routers must not split either transition into independent field writes." That sentence steers future coding agents (and humans) in this repo away from reintroducing split lifecycle writes — accurate against the code as reviewed and consistent with the tripwire this PR extends. Flagging it so the contract is consciously ratified rather than landing silently.

Leaving for human maintainer review before merge: the lifecycle state-machine choice above, and merging against a green (or harness-fixed) unit suite — this touches the core conversation data path, so it should get maintainer eyes on top of the automated pass.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[kodjima33]

Confirmed the race is real and not yet fixed on main (current _reacquire_deferred_processing_txn only checks status==processing, not deferred) — fixes #12286 legitimately. Holding merge: Backend unit suite check is failing.

[aryanorastar]

Yes — still in progress. The lifecycle fix itself is complete and approved; I am clearing the inherited backend-suite gate.

On the maintainer semantics question: accepting completed + deferred=false in _recover_deferred_processing_failure_txn is deliberate and necessary for the current processing order, not merely tolerance for an unreachable state.

process_conversation() persists the conversation as completed at process_conversation.py:1945-1954 before it runs the derived-effect bundle. A later app, memory, vector, action-item, receipt, or other post-persistence effect can still raise back through _run_enrichment. At that point this same lazy-enrichment owner legitimately observes completed + deferred=false; recovery must publish completed + deferred=true so a later open can retry the incomplete enrichment. Rejecting completed here would leave a partially enriched conversation looking terminal with no retry path.

The compare-and-swap still fails closed for discarded, deferred=true, and every status outside {processing, completed}. A different owner that already re-armed the row therefore wins, while this stale recovery returns False. The unit-test case for completed is intentionally pinning the post-persistence-failure path.

The documentation statement is also intentional: both the claim (processing + deferred=false + lease) and recovery (completed + deferred=true) are paired lifecycle transitions, so routers must not split either across independent writes.

For CI, the red remains outside this diff. The authoritative backend job currently has three upstream pieces: the stale module stubs in #12292, the current BYOK-contract assertions in #12302, and two base-branch unused imports in utils/stt/streaming.py. I’m consolidating that repair so the suite can become genuinely green, then this approved PR can be refreshed against it.