fix(backend): restore the redis driver, prune bullmq 5 repeatables, and fix search escaping - #2194
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
f8b706b to
95fafae
Compare
…ix search escaping Three defects from the bullmq/jwks-rsa/escape-string-regexp majors (#2188). None reached production: only the frontend redeployed on that commit. bullmq 6 moved ioredis from a direct dependency to an OPTIONAL peer, and pnpm's auto-install-peers does not install optional peers, so the driver dropped out of the lockfile entirely. bull.config.ts passes a plain options object as `connection`, so bullmq has to require('ioredis') itself and threw "BullMQ could not load the optional 'ioredis' package". initQueues is awaited before app.listen inside a catch that exits, so this was a whole-backend outage on next deploy. ioredis is now a direct dependency, pinned to the 5.11.1 line main already ran. The scheduler ids carried across the migration unchanged, but that was the wrong thing to check: bullmq 5 never keyed a repeatable by jobId, it keyed by the md5 of name:jobId:endDate:tz:every. upsertJobScheduler keys by the plain id, so the upsert added a second entry beside the old one in the same repeat zset, and bullmq 6 still schedules from the legacy shape. Every recurring job would have fired twice, indefinitely: duplicate no-show marking, duplicate task reminders to users, doubled lab polling. initQueues now prunes the md5 entries before upserting. The escape helper was replaced by a LIKE escape. Both call sites feed the value into Prisma `contains`, which is an ILIKE pattern and not a regular expression, so regex escaping was always wrong. It only looked correct because a backslash before an ordinary character is a no-op in LIKE. Emitting \x2d for a hyphen broke that: "Jean-Luc" searched for "Jeanx2dLuc" and matched nothing. Escaping backslash, percent and underscore instead makes the search mean exactly what was typed, and stops user input acting as a wildcard. The scheduler suites mock every queue module, so no real Queue is ever built and they could not have caught the first two. redis-driver.test.ts checks the dependency graph itself instead.
95fafae to
829c6b4
Compare
|
Good catch, and it was a real bug rather than a false positive.
Fixed by branching on the result. A Two tests were added for it, and both fail if the boolean is ignored again:
|
|
|
|
|
2 participants
PR Checklist
What is the current behavior?
Three defects arrived on
devwith the dependency majors in #2188. They were found by an audit of thedevtomainpromotion, which is on hold until this lands.None of them reached production. Only the frontend redeployed on that commit; the backend has not picked up bullmq 6, and
mainnever received it.1. The backend cannot boot
bullmq 6 moved
ioredisfrom a direct dependency to an optional peer. pnpm'sauto-install-peersinstalls missing non-optional peers only, so the driver dropped out of the lockfile completely:mainresolves/ioredis@5.11.1,devhas no entry at all.apps/backend/src/queues/bull.config.tspasses a plain options object asconnectionrather than a client instance, so bullmq has to load a driver itself. Reproduced by running it against thedevtree:initQueues()is awaited inmain.tsbeforeapp.listen, inside acatchthat callsprocess.exit(1), so this is a whole-backend outage on next deploy rather than a degraded subsystem.2. Every recurring job would fire twice, permanently
#2188 verified that the seven scheduler ids and intervals were unchanged. That check passed and was the wrong check.
bullmq 5 never used
jobIdas the Redis key for a repeatable.Repeat.updateRepeatableJobbuiltname:jobId:endDate:tz:everyand stored the entry under the md5 of that string.upsertJobScheduler(id, ...)stores under the plain id. Carrying the same id string across the migration therefore adds a second entry next to the old one rather than replacing it, and both sit in the samerepeatsorted set, which bullmq 6 still reads and still schedules from.The effect on a running system: no-show marking twice a minute, task reminders delivered twice, task recurrence generated twice per 6 hours, both lab pollers hitting the external lab APIs twice per 5 minutes, IDEXX sync twice a week.
3. Hyphenated searches silently return nothing
Both call sites feed the escaped value into Prisma
contains, which compiles to anILIKEpattern. That is not a regular expression, so regex escaping was never the right tool. It only appeared to work because escaping an ordinary character with a backslash is a no-op in LIKE, soO'Brien \(Jr\.\)still matched.The new helper matched escape-string-regexp 5.0.0, which escapes a hyphen as
\x2d. LIKE reads that as the literal textx2d, so a search forJean-Luclooked forJeanx2dLucand matched nothing. No error, just an empty result. This hitsParentService.getByName(first name, last name, email) andCompanionService.getByName. Hyphens are common in surnames and near universal in email domains.What is the new behavior?
1.
ioredisis a direct dependency ofapps/backend, pinned to^5.11.1- the same linemainruns today, so nothing changes but the driver being present again.2. New
apps/backend/src/queues/legacy-repeatables.ts, called frominitQueues()before any upsert. It lists the schedulers on each of the seven queues and removes the bullmq 5 leftovers. It only removes keys that are exactly a 32 character lowercase hex digest, which is the md5 shape bullmq 5 produced and which none of this app's ids can collide with, so a scheduler this app owns is never removed even if the expected list were incomplete. A removal failure is logged and skipped rather than thrown, because leaving one stale entry costs a duplicate job whereas throwing takes the API down.3.
escape-regexp.tsis replaced byescape-like.ts, which escapes backslash, percent and underscore. That makes the search mean exactly the text typed, and as a side effect closes a wildcard issue that predates all of this: Prisma does not escape LIKE metacharacters, so onmaintoday a search for100%matches everything starting with100, anda_cmatchesabc.catalog.service.tshas its own localescapeRegExp, which is used inside a realnew RegExp(...)and is correct. It is untouched.Impact area
apps/backendonly. No API surface, route, schema or migration changes.One deployment note: the prune in item 2 mutates the
repeatsorted set in the Redis the backend connects to. It is idempotent and a no-op once it has run.Validation performed
Run locally on Node 20:
pnpm run type-check- 17/17 passpnpm run lint- 10/10 passpnpm run test:scripts- passapps/backendfull suite - 257 suites, 4864 tests, all passing (was 255 / 4848)Coverage on both new files:
src/queues/legacy-repeatables.tssrc/utils/escape-like.tsWhy the original tests missed all three
Worth stating plainly, because it shaped what was added here. The scheduler suites
jest.mockevery*.queuemodule, so a realQueueis never constructed and no driver is ever loaded - that test design is structurally incapable of catching defects 1 and 2, however many cases it contains. And the escape test asserted the new helper against an inlined copy of the spec it had been written to, never against the context the value is actually used in.So
test/queues/redis-driver.test.tsdeliberately uses no mocks and checks the dependency graph itself: thatioredisis a declared direct dependency, that it resolves fromapps/backend, and that the installed version satisfies the range bullmq asks for. Andescape-like.test.tsasserts through a model of how PostgreSQL reads a LIKE pattern, so the cases are about what the database will match rather than what the function returns.Mutation testing
Each fix was reverted to the exact defect that shipped and the suite confirmed to go red, then restored and confirmed green:
\x2dregex escape - 4 tests failioredisfromapps/backend/package.json- the direct-dependency test fails (locally only that one, sincenode_modulesstill holds the package; under CI's--frozen-lockfileinstall the resolve and peer-range tests fail too)Defect 1 was also verified end to end by constructing a real
Queueagainst thedevtree and observing the driver error, then repeating it on this branch and observing the error replaced by an ordinary connection attempt.A note on the ioredis advisory
The Aikido gate reports one new MEDIUM on this PR: AIKIDO-2026-538318 in
ioredis5.11.1. It is worth being precise about what that means, because the label is misleading here.This PR does not add exposure. It restores parity with
main.mainresolves/ioredis@5.11.1today, via bullmq 5's direct dependency, and carries this same advisory in production right now. Aikido calls it "new" only because it diffs againstdev, anddevlost the package when #2188 accidentally dropped the driver. Aikido's own feed already lists this issue as open against thedevbranch with a remediate-by date of 2026-08-10.It cannot be fixed in this PR:
engines: node >=20.node:18(apps/backend/Dockerfileline 1).Taking 6.x was tried on this branch and reverted. Node does not enforce
enginesat runtime, and #2125 documents that 26 packages in this tree already declare>=20against the Node 18 image, so it might well work - but "might well work" is not a basis for changing the Redis driver that every background job depends on, inside a hotfix, and it could not be verified here.So
ioredisis frozen at 5.x in.github/dependabot.yml, in the same documented style as the other freezes waiting on the runtime upgrade, and #2125 has been updated to unfreeze it when Node moves. That makes this the first freeze where the old runtime is holding back a security fix rather than only an unsupported-version warning, which is worth weighing when #2125 is scheduled.Related Issue(s)
Fixes defects introduced by #2188. Unblocks the
devtomainpromotion.