Skip to content

tenant-audit, the update half: ack is a dispatcher sweep but redeliver is request-reachable — two sites on one object with OPPOSITE classifications #10740

Description

@os-warren

Found while implementing #10673 (PR #10725), which classified the updateMany half of the tenant-audit surface on sys_http_delivery / sys_notification_delivery. Filed separately rather than widened into that PR: one of the two sites here needs a contract decision, which fails the bounded-inline-fix test.

Read the two classifications before touching either. They are opposite, and the dangerous mistake is to treat this as one sweep.

The surface

The same two objects carry three single-record (multi: false) writes that the SQL driver audits under the update op — a different op from the updateMany one #10673 covered:

sitereachable fromclassification
SqlNotificationOutbox.ackdispatcher tick onlyglobal sweep — same warrant as #10673's six
SqlHttpOutbox.ackdispatcher tick onlyglobal sweep — same warrant
SqlHttpOutbox.redeliverPOST /api/v1/webhooks/redeliverrequest-contextual — a bypass must NEVER be put here

plugin-webhooks' own comment on that route says any authenticated user can reach it. So on a deployment with OS_TENANCY_POSTURE=isolated, redeliver is an unscoped write reachable by an authenticated user — exactly the case the tenant audit exists to catch, and the one case where silencing the audit line would convert a detectable hole into an undetectable one.

Why this is not simply "finish the job from #10673"

#10673's six sites earned bypassTenantAudit on a structural warrant, not a judgement call: their only callers are the dispatchers' setInterval tick under a cluster lock (no request context exists on that path); ClaimOptions / HttpClaimOptions are {nodeId, limit, partition, claimTtlMs, now} and carry no tenant even in principle; partitioning is hash(refId|notificationId|digestKey) mod N — a load-spreading key, not an org key, so a partition holds every org's rows by construction; and one outbox per environment drains the whole queue, so a per-org predicate would strand every other org's deliveries forever and leave a crashed node's rows permanently in_flight.

redeliver satisfies none of that. It has a request context, it is reachable by a user, and the remedy is to thread a tenant, not to declare a sweep — which changes the IHttpOutbox.redeliver contract. That is the decision this card exists to get made.

⚠️ Why the startup log did not show it — the evidence surface is narrower than it looks

#10673's card quotes a composed-boot startup log showing exactly two [tenant-audit] lines. That is not evidence that the update op is clean:

The reap UPDATE runs unconditionally on every dispatcher tick, while ack / redeliver only run once a delivery is actually processed. A startup log taken on an empty queue therefore shows exactly the two updateMany lines and says nothing at all about the update-op surface.

Anyone re-measuring this should drive an actual delivery through, not read a boot log.

What a fix must decide

  1. ack × 2 — carry the same documented warrant (the helper packages/services/service-messaging/src/outbox-dispatcher-scope.ts, added by PR fix(messaging): classify the delivery dispatchers' updateMany sweeps as global environment sweeps #10725, already holds the reasoning; note its return type is & { multi: true }, so it cannot be reused here as-is and that is deliberate).
  2. redeliver — thread the request's tenant. This changes IHttpOutbox.redeliver's signature, so it is a contract change and wants the contract-review tier. ⛔ It must not be given bypassTenantAudit to make the line go away — the audit's whole purpose is to force this classification, and this is the site it was built for.

Refs

Filed unassigned for triage. Duplicate search run before filing (nothing open beyond #10673 itself).

Metadata

Metadata

Assignees

Labels

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions