Skip to content

Fall back to Postgres when the Redis breach cache read fails (mntor-5378) - #6829

Merged
GregHilstonMozilla merged 6 commits into
mainfrom
mntor-5378
Sep 8, 2026
Merged

GregHilstonMozilla merged 6 commits into
mainfrom
mntor-5378

Conversation

@GregHilstonMozilla

@GregHilstonMozilla GregHilstonMozilla commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator

References:

Jira: MNTOR-5378

Incident doc: 2026-09-03 Monitor Website Unresponsive

Description

getAllBreachesFromDb() wrapped the cache read, the Postgres read and the cache write in one try, and returned [] from the shared catch. Two things fell out of that.

A Redis fault surfaced as an empty list, and getBreaches() reads an empty list as "the breaches table is empty", so it answered with a full HIBP fetch plus upsert. Since it holds no in-memory cache, that ran on every request. A Postgres timeout was also reported as Failed to read breaches from redis, then re-queried against the pool that had just timed out.

Split into three independent steps, each with its own try and its own log name, so Grafana can tell "degraded but serving" from "both down". A fault now reads Postgres once and skips the cache refill, since a second command against a faulted Redis only stalls again.

Depends on nothing in #6826, which stops the connection leak. That one is the urgent half; this is the hardening that keeps a future Redis fault from becoming a site outage.

Testing Before and After

Three log names now instead of one, so a reviewer can see which subsystem failed:

what failed before after
Redis read get_all_breaches_from_db get_breaches_from_redis_failed
Postgres read get_all_breaches_from_db get_all_breaches_from_db_failed
Redis write get_all_breaches_from_db set_breaches_in_redis_failed

Caught unplanned: a production build ran while Redis happened to be down and completed anyway, serving all 1,005 breaches from Postgres on every prerender. On main each of those prerenders would instead have fired a live HIBP fetch and upsert.

How to test

npx vitest run src/utils/hibp.test.ts \
    src/app/functions/server/getBreaches.test.ts

Each regression case fails on main, which reports:

  • hibp.test.ts: [] instead of the Postgres rows, and blames Redis for a Postgres timeout
  • getBreaches.test.ts: an empty catalog, because the Redis fault reached HIBP

Checklist (Definition of Done)

  • Localization strings (if needed) have been added. Not applicable, no user-facing strings.
  • Commits in this PR are minimal and have descriptive commit messages. Six commits.
  • I've added or updated the relevant sections in readme and/or code comments. docs/architecture/flows/breach-read-path.md now distinguishes a cache miss from a cache fault.
  • I've added a unit test to test for potential regressions of this bug. getBreaches.ts had none before this PR.
  • If this PR implements a feature flag or experimentation, I've checked that it still works with the flag both on, and with the flag off. Not applicable.
  • If this PR implements a feature flag or experimentation, the Ship Behind Feature Flag status in Jira has been set. Not applicable.
  • Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
  • All acceptance criteria are met. Two of three. "Once, not per request" is not covered, see below.
  • Jira ticket has been updated (if needed) to match changes made during the development process.
  • Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

Follow Up Action

getAllBreachesFromDb still returns [] when Postgres fails, and getBreaches still answers that with a per-request HIBP fetch plus upsert. Same amplifier, different trigger, and the likelier remaining one now that the Redis path is fixed. The comment above that branch says so rather than hiding it.

Comment thread src/utils/hibp.ts

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

big take away here is we've separated out the three failures into three separate logs we can more easily trace. we can make these metrics in a future effort

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some simple update to our documentation now that we made a change to the flow of data when Redis is down.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is being added as the original src/app/functions/server/getBreaches.ts had no tests

getAllBreachesFromDb swallowed a Redis fault and returned []. getBreaches()
reads that as "the breaches table is empty" and answers by fetching the
whole catalogue from HIBP and re-upserting it, per request.
The single try wrapped the cache read, the Postgres read and the cache
write, so all three failures logged "Failed to read breaches from redis"
and all three landed in the same fallback. A Postgres timeout was
therefore reported as a Redis fault and then re-queried.
@GregHilstonMozilla
GregHilstonMozilla marked this pull request as ready for review September 8, 2026 16:29

@joeherm joeherm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thanks for adding test cases for all the scenarios.

@GregHilstonMozilla
GregHilstonMozilla added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit cb36e8c Sep 8, 2026
19 checks passed
@GregHilstonMozilla
GregHilstonMozilla deleted the mntor-5378 branch September 8, 2026 17:03
@GregHilstonMozilla GregHilstonMozilla changed the title Mntor 5378 Fall back to Postgres when the Redis breach cache read fails (mntor-5378) Oct 16, 2026
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.

2 participants