fix(chat): rollback session on pre-stream failure log commit error

[fredifo]

Orca-Code-Review — push 1

Severity	Count
P0	0
P1	0
P2	0
P3	0

✅ no blocking findings

When _log_pre_stream_failure fails to commit the request log row (e.g. transient SQLite busy error), the request-scoped session is left dirty with the pending INSERT still attached. Any subsequent DB operation on that session then fails with InvalidRequestError — a secondary failure caused by our own error handling, not the original upstream error.

The streaming _finalize path already handles this correctly (its _commit_row does rollback on failure), but the pre-stream path was missing the same guard.

Fix: add await db.rollback() in the except clause so the session is clean for any subsequent operations (streaming _finalize, blocking path commit).

Files changed:

• app/routes/chat.py — rollback in _log_pre_stream_failure except clause
• tests/unit/test_pre_stream_rollback.py — regression test

[orcacode-review]

🐳 OrcaCode Review

✅ No findings — nothing to flag in this PR. Great work!

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 376 calls · 22.8M tokens · 98% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[xizhuomengcontin]

Verified end-to-end against main before merging — the fix is correct and the failure mode it guards is real.

I confirmed the premise with a real SQLAlchemy session: drop requests_log so the commit inside _log_pre_stream_failure genuinely fails, then touch the same session again.

	next query	next write
without the rollback	PendingRollbackError: This Session's transaction has been rolled back due to a previous exception	same
with the rollback	OK	OK

Merging. Two notes for follow-up, neither blocking:

1. The test doesn't guard the fix. tests/unit/test_pre_stream_rollback.py never imports production code — it re-implements the rollback in the test body and asserts on the mock:

db.add(log_row)
try:
    await db.commit()
except Exception:
    try:
        await db.rollback()
    except Exception:
        pass
db.rollback.assert_awaited_once()

I deleted the await db.rollback() block from app/routes/chat.py and re-ran that file: 1 passed. So if someone removes this fix later, nothing goes red. Worth replacing with a case that actually drives _log_pre_stream_failure — e.g. patch the session so commit() raises, call execute_chat with a streaming request that fails upstream, and assert rollback was awaited on the real session.

2. The guard isn't reachable in today's flow. Both call sites (chat.py:568 and :579) are immediately followed by raise HTTPException, and the request-scoped session is closed right after; _finalize only runs on the success path. End-to-end (dropped requests_log + upstream 503 + stream=true) main and this branch return identically, with only request_log_commit_failed in the log and no PendingRollbackError on either.

That doesn't argue against merging — it's a correct guard, and it becomes load-bearing the moment any DB call is added before that raise. Just flagging that the current behaviour difference is zero, so the note above about the test matters more than it looks.