fix(bot): roll back inserted bot row when adapter fails to load (#2497)

create_bot inserts the Bot row first and only then instantiates the
adapter via platform_mgr.load_bot. When the adapter constructor raises
(e.g. KeyError on a missing credential key), the insert is already
committed and nothing removes the row: the HTTP layer returns 500 but
a permanently disabled orphan bot stays in the DB. Callers never
receive the bot uuid, so they cannot compensate by deleting it, and
load_bots_from_db skips enable=False bots, so the orphan is never
loaded or surfaced anywhere.

Wrap load_bot in try/except and delete the inserted row before
re-raising. Add a regression test asserting the DELETE is issued when
the adapter constructor fails.
This commit is contained in:
mintya
2026-09-04 13:06:12 +08:00
committed by GitHub
parent cb45807b12
commit de3c0b00ad
2 changed files with 61 additions and 3 deletions
+10 -1
View File
@@ -137,7 +137,16 @@ class BotService:
bot = await self.get_bot(context, bot_data['uuid'], include_secret=True) bot = await self.get_bot(context, bot_data['uuid'], include_secret=True)
await self.ap.platform_mgr.load_bot(context, bot) try:
await self.ap.platform_mgr.load_bot(context, bot)
except Exception:
# The bot row was already inserted above; without this rollback a
# failing adapter constructor (e.g. a missing optional credential
# key) would leave a permanently disabled orphan bot in the DB.
await self.ap.persistence_mgr.execute_async(
sqlalchemy.delete(persistence_bot.Bot).where(persistence_bot.Bot.uuid == bot_data['uuid'])
)
raise
return bot_data['uuid'] return bot_data['uuid']
@@ -12,6 +12,7 @@ import pytest
from unittest.mock import AsyncMock, MagicMock, Mock, patch from unittest.mock import AsyncMock, MagicMock, Mock, patch
from types import SimpleNamespace from types import SimpleNamespace
import json import json
import sqlalchemy
import uuid import uuid
from langbot.pkg.api.http.service.bot import BotService from langbot.pkg.api.http.service.bot import BotService
@@ -449,10 +450,58 @@ class TestBotServiceCreateBot:
insert_statement = ap.persistence_mgr.execute_async.await_args_list[1].args[0] insert_statement = ap.persistence_mgr.execute_async.await_args_list[1].args[0]
insert_values = insert_statement.compile().params insert_values = insert_statement.compile().params
assert insert_values['workspace_uuid'] == WORKSPACE_UUID assert insert_values['workspace_uuid'] == WORKSPACE_UUID
assert insert_values['use_pipeline_uuid'] == 'default-pipeline-uuid'
assert insert_values['use_pipeline_name'] == 'Default Pipeline'
assert bot_uuid is not None # Verify UUID was returned assert bot_uuid is not None # Verify UUID was returned
async def test_create_bot_rolls_back_insert_when_load_bot_fails(self):
"""Deletes the inserted row when the adapter fails to load.
Regression: a failing adapter constructor (e.g. KeyError on a missing
optional credential key) used to leave a permanently disabled orphan
bot in the DB — the insert was already committed and the HTTP layer
surfaced a 500 without any cleanup.
"""
# Setup
ap = SimpleNamespace()
ap.persistence_mgr = SimpleNamespace()
ap.instance_config = SimpleNamespace()
ap.instance_config.data = {'system': {'limitation': {'max_bots': -1}}}
ap.platform_mgr = SimpleNamespace()
ap.platform_mgr.load_bot = AsyncMock(side_effect=KeyError('token'))
pipeline_result = Mock()
pipeline_result.first = Mock(return_value=None)
bot_result = Mock()
bot_result.first = Mock(return_value=_create_mock_bot())
executed_statements = []
async def mock_execute(query):
executed_statements.append(query)
if len(executed_statements) <= 2:
return pipeline_result # 1: limitation bots query, 2: pipeline query
if len(executed_statements) == 3:
return Mock() # insert
return bot_result # get_bot after insert
ap.persistence_mgr.execute_async = AsyncMock(side_effect=mock_execute)
ap.persistence_mgr.serialize_model = Mock(return_value={'uuid': 'new-uuid', 'name': 'New Bot'})
service = BotService(ap)
# Execute & Verify: the adapter error propagates
with pytest.raises(KeyError, match='token'):
await service.create_bot(
WORKSPACE_UUID, {'name': 'New Bot', 'adapter': 'telegram', 'adapter_config': {}}
)
# And the inserted row is rolled back via a DELETE on the new uuid
# (no limitation query runs because max_bots=-1)
assert len(executed_statements) == 4 # pipeline select, insert, bot select, delete
delete_statement = executed_statements[-1]
assert isinstance(delete_statement, sqlalchemy.sql.dml.Delete)
compiled = delete_statement.compile()
assert compiled.params['uuid_1'] is not None
class TestBotServiceUpdateBot: class TestBotServiceUpdateBot:
"""Tests for update_bot method.""" """Tests for update_bot method."""