Skip to content

2026 11 gg big bug hunt - #331

Merged
grahamegrieve merged 10 commits into
mainfrom
2026-11-gg-big-bug-hunt
Nov 3, 2026
Merged

grahamegrieve merged 10 commits into
mainfrom
2026-11-gg-big-bug-hunt

Conversation

@grahamegrieve

Copy link
Copy Markdown
Contributor

No description provided.

@jmandel

jmandel commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

RxNorm relationship filters with an AUI: value now return nothing (tx/cs/cs-rxnorm.js:498)

The new query takes the target concept from RXNREL.RXCUI2 and only accepts SAB = 'RXNORM' rows. Atom-level relationship rows leave RXCUI2 blank, though. In rxnorm_03022026.db that covers all 4.3M MTHSPL rows, 580k of the 2.2M RXNORM rows, and every MMSL, VANDF, NDDF, SNOMEDCT_US, ATC, USP and CVX row. So an AUI: filter matches no rows. CUI: filters are fine.

Works today on tx.fhir.org and tx-dev:

curl -s -X POST https://tx.fhir.org/r4/ValueSet/\$expand \
  -H 'Content-Type: application/fhir+json' -H 'Accept: application/fhir+json' \
  -d '{"resourceType":"Parameters","parameter":[{"name":"valueSet","resource":{"resourceType":"ValueSet","compose":{"include":[{"system":"http://www.nlm.nih.gov/research/umls/rxnorm","filter":[{"property":"has_ingredient","op":"=","value":"AUI:828"}]}]}}}]}'
# -> 105050 Parlodel 5 MG Oral Capsule, 105446 Parlodel 2.5 MG Oral Tablet

Same filter on the provider directly (getPrepContext → filter('has_ingredient','=','AUI:828') → executeFilters → iterate), against a copy of rxnorm_03022026.db:

  • main (e4a3b1f): 105050, 105446 (each listed twice, which is the old duplicate problem)
  • this PR (e492367): []

The rows involved:

sqlite3 rxnorm_03022026.db "select RXAUI1, RXCUI2, RXAUI2, RELA, SAB from rxnrel where RXAUI1='828' and RELA='has_ingredient'"
# 828||788773|has_ingredient|MMSL
# 828||790716|has_ingredient|MMSL      <- RXCUI2 empty, SAB not RXNORM

The test subset in tx/data/rxnorm has no atom-level rows, so cs-rxnorm-filters.test.js can't catch this.

Suggested fix: for AUI: values, keep the old path. Match RXAUI1, map RXAUI2 to its concept through rxnconso, and leave out the SAB restriction.

@jmandel

jmandel commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Memory guard: forced full GC on the request path (library/memory-guard.js:187–193, line 103)

While the server is over the reject threshold, every 5 s the next incoming request runs gc() synchronously before it is answered. A full collection stops everything, so every request in flight waits for it.

What it costs: a full collection of a 1.35 GB heap of small live objects (about the size of main's heap with VSAC and the package resources) took 0.5–0.8 s on a desktop:

node --expose-gc -e "const m=new Map();let i=0;while(process.memoryUsage().heapUsed<1.3e9){m.set('k'+i,{u:'http://x/'+i,v:'1.0.'+(i%50),c:[String(i)],n:i});i++}for(let r=0;r<3;r++){const t=Date.now();gc();console.log('full GC ms',Date.now()-t)}"

When most of RSS is outside the JS heap, as with the SNOMED/LOINC cache buffers on tx.fhir.org, the GC frees little and the server stays over the threshold. It then pauses every 5 s for as long as memory stays high. Separately, Node documents changing V8 flags after startup (v8.setFlagsFromString('--expose-gc')) as unsupported.

Suggestions:

  • Run the collection from a timer while refusing, not inside a request.
  • Only force it when heapUsed is a large share of RSS, since a GC can't release anything else.
  • Prefer --expose-gc on the command line (the Dockerfile / start script) to setting it at runtime.

(For #330: this composes with measuring RSS minus file-backed pages. I have a merge of #330 onto this branch that keeps both and passes all the memory-guard tests, ready for when this lands.)

@grahamegrieve

Copy link
Copy Markdown
Contributor Author

Here's one reply covering both review comments, ready to paste:


Thanks, both fixed.

RxNorm AUI: filters: agreed, that was a regression. AUI: values are back on the atom path. They match RXAUI1 and map RXAUI2 to its concept through rxnconso, from any source. That's deliberately an exception to the RxNorm-only rule, which still applies to CUI: filters and to $lookup relationships. CUI: values are unchanged: RXCUI1 → RXCUI2, SAB=RXNORM only. Against rxnorm_03022026.db, has_ingredient = AUI:828 gives 105050 and 105446 again, now once each. There's a new RXNREL(RELA, RXAUI1) index, which both the importer and the provider's load-time index check create; without it the filter scanned every row of the relationship type. The test data has no atom-level rows, so cs-rxnorm-filters.test.js now inserts an MMSL atom-level row, and tests both AUI: on its own and mixed with CUI: in an in filter.

Memory guard forced GC: two changes, plus a startup change:

  • Gated: a forced GC only runs when it could help. If the whole JS heap is smaller than the amount RSS is over the reject threshold, a collection can't get the server back under, so the guard skips it and logs why. That covers the case where most of RSS is the cache buffers.
  • Back-off: a GC (or skipped GC) that leaves the server still over doubles the wait before the next one, from 5 s up to 60 s. It resets as soon as the server recovers.
  • Startup: the Dockerfile CMD and npm start/dev now pass --expose-gc. The runtime setFlagsFromString route stays only as a fallback. For systemd deployments the flag goes in ExecStart.

I left the GC on the request path rather than moving it to a timer. A full collection stops the whole process wherever it's triggered, and the request that triggers it is normally being refused anyway. With the gate and back-off, futile pauses no longer repeat every 5 s. Two new tests cover the gate and the back-off. Happy to take the #330 merge once this lands.


@grahamegrieve
grahamegrieve merged commit 60963ae into main Nov 3, 2026
9 checks passed
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