fix(automation): Mark Slack missing-standup reminder sent only after a successful send - #54
Open
detail-app[bot] wants to merge 1 commit into
Conversation
…a successful send
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detail bug report: View on Detail
Bug
standupMissingReminderSend(the 5-minute cron task that nudge-emails and Slacks students who missed a required standup) wrotesentMissingReminderSlack = truebefore attempting the Slack send, and thesendSlackProjectMessagecall was not wrapped intry/catch. On a transient Slack API failure (chat.postMessagerejection — rate limit, network blip, 5xx) the flag was alreadytrue, the unhandled throw propagated to the cron runner'stryCrontabwrapper (which only logs and swallows), and the row was then permanently excluded from the task'sfindMany(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 siblingsendScheduledAnnouncementstask for the Slack medium) is to flip the idempotency flag only after a successful send.Fix
src/automation/tasks/standupMissingReminderSend.ts:sentMissingReminderSlack: true, sentMissingReminderEmail: true) before any send.try/catch;sentMissingReminderSlackis settrueonly aftersendSlackProjectMessageresolves successfully. A failure leaves the flagfalseso the next cron tick re-selects the row and retries (matchingsendScheduledAnnouncementsfor Slack).elsebranch (Slack not configured / no missing student has aslackId) and amissingStudents.length === 0short-circuit that both marksentMissingReminderSlack = truewithout sending — without these, those rows would be re-fetched every 5 minutes forever.sendScheduledAnnouncements's accepted email convention.runStandupMissingReminderSend({ prisma, sendSlackProjectMessage, sendTemplateEmail })+ thin default-export wrapper that still resolves them from thetypedicontainer). This mirrors the existingprojectParticipations(prisma)seam used insyncAlumniInteractions.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
src/automation/tasks/standupMissingReminderSend.test.ts, run withnpx 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 thefindManyOR filter, every "nothing to send" branch still closes the flag (no infinite re-fetch), and the email no-retry convention is preserved. All pass.postgres:14container and applied the schema withprisma db push(note:prisma migrate deployfails on a pre-existing broken migration20220610164033...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 stayedfalseafter the failed tick and flipped totrueonly on the successful retry tick.DATABASE_URL, Elasticsearch, Slack, etc. reachable); sincetryCrontabisn'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 staysfalse, and the next tick re-selects and retries to success.chat.postMessagecould not be exercised — no Slack credentials exist in this environment (direct attempt returnsnot_authed). The DB-state behavior — the substance of the fix — is fully covered by the stubbed-Slack runs above.npx tsc --skipLibCheck --noEmit) passes.eslintfails with a pre-existing@typescript-eslint/parservs TypeScript 5 incompatibility that reproduces identically on unmodified files; it is not a CI gate (CircleCI runs onlydocker build/yarn run build).Automatic Fixes PRs can be configured here.