Repository navigation
feat(cdp): send transactional email files by reference - #131944
trunk-io[bot] merged 107 commits into
Conversation
The code-send route takes `attachments` as references to files uploaded through the upload route. A team off the attachments allowlist is refused before its idempotency key is reserved. After the existing checks, cdp-api verifies each file under the caller's team, keeps it for 7 days, and estimates the message from the file sizes and the bodies: the send's content, or the workflow's email step rendered the way the worker renders it. Refusals release the key. The queued run carries each file's metadata and no bytes, and the 202 answers the file count and bytes. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The send estimate takes the renamed header constant, and acceptSend sits beside chargeDailyUploadBytes. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The send check reads the system clock like the upload route now does, and the layer's code comments move here: - Each refusal after the reservation releases the key, because nothing was queued and the caller's retry with the same key must be able to send. - The bodies counted are the send's own content, or else its email step rendered the way the worker renders it, from the same run. A step that fails to render at accept fails in the worker too, so it counts as empty rather than refusing a send that would fail anyway. - The queued send carries each file's metadata; the worker loads the bytes from the attachment store. - Bodies are read only after every file is verified, because rendering can cost more than the checks that refuse first. - The upload and send routes share one allowlist, so a team uses files on both or on neither. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A send with files and no content whose email step fails to build at accept counts as having no body, answers 202 with its file totals and is queued. The accept-time render has no person or groups, so refusing on a render error could block sends the worker would deliver; a rare oversized message is refused by SES and recorded as failed instead. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@PostHog review |
|
@Silthus PostHog Review is reviewing this pull request. |
🤖 CI report✅ Trunk lane — non-backend laneThis 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) — cleanNew 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) — cleanNew 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. |
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 941f437: 2 Should fix.
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.
Accept counted the bodies at their raw size and the html without the preheader, and the worker checked only the files, so a send could get a 202 and then be refused by SES. The estimate now counts each body at its base64 transfer size, one function for both sides. Accept counts the html with the preheader the worker adds. The worker estimates the final message, with the bodies after the person render, the tracking rewrite and the preheader, and fails the step as message_too_large before the SES request is signed. Accept refuses what it can see; a send that grows past the limit at render fails at the worker and never reaches SES. A replay with the same key now answers the totals of the first 202. They are stored with the key; a key recorded by an older pod has none and replays as a send without files, which is the only kind older pods queue. The render-failure test drives a stored template with an unclosed liquid tag through the route instead of a spy, and the size tables split into accepted and refused tests with their sizes derived from the estimate. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@PostHog review |
|
@Silthus PostHog Review is reviewing this pull request. |
… gives up Accept now refuses an attachment filename over 255 characters and a content_type or content_id outside 1 to 78 characters, the limits of the SES v2 Attachment type. The refusal is the usual invalid_request 400 and names the field and its limit, so no such send gets a 202 and then fails at SES after its key is marked queued. Only a template that fails to render or parse counts as no body when a send with files is sized. Any other failure while loading the email step, such as the template lookup, answers 503 delivery_unavailable with the outage warning and releases the key. Checking the files of a send and sizing its message now share one deadline, half of the 10 s the Django send facade waits for cdp-api. Past it the send answers 503 delivery_unavailable, releases the key and queues nothing, so Django never reports a send unavailable that cdp-api then queues. The refusal table that switched on a 503 status is split into a 400 table and a 503 table that always checks the outage warning. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SES refuses attachments with any of 90 file extensions, and a send with one of them got a 202 and then failed at SES after its key was marked queued. Accept now compares the last extension of each filename, ignoring case, with the list on the SES page on unsupported attachment types (dg/attachments.html#mime-types), and refuses a match with the existing invalid_request 400. The reason names the file and the type, and the key stays free. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
email.service.test.ts: the missing-file test and the inline-reload table stay exactly as layer 5 has them, including the check that the other download is aborted. This layer's preheader case is its own test, which sends nothing and fails as message_too_large, so no table switches on a reason. attachment-uploads.ts: layer 5 renamed settleBefore to settleBeforeAbortEvenWhileCredentialsStall with the same behavior, so the deadline on verifying a send's files uses the new name. The worker's final-size check still runs after the files load and before the SES command is built, so a too-large message is never tracked as an attempt SES may have accepted. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@PostHog review |
|
@Silthus PostHog Review is reviewing this pull request. |
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 08e2006: 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.
|
@PostHog review |
|
@PostHog review |
The send route answers 503 once file verification passes its deadline, but the store kept checking and keeping files, and the route went on to render the email step. A retry then overlapped that abandoned work. The deadline now reaches every stage: each HEAD and COPY aborts on its own timeout or the deadline, no COPY starts after it, and verification stops before rendering the bodies and before the size estimate. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@PostHog review |
…end-references Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…end-references Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…end-references Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
New commits were pushed — dismissing the stamphog approval from an earlier head. Re-add the trigger label to request a fresh review.
…below Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 6be27df: 1 Must fix. This pull request is large, so the review ran in 4 parts with less depth than usual.
This is review round 18 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.
A send with files now refuses an email step whose single {% liquid %}
block holds more statements than the tag limit allows, the same way it
refuses that many separate tags.
Refs PostHog/michaelr-context#77
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tag count now reads the decoded template, so a block opened with an encoded space counts its statements, and it keeps counting every tag after a block opener, so a quoted opener no longer hides the tags after it. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 65c95f5: 1 Should fix. This pull request is large, so the review ran in 4 parts with less depth than usual.
This is review round 19 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.
LiquidJS treats U+180E as a blank before a tag name, so the tag count now does too, and a block opened after one counts its statements. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…end-references Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…end-references Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/trunk merge |
|
😎 Stack merged successfully - details. |
Deploy status
This table shows the |
Merge gate: passed.
fleet proof gatepasses on 6be27df (live run, prod shape, cross-version, shadow diff, review at HEAD, CI green, judge fail accepted by standing rule). Layer 4 of 4 in the node-store attachment stack: #133482, #131833, #133495, then this PR. The stack merges from its top PR; #131567 is live in both regions and the env settings (PostHog/michaelr-context#76) are merged.Problem
Note
🟡 Merge risk: medium · two-way door
field_not_acceptedwhile the allowlist stays empty. Sends without files queue the same state as before.attachment_countandattachment_byteson its idempotency record, which expires after 24 hours; an older pod ignores both on replay.CDP_EMAIL_ATTACHMENTS_TEAMS; fix forward.Changes
attachmentsas references (sha256,filename, optionalcontent_typeandcontent_id) to files the team uploaded.field_not_acceptedbefore its idempotency key is reserved.workflow_shape_not_supported; an email step the frequency cap can hold, or a workflow under a sending rate limit, gets 400workflow_can_wait. Both name the step and the fix.invalid_requestnaming the field: a filename over 255 characters, acontent_typeorcontent_idoutside 1 to 78, or a file type on the SES unsupported list.attachment_not_uploadedwith itssha256.message_too_largewith the byte totals. The email worker checks the same estimate before SES.utm_value_too_long. One with more than 10,000 Liquid tags across its templated fields gets 400email_body_too_many_tags. Both are refused before anything parses the value, because LiquidJS parse time grows with the square of the tag count. Sends without files are not checked.email_render_limit_exceeded. A store outage, or a failed person or template lookup, answers 503delivery_unavailable.attachment_countandattachment_bytes.How it works
--- title: Upload, then send --- sequenceDiagram participant C as caller participant A as cdp-api participant B as bucket participant W as email worker rect rgba(9, 105, 218, 0.15) Note over C,B: upload #35;131833 C->>A: sends the sha256 and size of each file A->>B: HEADs team/<id>/<sha256> A-->>C: answers present, or a PUT URL signed for that size and hash C->>B: PUTs the bytes end rect rgba(26, 127, 55, 0.15) Note over C,B: send #35;131944 C->>A: sends, naming each file by its sha256 A->>B: HEADs each file, copies any with under 7 days left A-->>C: answers 202 and queues a job without bytes end W->>B: GETs each file by sha256, at most 6 days latercdp-api.tsand onAttachmentUploads.acceptForSend, in the send span of the flow above.attachmentsfield, so a send that named files got 400invalid_request.sha256. cdp-api HEADs each file, sizes the message, and queues a job that holds file metadata and no bytes.acceptForSendcopies any file with under 7 days left, so a file's expiry only moves later.How did you test this code?
content_typeandcontent_id: sends with files deliver with their files intact, an inline image keeps itscid:reference andContent-ID, and each refusal answers synchronously through Django's facade with nothing queued, a rate-limited workflow included.attachments: []now accepted.attachments_missingbefore SES, so no email goes out without its files. An older cdp-api refusesattachmentswith 400invalid_request, even on a retry of a key this head stored, so the allowlist stays empty until every pod runs this head. Idempotency keys replay across versions for sends without files.Release status
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5
Skills:
/writing-tests,/writing-code-comments,/writing-pr-descriptions,/stacking-prsemail_render_limit_exceeded. Any other render error counts as no body and queues; the worker fails it the same waymessage_too_largename a file?Involvement: 4 cross-model review rounds and 24 final reads (GPT-6.1 Sol, Opus fresh eyes), one full qa-swarm plus the stack-tip pass, PostHog Review on each head; live run, end-to-end, shadow-diff, cross-version and production-shape proofs captured. CodeRabbit is retired and skipped.
More context for AI agents
messageBodyBytesincdp-api.ts,files-workflow-shape.ts,hogflow-person.ts, andacceptSendinattachment-uploads.ts.🤖 Generated with Claude Code