fix(tracing): disable autoflush on async_sessionmaker to prevent span FK violations - #14229
Conversation
… FK violations SQLAlchemy's default autoflush=True can fire an implicit flush between individual session.merge() calls inside _flush_to_database(). When that happens, a child span may reach the database before its parent, producing an IntegrityError on span.parent_span_id -> span.id even though topological_sort_spans() has already sorted the spans in the correct order. Fix: pass autoflush=False to both async_sessionmaker() call-sites in DatabaseService.__init__(). Writes are never lost because session_scope() calls await session.commit() explicitly on exit. Adds 4 regression tests in test_autoflush_disabled.py: - test_async_session_maker_has_autoflush_false (guards factory config) - test_default_autoflush_is_true_without_fix (documents unsafe baseline) - test_topo_sorted_merge_with_autoflush_false_does_not_raise (E2E w/ FK) - test_session_maker_factory_produces_autoflush_false_sessions Fixes: DSLF-524
WalkthroughAsync database sessionmakers now use ChangesAsync session autoflush fix
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested labels: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/backend/tests/unit/services/database/test_autoflush_disabled.py (1)
134-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant asyncio marker.
pytest-asyncioauto mode already runs these async test methods. Based on learnings, pytest-asyncio is configured withasyncio_mode = 'auto'; tests should avoid unnecessarypytest.mark.asynciodecorators.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/tests/unit/services/database/test_autoflush_disabled.py` around lines 134 - 135, Remove the redundant `@pytest.mark.asyncio` decorator from the TestAutoflushFKViolationRegression class, relying on the configured pytest-asyncio auto mode while leaving the async test methods unchanged.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/tests/unit/services/database/test_autoflush_disabled.py`:
- Around line 58-64: Update
src/backend/tests/unit/services/database/test_autoflush_disabled.py at lines
58-64, 182-186, and 214-219 to test the real DatabaseService rather than
independently recreated async_sessionmaker instances: use
DatabaseService.async_session_maker for the initial-session assertions, run the
merge regression through that service-created factory, and call
DatabaseService.reload_engine() before asserting the replacement factory still
has autoflush=False.
---
Nitpick comments:
In `@src/backend/tests/unit/services/database/test_autoflush_disabled.py`:
- Around line 134-135: Remove the redundant `@pytest.mark.asyncio` decorator from
the TestAutoflushFKViolationRegression class, relying on the configured
pytest-asyncio auto mode while leaving the async test methods unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ffced404-848d-40e1-9ba0-e5ccfb0025bb
📒 Files selected for processing (2)
src/backend/base/langflow/services/database/service.pysrc/backend/tests/unit/services/database/test_autoflush_disabled.py
| # Replicate the exact factory construction used in DatabaseService.__init__ | ||
| factory = async_sessionmaker( | ||
| engine, | ||
| class_=SQLModelAsyncSession, | ||
| expire_on_commit=False, | ||
| autoflush=False, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test DatabaseService rather than recreated session factories.
These tests can all pass if DatabaseService.__init__() or reload_engine() stops passing autoflush=False. Use a real DatabaseService fixture, assert a session from its initial factory, call reload_engine(), then assert a session from the rebuilt factory.
src/backend/tests/unit/services/database/test_autoflush_disabled.py#L58-L64: replace the standalone factory with the initialDatabaseService.async_session_maker.src/backend/tests/unit/services/database/test_autoflush_disabled.py#L182-L186: run the merge regression through the service-created session factory.src/backend/tests/unit/services/database/test_autoflush_disabled.py#L214-L219: exerciseDatabaseService.reload_engine()and assert its replacement factory retainsautoflush=False.
As per coding guidelines, new backend bug fixes must include tests that cover the changed behavior rather than placeholders.
📍 Affects 1 file
src/backend/tests/unit/services/database/test_autoflush_disabled.py#L58-L64(this comment)src/backend/tests/unit/services/database/test_autoflush_disabled.py#L182-L186src/backend/tests/unit/services/database/test_autoflush_disabled.py#L214-L219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/tests/unit/services/database/test_autoflush_disabled.py` around
lines 58 - 64, Update
src/backend/tests/unit/services/database/test_autoflush_disabled.py at lines
58-64, 182-186, and 214-219 to test the real DatabaseService rather than
independently recreated async_sessionmaker instances: use
DatabaseService.async_session_maker for the initial-session assertions, run the
merge regression through that service-created factory, and call
DatabaseService.reload_engine() before asserting the replacement factory still
has autoflush=False.
Source: Coding guidelines
Summary
SQLAlchemy's default
autoflush=Truecauses implicit flushes between individualsession.merge()calls inside_flush_to_database(). When this fires, a child span is written to the DB before its parent span, producing a FK violation onspan.parent_span_id → span.id— even thoughtopological_sort_spans()(added in #12242) has already sorted spans into the correct order.The topological sort was correct all along. The problem is SQLAlchemy's autoflush defeating it mid-loop.
Root cause
In
DatabaseService.__init__(), bothasync_sessionmaker()call-sites use the defaultautoflush=True. Inside_flush_to_database()innative.py:When autoflush fires between two
merge()calls, a child span whose parent hasn't been merged yet lands in the DB first →IntegrityError.Fix
Pass
autoflush=Falseto bothasync_sessionmaker()call-sites inDatabaseService.__init__(). No writes are lost becausesession_scope()callsawait session.commit()explicitly on exit.Tests
4 regression tests added in
test_autoflush_disabled.py:test_async_session_maker_has_autoflush_falsetest_default_autoflush_is_true_without_fixtest_topo_sorted_merge_with_autoflush_false_does_not_raisetest_session_maker_factory_produces_autoflush_false_sessionsAll 4 pass locally.
Relationship to prior work
topological_sort_spans()to fix this — it is correct but SQLAlchemy autoflush defeats it at runtimeFixes: DSLF-524
Summary by CodeRabbit
Bug Fixes
Tests