Close TD-22 chat delete transaction
This commit is contained in:
12
TECHDEBTS.md
12
TECHDEBTS.md
@@ -824,7 +824,17 @@ are scanned (check rebuild-from-files glob patterns ignore them).
|
|||||||
**Acceptance.** Concurrent-writer test produces two intact outcomes (last
|
**Acceptance.** Concurrent-writer test produces two intact outcomes (last
|
||||||
write wins, no corruption); rebuild file globs ignore temp files.
|
write wins, no corruption); rebuild file globs ignore temp files.
|
||||||
|
|
||||||
### TD-22: Wrap `delete_chat_conversation` in a transaction
|
### TD-22: Wrap `delete_chat_conversation` in a transaction ✅ DONE (2026-06-12)
|
||||||
|
|
||||||
|
**Status: implemented.** `BDS.AI.Chat.delete_chat_conversation/1` now performs
|
||||||
|
message deletion and conversation deletion inside a single `Repo.transaction`.
|
||||||
|
If the second step fails, the whole delete rolls back and both the
|
||||||
|
conversation and its messages remain intact. The regression test injects a
|
||||||
|
mid-delete failure through a local test hook and proves the rollback by
|
||||||
|
asserting both rows still exist afterward. The nearby audit requested by this
|
||||||
|
TD found no equivalent multi-statement write outlier in `lib/bds/ai/catalog.ex`
|
||||||
|
or `lib/bds/scripts.ex`: catalog refresh already runs in a transaction and the
|
||||||
|
script operations are single-row writes.
|
||||||
|
|
||||||
**Context.** `chat.ex` deletes all messages (`Repo.delete_all`) then the
|
**Context.** `chat.ex` deletes all messages (`Repo.delete_all`) then the
|
||||||
conversation (`Repo.delete`) without a transaction — a failure in between
|
conversation (`Repo.delete`) without a transaction — a failure in between
|
||||||
|
|||||||
@@ -106,12 +106,24 @@ defmodule BDS.AI.Chat do
|
|||||||
{:error, :not_found}
|
{:error, :not_found}
|
||||||
|
|
||||||
%ChatConversation{} = conversation ->
|
%ChatConversation{} = conversation ->
|
||||||
|
Repo.transaction(fn ->
|
||||||
Repo.delete_all(
|
Repo.delete_all(
|
||||||
from message in ChatMessage, where: message.conversation_id == ^conversation_id
|
from message in ChatMessage, where: message.conversation_id == ^conversation_id
|
||||||
)
|
)
|
||||||
|
|
||||||
|
case delete_chat_conversation_test_hook(conversation_id) do
|
||||||
|
:ok ->
|
||||||
case Repo.delete(conversation) do
|
case Repo.delete(conversation) do
|
||||||
{:ok, _conversation} -> {:ok, :deleted}
|
{:ok, _conversation} -> :ok
|
||||||
|
{:error, reason} -> Repo.rollback(reason)
|
||||||
|
end
|
||||||
|
|
||||||
|
{:error, reason} ->
|
||||||
|
Repo.rollback(reason)
|
||||||
|
end
|
||||||
|
end)
|
||||||
|
|> case do
|
||||||
|
{:ok, :ok} -> {:ok, :deleted}
|
||||||
{:error, reason} -> {:error, reason}
|
{:error, reason} -> {:error, reason}
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
@@ -610,6 +622,13 @@ defmodule BDS.AI.Chat do
|
|||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
defp delete_chat_conversation_test_hook(conversation_id) do
|
||||||
|
case Application.get_env(:bds, :chat_delete_conversation_test_hook) do
|
||||||
|
hook when is_function(hook, 1) -> hook.(conversation_id)
|
||||||
|
_other -> :ok
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
defp persist_assistant_response(conversation_id, response) do
|
defp persist_assistant_response(conversation_id, response) do
|
||||||
usage = normalize_usage(response.usage)
|
usage = normalize_usage(response.usage)
|
||||||
|
|
||||||
|
|||||||
@@ -1641,6 +1641,36 @@ defmodule BDS.AITest do
|
|||||||
end)
|
end)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
test "delete_chat_conversation rolls back message deletion when the conversation delete fails" do
|
||||||
|
previous_hook = Application.get_env(:bds, :chat_delete_conversation_test_hook)
|
||||||
|
|
||||||
|
on_exit(fn ->
|
||||||
|
if is_nil(previous_hook) do
|
||||||
|
Application.delete_env(:bds, :chat_delete_conversation_test_hook)
|
||||||
|
else
|
||||||
|
Application.put_env(:bds, :chat_delete_conversation_test_hook, previous_hook)
|
||||||
|
end
|
||||||
|
end)
|
||||||
|
|
||||||
|
assert {:ok, conversation} = BDS.AI.start_chat(%{title: "Rollback Chat", model: "gpt-4o-mini"})
|
||||||
|
|
||||||
|
message =
|
||||||
|
Repo.insert!(%BDS.AI.ChatMessage{
|
||||||
|
conversation_id: conversation.id,
|
||||||
|
role: :user,
|
||||||
|
content: "keep me",
|
||||||
|
created_at: Persistence.now_ms()
|
||||||
|
})
|
||||||
|
|
||||||
|
Application.put_env(:bds, :chat_delete_conversation_test_hook, fn _conversation_id ->
|
||||||
|
{:error, :forced_failure}
|
||||||
|
end)
|
||||||
|
|
||||||
|
assert {:error, :forced_failure} = BDS.AI.delete_chat_conversation(conversation.id)
|
||||||
|
assert Repo.get(BDS.AI.ChatConversation, conversation.id)
|
||||||
|
assert Repo.get(BDS.AI.ChatMessage, message.id)
|
||||||
|
end
|
||||||
|
|
||||||
test "non-stat chat tools expose concrete project data" do
|
test "non-stat chat tools expose concrete project data" do
|
||||||
{:ok, project} = create_project_fixture("Concrete Tools")
|
{:ok, project} = create_project_fixture("Concrete Tools")
|
||||||
%{post: post, media: media} = seed_project_content(project.id)
|
%{post: post, media: media} = seed_project_content(project.id)
|
||||||
|
|||||||
Reference in New Issue
Block a user