fix(plugins)!: a failed business-unit hierarchy read refuses instead of answering nobody, and a unit keeps every child unit past 1,000

.changeset/29655-business-unit-hierarchy-reads-refuse.md

@@ -0,0 +1,25 @@
+---
+'@objectstack/plugin-sharing': patch
+'@objectstack/plugin-approvals': patch
+---
+
+fix(plugins)!: a failed `sys_business_unit` read refuses instead of answering nobody, and a business unit with more than 1,000 child units keeps every child
+
+Clause-②: no (narrowing)
+Metadata-protocol: none
+
+<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped and no metadata changes: the business-unit expansions read the unit rows they already read, to completion, and refuse a read that already failed. No author has anything to rewrite. -->
+
+**BREAKING** only where a `sys_business_unit` read fails: the business-unit expansions used to answer nobody (an empty list) then, and now refuse.
+
+The member reads of a business unit (`sys_business_unit_member`) read to completion and refuse on failure. The reads of the unit table they are keyed by did not: the check that the unit exists and is active, and the read of each unit's child units, still turned any read error into nobody, and the child-unit read stopped at 1,000 rows.
+
+- **A failed unit read refuses.** In plugin-sharing, `BusinessUnitGraphService.descendants`, `expandUnitMembers` (every `business_unit` sharing recipient) and `expandUsers` (every `unit_and_subordinates` recipient) now throw when the unit's own row or a child-unit read fails. In plugin-approvals, the `department` / `business_unit` / `bu` approver and an `expression` approver with `resolveAs: 'department'` do the same.
+  - Sharing: the expansion rejects, so that rule's reconcile or revoke pass fails, its callers log their existing per-rule warning, and the grants it already holds stay. Before, a failed unit read answered `[]` and that answer was kept for the rest of the pass, so the recompute a business-unit graph write triggers revoked every grant the rule gave.
+  - Approvals: the approver slot falls back to its literal (`department:ID`) and logs `could not be expanded — a directory or read-filter lookup failed` with its `error`. Before, a failed seed read logged "expanded to nobody", and a failed child-unit read routed to the units that did read, with no signal.
+  - Ownership queues: a `business_unit` or `unit_and_subordinates` queue specification whose unit read fails contributes nobody, the posture the queue declares for a failed expansion, and logs it once.
+- **Nothing is cached after a failed read.** The next expansion on the same pass reads again.
+- **Every child unit.** The child units of each unit are read in pages of 1,000 ordered by id until they are exhausted. A unit with more than 1,000 active child units used to lose the children past the 1,000th row, with all their members and descendants. A unit with fewer issues one read, as before.
+- **A missing object is still nobody.** A stack that never registers, or never creates, `sys_business_unit` keeps answering no members. That is judged by `isObjectAbsentError(error, 'sys_business_unit')`; any other error stays loud.
+
+**The remedy.** None is needed. A refusal means the business-unit table could not be read: fix that read, and the next expansion succeeds.

content/docs/automation/approvals.mdx

@@ -458,7 +458,8 @@ What an entry that resolves to nobody leaves on the slate depends on its kind:
   `behavior` that needs every slot (`unanimous`) waits on it too. A `team`,
   `position`, `department` or `org_membership_level` entry whose holder read
   failed (`sys_team_member`, `sys_user_position`, `sys_business_unit_member`
-  or `sys_member`) keeps its literal the same way. It logs `could not be
+  or `sys_member`, and for a `department` also the `sys_business_unit` read of
+  the unit and of its child units) keeps its literal the same way. It logs `could not be
   expanded — a directory or read-filter lookup failed` with the `error`,
   instead of `expanded to nobody`, and never routes to the holders that
   happened to read. An `expression` value whose `resolveAs` read failed keeps

packages/plugins/plugin-approvals/src/approval-service.ts

@@ -1127,6 +1127,25 @@ const MEMBER_SCREEN_READ_LIMIT = 50000;
  */
 const HOLDER_PAGE_ROWS = 10000;
 
+/**
+ * [#29655] Rows per page of the business-unit descent's child-unit read — the
+ * bound that read always carried, so a parent with fewer children issues the
+ * one read it always did, and one with more is read in further pages.
+ */
+const CHILD_UNIT_PAGE_ROWS = 1000;
+
+/**
+ * [#29362, #29655] Why a directory read refused: what could not be read, the
+ * driver's reason, and why the answer is a refusal rather than nobody. The
+ * driver's error rides along as `cause`.
+ */
+function holderReadRefusal(scope: string, err: unknown): Error {
+  return Object.assign(new Error(
+    `[approvals] ${scope} could not be read: ${err instanceof Error ? err.message : String(err)}. `
+    + 'Routing to a shortened slate would leave holders unasked, so the expansion refuses instead.',
+  ), { cause: err });
+}
+
 /**
  * Approver types resolved by QUERYING a graph rather than by taking `value`
  * literally (#3807). Each can legitimately come back empty — an unstaffed
@@ -3214,25 +3233,37 @@ export class ApprovalService implements IApprovalService {
    */
   private async expandBusinessUnitUsers(businessUnitId: string, organizationId?: string | null): Promise<string[]> {
     if (!businessUnitId) return [];
+    const inOrg = organizationId ? ` in organization '${organizationId}'` : '';
     // Seed sanity check: skip if dept doesn't exist or is inactive within tenant.
+    // [#29655] A failed seed read refuses — the slot then says the read failed
+    // rather than "expanded to nobody". A stack with no `sys_business_unit`
+    // has no departments.
+    let live: boolean;
     try {
-      if (!(await this.businessUnitIsLive(businessUnitId, organizationId))) return [];
-    } catch { return []; }
+      live = await this.businessUnitIsLive(businessUnitId, organizationId);
+    } catch (err) {
+      if (isObjectAbsentError(err, 'sys_business_unit')) return [];
+      throw holderReadRefusal(`business unit '${businessUnitId}'${inOrg}`, err);
+    }
+    if (!live) return [];
 
     const seen = new Set<string>([businessUnitId]);
     const queue: string[] = [businessUnitId];
     while (queue.length) {
       const parent = queue.shift()!;
-      let kids: any[] = [];
-      try {
-        const filter = this.businessUnitOrgScope(
-          { parent_business_unit_id: parent, active: { $ne: false } },
-          organizationId,
-        );
-        kids = await this.engine.find('sys_business_unit', { filter, fields: ['id'], limit: 1000, context: SYSTEM_CTX } as any);
-      } catch { kids = []; }
+      // [#29655] Every active child unit, read to completion — or a refusal.
+      // It read once at a 1,000-row bound and answered `[]` on any failure:
+      // a child past the bound, or every child of a parent whose read failed,
+      // left the slate with its members and no signal.
+      const kids = await this.readRowsToCompletion(
+        'sys_business_unit',
+        this.businessUnitOrgScope({ parent_business_unit_id: parent, active: { $ne: false } }, organizationId),
+        [],
+        `the child units of business unit '${parent}'${inOrg}`,
+        CHILD_UNIT_PAGE_ROWS,
+      );
       for (const k of kids ?? []) {
-        const kid = String((k as any).id ?? '');
+        const kid = String(k?.id ?? '');
         if (kid && !seen.has(kid)) { seen.add(kid); queue.push(kid); }
       }
     }
@@ -3249,8 +3280,7 @@ export class ApprovalService implements IApprovalService {
       // proved their tenancy; these rows have not, and the
       // shared seeded unit id is exactly where other tenants' rows sit.
       this.businessUnitMemberScope({ business_unit_id: { $in: Array.from(seen) } }, organizationId),
-      `the members of business unit '${businessUnitId}' and its descendant units`
-        + `${organizationId ? ` in organization '${organizationId}'` : ''}`,
+      `the members of business unit '${businessUnitId}' and its descendant units${inOrg}`,
     );
   }
 
@@ -3488,24 +3518,40 @@ export class ApprovalService implements IApprovalService {
    * attributed to `object`, so an absence of another object is still loud.
    */
   private async readHoldersToCompletion(object: string, where: Record<string, unknown>, scope: string): Promise<string[]> {
+    const rows = await this.readRowsToCompletion(object, where, ['user_id'], scope, HOLDER_PAGE_ROWS);
     const users = new Set<string>();
+    for (const row of rows ?? []) {
+      const uid = String(row?.user_id ?? '');
+      if (uid) users.add(uid);
+    }
+    return Array.from(users);
+  }
+
+  /**
+   * [#29362, #29503, #29655] Every `object` row matching `where` (its `id`
+   * plus `fields`), read to COMPLETION through the platform's one keyset walk
+   * — or a refusal; `null` when `object` does not exist on this stack, so each
+   * caller says what absence means for it. The one walk behind
+   * {@link readHoldersToCompletion} and the business-unit descent of
+   * {@link expandBusinessUnitUsers}.
+   */
+  private async readRowsToCompletion(
+    object: string,
+    where: Record<string, unknown>,
+    fields: readonly string[],
+    scope: string,
+    pageSize: number,
+  ): Promise<Record<string, unknown>[] | null> {
+    const rows: Record<string, unknown>[] = [];
     const walk = keysetWalk<Record<string, unknown>>(
-      (page) => this.engine.find(object, { ...page, fields: ['id', 'user_id'], context: SYSTEM_CTX }),
-      { where, pageSize: HOLDER_PAGE_ROWS },
+      (page) => this.engine.find(object, { ...page, fields: ['id', ...fields], context: SYSTEM_CTX }),
+      { where, pageSize },
     );
     try {
-      for await (const page of walk.pages()) {
-        for (const row of page) {
-          const uid = String(row?.user_id ?? '');
-          if (uid) users.add(uid);
-        }
-      }
+      for await (const page of walk.pages()) rows.push(...page);
     } catch (err) {
-      if (isObjectAbsentError(err, object)) return [];
-      throw Object.assign(new Error(
-        `[approvals] ${scope} could not be read: ${err instanceof Error ? err.message : String(err)}. `
-        + 'Routing to a shortened slate would leave holders unasked, so the expansion refuses instead.',
-      ), { cause: err });
+      if (isObjectAbsentError(err, object)) return null;
+      throw holderReadRefusal(scope, err);
     }
     if (walk.truncated) {
       throw new Error(
@@ -3514,7 +3560,7 @@ export class ApprovalService implements IApprovalService {
         + 'holders unasked, so the expansion refuses instead.',
       );
     }
-    return Array.from(users);
+    return rows;
   }
 
   /**

packages/plugins/plugin-approvals/src/business-unit-hierarchy-complete.integration.test.ts

@@ -0,0 +1,208 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * [#29655] The approver business-unit expansion's `sys_business_unit` reads —
+ * the seed check and the child-unit descent — answer EVERY unit, or refuse.
+ * #29606 closed the member read; this closes the unit reads it is keyed by.
+ *
+ * The seed check answered `[]` on a read that threw, and the descent read
+ * each parent's child units once at a 1,000-row bound and answered `[]` on any
+ * failure: a failed seed read logged "expanded to nobody", indistinguishable
+ * from an unstaffed unit, and a failed or cut descent routed to a shrunk slate
+ * with no signal at all.
+ *
+ * What a caller sees now: the expansion throws, the approver-spec resolver's
+ * directory catch takes it, and the slot falls back to its literal with the
+ * warning that names the failed read and carries its `error`. A unit object
+ * that does not exist is still no members.
+ *
+ * Measured on a REAL engine (ObjectQL + SqlDriver over in-memory SQLite)
+ * carrying the shipped `SysBusinessUnit` and `SysBusinessUnitMember`.
+ */
+
+import { describe, it, expect, afterEach, vi } from 'vitest';
+import { ObjectQL } from '@objectstack/objectql';
+import { SqlDriver } from '@objectstack/driver-sql';
+import { SysBusinessUnit, SysBusinessUnitMember, SysMember, SysOrganization } from '@objectstack/platform-objects/identity';
+import { ApprovalService } from './approval-service.js';
+
+const ORG = 'org_tree';
+const ROOT = 'bu_root';
+const CHILD = 'bu_child';
+/** The bound each child-unit read is paged at. */
+const PAGE = 1000;
+
+const pad = (n: number) => String(n).padStart(5, '0');
+
+/** Which `sys_business_unit` read a fault targets. */
+type UnitRead = 'seed' | 'descent';
+
+interface Rig {
+  engine: ObjectQL;
+  driver: SqlDriver;
+  svc: ApprovalService;
+  warn: ReturnType<typeof vi.fn>;
+  /** Every member of the tree, sorted. */
+  members: string[];
+  /** The child-unit reads the driver ran for ROOT's children. */
+  rootChildReads: () => number;
+  /** Fail every read of one kind; every other read runs as before. */
+  failUnitReads: (which: UnitRead) => void;
+}
+
+const engines: ObjectQL[] = [];
+afterEach(async () => {
+  vi.restoreAllMocks();
+  while (engines.length) {
+    try { await engines.pop()?.destroy(); } catch { /* noop */ }
+  }
+});
+
+/**
+ * ROOT, its child CHILD, and `extra` more child units of ROOT, each with one
+ * member. Unit ids are scrambled against creation order.
+ */
+async function boot(extra: number, opts: { omit?: string[] } = {}): Promise<Rig> {
+  const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as never);
+  const engine = new ObjectQL();
+  engine.registerDriver(driver as never, true);
+  await engine.init();
+  const omit = new Set(opts.omit ?? []);
+  engine.registerApp({
+    id: 'com.objectstack.qa.business-unit-hierarchy-complete',
+    name: 'business unit hierarchy complete',
+    version: '1.0.0',
+    type: 'plugin',
+    scope: 'system',
+    objects: [SysOrganization, SysMember, SysBusinessUnit, SysBusinessUnitMember].filter((o) => !omit.has(o.name)),
+  } as never);
+  await engine.syncSchemas();
+  engines.push(engine);
+
+  const e = engine as any;
+  await e.insert('sys_organization', { id: ORG, name: ORG, slug: 'tree' }, { context: { isSystem: true } });
+  const kids = Array.from({ length: extra }, (_, i) => `bu_kid_${pad((i * 7919) % 100003)}`);
+  const units = [ROOT, CHILD, ...kids];
+  if (!omit.has('sys_business_unit')) {
+    const rows: Record<string, unknown>[] = [
+      { id: ROOT, name: 'Root', kind: 'division', active: true, organization_id: ORG },
+      { id: CHILD, name: 'Child', kind: 'department', active: true, parent_business_unit_id: ROOT, organization_id: ORG },
+      ...kids.map((id) => ({ id, name: id, kind: 'department', active: true, parent_business_unit_id: ROOT, organization_id: ORG })),
+    ];
+    const d = e.getDriverForObject('sys_business_unit');
+    for (let i = 0; i < rows.length; i += 250) await d.bulkCreate('sys_business_unit', rows.slice(i, i + 250));
+  }
+  const members = units.map((u) => `usr_of_${u}`);
+  const memberRows = units.map((u, i) => ({
+    id: `bum_${pad(i)}`, user_id: `usr_of_${u}`, business_unit_id: u, organization_id: ORG,
+  }));
+  const md = e.getDriverForObject('sys_business_unit_member');
+  for (let i = 0; i < memberRows.length; i += 250) await md.bulkCreate('sys_business_unit_member', memberRows.slice(i, i + 250));
+
+  let rootChildReads = 0;
+  (driver as any).knex.on('query', (q: { sql: string; bindings?: unknown[] }) => {
+    if (/^select .* from [`"]sys_business_unit[`"]/i.test(q.sql) && /parent_business_unit_id/.test(q.sql)
+      && (q.bindings ?? []).includes(ROOT)) rootChildReads += 1;
+  });
+  const failUnitReads = (which: UnitRead) => {
+    const d = e.getDriverForObject('sys_business_unit');
+    const real = d.find.bind(d);
+    vi.spyOn(d, 'find').mockImplementation(((obj: string, ...rest: unknown[]) => {
+      const descent = JSON.stringify(rest).includes('parent_business_unit_id');
+      if (obj === 'sys_business_unit' && (which === 'descent') === descent) {
+        return Promise.reject(Object.assign(new Error('SQLITE_IOERR: disk I/O error'), { code: 'SQLITE_IOERR' }));
+      }
+      return real(obj, ...rest);
+    }) as never);
+  };
+  const warn = vi.fn();
+  const svc = new ApprovalService({
+    engine: engine as any,
+    logger: { warn, info: () => {}, error: () => {}, debug: () => {} } as any,
+  });
+  return { engine, driver, svc, warn, members: [...members].sort(), rootChildReads: () => rootChildReads, failUnitReads };
+}
+
+/** The private expansion and the resolver, as the request opener calls them. */
+const department = (rig: Rig, value = ROOT) =>
+  (rig.svc as any).expandBusinessUnitUsers(value, ORG) as Promise<string[]>;
+const resolveSpec = (rig: Rig, a: { type: string; value: string }) =>
+  (rig.svc as any).resolveApproverSpec(a, undefined, ORG, Date.now()) as Promise<string[]>;
+const failureWarnings = (rig: Rig) => rig.warn.mock.calls.filter(([, meta]) => typeof meta?.error === 'string');
+
+describe('[#29655] approvals business-unit expansion: a failed seed read refuses', () => {
+  it('⛔ the expansion refuses — it never answers an invented empty slate', async () => {
+    const rig = await boot(0);
+    rig.failUnitReads('seed');
+
+    await expect(department(rig)).rejects.toThrow(
+      /\[approvals\] business unit 'bu_root' in organization 'org_tree' could not be read: SQLITE_IOERR/,
+    );
+  });
+
+  it('⛔ what the caller sees: the department slot falls back to its literal, and the warning names the failed read', async () => {
+    const rig = await boot(0);
+    rig.failUnitReads('seed');
+
+    expect(await resolveSpec(rig, { type: 'department', value: ROOT })).toEqual([`department:${ROOT}`]);
+    const failed = failureWarnings(rig);
+    expect(failed.map(([, meta]) => meta.type)).toEqual(['department']);
+    expect(failed[0][0]).toMatch(/could not be expanded — a directory or read-filter lookup failed/);
+    expect(failed[0][1].error).toMatch(/SQLITE_IOERR/);
+  });
+});
+
+describe('[#29655] approvals business-unit expansion: a failed descent read refuses', () => {
+  it('⛔ the expansion refuses — the slate never shrinks to the seed unit', async () => {
+    const rig = await boot(0);
+    rig.failUnitReads('descent');
+
+    await expect(department(rig)).rejects.toThrow(
+      /\[approvals\] the child units of business unit 'bu_root' in organization 'org_tree' could not be read: SQLITE_IOERR/,
+    );
+  });
+
+  it('⛔ what the caller sees: the literal and the failed-read warning, not a shrunk slate', async () => {
+    const rig = await boot(0);
+    rig.failUnitReads('descent');
+
+    expect(await resolveSpec(rig, { type: 'department', value: ROOT })).toEqual([`department:${ROOT}`]);
+    expect(failureWarnings(rig).map(([, meta]) => meta.type)).toEqual(['department']);
+  });
+});
+
+describe('[#29655] approvals business-unit expansion reads every child unit past the 1,000-row bound', () => {
+  it('⭐ 1,001 child units: every member of every child is on the slate', async () => {
+    // CHILD plus 1,000 more: 1,001 children of ROOT.
+    const rig = await boot(PAGE);
+
+    expect([...(await department(rig))].sort()).toEqual(rig.members);
+    expect(rig.members).toHaveLength(PAGE + 2);
+    expect(rig.rootChildReads()).toBe(2);
+  }, 60_000);
+
+  it('control — under the bound: one child-unit read, the same slate as before', async () => {
+    // CHILD plus 998 more: 999 children of ROOT.
+    const rig = await boot(PAGE - 2);
+
+    expect([...(await department(rig))].sort()).toEqual(rig.members);
+    expect(rig.rootChildReads()).toBe(1);
+  }, 60_000);
+});
+
+describe('[#29655] approvals business-unit expansion: absence still answers nobody', () => {
+  it('a composition that never registers sys_business_unit has no departments: the literal, with no failure warning', async () => {
+    const rig = await boot(0, { omit: ['sys_business_unit'] });
+
+    expect(await department(rig)).toEqual([]);
+    expect(await resolveSpec(rig, { type: 'department', value: ROOT })).toEqual([`department:${ROOT}`]);
+    expect(failureWarnings(rig)).toEqual([]);
+  });
+
+  it('a stack whose sys_business_unit table was never created has no departments', async () => {
+    const rig = await boot(0);
+    await (rig.driver as any).knex.schema.dropTable('sys_business_unit');
+
+    expect(await department(rig)).toEqual([]);
+  });
+});