rbrooks/Quest-Board

Watch

Fork 0

Routes answer 200 before their commit: get_db commits after the response is sent #851

New issue

Closed

opened 2026-09-26 18:16:23 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-09-26 18:16:23 +00:00

Collaborator

Found by lane 6's J3/J2 runs (#732, on build/lane6-j3-seed) and confirmed from a trace.

The bug. get_db commits when the request's dependencies close, and FastAPI 0.141 closes them after the response has been sent. Every route that leaves its commit to get_db therefore answers before its write is committed. There are about 400 Depends(get_db) uses.

Consequences:

A client's next request can miss the write. The J2 vote grid pressed the same cell twice, 100 ms apart. The second press found no vote, inserted one and hit uq_votes_slot_user. Main's #847 made that particular insert race-safe, but the ordering problem remains. It may also explain #845, the stale status once after a start.
A commit that fails after the 200 is lost silently. The client was told it succeeded.

Also: vote_service.notify_vote_cast doesn't catch a failed enqueue. The broker is set to fail fast because request-path enqueues are meant to be non-fatal, so a broker hiccup can fail the vote. lane 6's branch fixes this for votes only.

Fix:

Make commits happen before the response for every non-streaming route.
Check the streaming endpoints and any route that relies on post-response commit.
Add a regression test that asserts commit-before-response generally.
Make vote-path enqueues best-effort.

Refs #845, #844, #732.

Found by lane 6's J3/J2 runs (#732, on `build/lane6-j3-seed`) and confirmed from a trace. **The bug.** `get_db` commits when the request's dependencies close, and FastAPI 0.141 closes them **after the response has been sent**. Every route that leaves its commit to `get_db` therefore answers before its write is committed. There are about 400 `Depends(get_db)` uses. **Consequences:** 1. **A client's next request can miss the write.** The J2 vote grid pressed the same cell twice, 100 ms apart. The second press found no vote, inserted one and hit `uq_votes_slot_user`. Main's #847 made that particular insert race-safe, but the ordering problem remains. It may also explain #845, the stale status once after a start. 2. **A commit that fails after the 200 is lost silently.** The client was told it succeeded. **Also:** `vote_service.notify_vote_cast` doesn't catch a failed enqueue. The broker is set to fail fast because request-path enqueues are meant to be non-fatal, so a broker hiccup can fail the vote. lane 6's branch fixes this for votes only. **Fix:** - Make commits happen before the response for every non-streaming route. - Check the streaming endpoints and any route that relies on post-response commit. - Add a regression test that asserts commit-before-response generally. - Make vote-path enqueues best-effort. Refs #845, #844, #732.

claude-bot referenced this issue

2026-09-26 18:18:20 +00:00

Build lane 6: Prep and Planning (PREP-1 to PREP-4) #732

rbrooks referenced this issue from a commit

2026-09-26 19:05:07 +00:00

test(backend): a route's write is committed before its response leaves (#851)

rbrooks referenced this issue from a commit

2026-09-26 19:05:07 +00:00

fix(webapp): a route's write is committed before it answers (#851)

rbrooks referenced this issue from a commit

2026-09-26 19:05:07 +00:00

fix(webapp): a vote stands when its notification can't be queued (#851)

claude-bot referenced this issue from a pull request that will close it,

2026-09-26 19:05:24 +00:00

fix(webapp): commit before the response, app-wide (#851) #854

rbrooks referenced this issue from a commit

2026-09-26 19:46:55 +00:00

Merge origin/main into lane 6 (#732); re-chain b8c0d2e4f6a7 onto c4e6a8b0d2f1

claude-bot referenced this issue

2026-09-26 19:47:09 +00:00

feat: lane 6, prep and planning: the prep sheet, the tool margin, History and Planning, J3 and the seed (#732) #858

claude-bot closed this issue

2026-09-26 20:01:26 +00:00

claude-bot referenced this issue from a commit

2026-09-26 20:01:27 +00:00

Merge PR #854: commit before the response, app-wide (#851)

claude-bot commented

2026-09-26 20:02:54 +00:00

Author

Collaborator

Fixed by #854, merged 2026-09-26.

The fix:

All 377 Depends(get_db) are now scope="function", so every route commits before the response is sent.
A commit that fails now returns 500 instead of being lost.
Votes commit before notifying, and their enqueue is best-effort.

The guard: tests/test_commit_before_response.py walks every route and fails if any get_db isn't function-scoped. It also probes vote PUT and DELETE, and a failing commit, from a second connection.

Corroborating evidence: the J5 phone failures seen on main and on #853 had the same shape. A tap's new state was overwritten by the next poll, which read stale data. J5 passed in #854's own CI run.

Fixed by #854, merged 2026-09-26. **The fix:** - All 377 `Depends(get_db)` are now `scope="function"`, so every route commits before the response is sent. - A commit that fails now returns 500 instead of being lost. - Votes commit before notifying, and their enqueue is best-effort. **The guard:** `tests/test_commit_before_response.py` walks every route and fails if any `get_db` isn't function-scoped. It also probes vote PUT and DELETE, and a failing commit, from a second connection. **Corroborating evidence:** the J5 phone failures seen on main and on #853 had the same shape. A tap's new state was overwritten by the next poll, which read stale data. J5 passed in #854's own CI run.

rbrooks referenced this issue from a commit

2026-09-26 22:17:13 +00:00

chore(repo): CHANGELOG fragments so parallel PRs stop clashing on CHANGELOG.md (#851)

claude-bot referenced this issue

2026-09-27 00:03:55 +00:00

Build lane 6: Prep and Planning (PREP-1 to PREP-4) #732

claude-bot referenced this issue

2026-09-27 02:48:50 +00:00

J5 on the phone project fails intermittently on main (strike/reword, "looks right") #875

claude-bot referenced this issue

2026-10-01 23:39:05 +00:00

After /record/start returns 200, GET /sessions/{id} once still read "confirmed" (D116) #845