fix(organizations): route user-deletion org cascade through the removal seam and make org removal atomic (#3971)

* fix(users): route sole-owner org deletion through the organization removal seam

users.service.js#remove() called OrganizationsRepository.remove() directly
when deleting a sole-owned org, bypassing organizations.crud.service.js#remove()
and its runOrganizationRemovedHandlers call — the onOrganizationRemoved seam
modules/tasks/tasks.init.js registers for org-scoped task cleanup. Org-scoped
tasks (and any future handler) were silently orphaned.

The cascade now delegates the full org teardown to
OrganizationsCrudService.remove() (lazy-imported to avoid a static cycle with
users.service.js, matching billing.init.js's existing pattern). A handler
error propagates out of the crud service by design (aborts that org's own
removal) but is caught + logged at this call site so it never aborts the
surrounding user deletion.

Centralized the co-member currentOrganization reassignment's null-org guard
(#3709) into organizations.crud.service.js#remove() since it's now the sole
implementation — the crud service's own version lacked it, which would have
crashed on a dangling membership once other callers started relying on this
path too.

Also fixes a latent test-fragility bug in tasks.integration.tests.js: two
"not a user's task" tests referenced a task orphaned by a sibling test's user
deletion, which only stayed queryable because of the very bug fixed here;
replaced with a properly isolated foreign-user fixture.

Closes #3965

Claude-Session: https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup

* fix(organizations): make org removal atomic so user deletion cannot orphan an org

Phase-0 BLOCK finding on #3965: users.service.js#remove() wrapped the ENTIRE
OrganizationsCrudService.remove() call in try/catch, then unconditionally
deleted the user. Because the crud remove() wiped memberships and reassigned
co-members BEFORE running onOrganizationRemoved handlers and the final
OrganizationsRepository.remove(), a handler failure anywhere in that cascade
was swallowed while the user still got deleted — a half-removed org (zombie
doc, or membership rows dangling on a deleted userId).

Fix (Option A — atomic reorder, chosen over narrowing the seam's error
posture per-caller): organizations.crud.service.js#remove() now removes the
org repository doc BEFORE running onOrganizationRemoved handlers, so once
removal starts the org always ends fully removed regardless of handler
outcome. Handler failures are caught + logged there for reconciliation
(never re-thrown) — same posture for every caller, not special-cased for
the user-deletion path. A STRUCTURAL failure (membership wipe, reassignment,
or the org repository delete itself) is not caught and still propagates.

Verified only two callers of the crud remove()/removeById() exist:
organizations.controller.js's admin delete-org endpoint and
users.service.js's sole-owner cascade — both go through the same function,
so no per-caller option/param was needed; relaxing the handler-error
posture to best-effort is safe for both.

users.service.js#remove() drops its blanket try/catch around the seam call:
handler failures are already swallowed inside the seam, so anything that
still escapes is a structural failure, which must propagate and abort the
whole user deletion (pre-diff behavior) rather than delete the user on top
of a genuinely broken teardown.

Tests: organizations.crud.orgRemoval.unit.tests.js now asserts a handler
throw is logged but the org is still removed, plus a new structural-failure
case that still propagates. users.service.remove.orgRemovalSeam.unit.tests.js
adds org-doc-removed assertions on handler throw (no zombie) and two new
structural-failure cases (org repo delete / membership wipe throwing) that
assert the user is NOT removed.

Claude-Session: https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup

* fix(organizations): isolate org-removal cleanup handlers so one failure never skips the rest

Per-handler try/catch in runOrganizationRemovedHandlers: every handler runs
even if an earlier one throws (a bug in one module's cleanup can no longer
orphan another module's org-scoped rows). Failures are collected and re-raised
as a single AggregateError; the crud service logs each one individually for
reconciliation. Best-effort semantics and the atomic-by-ordering removal are
unchanged.

Addresses CodeRabbit review on #3971.

Author: PierreBrisorgueil

ERRORS.md

@@ -32,3 +32,4 @@ Use this file as a compact memory of recurring AI mistakes.
 - [2026-06-15] deps/audit: leaving `npm audit` advisories unaddressed on the assumption they need a major bump -> run `npm audit fix` (never `--force`) first; the runtime-tree DoS/ReDoS items (`qs`, `path-to-regexp`, `brace-expansion`) all fixed via in-range bumps, no residual. These are DoS-class but NOT attacker-reachable in this stack: Express route patterns are static (no user-controlled `path-to-regexp` input) and `qs`/`brace-expansion` only parse server-side query strings under fixed code paths — still bump them to keep the tree clean and avoid scanner noise.
 - [2026-07-16] security: `users.repository.js findByIdAndUpdatePopulated()` does `.populate()` with no `.select()`, so `organizations.controller.js switchOrganization` serialized the raw doc (password hash + OAuth tokens + reset/verification tokens) straight to the client; `users.account.controller.js me()` separately forwarded `providerData` (OAuth tokens) verbatim -> any endpoint returning a Mongoose user doc must go through `UserService.removeSensitive()` (whitelist, `modules/users/utils/sanitizeUser.js`) at the response boundary, never serialize `req.user`/a populated doc directly; a local-signup fixture's `providerData` defaults to `{}` so a naive falsy check won't catch this — seed a fake OAuth token in tests to prove the leak is actually closed; see pierreb-devkit/Node#3963
 - [2026-07-16] billing/stripe: the #3742 priceId-map fix (`resolvePlan`/`buildPriceIdToPlanMap`) was applied only to the webhook handler; `billing.admin.service.js` `resolveStripePlan` (admin force-sync, a DB WRITE path) and `billing.reconcile.service.js` `resolveStripePlan` (LOG-ONLY divergence check) kept their own copies reading only `metadata.planId` -> a paid org's Stripe subscription (which never carries `metadata.planId`) resolved to `'free'` on admin sync (silently downgrading a paying org) and produced a false `planMismatch` alert on every reconcile run; fix = one shared resolver (`modules/billing/lib/billing.planResolver.js`) used by all three call sites, and a null-unresolved sentinel (never guess `'free'`) so the admin write path ABORTS instead of downgrading and the reconcile log path skips the comparison instead of alerting; see pierreb-devkit/Node#3964
+- [2026-07-16] architecture: `users.service.js remove()`'s sole-owner cascade called `OrganizationsRepository.remove()` directly instead of `organizations.crud.service.js#remove()`, silently skipping `runOrganizationRemovedHandlers` (the `onOrganizationRemoved` seam `modules/tasks/tasks.init.js` registers for org-scoped task cleanup) -> deleting an org must always route through the owning module's SERVICE method, never straight to its Repository, even for a "bare" cascade delete from another module — a direct-repository shortcut silently bypasses any registered removal hook; a stale cross-test fixture in `tasks.integration.tests.js` (an orphaned-but-still-existing task reused across unrelated test cases) was inadvertently relying on this bug and needed a properly isolated fixture once the cascade actually cleaned up; see pierreb-devkit/Node#3965

modules/organizations/lib/orgRemoval.registry.js

@@ -2,9 +2,17 @@
  * @module organizations/lib/orgRemoval.registry
  * @description Subscriber registry for organization-removal cleanup.
  *   Optional modules register a handler at load time via `onOrganizationRemoved`;
- *   the organization crud service runs them sequentially on org delete and
- *   propagates their errors (no silent swallow). Config-free, import-safe leaf —
- *   it must not import organization/tasks services (avoids an import cycle).
+ *   `runOrganizationRemovedHandlers` runs every handler sequentially, ISOLATING each —
+ *   one handler throwing never prevents the remaining handlers from running (so a bug
+ *   in one module's cleanup can't orphan another module's org-scoped rows). Failures are
+ *   collected and, after the full run, re-raised together as a single AggregateError to
+ *   ITS caller (no silent swallow inside this module). The current sole caller,
+ *   organizations.crud.service.js#remove, calls this AFTER the org doc and its
+ *   memberships are already gone, and deliberately catches + logs each aggregated error
+ *   there (best-effort) so a handler bug can never leave a zombie org or abort an
+ *   unrelated caller's own work (#3965) — see that function's docblock. Config-free,
+ *   import-safe leaf — it must not import organization/tasks services (avoids an
+ *   import cycle).
  */
 
 const handlers = new Set();
@@ -24,13 +32,24 @@ export const onOrganizationRemoved = (fn) => {
 
 /**
  * @function runOrganizationRemovedHandlers
- * @description Run every registered handler sequentially. Errors propagate (not swallowed).
+ * @description Run every registered handler sequentially, isolating each: a handler
+ *   throwing is caught so every later handler still runs, then all failures are re-raised
+ *   together as a single AggregateError once the full run completes (not swallowed).
  * @param {Object} payload - { organizationId, organization }.
  * @returns {Promise<void>}
+ * @throws {AggregateError} If one or more handlers threw — carries every handler error.
  */
 export const runOrganizationRemovedHandlers = async (payload) => {
+  const errors = [];
   for (const fn of handlers) {
-    await fn(payload); // sequential: each handler must resolve before the next runs
+    try {
+      await fn(payload); // sequential: each handler must resolve before the next runs
+    } catch (err) {
+      errors.push(err); // isolate: a failing handler must not block the remaining ones
+    }
+  }
+  if (errors.length > 0) {
+    throw new AggregateError(errors, `${errors.length} organization-removed handler(s) failed`);
   }
 };
 

modules/organizations/services/organizations.crud.service.js

@@ -188,11 +188,22 @@ const update = async (organization, body) => {
 
 /**
  * @function remove
- * @description Service to delete an organization, all its memberships, and any
- *   data owned by optional modules that registered an org-removal handler via
- *   `onOrganizationRemoved`. Handlers run sequentially after membership cleanup;
- *   any handler error propagates and aborts the delete before the repository
- *   removal (no silent swallow).
+ * @description Service to delete an organization and all its memberships. Removal is
+ *   atomic-by-ordering: membership cleanup, affected-user reassignment, and the org
+ *   repository delete itself all happen BEFORE any onOrganizationRemoved handler runs —
+ *   once removal starts, the org always ends fully removed (no zombie org doc, no
+ *   memberships left pointing at it), regardless of what a handler does (#3965).
+ *   Handlers (data owned by optional modules, e.g. tasks) then run sequentially and
+ *   ISOLATED — one throwing never skips the rest — BEST-EFFORT: every handler error is
+ *   logged here for manual reconciliation of that module's leftover org-scoped rows, but
+ *   never re-thrown. This is deliberate — a
+ *   handler failure must not resurrect/block a removal that has already committed, and
+ *   must not propagate into an unrelated caller (e.g. users.service.js#remove's
+ *   sole-owner cascade, which deletes the user right after this call and must not have
+ *   that deletion aborted by a downstream task-cleanup bug).
+ *   A STRUCTURAL failure — the membership wipe, the reassignment loop, or the org
+ *   repository delete itself throwing — is NOT caught here and propagates to the
+ *   caller, so a genuinely broken teardown is never reported as success.
  * @param {Object} organization - The organization to delete.
  * @returns {Promise<Object>} A promise resolving to a confirmation of the deletion.
  */
@@ -205,20 +216,41 @@ const remove = async (organization) => {
   // Remove all memberships for this organization
   await MembershipRepository.deleteMany({ organizationId: orgId });
 
-  // For each affected user, switch to their next available org or set null
+  // For each affected user, switch to their next available org or set null.
+  // Guard against a dangling membership whose populated organizationId is null (ref
+  // to another, already-deleted org) — otherwise `.organizationId._id` throws (#3709,
+  // centralized here from users.service.js's own cascade by #3965).
   await Promise.all(affectedUsers.map(async (u) => {
     const remaining = await MembershipRepository.list({ userId: u._id, status: MEMBERSHIP_STATUSES.ACTIVE });
-    const nextOrg = remaining.length > 0
-      ? (remaining[0].organizationId._id || remaining[0].organizationId)
+    const liveMemberships = remaining.filter((m) => m.organizationId != null);
+    const nextOrg = liveMemberships.length > 0
+      ? (liveMemberships[0].organizationId._id || liveMemberships[0].organizationId)
       : null;
     await UserService.updateById(u._id, { currentOrganization: nextOrg });
   }));
 
-  // Run org-removal cleanup handlers registered by optional modules (e.g. tasks).
-  // Errors propagate and abort the delete before the repository removal.
-  await runOrganizationRemovedHandlers({ organizationId: orgId, organization });
-
+  // Structural teardown is complete — memberships gone, affected users reassigned.
+  // Remove the org doc itself BEFORE running any optional cleanup handler, so the org
+  // can never be left half-removed by a handler throwing (#3965).
   const result = await OrganizationsRepository.remove(organization);
+
+  // Run org-removal cleanup handlers registered by optional modules (e.g. tasks) AFTER
+  // the org is structurally gone. Best-effort: log and continue on failure (see docblock).
+  try {
+    await runOrganizationRemovedHandlers({ organizationId: orgId, organization });
+  } catch (err) {
+    // The registry isolates each handler and re-raises every failure as one AggregateError,
+    // so later handlers already ran. Log each failure individually for reconciliation.
+    const failures = err instanceof AggregateError ? err.errors : [err];
+    failures.forEach((failure) => {
+      logger.error('organizations.crud.remove: org-removal cleanup handler failed after the org was removed (needs reconciliation)', {
+        organizationId: String(orgId),
+        message: failure?.message,
+        stack: failure?.stack,
+      });
+    });
+  }
+
   return result;
 };
 

modules/organizations/tests/organizations.crud.orgRemoval.unit.tests.js

@@ -1,5 +1,7 @@
 /**
- * Unit tests — OrgCrudService.remove() fires the org-removal registry and propagates handler errors.
+ * Unit tests — OrgCrudService.remove() fires the org-removal registry and is atomic:
+ * the org doc + memberships are always gone together before any handler runs, and a
+ * handler error is caught + logged (best-effort), never re-thrown (#3965).
  */
 import { jest, describe, test, expect, beforeEach } from '@jest/globals';
 
@@ -8,9 +10,10 @@ const mockMembershipDeleteMany = jest.fn().mockResolvedValue({ deletedCount: 0 }
 const mockMembershipList = jest.fn().mockResolvedValue([]);
 const mockUpdateById = jest.fn().mockResolvedValue({});
 const mockFindWithFilter = jest.fn().mockResolvedValue([]);
+const mockLoggerError = jest.fn();
 
 jest.unstable_mockModule('../../../lib/services/logger.js', () => ({
-  default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() },
+  default: { error: mockLoggerError, warn: jest.fn(), info: jest.fn() },
 }));
 
 jest.unstable_mockModule('../../../config/index.js', () => ({
@@ -61,17 +64,103 @@ describe('OrgCrudService.remove() — org-removal registry', () => {
     expect(mockOrgRemove).toHaveBeenCalledWith(organization);
   });
 
-  test('propagates a handler error and aborts before the repository remove', async () => {
+  test('a handler error is best-effort — logged, never re-thrown, and the org is still fully removed (#3965)', async () => {
     onOrganizationRemoved(async () => {
       throw new Error('tasks cleanup failed');
     });
 
-    await expect(OrgCrudService.remove(organization)).rejects.toThrow('tasks cleanup failed');
-    expect(mockOrgRemove).not.toHaveBeenCalled();
+    await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true });
+    // The org repository delete ran BEFORE the handler — removal is atomic-by-ordering.
+    expect(mockOrgRemove).toHaveBeenCalledWith(organization);
+    expect(mockLoggerError).toHaveBeenCalledWith(
+      'organizations.crud.remove: org-removal cleanup handler failed after the org was removed (needs reconciliation)',
+      expect.objectContaining({ organizationId: 'org-1', message: 'tasks cleanup failed' }),
+    );
+  });
+
+  test('isolates handler failures — a failing handler does not skip a later one, and every failure is logged (#3965)', async () => {
+    const failingFirst = jest.fn().mockRejectedValue(new Error('tasks cleanup failed'));
+    const succeedingSecond = jest.fn().mockResolvedValue(undefined);
+    onOrganizationRemoved(failingFirst);
+    onOrganizationRemoved(succeedingSecond);
+
+    await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true });
+
+    // The second handler still ran despite the first throwing (per-handler isolation).
+    expect(failingFirst).toHaveBeenCalledTimes(1);
+    expect(succeedingSecond).toHaveBeenCalledTimes(1);
+    expect(succeedingSecond).toHaveBeenCalledWith({ organizationId: 'org-1', organization });
+    // The single failure is logged for reconciliation; the org is still fully removed.
+    expect(mockOrgRemove).toHaveBeenCalledWith(organization);
+    expect(mockLoggerError).toHaveBeenCalledTimes(1);
+    expect(mockLoggerError).toHaveBeenCalledWith(
+      'organizations.crud.remove: org-removal cleanup handler failed after the org was removed (needs reconciliation)',
+      expect.objectContaining({ organizationId: 'org-1', message: 'tasks cleanup failed' }),
+    );
+  });
+
+  test('logs each failure when multiple handlers throw, and still runs every handler (#3965)', async () => {
+    const failingA = jest.fn().mockRejectedValue(new Error('tasks cleanup failed'));
+    const failingB = jest.fn().mockRejectedValue(new Error('files cleanup failed'));
+    const succeeding = jest.fn().mockResolvedValue(undefined);
+    onOrganizationRemoved(failingA);
+    onOrganizationRemoved(failingB);
+    onOrganizationRemoved(succeeding);
+
+    await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true });
+
+    expect(failingA).toHaveBeenCalledTimes(1);
+    expect(failingB).toHaveBeenCalledTimes(1);
+    expect(succeeding).toHaveBeenCalledTimes(1);
+    // One log line per failing handler (both aggregated failures surfaced individually).
+    expect(mockLoggerError).toHaveBeenCalledTimes(2);
+    expect(mockLoggerError).toHaveBeenCalledWith(
+      'organizations.crud.remove: org-removal cleanup handler failed after the org was removed (needs reconciliation)',
+      expect.objectContaining({ organizationId: 'org-1', message: 'tasks cleanup failed' }),
+    );
+    expect(mockLoggerError).toHaveBeenCalledWith(
+      'organizations.crud.remove: org-removal cleanup handler failed after the org was removed (needs reconciliation)',
+      expect.objectContaining({ organizationId: 'org-1', message: 'files cleanup failed' }),
+    );
+  });
+
+  test('a STRUCTURAL failure (org repository delete itself throws) propagates and is NOT swallowed', async () => {
+    mockOrgRemove.mockRejectedValueOnce(new Error('db delete failed'));
+
+    await expect(OrgCrudService.remove(organization)).rejects.toThrow('db delete failed');
   });
 
   test('with zero handlers registered, removes the organization without throwing', async () => {
     await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true });
     expect(mockOrgRemove).toHaveBeenCalledWith(organization);
   });
+
+  test('reassigns an affected user without throwing when their remaining membership.organizationId is null (dangling ref, #3709)', async () => {
+    mockFindWithFilter.mockResolvedValueOnce([{ _id: 'coUid' }]);
+    mockMembershipList.mockResolvedValueOnce([{ _id: 'm2', organizationId: null, status: 'active' }]);
+
+    await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true });
+    expect(mockUpdateById).toHaveBeenCalledWith('coUid', { currentOrganization: null });
+  });
+
+  test('skips a null-org membership and picks the first live org when reassigning an affected user (mixed, #3709)', async () => {
+    mockFindWithFilter.mockResolvedValueOnce([{ _id: 'coUid' }]);
+    mockMembershipList.mockResolvedValueOnce([
+      { _id: 'm2', organizationId: null, status: 'active' },
+      { _id: 'm3', organizationId: { _id: 'orgY' }, status: 'active' },
+    ]);
+
+    await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true });
+    expect(mockUpdateById).toHaveBeenCalledWith('coUid', { currentOrganization: 'orgY' });
+  });
+
+  test('reassigns to a non-populated (raw id) organizationId when the membership was not populated', async () => {
+    mockFindWithFilter.mockResolvedValueOnce([{ _id: 'coUid' }]);
+    mockMembershipList.mockResolvedValueOnce([
+      { _id: 'm4', organizationId: 'orgZ', status: 'active' },
+    ]);
+
+    await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true });
+    expect(mockUpdateById).toHaveBeenCalledWith('coUid', { currentOrganization: 'orgZ' });
+  });
 });

modules/organizations/tests/organizations.orgRemoval.registry.unit.tests.js

@@ -35,16 +35,35 @@ describe('orgRemoval.registry', () => {
     expect(order).toEqual(['first', 'second']);
   });
 
-  test('propagates a handler error (does not swallow) and aborts the remaining handlers', async () => {
+  test('isolates a handler error — the remaining handlers still run, then failures re-raise as one AggregateError', async () => {
     const boom = new Error('cleanup failed');
     const after = jest.fn().mockResolvedValue(undefined);
     onOrganizationRemoved(async () => {
       throw boom;
     });
     onOrganizationRemoved(after);
 
-    await expect(runOrganizationRemovedHandlers({ organizationId: 'org-1' })).rejects.toThrow('cleanup failed');
-    expect(after).not.toHaveBeenCalled();
+    await expect(runOrganizationRemovedHandlers({ organizationId: 'org-1' })).rejects.toThrow(AggregateError);
+    // A failing handler must not block a later one (avoids orphaning another module's rows).
+    expect(after).toHaveBeenCalledTimes(1);
+  });
+
+  test('aggregates every handler failure — each error is carried on the AggregateError', async () => {
+    const boomA = new Error('tasks cleanup failed');
+    const boomB = new Error('files cleanup failed');
+    const middle = jest.fn().mockResolvedValue(undefined);
+    onOrganizationRemoved(async () => {
+      throw boomA;
+    });
+    onOrganizationRemoved(middle);
+    onOrganizationRemoved(async () => {
+      throw boomB;
+    });
+
+    await expect(runOrganizationRemovedHandlers({ organizationId: 'org-1' })).rejects.toMatchObject({
+      errors: [boomA, boomB],
+    });
+    expect(middle).toHaveBeenCalledTimes(1);
   });
 
   test('runs zero handlers without throwing when none are registered', async () => {