[Hardening] Remove duplicate reminder delivery on session reschedule

claude-bot commented

2026-07-14 19:47:29 +00:00

Collaborator

Context

Session reminders have two delivery mechanisms that coexist:

Poller (system of record): the poll_session_reminders beat task, deduplicated by SessionReminderSent rows in Postgres (unique per session/time/offset).
Legacy ETA path: _schedule_reminders (webapp/backend/app/services/session_service.py:67) enqueues send_session_reminder Celery tasks with an ETA (apply_async at :120), tracked via session.celery_task_ids (:145-151), deduplicated by a Redis key qb:reminder-sent:{session_id}:{confirmed_time}:{hours} (webapp/backend/app/tasks/reminder_tasks.py:35).

confirm_session (session_service.py:561) correctly relies on the poller alone — see the comment at :605 ("Reminders are delivered by the poll_session_reminders beat task"), and the same note at :226.

Current behavior

The reschedule path (session_service.py:397-433) does both:

(a) clears the session's SessionReminderSent rows (:398-414) so the poller will re-fire for the new time, and
(b) calls _schedule_reminders (:423), storing the returned task IDs in session.celery_task_ids (:432), which enqueues legacy ETA tasks.

The two paths have independent dedup stores (Postgres row vs Redis key), so neither suppresses the other: every reminder for a rescheduled session is delivered twice (two Discord posts / bot notifications per offset).

Fix / Spec

Remove the _schedule_reminders(...) call (and the celery_task_ids bookkeeping) from the reschedule branch — clearing the SessionReminderSent rows is sufficient; the poller is the system of record, matching confirm_session.
Retire the legacy ETA path entirely in this issue: grep for all call sites of _schedule_reminders and send_session_reminder and remove them if the poller covers every case (confirm before deleting). Also remove the now-dead task-revocation helper that iterates session.celery_task_ids (session_service.py:145-151) and the Redis dedup key logic at reminder_tasks.py:35 if the task itself is deleted. Leave the celery_task_ids DB column for a later migration if dropping it is noisy — but stop writing to it.
If full retirement turns out to be unsafe (some case the poller doesn't cover), do step 1 only and file a follow-up documenting the gap.

Acceptance criteria

Test proving a rescheduled session gets each reminder offset exactly once (no double delivery).
No remaining callers of _schedule_reminders / no ETA-based send_session_reminder enqueues (or a documented follow-up if step 3 applied).
confirm_session behavior unchanged.
Backend test suite green.

References

webapp/backend/app/services/session_service.py:67-151 (legacy scheduler + revocation), :397-433 (reschedule double-path), :561 / :605 (confirm path, poller-only)
webapp/backend/app/tasks/reminder_tasks.py:35 (Redis dedup key), poll_session_reminders at :1027+

Filed from the July 2026 full-project review.

## Context Session reminders have two delivery mechanisms that coexist: 1. **Poller (system of record)**: the `poll_session_reminders` beat task, deduplicated by `SessionReminderSent` rows in Postgres (unique per session/time/offset). 2. **Legacy ETA path**: `_schedule_reminders` (`webapp/backend/app/services/session_service.py:67`) enqueues `send_session_reminder` Celery tasks with an ETA (`apply_async` at `:120`), tracked via `session.celery_task_ids` (`:145-151`), deduplicated by a **Redis** key `qb:reminder-sent:{session_id}:{confirmed_time}:{hours}` (`webapp/backend/app/tasks/reminder_tasks.py:35`). `confirm_session` (`session_service.py:561`) correctly relies on the poller alone — see the comment at `:605` ("Reminders are delivered by the poll_session_reminders beat task"), and the same note at `:226`. ## Current behavior The **reschedule** path (`session_service.py:397-433`) does both: - (a) clears the session's `SessionReminderSent` rows (`:398-414`) so the poller will re-fire for the new time, **and** - (b) calls `_schedule_reminders` (`:423`), storing the returned task IDs in `session.celery_task_ids` (`:432`), which enqueues legacy ETA tasks. The two paths have independent dedup stores (Postgres row vs Redis key), so neither suppresses the other: every reminder for a rescheduled session is delivered **twice** (two Discord posts / bot notifications per offset). ## Fix / Spec 1. Remove the `_schedule_reminders(...)` call (and the `celery_task_ids` bookkeeping) from the reschedule branch — clearing the `SessionReminderSent` rows is sufficient; the poller is the system of record, matching `confirm_session`. 2. Retire the legacy ETA path entirely in this issue: grep for all call sites of `_schedule_reminders` and `send_session_reminder` and remove them if the poller covers every case (confirm before deleting). Also remove the now-dead task-revocation helper that iterates `session.celery_task_ids` (`session_service.py:145-151`) and the Redis dedup key logic at `reminder_tasks.py:35` if the task itself is deleted. Leave the `celery_task_ids` DB column for a later migration if dropping it is noisy — but stop writing to it. 3. If full retirement turns out to be unsafe (some case the poller doesn't cover), do step 1 only and file a follow-up documenting the gap. ## Acceptance criteria - Test proving a rescheduled session gets each reminder offset exactly once (no double delivery). - No remaining callers of `_schedule_reminders` / no ETA-based `send_session_reminder` enqueues (or a documented follow-up if step 3 applied). - `confirm_session` behavior unchanged. - Backend test suite green. ## References - `webapp/backend/app/services/session_service.py:67-151` (legacy scheduler + revocation), `:397-433` (reschedule double-path), `:561` / `:605` (confirm path, poller-only) - `webapp/backend/app/tasks/reminder_tasks.py:35` (Redis dedup key), `poll_session_reminders` at `:1027+` _Filed from the July 2026 full-project review._

claude-bot added the

backend

bug

labels

2026-07-14 19:47:36 +00:00

claude-bot added this to the v3.3.0 - Hardening: Bugs & Security milestone

2026-07-14 19:47:37 +00:00

claude-bot referenced this issue

2026-07-14 20:26:35 +00:00

[Hardening] Replace builtin hash() with a deterministic digest in lore match chunk indexing #88

claude-bot commented

2026-07-14 20:26:41 +00:00

Author

Collaborator

Picking this up as part of a v3.3.0 push. Landing on branch hardening/backend together with #88, #97, #106, and #109 (grouped by component to keep the diffs reviewable).

Picking this up as part of a v3.3.0 push. Landing on branch `hardening/backend` together with #88, #97, #106, and #109 (grouped by component to keep the diffs reviewable).

claude-bot referenced this issue

2026-07-14 20:26:42 +00:00

[Hardening] Validate campaign discord_webhook_url against a Discord host allowlist (SSRF) #97

claude-bot referenced this issue

2026-07-14 20:26:45 +00:00

[Hardening] Reject placeholder and weak secrets at startup #106

claude-bot referenced this issue

2026-07-14 20:26:47 +00:00

[Hardening] Escape HTML in recap emails; stop echoing raw LLM errors from /ask #109

claude-bot commented

2026-07-14 21:33:41 +00:00

Author

Collaborator

Fixed on main (commit 13c2a33, merged via 1c9c19f). Legacy ETA path fully retired — the poller (deduped by SessionReminderSent rows) is now the sole delivery mechanism. Removed _schedule_reminders/_revoke_reminders and their call sites, and deleted send_session_reminder, _claim_reminder_delivery, and the Redis dedup key. The celery_task_ids column is kept but no longer written.

Two things the issue description didn't anticipate:

_schedule_reminders was also firing the immediate "session confirmed" notice. Deleting the call outright would have silently dropped that announcement on every reschedule. Replaced with _send_confirmation_notice(...), the same path confirm_session uses. confirm_session itself is unchanged.
One gap the poller didn't cover: update_session allows reschedule from in_progress and leaves the status as in_progress while confirmed_time moves into the future, but the poller only selected status == confirmed. Full retirement would have silently dropped reminders for that (frontend-supported) flow, so the poller now selects confirmed + in_progress. This is a no-op for genuinely running sessions — their confirmed_time is already older than the 5-minute lookback window, so the ETA-window gate means no offset can fire. (Verified this independently before merging.)

Tests: a rescheduled session delivers each offset exactly once, and the legacy ETA task is retired. Deploy note: ETA messages already sitting in the Redis broker at upgrade will be discarded by workers as unregistered tasks (logged, non-fatal) for up to 7 days — which is the desired no-duplicate outcome. Backend suite green (358 passed), ruff clean.

Fixed on `main` (commit `13c2a33`, merged via `1c9c19f`). Legacy ETA path **fully retired** — the poller (deduped by `SessionReminderSent` rows) is now the sole delivery mechanism. Removed `_schedule_reminders`/`_revoke_reminders` and their call sites, and deleted `send_session_reminder`, `_claim_reminder_delivery`, and the Redis dedup key. The `celery_task_ids` column is kept but no longer written. Two things the issue description didn't anticipate: 1. **`_schedule_reminders` was also firing the immediate "session confirmed" notice.** Deleting the call outright would have silently dropped that announcement on every reschedule. Replaced with `_send_confirmation_notice(...)`, the same path `confirm_session` uses. `confirm_session` itself is unchanged. 2. **One gap the poller didn't cover:** `update_session` allows reschedule from `in_progress` and leaves the status as `in_progress` while `confirmed_time` moves into the future, but the poller only selected `status == confirmed`. Full retirement would have silently dropped reminders for that (frontend-supported) flow, so the poller now selects `confirmed + in_progress`. This is a no-op for genuinely running sessions — their `confirmed_time` is already older than the 5-minute lookback window, so the ETA-window gate means no offset can fire. (Verified this independently before merging.) Tests: a rescheduled session delivers each offset exactly once, and the legacy ETA task is retired. **Deploy note:** ETA messages already sitting in the Redis broker at upgrade will be discarded by workers as unregistered tasks (logged, non-fatal) for up to 7 days — which is the desired no-duplicate outcome. Backend suite green (358 passed), ruff clean.

claude-bot closed this issue

2026-07-14 21:34:06 +00:00