Fix room deletion 500ing on attachments, integrations, or references (#55)

delete_room() only ever cleaned up Message and RoomMembership rows, but none of the FK constraints referencing a room (or its messages) are declared ON DELETE CASCADE at the DB level -- confirmed across every migration that added one. Any room that ever had an image/file attachment, an incoming webhook, an outgoing event subscription, or was ever #referenced from a message in a *different* room (the one that originally surfaced this as a message_room_references FK violation in production) couldn't be deleted at all. Now explicitly cleans up, in dependency order: message mentions, reactions, and room-references (both the message-id and room-id directions), the messages themselves, then room-scoped images/files (including unlinking the actual stored files after a successful commit, not just their DB rows) and incoming webhooks/event subscriptions, before removing memberships and the room. Added a test reproducing the full scenario -- attachments, integrations, and a cross-room reference all on one room -- that would have 500'd before this fix, plus a sanity check that deleting the room doesn't touch the unrelated room that referenced it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-08-19 15:41:40 -06:00
parent 222ca49355
commit 8a461ebb13
2 changed files with 189 additions and 4 deletions
@@ -1,13 +1,27 @@
 import uuid

-from sqlalchemy import delete, func, select
+from sqlalchemy import delete, func, or_, select
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy.ext.asyncio import AsyncSession
 from sqlalchemy.orm import selectinload

-from app.models import Message, MessageMention, Room, RoomMembership, RoomRole, User
+from app.models import (
+    EventSubscription,
+    Message,
+    MessageFile,
+    MessageImage,
+    MessageMention,
+    MessageReaction,
+    MessageRoomReference,
+    Room,
+    RoomMembership,
+    RoomRole,
+    User,
+    WebhookIncoming,
+)
 from app.schemas.room import RoomCreate, RoomUpdate
 from app.services.email_service import send_email
+from app.storage import delete_file


 class DuplicateRoomError(Exception):
@@ -203,12 +217,60 @@ async def update_room(db: AsyncSession, room: Room, data: RoomUpdate) -> Room:

 async def delete_room(db: AsyncSession, room: Room) -> None:
    # Explicit deletes rather than relying on ORM cascade + eager-loading —
-    # simpler and more predictable in async code.
+    # simpler and more predictable in async code. None of these FKs are
+    # declared ON DELETE CASCADE at the DB level (confirmed across every
+    # migration that added one), so every table referencing this room --
+    # directly, or indirectly via one of its messages -- has to be cleared
+    # explicitly, in dependency order, or the final room delete 500s on
+    # whichever one it happens to hit first (originally surfaced as a
+    # message_room_references FK violation, but every table below has the
+    # exact same gap).
+    room_message_ids = select(Message.id).where(Message.room_id == room.id).scalar_subquery()
+
+    # Message-child tables first -- these reference message_id, so they'd
+    # block deleting this room's own messages otherwise.
+    await db.execute(delete(MessageMention).where(MessageMention.message_id.in_(room_message_ids)))
+    await db.execute(delete(MessageReaction).where(MessageReaction.message_id.in_(room_message_ids)))
+    # Both directions: a reference *from* one of this room's own messages,
+    # and a reference *to* this room from a message in a completely
+    # different room (the case that originally surfaced this bug).
+    await db.execute(
+        delete(MessageRoomReference).where(
+            or_(
+                MessageRoomReference.message_id.in_(room_message_ids),
+                MessageRoomReference.room_id == room.id,
+            )
+        )
+    )
+
+    # Fetch attachment storage filenames before deleting their rows -- the
+    # actual files are only unlinked after a successful commit below, so a
+    # rolled-back transaction never leaves us having destroyed something we
+    # couldn't get back.
+    image_filenames = (
+        await db.execute(select(MessageImage.storage_filename).where(MessageImage.room_id == room.id))
+    ).scalars().all()
+    file_filenames = (
+        await db.execute(select(MessageFile.storage_filename).where(MessageFile.room_id == room.id))
+    ).scalars().all()
+
+    # Messages themselves, now that nothing still references them.
    await db.execute(delete(Message).where(Message.room_id == room.id))
+
+    # Room-scoped attachments/integrations -- messages.image_id/file_id
+    # reference these, so they must come after the message delete above.
+    await db.execute(delete(MessageImage).where(MessageImage.room_id == room.id))
+    await db.execute(delete(MessageFile).where(MessageFile.room_id == room.id))
+    await db.execute(delete(WebhookIncoming).where(WebhookIncoming.room_id == room.id))
+    await db.execute(delete(EventSubscription).where(EventSubscription.room_id == room.id))
+
    await db.execute(delete(RoomMembership).where(RoomMembership.room_id == room.id))
    await db.delete(room)
    await db.commit()

+    for filename in (*image_filenames, *file_filenames):
+        delete_file(filename)
+

 async def list_room_members(db: AsyncSession, room_id: uuid.UUID) -> list[RoomMembership]:
    result = await db.execute(
@@ -1,11 +1,52 @@
+import io
 import uuid

+from PIL import Image
 from sqlalchemy import select

-from app.models import Room, RoomMembership, RoomRole, User
+from app.models import (
+    EventSubscription,
+    MessageFile,
+    MessageImage,
+    MessageRoomReference,
+    Room,
+    RoomMembership,
+    RoomRole,
+    User,
+    WebhookIncoming,
+)
+from app.schemas.user import UserCreate
+from app.services.auth_service import register_user
+from app.storage import UPLOADS_DIR
 from tests.conftest import login_as, register_and_login


+def _unique(prefix: str) -> str:
+    return f"{prefix}-{uuid.uuid4().hex[:8]}"
+
+
+def _png_bytes() -> bytes:
+    buf = io.BytesIO()
+    Image.new("RGB", (10, 10), color=(255, 0, 0)).save(buf, format="PNG")
+    return buf.getvalue()
+
+
+def _register_ws(ws_client, username: str) -> dict:
+    async def _seed():
+        async with ws_client.session_factory() as session:
+            await register_user(
+                session,
+                UserCreate(username=username, email=f"{username}@example.com", password="password123"),
+            )
+
+    ws_client.portal.call(_seed)
+    resp = ws_client.post(
+        "/api/auth/login", json={"username_or_email": username, "password": "password123"}
+    )
+    assert resp.status_code == 200, resp.text
+    return resp.json()
+
+
 async def test_create_room_requires_auth(client):
    resp = await client.post("/api/rooms", json={"name": "general"})
    assert resp.status_code == 401
@@ -210,6 +251,88 @@ async def test_delete_room_owner_only(client, db_session):
    assert result.scalar_one_or_none() is None


+def test_delete_room_with_attachments_integrations_and_cross_room_reference(ws_client):
+    # Reproduces #55: every table below has a room_id (or message_id, for a
+    # room being deleted) foreign key with no ON DELETE CASCADE at the DB
+    # level, so a room that's ever had an attachment, an integration, or
+    # been #referenced from another room's message used to 500 on delete.
+    # This sets up one of each and confirms delete_room cleans all of them
+    # up, not just whichever one originally surfaced the bug.
+    _register_ws(ws_client, _unique("alice"))
+    target_name = _unique("target-room")
+    other_name = _unique("other-room")
+    target = ws_client.post("/api/rooms", json={"name": target_name}).json()
+    other = ws_client.post("/api/rooms", json={"name": other_name}).json()
+
+    image_id = ws_client.post(
+        f"/api/rooms/{target['id']}/images",
+        files={"file": ("test.png", _png_bytes(), "image/png")},
+    ).json()["id"]
+    file_upload = ws_client.post(
+        f"/api/rooms/{target['id']}/files",
+        files={"file": ("report.pdf", b"%PDF-1.4 not real", "application/pdf")},
+    ).json()
+    file_id = file_upload["id"]
+
+    webhook = ws_client.post(f"/api/rooms/{target['id']}/webhooks/incoming", json={}).json()
+    sub = ws_client.post(
+        f"/api/rooms/{target['id']}/event-subscriptions",
+        json={"event_types": ["message.created"], "target_url": "http://8.8.8.8/hook"},
+    ).json()
+
+    with ws_client.websocket_connect("/ws/chat") as ws:
+        ws.send_json({"type": "join", "room_id": target["id"]})
+        assert ws.receive_json()["type"] == "joined"
+        ws.send_json({"type": "message", "room_id": target["id"], "image_id": image_id})
+        ws.receive_json()
+        ws.send_json({"type": "message", "room_id": target["id"], "file_id": file_id})
+        ws.receive_json()
+
+        ws.send_json({"type": "join", "room_id": other["id"]})
+        assert ws.receive_json()["type"] == "joined"
+        # References target-room from a message that belongs to a
+        # *different* room -- the direction that originally 500'd, since
+        # it's keyed by the referenced room's id, not the message's room.
+        ws.send_json({"type": "message", "room_id": other["id"], "content": f"check out #{target_name}"})
+        other_message = ws.receive_json()
+
+    async def _storage_filenames():
+        async with ws_client.session_factory() as session:
+            image = await session.get(MessageImage, uuid.UUID(image_id))
+            file = await session.get(MessageFile, uuid.UUID(file_id))
+            return image.storage_filename, file.storage_filename
+
+    image_filename, file_filename = ws_client.portal.call(_storage_filenames)
+    assert (UPLOADS_DIR / image_filename).exists()
+    assert (UPLOADS_DIR / file_filename).exists()
+
+    resp = ws_client.delete(f"/api/rooms/{target['id']}")
+    assert resp.status_code == 204, resp.text
+
+    async def _assert_cleaned_up():
+        async with ws_client.session_factory() as session:
+            target_id = uuid.UUID(target["id"])
+            for model in (MessageImage, MessageFile, WebhookIncoming, EventSubscription):
+                result = await session.execute(select(model).where(model.room_id == target_id))
+                assert result.scalar_one_or_none() is None, model.__name__
+            result = await session.execute(
+                select(MessageRoomReference).where(MessageRoomReference.room_id == target_id)
+            )
+            assert result.scalar_one_or_none() is None
+
+    ws_client.portal.call(_assert_cleaned_up)
+
+    # The physical files were unlinked too, not just the DB rows.
+    assert not (UPLOADS_DIR / image_filename).exists()
+    assert not (UPLOADS_DIR / file_filename).exists()
+
+    # Sanity: deleting target-room didn't touch the unrelated other-room or
+    # its message -- the cross-room reference cleanup is scoped correctly.
+    history = ws_client.get(f"/api/rooms/{other['id']}/messages").json()
+    assert any(m["id"] == other_message["id"] for m in history)
+    assert webhook["id"] and sub["id"]  # created successfully, not otherwise asserted above
+
+
 async def test_leave_room(client, db_session):
    await register_and_login(client, db_session, username="alice")
    room_id = (await client.post("/api/rooms", json={"name": "general"})).json()["id"]