Name the guard that stops an APS creative render, and record it by aram356 · Pull Request #1300 · IABTechLab/trusted-server

aram356 · 2026-10-12T17:10:24Z

Why

A blank APS creative render is indistinguishable from a delivered one, and nothing on the render path says why. An APS bid that wins Prebid targeting is served by Ad Manager as a 1x1 universal creative that resizes itself only after the creative draws, so Ad Manager reports a non-empty 1x1 render either way and the tester framework reports "filled". Every guard on the render path returned silently: a frame that rejected the descriptor, one that never received it, and one that timed out all looked identical.

This replaces #1052, which is being closed rather than revived. That branch had not moved since 2026-09-01, carried five unaddressed CHANGES_REQUESTED reviews, conflicts with current main, and was anchored to the closed #1019. Its security shape was sound and is carried forward here; the six defects that stopped it are fixed or deferred explicitly.

Companion issue: #1299.

What changed

The sandboxed renderer document (aps.rs) reports which guard stopped it, on the existing failure message: bad_hash, nonce_mismatch, descriptor_keys, descriptor_fields, descriptor_envelope, amazon_script_error.

The Universal Creative source (render.ts) labels its own frame_timeout and frame_load_error and relays whichever reason it holds to the top window.

The GPT bridge (gpt/index.ts) consumes that relay and records the reason against the ad unit the capability was served to.

The diagnostics store and overlay learn the new categories, so a reason is recorded and displayed rather than discarded. This is the part #1052 was missing, and the reason nothing it emitted ever reached ts_console.

Three deliberate differences from #1052

One source of truth for the failure vocabulary. #1052 widened the type union in types.ts while the store's runtime allowlist and the overlay's switch stayed as they were, so all fifteen new reasons were dropped before being recorded. Here the union is derived from a single const array and the store validates against that same array, so a category that exists at compile time is recordable at runtime by construction. The overlay keeps its exhaustive switch with no default, which is what makes a new category a compile error there instead of an undefined fact.

The source check stays first. #1052 moved it below the shape check so a non-parent sender could be told source_mismatch. That reason is dropped here and the ordering is left alone: a sender that is not the parent is rejected before any reporting, so it cannot consume the frame's single report. The source_mismatch category does not exist.

The relay is source-checked. #1052 accepted a relayed failure with no check on e.source, so any frame with a top handle and a live ad ID could forge one, including the security-signal categories. The tombstone now carries the ad unit it was served to, and the relay requires the sender to be a frame inside it. The ad ID is not secret from the ad-serving chain, since it is published as hb_adid targeting.

The direct-path regression, fixed

Adding reason makes the failure message three keys. renderApsCreative's listener exact-matched two, so it dropped every failure and the hidden frame survived over publisher content for the full 10s timeout instead of failing fast. That listener now accepts a reason-bearing failure, including the nonce-less bad_hash report a frame sends before it has learned a nonce, while still rejecting a foreign nonce and still requiring an exact key set on the ready message. #1052 shipped this regression untested; it has four tests here.

Notes for review

The renderer bundle version moves 4 to 5 because the Universal Creative source changed, and the one test that hardcoded 4 now reads the constant.

The Prebid shim imports two unrelated helpers from aps/render.ts, and a module-scope Object.freeze on the reason table defeated tree-shaking and pushed the shim from 40787 to 41225 bytes, past the 41000-byte guard in prebid-artifact-integration.test.mjs. The table is built on first use instead, so the shim stays at its previous size and the guard is unchanged. The guard's comment asks for growth to be re-justified rather than raised, so it was not raised.

Security posture, since a reason crosses an origin boundary:

Reasons are fixed categories. A rejected descriptor is never echoed back.
Reporting from the frame is one-shot and answers through parent, never the sender.
Relayed reasons resolve through a null-prototype allowlist, so a hostile __proto__, constructor, or toString resolves to undefined.
The relay is recording only and never influences delivery.

Deferred, tracked in #1299

This does not attempt the attempt-lifecycle redesign, so a relayed failure is not yet attributed to the render it belongs to in every case:

recordTrustedServerCreativeResponse is recorded when the descriptor is posted, which closes the attempt before the frame can report. A narrower store API, or a post-completion failure state, is needed.
The slot association the APS path needs plants a request intent that belongs to the slot's next cycle, which mis-attributes it.

Both are described in #1299 with the measurements behind them. They need design review rather than a port, which is why they are not here.

Testing

Gates run locally, all green:

cargo fmt --all -- --check
cargo clippy-fastly
cargo test -p trusted-server-core (2912 passed)
cd crates/trusted-server-js/lib && npx vitest run (1225 passed, 50 files, no type errors)
cd crates/trusted-server-js/lib && npm run format

tsc --noEmit over src/ was compared against the merge base: the error set is byte-identical, so this branch introduces no new type errors. (The repo has 352 pre-existing ones and no CI gate for them.)

New coverage:

render.test.ts: direct-path teardown on a reason-bearing failure, on nonce-less bad_hash, rejection of a foreign nonce, rejection of a ready message with an extra key; the allowlist including inherited-key and non-string rejection; every mapped reason resolving to a category the store accepts; UC relay of a frame reason, frame timeout, and frame load error
store.test.ts: every declared category is recordable, iterating the array so a future addition cannot silently become unrecordable
ad_init.test.ts: a relayed failure recorded against a real GptDiagnosticsStore rather than a mock recorder, plus drops for a foreign sender and for unlisted reasons

The two tests guarding the #1052 defects were checked against the broken versions and fail there: the store test fails with expected undefined to deeply equal [ 'aps_bad_hash' ] against the old hand-written allowlist, and the bridge test fails with expected undefined to deeply equal [ 'aps_frame_timeout' ] with the relay recording disabled.

An APS bid that wins Prebid targeting is served by Ad Manager as a 1x1 universal creative that resizes itself only after the creative draws. Every guard on that render path returned silently, so a slot that never drew was indistinguishable from one that did: Ad Manager reports a non-empty 1x1 render either way, and the tester framework reports "filled". Name the guard that stopped the render instead. The sandboxed renderer document now reports bad_hash, nonce_mismatch, descriptor_keys, descriptor_fields, descriptor_envelope, and amazon_script_error on the existing failure message. Reporting is one-shot and answers through the parent, never the sender. The source check stays first, so a window that is not the parent is dropped before any reporting happens and cannot consume the frame's single report. The Universal Creative source labels its own frame_timeout and frame_load_error and relays whichever reason it holds to the top window. That relay crosses an origin boundary, so reasons resolve through a null-prototype allowlist that drops anything unlisted and leaves a hostile __proto__ or constructor as undefined. The table is built on first use so it does not become a module-level side effect that defeats tree-shaking and grows the Prebid shim, which imports two unrelated helpers from this module. The failure message now carries a third key, which the direct render path's listener rejected: it exact-matched two keys, so the frame survived hidden over publisher content until the 10s timeout rather than failing fast. That listener now accepts a reason-bearing failure, including the nonce-less bad_hash report a frame sends before it has learned a nonce. Reasons are fixed categories. A descriptor is never echoed back.

The reasons the render path now emits had nowhere to land. The creative failure union in types.ts is the compile-time contract, but the runtime contract was a hand-written allowlist in the diagnostics store, and the overlay rendered facts from a separate exhaustive switch. Three lists, kept in agreement by hand, and a reason missing from the store's list was dropped silently. Derive the union from one const array and validate against the same array, so a category that exists at compile time is recordable at runtime by construction. The overlay keeps its exhaustive switch with no default, which is what makes a new category a compile error there rather than an undefined fact. The bridge consumes the relay the creative frame posts to the top window. Every field is untrusted: the reason resolves through the allowlist, and the sender must be a frame inside the ad unit the capability was served to, so a frame that merely knows a live ad ID cannot forge a failure. The ad ID is not secret from the ad-serving chain, since it is published as hb_adid targeting. The consumed-ad-ID tombstone carries the ad unit it was served to for exactly that check. Recording only; the branch never answers the sender and never influences delivery. Attribution of the relayed failure to the render it belongs to still depends on the attempt lifecycle work tracked separately: a response recorded when the descriptor is posted closes the attempt before the frame reports, and the slot association API plants a request intent that belongs to the next cycle. Tests drive a real store rather than a mock recorder, so a reason that the store would reject fails the test. Both new store and bridge tests were checked against the pre-fix allowlist and the unwired relay, and fail there.

The Universal Creative source changed, so its version constant moved 4 to 5, and the browser spec asserted the literal 4 against the Prebid Response payload. Bumping the constant is the normal consequence of touching that source, so a literal here turns every such change into a failure that looks unrelated to the change that caused it. Read the constant from render.ts instead, the way the spec already reads the built bundles from that crate.

#1052 was closed in favor of #1300, which reimplements the APS render failure reasons against current main. Removing #1052's merge first keeps none of its superseded code beside #1300.

#1300 replaces #1052, reverted from this RC in the previous commit. Integration decisions: - gpt_diagnostics/store.ts keeps the trace imports beside #1300's creative failure list import. - #1300 adds eight aps_* creative failure reasons that #1107's trace projection does not accept, so any cycle carrying one failed trace capture. The projection now validates against the full diagnostics list and drops the APS reasons; unknown values are still rejected. The viewer bundle is unchanged; the pinned trace source digest moves.

prk-Jr

Summary

Labelling the guard that stopped an APS render is the right fix, and the security shape of the relay (source check first, null-prototype allowlist, fixed categories, one-shot reporting) is sound. However, the relay records nothing in a realistic configuration: the Prebid path keys the store by the Prebid ad unit code while associations are keyed by the Trusted Server slot id, and the server-side APS path is never consulted. Both were reproduced against a real GptDiagnosticsStore using this suite's own homepage_header / div-header fixture.

3 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied. Each suggestion was applied alone and as a batch in a scratch worktree: prettier, the full vitest suite (1225 passed, including the Prebid shim size guard), and the core integrations::aps tests all pass.

Blocking

🔧 wrench

Prebid-path relay records under the Prebid ad unit code, not the Trusted Server slot id — see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:2007
Server-side APS render failures are never relayed — see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:2005

Non-blocking

🤔 thinking / ♻️ refactor / 📝 note / ⛏ nitpick

🤔 nonce_mismatch can never be recorded (suggestion) — see inline at crates/trusted-server-core/src/integrations/aps.rs:126
🤔 Nonce-less failure acceptance makes reasons forgeable from inside the renderer frame (suggestions) — see inline at crates/trusted-server-js/lib/src/integrations/aps/render.ts:767 and :812
♻️ No test executes the renderer document's reasons — see inline at crates/trusted-server-core/src/integrations/aps.rs:3327
📝 aps_frame_load_error is effectively unreachable — see inline at crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:201
⛏ "Adding a member here is sufficient" overstates — see inline at crates/trusted-server-js/lib/src/core/types.ts:189

👍 praise

Single source of truth for the failure vocabulary — see inline at crates/trusted-server-js/lib/src/core/types.ts:196

Cross-cutting / body-level findings

♻️ Operator docs still list four failure categories — the "Creative-bridge failures" table in docs/guide/integrations/gpt-diagnostics.md (around line 225) documents only missing_render_source, cache_fetch_failed, invalid_cache_payload, and response_post_failed. This PR adds ten aps_* categories that now appear in the overlay; each needs a row describing where it is observed (renderer document vs. Universal Creative source) and, ideally, a sentence that APS reasons are relayed diagnostics, not bridge failures.
🌱 The direct render path discards the reason — renderApsCreative (reached from core/request.ts:59, the client /auction path) now accepts reason-bearing failures but ignores reason, and fail() logs APS renderer: frame load failed for every cause, including the 10s timeout. Logging the allowlisted reason (via apsRenderFailureReason) would make that path debuggable at no cost; recording it can follow the attempt-lifecycle work in #1299.
📝 Repeated relays are bounded — checked while reviewing: a frame inside the ad unit can post the relay repeatedly, but recordTrustedServerCreativeRequest returns the existing live attempt for the cycle and failures are deduplicated, so this cannot flood the store. No action needed.

CI Status

browser integration tests: PASS
integration tests: PASS
integration tests (Fastly EC lifecycle): PASS
CodeQL: PASS
cargo test (ts CLI, native) (macos-latest): PASS
cargo test (axum native): PASS
CLAUDE.md symlink guard: PASS
Analyze (actions): PASS
Analyze (python): PASS
cargo check/build/test (spin native + wasm32-wasip1): PASS
Analyze (javascript-typescript): PASS
format-docs: PASS (required)
format-typescript: PASS (required)
cargo check (cloudflare native + wasm32-unknown-unknown): PASS
cargo test: PASS (required)
cargo fmt: PASS (required)
cargo test (cross-adapter parity): PASS
cargo test (ts CLI, native) (ubuntu-latest): PASS
prepare integration artifacts: PASS
vitest: PASS
Analyze (rust): PASS

dhruv8sh

Summary

Giving each APS render guard a name, and having one array define the failure categories, are both the right moves. The renderer-document and direct-path changes look correct. However, the relay that is meant to carry a reason to ts_console cannot record anything in a real browser yet. Three independent gaps each drop the reason, and this review adds one that the unit tests cannot see: the message is posted from PUC's hidden renderer frame, so it fails the bridge's source check.

None of the inline comments below carry a one-click suggestion. The fix touches the Universal Creative source and its tests together, so it is described in prose.

Blocking

🔧 wrench

Relay is posted from PUC's hidden renderer frame, so the source check always fails — see inline at crates/trusted-server-js/lib/src/integrations/aps/render.ts:808

Cross-cutting / body-level findings

🔧 I agree with the two open wrenches from the earlier review, and they add to this one. The Prebid-path relay records under the Prebid ad unit code instead of the Trusted Server slot id (gpt/index.ts:2007). The server-side APS path is never consulted (gpt/index.ts:2005). I confirmed both against GptDiagnosticsStore.recordTrustedServerCreativeRequest, which looks up trustedServerSlots by slot id. To get a single reason recorded end to end, all three have to be fixed. The new ad_init.test.ts case records a relayed APS render failure against a real diagnostics store passes only because it uses a synthetic source and associates the slot under 'div-header'. Neither happens in production.
🌱 The tombstone can expire before the frame times out. The relay looks up consumedPrebidApsIds, and each tombstone expires at the renderer entry's expiresAt. A capability consumed with less than 10s of TTL left will lose the tombstone before frame_timeout fires, and the relay will be dropped. That is rare with the default 300s TTL. Once the relay works, keeping the tombstone until at least now + RENDERER_READY_TIMEOUT_MS would close the gap. It also keeps the ad ID blocked longer, which is the safer direction.

CI Status

browser integration tests: PASS
integration tests: PASS
integration tests (Fastly EC lifecycle): PASS
CodeQL: PASS
cargo test (ts CLI, native) (macos-latest): PASS
cargo test (axum native): PASS
CLAUDE.md symlink guard: PASS
Analyze (actions): PASS
Analyze (python): PASS
cargo check/build/test (spin native + wasm32-wasip1): PASS
Analyze (javascript-typescript): PASS
format-docs: PASS (required)
format-typescript: PASS (required)
cargo check (cloudflare native + wasm32-unknown-unknown): PASS
cargo test: PASS (required)
cargo fmt: PASS (required)
cargo test (cross-adapter parity): PASS
cargo test (ts CLI, native) (ubuntu-latest): PASS
prepare integration artifacts: PASS
vitest: PASS
Analyze (rust): PASS

ChristianPavilonis

Approved with non-blocking comments on the APS failure-reporting path:

Send the relay from the creative window so source authentication accepts it, rather than rejecting PUC’s dynamic-renderer iframe.
Recognize server-side APS winners as well as Prebid winners, preserving slot-scoped ownership.
Resolve the Trusted Server slot ID before recording failures when it differs from the Prebid div code.
Report nonce mismatches using the frame’s expected nonce so receivers retain nonce_mismatch rather than falling back to frame_timeout.

Review found that the relay recorded nothing in a real browser. Four independent gaps each dropped the reason, and the new bridge test passed only because it used keys and a sender that production never produces. The relay was posted from the wrong realm. Universal Creative runs the renderer source inside a hidden frame it creates in the creative document, so a postMessage call made from that source carries the hidden frame as event.source whatever window it targets: the source is the realm of the running function, not the window the method is called on. The top-window listener authenticates a sender by matching it against iframes in the publisher document, which the hidden frame is not, so every relay was dropped. Reports now go through Universal Creative's own sendMessage helper, which is a creative-realm function, so a report arrives as the creative iframe the listener already recognizes. Verified in headless Chromium against a page mirroring Universal Creative's structure: the previous call reports the hidden renderer frame and matches no slot iframe, the helper reports the creative iframe and matches. The Prebid path recorded under the publisher's ad unit code, which is a div id, while the store keys associations by the configured Trusted Server slot id. Whenever they differ the lookup missed, the failure was dropped, and a creative_request_without_slot attribution issue was recorded in its place, so a render failure surfaced as a misleading attribution problem. The relay now resolves the slot id through divToSlotId. Server-side APS winners are served the same renderer and emit the same relay, but are tracked per slot rather than through the Prebid tombstone, so their relays were dropped. They are now resolved too, requiring the sender to sit in the slot that consumed the ad ID because hb_adid is not unique per slot. nonce_mismatch could never be recorded: the report echoed the mismatching nonce, which no parent accepts. It now echoes the nonce the frame expects, which only the parent receives, and the parent is the window that set it. Accepting any nonce-less failure let code running inside the renderer frame tear the frame down without knowing the nonce. bad_hash is the only report a frame sends before it has learned one, so nonce-less acceptance is narrowed to that reason in both parents. A tombstone could also expire before the frame's timeout fired, dropping the reason for a capability consumed late in its TTL. Tombstones now live at least as long as that timeout, which also keeps the ad ID blocked longer. The browser spec now asserts the reasons the renderer document actually emits, including the echoed nonce. That is the only coverage that executes the document rather than matching substrings in its Rust source, and it is where the nonce_mismatch bug would have been caught. The direct render path logs the reason instead of reporting every cause as a load failure. Operator docs describe the ten categories and where each is observed.

aram356 · 2026-10-15T01:37:15Z

Addressed at a90f2f8. All four blocking findings were reproduced before fixing, and every fix is pinned by a test that fails without it.

Realm (@dhruv8sh, @ChristianPavilonis). Confirmed, and the diagnosis was exactly right. Verified in headless Chromium against a page mirroring Universal Creative's structure, with the real crossDomain.js from the installed Prebid.js (v10.26.0, lines 82-99) as the reference for that structure. The previous call reports the hidden renderer frame as event.source and matches no slot iframe; the helper reports the creative iframe and matches. Two controls worth recording: w.top vs w.parent resolve to different windows yet both report the hidden frame, and Reflect.apply(w.top.postMessage, w.top, ...) also reports the hidden frame. So it is the realm of the running function, not the target or the receiver.

One correction to the suggested fix: sendMessage is (type, payload, responseListener), not a bare poster, and it force-stamps message and adId via Object.assign({message: type, adId}, payload). Since payload is spread last a caller can override both, but the relay does not need to: the helper's own adId is the one the bridge wants. It also JSON.stringifys the body, which the bridge already parses, and targets pubDomain rather than '*', which is strictly better. d.adId is no longer read.

Slot-id keying (@prk-Jr, @ChristianPavilonis). Confirmed against the real store with production's association: keying on the div id gives attemptId = undefined, no failure, and ['creative_request_without_slot', 'creative_attempt_unknown']. Resolved through divToSlotId. My test did mask this by associating under 'div-header'; it now associates under 'homepage_header' and maps the div to it, and fails if the resolution is removed.

Server-side path (@prk-Jr, @ChristianPavilonis). Confirmed: both branches serve the same renderer (gpt/index.ts:2075 and :2154) but only the Prebid tombstone was consulted. Now resolved via consumedServerApsBySlot, requiring the sender to sit in the slot that consumed the ad ID since hb_adid is not unique per slot. New test, mutation-proven.

nonce_mismatch unreachable (@prk-Jr, @ChristianPavilonis). Confirmed by inspection and fixed as suggested: the report echoes expected. Only parent receives it, and parent set the fragment, so nothing new is exposed.

Nonce-less forgery (@prk-Jr). Narrowed to reason === 'bad_hash' in both parents, as suggested.

Tombstone expiry (@dhruv8sh 🌱). Fixed rather than deferred: tombstones now live at least APS_NATIVE_RENDERER_TIMEOUT_MS, which also keeps the ad ID blocked longer.

Renderer-document coverage (@prk-Jr ♻️). Added to aps-renderer.spec.ts, asserting reason and nonce for the mismatch-slot, missing-fragment-slot, and malformed-slot cases. This is the only coverage that executes the document rather than matching substrings, and it is where the nonce_mismatch bug would have surfaced.

Also done: operator docs now have an APS table covering all ten categories and where each is observed, with a note that an HTTP error on the renderer route surfaces as aps_frame_timeout rather than aps_frame_load_error (@prk-Jr 📝) and a softer overlay label to match; the direct path logs the allowlisted reason instead of reporting every cause as a load failure (@prk-Jr 🌱); the types.ts comment no longer overstates what adding a member achieves (@prk-Jr ⛏).

Local gates: cargo fmt, clippy-fastly, integrations::aps (38 passed), vitest (1227 passed, 50 files, no type errors), npm run format for both lib and docs. The 11 settings::tests failures in my local cargo test are present at the unmodified branch HEAD too, so they are environmental and not from this change.

Attempt-lifecycle work remains deferred to #1299 and is unchanged by this round.

dhruv8sh

Summary

The follow-up commit closes every blocking finding from the earlier reviews. The relay now goes out through Universal Creative's sendMessage helper, so it arrives from the creative iframe. Failures are recorded under the Trusted Server slot id, and server-side APS winners are relayed too. nonce_mismatch is now reachable, nonce-less acceptance is narrowed to bad_hash, the browser spec executes the renderer document, and the docs list all ten categories. What remains is non-blocking.

2 of the inline comments below carry a one-click GitHub suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comment is a note with no action required.

Non-blocking

🤔 thinking / ⛏ nitpick / 📝 note

🤔 The tombstone minimum has no margin, so a late frame_timeout relay is still dropped (suggestion). See inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1951.
⛏ The Rust test's doc comment overstates the JS-side coverage (suggestion). See inline at crates/trusted-server-core/src/integrations/aps.rs:3328.
📝 aps_unknown is effectively unreachable, and an unrecognised wire reason is dropped silently. See inline at crates/trusted-server-js/lib/src/integrations/aps/render.ts:834.

Cross-cutting / body-level findings

♻️ No real-browser test covers the main fix (which window the relay comes from). Routing the relay through sendMessage only matters in a real browser, because the sending window is determined by where the posting code runs. The vitest relay tests use a pucHelper stand-in that posts from the top window. The new browser-spec assertion covers the renderer document's own renderer-failed messages, not the relay. The commit message describes a manual headless-Chromium check, but nothing automated would catch a return to w.top.postMessage. The real-PUC test in crates/trusted-server-integration-tests/browser/tests/shared/aps-renderer.spec.ts (around line 228) already loads PUC_BANNER and is the natural place to lock it in. Apply manually: the test is outside this PR's diff hunks.
```
// In "renders through real PUC and expands only its authenticated 1x1 shell",
// make the renderer document report a failure (e.g. serve it a descriptor with
// a bad envelope), then assert the top window received the relay from the
// creative iframe itself:
const relay = await page.evaluate(() => (window as unknown as {
  apsRelays: Array<{ fromCreative: boolean; data: Record<string, unknown> }>;
}).apsRelays.find(({ data }) => data.message === 'trusted-server/aps/render-failed'));
expect(relay).toEqual({ fromCreative: true, data: expect.objectContaining({ reason: 'descriptor_envelope' }) });
```
👍 Thorough follow-up. The fix commit addresses every earlier finding and explains each one. The new browser-spec assertion executes the real renderer document and checks the echoed nonce. That is the coverage that would have caught the nonce_mismatch bug. The relay also keeps per-slot sender checks on both the Prebid and server-side paths.

CI Status

browser integration tests: PASS
integration tests: PASS
integration tests (Fastly EC lifecycle): PASS
CodeQL: PASS
cargo test (ts CLI, native) (macos-latest): PASS
CLAUDE.md symlink guard: PASS
format-docs: PASS (required)
cargo check (cloudflare native + wasm32-unknown-unknown): PASS
Analyze (javascript-typescript): PASS
cargo fmt: PASS (required)
cargo test (axum native): PASS
Analyze (actions): PASS
format-typescript: PASS (required)
Analyze (rust): PASS
cargo test: PASS (required)
vitest: PASS
cargo check/build/test (spin native + wasm32-wasip1): PASS
cargo test (ts CLI, native) (ubuntu-latest): PASS
cargo test (cross-adapter parity): PASS
prepare integration artifacts: PASS
Analyze (python): PASS

dhruv8sh · 2026-10-16T08:07:17Z

+  // A capability consumed with little TTL left would otherwise lose its
+  // tombstone before the renderer frame's timeout fires, dropping the relayed
+  // reason. Holding it for at least that long also keeps the ad ID blocked
+  // longer, which is the safer direction for a security tombstone.
+  const minimum = Date.now() + APS_NATIVE_RENDERER_TIMEOUT_MS;


🤔 thinking — The tombstone minimum has no margin, so a late frame_timeout relay is still dropped.

recordConsumedPrebidApsId holds the tombstone until Date.now() + APS_NATIVE_RENDERER_TIMEOUT_MS, measured from when the capability is consumed. The Universal Creative source starts its own 10s frame_timeout timer only after the response round trip, so the timeout relay always arrives slightly after consumption + 10s. By then pruneConsumedPrebidApsIds (expiresAt <= now) has deleted the tombstone and the reason is dropped. That is the late-TTL case this comment says it closes. Smaller point: the bound uses APS_NATIVE_RENDERER_TIMEOUT_MS, while the frame's timer uses the module-private RENDERER_READY_TIMEOUT_MS. Both are 10_000 today but could drift apart.

Reproduced with a scratch test in ad_init.test.ts using a real GptDiagnosticsStore, an entry expiring at t0 + 1s, and consumption at t0. A relay at t0 + 10_050 recorded nothing (expected undefined to deeply equal ['aps_frame_timeout']), while the same relay at t0 + 9_000 was recorded. With the suggestion applied, the t0 + 10_050 relay is recorded.

Suggested change

// A capability consumed with little TTL left would otherwise lose its

// tombstone before the renderer frame's timeout fires, dropping the relayed

// reason. Holding it for at least that long also keeps the ad ID blocked

// longer, which is the safer direction for a security tombstone.

const minimum = Date.now() + APS_NATIVE_RENDERER_TIMEOUT_MS;

// A capability consumed with little TTL left would otherwise lose its

// tombstone before the renderer frame's timeout relay arrives, dropping the

// reason. The frame starts its timer only after the response round trip, so

// hold the tombstone for twice that timeout rather than exactly it. This also

// keeps the ad ID blocked longer, which is the safer direction for a

// security tombstone.

const minimum = Date.now() + 2 * APS_NATIVE_RENDERER_TIMEOUT_MS;

(Verified alone in a scratch worktree: npx vitest run, npm run format, and node build-all.mjs pass. The repro test is not part of the suggestion; add a regression test alongside if you want it covered.)

dhruv8sh · 2026-10-16T08:07:17Z

+    /// Reachability of each reason is covered on the JavaScript side, where the
+    /// document is evaluated rather than read as text. The assertions kept here


⛏ nitpick — "Reachability of each reason is covered on the JavaScript side" overstates the coverage. The browser spec executes the document only for bad_hash, nonce_mismatch, and descriptor_keys. descriptor_fields, descriptor_envelope, and amazon_script_error are not executed anywhere.

Suggested change

/// Reachability of each reason is covered on the JavaScript side, where the

/// document is evaluated rather than read as text. The assertions kept here

/// The browser spec executes the document and asserts `bad_hash`,

/// `nonce_mismatch`, and `descriptor_keys`; the other reasons are not yet

/// executed anywhere. The assertions kept here

dhruv8sh · 2026-10-16T08:07:17Z

-if(m.message==="${RENDERER_READY_MESSAGE}"){done=true;clean();resolve();}
-else if(m.message==="${RENDERER_FAILED_MESSAGE}")fail();}
+function report(x){try{if(h&&typeof h.sendMessage==="function")h.sendMessage("${APS_RENDER_FAILED_MESSAGE}",{reason:x});}catch(_e){}}
+function fail(x){if(done)return;done=true;clean();f.remove();report(x||"unknown");reject(new Error("APS renderer frame failed"));}


📝 note — aps_unknown is effectively unreachable, and an unrecognised wire reason is dropped silently.

fail(x) falls back to "unknown" only when x is falsy. Every caller passes a non-empty literal, or m.reason when it is a string, so aps_unknown happens only for an empty-string reason. The docs row (docs/guide/integrations/gpt-diagnostics.md:261) says "The render failed with no reason available". Also, when the renderer document and the JS bundle are on different versions and the frame sends a reason string the top-window allowlist doesn't know, the relay is dropped. It never becomes aps_frame_reported_failure. No action required; the docs wording could match, e.g.:

| `aps_unknown` | Universal Creative source | The frame reported an empty reason. Unrecognised reason strings are dropped rather than recorded. |

aram356 added 2 commits October 12, 2026 10:09

aram356 mentioned this pull request Oct 12, 2026

A blank APS creative render is indistinguishable from a delivered one #1299

Open

aram356 self-assigned this Oct 12, 2026

aram356 mentioned this pull request Oct 12, 2026

Name the guard that stops an APS creative render #1052

Closed

aram356 added this to the 202610 milestone Oct 12, 2026

aram356 linked an issue Oct 12, 2026 that may be closed by this pull request

A blank APS creative render is indistinguishable from a delivered one #1299

Open

Merge branch 'main' into feature/aps-render-failure-reasons

a102c12

aram356 requested review from ChristianPavilonis, dhruv8sh, jevansnyc and prk-Jr and removed request for jevansnyc October 12, 2026 17:41

aram356 added a commit that referenced this pull request Oct 12, 2026

Revert PR #1052 from rc/202610

549120c

#1052 was closed in favor of #1300, which reimplements the APS render failure reasons against current main. Removing #1052's merge first keeps none of its superseded code beside #1300.

aram356 mentioned this pull request Oct 12, 2026

October release candidate (DO NOT MERGE) #1228

Draft

Merge branch 'main' into feature/aps-render-failure-reasons

3590f32

prk-Jr requested changes Oct 13, 2026

View reviewed changes

dhruv8sh requested changes Oct 13, 2026

View reviewed changes

Comment thread crates/trusted-server-js/lib/src/integrations/aps/render.ts Outdated

Merge branch 'main' into feature/aps-render-failure-reasons

bdf63d3

ChristianPavilonis approved these changes Oct 13, 2026

View reviewed changes

aram356 added 2 commits October 14, 2026 15:37

Merge branch 'main' into feature/aps-render-failure-reasons

8e31ebe

aram356 added a commit that referenced this pull request Oct 15, 2026

Merge PR #1300 into rc/202610

9895c54

aram356 requested review from dhruv8sh and prk-Jr October 15, 2026 15:31

dhruv8sh approved these changes Oct 16, 2026

View reviewed changes

		/// Reachability of each reason is covered on the JavaScript side, where the
		/// document is evaluated rather than read as text. The assertions kept here

-    /// Reachability of each reason is covered on the JavaScript side, where the
-    /// document is evaluated rather than read as text. The assertions kept here
+    /// The browser spec executes the document and asserts `bad_hash`,
+    /// `nonce_mismatch`, and `descriptor_keys`; the other reasons are not yet
+    /// executed anywhere. The assertions kept here

Conversation

aram356 commented Oct 12, 2026

Why

What changed

Three deliberate differences from #1052

The direct-path regression, fixed

Notes for review

Deferred, tracked in #1299

Testing

Uh oh!

prk-Jr left a comment

Choose a reason for hiding this comment

Summary

Blocking

🔧 wrench

Non-blocking

🤔 thinking / ♻️ refactor / 📝 note / ⛏ nitpick

👍 praise

Cross-cutting / body-level findings

CI Status

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhruv8sh left a comment

Choose a reason for hiding this comment

Summary

Blocking

🔧 wrench

Cross-cutting / body-level findings

CI Status

Uh oh!

Uh oh!

ChristianPavilonis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aram356 commented Oct 15, 2026

Uh oh!

dhruv8sh left a comment

Choose a reason for hiding this comment

Summary

Non-blocking

🤔 thinking / ⛏ nitpick / 📝 note

Cross-cutting / body-level findings

CI Status

Uh oh!

dhruv8sh Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

dhruv8sh Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

dhruv8sh Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants