fix(backend): the reminder and at-risk polls pass over what was already sent (#1115) #1118

Merged

claude-bot merged 1 commit from fix/1115-reminder-poll-rollback into main 2026-10-10 18:44:30 +00:00

Conversation 0 Commits 1 Files changed 4 +404 -36

claude-bot commented 2026-10-10 18:08:43 +00:00

Collaborator

Copy link

Closes #1115.

Cause

Both polls claim an "already told" marker by inserting it and catching IntegrityError, and the handler called await db.rollback(). task_session() sets expire_on_commit=False, which does not cover rollback: every loaded row was expired, and the log line's session.id then raised MissingGreenlet and ended the task. "Already told" is what every poll after the first finds, so:

• poll_session_at_risk (since 3.6.0) stopped at the first already-warned session, and the sessions after it in the list were not checked.
• poll_session_reminders (since 1.1.0) did the same for the minutes after a reminder went out.
• The delivery-failure branches had the same fault: a failed publish also ended the poll.

Seen on dev (1,152 failures of the at-risk poll in four days) and on prod 4.4.1 (15 of the reminder poll on 7 October).

Fix

• The claim runs in a savepoint (db.begin_nested()): a duplicate undoes the insert and nothing else.
• Both polls walk a list of ids and read each session as its turn comes, so a rollback in one cannot poison the next.
• The delivery-failure branches keep their full rollback (the claim must go, so the next poll retries) and log from plain values; the reminder branch re-reads the session and campaign before that session's next offset.
• The idempotency rule is unchanged: a marker commits with its notifications, after delivery was attempted.

Tests

tests/test_poll_marker_claims.py (new, four tests) commits real rows and lets the polls open the real task_session(). The existing tests could not see this: the test session aliases commit to flush, so the rows were never persistent, and a rollback expunged them with their values loaded instead of expiring them. On main's code all four fail with MissingGreenlet; with the fix they pass. test_at_risk_task_notifies_once_per_confirmed_time now asserts the marker survives the second run.

Run before this PR: the four new tests (fail on main, pass here), 202 related backend tests, ruff. The full suite is CI's.

Not in this PR: import_jobs.Heartbeat rolls back after a failed heartbeat write; whether anything reads an expired row afterwards was not traced.

🤖 Generated with Claude Code

Closes #1115. ## Cause Both polls claim an "already told" marker by inserting it and catching `IntegrityError`, and the handler called `await db.rollback()`. `task_session()` sets `expire_on_commit=False`, which does not cover rollback: every loaded row was expired, and the log line's `session.id` then raised `MissingGreenlet` and ended the task. "Already told" is what every poll after the first finds, so: - `poll_session_at_risk` (since 3.6.0) stopped at the first already-warned session, and the sessions after it in the list were not checked. - `poll_session_reminders` (since 1.1.0) did the same for the minutes after a reminder went out. - The delivery-failure branches had the same fault: a failed publish also ended the poll. Seen on dev (1,152 failures of the at-risk poll in four days) and on prod 4.4.1 (15 of the reminder poll on 7 October). ## Fix - The claim runs in a savepoint (`db.begin_nested()`): a duplicate undoes the insert and nothing else. - Both polls walk a list of ids and read each session as its turn comes, so a rollback in one cannot poison the next. - The delivery-failure branches keep their full rollback (the claim must go, so the next poll retries) and log from plain values; the reminder branch re-reads the session and campaign before that session's next offset. - The idempotency rule is unchanged: a marker commits with its notifications, after delivery was attempted. ## Tests `tests/test_poll_marker_claims.py` (new, four tests) commits real rows and lets the polls open the real `task_session()`. The existing tests could not see this: the test session aliases `commit` to `flush`, so the rows were never persistent, and a rollback expunged them with their values loaded instead of expiring them. On main's code all four fail with `MissingGreenlet`; with the fix they pass. `test_at_risk_task_notifies_once_per_confirmed_time` now asserts the marker survives the second run. Run before this PR: the four new tests (fail on main, pass here), 202 related backend tests, ruff. The full suite is CI's. Not in this PR: `import_jobs.Heartbeat` rolls back after a failed heartbeat write; whether anything reads an expired row afterwards was not traced. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit 2026-10-10 18:08:44 +00:00

fix(reminders): an already-sent marker no longer ends the poll (#1115) ... a09d171035

poll_session_at_risk and poll_session_reminders claim an "already told"
marker by inserting it and taking a unique-constraint failure to mean it
was sent before. They answered that failure with db.rollback(), which
expires every row the session has loaded; the log line straight after
read session.id outside await and raised MissingGreenlet, ending the
task. "Already told" is what every poll after the first finds, so the
at-risk poll died at the first warned session on every run and never
reached the sessions after it.

The claim is now made in a savepoint, so a duplicate undoes the insert
and nothing else. The delivery-failure branches keep their full rollback
(the claim must go, so the next poll tries again) and no longer touch
what it expired: both polls walk session ids and read each session as
its turn comes, and the reminder poll reads the session and campaign
again before the same session's next offset.

The existing tests ran the polls on the test's own never-committed
transaction, where a rollback expunges the rows rather than expiring
them, so the crash could not happen there. The new tests commit their
rows and let the poll open its own task_session().

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude-bot merged commit e6058b3650 into main 2026-10-10 18:44:30 +00:00

claude-bot referenced this pull request from a commit 2026-10-10 18:44:31 +00:00

Merge PR #1118: the reminder and at-risk polls pass over what was already sent (#1115)

claude-bot deleted branch fix/1115-reminder-poll-rollback 2026-10-10 18:44:31 +00:00

claude-bot referenced this pull request 2026-10-10 18:44:37 +00:00

Reminder polls crash on every run once a session has been warned (MissingGreenlet after rollback) #1115

claude-bot referenced this pull request 2026-10-10 18:44:49 +00:00

chore(release): v4.5.0-rc10 #1119

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

backend

Issue related to the backend

  

bot

Issue related to the bot

  

bug

Something is not working

  

duplicate

This issue or pull request already exists

  

enhancement

New feature

  

frontend

Issue related to the frontend

  

help wanted

Need some help

  

invalid

Something is wrong

  

question

More information is needed

  

recording

Issue related to the recording system

  

security

Security hardening / vulnerability work

  

wontfix

This won't be fixed

No labels

backend

bot

bug

duplicate

enhancement

frontend

help wanted

invalid

question

recording

security

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rbrooks/Quest-Board!1118

No description provided.