fix(dashboard): give a session a terminal status after an interrupted review - #864
Conversation
… review A session row is written in_progress when a review starts and updated when it ends, so a review killed between those two writes never got the terminal one. Nothing reconciled the row afterwards and it stayed in_progress for the life of the database, counting as a running review and hiding the genuinely in-flight ones among the stale rows. A new startup sweep moves every in_progress row to failed with "Review interrupted before it finished (bot restart or crash)" as the reason. No review survives a restart, so a row still in progress at boot belongs to a review that is over, whatever killed it: the sweep is unconditional rather than age-based, and it reconciles the rows stranded before it existed. The reason, not a status of its own, is what separates an interruption from a review that failed on its own. An unknown status would render as the pending hourglass those rows already show and would fall outside both the completed and the failed counters on the overview. Only the status and the message are written, so the tokens and the cost the review had paid for stay on the row, and no schema change is needed. Sweeping every row at boot is safe because the bot is a single process. The javadoc says so, and says what a second replica would need first.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesAdds an @ApplicationScoped InterruptedSessionReconciler that observes StartupEvent and bulk-updates every reviewsession row still marked in_progress to failed with the message "Review interrupted before it finished (bot restart or crash)", keeping tokens and cost intact, with failure logged and swallowed so a database problem cannot block boot. Documents the behavior in the README and CHANGELOG and adds unit/integration tests covering the sweep, the dashboard counters, and the swallowed-failure startup path. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["Quarkus boot completes"] --> B["onStart observes StartupEvent"]
B --> C{"repository bulk update\nin_progress -> failed"}
C -->|"reconciled > 0"| D["log.info count"]
C -->|"0 rows"| E["no log, done"]
C -->|"RuntimeException"| F["log.warn and continue boot"]
Changes Overview
Changed Files
Risk Assessment
No new issues found in this PR, but the review cannot be approved until required CI is confirmed green.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| test | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
thrillhousebot
Bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
Required CI is now green for e13a9c1, so the approval the earlier review held back is posted. The code was not re-reviewed: that review found no issues, and only the CI gate held its approval.
|
/review |
|
/pause |
|
⏸️ ThrillhouseBot is now paused on this PR — automatic and manual reviews are silenced. Comment |
1 participant
What type of PR is this?
Description
A
reviewsessionrow is writtenin_progressbyReviewSession.createwhen the review starts, and onlyReviewOrchestrator.applyReviewResult/applyReviewFailure(andReviewSessionUpdater.recordFailure) ever move it to a terminal status, from inside the process running that review. A review killed between those two writes — a deploy restart, a crash, adocker kill— never got the terminal one, and nothing reconciled the row afterwards, so it stayedin_progressfor the life of the database. I confirmed there is no sweep of any kind: the onlyStartupEventobservers areStartupConfigValidator,DashboardWebSocketKeepAliveandSessionCostBackfill(cost only), there is no@Scheduledanywhere, and the onlyShutdownEventobserver is the WebSocket keepalive.InterruptedSessionReconcilerobservesStartupEventand moves everyin_progressrow tofailedwithReview interrupted before it finished (bot restart or crash)as theerrormessage. It is a bulk JPQL update touching those two columns only, so the tokens and cost already accumulated on a stranded row are kept, and no schema change is needed. A failure of the sweep is logged and swallowed, as inSessionCostBackfill, so a database problem cannot stop the bot from booting.failed+ a distinct reason, not a new status value. The dashboard's session list maps status to an icon withif (status === 'completed') … if (status === 'failed') … return '⏳'(frontend/app/(dashboard)/sessions/page.tsx:340-350), so a row under a new status would keep rendering as the pending hourglass in yellow — exactly the "looks like it is still running" state this issue is about — and the frontend would have to ship before the backend to avoid it./summarycountscompletedReviewsandfailedReviewswith twostatus = '…'counts against atotalReviewsthat counts everything (DashboardResource:342-353), so a third value would leave the overview cards not adding up. The cost and token analytics group by model overstatus = 'completed'only, so they are unaffected either way.errormessageis already carried throughtoSessionDetailand rendered on the session page, which is what makes an interruption distinguishable from a review that failed on its own, at no cost to the filters or the statistics. Reusingfaileddoes move these rows into the "Failed (30d)" card; that is the trade-off, and it is the smaller distortion of the two — the alternative leaves them in neither card and still looking alive.No age-based sweep. A row can only be stranded by the process dying, so the next startup reconciles it whatever killed it; the exposure is the window between the death and the restart, when nobody is looking at a dashboard served by a process that is down. A periodic sweep would have to guess a safe age:
ReviewDispatchersubmits to an executor with no wall-clock bound, so the longest a live review can take ismax-ai-calls(6) model calls of up toai-timeout-seconds(300s) each, plus retries, publication and GitHub write backoff (GITHUB_WRITE_RETRY_BUDGET, 5m) — a threshold, not a bound, and one that would fail a live long review as interrupted if set too low. It would also need thequarkus-schedulerextension, which the project does not depend on today. The startup sweep covers every case that actually strands a row, including the 10 existing ones, so this stays out until there is a reason for it.Issue claim that does not hold. The issue suggests the age sweep could key off "the longest a review can take, which is bounded by the dispatcher timeout". There is no dispatcher timeout:
ReviewDispatcher.dispatchcallsreviewExecutor.execute(...)and returns, and nothing applies a wall-clock limit to the review as a whole — the only timeouts are per AI call (thrillhousebot.review.ai-timeout-seconds,quarkus.langchain4j.openai.timeout) and per GitHub write. That is part of why the sweep is unconditional rather than age-based. The production row counts (10 rows, oldest 2026-06-09, newest 2026-09-09T15:00:07Z) are taken from the issue; I have no access to the production database. Everything else in the issue matched the code.The single-process assumption (README, "Known limitations") is the reason an unconditional sweep is safe, and the class javadoc states it together with what a second replica would need first (an owner or a lease on the row).
Related Issues
Fixes #863
How Has This Been Tested?
Red first. The tests were written before the reconciler existed, so the suite did not compile against
main:With the class in place but the sweep not implemented (
reconcile()returning 0 without touching a row), the same tests failed on behaviour:with, in full, for the two dashboard expectations:
Green after the fix. Eight new tests: a stranded row becomes
failedwith the interrupted reason; its tokens and cost survive the sweep; a database whose rows are all terminal is left alone and the sweep reports 0; the startup path does the same work as the direct call;/api/dashboard/summarycounts a reconciled row as failed (1 total, 0 completed, 1 failed);/api/dashboard/sessions/{id}renders it asfailedcarrying the interrupted reason; and two mock-based tests for the startup path with nothing to reconcile and with the database refusing.Gates, from the worktree root:
Patch coverage of the
src/maindiff againstorigin/main, line and branch, fromtarget/site/jacoco/jacoco.xml:GAPS: none.Checklist
Additional Notes
The 10 stranded production rows are reconciled by this sweep on the first boot after the deploy, with no manual SQL. They move into the failed count on the overview, where nine of them are older than the card's 30-day window.
This is not #26. A graceful drain reduces how often a review is interrupted; it cannot reconcile a row already stranded, nor one from a crash or a
docker kill. The two are complementary, and nothing here depends on #26.