perf(api): batch session creation and scope the upload response (alternative to #1424)

[gdevenyi]

Alternative to #1424. Same issue (#1414), but this one takes the SessionsService refactor that #1424 deliberately deferred. Merge one or the other, not both.

What #1424 does, and what this adds

	#1424	this PR
Response scoped to the upload	✅	✅
Validate before writing	✅	✅
Remove the re-read in SessionsService.create	✅	✅ (subsumed by the rewrite)
Batch session creation (the N+1)	❌ deferred	✅
Fix the session groupId bug	❌	✅

The refactor

SessionsService.createMany resolves subjects, the user and the group once for the whole batch, associates subjects with the group in a single updateMany, and inserts every session in one call. Session ids are generated up front so results come back in input order and a caller can pair each session with the entry that produced it by index. create now delegates to it, so there is one implementation rather than two that can drift.

Measured over HTTP, live instance, uploading 25 records

	records returned	bytes	mongo ops
main	85 → 110 (grows every upload)	51–66 KB	~242–276
this branch	25 (flat)	14.8 KB	~95

For reference, #1424 measured ~216 ops on the same workload — it removed one query per session but kept the per-record loop.

Correctness verified on the same run: 25 records → 25 distinct sessions, every record paired with the session holding its own subject.

The bug this fixes

create set a session's groupId only when the subject was not already a member of that group:

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

So every visit after a subject's first produced a session with no groupId. A GROUP_MANAGER's Session rule is { groupId: { in: groupIds } } — those sessions were invisible to them, and uncounted by every group-scoped query including the dashboard summary.

Reproduced on a live instance, two sessions for one subject in one group:

main behaviour  (quirk-subject): set, MISSING
this branch     (fixed-subject): set, set

This is the same class of problem as #1404 ("old data not visible"), and it is why I did not want to batch inside upload in #1424 — doing so would have duplicated the quirk into a second place rather than settling it.

This is a behaviour change, and it is the reason to review this PR carefully. Sessions created from now on will carry a groupId where they previously would not. Nothing becomes less visible; some previously-hidden sessions become visible to group managers and appear in group-scoped counts. Existing rows are untouched — a backfill for historical sessions would be a separate migration, and is worth considering.

One more thing found on the way

SubjectsService.createMany kept only the id of each entry and discarded the demographics a clinical subject carries:

const subjectsToCreate: CreateSubjectDto[] = subjectsToCreateIds.map((record) => ({ id: record }));

Harmless while its only caller passed ids alone, but the batched session path resolves subjects through it, so it now persists what it is given.

Tests

The existing upload tests are updated to the batched call and keep their assertions. Added:

• every record goes into a single createMany call (25 records → 1 call, 25 entries) — the N+1 guard;
• each record is paired with its own session by position;
• the response is scoped to the sessions created;
• an invalid record rejects before any session is created.

Checks

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

The one failure is upload > should create records with pending set. This branch is cut from main, so it carries that pre-existing failure; #1422 fixes it. These two PRs touch adjacent lines and will need a trivial rebase depending on merge order.

Still not done

The transaction wrapper (item 5 of #1414). Sessions are now created in one call and records in another, so a failure between them still leaves orphaned sessions — the catch still compensates by deleting them, as before. prismaClient.$transaction would make that atomic; the replica set is already configured for it.

---

Rebased onto main, review items addressed

Rebased onto 26ec89168; conflicts resolved, CI green. Still mutually exclusive with #1424 — that
choice is yours, and the comparison table in #1424 still holds.

Copilot: the rollback spanned the response read-back. Fixed. Only the record insert is inside the
catch now; deleting the sessions after the insert had succeeded would have stranded the records that
reference them. Both directions are pinned by tests.

Copilot: bulk records omitted pending. This was a genuine regression against main, which
writes it since 573f307c9. Restored, and written per instrument kind as create does, so a file
record stays pending until its files arrive.

Copilot: console.error on validation failure. Removed; the exception carries the zod issues and
names the offending record's index.

Copilot: a concurrent groupIds push could duplicate. Fixed. The subject list is a snapshot read
before the write, so the updateMany now re-checks membership in its own where
(NOT: { groupIds: { has: group.id } }), making the write itself the arbiter.

SessionsService had no test file at all, which is a poor state for a PR that rewrites it. Added
one covering the groupId fix, input-order pairing, single-call batching, the association guard, and
one-user-per-batch resolution. Verified by mutation — reverting the groupId fix or the association
guard turns it red.

---

Round 2: rebased onto 5bfe4b2dc, review items addressed

Conflict was a one-line import clash with #1420, now merged.

1. End-to-end. Added to dashboard.spec.ts: two sessions for the same subject in the same group,
asserted visible to that group's manager. This is the sharpest pin available for the groupId fix —
reinstating the old behaviour returns one of the two sessions, verified by mutation.

2. File instruments. upload now rejects them with UnprocessableEntityException alongside the
existing series rejection, and pending: instrument.kind === 'FILE' is gone. Agreed with the
reasoning — the payload cannot carry a file, the path never attaches one, and the client discards the
response ids, so the record could only ever be incomplete. Pinned by a test asserting the rejection
and that no session or record is written.

3. CreateManySessionsData. Now derived: Pick<CreateSessionData, 'groupId' | 'type' | 'username'>
plus per-entry Pick<CreateSessionData, 'date' | 'subjectData'>, so it cannot drift.

4. The retrospective comment. Removed — that reasoning lives in the commit message. Did the same
to the response-scoping comment in instrument-records.service.ts, which #1424's review flagged for
the same reason.

5. { ...subject, groupIds: [] }. Now names the CreateSubjectDto fields explicitly. Good
catch on demo.service.ts:126 handing it a full Prisma row.

6. Reaching into prismaClient.subject. The batched association moved behind SubjectsService,
next to the other subject writes.

7. addGroupForSubject. Removed — item 6 replaces it with the batched addGroupForSubjects, so
the single-subject version has no callers and no remaining reason to exist.

Also carried over from #1424's review, since that PR is being closed: the rollback no longer spans
the response read-back (already fixed here), and SessionsService now has the test file it lacked.
And from #1422: the stale spec comment 722f0a0b0 left behind is deleted here, so it is not lost
with that PR.

Both isolatedGroupManager-style seeding and createSession on the api client also appear in #1425
— trivial to reconcile, but worth knowing before merging both.

🤖 Generated with Claude Code

https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc

[Copilot]

Pull request overview

This PR is an alternative implementation to #1424 for addressing #1414 in apps/api, focusing on making bulk instrument record upload scale better and behave more predictably by batching session creation, scoping the upload response to the request, and fixing session groupId assignment so group-scoped visibility/counts are correct.

Changes:

• Refactors SessionsService to add createMany, resolving user/group/subjects once per batch and inserting sessions in bulk (avoids per-record N+1 work and fixes the groupId omission bug).
• Updates InstrumentRecordsService.upload to validate all records up front, create sessions in one batch, insert records in one createMany, and return only records for the created sessions.
• Adjusts subject bulk creation so createMany preserves provided subject fields (not just id) while de-duping within-request duplicates.

Reviewed changes

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

File	Description
apps/api/src/subjects/subjects.service.ts	Preserve full CreateSubjectDto entries during bulk subject creation instead of dropping non-id fields; de-dupe via map keyed by id.
apps/api/src/sessions/sessions.service.ts	Adds batched createMany for sessions (pre-generated ids + readback in input order) and ensures groupId is consistently set.
apps/api/src/instrument-records/instrument-records.service.ts	Upload now validates before writing, batches session creation, scopes response to created sessions, and reduces DB round-trips.
apps/api/src/instrument-records/tests/instrument-records.service.spec.ts	Updates upload unit tests to assert batching semantics, positional pairing, scoped response, and “validate before write” behavior.

---

💡 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 29772223521

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. The result is stale.

Also: GitHub reports this branch as CONFLICTING with main.

Please rebase or merge main into perf/sessions-create-many, resolve the conflicts, and push. That will 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. Conflict resolution against a changed main, across four service files — and this PR and #1424 are mutually exclusive, so the rebase can't be done without regard to which one is being kept.

[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, so the red result predates the fix. Rebase perf/sessions-create-many, 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. Note this PR and #1424 remain mutually exclusive, so which one survives is worth settling before you invest in the rebase.

[joshunrau]

Thanks — this is careful work, and the evidence you gathered makes it easy to review. The groupId
finding is real: I confirmed it against main and against the GROUP_MANAGER Session rule in
ability.factory.ts. The merge result is lint-clean and the whole test suite passes (356), including
the upload test your branch reported failing.

Both maintainer decisions are now made: the groupId change lands here — widening what group
managers see and what the dashboard counts is the intended behaviour — and this PR is the one being
taken, so #1424 is being closed as superseded. A backfill for historical sessions stays a separate
ticket.

That leaves the following before it can merge:

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 groupId fix is exactly what an e2e can pin —
   a second session for the same subject in the same group, asserted visible to a group manager.
   testing/src/specs/dashboard.spec.ts or start-session.spec.ts is the natural home.
2. instrument-records.service.ts:441 — pending: instrument.kind === 'FILE' strands bulk-uploaded
   FILE records. find filters pending records out (:270), the bulk path never attaches files, and
   the web client discards the response ids, so such a record is invisible forever. This was settled
   on your #1422: upload should reject file instruments with UnprocessableEntityException
   alongside the existing SERIES rejection. Please drop the expression and add that guard, with a
   test — the bulk payload schema cannot carry a file, so such a record is incomplete however it is
   written.
3. sessions.service.ts:16-21 — CreateManySessionsData restates $CreateSessionData by hand.
   Derive it from the schema type so the two cannot drift.
4. sessions.service.ts:89-92 — the comment explains what the code used to do. That belongs in the
   commit message, which already says it well; comments here are reserved for the non-obvious.
5. subjects.service.ts:85 — { ...subject, groupIds: [] } spreads whatever the caller passed.
   demo.service.ts:126 hands sessionsService.create a full Prisma Subject row, and only misses
   this write because the subject always exists by then. Pick the CreateSubjectDto fields
   explicitly.
6. sessions.service.ts:72 reaches into prismaClient.subject.updateMany directly. Model<'X'> is
   the repository in this codebase and there is no separate data layer — put the batched association
   behind SubjectsService, next to the existing single-subject method.
7. subjects.service.ts:19 — addGroupForSubject has no callers left after this change. Remove it,
   or say why it stays.

If you hand this to Claude Code, Fable 5 is the right size — the work sits on the
group-scoping/session path and spans four files plus a new e2e spec.

Reviewed at commit b15673e.

[joshunrau]

Thanks — the batching is clean, the groupId fix is a real bug well argued, and adding the
SessionsService spec that never existed is the right call. Lint is clean and the api suite passes
(172/172). A few things before this is ready:

1. apps/api/src/sessions/sessions.service.ts:144-148 — resolveSubjects reads the subjects back
   through findByIds and then uses nothing from those rows except their ids, which it already has
   in ids. addGroupForSubjects re-checks membership in its own where, and the session rows take
   subjectId from entry.subjectData.id rather than from the resolved row — so a subject missing
   from that read is quietly left out of the group association while its session is written anyway.
   Either pass ids directly and drop the read (which also drops SubjectsService.findByIds, added
   here and with no other caller), or make it a real guard that throws when an id is missing. As it
   stands it is a round trip that buys nothing, in a PR about round trips.

2. apps/api/src/sessions/sessions.service.ts:143 — the comment says "in two queries regardless of
   count", but the path runs three: findMany, createMany, then findByIds. Worth correcting, or
   dropping once the above lands.

3. apps/api/src/sessions/sessions.service.ts:93 — ids.map((id) => byId.get(id)!) gives a session
   the read-back did not return the type Session. create then returns session!, so
   POST /sessions would answer 201 with an empty body rather than failing. Please throw if the
   read-back does not return every id.

4. testing/src/specs/dashboard.spec.ts:17 — this case never opens the dashboard; it is an HTTP
   assertion against /api/v1/sessions in a UI spec file, where the convention is one file per
   user-facing flow. Since the symptom you are pinning is a dashboard count, please assert through
   the dashboard (or datahub) page object, or move it to its own spec.

5. The headline change has no end-to-end coverage — there is no upload spec at all, so the batched
   upload, the scoped response and the new FILE rejection rest on unit tests with mocked services.
   Please add an e2e that uploads a small batch and asserts the response carries only that upload's
   records.

The two maintainer questions are both settled in your favour. The groupId change is accepted as the
bug fix you argued it was — sessions already stored with a null groupId will be left alone, so no
backfill is wanted here or in a follow-up. The new 422 on bulk-uploading a file instrument is also
accepted; it will get a release note, since it is a contract change for anything outside apps/web.

On ordering: this PR is planned to land last, after #1426 and then #1425, so it is the one that
rebases. It overlaps #1426 on apps/api/src/subjects/subjects.service.ts and
testing/src/support/api-client.ts, and #1425 on the api-client createSession helper and the
group-manager seeding — worth knowing before you rework the e2e items above.

If you hand this to Claude Code, Fable 5 is the right size — it spans session creation, CASL
group scoping and the end-to-end tier at once, on the area apps/api/AGENTS.md calls the
highest-severity part of the codebase.

Reviewed at commit ab5728b.

[joshunrau]

This branch no longer merges cleanly into main — GitHub reports it as conflicting, so it can't be reviewed or merged as it stands. main has moved since the last pass (#1444 and the e2e-test work landed).

Please merge or rebase onto current main and push. Review picks up again once the branch is mergeable and CI is green — the new commits since the last review haven't been looked at yet.

Reviewed at commit e6150dd.