Skip to content

feat(cdp): share sandbox member lookups and bound waiting ones - #129049

Closed
Silthus wants to merge 58 commits into
workflows-sandbox-split/rate-limiter-claimsfrom
workflows-sandbox-split/member-cache
Closed

Silthus wants to merge 58 commits into
workflows-sandbox-split/rate-limiter-claimsfrom
workflows-sandbox-split/member-cache

Conversation

@Silthus

@Silthus Silthus commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • Every sandbox send runs two fresh member lookups, so a busy workflow puts one Postgres query per check on the shared pool.
  • #128944 dropped the member snapshot cache and the bound on waiting lookups to keep the skeleton small. This PR brings both back.
  • Part 2 of the Node caps stack, on #128946: #129048 → this PR → #129050 → #129051 → #129052. Split from #128410, which stays open until this split replaces it.
  • Landing order: the Node pieces deploy after the skeleton and before the flag reaches any team beyond ours.

Merge danger

Caution

🔴 Merge risk: high · two-way door (merge-risk rubric)

  • LazyLoader is shared by most Node caches, and Semaphore by hog function execution. A bug in either can serve stale values or stall work for most teams.
  • What limits it: the existing get and run paths keep their behavior, and new tests pin the reload and give-up paths. The member check runs only for the sandbox sender.
  • Revert: git revert the merge commit. Nothing to clean up.

Changes

  • Sandbox sends from one project share one member snapshot, reloaded before it is a minute old. The check right before delivery still reads fresh members.
  • A cached check that would block reloads a snapshot older than 5 seconds first, so a member who just joined or verified is not blocked.
  • At most five lookups wait for a connection at once. Semaphore.run takes a give-up signal for this, and others fail closed at the 3 second deadline.
  • LazyLoader.reload joins another reload only before its query starts. Later callers share one queued follow-up, so a key gets at most two member queries per burst.
  • Every reload sees writes made before its call. A queued reload fails at once when the running one fails.
  • Trade-off: a blocked send that arrives during another send's reload waits for that lookup, then its own. That is up to two bounded lookups on success, and it fails closed either way.
  • BoundedPrimaryLookup holds the deadline, statement timeout and slot cap. The pause check in #129054 reuses it.

How did you test this code?

  • Live run and flag-off parity at this head: Not run yet.
  • lazy-loader.test.ts shows a reload that sees a write made during another reload, one shared follow-up load, and queued reloads failing with the running one.
  • organization-members.service.test.ts counts 3 queries for six blocked sends during a held reload, and covers sharing, expiry, the 5 second recheck and the five-lookup cap.
  • The same test returns or destroys every late or stalled client, so the next check succeeds.
  • sempahore.test.ts shows that a caller that gives up leaves the queue without running.
Earlier runs at older heads, not this head

Live run at the #129052 head bb10d28, which contains this PR. Same devbox kit as #128946: private Django and Node cdp-api, a seeded org, a local server that records each SES request, and the dev Valkey for the allowances. Sends used the editor's "Run test" path.

  • Members in To, CC and BCC, a custom From and preview-text cases: sent, after the fresh member check before delivery.
  • An outsider in To, CC or BCC, or an unverified member: skipped, and SES gets no request.

Member cache live at c3050aa, an earlier head of this PR. The real OrganizationMembersService ran against the kit database, with a second session locking the membership table to stall lookups.

Case Result
Two sends at once, then a later send All allowed, one lookup in total
Snapshot a minute old, its reload stalled, a second send starts during the reload Both allowed once the lock goes; the second send joined the reload
Member deactivated, cached check inside the minute Still allowed from the cache
Same member, fresh check before delivery Blocked
Membership locked, 8 fresh checks at once 5 lookups open on the server; all 8 fail at 3 s with "The organization member lookup timed out"
After the lock goes No lookup left running; the next fresh check is allowed

Flag-off parity at older head (bea3fbe). Own-domain sends must stay unchanged while the sandbox sender is off. A scratch Jest harness sent four own-domain sends through the real EmailService, on master fb75476b8a9 and on that head. A disabled sandbox sender and the member lookup were wired in as spies.

Send SES request, logs, metrics, result
Marketing workflow send Same as master
Marketing test send Same as master
Transactional, CC, BCC and Reply-To Same as master
Custom From name and address Same as master
  • Neither spy was called.
  • The comparison masks only team ids, token timestamps and signatures. Tenant team-<id> and configuration set posthog-messaging match.
  • The harness is scratch and not in the diff. The same result holds at the Node stack top 031a2c3d448.

Test rationale: each case catches a member check that serves a stale allow, storms the primary, or holds a slot for good.

Release status

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

Docs update

  • docs/internal/workflows-email-sending-tiers.md says the membership snapshot expires within one minute.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5

Skills: /writing-tests, /writing-code-comments, /stacking-prs, /writing-pr-descriptions

More context for AI agents
  • Session: Claude Code. Review rounds 1 and 2 and final reads 1 to 6 ran on Claude Opus 5.5 only, because the GPT reviewers were unreachable for the whole session.
  • Decisions: review replaced deadline-slots with a give-up signal on Semaphore, and dropped the second expiry clock for LazyLoader's own.
  • Decisions: review added the 5 second recheck for cached blocks, then the reload rules in final reads 2 to 5 to bound the query storm.
  • Decisions: the risk grade went from low to high, because the PR changes LazyLoader and Semaphore, which most Node paths share.
  • Follow-ups: null is_email_verified members are blocked; that decision stays open.
  • Read closely: reload in nodejs/src/common/utils/lazy-loader.ts, BoundedPrimaryLookup.run, then getBlockedRecipients in organization-members.service.ts.
  • Origin: split from #128410. The merged tops of the new stacks match the old stack's production code; see the split report.

Stack decisions, moved here from the visible body:

Question Answer Who Where
Who gets a sandbox email Verified members of the team's organization, checked at send time over To, CC and BCC 👤 Human decision record
Sender identity Fixed From address on a dedicated PostHog domain, "Acme via PostHog" display name, no From or Reply-To override 👤 Human decision record
Where it can send Workflow email steps and test sends only, not broadcasts or transactional email 👤 Human decision record
How the sender is stored One managed email integration per team, provider sandbox, created only by PostHog code 👤 Human decision record
SES setup One shared identity, configuration set and tenant for all sandbox sends 👤 Human decision record
Landing order Three-PR skeleton, then the flag to our team, then caps and polish (replaced landing the twelve-layer stack at once) 👤 Human decision record
Node gate Environment switch WORKFLOWS_SANDBOX_SENDER_ENABLED, no feature flag in CDP Node code 👤 Human decision record
Member check in the skeleton Keep it, although the ask was a skeleton with no limits 👍 Accepted default decision record
Behavior with everything off No change while the flag and switch are off 👍 Accepted default decision record
Earlier PRs Close #127149 to #127153, #127136 and #127137, each linked to its successor 👍 Accepted default decision record
PR shape Two native stacks of small PRs: Node (4) and product (5) 👍 Accepted default decision record
Member lookup cache in the skeleton Dropped; every check is one fresh lookup, the cache returns with the caps stack 👍 Accepted default decision record
Member lookup that gets its connection late Runs no query; the cap on waiting lookups returns with the cache 🤖 Agent decision record
Tier budget Sandbox sends never spend the team's tier budget 🤖 Agent decision record
When members are checked Before preparing the send and again fresh before delivery 🤖 Agent decision record
Member lookup fails Skip the send with a log 🤖 Agent decision record
Blocked-send events At most one per team, reason and hour 🤖 Agent decision record
How a team gets the sender Opening Channels with the flag on creates it 🤖 Agent decision record
Managed sender guards Nobody can edit, verify, delete or default the sandbox sender 🤖 Agent decision record
Quoted member addresses Skip the send, and canonicalize in the caps stack instead of respinning the skeleton 🤖 Agent decision record
Re-split of #128410 to #128414 Pieces of about 400 changed lines or fewer, Node and product code in separate PRs 👤 Human requested in the agent session that wrote this split
Where the lookup protections live One BoundedPrimaryLookup, shared by the member and pause checks 🤖 Agent decision record
When a bounded lookup frees its slot Once its connection is released or destroyed, including a stalled BEGIN 🤖 Agent decision record
Member snapshot scope One cached snapshot per project for up to a minute, not per organization 🤖 Agent decision record

🤖 Generated with Claude Code

Brings back the one-minute member snapshot that sends share, with a
fresh lookup right before delivery, and caps open lookups at five with
deadline-slots. A lookup that gets its connection after its deadline
still runs no query.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Silthus Silthus self-assigned this Nov 4, 2026
@github-actions

github-actions Bot commented Nov 4, 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.

✅ Comment density — 2% of added code lines are comments (11 of 636)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
nodejs/src/common/utils/lazy-loader.ts 9 76
nodejs/src/cdp/utils/sempahore.ts 2 20

This check does not block merging. It updates on every push and clears when the share drops.

Move the member lookup's deadline, statement timeout, slot bound and
late-connection guard into BoundedPrimaryLookup, so the sandbox pause
read can use the same protection.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silthus and others added 3 commits November 4, 2026 12:38
…lows-sandbox-split/member-cache

# Conflicts:
#	nodejs/src/cdp/services/managers/organization-members.service.ts
…eased

Bounded primary lookups now run through the deadline transaction. A
connection that arrives after the deadline sends no SQL, and one still
busy at the deadline, BEGIN included, is destroyed, so its slot frees
for the next lookup.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silthus and others added 16 commits November 4, 2026 14:03
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

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

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
stamphog[bot]

This comment was marked as outdated.

…box-split/member-cache

Carries the null-verification rule into the cached snapshot and fixes the
member cache in the same resolution:

- the LazyLoader refresh age alone bounds snapshot age; the second expiry
  clock, its injected now and both expiry branches are gone
- a cached block is checked again fresh, and a fresh block marks the cache
  stale, so neither a new member nor a removed one waits a minute
- getBlockedRecipients and checkSandboxRecipients take fresh as required
- Semaphore gains a give-up signal and replaces DeadlineSlots
- the cache holds at most 10,000 teams

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@stamphog
stamphog Bot dismissed their stale review November 6, 2026 19:43

New commits were pushed — dismissing the stamphog approval from an earlier head. Re-add the trigger label to request a fresh review.

@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 e8aa17a: 1 Should fix.

Silthus and others added 15 commits November 6, 2026 20:21
…box-split/member-cache

# Conflicts:
#	nodejs/src/cdp/services/managers/organization-members.service.ts
A cached block ran its own uncached lookup and then marked the team
stale, so a workflow mailing non-members cost two primary lookups per
send that no concurrent send shared. A block from a snapshot older than
5 seconds now reloads it through the cache instead, so concurrent and
later sends reuse that one reload.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A cached block older than 5 seconds marked its team stale before it
reloaded. When that reload failed, the entry stayed stale, so every
later send for the team waited on the primary and failed while it was
down. The block now reloads without marking the entry first, so a failed
reload fails only that send and the old snapshot keeps serving the rest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A reload joined a load already running for the key, so it could return a
value read before the caller's write. It now joins only a buffered load
that has not started, and an earlier load that finishes later no longer
overwrites the newer value.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A reload skipped a load already running, so every blocked send that came in
while a reload query ran started another one. A reload now joins a reload of
the same key that is already in flight, and otherwise starts a load after the
call, as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A reload that joined another reload's running query returned the value from
before its own call, so a member who verified or joined during that query was
blocked for the next snapshot window. Join a reload only while its query has
not started. Callers that arrive later share one queued reload that starts
when the running one settles, so a key still has at most two member queries
in flight.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A reload queued behind one that failed started another lookup, so under a
stalled primary each queued send waited for two lookup deadlines and added a
query to the struggling pool. Queued reloads now fail with the running one,
and the next reload after that loads again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
stamphog[bot]

This comment was marked as outdated.

Silthus and others added 2 commits November 9, 2026 04:01
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…box-split/member-cache

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@stamphog
stamphog Bot dismissed their stale review November 9, 2026 06:03

New commits were pushed — dismissing the stamphog approval from an earlier head. Re-add the trigger label to request a fresh review.

@veria-ai

veria-ai Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Reviews

@Silthus
Silthus marked this pull request as draft November 9, 2026 10:02
@Silthus

Silthus commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Note

🤖 Claude Opus 5.5 responding on behalf of Michael

Replaced by #131731 in the isolated sandbox rebuild.

@Silthus Silthus closed this Nov 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant