fix(api): materialize dynamic group membership instead of stranding it on a dead transaction - #3181
Open
ToddHebebrand wants to merge 2 commits into
Open
fix(api): materialize dynamic group membership instead of stranding it on a dead transaction#3181ToddHebebrand wants to merge 2 commits into
ToddHebebrand wants to merge 2 commits into
Conversation
…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>
Deploying breeze with
|
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
Creating a dynamic device group with
Architecture equals x64showed a server-side preview of 3 matching devices, butstayed at 0 indefinitely. Nothing appeared in the API logs — not even the
console.errorthe create route already had.Mechanism (verified against a real Postgres, not inferred)
apps/api/src/routes/groups.ts:511fired the evaluation and deliberately did not await it:authMiddlewareruns the whole handler insidewithDbAccessContext, which isbaseDb.transaction(...)(apps/api/src/db/index.ts:275) because the RLS GUCs are set withSET LOCAL. The sequence is:evaluateGroupMembershipruns synchronously up to its firstawait, so theSELECT ... FROM device_groups(services/groupMembership.ts:172) is dispatched on the still-open request transaction and succeeds.dbto the committed transaction handle, so the next query — on the create path that isevaluateFilter→withFilterStatementTimeout→db.transaction(...), i.e. aSAVEPOINT(services/filterEngine.ts:556) — is queued on a connection that transaction no longer owns..catch()never runs. Zero rows, zero errors, zero logs.Reproduced deterministically: with the evaluation started inside a live
withDbAccessContextand awaited afterwards, step 1 returned the group row, then the whole promise timed out unsettled anddevice_group_membershipsstayed at 0. It is not an RLS policy rejection (that raisesnew row violates row-level security policy, loudly) and not anorg_idmismatch — the org stamping was already correct.The preview endpoint worked precisely because it
awaitsevaluateFilterWithPreviewinside 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. ThesiteChangedbranch already awaited, which is why site moves worked.The fix
apps/api/src/routes/groups.tsPOST /groups:await evaluateGroupMembership(...), then readgetDeviceCountForGroup(...)so the 201 carries the real count instead of a hardcoded0. 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.tsgroupMembershipLogINSERTs (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.!groupresult 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".matchedis 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".MembershipUpdateSummarygains optionalmatched/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:
withSystemDbAccessContextfor detached work) would have removed that DB-level backstop in favour of app-layer scoping only; awaiting keeps the strict version.evaluateFilterfilters oneq(devices.orgId, group.orgId), and every membership row is stamped with the group'sorgId, never a device- or caller-derived one.org_id, the foreign tenant cannot see the group at all, and a membership INSERT claiming the owner'sorg_idis rejected withnew row violates row-level security policy.Pre-existing gap, now pinned by a test (not introduced here)
device_group_membershipspolicies key onorg_idalone (breeze_has_org_access(org_id)), and there is no composite FK tying(group_id, org_id)back todevice_groups. A foreign tenant that somehow learned a group's UUID can insert a row naming that group as long as it stamps its ownorg_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 aUNIQUE (id, org_id)ondevice_groupsplus 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, realauthMiddleware, realbreeze_appforced RLS — the mocked route suites resolveevaluateGroupMembershipfrom avi.fn(), so a detached call looks identical to an awaited one there):data.deviceCountagrees;org_id;org_idis rejected by RLS (with the quarantine gap above asserted explicitly);PATCHwith a new filter re-materializes before responding;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.tsgains 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:
origin/main)dynamicGroupMembershipMaterialization.integration.test.tsexpected [] to deeply equal [ …(2) ], i.e. exactly the 0-rows symptomgroupMembership.materialization.test.ts+groups_get_create.test.tsFull runs on the final tree:
tsc --noEmit -p apps/api/tsconfig.json→ exit 0groupMembership.siteScope,groupMembership.materialization,groups,groups_get_create,groups_update_delete,groups_devices,groups_list,groups_preview_pin,groups_log_multitenant,src/events→ 9 files / 88 tests passedIntegration tests genuinely ran, against a throwaway Postgres 16 on
:5434provisioned via the repo's ownautoMigrate+ the integrationglobalSetup/setupfiles (tmpfs +fsync=off, per the known local-perf trap), with the standardbreeze_appunprivileged role and forced RLS.🤖 Generated with Claude Code