Skip to content

feat(workflows): restrict sandbox email to verified org members - #127149

Closed
Silthus wants to merge 13 commits into
masterfrom
workflows-sandbox-delivery/member-gate
Closed

Silthus wants to merge 13 commits into
masterfrom
workflows-sandbox-delivery/member-gate

Conversation

@Silthus

@Silthus Silthus commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • A new team cannot send its first workflow email until it verifies a sending domain.
  • A PostHog-owned sandbox sender lets the team try email first, but it is one shared identity. It must never deliver outside the team's own organization, or report a send it did not make.
  • This PR is the first of five. It adds the member gate that every later sandbox send passes through.

Warning

🟡 Merge risk: medium · two-way door (merge-risk rubric)

  • The CDP email send path changes. Only sends through a sandbox integration take the new branches, and no such integration exists on master yet.
  • Sandbox sends now skip the team's tier sending budget and pass an explicit recipient list to the suppression check. Every other sender builds the same list as before.
  • Revert: git revert the merge commit. Nothing delivers through this PR and it stores no state.

Changes

A send through an email integration with provider: sandbox now always skips with a reason in the run log, and never records email_sent:

Situation Run log workflows sandbox email blocked reason
Worker switch off, or a route setting empty The sandbox sender is unavailable right now switch_off
A non-test send from a destination, not a workflow Only available for workflows and test sends none
A To, CC or BCC address is not an active member of the team's organization Lists the blocked addresses recipient_not_member
Every blocked address belongs to a member with an unverified email Lists the unverified addresses recipient_unverified
The member lookup fails, times out after 3 seconds, or resolves after its one-minute window Lists every address check_failed
Every check passes The sandbox sender is unavailable right now (no route yet) none
  • The switch is WORKFLOWS_SANDBOX_SENDER_ENABLED plus three SES_SANDBOX_* route settings. All default to off or empty.
  • The worker checks recipients before it prepares the send and again right before delivery, so a member removed mid-send is blocked.
  • OrganizationMembersService loads a team's member emails in one query and never trusts a snapshot older than a minute.
  • The blocked event fires at most once an hour per team, reason and send kind. Its store call runs under a 1 second deadline, so a stalled store never holds the send. At most ten store calls run at once, on Redis connections of their own, so a stalled store cannot take the connections hog watcher and the duplicate observer use. A capture waiting for a slot holds no connection and gives up at its deadline.
  • That pool is nonfatal: Redis refusing its connections never stops the worker.
  • The member lookup fails the send closed after 3 seconds, counting the wait for a pooled connection. A statement timeout also stops the query on the server. At most five lookups are open at once, so lookups past their deadline cannot pile up on the common pool, and one that gets its connection late runs no query.
  • The next PR replaces the final "unavailable" skip with delivery.

Stack

  1. feat(workflows): restrict sandbox email to verified org members #127149 member gate ← this PR
  2. feat(workflows): route sandbox email through its fixed ses identity #127150 sandbox route
  3. feat(workflows): cap sandbox sends per project and recipient each day #127151 daily caps
  4. feat(workflows): provision a managed sandbox email sender #127152 sender row
  5. feat(workflows): show the managed sandbox sender in channels #127153 Channels row

How did you test this code?

  • Live run with the Node cdp-api at this head (dd8e42a9780) behind the public API on a box dev stack: outside@example.com is skipped with the member message; a verified member passes the gate and skips as unavailable, since delivery arrives in feat(workflows): route sandbox email through its fixed ses identity #127150; with the worker switch off the send skips as unavailable. SES got no call. Stubs: a local SES endpoint that records each request, a DNS preload that answers an MX record for example.com, workflows-sandbox-sender answered true, PersonHog off (nothing on the send path reads persons).
  • Prod shape: no production integration has the sandbox provider, so nothing in production reaches the gate until the flagged sender ships. The member read is one indexed query per team, cached for a minute.
  • Stalls: fails closed after three seconds when the lookup cannot get a connection, keeps at most five lookups open while the pool has no connection, and runs no query for them once it recovers and fails closed instead of waiting when the member lookup stalls (an exclusive lock on the membership table from another transaction) cover the lookup. keeps at most ten blocked-event store calls open while the store stalls, and resumes once they settle and captures every event of a burst of blocked sends while the store answers cover the capture bound. deadline-slots.test.ts covers the slot helper both bounds share.
  • Live stall run at this head: a proxy in front of the CDP Redis stalls every blocked-event store call, and two bursts of 15 concurrent non-member sends plus a member send all answer within 2.5 seconds. Exactly ten store calls stall; the other captures give up at their deadline, and no other Redis user waits. Before the dedicated pool, three stalled captures took the shared pool and the remaining sends hung.
  • Review: GPT-6.1 Sol reviewed the PR and the fixes in several final reads; each finding is fixed in a commit on this stack, and the posthog[bot] threads are answered.

Test rationale: the new sandbox sender block in email.service.test.ts drives executeSendEmail against the test Postgres and Redis with SES stubbed. No existing test sent through a sandbox integration. Its membership cases cover CC, BCC, lists, ambiguous mailboxes, unverified, inactive and other-organization members; each catches a recipient reaching delivery without being a verified member. organization-members.service.test.ts uses an injected clock to catch a stale snapshot deciding a send.

Release status

  • This change is behind a feature flag and is not available to users

Docs update

  • docs/internal/workflows-email-sending-tiers.md gains a "Sandbox delivery" section: the switch, the member rule and the blocked event.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5 (claude-opus-5-5). Review by GPT-6.1 Sol.

Skills: /recreating-fork-prs, /stacking-prs, /writing-pr-descriptions, tdd, merge-risk, qa-swarm

More context for AI agents
  • Session: squashed from feat(workflows): restrict sandbox email to verified org members Silthus/posthog#594 at 9ffcb7a96ec onto master bf976a762be. The tree equals that fork head merged into the same master.
  • Decisions: a send that passes the gate skips as unavailable and emits no event, until the route PR.
  • Decisions: per-workflow pacing still applies to sandbox sends; only the team tier budget is skipped.
  • Decisions: the member lookup gets its own 3 second deadline, because the common pool sets no acquire or statement timeout and the send waits for it.
  • Decisions: blocked-event captures bound open store calls before they start, not after a deadline, because a deadline cannot cancel a call already sent. They get their own pool, sized to that bound and opened on first use: on the shared cdp-redis pool, a live stall run showed three stalled captures taking every connection.
  • Read closely: the sandbox branch in email.service.ts and organization-members.service.ts.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Silthus Silthus added hogbox-preview Spin up a per-PR PostHog preview on a hogland hogbox posthog-review labels Nov 2, 2026
@Silthus Silthus self-assigned this Nov 2, 2026
@trunk-io

trunk-io Bot commented Nov 2, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane

This PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Hogbox preview — ready, open the preview

▶ Open the preview

🔑 Login test@posthog.com / 12345678 (demo data)
🧩 Running this PR's backend on the PostHog :master base (frontend unchanged by this PR)
🔗 Link stable across rebuilds: a re-push swaps the box underneath, the URL stays
🔒 Access tailnet only (PostHog VPN)
🛠️ Admin inspect & debug state in hogland
💤 Idle sleeps after ~30 min idle (snapshot to S3, zero node cost) and wakes on your next visit in ~30s, behind a brief "waking up" screen

commit 5cf7753 · box box-ae5b66fe26e1 · ready in 2143s (push → usable) · build log · rebuilds on every push, torn down on close

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review · Standard · reviewed 6ade8db: 2 Should fix.

This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.

See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.

@hosthog

hosthog Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

HostHog preview — posthog-desktop-web

The previews for this PR have been torn down and no longer serve.

The final member check reused the cached snapshot, so a member removed while
the send was prepared still passed it for up to a minute. The final check now
reloads membership.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
markForRefresh only drops the cached snapshot. A fresh check still joined a lookup another
send started before the member was removed, and took its stale answer. The final check now
runs its own query.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ookup

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ture

A stalled membership query or event store held the skipped invocation open, and with it the
worker's whole batch. The lookup now runs under a statement timeout and fails closed; the
blocked-event capture gives up after a second.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A capture past its deadline still holds its Redis connection or pending acquisition, so a long
stall grew the backlog with every blocked send. At most ten wait at once; the rest are dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silthus and others added 2 commits November 2, 2026 12:08
…ill the cap

The cap counted every capture in flight, so a burst of blocked sends against a healthy store lost
events. Only captures that outlived their deadline count now, and each releases its slot when it
settles, so captures resume once the store answers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Client

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Counting only stalled captures let every capture in the first second start a store call, so a
burst during an outage could open any number of them. At most ten store calls run at once now;
a capture waiting for a slot holds no connection and gives up at its deadline, and a finished
call hands its slot to the next waiter, so a burst against a healthy store keeps every event.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Nov 4, 2026

This branch was successfully deployed

1 active deployment
hogbox-preview — 5cf7753c Deployed Nov 3, 2026 by github-actions[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hogbox-preview Spin up a per-PR PostHog preview on a hogland hogbox stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant