Skip to content

Commit 02cd0aa

Browse files
authored
fix(billing-cost-management-mcp-server): commit session-sql writes via conn.in_transaction (#4860)
The session-sql tool decided whether to commit using a keyword-prefix check on the query text (startswith INSERT/UPDATE/DELETE/CREATE/DROP/ ALTER). That check misses writes that sqlite3 still runs inside an implicit transaction: - comment-prefixed DML, e.g. "/* c */ INSERT ...", because .strip() does not remove a leading comment, so startswith('INSERT') is False - REPLACE statements, which are valid SQLite DML but absent from the keyword list For these, the implicit transaction was opened but never committed, so the write was silently rolled back when the connection closed while the tool still returned success. Replace the prefix check with `if conn.in_transaction: conn.commit()`, which reflects the engine's actual transaction state and commits exactly when there is uncommitted work. Add durability regression tests for a comment-prefixed INSERT and REPLACE (read back on a fresh connection) plus a plain INSERT/UPDATE baseline.
1 parent b936294 commit 02cd0aa

2 files changed

Lines changed: 99 additions & 9 deletions

File tree

‎src/billing-cost-management-mcp-server/awslabs/billing_cost_management_mcp_server/utilities/sql_utils.py‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1606,19 +1606,21 @@ async def execute_session_sql(
16061606
# Execute the main query
16071607
await ctx.info(f'Executing SQL query: {query[:100]}...')
16081608

1609-
# Get query type (read or write)
1610-
query_upper = query.strip().upper()
1611-
is_write_operation = any(
1612-
query_upper.startswith(cmd)
1613-
for cmd in ['INSERT', 'UPDATE', 'DELETE', 'CREATE', 'DROP', 'ALTER']
1614-
)
1615-
16161609
# Execute the query with validation
16171610
validate_sql_query(query)
16181611
cursor.execute(query)
16191612

1620-
# Commit if this is a write operation
1621-
if is_write_operation:
1613+
# Commit if the statement opened a transaction.
1614+
#
1615+
# We rely on sqlite3's own view of the transaction state rather than a
1616+
# keyword-prefix check on the query text. A prefix check misses writes
1617+
# that sqlite3 still runs inside an implicit transaction -- for example
1618+
# comment-prefixed DML ("/* c */ INSERT ...") and REPLACE statements --
1619+
# leaving the transaction uncommitted so the write is silently rolled
1620+
# back when the connection closes. conn.in_transaction reflects the
1621+
# actual engine state and commits exactly when there is something to
1622+
# commit.
1623+
if conn.in_transaction:
16221624
conn.commit()
16231625

16241626
# Get results

‎src/billing-cost-management-mcp-server/tests/utilities/test_sql_utils.py‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -598,6 +598,94 @@ async def test_execute_query_write_operation(self, mock_get_path, mock_connect,
598598
mock_connection.reset_mock()
599599
mock_cursor.reset_mock()
600600

601+
@patch('awslabs.billing_cost_management_mcp_server.utilities.sql_utils.get_session_db_path')
602+
async def test_comment_prefixed_insert_is_durable(
603+
self, mock_get_path, temp_db_path, mock_context
604+
):
605+
"""A comment-prefixed INSERT must persist after the connection closes.
606+
607+
Regression test for the keyword-prefix commit check: ``.strip()`` does
608+
not remove a leading SQL comment, so ``startswith('INSERT')`` was False
609+
and the write was never committed. sqlite3 still opens an implicit
610+
transaction for the statement, so the write was silently rolled back on
611+
close. We read the row back on a FRESH connection to assert durability,
612+
not just in-memory visibility.
613+
"""
614+
mock_get_path.return_value = temp_db_path
615+
616+
# Seed a table on its own connection so the INSERT below is the only
617+
# thing under test.
618+
seed = sqlite3.connect(temp_db_path)
619+
seed.execute('CREATE TABLE t (id INTEGER)')
620+
seed.execute('INSERT INTO t VALUES (101)')
621+
seed.commit()
622+
seed.close()
623+
624+
result = await execute_session_sql(mock_context, '/* c */ INSERT INTO t VALUES (2)')
625+
assert result['status'] == 'success'
626+
627+
# Fresh connection: only committed data is visible here.
628+
check = sqlite3.connect(temp_db_path)
629+
rows = check.execute('SELECT id FROM t ORDER BY id').fetchall()
630+
check.close()
631+
assert rows == [(2,), (101,)], f'comment-prefixed INSERT was not committed: {rows}'
632+
633+
@patch('awslabs.billing_cost_management_mcp_server.utilities.sql_utils.get_session_db_path')
634+
async def test_replace_statement_is_durable(self, mock_get_path, temp_db_path, mock_context):
635+
"""A REPLACE statement must persist after the connection closes.
636+
637+
Regression test: ``REPLACE`` is valid SQLite DML (synonym for
638+
``INSERT OR REPLACE``) but was absent from the keyword list, so the
639+
write was never committed and was rolled back on close.
640+
"""
641+
mock_get_path.return_value = temp_db_path
642+
643+
seed = sqlite3.connect(temp_db_path)
644+
seed.execute('CREATE TABLE t (id INTEGER PRIMARY KEY)')
645+
seed.execute('INSERT INTO t VALUES (101)')
646+
seed.commit()
647+
seed.close()
648+
649+
result = await execute_session_sql(mock_context, 'REPLACE INTO t VALUES (3)')
650+
assert result['status'] == 'success'
651+
652+
check = sqlite3.connect(temp_db_path)
653+
rows = check.execute('SELECT id FROM t ORDER BY id').fetchall()
654+
check.close()
655+
assert rows == [(3,), (101,)], f'REPLACE was not committed: {rows}'
656+
657+
@patch('awslabs.billing_cost_management_mcp_server.utilities.sql_utils.get_session_db_path')
658+
async def test_plain_write_operations_still_durable(
659+
self, mock_get_path, temp_db_path, mock_context
660+
):
661+
"""Baseline: plain INSERT/UPDATE still commit after the fix.
662+
663+
Guards against a regression where switching to ``conn.in_transaction``
664+
might stop committing ordinary writes. DELETE/DROP/ALTER/TRUNCATE are
665+
intentionally not covered because ``validate_sql_query`` rejects them.
666+
"""
667+
mock_get_path.return_value = temp_db_path
668+
669+
seed = sqlite3.connect(temp_db_path)
670+
seed.execute('CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT)')
671+
seed.commit()
672+
seed.close()
673+
674+
insert_result = await execute_session_sql(
675+
mock_context, "INSERT INTO t VALUES (1, 'alice')"
676+
)
677+
assert insert_result['status'] == 'success'
678+
679+
update_result = await execute_session_sql(
680+
mock_context, "UPDATE t SET name = 'bob' WHERE id = 1"
681+
)
682+
assert update_result['status'] == 'success'
683+
684+
check = sqlite3.connect(temp_db_path)
685+
rows = check.execute('SELECT id, name FROM t').fetchall()
686+
check.close()
687+
assert rows == [(1, 'bob')], f'plain write operations were not committed: {rows}'
688+
601689
@patch('sqlite3.connect')
602690
@patch('awslabs.billing_cost_management_mcp_server.utilities.sql_utils.get_session_db_path')
603691
async def test_execute_with_connection_error(self, mock_get_path, mock_connect, mock_context):

0 commit comments

Comments
 (0)