Skip to content

feat(workflows): mirror the sandbox SES tenant pause state in Django - #129058

Closed
Silthus wants to merge 48 commits into
workflows-sandbox-split/email-step-pickerfrom
workflows-sandbox-split/tenant-state-mirror
Closed

Silthus wants to merge 48 commits into
workflows-sandbox-split/email-step-pickerfrom
workflows-sandbox-split/tenant-state-mirror

Conversation

@Silthus

@Silthus Silthus commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • Django does not know when SES pauses the shared sandbox tenant, so nothing can tell the workers to stop sandbox sends.
  • Layer 6 of the product stack, on #128951. Next: #129059, #129060, #129061, #129062. Split from #128411, which stays open until this split replaces it.
  • Merge order: the Node gate #129056 reads this table, so its CI fails until this PR and #129059 merge.

Merge danger

Warning

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

  • The SES tenant read that every own-sender tenant sync uses is refactored, with master's call order and error codes. A bug stores a wrong pause state and skips or allows a team's sends: silent harm, on a path meant to behave as before.
  • Migration 0043 adds one table with no foreign key to a hot table. It stays empty while SES_SANDBOX_TENANT_NAME is empty.
  • Revert: git revert the merge commit. The empty table can stay.

Changes

  • Django mirrors the sandbox tenant's sending status into SandboxSenderTenantState, from the SES tenant-state webhook and the daily sweep.
  • A changed state tells the workers through reload-sandbox-sender-state after commit. Project admins get no email.
  • An unknown or unconfigured sandbox tenant is never paused. Deleting the tenant in SES clears a stored pause and logs "Sandbox SES tenant not found".
  • A failed read keeps the stored state and counts in workflows_sandbox_tenant_sync_failures_total, under missing, busy, transient or error. It does not stop the team sweep.
  • Only NotFoundException reads as a missing sandbox tenant. A rejected read keeps a stored pause. The team tenant read keeps master's handling.
  • The state row commits before the lock transaction starts, so an overlapping first sync finds it. The row lock uses NOWAIT. A held lock counts as busy and its task retries. Any other database error, including a failed create of the row, counts as error.
  • The alert on that counter lives in the monitoring config, outside this repo. The doc says to ship it before the flag reaches projects beyond test projects.

How did you test this code?

Test rationale: test_ses_tenant_state.py and test_ses_tenant_events.py catch a pause that is lost, stored without an announcement, or kept after the tenant is gone. They also cover each failure label in the sweep, and a held row lock.

  • Live run at this head: Not run yet.
  • Not tested: NOWAIT under a real second lock holder, because that needs two committed transactions.
Earlier local run, at ad87342 (not saved as proof)

A local SESv2 stub answered the tenant reads, with eager Celery and a Redis subscriber on the reload channel. This run predates the missing label and the busy and error split.

Step SES answers Stored status Reload message
Webhook enabled ENABLED yes
Webhook paused DISABLED yes
Webhook, same pause paused DISABLED no
Sweep tenant not found empty yes
Webhook paused DISABLED yes
Sweep throttled, then 500, then 403 DISABLED, kept no; counter transient 2, error 1
Sweep enabled ENABLED yes
  • Mixed rollout, new Django with master's workers: master's PubSub saw no subscriber on reload-sandbox-sender-state and kept running. The worker side ships in #129056.
CI report findings
  • The PR's diff against master includes Python, so Trunk runs it in the backend lane.
  • Migration 0043 creates one new table, workflows_sandboxsendertenantstate, with no rows and no reader before this PR; the risk check rates it Safe.
  • Hogbox preview is an optional preview build, not a test of this change.
  • Playwright reports one flaky test outside this diff.

Release status

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

Docs update

  • docs/internal/workflows-email-sending-tiers.md describes the mirror, the failure labels and the alert prerequisite.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5

Skills: /django-migrations, /writing-tests, /writing-code-comments, /stacking-prs, /writing-pr-descriptions, /qa-swarm

Decisions for the whole sandbox sender stack
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

No open questions.

More context for AI agents
  • Session: Claude Code. Review rounds 1, 2 and final reads 1 to 4 ran on Claude Opus 5.5 only, because the GPT reviewers were unreachable for the whole session.
  • Decisions: the team tenant read is a refactor with master's call order, so the sandbox mirror reuses it through TenantSendingState.
  • Decisions: the sandbox read uses the fail-fast SES status client, because the caller holds a row lock while it reads.
  • Decisions: review added the missing label, limited busy to LockNotAvailable, and built the sweep's provider inside the counted try (ff05e1e, ce9dc8a). Final reads counted a failed create of the row (49b6f2d) and kept that create outside the lock transaction (f5a9b48).
  • Origin: split from #128411. The merged tops of the new stacks match the old stack's production code.
  • Read closely: sync_sandbox_tenant_state in ses_tenant_state.py, and providers/ses.py.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs 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.

⚠️ Playwright — 1 flaky

🎭 Playwright report · View test results →

⚠️ 1 flaky test:

  • Override/discard lifecycle from dashboard (chromium)

These issues are not necessarily caused by your changes.
Annoyed by this section? Help fix flakies and failures and it will go green!

⚠️ Django migration SQL — 1 new migration to review

We've detected new migrations on this PR. Review the SQL output for each migration:

products/workflows/backend/migrations/0043_sandbox_sender_tenant_state.py

/home/runner/work/tmp/tool_cache/Python/3.14.7/x64/lib/python3.14/site-packages/anyio/from_thread.py:119: SyntaxWarning: 'return' in a 'finally' block
  return result
2026-11-09T07:43:49.033904Z [error    ] Path must be a valid database or directory containing databases. [posthog.exceptions_capture] pid=6326 tid=140030233402240
Traceback (most recent call last):
  File "/home/runner/work/posthog/posthog/posthog/geoip.py", line 15, in <module>
    geoip: Optional[GeoIP2] = GeoIP2(cache=8)
                              ~~~~~~^^^^^^^^^
  File "/home/runner/work/tmp/tool_cache/Python/3.14.7/x64/lib/python3.14/site-packages/django/contrib/gis/geoip2.py", line 114, in __init__
    raise GeoIP2Exception(
        "Path must be a valid database or directory containing databases."
    )
django.contrib.gis.geoip2.GeoIP2Exception: Path must be a valid database or directory containing databases.
/home/runner/work/tmp/tool_cache/Python/3.14.7/x64/lib/python3.14/site-packages/sshtunnel.py:1040: SyntaxWarning: 'return' in a 'finally' block
  return (ssh_host,
System check identified some issues:

WARNINGS:
?: (axes.W001) You are using the django-axes cache handler for login attempt tracking. Your cache configuration is however invalid and will not work correctly with django-axes. This can leave security holes in your login systems as attempts are not tracked correctly. Reconfigure settings.AXES_CACHE and settings.CACHES per django-axes configuration documentation.
?: (staticfiles.W004) The directory '/home/runner/work/posthog/posthog/frontend/dist' in the STATICFILES_DIRS setting does not exist.
BEGIN;
--
-- Create model SandboxSenderTenantState
--
CREATE TABLE "workflows_sandboxsendertenantstate" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "tenant_name" varchar(64) NOT NULL UNIQUE, "sending_status" varchar(32) DEFAULT '' NOT NULL, "synced_at" timestamp with time zone NULL);
CREATE INDEX "workflows_sandboxsendertenantstate_tenant_name_d55f9c1c_like" ON "workflows_sandboxsendertenantstate" ("tenant_name" varchar_pattern_ops);
COMMIT;

Last updated: 2026-11-09 07:44 UTC (514b181)

✅ Django migration risk — migration analysis complete

We've analyzed your migrations for potential risks.

Summary: 1 Safe | 0 Needs Review | 0 Blocked

✅ Safe

Brief or no lock, backwards compatible

workflows.0043_sandbox_sender_tenant_state
  └─ #1 ✅ CreateModel
     Creating new table is safe
     model: SandboxSenderTenantState
  │
  └──> ℹ️  INFO:
       ℹ️  Skipped operations on newly created tables (empty tables
       don't cause lock contention).

Last updated: 2026-11-09 07:45 UTC (514b181)

❌ Hogbox preview — build failed

The preview didn't come up for commit 514b181. See the build log for the failing step. It'll retry on the next push.

Previews are optional and never block merging. A failure here is often a hogland or tailnet hiccup rather than anything in your PR, so the check stays green and this section is the status.

@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 58c9220: 1 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.

Silthus and others added 3 commits November 4, 2026 14:04
…x-split/tenant-state-mirror

# Conflicts:
#	products/workflows/backend/migrations/max_migration.txt
…x-split/tenant-state-mirror

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silthus and others added 8 commits November 4, 2026 19:10
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>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…x-split/tenant-state-mirror

Master took workflows migration 0042 (push token imports), so the tenant
state migration moves to 0043 and follows it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silthus and others added 3 commits November 6, 2026 18:39
…x-split/tenant-state-mirror

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…x-split/tenant-state-mirror

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

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 45f5f67: 1 Should fix.

This is review round 3 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.

Silthus and others added 2 commits November 6, 2026 20:07
…x-split/tenant-state-mirror

# Conflicts:
#	docs/internal/workflows-email-sending-tiers.md
…ox tenant sync

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

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not approved — this change needs a human reviewer.

Re-add the stamphog label to request another review once you have addressed this.

Owning-team authorship provides assurance for the risky workflow changes, but @PostHog's substantive concern about uncounted initial database failures remains unfixed. Address that comment in products/workflows/backend/services/ses_tenant_state.py and test the initial lookup failure before re-requesting.

  • @PostHog on products/workflows/backend/services/ses_tenant_state.py: the initial get_or_create at line 118 remains outside both counted exception handlers, so a database failure there does not increment the sync failure counter. The database-error test only mocks the subsequent lock query. Include the initial lookup in the counted error path and add a regression test before re-requesting.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 283L, 9F substantive, 556L/13F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1d-complex (556L, 13F, cross-cutting, feat)
stamphog 2.7.0 .stamphog/policy.yml @ 62c7c2c · reviewed head 62c7c2c

The sync created the state row before the counted lock handler, so a
database failure there raised without a sync failure metric. Run it inside
the same handler.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Creating the row inside the lock transaction left the insert uncommitted
for the whole SES read, so an overlapping first sync waited on it rather
than failing at once with NOWAIT. The row now commits first, and a failed
create is still counted.

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

This comment was marked as outdated.

…x-split/tenant-state-mirror

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@stamphog
stamphog Bot dismissed stale reviews from themself November 9, 2026 07:40

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 commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Note

🤖 Claude Opus 5.5 responding on behalf of Michael

Replaced by #131749 (model) and #131840 (poll and staff controls) in the isolated sandbox rebuild.

@Silthus Silthus closed this Nov 9, 2026

This branch had an error being deployed

1 failed deployment
hogbox-preview — 514b181a Deployed Nov 9, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant