Skip to content

fix(api): close three retry/duplicate hazards - #82

Merged
mankatcheung merged 1 commit into
mainfrom
fix/api-idempotency-safety
Jul 24, 2026
Merged

mankatcheung merged 1 commit into
mainfrom
fix/api-idempotency-safety

Conversation

@mankatcheung

@mankatcheung mankatcheung commented Jul 24, 2026 •

Copy link
Copy Markdown
Owner

Summary

Follow-up to an API idempotency audit. Conclusion: a generic idempotency-key system is not necessary for this app (small trusted client set, no adversarial/high-volume third-party callers, MCP surface is 100% read-only). But three concrete, currently-unprotected retry/duplicate hazards were found and are fixed here — each cheap, and mirroring a pattern already proven elsewhere in the codebase.

  • Weekly digest duplicate-send guard. /admin/digest/send is a plain secret-gated HTTP route invoked by an external scheduler not tracked in this repo — exactly the kind of caller that may retry on a timeout/5xx. There was zero duplicate-send protection, so a retry meant every subscribed user got a duplicate digest email. Added a per-user lastDigestSentAt timestamp + a 6-day resend window, mirroring the existing reminderSentAt pattern used by follow-up reminders.
  • UpdateApplicationUseCase duplicate activity-log entries. The field_updated branch logged whenever a field was present in the input, not when it actually changed — unlike the status_changed branch right next to it, which already compared against the current value. A retried/duplicate update with identical values polluted the activity timeline every time. Now compares against the pre-update snapshot (with proper Date instant comparison for followUpAt).
  • Bulk-delete retry safety. BulkDeleteApplicationsUseCase used Promise.all, so retrying a partially-failed bulk-delete failed again on the already-deleted items. Switched to Promise.allSettled, treating NOT_FOUND as an idempotent no-op while other per-item failures (e.g. FORBIDDEN) still surface as before. No API contract change — still returns void / throws on real failures.

Deliberately out of scope (investigated, not fixed): MCP tools (confirmed 100% read-only), create-mutation dedup (a UI double-submit concern, not a server-side gap), and BulkUpdateApplicationsUseCase/BulkAddTagToApplicationsUseCase (no clean "already applied" no-op concept the way delete has).

Test plan

  • pnpm --filter @job-finder/api test — 642 tests passing, including new coverage for all three fixes
  • pnpm --filter @job-finder/api typecheck — clean
  • pnpm --filter @job-finder/api lint — clean
  • New Prisma migration (add_last_digest_sent_at) applied and Prisma client regenerated
  • Manual smoke test of the digest guard against a live server (blocked locally by an unrelated pre-existing gap in this repo's migration history — the Document table's initial creation was never captured in a migration file, so rebuilding a fresh dev DB from migration history fails; not something to fix as part of this PR)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Weekly digests now include a resend safeguard, preventing repeat emails within the configured window.
    • Digest delivery timestamps are recorded for improved scheduling.
  • Bug Fixes

    • Bulk deletion now continues when an application is already missing, while reporting other failures.
    • Activity logs now record only genuine application field changes, including follow-up dates.

Weekly digest has no duplicate-send guard despite being triggered by an
external scheduler over HTTP, so a retry currently double-sends to every
subscribed user. Add a per-user lastDigestSentAt timestamp with a 6-day
resend window, mirroring the existing reminderSentAt pattern.

UpdateApplicationUseCase logged a field_updated activity entry whenever a
field was present in the input, not when it actually changed, unlike the
correctly-guarded status_changed branch next to it — a retried no-op
update polluted the activity timeline. Now compares against the current
value before logging.

BulkDeleteApplicationsUseCase used Promise.all, so retrying a partially
failed bulk-delete failed again on the already-deleted items. Switch to
Promise.allSettled and treat NOT_FOUND as an idempotent no-op; other
per-item failures still surface as before. No API contract change.
@coderabbitai

coderabbitai Bot commented Jul 24, 2026 •

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f5ab77e5-704f-443d-ba4b-ddf4e413ff1d

📥 Commits

Reviewing files that changed from the base of the PR and between 1e9e9c0 and 36566a1.

📒 Files selected for processing (14)
  • apps/api/prisma/migrations/20260724205821_add_last_digest_sent_at/migration.sql
  • apps/api/prisma/schema.prisma
  • apps/api/src/__tests__/application/jobs/BulkDeleteApplicationsUseCase.test.ts
  • apps/api/src/__tests__/application/jobs/UpdateApplicationUseCase.test.ts
  • apps/api/src/__tests__/digest/SendWeeklyDigestUseCase.test.ts
  • apps/api/src/__tests__/helpers/createTestDb.ts
  • apps/api/src/__tests__/helpers/mocks.ts
  • apps/api/src/constants.ts
  • apps/api/src/domain/user/User.ts
  • apps/api/src/infrastructure/db/repositories/PrismaUserRepository.ts
  • apps/api/src/use-cases/digest/SendWeeklyDigestUseCase.ts
  • apps/api/src/use-cases/jobs/BulkDeleteApplicationsUseCase.ts
  • apps/api/src/use-cases/jobs/UpdateApplicationUseCase.ts
  • apps/api/src/use-cases/ports/IUserRepository.ts

Walkthrough

The PR adds persisted weekly digest send timestamps and resend-window checks, makes bulk deletion idempotent for missing applications, and restricts field-update activity logs to actual value changes.

Changes

Digest resend tracking

Layer / File(s) Summary
Weekly digest resend tracking
apps/api/prisma/..., apps/api/src/domain/user/User.ts, apps/api/src/use-cases/digest/..., apps/api/src/infrastructure/db/repositories/..., apps/api/src/__tests__/digest/...
Adds lastDigestSentAt to the user model, repository contract, persistence mapping, fixtures, and test database. The digest use case skips users within the six-day resend window and records successful sends.
Bulk deletion error handling
apps/api/src/use-cases/jobs/BulkDeleteApplicationsUseCase.ts, apps/api/src/__tests__/application/jobs/BulkDeleteApplicationsUseCase.test.ts
Uses settled deletion results, treats NOT_FOUND as a no-op, and continues throwing other errors.
Application activity change detection
apps/api/src/use-cases/jobs/UpdateApplicationUseCase.ts, apps/api/src/__tests__/application/jobs/UpdateApplicationUseCase.test.ts
Logs field_updated only for changed primitive or timestamp values and retains status_changed logging coverage.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DigestJob
  participant UserRepository
  participant Mailer
  DigestJob->>UserRepository: load users and lastDigestSentAt
  DigestJob->>Mailer: send digest after resend window
  DigestJob->>UserRepository: persist send timestamp
Loading

Possibly related PRs

Poem

A rabbit checks the digest clock,
Skips a send around the block.
Missing jobs cause no fright,
Changed fields now log just right.
Timestamps hop into their place—
Clean records leave a tidy trace.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/api-idempotency-safety

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mankatcheung
mankatcheung marked this pull request as ready for review July 24, 2026 22:50
@mankatcheung
mankatcheung merged commit 44523b8 into main Jul 24, 2026
5 checks passed
@mankatcheung
mankatcheung deleted the fix/api-idempotency-safety branch July 24, 2026 22:50
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