fix(api): scope offlineDetector DB contexts to DB statements only (#3233) - #3235
Merged
Conversation
) createOfflineWorker wrapped the entire job body in withSystemDbAccessContext, so every sweep's Redis fan-out (queue.addBulk) and the uninstall reaper's per-row writes ran while a pooled Postgres connection sat idle-in-transaction for the whole chunked loop. Observed on a local stack during v0.104.0 release QA: 13 held-context warnings in ~15 minutes at 5-10s each, all from offlineDetector — surfaced for the first time by the #3221 caller-attribution diagnostic. detect-offline runs every 30s (twice as often as the alertWorker evaluate-all sweep fixed in #3216) and holds ONE connection across the entire multi-chunk loop, so the hold grows with fleet size. Against a bounded pool (DB_POOL_MAX default 30, one region ~25) that eats directly into headroom and amplifies outages. The 5000/10000ms clustering is ioredis reconnect backoff (services/redis.ts:121) charged against the held connection. Same fix shape as alertWorker evaluate-all (#3216) and snmpWorker poll-scheduler (#3215): - Worker handler no longer blanket-wraps. The three sweep jobs self-manage; the two per-device jobs (mark-offline, reevaluate-offline) keep a whole-job context since they read+write a single row and do no fan-out. - processDetectOffline / processReevaluateOfflineSweep: page read moves into its own short system context that CLOSES before that page's queue.addBulk. - processReapUninstallIntent: page read gets its own context; each candidate's two writes then share one short context of their own, so the decommission and the replacement-linkage clear still commit together but no connection is held across the whole chunk. Pagination now spans transactions. That is intentional and safe: the keyset cursor (devices.id ascending) is stable, the eligibility timestamps are computed once per run so the window does not drift between pages, and a device whose status flips mid-sweep is picked up on the next cycle. Adds offlineDetector.dbcontext.test.ts, which tracks real context enter/exit depth and asserts WHICH depth each read, enqueue and write ran at — an identity fn => fn() mock cannot catch this class of bug. Verified to fail against the unfixed worker (addBulk lands at depth1 instead of depth0). Also documents that runOutsideDbContext is NOT a substitute here: it only exits the AsyncLocalStorage, it does not release the pooled connection held by the enclosing baseDb.transaction(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deploying breeze with
|
| Latest commit: |
0039d51
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5d0765e6.breeze-9te.pages.dev |
| Branch Preview URL: | https://fix-3233-offline-detector-db.breeze-9te.pages.dev |
The hoisted queue mock declared `addBulk: vi.fn(async () => [])`, so the per-test `mockImplementation((jobs) => ...)` that inspects the batch size failed TS2345 in CI. Local vitest does not typecheck, and the earlier `tsc --noEmit` ran before this test file existed, so it slipped through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ToddHebebrand
added a commit
that referenced
this pull request
Aug 8, 2026
…oS CVEs (#3241) Unblocks the PR queue. Trivy's vulnerability DB refreshed on 2026-08-08 and three new HIGH findings began failing **`Trivy Filesystem Scan` on every PR resolved after the refresh** — currently #3235 and #3238. **This is not caused by any of those PRs.** #3232 was scanned *before* the refresh and passed, which makes the failure look selective; it isn't, and it will redden `main` on the next push regardless of what gets merged. Exact repeat of the CVE-2026-59870 (js-yaml) episode in the previous release round. ## The three findings, and why they're handled differently | CVE | Package | Fix available? | Ships to users? | Action | |---|---|---|---|---| | CVE-2026-67213 | `nanoid` 3.3.16 | **Yes** — 3.3.17 / 5.1.6 | **Yes** — runtime dep of apps/mobile | pnpm override | | CVE-2025-71329 | `image-size` 1.2.1 | **No** | No — build-time only | `.trivyignore` | | CVE-2025-71330 | `image-size` 1.2.1 | **No** | No — build-time only | `.trivyignore` | ### nanoid — fixed properly Reached via `@react-navigation/native@7.3.8`, which is a **runtime** dependency of `apps/mobile`, so this one genuinely ships to devices. Override is upper-bounded `>=3.3.17 <4.0.0` so it stays a 3.3.16 → 3.3.17 patch bump rather than resolving to 6.x and dragging react-navigation through a major — the same bounding rationale used for the js-yaml override. The tree's other copies (5.1.16, 6.0.0) are already past the advisory's fixed versions and are untouched. **Lockfile diff is 2 lines**, `nanoid@3.3.16` removed: ``` $ grep -oE "nanoid@[0-9.]+" pnpm-lock.yaml | sort -u nanoid@3.3.17 nanoid@5.1.16 nanoid@6.0.0 ``` ### image-size — ignored, with justification There is **no fixed version upstream**. Trivy reports both as `affected` with an empty "Fixed Version" column, so no override or upgrade can close them today. Sole dependent is `metro@0.84.4` — the React Native bundler. Confirmed nothing else in the tree pulls it. metro runs on developer machines and in CI, never in a shipped image and never on a server or customer device. Both CVEs are DoS via a crafted image (a malformed ICNS for -71330) parsed by `image-size`; reaching that path means feeding a hostile image to our own bundler during our own build, i.e. the attacker already controls the source tree. The `.trivyignore` entry records the dependent, the non-exploitability argument, and explicit removal criteria — matching the justification standard the file already sets for its existing `node-ip` and pnpm blocks. **All 9 pre-existing entries are preserved**; this appends 2. ## Release note Worth a line in the v0.104.0 notes alongside the js-yaml entry: nanoid is a real (if low-severity) fix that reaches the mobile app; image-size is a documented, non-shipping accept. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Todd Hebebrand <todd@lanternops.io> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Fixes #3233.
What
createOfflineWorkerwrapped the entire job body inwithSystemDbAccessContext, so every sweep's Redis fan-out (queue.addBulk) and the uninstall reaper's per-row writes ran while a pooled Postgres connection sat idle-in-transaction for the whole chunked loop.Same shape already fixed for
alertWorkerevaluate-all (#3216) andsnmpWorkerpoll-scheduler (#3215). This is the third and worst member of the family:detect-offlineruns every 30s (twice as often as alertWorker's sweep)whileloop, so one connection is held for the entire run and the hold grows with fleet sizeHow it was found
Surfaced by #3221's caller-attribution diagnostic during v0.104.0 release QA — 13 held-context warnings in ~15 minutes, 5-10s each, all naming
offlineDetector.ts:26:45. The hold itself is pre-existing (the blanket wrap dates to2b62f023d, 2026-07-30); #3221 just made it attributable for the first time. Nothing in the v0.104.0 round introduced it.The round 5000/10000ms clustering is ioredis reconnect backoff (
services/redis.ts:121) charged against the held connection.Against a bounded pool (
DB_POOL_MAXdefault 30, one region ~25) this eats directly into headroom and amplifies outages. For scale reference, #3216's writeup measured its equivalent hold at ~4.9s every 60s and 15,469 warnings since June 16.Changes
mark-offline,reevaluate-offline) keep a whole-job context — they read+write a single row and do no fan-out, mirroring alertWorker'sevaluate-device/auto-resolve.processDetectOffline/processReevaluateOfflineSweep— page read moves into its own short system context that CLOSES before that page'squeue.addBulk.processReapUninstallIntent— page read gets its own context; each candidate's two writes then share one short context of their own, so the decommission and the replacement-linkage clear still commit together while no connection is held across the whole chunk.Pagination now spans transactions. Intentional and safe: the keyset cursor (
devices.idascending) is stable, eligibility timestamps are computed once per run so the window doesn't drift between pages, and a device whose status flips mid-sweep is picked up next cycle.Also documents that
runOutsideDbContextis not a substitute here — it only exits the AsyncLocalStorage, it does not release the pooled connection held by the enclosingbaseDb.transaction().Tests
Adds
offlineDetector.dbcontext.test.ts(6 tests), following thesnmpWorker.dbcontext.test.tspattern: it tracks real context enter/exit depth and asserts WHICH depth each read, enqueue and write ran at. An identityfn => fn()mock cannot catch this class of bug.Verified to fail against the unfixed worker —
addBulk(2)@depth1instead of the expected@depth0.tsc --noEmitcleanLive verification
Rebuilt the API image on a local stack and confirmed:
Detected 1 stale devices→ fan-out → device row flipsonline→offlineThe narrowing's failure mode would be a worker that silently stops writing, so the end-to-end write was checked explicitly rather than inferred from the absence of errors.
Known remaining hold (separate follow-up, documented in #3233)
Two 10s warnings still appear from the per-device path this PR deliberately leaves wrapped:
createAlert's Redis cooldown/flapping calls (alertCooldown.ts:61,93,153,387) run unescaped inside that per-device transaction. That is shared by everycreateAlertcaller including alertWorker's still-wrapped paths, so it can't be fixed by touchingofflineDetector.tsalone.🤖 Generated with Claude Code