Skip to content

Commit c6fc673

Browse files
authored
fix(conversations): reject reprocessing a soft-deleted conversation (#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. -->
2 parents eed01bf + 62a9ad9 commit c6fc673

4 files changed

Lines changed: 90 additions & 3 deletions

File tree

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
{
22
"files": {
3-
"backend/database/conversations.py": 1601,
3+
"backend/database/conversations.py": 1616,
44
"backend/database/users.py": 1943
55
},
66
"raise_justifications": {
7-
"backend/database/conversations.py": "Public shared-chat uses the existing conversation storage format for bounded, safe transcript decoding; extraction would duplicate crypto/storage invariants. +21 for the #10033 merge-target eligibility predicate and pure closest-match selector beside the query they filter."
7+
"backend/database/conversations.py": "Public shared-chat uses the existing conversation storage format for bounded, safe transcript decoding; extraction would duplicate crypto/storage invariants. +15 for the shared is_soft_deleted tombstone-eligibility predicate that eligible_merge_target and the reprocess guard converge on (#10119/#10262)."
88
},
99
"threshold": 1500
1010
}

backend/database/conversations.py

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1524,6 +1524,21 @@ def _store(transaction) -> bool:
15241524
# ********************************
15251525

15261526

1527+
def is_soft_deleted(conversation: Optional[dict]) -> bool:
1528+
"""Whether a conversation is a soft-deleted tombstone.
1529+
1530+
A tombstone is invisible to the user, so any content operation that reads it
1531+
and writes derived state — merging its segments, or reprocessing to
1532+
regenerate structured data, action items, memories and embeddings —
1533+
resurrects data the user deleted. Such operations must reject a tombstone.
1534+
1535+
Shared predicate behind that contract (sync #10119 via `eligible_merge_target`,
1536+
merge #10262, reprocess). Deliberately distinct from `discarded`, which stays
1537+
revivable: the merge and reprocess paths intentionally revive a discarded row.
1538+
"""
1539+
return bool(conversation) and bool(conversation.get('deleted'))
1540+
1541+
15271542
def eligible_merge_target(conversation: Optional[dict]) -> bool:
15281543
"""Whether synced audio may merge into this conversation (#10033).
15291544
@@ -1532,7 +1547,7 @@ def eligible_merge_target(conversation: Optional[dict]) -> bool:
15321547
conversation". Discarded rows stay eligible; the merge path reprocesses
15331548
and revives them.
15341549
"""
1535-
return bool(conversation) and not conversation.get('deleted')
1550+
return bool(conversation) and not is_soft_deleted(conversation)
15361551

15371552

15381553
def select_closest_conversation(conversations, start_timestamp: int, end_timestamp: int) -> Optional[dict]:

backend/routers/conversations.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,14 @@ def reprocess_conversation(
318318
:return: The updated conversation after reprocessing.
319319
"""
320320
conversation = _get_valid_conversation_by_id(uid, conversation_id)
321+
# Reprocess force-processes a *discarded* conversation to revive it, but a
322+
# soft-deleted tombstone is invisible to the user and must not be reprocessed:
323+
# process_conversation would regenerate structured data, action items, memories
324+
# and embeddings from content the user deleted, resurrecting it. Same
325+
# tombstone-eligibility contract as sync (#10119) and merge (#10262). Checked
326+
# on the raw doc because the Conversation model does not carry `deleted`.
327+
if conversations_db.is_soft_deleted(conversation):
328+
raise HTTPException(status_code=404, detail="Conversation not found")
321329
conversation = deserialize_conversation(conversation)
322330
if not language_code:
323331
language_code = conversation.language or 'en'
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
"""Reprocess must reject a soft-deleted conversation.
2+
3+
`POST /v1/conversations/{id}/reprocess` fetches through `_get_valid_conversation_by_id`,
4+
which does not filter soft-deleted tombstones (`get_conversation` returns them and the
5+
helper only 404s on a missing doc). Reprocessing runs `process_conversation` with
6+
`force_process=True`, regenerating structured data, action items, memories and
7+
embeddings — so reprocessing a tombstone resurrects content the user deleted.
8+
9+
The guard rejects a *deleted* conversation while still allowing a *discarded* one,
10+
which reprocess intentionally revives — the same tombstone-eligibility contract as
11+
sync (#10119) and merge (#10262), via the shared `is_soft_deleted` predicate.
12+
"""
13+
14+
from types import SimpleNamespace
15+
from unittest.mock import patch
16+
17+
import pytest
18+
from fastapi import HTTPException
19+
20+
import routers.conversations as conv_router
21+
from database.conversations import eligible_merge_target, is_soft_deleted
22+
23+
24+
class TestIsSoftDeleted:
25+
def test_deleted_is_tombstoned(self):
26+
assert is_soft_deleted({'id': 'c1', 'deleted': True}) is True
27+
28+
def test_discarded_is_not_tombstoned(self):
29+
# Discarded stays revivable — reprocess/merge intentionally revive it.
30+
assert is_soft_deleted({'id': 'c1', 'discarded': True}) is False
31+
32+
def test_plain_conversation_is_not_tombstoned(self):
33+
assert is_soft_deleted({'id': 'c1'}) is False
34+
35+
def test_none_is_not_tombstoned(self):
36+
assert is_soft_deleted(None) is False
37+
38+
def test_eligible_merge_target_still_excludes_only_deleted(self):
39+
# The refactor onto is_soft_deleted must be behaviour-preserving.
40+
assert eligible_merge_target({'id': 'c1', 'deleted': True}) is False
41+
assert eligible_merge_target({'id': 'c1', 'discarded': True}) is True
42+
assert eligible_merge_target(None) is False
43+
44+
45+
class TestReprocessTombstoneGuard:
46+
def test_reprocess_rejects_soft_deleted_conversation(self):
47+
deleted = {'id': 'c1', 'deleted': True, 'status': 'completed'}
48+
with patch.object(conv_router, '_get_valid_conversation_by_id', return_value=deleted), patch.object(
49+
conv_router, 'process_conversation'
50+
) as process:
51+
with pytest.raises(HTTPException) as exc:
52+
conv_router.reprocess_conversation(conversation_id='c1', uid='u1')
53+
assert exc.value.status_code == 404
54+
process.assert_not_called() # deleted content never re-enters the pipeline
55+
56+
def test_reprocess_still_allows_a_discarded_conversation(self):
57+
discarded = {'id': 'c1', 'discarded': True, 'status': 'completed'}
58+
fake_conv = SimpleNamespace(language='en')
59+
with patch.object(conv_router, '_get_valid_conversation_by_id', return_value=discarded), patch.object(
60+
conv_router, 'deserialize_conversation', return_value=fake_conv
61+
), patch.object(conv_router, 'process_conversation', return_value=fake_conv) as process:
62+
result = conv_router.reprocess_conversation(conversation_id='c1', uid='u1')
63+
process.assert_called_once()
64+
assert result is fake_conv

0 commit comments

Comments
 (0)