UN-3494 [FEAT] Email users and groups through PGMQ - #2224
kirtimanmishrazipstack wants to merge 30 commits into
Conversation
…ship changes Sharing a resource with a group gave its members access silently, and adding or removing someone from a group told nobody. Both now send email. - share_notifications.py holds the feature flag, the two task names and the two enqueue hooks. Dispatch uses the same resolve_transport branch the execution path uses: the PG queue where pg_queue_enabled is on for the org, Celery otherwise. - One hook in ResourceShareManagementMixin.share covers all 7 resource types plus cloud agentic, including service-account shares — every group share funnels through it and shared_groups has no PATCH path. No on_commit needed: _commit's transaction has closed by the time the view resumes, so the diff reads committed state. - Group membership hooks on the add and remove actions. The add serializer already subtracts existing members, so nobody is mailed twice. - Internal endpoints under /internal/v1/group-notification/ do the work the worker cannot: group expansion, OrganizationMember re-validation (this is where the offboarding race closes), resource lookup via ShareableResource, and the kind -> ResourceType mapping, which is not 1:1 — pipelines split on pipeline_type and adapters four ways on adapter_type. - Two worker tasks that only POST to that endpoint, since workers/ has no Django. They raise on failure, unlike _mark_buffer_outcome which has a reaper behind it, and retry transient 5xx in-task because a raise is terminal on the Celery transport. - The whole feature is gated on Flipt group_sharing_notifications_enabled and fails closed: a blind Flipt, a missing org, or any dispatch error means no notification, never a broken share. - worker-pg-notification compose service so the PG arm is not a black hole. Membership changes with no actor (the org-removal cascade, Django admin, group deletion) do not notify — SharingNotificationService requires an actor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe change adds asynchronous group resource-sharing and membership notifications through feature-gated dispatch, worker tasks, internal APIs, and notification services. Legacy partial-update notification paths are removed. Frontend co-owner management now stages and applies combined additions and removals. ChangesGroup notification pipeline
Staged co-owner management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ShareChange
participant share_notifications
participant NotificationWorker
participant InternalNotificationAPI
participant group_notification_service
participant NotificationPlugin
ShareChange->>share_notifications: Dispatch share or membership event
share_notifications->>NotificationWorker: Enqueue organization-scoped task
NotificationWorker->>InternalNotificationAPI: POST notification payload
InternalNotificationAPI->>group_notification_service: Validate and process payload
group_notification_service->>NotificationPlugin: Send filtered notification
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
UN-2977 moved sharing from PATCH to POST /{id}/share/, but the mixin's
share action only diffed the groups axis. The per-viewset
_notify_shared_users hooks stayed on partial_update, which nothing calls
anymore, so sharing a resource with a user sent no email.
Snapshot every declared axis and invoke the hook after the commit; declare
it on the mixin as a no-op for hosts without a direct-share email.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kirtimanmishrazipstack
changed the title
…revoked Sharing already emailed on grant; revoking told nobody. Both axes now notify, and the seven duplicated copies of the user hook collapse into the share mixin. - ResourceShareManagementMixin gains a concrete _notify_shared_users covering grant and revoke, driven by the OwnerManagementMixin seam every host already declares. The seven per-viewset overrides and their dead partial_update wrappers go with it — a host override would otherwise shadow the mixin and silently swallow the revoke mail. - share() diffs both axes through _read_axis directly; AxisDiff, snapshot_share_axes, diff_share_axes and the share_axes ClassVar had no callers left. - Group revoke rides the existing resource-shared route with a share_action discriminator, mirroring membership-changed — no new endpoint or worker task. Defaulted at every hop so in-flight messages still run. - Suppressed when the user still reaches the resource via a group or shared_to_org: losing one axis is not losing access. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kirtimanmishrazipstack
changed the title
…tton Adding a co-owner was staged until Apply, but revoking one fired the DELETE straight from the Popconfirm — so Cancel could not undo it, Apply stayed disabled for a removal-only edit, and the revoke email went out on click. Stage the roster the way SharePermission does: one selected-owners list seeded from the server, edited locally by both add and revoke, committed only by Apply. Collapse the hook's two mutation callbacks into one onApplyCoOwners that runs adds before removes (so a one-shot owner swap clears the backend's last-owner guard), refreshes once, and emits one summary alert. Apply now closes on a clean run and stays open on failure, matching useShareModal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kirtimanmishrazipstack
left a comment
kirtimanmishrazipstack
left a comment
There was a problem hiding this comment.
Self-review — UN-3494 (OSS)
Ran a multi-pass review over this branch (correctness, error handling, type design, comments) and verified each finding against the code rather than taking the analysis at face value. Six candidate findings turned out to be false and are not listed. Low-severity items are withheld; below is everything blocking / high / medium.
Cloud half: Zipstack/unstract-cloud#1698 — findings that span both repos are stated there from the cloud side.
Blocking
B1 — Every send result is discarded, so a failed send reports success and the queue acks it.
backend/tenant_account_v2/group_notification_service.py:102 and :153 call service.send_group_resource_shared_notification(...) / send_group_membership_notification(...) as bare statements. Both return bool. backend/tenant_account_v2/internal_views.py:83 and :95 then return 200 {"status": "success"} unconditionally, _post_group_notification sees 200 and returns, and the PG consumer deletes the message.
Failure path: SendGrid 429/503 → the plugin returns False → email silently lost, message acked, nothing above DEBUG anywhere.
This inverts the contract this file states about itself at internal_views.py:8-10 ("any unhandled problem must surface as non-2xx so the queue redelivers"), and it makes the whole retry apparatus in workers/notification/tasks.py — the 3-attempt loop, the httpx transport retries, the 120s VT, and the "deliberately raises on failure … a swallowed error would be a silently unsent email" docstring — guard a path that can no longer fail.
The plugin-missing case has the same shape: _service() returns None behind a logger.debug (:162-166). A backend built without sendgrid emails nobody while every layer reports green — and the recipient_count= INFO line never executes, so the one metric you would grep for is absent rather than zero, which is indistinguishable from "nobody was shared with".
Fix: collect the booleans in send_resource_shared / send_membership_changed and return non-2xx when any send failed retryably; log non-retryable causes (template unset, notifications disabled) at WARNING with a distinct {"status": "skipped", "reason": ...} so misconfiguration is separable from delivery.
B2 — Group-revoke emails ignore remaining access.
group_notification_service.py:89-111 mails every current member of each revoked group with no effective-access check. The direct-user path deliberately does the opposite — _users_left_without_access in backend/permissions/resource_share_views.py:81-93, with the comment "telling them their access was removed would be wrong."
Failure path: workflow W is shared with Group A and Group B; Alice is in both; an owner removes Group B. Alice is told her access was removed, and because share_action == revoked the cloud side rewrites both the CTA and resource URL to the dashboard — so the email walks her away from a resource she still fully reaches via Group A. Same for a shared_to_org=True resource, where nobody lost anything, and for members who also hold a direct VIEWER row.
The revoke recipient list should go through the same compute_effective_members filter the direct path uses.
High
H1 — The notification path in share() can 500 a share that already committed.
permissions/resource_share_views.py:175-195: only _send_share_notification and _send_revoke_notification are wrapped. _notification_context (:188, which invokes the host viewset's get_notification_resource_type override) and _users_left_without_access (:193, a DB query) run bare — after ShareAuthorizationService.authorize_and_commit has already committed at :155-161. A DB hiccup or a raising seam returns 500 for a share that succeeded, and the client retries.
The group path escapes this only by luck: _organization_slug and kind_for_instance are pure getattr/_meta reads and _feature_enabled is wrapped. Wrap the whole _notify_shared_users body plus the notify_resource_group_share_changed call at the share() call site.
H2 — A synchronous SendGrid HTTPS call is now on the live POST /share/ path.
The group path was deliberately made async (worker + internal API); the direct-user path in _notify_shared_users calls the plugin inline. This is newly-introduced request latency, not pre-existing — the previous home (partial_update) was dead code, so these emails were not firing at all before this branch.
H3 — Unbounded org-member scan pulled into that same request.
_users_left_without_access → compute_effective_members → _add_org_members (backend/tenant_account_v2/sharing_helpers.py:316-340) runs OrganizationMember.objects.filter(organization=...) with no pagination and iterates the whole result in Python, whenever shared_to_org is true. Un-sharing one user on an org-shared workflow in a 5,000-member org hydrates 5,000 OrganizationMember + User rows to answer "does this one user still have access?" Against ARCHITECTURE_PRINCIPLES §6 on unbounded querysets and heavy work in the request cycle.
Cheap and correct: guard-clause if getattr(instance, "shared_to_org", False): return [] — if the resource is org-shared, nobody who lost a direct row actually lost access.
H4 — The new direct-share revoke email ships with no feature flag.
_notify_shared_users has no Flipt check at all; only the group path is gated by GROUP_NOTIFICATION_FLAG_KEY. So the new revoke email goes live for every org the moment this deploys, with no kill switch. The module docstring at backend/tenant_account_v2/share_notifications.py:13-15 claims "The whole feature sits behind its own Flipt flag and fails closed everywhere", and the PR description repeats it — neither is true for this path. Either gate it or correct both statements. (Template-reuse half of this is on the cloud PR.)
H5 — Lookups are group-shareable but get no group email, and nothing logs it.
LookupDefinition is absent from SHAREABLE_RESOURCES (backend/tenant_account_v2/shareable_resources.py:28-52), so kind_for_instance returns None and share_notifications.py:100 returns with no log line at all — kind is None is collapsed into the same silent early-return as feature-flag-off. Direct-user lookup emails do fire (the cloud PR wires get_notification_resource_type for exactly that), so the result reads as a flaky feature rather than a gap.
Split that guard: flag-off is expected silence, but an unregistered kind and a resource with no organization are both bugs and should log at WARNING. Then either register LookupDefinition or reject shared_groups for hosts absent from the registry.
H6 — _get_user is the one org-unscoped query on a tenant-scoped path.
group_notification_service.py:170-171 resolves the actor with User.objects.filter(pk=user_id).first(). Every sibling lookup on this path re-validates against the org (_groups_in_org, _live_member_users, and _load_resource, which filters organization= explicitly and explains why). The resolved user's name and email render into the outgoing mail. Not exploitable today since the payload is worker-generated, but it is an unscoped query on a multi-tenant path. One filter through OrganizationMember fixes it.
H7 — Rolling deploy: new backend to an old worker drops the message.
notify_resource_shared_with_group (workers/notification/tasks.py:528-535) has a closed signature. A message carrying share_action delivered to a pod on the previous build raises TypeError — terminal on Celery, burns the attempt cap on PG. The producer has already returned 200 to the user via _dispatch_quietly, so nothing surfaces. **_: Any on both new task signatures closes it.
Related: the "defaulted so messages enqueued before this field existed still validate" comments (internal_views.py:40, tasks.py:538) describe a state that never existed — both the task and share_action were added on this branch, so there are no in-flight messages. The defaults are fine to keep; the stated rationale is not, and it obscures the fact that the real hazard runs the other way.
H8 — Rollout ordering: PG transport with no consumer deployed.
_dispatch routes to the PG queue whenever resolve_transport says so, and the notification consumer is off by default. Any org with pg_queue_enabled ramped but the consumer not running gets messages durably stored and never executed — logged as "group-notification: %s enqueued on PG queue %r (msg_id=%s)" at INFO, which reads as delivery. Needs to be an explicit ordering constraint in Env Config, not an inference. (Chart side on the cloud PR.)
Medium
- Dead exception handler. The
except Exceptionin_feature_enabled(share_notifications.py:164-170) can never fire — bothcheck_feature_flag_statusandFliptClient.evaluate_booleancatch and returnFalsefirst. Remove it or stop relying on it. - The default Flipt path logs nothing.
FLIPT_SERVICE_AVAILABLE != "true"at:154returnsFalsewith zero logging, and that is the default. Combined with H5's collapsed guard, "I ramped the flag and no email arrived" has no log line distinguishing which of four causes applied. recipient_countis post-filter only.group_notification_service.py:93-99and:144-150log the surviving count; the requested count is never logged, and_groups_in_org/_live_member_usersboth drop silently. "Half my team didn't get it" is unfalsifiable from logs. Logrequested / resolved / dropped.- 2N+1 on the group fan-out.
:89-92runs onevalues_listplus oneOrganizationMemberquery per group. Collapse to a singleGroupMembership.objects.filter(group__in=…).select_related("user", "group")grouped in Python. - Over the 30-line ceiling (CLAUDE.md):
send_resource_shared40,_post_group_notification35,send_membership_changed31. _notification_contextis duplicated. The module-level function atresource_share_views.py:62is a line-for-line copy ofOwnerManagementMixin._notification_context(permissions/membership_views.py:88). Two copies that will drift, and their docstrings already contradict each other on whether hosts overrideget_notification_resource_type— all seven do.- Docstrings that misstate contracts:
internal_views.py:8-10— "non-2xx so the queue redelivers" holds only on the PG transport; on Celery a raise is terminal, astasks.py:471-472itself says.tasks.py:481-487— self-contradictory: "a 4xx is not retried" versus "the raise leaves the message on the queue for redelivery". Thebreakat:513still falls through to theraiseat:524. Also the guard is< 500, not 4xx.share_notifications.py:9-10— transport is resolved per resource, not per org:_dispatchpasses the resource/group pk asexecution_id, which is whatresolve_transportbuckets the rollout on.resource_share_views.py:3-6— the mixin is no longer axis-agnostic (theshare_axesClassVar is gone and_read_axishardcodes both names), and it does not read_SUPPORTED_SHARE_AXES— only_extract_desired_share_statedoes.
- Frontend, partial-failure UX contradicts itself.
CoOwnerManagement.jsxkeeps the modal open on partial failure "so the user can see what was rejected and retry", butonApplyCoOwnersalways callsrefreshCoOwnerDatafirst, and theuseEffectre-seedsselectedOwnersfrom the refreshed roster — so the staged edits are already wiped and there is nothing to retry from.
Verified clean
The dead-code removal holds up: the shared_users M2Ms were dropped by the UN-2202 migrations and every serializer now exposes shared_users as a read-only SerializerMethodField, so those partial_update hooks could never have fired. No references to share_axes, AxisDiff, snapshot_share_axes, or diff_share_axes remain in either repo.
Auth on the new internal endpoints is genuinely enforced — InternalAPIAuthMiddleware gates every /internal/ path before DRF runs, and the route is reachable in all three deployments. Org scoping on _load_resource / _groups_in_org / _live_member_users is correct. Every OSS→cloud ResourceType mapping checks out. The onApplyCoOwners rename is fully propagated across all 11 consumers in both repos.
_users_left_without_access is safe despite compute_effective_members excluding owners: ResourceMembership has UniqueConstraint(user, content_type, object_id), so a user is OWNER or VIEWER and never both. Reading the diffs after _commit is also safe — ATOMIC_REQUESTS defaults to False and is pinned False in the chart.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
frontend/src/components/widgets/co-owner-management/CoOwnerManagement.jsx (2)
140-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSet an explicit
rowKeyon the List.
Listfalls back to the array index whenrowKeyis absent.selectedOwnersnow changes by insertion and removal, so index keys make React reuse a row component for a different user. Thekeyon the innerPopconfirmdoes not controlList.Itemreconciliation, so an open confirm popup can attach to the wrong row after a staged removal.♻️ Proposed change
<List dataSource={selectedOwners} + rowKey={(item) => item?.id} renderItem={(item) => (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/widgets/co-owner-management/CoOwnerManagement.jsx` around lines 140 - 142, Update the List rendering in CoOwnerManagement to provide an explicit rowKey based on each selected owner’s stable unique identifier, rather than allowing index-based keys. Keep the existing renderItem and Popconfirm behavior unchanged.
114-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClose and Cancel stay active during Apply.
confirmLoadingdisables the OK button only. The close icon and the Cancel button remain clickable whileapplyingis true. The user can dismiss the modal while requests are in flight. The requests still complete and the alert still appears, so the outcome is not lost, but the state is confusing.Disable both controls while
applyingis true.♻️ Proposed change
confirmLoading={applying} okButtonProps={{ disabled: !hasChanges }} + cancelButtonProps={{ disabled: applying }} maskClosable={false} centered - closable={true} + closable={!applying}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/widgets/co-owner-management/CoOwnerManagement.jsx` around lines 114 - 119, Update the CoOwnerManagement modal configuration so both the close control and Cancel action are disabled while applying is true, while preserving the existing confirmLoading behavior. Use the existing applying state in the modal’s closable and cancel-button properties.frontend/src/hooks/useCoOwnerManagement.jsx (1)
19-22: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard the zero-change call in the hook.
If
addUsersandremoveUsersare both empty,totalis 0 andfailed.length === totalis true.buildApplyAlertthen callshandleException(null, "Unable to update co-owners")and shows an error alert for a no-op.CoOwnerManagement.handleApplycurrently blocks this case, but the hook is a shared export and should not depend on that caller guard.♻️ Proposed guard
const total = addUsers.length + removeUsers.length; - if (failed.length === total) { + if (total === 0) { + return null; + } + if (failed.length === total) { return handleException(lastError, "Unable to update co-owners"); }
setAlertDetailswould then need to skip anullalert inonApplyCoOwners.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useCoOwnerManagement.jsx` around lines 19 - 22, Guard the zero-change case in the hook’s apply-result handling before comparing failed.length with total: when both addUsers and removeUsers are empty, skip error handling and avoid calling handleException with null. Update the related onApplyCoOwners alert flow as needed so setAlertDetails does not process a null alert, while preserving failure handling for actual changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tenant_account_v2/share_notifications.py`:
- Around line 150-170: Update the internal sender flow after organization
resolution to call _feature_enabled again before delivering the notification.
When the flag is disabled or Flipt is unavailable, skip delivery and return the
existing successful skipped response, preserving normal sending when the flag
remains enabled.
In `@docker/docker-compose.yaml`:
- Around line 854-857: Update the notification worker visibility-timeout
configuration around WORKER_PG_QUEUE_CONSUMER_VT_SECONDS to account for up to
three 30-second POST attempts with HTTPTransport retries=2, ensuring the
configured timeout exceeds the worst-case transport retry duration; keep
WORKER_PG_QUEUE_CONSUMER_HEALTH_STALE_SECONDS above the resulting visibility
timeout.
In `@frontend/src/hooks/useCoOwnerManagement.jsx`:
- Around line 144-155: Update refreshCoOwnerData and its caller in the apply
flow so it returns whether the resource-not-found (404) branch was reached;
after awaiting refreshCoOwnerData, only call setAlertDetails with
buildApplyAlert when that result indicates no 404 occurred, preserving the
existing resource-gone alert and modal/list behavior.
In `@workers/notification/tasks.py`:
- Around line 503-524: Add an immutable job ID to each notification task and
propagate it through the internal notification API and payload. In the backend
handler, deduplicate requests using that job ID before invoking the notification
plugin, recording successful delivery so retries and PG queue redelivery do not
send the same notification again. Update the retry flow around client.post and
the corresponding task/API symbols while preserving existing retry behavior for
failed deliveries.
---
Nitpick comments:
In `@frontend/src/components/widgets/co-owner-management/CoOwnerManagement.jsx`:
- Around line 140-142: Update the List rendering in CoOwnerManagement to provide
an explicit rowKey based on each selected owner’s stable unique identifier,
rather than allowing index-based keys. Keep the existing renderItem and
Popconfirm behavior unchanged.
- Around line 114-119: Update the CoOwnerManagement modal configuration so both
the close control and Cancel action are disabled while applying is true, while
preserving the existing confirmLoading behavior. Use the existing applying state
in the modal’s closable and cancel-button properties.
In `@frontend/src/hooks/useCoOwnerManagement.jsx`:
- Around line 19-22: Guard the zero-change case in the hook’s apply-result
handling before comparing failed.length with total: when both addUsers and
removeUsers are empty, skip error handling and avoid calling handleException
with null. Update the related onApplyCoOwners alert flow as needed so
setAlertDetails does not process a null alert, while preserving failure handling
for actual changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b71cb647-64c1-4252-a93d-5996186b95b2
📒 Files selected for processing (22)
backend/adapter_processor_v2/views.pybackend/api_v2/api_deployment_views.pybackend/backend/internal_base_urls.pybackend/connector_v2/views.pybackend/permissions/resource_share_views.pybackend/pipeline_v2/views.pybackend/prompt_studio/prompt_studio_core_v2/views.pybackend/tenant_account_v2/group_notification_service.pybackend/tenant_account_v2/group_views.pybackend/tenant_account_v2/internal_urls.pybackend/tenant_account_v2/internal_views.pybackend/tenant_account_v2/share_notifications.pybackend/tenant_account_v2/shareable_resources.pybackend/workflow_manager/workflow_v2/views.pydocker/docker-compose.yamlfrontend/src/components/deployments/api-deployment/ApiDeployment.jsxfrontend/src/components/pipelines-or-deployments/pipelines/Pipelines.jsxfrontend/src/components/widgets/co-owner-management/CoOwnerManagement.cssfrontend/src/components/widgets/co-owner-management/CoOwnerManagement.jsxfrontend/src/components/widgets/co-owner-management/CoOwnerModal.jsxfrontend/src/hooks/useCoOwnerManagement.jsxworkers/notification/tasks.py
💤 Files with no reviewable changes (7)
- frontend/src/components/widgets/co-owner-management/CoOwnerManagement.css
- backend/prompt_studio/prompt_studio_core_v2/views.py
- backend/pipeline_v2/views.py
- backend/api_v2/api_deployment_views.py
- backend/workflow_manager/workflow_v2/views.py
- backend/adapter_processor_v2/views.py
- backend/connector_v2/views.py
Group revoke no longer mails members who kept access another way. The revoke recipient list now runs through the same effective-access filter the direct path uses, with owners folded in — compute_effective_members excludes them by design, and the sharer is usually a member of the group they shared with, so revoking told the owner their own access was removed and pointed them at the dashboard. - _get_user is org-scoped through OrganizationMember, the one unscoped query left on this tenant path. Service accounts are kept so a platform-account share still notifies. - _notify_shared_users is wrapped: the share has already committed by the time it runs, so a raising seam or a DB hiccup must not 500 a share that worked. - _users_left_without_access short-circuits on shared_to_org — nobody lost access, and answering it otherwise hydrates every member of the org. - _notification_context loses its duplicate copy and uses the OwnerManagementMixin definition every host already inherits. - Logs the Flipt decision, and how many recipients were dropped versus requested, so a missing email is diagnosable. - Docstrings corrected: the mixin is not axis-agnostic, transport resolves per resource id not per org, the flag is evaluated once at enqueue, and delivery is at-least-once. - Sonar S7632: the noqa directive carried trailing prose. - Co-owner apply no longer overwrites the resource-gone alert with its summary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…vice The PG-queue notification consumer is local dev config and does not belong in the PR. The k8s chart already carries workerPgNotification from UN-3445 (#1688), which is the real deployment surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Self-review dispositions (OSS)Re-verified every finding from my review above against the code, with an independent adversarial pass on each verdict. Five of my own findings did not survive and are withdrawn below rather than quietly dropped. Code changes in c1d3095; the compose service removal in 9e9f57c. Fixed
Withdrawn — my findings, wrong
Not changing
|
… revoked A revoke resolves recipients from the group's live membership at delivery time, so anyone who joined between the click and the send was told their access was removed for a group through which they never held it. Normally a few seconds; on the PG transport with no consumer deployed the backlog can sit far longer. The revoke now carries the timestamp of the change and delivery drops memberships created after it. One string on the payload rather than the frozen member list, which would grow with the group. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ready gone A grant enqueued before a revoke could still be delivered after it, mailing the resource name and id to members who can no longer reach the resource. Delivery now revalidates the live ResourceGroupShare on the grant direction and drops groups that no longer hold it. The revoke direction needs no equivalent check — its share row is gone by delivery, and _retained_user_ids already covers members who kept access another way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@greptileai review |
revoked_at was captured after _feature_enabled(), so the window between the share-removal commit and the timestamp spanned a Flipt network call. A user joining the group inside it passed the cutoff and was mailed a revocation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
965f4d2 to
70b42c8
Compare
|
@greptileai please review |
Enqueue side (unit tier, no DB): payload shape, the revoked_at stamp landing before the Flipt round-trip, and the skip/swallow paths. Delivery side (integration tier): recipient selection - the live re-read on a grant, the revoked_at cutoff, org scoping and retained access - plus the direct-user share/revoke wiring on the share endpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
kirtimanmishrazipstack
changed the title
…-only Main moved past this branch with UN-4046 (PG queue out of the pg_queue_enabled flag) and #2212 (Ant Design out of the OSS frontend). Three things needed resolving: - CoOwnerManagement.jsx: the only textual conflict. Kept this branch's staged co-owner roster (removals wait for Apply) on top of main's shadcn shim imports and lucide icons. - share_notifications._dispatch imported resolve_transport from workflow_manager.workflow_v2.transport, which UN-4046 deleted — it would have raised ImportError on the first share. It now enqueues on the PG queue directly, mirroring notification_dispatch. entity_id existed only to give resolve_transport a sticky id, so it is gone from _dispatch_quietly, both callers, and the two assertions that read it. - workers/notification: the in-task retry loop was justified by "on Celery a raise is terminal". Celery is gone; the loop stays because it absorbs a brief backend blip in-process rather than costing a lease-expiry redelivery plus one of the consumer's bounded attempts. Verified: manage.py check clean (warnings all pre-existing shapes), 11 unit + 36 integration tests pass across test_share_notification_dispatch, permissions/tests/test_share_notifications and tenant_account_v2/tests, pre-commit clean on the touched files, frontend build succeeds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LfrKHgSfDQxaXwJWrMxUWG
…p-sharing-notification
The flag existed only because the PG queue it dispatches onto was itself behind pg_queue_enabled. UN-4046 removed that flag and made PG the only transport, and this feature merges in one shot rather than landing in main a piece at a time, so there is nothing left for a flag to buy. Removes _feature_enabled, GROUP_NOTIFICATION_FLAG_KEY and the FLIPT_SERVICE_AVAILABLE pre-check. The two dispatch guards keep their real checks -- a resolvable org, and a resource kind the email plugin has a type for. The revoke cutoff stays: the queue can still lag, so a member who joins after the access was taken away must not be mailed about it. Its comment no longer cites a Flipt round-trip, and the regression test that guarded that specific window goes with it -- nothing slow sits between the stamp and its use now. Verified: 45 tests pass across the three notification modules. Mutating both surviving guards to `if False:` fails exactly test_unknown_resource_kind_ skips_dispatch and test_missing_organization_skips_dispatch, so what remains is covered rather than merely present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LfrKHgSfDQxaXwJWrMxUWG
F2 A plugin that fails to import logs only at DEBUG, so a cloud build missing
sendgrid mails nobody while every layer reports success. Warn when email is
switched on and the plugin is absent -- the one state that can only be a
broken build.
F4 _retained_user_ids hydrated every member of the org on a revoke of an
org-shared resource, to conclude nobody lost access. Short-circuit it, the
same guard _users_left_without_access already carries.
F5 The enqueue guards returned silently on a missing org and on an unregistered
resource kind, both defects rather than routine skips; and _groups_to_mail
dropped groups with no count.
F6 State the deploy ordering at the enqueue site: a consumer on the previous
image cannot resolve these task names and DELETES the rows, with no
dead-letter, since nothing here passes reply_key or on_error.
F7 The at-least-once note claimed one duplicate email. A retry re-posts the
whole payload and the backend mails group by group with no checkpoint, so
the bound is 3 attempts times the consumer's cap.
F8 VT is 300s and its justification described one POST per task.
HTTPTransport(retries=2) retries the CONNECT, so worst case is ~274s --
measured, not inferred.
F11 A failed add no longer falls through to the removals: the roster never
grew, so removing could strip the very owner the swap was replacing.
F12 refreshCoOwnerData returns a verdict instead of leaving four post-await
sites to re-check the ref, only one of which did. A late apply could close
whichever co-owner modal was open by then.
F13 Restore the `applying` half of the body guard this branch dropped, so edits
made mid-apply are not silently discarded by the re-seed; and correct the
comment promising a retry surface the re-seed removes.
F14 Seed the staged roster during render rather than in an effect, so the first
frame of a new resource cannot show the previous resource's list.
F15 Say why shared_to_org is not a notification axis.
F16 share_action and revoked_at are always sent, so drop the defaults that
would turn a renamed field into a revoke mailed as a share.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
…the apply verdicts Verification of the previous commit found these in its own new lines. - The ~274s figure omitted httpcore's 0.5s/1.0s connect backoff; the real ceiling is ~278s against a 300s VT. The closing sentence claimed raising either constant overruns the budget, which is false for a modest bump of the retry delay -- exactly the wrong thing to leave a maintainer reasoning from. - The serializer comment said a default could not turn a renamed field into a revoke mailed as a share, but the worker task defaulted share_action to "shared" one hop upstream, where the serializer cannot see it. The task now requires it. revoked_at keeps its default: the producer omits it on the share direction deliberately. - The group-drop log offered "or group gone since enqueue" as a cause, but _groups_in_org removes deleted and out-of-org groups before the count, so that alternative can never be the one reported. - refreshCoOwnerData returned "ok" after a refresh that failed, so the caller stacked a success summary on a standing error alert and closed the modal on a roster it could not verify. It now returns "error". - The stale verdict also skipped onListRefresh, but the mutations had already landed and the list is page-scoped, not modal-scoped. - "Failed for:" named removals that were deliberately never attempted after an addition failed. Now "Not applied for:", which is true of both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
…view's comment and observability gaps
N3/N4 The post could outlive the queue's visibility timeout. httpx has no
whole-request timeout: a scalar `timeout=30.0` applies to connect, write
and read SEPARATELY, and `HTTPTransport(retries=2)` retries only the
CONNECT phase, inside one post, stacking its own timeouts underneath.
Worst case was connect-fail 30 + 30, backoff 0.5, connect OK, write 30,
read 30 -- about 120s per post, ~365s for the task, over both VT (300s,
a sibling re-claims mid-fan-out) and health-stale (360s, the pod is
restarted mid-task). Earlier derivations missed this because they only
measured the all-connect-fail case.
Now bounded per phase, with the task's own loop as the only retry:
5+10+30+5 = 50s a post, 154s for the task. A read or write timeout also
ends the in-process attempts, because the request reached the backend,
the backend does not stop when we disconnect, and the send path has no
checkpoint -- re-posting re-mails every group that already succeeded.
N1 The module's "failure contract" paragraph said a deleted resource was the
ONE deliberate 200-on-failure. The cloud revert made a failed SendGrid
send a second one. Deleted rather than rewritten: both real sites state
the carve-out correctly where it happens.
N2 An unregistered resource kind is not necessarily a client bug -- the share
endpoint accepts `shared_groups` for any host viewset.
N5 Neither failure on this path carried a `metric=` prefix, which is what the
metrics pipeline scrapes, while every sibling webhook path has one. Added
to the enqueue swallow and the worker's terminal raise.
N6 `send_resource_shared` kept a `share_action` default -- exactly the trap
its own serializer comment documents holding the line against.
R1/R2 One derivation of the retry budget, in the code. The two config comments
point at it instead of restating a number that was wrong in three
different ways across three files.
R3 The "error" verdict left the modal open on a stale roster with the staged
diff intact, so a second Apply re-posted mutations the backend had already
accepted and reported every one as a failure.
R4 The verdict legend contradicted its only caller on "stale".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
…prose that keeps being wrong Verification of the previous commit found both. - The `break` encodes a rule -- the request left, the backend does not stop when we disconnect, so re-posting re-mails every group that already succeeded -- and enumerated two of its four members. `ReadError` and `RemoteProtocolError` satisfy it too, and a backend pod rollout mid-fan-out raises exactly those. They were falling through to the generic arm and buying two more full re-posts inside a single delivery. - Three comments have now been rewritten once per round and been wrong every time. The retry budget was ~274s, then ~278s, then a correct bound attached to a re-claim mechanism the renewable lease replaced (the consumer says so itself: VT is the drain bound, not the claim window). The failure contract was "the one deliberate 200", then "cases a retry cannot help" -- still false, because a transient SendGrid rejection is retryable and is answered 200 anyway. Each rewrite is a fresh set of claims and a fresh surface for the same defect, so these keep only what is checkable and drop the mechanism stories. The budget comment also no longer implies it covers DNS, which `connect` does not. - The co-owner fall-through comment claimed to close the re-Apply hole. It does not: on a partial failure the function still returns false and the staged diff is still computed against an unrefreshed roster. Narrowed to what the change actually does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
A mutation pass over this branch found that large parts of the feature could be
deleted with the whole suite still green. These close the ones that matter, and
each was written by breaking the production line first and confirming the test
fails.
workers/tests/test_group_notification_post.py (18, unit tier)
The worker leg had no test of any kind, and it is entirely failure-path
logic. The branch that matters is which exceptions mean "the request
reached the backend": those must not be re-posted, because the backend does
not stop when we disconnect and the send path has no checkpoint, so a retry
re-mails every group that already succeeded. Connect-side failures must
still retry, because nothing happened. Also pins the attempt cap, the
sub-500 break, the credential guard, both task payloads, and that the
timeout is per-phase -- a scalar one silently triples the task's worst case
and pushed it past the consumer's visibility timeout.
Killed: dropping ReadError+RemoteProtocolError from the request-sent tuple
(2 failures), retrying instead of breaking (4), treating 4xx as retryable
(1), scalar timeout (2).
tenant_account_v2/tests.py (+8)
test_group_from_another_org_is_never_mailed was vacuous: its foreign group
had no members, so it passed on an empty recipient list rather than on the
org filter, and stayed green with that filter deleted. Its group now has a
member who also belongs to the sharing org, which is the only arrangement
that makes the filter load-bearing -- users belong to any number of orgs
here.
Added: a group member outside the org, an actor outside the org, a resource
from another org, the org-wide revoke case, an owner inside a revoked group,
the membership REMOVED direction, and recipient re-validation on membership.
Note on what these pin. OrganizationGroup has no org-scoped default manager
("org filtering is explicit on every query"), so _groups_in_org's filter is
genuinely load-bearing and is now pinned. OrganizationMember and Workflow do
have one, so the explicit filters in _live_member_users, _get_user and
_load_resource are defence-in-depth and their removal is behaviour-
preserving -- these tests pin the outcome, not those lines. The docstrings
say so rather than implying coverage they do not have. The one case
_load_resource's filter genuinely guards is AgenticProject, whose manager
deliberately spans orgs; that model is cloud-only and cannot be exercised
here.
Verified through the rig, not just pytest: tox -e groups -- unit-workers
(1443 passed, 1 skipped) and integration-backend, where the new backend tests
land because conftest auto-marks TestCase as integration.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
A revoke mailed "your access was removed" to two sets of people who had lost nothing. An org admin reaches every resource because for_user hands admins the whole queryset, and a frictionless adapter is admitted unconditionally. The retained set was built from share rows alone, so neither route appeared in it. Both routes now sit in sharing_helpers beside compute_effective_members, so the two places that decide who lost access -- the group fan-out and the direct share view -- share one answer rather than each carrying its own partial copy. The direct share view had the identical hole. The admin check goes through AuthenticationController instead of comparing to a role string, which differs between the OSS and auth0 plugins. Each line was verified by breaking it: dropping the admin add-back fails the new admin test, and dropping the frictionless route fails 2 of the 6 parametrized retention cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
Two /code-review passes over the pushed state found ten real issues here. Two more were real but not fixed -- flagged below with why. - LookupDefinition was missing from the group-shareable resource registry. Its ViewSet already exposes the group-share action, so a group share on a lookup resolved no notification type and silently logged a warning instead of mailing anyone. - share() diffed both sharing axes even when a request's payload touched only one. A concurrent request changing the untouched axis landed inside that window and got attributed to the wrong actor's name in the notification. Now each axis is only read, diffed and notified when this request's own payload actually names it. - The worker's "already reached the backend, don't retry" exception tuple had ReadError but not its write-side sibling WriteError -- a mid-write socket failure retried the full attempt cap instead of stopping after one, same as every other request-sent-but-outcome-unknown case already does. - A group-membership add validated "not already a member" once, then wrote with ignore_conflicts=True. A concurrent add for the same user landed silently as a no-op, and the notification still fired for the user this request never actually added. Re-checks membership immediately before the write to shrink that window. - The revoke-path retention logic (who still has access another way: a group, ownership, an org-wide share, admin) was hand-rolled twice, once per call site, with owners present in one copy and silently absent from the other. Consolidated into sharing_helpers.retained_user_ids, used by both. - The group fan-out queried OrganizationMember once per group being mailed. Batched into one query across the whole group list per notification event. - _post_group_notification and onApplyCoOwners were both well over the 30-line function cap; module docstrings across three files cited ticket numbers rather than staying purpose-only. Extracted the retry-attempt shape and the mutation phase into their own functions; dropped the ticket references. - Co-owner demotion never checked whether the demoted user still reaches the resource another way (a group, a direct share, org-wide, or being an org admin) before mailing "your access was removed" -- the same class of bug the group-revoke and direct-share paths already guard against, just never extended to this call site. This PR had only added a docstring here; the bug predates it. Fixed by reusing the same retained_user_ids check, with a mutation-verified test pinning it. Flagged, not fixed -- real, disproportionate to fix here: - _post_group_notification hand-rolls a retry loop that resembles workers/shared/clients/base_client.py's BaseAPIClient. Different library (requests/urllib3, not httpx), no per-phase timeout, no request-sent classification -- the two things this PR already tuned. Swapping would risk regressing both for a cosmetic duplication. - group_notification_service.py's resource-type mapping duplicates what each ViewSet's own get_notification_resource_type computes. A proper fix reaches into five files this PR never touched to refactor an already-shipped feature; out of scope for a remediation pass. One reported finding did not hold up: a claim that a failed roster refresh leaves the co-owner modal showing stale data on reopen. Reopening always refetches fresh (handleCoOwner), so the claimed path does not exist. Verified: tox unit-backend (1294), unit-workers (1445), integration-backend (583) all pass; the sendgrid-plugin collection error is the known, expected local-only gap. New test mutation-verified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
…p-sharing-notification
The only change left in it was a comment on worker-pg-notification. Reverted to main so this PR touches no compose file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LfrKHgSfDQxaXwJWrMxUWG
…fication Reopening a notification to edit it always showed a blank name/URL, BEARER auth, and an unchecked "notify on failures only" -- the actual saved values, regardless of what they were. formDetails started at DEFAULT_FORM_DETAILS on the component's first render; the real row only arrived one render later, via an effect. The antd-compatible Form shim seeds its fields from initialValues once, on its own first mount -- matching real antd -- which happens on that same first render, before the effect runs. The form always mounted on the blanks. A second effect tried to patch this with form.resetFields(), but the shim's resetFields() resets to an empty object rather than back to initialValues, so it could only re-blank the form, never repair it. This component remounts fresh every time Edit opens (NotificationModal renders DisplayNotifications in between), so editDetails is already the row being edited by the first render. Seeding formDetails from it via a lazy useState initializer fixes the timing directly and makes both effects unnecessary. Verified live: editing a notification now shows its real name, URL, authorization type, and notify-on-failures setting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
…ares Greptile P1 on membership_views.py:127-129. retained_user_ids returns None to mean access is unconditional (org-wide share, frictionless adapter) -- the guard only checked the concrete-set case, so a None result fell through and sent an incorrect access-removed email. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
id_field was "id", but LookupDefinition's actual primary key is lookup_id (a UUIDField). This failed Django's system check outright (tenant_account_v2.E001), which blocks manage.py check/migrate/test for the whole project -- found while verifying the fix above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
…p-sharing-notification # Conflicts: # backend/permissions/tests/test_owner_management.py # backend/tenant_account_v2/shareable_resources.py
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Unstract test resultsPer-group results
Critical paths
|
muhammad-ali-e
left a comment
muhammad-ali-e
left a comment
There was a problem hiding this comment.
Standardized pre-merge review — BLOCK
Summary — Critical: 1 · High: 4 · Medium: 15 · Low: 15 · Lenses run: 16/16
Reviewed together with Zipstack/unstract-cloud#1698 as one atomic change set (OSS decides who to notify and enqueues; cloud sends). Mode: INITIAL. Heads reviewed: OSS 04402d05, cloud d6884044. Findings are inline; the ones with no diff line to attach to are at the bottom.
Lens checklist (16/16)
| # | Lens | |
|---|---|---|
| 1 | Spec & intent | See findings — Critical; scope creep; description inaccuracies |
| 2 | Architectural fit & precedent | See findings — Critical, duplicated mapping, missing plugin gate |
| 3 | Correctness & edge cases | See findings — 3 High, OCR fallback |
| 4 | Security | Clean. New internal endpoints inherit the established InternalAPIAuthMiddleware posture (DEFAULT_PERMISSION_CLASSES: [] project-wide, matching usage_v2/dashboard_metrics); tenant isolation verified across the queue boundary (org slug echoed as X-Organization-ID, every downstream query re-filters — _load_resource, _groups_in_org, _live_member_users); retained-access logic correctly withholds resource name/id from users who cannot reach it. Noted, not flagged: the endpoints are an unrate-limited email fan-out for anyone holding INTERNAL_SERVICE_API_KEY — same trust boundary as existing internal APIs, new blast radius. |
| 5 | Data integrity & migrations | See findings. Zero migrations, zero model changes (verified by diffing *models.py and */migrations/* — empty in both repos). Enqueue is ORM-backed (PgQueueMessage) so transactional with any enclosing request transaction. No idempotency key on enqueue_task — this is the structural cause of the duplicate-email High. |
| 6 | Concurrency | See findings. Consumer is prefork with single-threaded children, batch_size forced to 1, lease renewed every LEASE/3 — so a blocked task occupies a whole slot, and redelivery lands ~2 min after a raise, not 300s. |
| 7 | API & contract compatibility | See findings — Critical, duplicated mapping, str(enum) on the wire |
| 8 | Reliability & resilience | See findings — 3 High, unbounded thread pool, wrong timeout formula |
| 9 | Performance & cost | See findings — futile OSS round trips, full-org admin scan, thread pool |
| 10 | Observability | See findings — 200-on-failure, missing metric= keys |
| 11 | Operational safety | See findings. Kill switch is partial: blanking the two new template IDs stops the sends but not the enqueue, the queue traffic or the internal POSTs — and does not cover the newly-live inline direct-share email, which falls back to the generic SENDGRID_TEMPLATE_ID. Roll-back carries the same unknown-task-drop trap as roll-forward. |
| 12 | LLM/agent | N/A — the adapter / prompt-studio / agentic files are touched only for sharing code (2 added lines across the two OSS ones). No prompt templates, model or tool config, agent loops, or evals. |
| 13 | Testing | See findings. No test was weakened or deleted — both modified test files are additive; the test_owner_management.py changes are ruff format reflows plus one new test pinning the retained_user_ids guard. |
| 14 | Dependencies & build | N/A — no lockfile, manifest, Dockerfile or CI-workflow changes in either repo. |
| 15 | Code quality | See findings — dead unconfigured_message arg and dead send_template_email (both in the cloud PR) |
| 16 | Doc & comment accuracy | See findings — worker docstrings (High), 4 Medium, 5 Low |
Unanchored findings
[Critical] cross-repo merge order — anchored inline at resource_share_views.py:178, but the other half of the evidence lives on cloud origin/main (backend/pluggable_apps/agentic_studio_v1/views/projects.py:156,163,171), which no diff here touches. Merge unstract-cloud#1698 first, or ship a transitional OSS commit keeping snapshot_share_axes/diff_share_axes/AxisDiff as deprecated wrappers for one release.
[Low] [Lens 1] scope creep — frontend/src/components/pipelines-or-deployments/notification-modal/CreateNotification.jsx:70-73 is an ETL notification edit-form fix with no relationship to UN-3494, in a change set that already spans two repositories and must land atomically. It widens the revert surface for no reason. The fix itself is sound: CreateNotification is conditionally mounted (NotificationModal.jsx:150-152), so it remounts on every Edit and the lazy useState initializer re-runs; the deleted form.resetFields() effect pair was genuinely redundant. Split it out or call it out in the description.
[Low] [Lens 16] PR description — "Recipients and access are resolved at send time, not click time" holds for only one of three paths. The direct-user path resolves and mails inline (resource_share_views.py:172); the group membership path deliberately freezes recipients at click time and its own docstring says so (share_notifications.py:135-137). The parenthetical about not telling a still-connected user they lost access does hold everywhere; it is the generalization that does not. The rest of the description's table checks out, as does "seven per-resource copies deleted, covering all eight shareable types".
Verified clean (recorded because several were non-obvious)
notification_plugingating on the inline paths holds in OSS —_notify_shared_usersreaches the senders only after_notification_contextreturns non-None, which returnsNonewhen the plugin is absent. An OSS deployment logs no traceback per share._retained_user_ids'None-vs-empty-set contract is honoured by all three callers.revoked_at's required-but-nullable wire contract holds end to end: the producer omits the key on a grant, but the worker task defaults it and always writes it into the POST body._resource_type_forcovers all 8SHAREABLE_RESOURCESkinds today (verified independently three ways).all(sent)overpool.mapcannot re-raise —_send_personalizationswraps its whole body.NOTIFICATION_QUEUE = "notifications"matchesQueueName.NOTIFICATION; the unknown-task-name → delete claim matchesconsumer.py:528-537;_organization_slugreally isOrganization.organization_id, not the pk; the auth0-vs-OSS admin role strings really do differ.CoOwnerManagement.jsx's render-phase derived state is legal and loop-free;createdBywas a dead prop onmain.- Test suite strengths worth preserving: the worker's retry-classification parametrization, the tenant-isolation tests that deliberately defeat their own false-green, and the mirror-image grant/revoke pair for
_groups_to_mail.
Open questions
- Which PR lands first, and is anything enforcing it?
- On a lost-after-send response, is the intent "accept the drop" or "retry"? The code does both; the comments claim only the first.
- Was the unbounded
max_workersdeliberate, or is a small ceiling acceptable?
Assumptions (each would change a severity if wrong)
DJANGO_ATOMIC_REQUESTSstaysFalse(pinned at cloudvalues.yaml:1553). If enabled, the_commitfinding becomes High.workerPgNotificationis deployed everywhere viaglobal.pgWorkerFleet.enabled: true(verified in base chart values). If an OSS install omits it, the queue-row accumulation becomes the dominant effect of the missing plugin gate.- Cloud CI's 5 red integration tests (
test_pg_barrier.py,could not translate host name "unstract-db") are CI DNS infrastructure, not this diff — cloudmainis green on its last 8 runs. Needs a re-run, not a code fix.
Standardized 16-lens review, unstract:standard-review (plugin v0.18.1). Posted as COMMENT — the merge-gate decision is the reviewer's.
| removed=groups_before - groups_after, | ||
| actor=request.user, | ||
| ) | ||
| self._notify_shared_users( |
There was a problem hiding this comment.
[Critical] [Lens 7, 2, 1] — OSS-first merge is a 500 on a share that already committed, not just missing emails
This call is unwrapped, and on a cloud image built from OSS main before unstract-cloud#1698 lands it does not reach the mixin's own _notify_shared_users.
cloud-backend-docker-build-push.yaml:21-25 checks out Zipstack/unstract at oss-branch and overlays the cloud tree, so OSS resource_share_views.py and cloud agentic_studio_v1/views/projects.py run in one process. On cloud origin/main, AgenticProjectViewSet still defines _notify_shared_users(self, project, before, request_data, actor) — exactly 4 positional params — which shadows the mixin's and binds this 4-arg call silently. Its first statement is self.diff_share_axes(...), deleted by this PR, raised before that method's own try:.
Two primary flows break:
PATCH /agentic-project/<pk>/→snapshot_share_axes→AttributeError→ 500 on every rename/description edit.POST /agentic-project/<pk>/share/→ this line →AttributeError→ 500 on a share that already committed.
Both PR descriptions characterise the ordering risk as only "these emails won't have anywhere to go yet", so the mitigating knowledge is not where a merger will look, and nothing across two repos enforces the order.
Fix: merge unstract-cloud#1698 first, or land a transitional OSS commit keeping snapshot_share_axes/diff_share_axes/AxisDiff as deprecated wrappers over _read_axis for one release. Either way, correct both descriptions. Reverse order is benign (lost emails only).
Confidence: High — overlay build and override binding both verified directly.
| ) -> tuple[bool, bool, str]: | ||
| """One POST attempt. Returns ``(succeeded, retryable, error)``. | ||
|
|
||
| A response lost after the backend already received it is treated like a |
There was a problem hiding this comment.
[High] [Lens 3, 6, 8, 16] — the "never re-post" classification is defeated one layer up; a lost-after-send response re-mails every group up to 5 times
This docstring is the reason ReadTimeout/WriteTimeout/ReadError/WriteError/RemoteProtocolError are marked retryable=False. But retryable=False only ends the in-process loop — control falls to _fail_group_notification, which raises.
These tasks are fire-and-forget (share_notifications._dispatch passes no reply_key, on_success or on_error), so the raise lands in the consumer's fire-and-forget branch at workers/queue_backend/pg_queue/consumer.py:613-623 — "leave the row — its vt expires and it is redelivered" — bounded by _DEFAULT_MAX_ATTEMPTS = 5 (consumer.py:82, unset for this consumer). Up to four further full fan-outs, each re-mailing every group the backend already mailed.
A ReadTimeout here is the expected failure shape, not an exotic one: the backend mails group-by-group synchronously over a SendGrid client built with no timeout (see unstract-cloud#1698, email_service.py:29).
Structural cause: enqueue_task has no idempotency/dedup key (backend/pg_queue/producer.py:103-118), so nothing downstream can suppress a repeat.
The guard test does not catch this — workers/tests/test_group_notification_post.py:115-122 asserts both len(client.calls) == 1 and isinstance(raised, RuntimeError), so it passes while the queue re-posts.
Fix: return the final (retryable, error) and raise only when the last attempt was retryable (transport never reached the backend). For the lost-after-send case, log metric=group_notification_post_failed_total at ERROR and return normally so the consumer acks. Redelivery is only safe once the send path has a per-group checkpoint — it has none by this comment's own admission.
Confidence: High.
| # Deleted between the share and the send — a retry cannot help. | ||
| logger.info("group-notification: dropping resource share (%s)", exc) | ||
| return Response({"status": "skipped"}, status=status.HTTP_200_OK) | ||
| return Response({"status": "success"}, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
[High] [Lens 3, 8, 10] — returns 200 whether or not any mail was sent, so the retry policy is exactly backwards
send_resource_shared returns nothing actionable and this returns {"status": "success"} unconditionally; same at line 96. Downstream, _mail_group (group_notification_service.py:315-324) and send_membership_changed (:166-172) both discard the plugin's bool, and _send_via_template converts every send-path exception into return False (unstract-cloud#1698, sharing_notification.py:56-58).
Net: a run that mailed zero of N groups is byte-identical on the wire to one that mailed all N.
This goes past the acknowledged "we won't know a refusal failed" limitation, because that 200 is now load-bearing for retry: redelivery fires on transport faults (where it duplicates mail) and never on an actual send failure (where a retry is the correct action).
Not flagged — the ResourceNotFoundError → 200 {"status": "skipped"} branch above is correct: a retry cannot resurrect a deleted resource.
Fix: return per-group (attempted, sent) counts and surface them; have the worker log sent == 0 and attempted > 0 under its own metric. Don't force redelivery until there is a checkpoint.
Confidence: High.
| revoked_at=revoked_at, | ||
| ) | ||
|
|
||
| def _mailed(self) -> list[tuple[str, list[str]]]: |
There was a problem hiding this comment.
[High] [Lens 13, 3] — the multi-group fan-out, the sole reason _group_recipients_batch exists, is never tested with more than one group
send_resource_shared's contract is "one email per group, so group_name in the template is always the group the recipient actually belongs to" (group_notification_service.py:87-89), and _group_recipients_batch (:265-296) is the batched implementation upholding it.
Every delivery test passes exactly one group id. A regression to a union — user_ids_by_group.get(group.pk) → all_user_ids — mails a "Finance" member saying the resource was shared with "Legal": a group-name disclosure to a non-member, with the whole suite green.
This helper already returns a list of (group_name, recipients) tuples — the right assertion shape — but no test ever produces a list longer than one element.
Fix: one test with two groups having overlapping and disjoint members, asserting _mailed() equals the exact two-entry list; and assert a user in both revoked groups receives two emails, so the documented per-group behaviour is pinned rather than incidental.
Confidence: High.
| return getattr(organization, "organization_id", None) | ||
|
|
||
|
|
||
| def _dispatch_quietly( |
There was a problem hiding this comment.
[Medium] [Lens 2, 9] — the group enqueue path has no plugin gate, unlike every sibling path
ENABLE_EMAIL_NOTIFICATIONS appears in this module only inside the docstring at line 14 — never in code — and neither call site gates (resource_share_views.py:172-177, group_views.py:173-178 and :203-208).
On a pure-OSS deployment, where notification_plugin can never exist, every group share, revoke, member add and member remove writes a pg_queue_message row, the worker claims it, POSTs to /internal/v1/group-notification/, and _service() returns None — a guaranteed-futile round trip per event. If an install does not run workerPgNotification, the rows accumulate indefinitely with nothing to consume them.
The sibling direct-user path is gated: _notification_context returns None when not notification_plugin (permissions/membership_views.py:89-90), and all six ViewSet get_notification_resource_type overrides open with the same check. This breaks that precedent — which is the one the next engineer copies.
Fix: check notification_plugin (or settings.ENABLE_EMAIL_NOTIFICATIONS) here in _dispatch_quietly and return early. One check covers both task types.
Confidence: High on the futile round trip; Medium on row accumulation (depends on the OSS chart shipping the worker).
| "actor_id": actor.pk, | ||
| "resource_kind": kind, | ||
| "resource_id": str(resource.pk), | ||
| "share_action": str(share_action), |
There was a problem hiding this comment.
[Low] [Lens 7] — serialises the enum via str() rather than .value on a cross-process payload
Correct today only because both are StrEnum (3.11+ __str__ returns the value). The same repo defines QueueName(str, Enum) (workers/shared/enums/worker_enums_base.py:146) with no __str__ override, where this idiom writes "ShareAction.SHARED" onto the wire.
Since this crosses a process boundary into a ChoiceField, a base-class change yields a 400 on every message rather than a type error at the write site. Everywhere else in the change set uses .value explicitly (group_notification_service.py:221, :250, :284).
Fix: share_action.value, and action.value at line 151.
|
|
||
| Revoking a share removes nothing in that case, so nobody should be told it | ||
| did. ``shared_to_org`` covers every org member; ``is_friction_less`` is the | ||
| adapter equivalent -- ``AdapterInstance.for_user`` admits it unconditionally. |
There was a problem hiding this comment.
[Low] [Lens 16] — "AdapterInstance.for_user admits it unconditionally" is false for service accounts
The manager excludes frictionless adapters for service accounts: return self.get_queryset().filter(is_friction_less=False) (adapter_processor_v2/models.py:44-45).
No live bug — service accounts are filtered out of every recipient list anyway by _live_member_users — but the word invites a future caller to rely on a property the manager does not have.
Fix: "…admits it for every non-service-account member."
| ) -> dict[int, list[User]]: | ||
| """Live members of each of ``groups`` who did not keep access via ``retained``. | ||
|
|
||
| One query across every group in the fan-out rather than one per group -- |
There was a problem hiding this comment.
[Low] [Lens 16] — "One query across every group" understates the count
_group_recipients_batch issues two queries: the GroupMembership.values_list at :283-288 and the OrganizationMember query inside _live_member_users (:355-357).
The rest of the paragraph makes clear the claim is about the OrganizationMember lookup specifically, but the opening clause reads as an absolute.
Fix: "One OrganizationMember query across every group in the fan-out rather than one per group."
| on the resource model, while ``shared_groups`` is stored polymorphically in | ||
| ``ResourceGroupShare`` (not an M2M) and routed through the sharing helpers; new | ||
| axes can be added by extending that attribute. | ||
| The mixin reads the sharing "axes" named in ``_SUPPORTED_SHARE_AXES``. |
There was a problem hiding this comment.
[Low] [Lens 16] — the docstring says the miin reads the aes in _SUPPORTED_SHARE_AXES; _read_ais cannot read one of them
_SUPPORTED_SHARE_AXES (line 24) includes "shared_to_org", which is a BooleanField — _read_ais's fallback set(getattr(instance, ais).all()) would raise AttributeError: 'bool' object has no attribute 'all'.
_SUPPORTED_SHARE_AXES is the accepted-payload-keys allowlist, not a read set; share() only ever reads the two per-recipient aes and says so at :158-160. The docstring conflates the two roles.
Fi: "_SUPPORTED_SHARE_AXES names the aes a POST /share/ body may set; only shared_users and shared_groups are readable via _read_ais."
| for the task's duration, so health-stale (360s) is the other ceiling. | ||
| """ | ||
| t = _GROUP_NOTIFICATION_TIMEOUT | ||
| per_post = t.connect + t.write + t.read + t.pool |
There was a problem hiding this comment.
[Low] [Lens 13, 16] — this test does not compute the worst case, and does not assert the bound its docstring names
per_post omits _GROUP_NOTIFICATION_RETRY_DELAY × 2 inter-attempt sleeps (tasks.py:514, slept at :610). Real worst case is 154s, not 150s.
It passes today because the margin is large — but _GROUP_NOTIFICATION_RETRY_DELAY appears nowhere in the formula, so raising it is invisible to the test meant to bound it. At a delay of 80s the real worst case is 310s, past the visibility timeout, and this assertion still reports 150.
Separately, the docstring introduces health-stale (360s) as "the other ceiling" and then asserts only against 300. Currently the stricter of the two, so not wrong — but the 360 bound is never exercised, so the docstring overstates what the test guards.
Fix: worst = per_post * ATTEMPTS + _GROUP_NOTIFICATION_RETRY_DELAY * (ATTEMPTS - 1), asserted against both ceilings, so a future VT change cannot quietly make health-stale the binding constraint.
2 participants
What
Not every action here notifies the same way — some go out instantly as part of the request, some queue up and send in the background, and one sends nothing at all:
The queue exists so emailing a whole group at once doesn't hold up the request; a single-recipient notification is fast enough to just send inline. Removing someone from the organization entirely is separate, pre-existing behavior this PR doesn't touch.
Why
How
Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
Relevant Docs
Dependencies Versions
Notes on Testing
68 tests across six modules (52 here, 16 in the cloud PR): direct/group share-revoke, group membership add/remove, retained-access skips, the restored direct-share email, retry/dedup safety, and email wording/skip logic. Co-owner modal Cancel verified manually in-browser (no automated FE test).
Screenshots
Checklist
I have read and understood the Contribution Guidelines.