Skip to content

fix(listen): recover orphaned in_progress conversations at session boundaries#10060

Open
aryanorastar wants to merge 6 commits into
BasedHardware:mainfrom
aryanorastar:fix/9809-recover-stale-in-progress
Open

fix(listen): recover orphaned in_progress conversations at session boundaries#10060
aryanorastar wants to merge 6 commits into
BasedHardware:mainfrom
aryanorastar:fix/9809-recover-stale-in-progress

Conversation

@aryanorastar

@aryanorastar aryanorastar commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • process_pending (the session-boundary recovery pass that already re-dispatches stuck processing conversations, Finalize pending conversations when a listen session ends inside the retry window #9960) now also recovers orphaned in_progress conversations — the largest stuck bucket in Bug: conversations get stuck in processing states and never finalize #9809 (~161 rows in production, some months old, several with real transcripts, all invisible to users because the web app filters to processing,completed).
  • Recovery routes each orphan through process_conversation — the exact call a live conversation timeout makes: content goes through the durable finalization job seam; empty rows are deleted by delete_empty_recording_conversation. No new recovery mechanism, no new state.
  • Orphanhood is proven by idle time, not status alone: any live session refreshes finished_at on every segment and its lifecycle loop processes an idle conversation within the ~2-minute conversation timeout, so an hour of silence means no session owns the row — including a session on another device. The session's own current conversation is excluded explicitly.
  • The fetch is an equality-only status query (no composite index, no firestore.indexes.json change), age-cut client-side on a bounded read, oldest-first, capped at 10 per session so a large backlog spreads across reconnects instead of fanning dozens of LLM finalizations out of one.

Addresses the in_progress bucket of #9809.

Root cause

A listen session that dies between conversation creation and finalization leaves the row in_progress with no automatic owner: the lifecycle loop only watches the current conversation, process_pending only re-dispatched processing, and the durable-job reconciler only replays rows that already have a finalization job. A recovery path does exist — POST /v1/conversations (the app's force-processing action) falls back to get_in_progress_conversation, a status query — but it is user-initiated only and limit(1) newest-first, so an orphan behind a newer in_progress row never qualifies, and a user who never taps force-processing never triggers it. (Corrected from an earlier version of this description that overstated this as "nothing ever asked" — thanks to beastoin for the push-back.) The manual /v1/conversations/{id}/finalize workaround in #9809 proves the content is intact and the pipeline works when asked.

Failure class

Failure-Class: none

No registered class on main covers this. The honest classification is FC-session-scoped-durable-work — the class #10044 proposes (durable work must not silently share the fate of the session that dispatched it; this instance is the admission-side gap, where the session died before durable finalization intent existed). If #10044 lands first, I'll update this declaration to cite it.

Durable guard

  • Recovery reuses the two existing owner seams (process_conversation → durable finalization intent / empty-row deletion) rather than adding a parallel path; the revision-fenced create_or_get_finalization_intent makes re-recovery idempotent.
  • select_stale_in_progress is a pure decision function (age proof, missing/bad finished_at excluded as unprovable, oldest-first, bounded) with direct tests.
  • Named follow-ups, deliberately out of scope: merging/failed recovery (different machinery — reprocess is an inline LLM call), and a server-side sweep for users who never open another session (needs a cross-user collection-group index and spend controls; belongs with the monitoring contract in [P1] Make monitoring delivery and telemetry coverage an explicit production contract #9587).

Product invariants affected

none (verified with scripts/pr-preflight --suggest)

Validation

  • Old-code regression check (controller change stashed): pytest tests/unit/test_listen_process_pending_shutdown.py2 failed (orphans never recovered); new code → 6 passed.
  • Related suites: pytest tests/unit/test_listen_process_pending_shutdown.py tests/unit/test_lazy_conversation_processing.py tests/unit/test_listen_fallback_removal.py36 passed.
  • Tests execute the real process_pending/recover_stale_in_progress through the same host seam Finalize pending conversations when a listen session ends inside the retry window #9960's regression tests established (no patching, no sys.modules mutation), plus direct tests on select_stale_in_progress covering the age filter, unprovable-clock exclusion, ordering, and the batch bound.
  • make preflight (local lane, this PR body) → all selected checks pass.
  • black --line-length 120 --skip-string-normalization clean on the three changed files.
  • Live local stack e2e: new cloud-orphan-recovery scenario in the listen-pusher gauntlet (backend/testing/listen_pusher_stack/run.sh) — a seeded 2h-idle orphan row, a real listen session, real backend/pusher/finalization-worker processes, Firestore emulator, Redis, loopback Cloud Tasks. The session-boundary pass admits the orphan into a durable finalization job, the worker completes it, and the row lands completed with content. Full gauntlet (all six suites) passed locally. On pre-fix code the scenario fails by construction (no recovery pass exists; job admission times out).

Not exercised live: a real device session against production Firestore (no credentials in this environment). The recovery path from process_conversation down is the same code every live conversation timeout already exercises in production; this PR only adds the orphan selection in front of it.

Review in cubic

…undaries

Conversations from sessions that died before processing sit in
in_progress forever, invisible to users (the web app filters to
processing/completed). Production data in BasedHardware#9809: ~161 stuck in_progress
rows, some months old, several with real transcripts. Session-boundary
recovery already re-dispatches stuck processing rows (BasedHardware#9960) but never
looked at in_progress.

process_pending now also routes orphaned in_progress conversations
through process_conversation — the exact call a live timeout makes:
content goes through the durable finalization seam, empty rows are
deleted. Orphanhood is proven by idle time: any live session refreshes
finished_at continuously and processes an idle conversation within the
~2-minute conversation timeout, so an hour of silence means no session
owns the row, including on another device. The session's own current
conversation is excluded explicitly.

The fetch is an equality-only status query (no new index) with the age
cut client-side, oldest-first and bounded to 10 per session so a large
backlog spreads across sessions instead of stampeding the pipeline.

Covers the in_progress bucket of BasedHardware#9809; stuck processing rows are
already re-dispatched here, and merging/failed recovery plus a
server-side sweep for users who never reconnect are named follow-ups.

Failure-Class: none

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

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

@Git-on-my-level Git-on-my-level added needs-maintainer-review Needs a human maintainer to sign off before merge security-review Touches auth, provider routing, secrets, or security-sensitive surfaces labels Jul 20, 2026
@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks for the careful write-up and tests here. This looks like the right recovery shape to me: stale in_progress rows are selected conservatively, the session's current conversation is explicitly skipped, and recovered rows are sent back through the existing process_conversation / durable finalization path instead of adding a second finalization mechanism.

I verified the focused regression suite locally on the PR head:

OMI_ENV_STAGE=offline PROVIDER_MODE=offline uv run pytest tests/unit/test_listen_process_pending_shutdown.py -q → 6 passed.

Because this touches listen-session recovery for user conversation data and can dispatch finalization work, I’m not formally approving it from automation. A human maintainer should still review the operational assumptions before merge, especially that the 1-hour stale threshold and bounded status == in_progress read are appropriate for the current production backlog and expected future backlog size. No code changes requested from my side.


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

@aryanorastar

Copy link
Copy Markdown
Contributor Author

Thanks @Git-on-my-level. Some concrete numbers on the two operational questions, for whoever does the human pass:

Bounded read vs backlog size: the query is capped at .limit(200) documents, read once per listen session inside process_pending (so at most one bounded read per session start, after the existing processing re-dispatch read). The worst per-user backlog observed in #9809 is ~161 in_progress rows — inside the cap. Typical users hold 0–2 rows, so steady-state read amplification is negligible (relevant to the concerns in #9889). If a user ever exceeds 200 stuck rows, recovery still converges: each session drains up to 10 oldest rows, shrinking the set the next read sees.

1-hour threshold: any session that owns an in_progress row refreshes finished_at on every transcript segment, and its lifecycle loop force-processes an idle conversation at the ~120s conversation timeout. One hour is therefore ~30× the longest a legitimately-owned row can sit idle, including a session on another device. Rows with a missing or non-datetime finished_at are excluded as unprovable rather than recovered.

Drain rate: ≤10 recoveries per session start, oldest first, means the reported 161-row backlog clears over ~17 sessions for that user — spread out by design so one reconnect never fans out dozens of LLM finalizations. The manual /v1/conversations/{id}/finalize route remains available for anyone who wants their backlog cleared immediately.

Happy to adjust the threshold or the caps if the maintainer pass prefers different numbers — both are single constants (STALE_IN_PROGRESS_RECOVERY_AGE_SECONDS, STALE_IN_PROGRESS_RECOVERY_BATCH).

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed by automated PR reviewer — approving; holding merge (see notes).

@aryanorastar

Copy link
Copy Markdown
Contributor Author

Fixed the failing PR Metadata Preflight: main's ratchet baseline for database/conversations.py moved under this PR (now 1580 on main); raised to the exact post-change count (1616) with justification and merged current main. Local preflight 21/21 green, recovery suite still 6/6. Ready when you are @kodjima33.

…tack gauntlet

New cloud-orphan-recovery scenario in the listen-pusher gauntlet: seed an
in_progress row no session owns (2h idle, content intact — production
orphans predate any single cause, so the row is seeded directly while the
recovery session itself runs the real listener), open a real session, and
prove its process_pending pass admits the orphan into a durable
finalization job, the loopback Cloud Tasks delivery completes it through
the real worker, and the conversation lands completed with content.

The session stays open while waiting: process_pending is a finite task
started at session begin with a built-in 7s delay, and teardown cancels
finite tasks — closing immediately is exactly how the scenario first
failed.

Verification: full gauntlet (all six suites) passed locally against real
backend/pusher/worker processes, Firestore emulator, Redis, and the
strict loopback Cloud Tasks client. On pre-fix code the scenario fails
by construction: no recovery pass exists, so the orphan's job admission
times out.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Re-reviewed the current head (8b108d5) after the ratchet-baseline update. I still don’t see a code-level blocker in the recovery path: the new stale in_progress selection is conservative, skips the live session’s current conversation, and routes recovery through the existing process_conversation / durable finalization seam rather than inventing a parallel processor.

Verification I ran locally on this head with secrets stripped/offline mode:

OMI_ENV_STAGE=offline PROVIDER_MODE=offline .venv/bin/pytest tests/unit/test_listen_process_pending_shutdown.py -q → 6 passed.

I also reproduced the two files named by the current Backend unit suite failure via test.sh; those failures are in tests/unit/test_ws_b_short_term_lifecycle.py validation paths and a test_desktop_transcribe.py duration guard, not in the listen recovery test added here. I’m therefore not requesting PR changes from the listen recovery diff itself.

Keeping this as human-maintainer review rather than automation approval because it touches user conversation recovery and can dispatch finalization jobs. The maintainer pass should still sign off on the operational caps/thresholds (3600s, 10 per session, bounded status == in_progress read) before merge.


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

@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level @undivisible This PR is fully approved. I've pushed a fix for the pre-existing Backend unit suite timebomb that was failing CI (the short-term lifecycle tests hardcoded June 20, 2026 and hit +30 days expiry validation failures today). CI should be green shortly. Could you please merge when you have a chance?

@aryanorastar

Copy link
Copy Markdown
Contributor Author

Thanks for the re-review. The operational caps are all single constants for easy adjustment: STALE_IN_PROGRESS_RECOVERY_AGE_SECONDS = 3600, STALE_IN_PROGRESS_RECOVERY_BATCH = 10, and the equality-only status == in_progress read. Happy to tune any of them if the maintainer pass prefers different numbers.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

@kodjima33 approved and green. The operational caps flagged for the human pass are all single constants for easy tuning — STALE_IN_PROGRESS_RECOVERY_AGE_SECONDS = 3600, STALE_IN_PROGRESS_RECOVERY_BATCH = 10, and the equality-only status == in_progress read. Happy to adjust any, but nothing's blocking. Ready to merge when you lift the hold.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level need human response — requesting David's merge sign-off. Approved by @kodjima33, all checks green, no conflicts. The item flagged for a human is the operational caps on orphan recovery — all single constants (STALE_IN_PROGRESS_RECOVERY_AGE_SECONDS = 3600, STALE_IN_PROGRESS_RECOVERY_BATCH = 10), trivially adjustable. Ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-maintainer-review Needs a human maintainer to sign off before merge security-review Touches auth, provider routing, secrets, or security-sensitive surfaces

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants