Skip to content

fix(automation): Mark Slack missing-standup reminder sent only after a successful send - #54

Open
detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-automation-mark-slack-missing-standup-reminder-f913e4
Open

detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-automation-mark-slack-missing-standup-reminder-f913e4

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Detail bug report: View on Detail

Bug

standupMissingReminderSend (the 5-minute cron task that nudge-emails and Slacks students who missed a required standup) wrote sentMissingReminderSlack = true before attempting the Slack send, and the sendSlackProjectMessage call was not wrapped in try/catch. On a transient Slack API failure (chat.postMessage rejection — rate limit, network blip, 5xx) the flag was already true, the unhandled throw propagated to the cron runner's tryCrontab wrapper (which only logs and swallows), and the row was then permanently excluded from the task's findMany (OR: [{ sentMissingReminderSlack: false }, { sentMissingReminderEmail: false }]) — the Slack reminder was silently lost forever while the DB audit trail (sentMissingReminderSlack: true) falsely asserted delivery. The intended pattern (used by the sibling sendScheduledAnnouncements task for the Slack medium) is to flip the idempotency flag only after a successful send.

Fix

src/automation/tasks/standupMissingReminderSend.ts:

  • Removed the unconditional combined early write of both flags (sentMissingReminderSlack: true, sentMissingReminderEmail: true) before any send.
  • Slack send is now wrapped in try/catch; sentMissingReminderSlack is set true only after sendSlackProjectMessage resolves successfully. A failure leaves the flag false so the next cron tick re-selects the row and retries (matching sendScheduledAnnouncements for Slack).
  • Added an else branch (Slack not configured / no missing student has a slackId) and a missingStudents.length === 0 short-circuit that both mark sentMissingReminderSlack = true without sending — without these, those rows would be re-fetched every 5 minutes forever.
  • Email channel behavior is intentionally unchanged: the email flag is set before the send and email send errors are swallowed (no retry), matching sendScheduledAnnouncements's accepted email convention.
  • Extracted a small dependency-injection seam (runStandupMissingReminderSend({ prisma, sendSlackProjectMessage, sendTemplateEmail }) + thin default-export wrapper that still resolves them from the typedi container). This mirrors the existing projectParticipations(prisma) seam used in syncAlumniInteractions.test.ts — the only precedent for offline unit tests in this repo — and is what makes the fix testable without a live DB/Slack/SMTP.

Testing

  • Offline unit tests (src/automation/tasks/standupMissingReminderSend.test.ts, run with npx tsx ...): 8 cases / 18 assertions pinning the invariants that are easy to silently regress — a failed Slack send leaves the flag false, the flag is written after the send (not before), a failing row does not abort the rest of the batch, end-to-end retry across ticks via the findMany OR filter, every "nothing to send" branch still closes the flag (no infinite re-fetch), and the email no-retry convention is preserved. All pass.
  • Integration against a real Postgres (scratch script, not committed): brought up a postgres:14 container and applied the schema with prisma db push (note: prisma migrate deploy fails on a pre-existing broken migration 20220610164033... unrelated to this change). Ran 5 end-to-end scenarios — happy path with idempotent re-run, transient-failure→flag-stays-false→retry-succeeds→flag-true, batch isolation, no-missing-students, and Slack-not-configured — against the real DB with a stubbed Slack sender. All verified the DB ends in the correct state; the row's flag stayed false after the failed tick and flipped to true only on the successful retry tick.
  • Cron-runner behavior: the full app could not be booted here (it eagerly constructs Elasticsearch/SMTP/Slack clients and needs all of DATABASE_URL, Elasticsearch, Slack, etc. reachable); since tryCrontab isn't exported I ran its verbatim body wrapping the real task against the real DB. Verified the wrapper logs the error and exits cleanly (Crontab standupMissingReminderSend exited.) after the throw, the flag stays false, and the next tick re-selects and retries to success.
  • Live Slack chat.postMessage could not be exercised — no Slack credentials exist in this environment (direct attempt returns not_authed). The DB-state behavior — the substance of the fix — is fully covered by the stubbed-Slack runs above.
  • Typecheck (npx tsc --skipLibCheck --noEmit) passes. eslint fails with a pre-existing @typescript-eslint/parser vs TypeScript 5 incompatibility that reproduces identically on unmodified files; it is not a CI gate (CircleCI runs only docker build/yarn run build).

Automatic Fixes PRs can be configured here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant