Preserve Prebid ad units across GPT refreshes#912
Conversation
aram356
left a comment
There was a problem hiding this comment.
Summary
The refresh snapshot and delivery-correlation behavior passes the local JavaScript checks, but the change introduces unbounded snapshot retention and the required integration CI gate is currently failing.
Blocking
🔧 wrench
- Required integration CI is failing:
prepare integration artifactsfails becauseERROR_TYPE_HTTP_STATUSis unused atcrates/trusted-server-core/src/auction/orchestrator.rs:103, and the downstream integration and browser jobs are consequently skipped. This error is inherited from the stacked base rather than introduced by either file in this diff, but the base needs to be fixed or updated and CI rerun successfully before merge. - Publisher snapshot cache grows without bound: see the inline comment on
crates/trusted-server-js/lib/src/integrations/prebid/index.ts.
CI Status
- JS tests: PASS (421/421; 74 focused Prebid tests)
- JS lint: PASS
- JS format: PASS
- JS bundle build: PASS
- prepare integration artifacts: FAIL
- downstream integration tests: SKIPPED
- browser integration tests: SKIPPED
aram356
left a comment
There was a problem hiding this comment.
Summary
The snapshot mechanism introduces a regression against the base branch: because the snapshot short-circuit keys on presence rather than content, an empty snapshot now beats live pbjs.adUnits config and silently drops the publisher's inline PBS demand from every refresh — the same class of failure #911 is meant to fix. Separately, the delivery-correlation heuristic is timing-based where a deterministic signal is available, and the unbounded snapshot cache from the previous review is still unaddressed.
Blocking
🔧 wrench
- Empty snapshot beats live config (regression):
capturePublisherAdUnitSnapshotreturns a truthy snapshot for any unit with a non-emptycode, andserverSideBidderParamsForRefresh/clientSideBidsForRefreshreturn early on presence, never reaching thefindRefreshAdUnitfallback. Re-registering an existing code as a fresh object with no bids yields{}where the base yields the publisher's params — verified by running the same probe against both branches, withpbjs.adUnitsstill holding the folded params in both. See the inline comment onserverSideBidderParamsForRefresh(crates/trusted-server-js/lib/src/integrations/prebid/index.ts:544). - Publisher snapshot cache still grows without bound: unchanged from the previous review — no eviction path other than
installPrebidNpm(). It additionally changes behavior: stale params now survivepbjs.removeAdUnit()and keep reaching/auctionforever, where the base returned{}(index.ts:771). - Mixed explicit refresh bypasses the entire slot list: any single covered slot attributes the whole call to delivery, so an uncovered TS-owned prebid slot in the same call loses both its auction and its
clearRefreshTargeting— GAM then re-requests it with stalehb_adid/hb_cache_pathand a persistingts_initial=1(index.ts:643). - Required integration CI is failing:
prepare integration artifactsfails and the downstream integration and browser jobs are consequently skipped. Inherited from the stacked base rather than introduced by either file in this diff, but the base needs to be fixed or updated and CI rerun successfully before merge.
❓ question
-
Why a 1000ms timer rather than correlating the actual auction?
PUBLISHER_DELIVERY_CONTEXT_TIMEOUT_MSis racy in both directions: a publisher whose refresh lands past 1s (lazy-load gate, consent await, loaded main thread on a low-end device, background-tab timer clamping) gets the duplicate auction the PR is meant to prevent, while any refresh of a context code arriving inside the window is bypassed on code membership alone — silently losing a legitimate auction and serving stale targeting. The second direction is the more damaging one and is untested; note the retain path is the common path, since an argument-lesssetTargetingForGPTAsync()— the canonical Prebid call — retains via theundefinedbranch.Two deterministic signals are already reachable from this file:
hb_adidcorrelation: aftersetTargetingForGPTAsync, the GPT slot carrieshb_adidfor the winning bid.slot.getTargetingis already onRefreshGptSlotand already used atindex.ts:1029. Recording the pending auction's winner and treating a slot as delivery iff itshb_adidmatches an unconsumed pending adId is time-free and naturally per-slot.auctionId: the chained handler atindex.ts:857takes...argsand discards them; Prebid passes(bidResponses, timedOut, auctionId). Keying contexts byauctionIdgives real identity instead of a timer.
Per-slot correlation would collapse the mixed-list bypass, the bare-refresh scope mismatch, both timer directions, and the duplicate-code leak into one rule. Was this considered and rejected?
Non-blocking
🤔 thinking
- Async targeting defeats the wrapper: the wrapper lives only for the synchronous
bidsBackHandlerframe, so a publisher setting targeting in asetTimeout/.then()misses it. The test atindex.test.ts:1912currently asserts the resulting duplicate auction as intended behavior (index.ts:889). setTargetingForGPTAsync(null, matcher)never retains: onlyundefinedis special-cased, butnullis a legitimate "all ad units" call (index.ts:598).- Bare refresh consumes one context but delivers all slots: uncovered TS slots serve stale targeting, and sibling contexts survive a refresh that just delivered their codes (
index.ts:606). - Duplicate-code context leak: consumption breaks at the newest covering context, leaving an older context to suppress a legitimate refresh (
index.ts:639). - A non-object slot entry both forces the duplicate auction and leaks the context: the
hasOnlyValidExplicitSlotsguard short-circuits before consumption (index.ts:1005).
⛏ nitpick
- Orphaned and stale docs: inserting
findRefreshSnapshotdetached thepbjs.adUnitsdoc comment fromfindRefreshAdUnit, and the docblocks atindex.ts:496-498and:532-537still describepbjs.adUnitsas the source when it is now the fallback. New helpers are undocumented in an otherwise heavily-documented file (index.ts:383).
CI Status
- JS tests: PASS (421/421 locally)
- JS lint: PASS
- JS format: PASS
- prepare integration artifacts: FAIL
- integration tests: SKIPPED
- browser integration tests: SKIPPED
4ca09fa to
7af7598
Compare
c59a064 to
2a2b39d
Compare
2a2b39d to
fed8439
Compare
Summary
pbjs.adUnitsis empty.add-prebid-error-diagnostics).Changes
crates/trusted-server-js/lib/src/integrations/prebid/index.tscrates/trusted-server-js/lib/test/integrations/prebid/index.test.tsCloses
Closes #911
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run(421 passed; 74 focused Prebid tests)cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servenpm run lint,node build-all.mjs, external Prebid bundle build, independent review, and natural proxy validation with the rebuilt bundle. A delayed explicit GPT delivery refresh containing four publisher slots plus a GAM-only interstitial produced no duplicate auction; a later independent header refresh retained Kargo/PubMatic params and zone.Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!) — no logging was added