fix(api): resolve the instrument subject filter without a relation join

[gdevenyi]

Fixes #1416.

GET /v1/instruments/info?subjectId= returns HTTP 500 — and the datahub subject view stops rendering entirely — once a single instrument accumulates roughly 182,000 records.

Cause

InstrumentsService.find expressed "instruments this subject has records for" as a prisma relation filter:

records: query.subjectId ? { some: { subjectId: query.subjectId } } : undefined

On MongoDB, prisma compiles that into a $lookup which materialises every record belonging to an instrument into a single array before applying the predicate. MongoDB caps one document's $lookup output at 100 MiB and aborts the whole aggregation past it (error 4568):

Executor error during aggregate command on namespace: <db>.InstrumentModel
:: caused by :: Total size of documents in InstrumentRecordModel matching
pipeline's $lookup stage exceeds 104857600 bytes

The limit cannot be raised, and allowDiskUse does not apply to it.

Two things make this worse than the raw number suggests:

• The threshold is per-instrument, not per-collection, so the busiest instrument hits it first. A core intake questionnaire administered at every visit reaches 182,000 records at 5,000 subjects × 36 visits — an ordinary size here.
• There is no partial degradation. useInstrumentVisualization calls this endpoint on mount, and it backs both /datahub/$subjectId/table and /datahub/$subjectId/graph, so both pages fail and the user has no workaround.

The fix

Query the child collection instead of joining from the parent. The work is then bounded by the subject's records (tens) rather than the instrument's (hundreds of thousands), and there is no size ceiling:

const subjectInstrumentIds = query.subjectId ? await this.findInstrumentIdsBySubject(query.subjectId) : null;

const instruments = await this.instrumentModel.findMany({
  where: {
    AND: [subjectInstrumentIds ? { id: { in: subjectInstrumentIds } } : {}, accessibleQuery(ability, 'read', 'Instrument')]
  }
});

Verified against a live instance

Real API, real MongoDB replica set, database provisioned through the app's own POST /v1/setup, then one instrument pushed past the ceiling.

Above the ceiling — busiest instrument at 105.7 MiB, identical database for both:

	result
main	HTTP 500 (three times), exceeds 104857600 bytes
this branch	HTTP 200 in 77 ms, 811 bytes

Below the ceiling — same 55.3 MiB instrument, where main still answers:

	latency
main	1.83s / 1.69s / 1.66s
this branch	0.10s / 0.042s / 0.041s

So this is not only an outage fix: even well under the limit the endpoint was taking ~1.7 seconds to return 811 bytes, and it now takes ~40 ms.

Tests

Three added to instruments.service.spec.ts:

• the subject filter is resolved against InstrumentRecord and the instrument query contains no records relation filter (this is the regression guard — it asserts the absence of the construct that fails);
• no record query is issued when no subject is given;
• a subject with no records yields { id: { in: [] } } rather than an unfiltered query.

The existing findInfo tests are unchanged and still pass.

Checks

• tsc --noEmit on apps/api — clean
• eslint src — clean
• prettier --check — clean
• vitest run (whole workspace) — 255 passed, 1 skipped, 1 failed

The one failure is instrument-records.service.spec.ts > upload > should create records with pending set. This branch is cut from main, so it still carries that pre-existing failure — #1422 fixes it, and the suite is fully green there.

Notes

• @@index([instrumentId]) on InstrumentRecord (proposed in Performance: no indexes are ever created on any MongoDB collection #1406) is a useful mitigation for the old query but not a fix — it does not remove the 100 MiB ceiling. This change removes the ceiling from the picture entirely; the index still helps the new query.
• Because the failure only appears above a data-volume threshold, no small-fixture test would have caught it. A seeded at-scale test in the e2e suite would be worth adding separately.
• I swept for other relation filters on high-cardinality relations. FilesService.find uses files: { every: ... }, which is bounded by files-per-record and is safe today; it is the only other instance.

---

Rebased onto main, review items addressed

Rebased onto 26ec89168. The stale CI is gone and the branch is mergeable; lint-and-test is green.

Conflict resolution. main gained a seriesGroupId clause in the same where this PR rewrites.
Both are kept — the records: { some: ... } relation filter is the only thing removed, replaced by
the resolved id list. The spec conflicts were pure additions from main (series fixtures), kept as-is.

Copilot: the record lookup was unscoped. Fixed. findInstrumentIdsBySubject now takes the
caller's ability and applies accessibleQuery(ability, 'read', 'InstrumentRecord'). This mattered:
the ids feed straight into the instrument query, so an unscoped lookup would disclose which
instruments a subject had been administered outside the caller's groups — something the relation
filter it replaces did not do. Pinned by a test asserting the exact accessibleQuery clause, using a
conditioned ability so it cannot pass vacuously.

Copilot: misleading test name. Renamed to say what it actually asserts — that no record query
runs when no subject is given.

---

Round 2: rebased onto 5bfe4b2dc, review items addressed

1. The new 500 — confirmed and fixed. You were right, and I reproduced it exactly: with an
ability holding no read InstrumentRecord rule, accessibleQuery throws
ForbiddenError: It's not allowed to run "read" on "InstrumentRecord" rather than returning {}.
Only an undefined ability returns {}. findInstrumentIdsBySubject now returns an empty id list
for such a caller, before accessibleQuery is reached. The scoping itself stays, as you asked.

2. A test that would have caught it. Added, built through
AbilityFactory.createForPayload({ basePermissionLevel: 'STANDARD', ... }) rather than by hand —
your point about the existing test is exactly right, a hand-built ability tends to carry the very
rule whose absence breaks this. Verified by mutation: removing the guard fails it with that
ForbiddenError.

3. End-to-end. Added to authorization.spec.ts: a STANDARD case asserting the endpoint answers
200 rather than 500, and a group-manager case alongside it. The STANDARD one returns 500 without the
guard — verified. The at-scale reproduction is noted in the spec as a deliberate gap rather than left
silent, as you suggested; the populated UI path is already covered by instrument-completion.spec.ts,
so I pointed at that rather than duplicating it.

Noted on @@index([subjectId]) — agreed it is the natural companion, and I have left it out of this
PR as you asked. The group-visibility consequence stays as decided.

🤖 Generated with Claude Code

https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc

[Copilot]

Pull request overview

This PR fixes a MongoDB/Prisma scaling failure in the API’s instrument info query (GET /v1/instruments/info?subjectId=) by avoiding a high-cardinality relation filter that Prisma compiles into a $lookup capable of exceeding MongoDB’s 100 MiB per-document limit. Instead of joining from Instrument to InstrumentRecord, it first queries the child collection to find relevant instrument IDs for the subject, then filters instruments by those IDs.

Changes:

• Replace the records: { some: { subjectId } } Prisma relation filter with a two-step query: InstrumentRecord distinct instrument IDs → Instrument filtered by id in [...].
• Add a private helper (findInstrumentIdsBySubject) to encapsulate the distinct ID lookup.
• Add unit tests guarding against reintroducing the relation filter and verifying the new query behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
apps/api/src/instruments/instruments.service.ts	Reworks subject filtering to query InstrumentRecord for distinct instrument IDs, avoiding a MongoDB $lookup blowup.
apps/api/src/instruments/tests/instruments.service.spec.ts	Adds regression tests ensuring the subject filter is resolved via InstrumentRecord and that no record query runs when subjectId is absent.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[joshunrau]

CI is red on this branch, so I have not reviewed it yet.

Failing check: lint-and-test — run 29770064355

FAIL  api  src/instrument-records/__tests__/instrument-records.service.spec.ts
  > InstrumentRecordsService > upload > should create records with pending set,
    so they are not excluded from queries filtering on the field
AssertionError: expected "vi.fn()" to be called with arguments: [ { data: [ ObjectContaining{ pending: false } ] } ]

That test is not in your diff and no longer exists on main — it was removed in 722f0a0b0 ("test(api): update stale pending-on-create assertion for instrument records", 2026-07-20 22:39 EDT), which landed after this run was created. The run predates the fix, so the result is stale.

Also: GitHub reports this branch as CONFLICTING with main.

Please rebase or merge main into fix/instruments-subject-filter-lookup, resolve the conflicts, and push. That will both clear the conflict and re-run CI against the corrected main. I'll pick the review back up once it's green.

[joshunrau]

Suggested model: Opus 5

Sizing the remaining work on this PR for whoever picks it up, human or agent. Beyond the stale CI, the conflicts land in a service main has since changed, so resolution needs the query semantics held in mind rather than a textual merge.

[joshunrau]

Requesting changes so this tracks as needing author action.

Blocking: conflicts with main, plus stale CI. Details in my earlier comment — the failing test isn't in your diff and no longer exists on main. Rebase fix/instruments-subject-filter-lookup, resolve the conflicts, and push; that clears both.

No content review has been done on this branch yet — it's gated on CI being green.

[joshunrau]

Thanks for this — the diagnosis is correct and the $lookup ceiling is a real outage, so the
direction is right and the measurements are appreciated. Three things before it can land.

1. The scoped record lookup introduces a new 500 on the endpoint you are fixing.
   accessibleQuery(ability, 'read', 'InstrumentRecord') in
   apps/api/src/instruments/instruments.service.ts:537 does not return an empty filter when the
   caller holds no read InstrumentRecord rule — it throws
   ForbiddenError: It's not allowed to run "read" on "InstrumentRecord". Only an undefined ability
   returns {}. basePermissionLevel: 'STANDARD' grants create but not read on InstrumentRecord
   (apps/api/src/auth/ability.factory.ts:47-52), and GET /v1/instruments/info is gated on
   read Instrument, which STANDARD does hold. libnest's GlobalExceptionFilter turns a
   non-HttpException into a 500, so a STANDARD user calling that endpoint with a subjectId now gets
   an Internal Server Error where they previously got 200. Every other
   accessibleQuery(..., 'InstrumentRecord') call site in the repo sits behind a matching
   @RouteAccess({ action: 'read', subject: 'InstrumentRecord' }); this one does not. Please return an
   empty id list for a caller who cannot read records rather than letting the error escape.

2. Add a test that would have caught it, in
   apps/api/src/instruments/__tests__/instruments.service.spec.ts alongside the scoping test at line
   681. Build the ability with AbilityFactory.createForPayload({ basePermissionLevel: 'STANDARD', ... })
   and assert find({ subjectId }) resolves. The current scoping test hand-builds an ability that
   happens to include a read InstrumentRecord rule, so it passes either way.

3. An end-to-end test in testing/ is required for every change here. A spec loading
   /datahub/$subjectId/table as a GROUP_MANAGER and asserting the instrument list renders, plus a
   STANDARD case in src/specs/authorization.spec.ts asserting the endpoint does not return a 500,
   would cover both the fix and point 1. You are right that the at-scale reproduction cannot be
   expressed there — note that in the spec so the gap is deliberate rather than missing.

One more thing, for a separate PR rather than this one: InstrumentRecord has no @@index at all, so
the new lookup is a collection scan filtered by subjectId. It is still far better than the $lookup
it replaces and it has no ceiling, but @@index([subjectId]) is the natural companion to this change.

On the visibility question: adding the record-level scoping means that for a subject enrolled in more
than one group, instruments administered in another group disappear from a group manager's datahub
list. Joshua has confirmed he wants that — it is a disclosure fix and it matches
/v1/instrument-records — so keep the scoping, and fix point 1 by returning an empty id list rather
than by removing it.

If you hand this to Claude Code, Fable 5 is the right size — the fix turns on CASL ability
semantics, route-guard alignment and group-scoping intent at once, on the auth and data-integrity path.

Reviewed at commit 3c3e25f.

[joshunrau]

Thanks — this is in good shape. The empty-id-list guard, the AbilityFactory-built unit test and the
STANDARD end-to-end case all landed as asked, and you are right that instrument-completion.spec.ts
already covers the populated group-manager path: it picks the instrument out of the datahub combobox,
which is exactly this endpoint. Lint is clean and the api suite is green here. Two things.

1. testing/src/specs/authorization.spec.ts:68-80 — the group-manager case asserts an empty list for
   subjectId=any-subject, a subject that does not exist, so it passes whether the scoping works or
   is broken for every caller. The title says it "should serve the subject-filtered instrument list",
   which it cannot demonstrate. Either seed a record through the api fixture and assert that
   instrument comes back, or rename it to what it does assert — that the endpoint answers rather than
   erroring.
2. The accessibleQuery behaviour your guard depends on is documented nowhere.
   .agents/docs/architecture/auth-and-permissions.md:66-71 and apps/api/AGENTS.md:81-84 describe
   only the undefined ability → {} case. Please add the other one to those paragraphs: a defined
   ability holding no rule for the subject throws ForbiddenError, which is not an HttpException
   and so renders as a 500. That is the whole reason findInstrumentIdsBySubject needs its
   short-circuit, and it should not have to be rediscovered.

On the broader question your guard raises — whether accessibleQuery should return a deny-all filter
rather than throwing — the decision is to leave it as it is. Every current call site was checked and
none is reachable by a caller holding a defined ability with no rule for the subject it queries, so
the per-call-site guard stays the pattern. Nothing to do for this PR beyond item 2, which is what
keeps that from being rediscovered the hard way.

If you hand this to Claude Code, Fable 5 is the right size — the work sits on the auth and
group-scoping path, and the documentation item means writing the normative description of the repo's
most safety-critical helper.

Reviewed at commit 40e51f5.