Skip to content

fix(api): materialize dynamic group membership instead of stranding it on a dead transaction - #3181

Open
ToddHebebrand wants to merge 2 commits into
mainfrom
fix/dynamic-group-membership-materialization
Open

fix(api): materialize dynamic group membership instead of stranding it on a dead transaction#3181
ToddHebebrand wants to merge 2 commits into
mainfrom
fix/dynamic-group-membership-materialization

Conversation

@ToddHebebrand

Copy link
Copy Markdown
Collaborator

The defect

Creating a dynamic device group with Architecture equals x64 showed a server-side preview of 3 matching devices, but

SELECT count(*) FROM device_group_memberships WHERE group_id = '<new group>';

stayed at 0 indefinitely. Nothing appeared in the API logs — not even the console.error the create route already had.

Mechanism (verified against a real Postgres, not inferred)

apps/api/src/routes/groups.ts:511 fired the evaluation and deliberately did not await it:

evaluateGroupMembership(group.id).catch((err) => { console.error(...); });

authMiddleware runs the whole handler inside withDbAccessContext, which is baseDb.transaction(...) (apps/api/src/db/index.ts:275) because the RLS GUCs are set with SET LOCAL. The sequence is:

  1. evaluateGroupMembership runs synchronously up to its first await, so the SELECT ... FROM device_groups (services/groupMembership.ts:172) is dispatched on the still-open request transaction and succeeds.
  2. The handler returns; drizzle/postgres.js commits that transaction and releases its reserved pooled connection.
  3. The detached continuation resumes. AsyncLocalStorage still resolves db to the committed transaction handle, so the next query — on the create path that is evaluateFilterwithFilterStatementTimeoutdb.transaction(...), i.e. a SAVEPOINT (services/filterEngine.ts:556) — is queued on a connection that transaction no longer owns.
  4. That query is never executed and never errors. The promise never settles, so the route's .catch() never runs. Zero rows, zero errors, zero logs.

Reproduced deterministically: with the evaluation started inside a live withDbAccessContext and awaited afterwards, step 1 returned the group row, then the whole promise timed out unsettled and device_group_memberships stayed at 0. It is not an RLS policy rejection (that raises new row violates row-level security policy, loudly) and not an org_id mismatch — the org stamping was already correct.

The preview endpoint worked precisely because it awaits evaluateFilterWithPreview inside the request (routes/groups.ts:966), so it never leaves the live transaction.

The filter-only branch of PATCH /groups/:id (routes/groups.ts:637) had the identical shape and was equally inert. The siteChanged branch already awaited, which is why site moves worked.

The fix

apps/api/src/routes/groups.ts

  • POST /groups: await evaluateGroupMembership(...), then read getDeviceCountForGroup(...) so the 201 carries the real count instead of a hardcoded 0. A failure is caught and logged but still returns 201 — the group was created, and failing the request would invite a duplicate on retry.
  • PATCH /groups/:id: the fire-and-forget branch is gone; both filter and site changes await. A failure propagates (PATCH is idempotent, so a 500 is retryable) rather than leaving membership silently stale against the new filter.

apps/api/src/services/groupMembership.ts

  • The per-device groupMembershipLog INSERTs (Promise.all(toAdd.map(logMembershipChange))) are batched into one multi-row INSERT. This is what makes awaiting cheap: the whole evaluation is now a fixed handful of statements regardless of how many devices matched, instead of O(devices) round trips.
  • Loud on invisible group: a !group result now logs an error naming the group id and the active DB access context (dbAccessContext=none|organization:<id>|...) instead of returning a bland zero summary. A caller only ever evaluates a group it just created or read, so an invisible row is always an access-context bug, never "the group is gone".
  • Loud on shortfall: after writing, the service re-counts the group's membership rows and logs an error if the count is below what the filter matched (plus surviving pinned members). matched is the same number the preview endpoint reports for that filter, so this diagnostic states the exact reported symptom: "filter matched 3 device(s), expected 3 membership row(s), found 0".
  • MembershipUpdateSummary gains optional matched / materialized.

No new tables, no new columns, no migration — nothing touches the cascade-registration or export-policy lists.

Tenant safety

The write is now more tenant-constrained than before, not less:

  • The evaluation runs inside the caller's own org-scoped request context, so Postgres RLS enforces the boundary on every read and write. The alternative (escalating to withSystemDbAccessContext for detached work) would have removed that DB-level backstop in favour of app-layer scoping only; awaiting keeps the strict version.
  • App-layer scoping is unchanged and correct: evaluateFilter filters on eq(devices.orgId, group.orgId), and every membership row is stamped with the group's orgId, never a device- or caller-derived one.
  • Proven against real Postgres: a matching device in a different partner's org is never absorbed, every materialized row carries the group's org_id, the foreign tenant cannot see the group at all, and a membership INSERT claiming the owner's org_id is rejected with new row violates row-level security policy.

Pre-existing gap, now pinned by a test (not introduced here)

device_group_memberships policies key on org_id alone (breeze_has_org_access(org_id)), and there is no composite FK tying (group_id, org_id) back to device_groups. A foreign tenant that somehow learned a group's UUID can insert a row naming that group as long as it stamps its own org_id. Such a row is quarantined — the owning org's context cannot read it, so it can never surface as a member of their group — and the group's own materialization never produces it. The integration test asserts both the rejection and the quarantine so the behaviour can't drift unnoticed. Closing it properly needs a UNIQUE (id, org_id) on device_groups plus a composite FK and a backfill; that is deliberately out of scope for this fix.

Test evidence

New apps/api/src/__tests__/integration/dynamicGroupMembershipMaterialization.integration.test.ts (real Postgres, real JWT, real authMiddleware, real breeze_app forced RLS — the mocked route suites resolve evaluateGroupMembership from a vi.fn(), so a detached call looks identical to an awaited one there):

  1. membership rows are readable the instant the 201 lands, and data.deviceCount agrees;
  2. a matching device in another partner's org is never absorbed; every row carries the group's org_id;
  3. the foreign tenant cannot see the group, and an INSERT claiming the owner's org_id is rejected by RLS (with the quarantine gap above asserted explicitly);
  4. PATCH with a new filter re-materializes before responding;
  5. an evaluation run under a context that cannot see the group logs the loud error and writes nothing.

New apps/api/src/services/groupMembership.materialization.test.ts (6 unit tests) covers the invisible-group log, the shortfall log, the quiet happy path, pinned members counting toward the expected total, and the batched log INSERT.

apps/api/src/routes/groups_get_create.test.ts gains two cases: the create handler must not respond while the evaluation is still pending, and a failed materialization still returns 201 while logging.

Verified in both directions:

Suite Pre-fix (source reverted to origin/main) Post-fix
dynamicGroupMembershipMaterialization.integration.test.ts 4 failed / 1 passedexpected [] to deeply equal [ …(2) ], i.e. exactly the 0-rows symptom 5 passed
groupMembership.materialization.test.ts + groups_get_create.test.ts 6 failed / 16 passed 22 passed

Full runs on the final tree:

  • tsc --noEmit -p apps/api/tsconfig.json → exit 0
  • groupMembership.siteScope, groupMembership.materialization, groups, groups_get_create, groups_update_delete, groups_devices, groups_list, groups_preview_pin, groups_log_multitenant, src/events9 files / 88 tests passed
  • integration suite above → 5 passed

Integration tests genuinely ran, against a throwaway Postgres 16 on :5434 provisioned via the repo's own autoMigrate + the integration globalSetup/setup files (tmpfs + fsync=off, per the known local-perf trap), with the standard breeze_app unprivileged role and forced RLS.

🤖 Generated with Claude Code

…t on a dead transaction

Creating a dynamic device group previewed N matching devices but left
`device_group_memberships` at 0 rows forever, with nothing in the API logs.

Mechanism: `routes/groups.ts:511` fired the evaluation without awaiting it
(`evaluateGroupMembership(id).catch(...)`). Only the evaluation's FIRST query
was dispatched while the request's `withDbAccessContext` transaction was still
open. The handler then returned, drizzle/postgres.js committed that transaction
and released its pooled connection, and the detached continuation's next query
was queued on a transaction handle that no longer owned a connection. That query
never executed, so the promise never settled, so the `.catch()` never ran.

Fix:
- await the evaluation in both the create and the filter-only update branch, so
  the writes stay inside the caller's own live org-scoped RLS context;
- batch the per-device membership-log INSERTs into one multi-row INSERT so
  awaiting costs a handful of statements regardless of group size;
- log loudly when the group row is invisible to the current DB access context,
  and when the materialized row count falls short of what the filter matched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 6, 2026

Copy link
Copy Markdown

Deploying breeze with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8ac92ba
Status: ✅  Deploy successful!
Preview URL: https://47a8a48a.breeze-9te.pages.dev
Branch Preview URL: https://fix-dynamic-group-membership.breeze-9te.pages.dev

View logs

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.

1 participant