fix(sync): never merge synced audio into a soft-deleted conversation#10119
Conversation
get_closest_conversation_to_timestamps matched any conversation in its ±2-minute window — including deleted: True tombstones — so offline audio recorded near a conversation the user later deleted merged its segments into the tombstone and vanished: uploads reported success, the job completed, and no visible conversation ever appeared (BasedHardware#10033's never-creates-a-conversation symptom). The auto-sync target-attach path had the same hole through its direct get_conversation fetch. The closest-match choice is now a pure selector (select_closest_conversation) over eligible merge targets, with one shared predicate (eligible_merge_target) excluding soft-deleted rows; the target-attach path consults the same predicate and falls back to the filtered timestamp lookup. A window containing only tombstones now creates a fresh conversation instead of feeding the dead one. Discarded rows stay eligible — the merge path already reprocesses and revives them. Regression tests (fail on old code — the selector did not exist and the inline loop accepted tombstones): deleted-only window yields None, a closer deleted row loses to a live one, boundary-distance choice and the predicate matrix, plus a labeled source tripwire that the target-attach path gates through the shared predicate. Fixes the never-creates-a-conversation symptom of BasedHardware#10033 (server side); the stuck-queue symptom is BasedHardware#10098. Failure-Class: none Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for chasing this — the shape of the fix looks solid to me. Extracting a shared eligible_merge_target predicate and using it both for timestamp matching and direct target_conversation_id attachment addresses the tombstone-merge failure without changing the intended discarded-row revival path.
I’m not formally approving because this touches conversation deletion / synced-audio merge behavior and the current head still has failing CI (Backend unit suite, plus cancelled/skipped repo formatting/hygiene jobs in the rollup), so this should get maintainer review before merge.
Validation I did:
- Reviewed the changed merge-target selection and sync attach path.
- Ran the new focused backend test through
backend/test.sh;tests/unit/test_sync_merge_target_selection.pypassed locally (4 passed). - Checked the failing backend CI log: it reports failures in
tests/unit/test_ws_b_short_term_lifecycle.py, which looks unrelated to this diff, but the red required check still needs a maintainer decision or rerun/fix before merge.
One small follow-up I’d prefer before merge if convenient: replace the source-text tripwire in test_sync_target_attach_consults_the_shared_predicate with a behavior-level test around the sync target attach fallback if there is an existing lightweight way to stub process_segment. I don’t think that needs to block if the maintainers are comfortable with the current coverage, since the core selector/predicate behavior is directly tested.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
@Git-on-my-level I've replaced the source-text tripwire with a behavior-level test using a mocked |
…it duration guard The behavior test added for the BasedHardware#10119 review (drives the real process_segment target-attach fallback) is the sole heavy test in its file, so it amortizes the sync-pipeline import graph into its call phase and exceeds the 0.12s fast-unit CPU budget — the same file-isolation structural cost the allowlist already grandfathers for test_sync_silent_failure.py::TestProcessSegmentReal. Add its node id alongside, per the allowlist's documented purpose. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done — replaced the source-text tripwire with a behavioral test ( This head also carries a one-line fix for the unrelated |
kodjima33
left a comment
There was a problem hiding this comment.
Backend bug fix: exclude soft-deleted/tombstone conversations from sync merge-target selection so offline audio near a deleted conversation no longer vanishes (fixes #10033 server side); shared eligibility predicate + test, CI green, conf 5/5
The user-initiated merge path validated locked + completed but never soft-delete. get_conversation returns tombstones unfiltered and POST /v1/conversations/merge only 404s on a missing (None) doc, so a deleted conversation id — passed by an API client, or arriving via a delete-vs-merge race — flowed straight through and merged. That resurrects deleted content into a new visible conversation: the inverse of the tombstone contract the sync merge path already enforces (conversations_db.eligible_merge_target, BasedHardware#10119). Add the soft-delete rejection to validate_merge_compatibility, mirroring the existing locked/completed gates, plus a hermetic regression test. Verified: python -m pytest tests/unit/test_merge_validation.py -> 40 passed. Failure-Class: none Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
reprocess_conversation fetches through _get_valid_conversation_by_id, which does not filter soft-deleted tombstones (get_conversation returns them; the helper only 404s on a missing doc), then runs process_conversation with force_process=True — regenerating structured data, action items, memories and embeddings. Reprocessing a tombstone (via a direct API call, or a delete-vs- reprocess race) therefore resurrects content the user deleted back into their memories and derived data. Reject a deleted conversation while still allowing a discarded one, which reprocess intentionally revives. Third instance of the tombstone-eligibility contract (sync BasedHardware#10119, merge BasedHardware#10262); extract the shared is_soft_deleted predicate and converge eligible_merge_target onto it. Verified: pytest test_reprocess_tombstone_guard.py test_sync_merge_target_selection.py test_merge_validation.py -> 49 passed (behaviour-preserving for eligible_merge_target). Failure-Class: none Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The user-initiated merge path validated locked + completed but never soft-delete. get_conversation returns tombstones unfiltered and POST /v1/conversations/merge only 404s on a missing (None) doc, so a deleted conversation id — passed by an API client, or arriving via a delete-vs-merge race — flowed straight through and merged. That resurrects deleted content into a new visible conversation: the inverse of the tombstone contract the sync merge path already enforces (conversations_db.eligible_merge_target, BasedHardware#10119). Add the soft-delete rejection to validate_merge_compatibility, mirroring the existing locked/completed gates, plus a hermetic regression test. Verified: python -m pytest tests/unit/test_merge_validation.py -> 40 passed. Failure-Class: none Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The user-initiated merge path validated locked + completed but never soft-delete. get_conversation returns tombstones unfiltered and POST /v1/conversations/merge only 404s on a missing (None) doc, so a deleted conversation id — passed by an API client, or arriving via a delete-vs-merge race — flowed straight through and merged. That resurrects deleted content into a new visible conversation: the inverse of the tombstone contract the sync merge path already enforces (conversations_db.eligible_merge_target, BasedHardware#10119). Add the soft-delete rejection to validate_merge_compatibility, mirroring the existing locked/completed gates, plus a hermetic regression test. Verified: python -m pytest tests/unit/test_merge_validation.py -> 40 passed. Failure-Class: none Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
reprocess_conversation fetches through _get_valid_conversation_by_id, which does not filter soft-deleted tombstones (get_conversation returns them; the helper only 404s on a missing doc), then runs process_conversation with force_process=True — regenerating structured data, action items, memories and embeddings. Reprocessing a tombstone (via a direct API call, or a delete-vs- reprocess race) therefore resurrects content the user deleted back into their memories and derived data. Reject a deleted conversation while still allowing a discarded one, which reprocess intentionally revives. Third instance of the tombstone-eligibility contract (sync BasedHardware#10119, merge BasedHardware#10262); extract the shared is_soft_deleted predicate and converge eligible_merge_target onto it. Verified: pytest test_reprocess_tombstone_guard.py test_sync_merge_target_selection.py test_merge_validation.py -> 49 passed (behaviour-preserving for eligible_merge_target). Failure-Class: none Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…10270) ## What A soft-delete/tombstone data-integrity gap: `POST /v1/conversations/{id}/reprocess` regenerates a soft-deleted conversation's derived data — resurrecting content the user deleted. `reprocess_conversation` fetches through `_get_valid_conversation_by_id`, which does not filter soft-deleted tombstones (`get_conversation` returns them; the helper only 404s on a missing doc). It then runs `process_conversation(force_process=True, is_reprocess=True)`, which regenerates structured data, action items, **memories** and embeddings. So reprocessing a deleted conversation — via a direct API call, or a delete-vs-reprocess race — re-derives the user's memories / action items / embeddings from content they deleted. ## Fix Reject a **deleted** conversation in `reprocess_conversation` (404), while still allowing a **discarded** one — which reprocess intentionally revives (per its docstring, and the `eligible_merge_target` contract). Checked on the raw doc because the `Conversation` model does not carry `deleted`. ## Repairing the failure-class boundary This is the **third instance** of one tombstone-eligibility contract — *a soft-deleted tombstone must not have content operations applied, or deleted data resurfaces*: - sync merge target — #10119 (`eligible_merge_target`) - user-initiated merge — #10262 - reprocess — this PR Per AGENTS.md ("if two or more recent fixes share the cause, add a reusable guard surface"), rather than a third point-fix I extracted the shared **`is_soft_deleted`** predicate and converged `eligible_merge_target` onto it (behaviour-preserving), so the reprocess guard and the sync guard share one definition. #10262's inline merge check can adopt it too — a small follow-up rather than re-touching that merged path here; formalizing a named failure class is a reasonable registry follow-up. ## Tests (hermetic) - `test_reprocess_tombstone_guard.py` — the predicate (deleted → tombstone; discarded / plain / None → not), reprocess **rejects** a deleted conversation (404, `process_conversation` never called), and still **allows** a discarded one. - Existing `eligible_merge_target` and merge-validation tests stay green (behaviour-preserving refactor). **49 passed.** ## Product invariants affected none Failure-Class: none <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/BasedHardware/omi/pull/10270?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
) ## What The user-initiated conversation merge (`POST /v1/conversations/merge`) validated that sources are unlocked and completed, but never that they are not **soft-deleted**. `conversations_db.get_conversation` returns tombstones unfiltered, and the endpoint only 404s on a missing (`None`) doc. So a soft-deleted conversation id — passed directly by an API client, or arriving via a delete-vs-merge race — passed both the endpoint's `None` check and `validate_merge_compatibility`, and was merged. Merging a tombstone **resurrects deleted content into a new visible conversation**: the merge reads the deleted source's transcript/photos/audio, builds a new visible conversation from them, then re-deletes the sources. That is the inverse of the tombstone contract the sync merge path already enforces (`conversations_db.eligible_merge_target`, #10119 — "a soft-deleted tombstone must never absorb new segments"). ## Fix Reject soft-deleted sources in `validate_merge_compatibility`, mirroring the existing locked/completed gates. This is the direct sibling of #10119: that guarded the automatic sync merge target; this guards the user-initiated merge path. Narrow and behavior-preserving for every non-deleted source. ## Verification - `python -m pytest tests/unit/test_merge_validation.py` → **40 passed** (2 new: rejects a deleted source; still allows non-deleted sources). The new `test_rejects_deleted_conversation` fails without the guard (validation would return `ok=True`). ## Product invariants affected none Failure-Class: none <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/BasedHardware/omi/pull/10262?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
Summary
get_closest_conversation_to_timestampsmatched any row in its ±2-minute window — includingdeleted: Truetombstones — so audio recorded near a conversation the user later deleted merged into the tombstone and vanished: upload succeeded, the sync job completed, and no visible conversation ever appeared. The auto-sync target-attach path had the same hole through its directget_conversationfetch.select_closest_conversation) over eligible merge targets with one shared predicate (eligible_merge_target); the target-attach path consults the same predicate and falls back to the filtered timestamp lookup. A window containing only tombstones now creates a fresh conversation.reprocessing (discarded) after segment merge).Fixes the never-creates-a-conversation symptom of #10033 (server side); the stuck-queue symptom is #10098.
Root cause
Merge-target selection had no eligibility policy — only geometry. Any row satisfying the timestamp window qualified, deleted or not. Soft-deleted rows keep their
started_at/finished_at, so a recording made around the time of a later-deleted conversation reliably lands in its window; the merge writes segments into a document no reader will ever show.updated_memoriesthen reports an id the app cannot display, which is exactly the user-visible shape reported in #10033: recordings sync "successfully" and nothing appears.Failure class
Failure-Class: none
No registered class covers write-path target selection ignoring row lifecycle state. Single-instance fix with the decision extracted to one owner; if a sibling surfaces (e.g. another merge path accepting tombstones), that recurrence is the signal to register a class.
Product invariants affected
none (verified with
scripts/pr-preflight --suggest)Validation
tests/unit/test_sync_merge_target_selection.py).None(→ fresh conversation upstream); a closer deleted row loses to a farther live one; boundary-distance choice; predicate matrix (live/deleted/None/discarded-stays-eligible); labeled static tripwire that the target-attach path gates through the shared predicate.tests/unit/test_sync_ordered_assignment.py+tests/unit/test_sync_silent_failure.py→ 38 passed.make preflight(local lane, this PR body) → all selected checks pass.blackclean on all changed files.backend/database/conversations.pyfrozen-size baseline raised 1580 → 1601 (+21, predicate + selector) with justification. Heads-up: fix(listen): recover orphaned in_progress conversations at session boundaries #10060 raises the same baseline (to 1616 on its branch) — whichever merges second reconciles the count; both raises are additive and independent.Not exercised live: a real device deleting a conversation then syncing overlapping offline audio (no device here). The decision seam is driven directly by the tests; the merge/reprocess machinery around it is unchanged.
Out of scope, named