Repository navigation
fix: keep session stream alive during SQLite contention - #293
Conversation
pilipilisbot
left a comment
There was a problem hiding this comment.
Thanks, the narrow fix makes sense: catching transient SQLITE_BUSY/SQLITE_LOCKED inside the SSE generator is the right place, because once the stream has started FastAPI cannot turn the exception into the normal JSON/503 response. I checked the Sentry context from #292, the changed generator/tests, and ran the focused backend SSE tests locally.
Blocking this for integration, not for the local fix itself: GitHub currently reports the PR as CONFLICTING, and current main has moved the session stream onto DashboardQueries as the dashboard read-model boundary. When rebasing, please adapt this retry around the current DashboardQueries-based reads instead of restoring the older module-level job_session_events/job_session_transcript path. That keeps the read-only dashboard contract from #284 intact while preserving this contention behavior.
Validation performed:
- pytest -q tests/test_backend.py -k dashboard_sse -> 5 passed, 71 deselected
- CI on the PR head is green for dashboard and pytest 3.11/3.12, but the PR still needs rebase/conflict resolution before merge.
6307847 to
e045cc4
Compare
pilipilisbot
left a comment
There was a problem hiding this comment.
Approved on the rebased head e045cc4.
I re-checked the Sentry failure mode from #292 and the previous review blocker. The retry is now wrapped around the current DashboardQueries-based session stream reads, so it preserves the dashboard read-model boundary instead of restoring the old module-level path. The handling stays narrow to transient SQLite contention: SQLITE_BUSY/SQLITE_LOCKED, plus the legacy message fallback for database locked errors; non-contention OperationalError cases still propagate.
Validation performed locally in a temporary venv outside the checkout:
- python3 -m pytest -q tests/test_backend.py -k 'dashboard_sse' -> 5 passed, 71 deselected
- python3 -m pytest -q -> 539 passed, 1 warning
The review worktree remained clean after validation.
|
The integration blocker from this review is resolved on the current head e045cc4. I rechecked it against the current base 8f6e1c8: the SSE reads remain behind DashboardQueries, GitHub reports mergeable_state=clean, all required checks pass, and the focused dashboard SSE tests pass (5 passed, 71 deselected). No additional code change was needed for this stale review delivery. |
|
Post-merge cleanup completed.
|
|
Post-merge follow-up: verified merge commit |
Closes #292.
What changed
SQLITE_BUSYorSQLITE_LOCKEDcontention;This handles the error inside the stream, where FastAPI can no longer replace the response after headers have started. The branch is rebased onto current
mainand uses theDashboardQueriesread boundary introduced there.Validation
python3 -m pytest -q tests/test_backend.py -k 'dashboard_sse'— 5 passedpython3 -m pytest -q— 539 passedRequested by: no human requester; automated Sentry issue #292 assigned the bridge bot.