UN-3494 [FEAT] Email users and groups through PGMQ - #2224
kirtimanmishrazipstack wants to merge 37 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
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
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.
…p-sharing-notification
|
@muhammad-ali-e Went through every Critical/High/Medium/Low finding here (and the paired cloud review) and pushed fixes for all of them except the ones below — status on each: Turned out stale/hallucinated — the referenced code doesn't existChecked each of these against the actual file content at the exact commit cited as reviewed (
Happy to be pointed at the actual commit/diff this was read from if there's a real version of this concern I'm missing — as written, it doesn't match what's in the tree. FixedAll 4 High findings, and 9 of the Medium/Low each:
150 tests green across Valid, deferred (not fixed here)
Valid, but not new — pre-existing on an unrelated, already-shipped path
No action needed
|
High: a permanent send failure no longer raises into PG redelivery (re-mailing a whole group on a lost-after-send response); send results are now propagated end to end instead of discarded, so a failed send returns non-2xx and a misconfiguration returns 200/skipped; missing multi-group fan-out test coverage added. Medium: enqueue path now skips when the notification plugin isn't loaded (pure OSS no longer writes dead queue rows); split the conflated actor-left-org vs unresolved-resource-type log into distinct metric keys; corrected the retry-timeout-budget comment (154s worst case, not 150 -- missed the inter-attempt sleep delays) and the test pinning it; stale docstrings corrected (atomic-block claim, axis-agnostic claim, ENABLE_EMAIL_NOTIFICATIONS-only claim). Low: added_user_ids response now echoes what was actually inserted, not the full request, plus a test for the concurrent-add narrowing; str(enum) -> .value on both enum serializations in the outbound payload; two more stale docstrings corrected (one-query-vs-two, service-account exception to the frictionless-adapter claim). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
_retained_user_ids checks four routes a member could still reach a resource through (another group, a direct share, ownership, org admin); only the direct-share one had a test. A member who is in both the revoked group and a second group that still has access was untested and would have silently regressed to being mailed a wrong "you lost access" notice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
…unction length - group_notification_service.py: send each group's notification concurrently (ThreadPoolExecutor, capped at 10) instead of one blocking SendGrid call per group in sequence; extracted _resolve_share/_mail_all_groups so send_resource_shared stays under the repo's 30-line function limit. - notification_resource_types.py (new): single source of truth for the adapter/pipeline -> notification ResourceType mapping, now shared by group_notification_service.py, adapter_processor_v2/views.py, and pipeline_v2/views.py instead of three independently hand-maintained copies. - resource_share_views.py: documented why direct-share notifications stay synchronous (matches every other share call site) while group notifications dispatch via PGMQ (unbounded group size justifies it). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
muhammad-ali-e
left a comment
muhammad-ali-e
left a comment
There was a problem hiding this comment.
Standardized review — FOLLOWUP — BLOCK
Summary — Critical: 2 · High: 7 · Medium: 9 · Low: 4 · Lenses run: 16/16
Re-reviewed with the paired PR as one change set. Previous boundary: OSS 04402d05 / cloud d6884044. Now: OSS 8b9f034e / cloud 0cedc74f. Every one of my 35 prior findings has an explicit status in its own thread above.
Scope change: YES — new module notification_resource_types.py; a ThreadPoolExecutor on the backend send path (the first ORM-touching thread pool anywhere in backend/); a new 502 on the internal wire contract; a deleted public method in cloud. All re-scanned.
Prior findings
Resolved (17) · Partially resolved (7) · Not resolved (9, four of them acknowledged as deliberately skipped) · Waived (1 — the OCR fallback; your reasoning is right, it predates this PR)
The work here is real: 17 closed outright, the test additions are genuinely good (I verified the multi-group test would actually catch a union regression, and the get_plugin seam did not hollow out existing assertions), and no test was deleted or weakened — the two removed isinstance(raised, RuntimeError) assertions were correctly replaced to match the deliberate _fail/_drop split.
Why this is still BLOCK
The two largest fixes introduced a worse defect than the ones they closed. C1 and three of the seven Highs share one root cause: a single boolean is carrying two meanings. A tri-state at the plugin boundary — SENT / SKIPPED / FAILED, with 502 only on FAILED — collapses C1, H1, H3 and H4 together. H2 is a one-line list(...). That is the short path from here to a much smaller list.
Two things to untangle, both fair
The Critical was checked against the wrong refs. snapshot_share_axes / diff_share_axes are on cloud origin/main (3b7289ce) at projects.py:156,171; they are absent from pr1698 because deleting them is what that PR does. Full reproduction in the thread. build-push.yaml defaults oss-branch to "main" so dev/staging track OSS main HEAD — the RC path pins a release, which is a genuine mitigation that keeps production out of it. Merging cloud #1698 first resolves it with no code change.
Findings attributed to me that aren't mine. The "stale/hallucinated" bucket lists a High about feature flags plus _feature_enabled, GROUP_NOTIFICATION_FLAG_KEY and Flipt. A scan of all 35 of my comments returns zero matches — those came from coderabbitai and greptile-apps. You are very likely right that they are hallucinated; the only issue is that bundling them with the Critical made it look like more of the same.
Lens checklist (16/16)
1 See findings · 2 See findings · 3 See findings · 4 Clean — no auth/tenant surface changed; isolation re-verified intact · 5 See findings — still zero migrations, still no idempotency key on enqueue_task · 6 See findings — new thread pool, H2 and H7 · 7 See findings — the 502 is a wire-contract change · 8 See findings · 9 See findings — H7, and nested 10×10 pools · 10 See findings — the drop path logs no payload · 11 See findings — C1 · 12 N/A — no model/prompt/tool/eval paths · 13 See findings — regression sweep clean · 14 N/A — no dependency, lockfile or CI changes · 15 See findings · 16 See findings.
Unanchored findings
[Medium] _STATIC_RESOURCE_TYPES is still a second copy, and its comment is now false. notification_resource_types.py genuinely deduped adapter_instance and pipeline — both ViewSets delegate now — but the other six kinds remain mapped twice. The comment above _STATIC_RESOURCE_TYPES still says "OSS must not import a cloud-only enum", which notification_resource_types.py:15 now does; it is safe only because every caller happens to gate on notification_plugin, and nothing enforces that. backend/tenant_account_v2/checks.py:23 already walks SHAREABLE_RESOURCES — asserting mapping coverage there would make a missing kind fail at startup instead of going quiet. Details in the thread on that comment.
[Low] Stale doc. [CLOUD] prompting/tasks/WORKFLOW_SHARING_IMPLEMENTATION.md:248 still documents send_template_email(), which this change set deletes.
[Low] _live_member_users docstring names one of two exclusions. group_notification_service.py:396-399 says service accounts are excluded; the comprehension at :404-408 also drops any user with a falsy email. The log line at :411-413 gets it right. Since this list decides whether a group has recipients — and therefore whether the 502 fires — the hidden filter matters.
Assumption
RC builds pin an OSS release rather than tracking main, which is what keeps the Critical off production. If any RC has ever been cut from main, it escalates.
Standardized 16-lens review, unstract:standard-review (plugin v0.18.1), FOLLOWUP mode. Posted as COMMENT — the merge-gate decision is the reviewer's.
…and the rest of ali's FOLLOWUP Critical (both): the 502-on-False contract collapsed four different plugin outcomes -- unset template, ENABLE_EMAIL_NOTIFICATIONS=false, bad input, and a genuine SendGrid failure -- into one signal, and the worker retries any 502 to its attempt cap. Every non-delivery condition was looping forever; confirmed live on dev, where ENABLE_EMAIL_NOTIFICATIONS is false today. Fixed with a tri-state result (True=sent, None=skipped/no-retry-needed, False=genuine failure) threaded from the plugin boundary through to the view's status code. Only an explicit False now returns 502. High: ThreadPoolExecutor.map wrapped in all() cancelled pending futures on the first False -- groups past the concurrency cap silently never got mailed. Fixed by materializing the full result list before aggregating. A partial group failure (some sent, some didn't) is deliberately not retried either -- redelivery would re-mail the groups that already succeeded, the exact duplication the retry split exists to prevent; the loss is logged instead. The ORM query resolving a resource's organization ran lazily inside pool threads with no connection cleanup (up to 10 leaked connections per request) -- fixed by populating the FK cache with the organization already in scope, before entering the pool. Also: cross-repo Critical from the first round reopened -- the deleted axis-diff shim was checked against the PR branches (correctly absent) instead of cloud's origin/main, where the still-stale AgenticProjectViewSet.partial_update calls it. Restored share_axes / snapshot_share_axes / diff_share_axes / AxisDiff as a temporary, explicitly deprecated shim (matches the original always-empty-diff contract exactly, pinned by a new test) so cloud's stale main doesn't AttributeError once this branch's mixin lands -- delete it once cloud #1698 merges. Medium/Low: drop-path now logs the payload (a dropped revoke is compliance-visible, not silent); explicit invariant + comment on the retry-loop's seed value; corrected the timeout-budget comment's "real bound" framing to "nominal" (a trickling response defeats per-phase read timeouts); stale docstring/comment fixes (env-knob framing, resource- type-mapping duplication, _live_member_users' undocumented email filter); notification-narrowing test for the add-members endpoint. 11 new/extended tests. 1719 backend tests (1714 pass, 5 pre-existing environment-gap errors unrelated to this diff, 8 skipped), 1522 worker tests (1 skipped) -- zero regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
…n the notification worker - retryable=True -> False as the loop's zero-iteration seed: a misconfigured attempt-cap of 0 must drop, not raise into an endless redelivery loop that never actually attempts a POST. - Rename the wall-time consistency test off a name that promised a measured guarantee it never checked, drop the brittle ==154 pin in favor of the two ceilings (VT 300s, health-stale 360s) it exists to protect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018KZLGSa3oWxgVJdFqvRUQX
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Unstract test resultsPer-group results
Critical paths
|
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.