perf(api): scope the upload response and validate before writing

[gdevenyi]

Addresses part of #1414.

1. The response described the database, not the request

upload returned every record in the group for that instrument:

return this.instrumentRecordModel.findMany({ where: { groupId, instrumentId } });

so the response grew with the size of the database rather than the size of the upload, and the caller (useUploadInstrumentRecordsMutation) has no use for the pre-existing rows.

Measured against a live instance, uploading 25 records into an instrument that already held 1,200:

	records returned	bytes
main	1,300	871,056
this branch	25	14,691

Note the unbounded growth on main — two successive 25-record uploads returned 1,275 then 1,300 records. The branch returns 25 both times.

2. Records are validated before anything is written

A malformed record in the middle of a batch previously rejected the request only after sessions had been created for the records ahead of it, which the catch handler then had to roll back. Validation now happens up front, so an invalid batch creates nothing at all.

3. One fewer round trip per session, for every caller

SessionsService.create created a session and then immediately re-read it to attach the subject:

const { id } = await this.sessionModel.create({ data: { … } });
return (await this.sessionModel.findUnique({ include: { subject: true }, where: { id } }))!;

The create now returns it directly with the same include. Identical return shape, one less query — and this benefits the upload path, the gateway synchronizer, the demo seeder and the sessions controller alike. Measured ~25 fewer MongoDB operations on a 25-record upload, matching one fewer query per session.

What this deliberately does not do

Sessions are still created one per record. I looked at batching them into a single createMany and decided against it in this PR.

SessionsService.create carries semantics that would have to be reproduced inline in upload to batch it: find-or-create for each subject, and appending the group to the subject when it isn't already a member. It also has an existing quirk worth knowing about before anyone touches it —

let group: Group | null = null;
if (groupId && !subject.groupIds.includes(groupId)) { group = await this.groupsService.findById(groupId); … }
// …
group: group ? { connect: { id: group.id } } : undefined,

group is only non-null when the subject was not already in the group, so a session created for an existing group member is persisted with groupId: null. That looks like a bug, and it is why uploaded records show groupId: null in practice — but "fix it" and "batch it" are different changes, and doing them together inside upload would duplicate the logic and invite drift.

Batching belongs in a refactor of SessionsService (a real createMany there, with the group behaviour settled deliberately), reviewed on its own. #1414 stays open for it, as does the transaction wrapper (item 5).

Tests

Two added:

• the response is scoped to the sessions this upload created, and the query carries neither groupId nor instrumentId;
• an invalid record rejects before sessionsService.create, subjectsService.createMany or instrumentRecordModel.createMany is called.

The existing upload tests are unchanged and still pass.

Checks

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

The one failure is 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 is green there. The two PRs touch adjacent lines in the same object literal and will need a trivial rebase depending on merge order.

---

Rebased onto main, review items addressed

Rebased onto 26ec89168; CI green. Still mutually exclusive with #1427.

Copilot: Promise.all could orphan a session. Real, and now fixed. all rejects on the first
failure while its siblings are still in flight, so a session created after the catch had begun was
missing from createdSessionsArray and survived the rollback. Switched to allSettled, then rethrow,
so every session that exists is recorded before anything is undone. Pinned by a test that fails
against Promise.all — verified by reverting the change and watching it go red.

Copilot: validation logging and dropped issues. console.error(parseResult.error.issues) is gone.
The exception now carries the issues, matching what create already does, and the message names the
offending record's index — which a batch needs and a single-record create does not.

Copilot: comment grammar. Fixed.

🤖 Generated with Claude Code

https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc

[Copilot]

Pull request overview

This PR improves the scalability and correctness of InstrumentRecordsService.upload() in apps/api by (1) validating all records before any writes, (2) returning only the records created by the upload (instead of all existing records for the group/instrument), and (3) removing an extra DB round-trip in SessionsService.create() by returning the created session with its subject included.

Changes:

• Validate the entire upload batch up front to avoid partial writes when encountering an invalid record mid-batch.
• Scope the upload response to records associated with the newly created session IDs.
• Optimize SessionsService.create() to return the created session with include: { subject: true } in a single query.
• Add tests to assert response scoping and “validate-before-write” behavior.

Reviewed changes

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

File	Description
apps/api/src/sessions/sessions.service.ts	Avoids a second query by returning the created session (with subject included) directly from create.
apps/api/src/instrument-records/instrument-records.service.ts	Validates records before writes and returns only records created by the current upload.
apps/api/src/instrument-records/tests/instrument-records.service.spec.ts	Adds tests for response scoping and early rejection of invalid batches.

---

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

[gdevenyi]

#1427 is an alternative to this PR — it does everything here plus the SessionsService refactor I deferred, so the two are mutually exclusive. Merge one.

The short version:

	this PR (#1424)	#1427
Response scoped to the upload	✅	✅
Validate before writing	✅	✅
Batch session creation (the N+1)	❌ deferred	✅ ~242–276 → ~95 mongo ops
Fix the session groupId bug	❌	✅

The deciding factor is the groupId quirk I described above. #1427 settles it rather than working around it: create currently sets a session's groupId only when the subject was not already a group member, so every visit after a subject's first produced a session invisible to group managers and uncounted by group-scoped queries. Reproduced live — set, MISSING on main vs set, set on #1427.

That makes #1427 the better change but a larger one, since it alters behaviour for every caller of SessionsService.create. This PR is the conservative option if you would rather take the safe wins now and schedule the refactor separately.

[joshunrau]

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

Failing check: lint-and-test — run 29770604263

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). This run was re-run on 2026-07-21, but a re-run reuses the original merge commit, so it still evaluated against the pre-fix main.

Please merge or rebase onto current main and push, so CI produces a fresh merge commit. I'll pick the review back up once it's green.

[joshunrau]

Suggested model: Sonnet 5

Sizing the remaining work on this PR for whoever picks it up, human or agent. The branch is mergeable and the only blocker is stale CI, so remediation is merging main and pushing. Note the choice between this PR and #1427 is a maintainer decision, not work inside this branch — that's excluded from the sizing.

[joshunrau]

Requesting changes so this tracks as needing author action.

Blocking: stale CI only — the branch is mergeable. Details in my earlier comment: the failing test isn't in your diff and was removed from main in 722f0a0b0. The Jul 21 re-run reused the original merge commit, so it stayed stale — a fresh push is what's needed, not another re-run.

No content review has been done on this branch yet — it's gated on CI being green. This PR and #1427 remain mutually exclusive; which one survives is worth settling first.

[joshunrau]

Thanks — this is a tight, well-evidenced change, and the three parts of it are each clearly
motivated. The merge result is lint-clean and the whole suite passes (344), including the upload test
your branch reported failing; that one was already reworked on main. The allSettled fix is right,
and pinning it with a test that fails against Promise.all is exactly the right way to land it.

The one decision this was waiting on has been made, and it goes to #1427: it is a superset of this
change and also fixes the session groupId bug, and the group-visibility widening that comes with it
is wanted. So this PR is being closed as superseded rather than merged — not for anything wrong with
it. The conservative case for it was real, and the review of it is the reason #1427's own rework list
is as specific as it is.

A few things from this review are worth carrying into #1427, since they apply there too:

1. There is no end-to-end test. Every change needs a unit test and an e2e test in testing/, and
   testing/src/specs/ has no upload spec at all. The scoped response is observable through the
   /upload/$instrumentId page — uploading a batch into an instrument that already holds records is
   the natural pin.
2. sessions.service.ts:54 changes, but apps/api/src/sessions/ has no __tests__ directory at
   all, so nothing asserts that create still returns the session with its subject attached. Add
   apps/api/src/sessions/__tests__/sessions.service.spec.ts covering that the create carries
   include: { subject: true } and that no second read follows it.
3. instrument-records.service.ts:478 — the rollback still spans the response read-back. createMany
   succeeds at :472, then a failure in the findMany at :478 lands in the catch at :483 and
   deletes every session, stranding the records that were just written against sessions that no
   longer exist. Move the response read outside the try, or narrow the catch to the writes. It is
   inherited from main, but this PR reworks that rollback, and #1427 already fixed it.
4. Two of the new comments explain what the code used to do rather than what it does:
   instrument-records.service.ts:476-477 ("rather than every record in the group for this
   instrument") and sessions.service.ts:72-73 ("re-reading it afterwards cost a second round trip").
   Both commit messages already say that well; comments here are reserved for the non-obvious. The
   allSettled comment at :430-433 is the right kind — keep it.

If you hand the #1427 rework to Claude Code, Fable 5 is the right size — it sits on the
session/data-integrity path and spans two services plus a new e2e spec and a new unit-test file.

Reviewed at commit cba4f8f.

[joshunrau]

PR #1424 — perf(api): scope the upload response and validate before writing

CLOSE · confidence: high · gdevenyi · +117/-24, 3 files
#1424 · reviewed at cba4f8f

What it does

• InstrumentRecordsService.upload now returns only the records the request created (where: { sessionId: { in: [...] } }) instead of every record in the group for that instrument, so the response no longer grows with the database.
• Every record is validated up front, before any subject or session is written, so a malformed record in the middle of a batch creates nothing at all.
• Session creation switched from Promise.all to Promise.allSettled + rethrow, so a session created after the first failure is still recorded in createdSessionsArray and gets rolled back rather than orphaned.
• Validation failure no longer writes zod issues to stdout; the 422 carries the issues and names the offending record's index.
• SessionsService.create returns the created session with include: { subject: true } instead of creating and then re-reading it — one fewer query per session for the upload path, gateway synchronizer, demo seeder and sessions controller.
• Sessions are still created one per record; batching and the session groupId bug are deliberately left to perf(api): batch session creation and scope the upload response (alternative to #1424) #1427.

Why this verdict

Nothing here is about the quality of the work — the engineering is good: focused, lint-clean, whole suite green, and the added tests are real (the rollback test was verified by mutation against Promise.all). It is superseded. The author offered #1427 as a mutually exclusive alternative and Joshua has taken that one: #1427 does everything this PR does, plus batched session creation (~95 vs ~216 mongo ops on a 25-record upload) and the session groupId fix. The two rewrite the same files, so this one closes unmerged. Closing is Joshua's to do; nothing was closed by the review.

Settled (2026-07-28)

• perf(api): batch session creation and scope the upload response (alternative to #1424) #1427 wins. Joshua chose the superset, accepting the group-visibility widening that comes with
  it. This PR is superseded and should be closed. The one finding below that is not already handled
  in perf(api): batch session creation and scope the upload response (alternative to #1424) #1427 — the missing e2e test — is an action item on perf(api): batch session creation and scope the upload response (alternative to #1424) #1427 as well, so nothing is lost by
  closing this.

Action items

1. No e2e test. CLAUDE.md requires a unit test and an e2e test in testing/ for every change, and testing/src/specs/ has no upload spec at all. The scoped response is directly observable through the /upload/$instrumentId page, so a spec that uploads a batch into an instrument that already holds records is the natural pin.
2. apps/api/src/sessions/sessions.service.ts:54 is changed and apps/api/src/sessions/ has no __tests__ directory at all — nothing anywhere asserts that create still returns the session with its subject attached. Add apps/api/src/sessions/__tests__/sessions.service.spec.ts pinning that the create call carries include: { subject: true } and that no second read follows it. (perf(api): batch session creation and scope the upload response (alternative to #1424) #1427 adds this file; if perf(api): scope the upload response and validate before writing #1424 lands instead, it needs its own.)
3. apps/api/src/instrument-records/instrument-records.service.ts:478 — the rollback still spans the response read-back. createMany succeeds at :472, then a failure in the findMany at :478 falls into the catch at :483 and deletes every session, leaving the records that were just written pointing at sessions that no longer exist. Move the response read outside the try, or narrow the catch to the writes. This is inherited from main rather than introduced here, but this PR restructures exactly that rollback and perf(api): batch session creation and scope the upload response (alternative to #1424) #1427 already fixed it.
4. Two of the new comments narrate the old behaviour rather than the code: instrument-records.service.ts:476-477 ("rather than every record in the group for this instrument") and sessions.service.ts:72-73 ("re-reading it afterwards cost a second round trip"). That is commit-message material, and both commit messages already say it well. CLAUDE.md reserves comments for the non-obvious — the allSettled comment at :430-433 is the right kind and should stay.

Suggested model: Fable 5 — the change sits on the session/data-integrity escalation path, and the action items span two services plus a new e2e spec and a new unit-test file.

Evidence

ok: worktree on the merge result (c24df4c5c, merges cba4f8f into 1ff80c5); pnpm install --frozen-lockfile, pnpm generate:env, pnpm lint (33/33 tasks, exit 0); pnpm test (49 files, 344 passed, 1 skipped) — the "1 failed" the PR body reports against its own branch is gone in the merge result, since the pending test was already reworked on main; all three changed files read whole in the merged tree and diffed against main; every caller of SessionsService.create traced (sessions.controller.ts:21, gateway.synchronizer.ts:97, demo.service.ts:126, instrument-records.service.ts:438); confirmed the only in-repo consumer of the upload response, apps/web/src/hooks/useUploadInstrumentRecordsMutation.ts:12, discards the body entirely, so scoping it breaks no caller; response shape is not documented in docs/ or apps/outreach/; root CLAUDE.md and apps/api/AGENTS.md applied; compared feature-by-feature against #1427.

not checked: nothing was run against a real MongoDB — the author's byte counts and mongo-op counts are taken on trust, and Model<'Session'>.create with include is verified only by tsc and mocks. Playwright was not run. pending: false at instrument-records.service.ts:458 and the missing storageService.isEnabled guard in upload are unchanged from main and were not treated as this PR's problem. The non-atomicity between the session inserts and the record insert (item 5 of #1414) is unchanged and out of scope by the author's own statement.

Reply to author

Thanks — this is a tight, well-evidenced change, and the three parts of it are each clearly
motivated. The merge result is lint-clean and the whole suite passes (344), including the upload test
your branch reported failing; that one was already reworked on main. The allSettled fix is right,
and pinning it with a test that fails against Promise.all is exactly the right way to land it.

The one decision this was waiting on has been made, and it goes to #1427: it is a superset of this
change and also fixes the session groupId bug, and the group-visibility widening that comes with it
is wanted. So this PR is being closed as superseded rather than merged — not for anything wrong with
it. The conservative case for it was real, and the review of it is the reason #1427's own rework list
is as specific as it is.

A few things from this review are worth carrying into #1427, since they apply there too:

1. There is no end-to-end test. Every change needs a unit test and an e2e test in testing/, and
   testing/src/specs/ has no upload spec at all. The scoped response is observable through the
   /upload/$instrumentId page — uploading a batch into an instrument that already holds records is
   the natural pin.
2. sessions.service.ts:54 changes, but apps/api/src/sessions/ has no __tests__ directory at
   all, so nothing asserts that create still returns the session with its subject attached. Add
   apps/api/src/sessions/__tests__/sessions.service.spec.ts covering that the create carries
   include: { subject: true } and that no second read follows it.
3. instrument-records.service.ts:478 — the rollback still spans the response read-back. createMany
   succeeds at :472, then a failure in the findMany at :478 lands in the catch at :483 and
   deletes every session, stranding the records that were just written against sessions that no
   longer exist. Move the response read outside the try, or narrow the catch to the writes. It is
   inherited from main, but this PR reworks that rollback, and perf(api): batch session creation and scope the upload response (alternative to #1424) #1427 already fixed it.
4. Two of the new comments explain what the code used to do rather than what it does:
   instrument-records.service.ts:476-477 ("rather than every record in the group for this
   instrument") and sessions.service.ts:72-73 ("re-reading it afterwards cost a second round trip").
   Both commit messages already say that well; comments here are reserved for the non-obvious. The
   allSettled comment at :430-433 is the right kind — keep it.

If you hand the #1427 rework to Claude Code, Fable 5 is the right size — it sits on the
session/data-integrity path and spans two services plus a new e2e spec and a new unit-test file.

Reviewed at commit cba4f8f.

[gdevenyi]

Rebased onto 5bfe4b2dc; CI green. Kept open pending @gdevenyi's call rather than closed — flagging that so it is not read as disagreement with being superseded.

All four points from your review are carried into #1427:

1. e2e coverage — added there, pinning the groupId fix (reinstating the old behaviour returns one of two sessions).
2. SessionsService test file — added there.
3. rollback spanning the response read-back — already fixed there; the read sits outside the catch.
4. the two retrospective comments — removed there, including the response-scoping one you flagged here.

Nothing here needs re-reviewing.