rbrooks/Quest-Board
Watch
1
0
Fork
You've already forked Quest-Board
 0
Code Issues 77 Pull requests 2 Projects Releases 42 Packages Wiki Activity Actions

[Backend] Archiving a campaign does not stop it creating sessions or sending Discord reminders #479

New issue
Closed
opened 2026-09-01 16:12:37 +00:00 by claude-bot · 3 comments
claude-bot commented 2026-09-01 16:12:37 +00:00
Contributor
Copy link

Severity: MEDIUM

Found while implementing #405, where the proposed fix was to route campaign deletion through the existing archive feature. That turned out not to work, and the reason is this issue.

What archiving does and does not do

Archiving is not a no-op, and this issue is not a claim that it is. It does exactly what its own UI text promises — "hidden from the default active dashboard list, but its history stays available here for reference and export":

  • webapp/frontend/src/pages/Dashboard.jsx:274-275 splits active from archived
  • webapp/frontend/src/pages/CampaignDetail.jsx:1400 shows the archived banner
  • webapp/backend/app/services/campaign_service.py:236 blocks join-by-invite

What it does not do is suppress anything in the backend. is_archived appears in exactly three places server-side — the model (models/campaign.py:115), the schemas (schemas/campaign.py:250,288), and campaign_service (the archive/restore functions plus that one join guard). There are zero references in app/tasks/, in routers/bot.py, or anywhere in the bot/ package.

So archiving is a display-level lifecycle state. Every scheduled task treats an archived campaign as fully live.

The root is session creation, not the reminders

The reminders are the symptom. materialize_session_series (app/tasks/reminder_tasks.py, hourly Beat) selects:

select(SessionSeries).where(SessionSeries.active.is_(True))

with no campaign check at all. An archived campaign with an active recurring series therefore keeps materialising new confirmed sessions, 28 days ahead, every hour. Those sessions are real rows, so poll_session_reminders then does its job on them correctly.

Nothing prevents archiving a campaign that has a series or future sessions — archive_campaign only guards against double-archiving.

Failure scenario

A GM archives a campaign at the end of a season, exactly as the feature invites them to. The group's weekly recurring series keeps generating sessions, and Discord keeps posting reminders for a campaign nobody is playing — 7-day, 24-hour and 1-hour pings, plus at-risk warnings when nobody RSVPs to a session that was never meant to exist. The GM's only recourse is to hunt down and deactivate the series, or delete the campaign outright, which is precisely the destructive action archiving exists to be an alternative to.

Proposed fix, and the scoping that matters

Two changes, in this order:

  1. Stop materialising. materialize_session_series should skip series whose campaign is archived. This is the actual fix — no new sessions means no reminders for them.
  2. Suppress outbound notifications for sessions that already exist in an archived campaign.

Do not implement (2) by adding the archived check to campaign_service.get_live_campaign. That helper was added in #405 and is called at all 11 campaign-resolution sites in reminder_tasks.py, but those sites are not all outbound notifications — and suppressing the wrong ones loses data rather than noise:

must stay live why
process_audio's campaign lookup archive mid-processing and the transcript loses its speaker/campaign context
generate_journal_entry archive mid-processing and the journal entry is silently never written
warn_before_audio_deletion retention still deletes an archived campaign's audio, so the warning must still fire — arguably more important when nobody is looking at the campaign

The suppression wants a separate predicate applied only to the genuinely outbound sites (session reminders, at-risk warnings, vote notifications, recap email, attendance posts), not a blanket filter on campaign resolution. That distinction is the substance of this issue; getting it wrong converts an annoyance into silent data loss.

This is deliberately not how #405 handled deletion. A trashed campaign is invisible to everyone including its GM, so a blanket filter is correct there. An archived campaign stays fully readable, so only its outbound behaviour should change.

Acceptance criteria

  • An archived campaign with an active recurring series stops materialising new sessions.
  • Existing future sessions in an archived campaign stop sending reminders, at-risk warnings and vote notifications.
  • Audio processing, journal generation and audio-deletion warnings are unaffected by archiving — with a test per item, since these are the failure mode of the obvious implementation.
  • Un-archiving resumes normal behaviour, including materialisation.
  • Decide and document whether archiving should refuse, warn about, or silently keep already-materialised future sessions.
**Severity: MEDIUM** Found while implementing #405, where the proposed fix was to route campaign deletion through the existing archive feature. That turned out not to work, and the reason is this issue. ## What archiving does and does not do Archiving is **not** a no-op, and this issue is not a claim that it is. It does exactly what its own UI text promises — *"hidden from the default active dashboard list, but its history stays available here for reference and export"*: - `webapp/frontend/src/pages/Dashboard.jsx:274-275` splits active from archived - `webapp/frontend/src/pages/CampaignDetail.jsx:1400` shows the archived banner - `webapp/backend/app/services/campaign_service.py:236` blocks join-by-invite What it does not do is suppress anything in the backend. `is_archived` appears in exactly three places server-side — the model (`models/campaign.py:115`), the schemas (`schemas/campaign.py:250,288`), and `campaign_service` (the archive/restore functions plus that one join guard). **There are zero references in `app/tasks/`, in `routers/bot.py`, or anywhere in the `bot/` package.** So archiving is a display-level lifecycle state. Every scheduled task treats an archived campaign as fully live. ## The root is session creation, not the reminders The reminders are the symptom. `materialize_session_series` (`app/tasks/reminder_tasks.py`, hourly Beat) selects: ```python select(SessionSeries).where(SessionSeries.active.is_(True)) ``` with no campaign check at all. An archived campaign with an active recurring series therefore keeps **materialising new confirmed sessions**, 28 days ahead, every hour. Those sessions are real rows, so `poll_session_reminders` then does its job on them correctly. Nothing prevents archiving a campaign that has a series or future sessions — `archive_campaign` only guards against double-archiving. ## Failure scenario A GM archives a campaign at the end of a season, exactly as the feature invites them to. The group's weekly recurring series keeps generating sessions, and Discord keeps posting reminders for a campaign nobody is playing — 7-day, 24-hour and 1-hour pings, plus at-risk warnings when nobody RSVPs to a session that was never meant to exist. The GM's only recourse is to hunt down and deactivate the series, or delete the campaign outright, which is precisely the destructive action archiving exists to be an alternative to. ## Proposed fix, and the scoping that matters Two changes, in this order: 1. **Stop materialising.** `materialize_session_series` should skip series whose campaign is archived. This is the actual fix — no new sessions means no reminders for them. 2. **Suppress outbound notifications** for sessions that already exist in an archived campaign. **Do not implement (2) by adding the archived check to `campaign_service.get_live_campaign`.** That helper was added in #405 and is called at all 11 campaign-resolution sites in `reminder_tasks.py`, but those sites are not all outbound notifications — and suppressing the wrong ones loses data rather than noise: | must stay live | why | |---|---| | `process_audio`'s campaign lookup | archive mid-processing and the transcript loses its speaker/campaign context | | `generate_journal_entry` | archive mid-processing and the journal entry is silently never written | | `warn_before_audio_deletion` | retention still deletes an archived campaign's audio, so the warning must still fire — arguably *more* important when nobody is looking at the campaign | The suppression wants a separate predicate applied only to the genuinely outbound sites (session reminders, at-risk warnings, vote notifications, recap email, attendance posts), not a blanket filter on campaign resolution. That distinction is the substance of this issue; getting it wrong converts an annoyance into silent data loss. This is deliberately **not** how #405 handled deletion. A trashed campaign is invisible to everyone including its GM, so a blanket filter is correct there. An archived campaign stays fully readable, so only its *outbound* behaviour should change. ## Acceptance criteria - [ ] An archived campaign with an active recurring series stops materialising new sessions. - [ ] Existing future sessions in an archived campaign stop sending reminders, at-risk warnings and vote notifications. - [ ] Audio processing, journal generation and audio-deletion warnings are **unaffected** by archiving — with a test per item, since these are the failure mode of the obvious implementation. - [ ] Un-archiving resumes normal behaviour, including materialisation. - [ ] Decide and document whether archiving should refuse, warn about, or silently keep already-materialised future sessions.
claude-bot commented 2026-09-01 17:03:31 +00:00
Author
Contributor
Copy link

Also in scope: trashed campaigns keep materialising sessions

Found while implementing #405 and deliberately left out of PR #478, because it belongs with this issue — materialize_session_series is the one function that needs a lifecycle check for both states, and settling them separately would mean touching it twice with two different predicates.

#405 routed the scheduled tasks' campaign lookups through campaign_service.get_live_campaign, which returns None for a trashed campaign so the if not campaign: continue guard those tasks already had does the suppression. But the series materialiser does not use that lookup as a gate — it resolves the campaign only for the timezone:

campaign = await campaign_service.get_live_campaign(db, series.campaign_id)
tz_name = campaign.timezone if campaign else None

So a trashed campaign with an active series keeps generating sessions too, falling back to tz_name = None rather than skipping.

Why it was left rather than fixed in #478

It is close to harmless today, and the argument for leaving it is real enough to record rather than assume away:

  • The sessions are invisible — the auth gate 404s anything under a trashed campaign.
  • No Discord noise — the reminder path does check, so nothing fires for them.
  • They are purged with the campaign when the grace elapses.
  • On restore you get the sessions the series would have created anyway, which is arguably the correct outcome.

So it is rows written for a campaign scheduled for deletion, not user-visible harm. The one wrinkle: a restored campaign would hold sessions that never had reminders sent, which is a quiet inconsistency rather than a bug.

What to do here

Whatever gate this issue adds for archived campaigns, apply the trashed case in the same place. Concretely, the series selection wants to exclude both — join Campaign and filter deleted_at IS NULL and is_archived IS false — rather than resolving the campaign afterwards purely for a timezone.

Note the two states want the same treatment here, which is unusual: elsewhere in #405 they differ sharply (a trashed campaign is invisible to everyone; an archived one stays fully readable). Session creation is the one behaviour neither state should have.

Extra acceptance criterion

  • A campaign in the trash also stops materialising new sessions, with a test — and the test must confirm sessions are created for a live campaign in the same run, or it passes for the wrong reason whether or not the filter is there.
## Also in scope: trashed campaigns keep materialising sessions Found while implementing #405 and deliberately left out of PR #478, because it belongs with this issue — `materialize_session_series` is the one function that needs a lifecycle check for **both** states, and settling them separately would mean touching it twice with two different predicates. #405 routed the scheduled tasks' campaign lookups through `campaign_service.get_live_campaign`, which returns `None` for a trashed campaign so the `if not campaign: continue` guard those tasks already had does the suppression. But the series materialiser does not use that lookup as a gate — it resolves the campaign only for the timezone: ```python campaign = await campaign_service.get_live_campaign(db, series.campaign_id) tz_name = campaign.timezone if campaign else None ``` So a **trashed** campaign with an active series keeps generating sessions too, falling back to `tz_name = None` rather than skipping. ### Why it was left rather than fixed in #478 It is close to harmless today, and the argument for leaving it is real enough to record rather than assume away: - The sessions are invisible — the auth gate 404s anything under a trashed campaign. - No Discord noise — the reminder path *does* check, so nothing fires for them. - They are purged with the campaign when the grace elapses. - On restore you get the sessions the series would have created anyway, which is arguably the correct outcome. So it is rows written for a campaign scheduled for deletion, not user-visible harm. The one wrinkle: a restored campaign would hold sessions that never had reminders sent, which is a quiet inconsistency rather than a bug. ### What to do here Whatever gate this issue adds for archived campaigns, apply the trashed case in the same place. Concretely, the series selection wants to exclude both — join `Campaign` and filter `deleted_at IS NULL` *and* `is_archived IS false` — rather than resolving the campaign afterwards purely for a timezone. Note the two states want the **same** treatment here, which is unusual: elsewhere in #405 they differ sharply (a trashed campaign is invisible to everyone; an archived one stays fully readable). Session *creation* is the one behaviour neither state should have. ### Extra acceptance criterion - [ ] A campaign in the trash also stops materialising new sessions, with a test — and the test must confirm sessions are created for a live campaign in the same run, or it passes for the wrong reason whether or not the filter is there.
claude-bot commented 2026-09-06 00:41:12 +00:00
Author
Contributor
Copy link

Picking this up as part of v4.3.0 phase 1 (#514), shipping early as v4.2.3. Following the scoping in the body exactly: materialisation skips archived campaigns; a separate outbound-only predicate at the reminder, at-risk, vote, recap-email and attendance sites; process_audio, journal generation and the audio-deletion warning stay live with a test each. For already-materialised future sessions the decision is warn, not refuse: archiving keeps them and reports how many will go quiet.

Picking this up as part of v4.3.0 phase 1 (#514), shipping early as v4.2.3. Following the scoping in the body exactly: materialisation skips archived campaigns; a separate outbound-only predicate at the reminder, at-risk, vote, recap-email and attendance sites; `process_audio`, journal generation and the audio-deletion warning stay live with a test each. For already-materialised future sessions the decision is warn, not refuse: archiving keeps them and reports how many will go quiet.
claude-bot commented 2026-09-06 01:43:23 +00:00
Author
Contributor
Copy link

Fixed in PR #516 (merged), shipping in v4.2.3.

The root, as the issue called it, was session creation. materialize_session_series now joins the campaign it was already fetching one row at a time for a timezone, and skips a series whose campaign is archived, or trashed, which turned out to share the hole: the old get_live_campaign returned None for a trashed campaign and the loop materialised into it anyway. The series is not deactivated, so un-archiving resumes with nothing to backfill (rolling window plus ON CONFLICT DO NOTHING).

Suppression is a separate predicate, campaign_service.campaign_accepts_outbound, applied at exactly five sites: session reminder, at-risk warning, three-day vote nudge, recap email, end-of-session attendance post, each logging the campaign id once at info. The line drawn is unprompted, not Discord-facing: a vote someone casts and a card a GM reveals still say what they were asked to say, because swallowing a button press would be a worse bug than this one. Two placements matter and are asserted on: the reminder check precedes the SessionReminderSent claim and the vote check precedes the vote_reminder_sent_at stamp, so an archived campaign never burns a marker it did not use.

process_audio, generate_journal_entry and warn_before_audio_deletion are untouched and have a test each. To prove those tests bite, the exact wrong change this issue warns about (or campaign.is_archived inside get_live_campaign) was applied temporarily: those three go red and nothing else does.

Decision on already-materialised future sessions: warn, not refuse. Deleting them would destroy rows a group may have notes and attendance on; refusing would make archiving fail on exactly the campaigns it exists for, since stopping mid-series is the common case. They stay and go quiet. POST /campaigns/{id}/archive now returns silenced_future_sessions (additive, on a subclass of CampaignResponse): the count of confirmed or in-progress sessions still ahead, which is precisely the set the poller would have announced. Also recorded in the audit context. No UI here; #382 can render it.

No migration. No /api/bot/* change, so no BOT_CONTRACT_VERSION bump. New tests/test_archived_campaign_outbound.py (13 tests); full backend suite 2349 passed / 13 skipped; ruff clean.

One judgment call flagged for a second opinion: send_vote_notification (the ping when a player actually casts a vote, routers/votes.py) is deliberately not suppressed, for the human-in-the-loop reason above. If the intent was to silence that too, it is a three-line change plus inverting one test that exists so the choice stays deliberate.

Fixed in PR #516 (merged), shipping in v4.2.3. **The root, as the issue called it, was session creation.** `materialize_session_series` now joins the campaign it was already fetching one row at a time for a timezone, and skips a series whose campaign is archived, or trashed, which turned out to share the hole: the old `get_live_campaign` returned `None` for a trashed campaign and the loop materialised into it anyway. The series is not deactivated, so un-archiving resumes with nothing to backfill (rolling window plus `ON CONFLICT DO NOTHING`). **Suppression is a separate predicate**, `campaign_service.campaign_accepts_outbound`, applied at exactly five sites: session reminder, at-risk warning, three-day vote nudge, recap email, end-of-session attendance post, each logging the campaign id once at info. The line drawn is *unprompted*, not *Discord-facing*: a vote someone casts and a card a GM reveals still say what they were asked to say, because swallowing a button press would be a worse bug than this one. Two placements matter and are asserted on: the reminder check precedes the `SessionReminderSent` claim and the vote check precedes the `vote_reminder_sent_at` stamp, so an archived campaign never burns a marker it did not use. `process_audio`, `generate_journal_entry` and `warn_before_audio_deletion` are untouched and have a test each. To prove those tests bite, the exact wrong change this issue warns about (`or campaign.is_archived` inside `get_live_campaign`) was applied temporarily: those three go red and nothing else does. **Decision on already-materialised future sessions: warn, not refuse.** Deleting them would destroy rows a group may have notes and attendance on; refusing would make archiving fail on exactly the campaigns it exists for, since stopping mid-series is the common case. They stay and go quiet. `POST /campaigns/{id}/archive` now returns `silenced_future_sessions` (additive, on a subclass of `CampaignResponse`): the count of confirmed or in-progress sessions still ahead, which is precisely the set the poller would have announced. Also recorded in the audit context. No UI here; #382 can render it. No migration. No `/api/bot/*` change, so no `BOT_CONTRACT_VERSION` bump. New `tests/test_archived_campaign_outbound.py` (13 tests); full backend suite 2349 passed / 13 skipped; ruff clean. One judgment call flagged for a second opinion: `send_vote_notification` (the ping when a player actually casts a vote, `routers/votes.py`) is deliberately **not** suppressed, for the human-in-the-loop reason above. If the intent was to silence that too, it is a three-line change plus inverting one test that exists so the choice stays deliberate.
rbrooks referenced this issue from a commit 2026-09-06 02:00:15 +00:00
chore(release): v4.2.3
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
ui/table-notes-label
renovate/npm-vitest-vulnerability
chore/87-align-python-3.12
v4.3.0
v4.2.3
v4.2.2
v4.2.1
v4.2.0
v4.1.2
v4.1.1
v4.1.0
v4.0.1
v4.0.0
v3.11.4
v3.11.3
v3.11.2
v3.11.1
v3.11.0
v3.10.0
v3.9.1
v3.9.0
v3.8.0
v3.7.0
v3.6.1
v3.6.0
v3.5.0
v3.4.1
v3.4.0
v3.3.0
v3.2.0
v3.1.0
v3.0.0
v2.1.0
v2.0.0
v1.1.0
v1.0.0
v0.12.0
v0.11.0
v0.10.0
v0.9.1
v0.3.3
v0.9.0
v0.8.0
v0.3.2
v0.3.1
v0.3.0
v0.7.0
v0.6.0
v0.5.0
v0.4.0
v0.2.1
v0.2.0
v0.1.1
v0.1.0
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
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
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#479
No description provided.