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
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Solid, well-tested change: request-scoped Prebid ad-unit snapshots (bounded LRU) plus GPT-delivery correlation (bounded pending-bid map) fix duplicate auctions on refresh while preserving publisher bidder params, client-side bids, and zone. Read the full diff (prebid.rs, index.ts, test file, publisher.rs, docs) and ran the full local suite in place of CI, which never triggered on this commit (see CI Status). No blocking issues found.
Non-blocking
🤔 thinking
- Double-wrap on repeated
installPrebidNpm()calls: wraps whatever is currently onpbjs.removeAdUnit, so a secondinstallPrebidNpm()call double-wraps and firesremovePublisherStatetwice per real call. Harmless today, but a one-line sentinel guard would future-proof it. (crates/trusted-server-js/lib/src/integrations/prebid/index.ts:724, see inline comment)
🌱 seedling
- CI never ran on this commit: GitHub's "Run Tests" and "Run Format" workflows never triggered for head commit
fed84393— only "Integration Tests" shows in the check-runs API. Worth a maintainer look at why (independent of this PR's code). I ran the equivalent locally in its place — see CI Status below.
👍 praise
- Snapshot/LRU/delivery-correlation design (
crates/trusted-server-js/lib/src/integrations/prebid/index.ts):capturePublisherAdUnitSnapshot+findRefreshSnapshot+publisherDeliverySlotssolve a genuinely tricky reentrancy problem (nestedbidsBackHandlers, synthetic vs. publisher auctions, live-vs-snapshot precedence) cleanly, with bounded LRU maps (256 snapshots / 2048 pending bids) guarding memory. 82 dedicated tests cover nested callbacks, throwing callbacks, timer races, and malformed bid shapes. - Prebid error-diagnostics security posture (
crates/trusted-server-core/src/integrations/prebid.rs, stacked from #893): bounded, allowlisted-field JSON extraction, HTML-page rejection, control-character normalization, and debug-gated exposure of upstream error text — well tested including oversized and streaming body edge cases.
CI Status
- fmt: PASS (
cargo fmt --all -- --check, local) - clippy: PASS (
clippy-fastly,clippy-axum,clippy-cloudflare,clippy-cloudflare-wasm,clippy-spin-native,clippy-spin-wasm, local) - rust tests: PASS (
cargo test-axum: 32 passed;cargo test-fastlyvia Viceroy: 1639 core + 21 openrtb passed, 0 failed — local, since GitHub's Run Tests workflow did not trigger on this commit) - js tests: PASS (
npx vitest run: 429 passed;npm run lintandnpm run formatclean) - GitHub Integration Tests: PASS (per
gh pr checks) - Note: PR currently shows
mergeStateStatus: CONFLICTINGagainstmain— needs a rebase/merge resolution before merge regardless of review outcome.
aram356
left a comment
There was a problem hiding this comment.
Summary
All nine findings from the previous review are resolved, and the delivery-correlation mechanism was rewritten rather than patched: the timer wrapper is gone, live pbjs.adUnits is now authoritative with snapshots as fallback, snapshots are a bounded LRU tied to a removeAdUnit wrapper, and refreshes partition per slot. CI is green and JS tests pass 429/429 locally.
The rewrite relocates the risk rather than removing it. Two blocking issues: publisher auctions without a bidsBackHandler get no correlation at all and are misclassified into duplicate auctions, and the new per-slot partition splits one publisher refresh() into two GAM requests, which breaks Single Request Architecture on pages this codebase itself puts into SRA mode.
Blocking
🔧 wrench
- Auctions without a
bidsBackHandlerregister no pending bids: thetypeof originalBidsBack !== 'function'guard sits aboveregisterPendingPublisherBids, so a callback-dispatch concern gates state registration. The Prebid 8+ promise form and theonEvent('auctionEnd')pattern both pass no handler. Probed in isolation on this branch: without a handler the delivery refresh produced 1 synthetic auction, 0 passthrough refreshes, and cleared all 6TS_REFRESH_TARGETING_KEYS; with a handler it correctly produced 0 synthetic auctions and 1 passthrough. One-line reorder fixes it (crates/trusted-server-js/lib/src/integrations/prebid/index.ts:905). - The refresh split breaks GPT Single Request Architecture: one publisher
refresh()now yields twooriginalRefreshcalls up to 1500ms apart. This is not hypothetical —src/integrations/gpt/index.ts:585andcrates/trusted-server-core/src/integrations/gpt_bootstrap.js:137both callenableSingleRequest(). Under SRA one refresh is one ad request, so the split loses competitive-exclusion enforcement (labels are enforced within a single request), breaks roadblocks spanning the boundary, and breaks companion pairing. Frequency capping is not affected. A single combined deferred refresh fixes it, at the cost of up to 1500ms on the publisher's already-won render — a tradeoff worth making explicitly rather than silently (index.ts:1017,:1068).
❓ question
- Correlator double-bump: the same
optsreference is passed to both split refresh calls, andchangeCorrelatordefaults totrue, so the common case records two GAM page views for one publisher refresh — compounding the SRA finding. Entirely untested; everyrefreshassertion in the suite passesundefinedoptions. Was this considered? (index.ts:1068)
Non-blocking
🤔 thinking
- A no-bid slot splits the refresh:
groupByPlacementomits ad units that received no bids, so a mixed bid/no-bidrefresh([A,B])tears into two GAM requests and clears targeting on the no-bid slot (index.ts:621). hb_adidis the sole correlation key: publishers usingtargetingControls.allowTargetingKeyswithoutAD_ID, or applying their own targeting, are permanently and silently misclassified. Falling back to ad-unit-code matching would makehb_adidan optimization rather than the only signal (index.ts:646).- Pending bids have no TTL: a first impression rendered via
googletag.display()never consumes its entry, so the next viewability refresh is suppressed once before self-healing. Removing the timer was right; a bound on data staleness is a separate concern (index.ts:603). - New partial-refresh failure mode: if the synthetic
requestBidsthrows or its handler never fires, delivery slots have already refreshed and independent slots never do. The previous all-or-nothing design failed cleanly (index.ts:1064).
⛏ nitpick
removeAdUnitwrapper stacks across repeatedinstallPrebidNpm()calls. Verified benign — the base implementation still runs once per call andremovePublisherStateis idempotent — and it mirrors the pre-existingrequestBidspattern. A__tsRemoveAdUnitWrappedsentinel would match the__tsRefreshWrappedidiom (index.ts:723).- Dropped
hasOnlyValidExplicitSlotsguard:refresh([slotA, null])now silently drops thenullinstead of taking the untouched passthrough. Very low impact, but the removal looks incidental (index.ts:999).
CI Status
- prepare integration artifacts: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- JS tests: PASS (429/429 locally)
bd3ffe7 to
e45b6b5
Compare
aram356
left a comment
There was a problem hiding this comment.
Summary
This revision resolves every finding from the prior review — the SRA split is replaced with a single combined deferred refresh, the bidsBackHandler guard is reordered (and opts is now copied), pendingPublisherCodes adds requested-code fallback matching for no-bid and custom-targeting auctions, a 5s TTL bounds pending state, the removeAdUnit wrapper is guarded with a sentinel, and the invalid-slot passthrough is restored. CI is fully green.
One new correctness bug remains in the fallback-timer path: because the fallback and the Prebid auction share the same timeoutMs delay and targeting is only applied inside the auction callback, a slow-but-valid auction has its hb_* targeting silently dropped on refresh.
Blocking
🔧 wrench
- Fallback timer preempts a slow-but-valid auction callback, dropping its targeting: the independent slots are cleared up front,
setTargetingForGPTAsyncruns only insidebidsBackHandler(gated behindcompleted), and the fallbacksetTimeout(completeRefresh, timeoutMs)uses the same delay as the auction. On the timeout path (auction slower thantimeoutMs), Prebid'stimeoutBuffer(~400ms) makes the fallback win deterministically, so the independent slots refresh to GAM with no header-bidding targeting. The existing test attest/integrations/prebid/index.test.ts:2476codifies this data-loss as intended. Fix by applying targeting insidecompleteRefresh(or, minimally,timeoutMs + buffer) and reframing that test. See the inline comment (crates/trusted-server-js/lib/src/integrations/prebid/index.ts:1197).
Non-blocking
🤔 thinking
- Code-fallback suppresses a custom-targeting publisher's first independent refresh once: a publisher who renders the initial impression via
googletag.display()and never callssetTargetingForGPTAsynchas their first viewability/scroll refresh within the 5s TTL misclassified as delivery. Bounded (one-shot consume + TTL) and inherent to correlating withouthb_adid; worth a test that names the scenario (index.ts:734).
♻️ refactor
prunePendingPublisherBidsdoes redundant O(n²) work: the first loop'sremovePendingPublisherBidsForCodefull-scans the bid map for each expired code, but the second loop already reaps those bids byexpiresAt. Replace with a barependingPublisherCodes.delete(index.ts:644).
📝 note
- The two pending maps evict independently at the 2048 cap: a bid can be dropped without its code, causing a redundant extra auction (never a wrongful suppression) past 2048 concurrent pending bids. Low severity; optional test (
index.ts:668).
CI Status
- cargo fmt / clippy / test (all adapters + parity + CLI): PASS
- vitest: PASS (429/429 locally, 99 prebid)
- format-typescript / format-docs: PASS
- integration tests / browser integration tests / Fastly EC lifecycle: PASS
- CodeQL / Analyze: PASS
| // Prebid schedules its own timeout during requestBids(). Schedule this | ||
| // fallback afterward so its normal timeout callback gets first chance | ||
| // to apply targeting before the one-shot GPT completion path runs. | ||
| if (!completed) fallbackTimer = setTimeout(completeRefresh, timeoutMs); |
There was a problem hiding this comment.
🔧 wrench — The fallback timer and the Prebid auction use the same timeoutMs delay, and because setTargetingForGPTAsync lives only inside bidsBackHandler (gated behind if (completed) return), a fallback that wins the race refreshes the independent slots to GAM with their hb_* targeting cleared and never reapplied — silently dropping the auction result those slots just computed.
Trace:
- :1121
independentSlots.forEach(clearRefreshTargeting)stripsTS_REFRESH_TARGETING_KEYSup front. - :1172-1177
completeRefreshcallsoriginalRefresh(slots, opts)but does not apply targeting. - :1182-1191
setTargetingForGPTAsync(refreshAdUnitCodes)runs only insidebidsBackHandler, which starts withif (completed) return(:1183). - :1192 auction
timeout: timeoutMs; :1197fallbackTimer = setTimeout(completeRefresh, timeoutMs)— identical delay.
This is not a rare tie. On the timeout path (auction slower than timeoutMs), Prebid adds a timeoutBuffer (~400ms by default) before it invokes bidsBackHandler, so the bare setTimeout(…, timeoutMs) fallback fires first by a wide margin and deterministically wins — the auction's own timeout callback (which would have applied targeting for whatever bids arrived) never runs. The happy path where /auction returns before timeoutMs is unaffected. (The code-level facts — same delay, targeting gated behind completed, slots pre-cleared — are traced here; the ~400ms buffer is Prebid runtime behavior, not from this repo.)
The comment at :1194-1196 assumes FIFO scheduling gives Prebid "first chance," but FIFO only guarantees Prebid's timer macrotask runs first — not that bidsBackHandler is reached synchronously within it. Real Prebid does post-timeout work (timing out non-responding bidders, emitting AUCTION_END, response processing) before calling back.
The existing test at test/integrations/prebid/index.test.ts:2476 ("ignores a synthetic callback that arrives after the fallback refresh") advances the fallback, then fires the callback, and asserts setTargetingForGPTAsync was not called (:2498) — it codifies exactly this data-loss as intended behavior.
Fix — apply targeting inside completeRefresh before originalRefresh, so it lands regardless of which timer wins:
function completeRefresh(): void {
if (completed) return;
completed = true;
if (fallbackTimer !== undefined) clearTimeout(fallbackTimer);
try {
pbjs.setTargetingForGPTAsync?.(refreshAdUnitCodes);
} catch (error) {
log.error('[tsjs-prebid] refresh targeting failed', error);
}
originalRefresh(slots, opts);
}The bidsBackHandler then no longer needs its own setTargetingForGPTAsync call. A minimal alternative is setTimeout(completeRefresh, timeoutMs + buffer) with buffer ≥ Prebid's ~400ms, so a legitimately-timing-out auction wins — but folding targeting into completeRefresh closes the window entirely. Either way, test 2476 must be reframed: its current premise asserts the bug. Applying the fix will also require updating the no-callback test at :2452 (advance by timeoutMs + buffer if you take the buffer route).
| .map((adId) => pendingPublisherBids.get(adId)) | ||
| .find((bid): bid is PendingPublisherBid => bid !== undefined) | ||
| : undefined; | ||
| const hasAdId = |
There was a problem hiding this comment.
🤔 thinking — The requested-code fallback only evaluates when the slot has no hb_adid (hasAdId false), which means a publisher who applies custom targeting and never calls setTargetingForGPTAsync cannot be distinguished from one delivering a just-won bid.
Scenario: such a publisher runs an auction, renders the first impression via googletag.display() (no refresh()), then a genuine viewability/scroll refresh of the same slot fires within the 5s TTL. The code fallback matches the still-pending code and classifies that legitimate independent refresh as delivery, suppressing the TS auction for it.
This is bounded and self-healing: one-shot consume (deliveredCodes.forEach(removePendingPublisherBidsForCode) at :749) plus the 5s TTL mean it happens at most once per auction, and the next refresh runs the auction normally. It's also inherent — without hb_adid there's no signal to tell the two apart — so I read it as an acceptable trade rather than a defect. Worth a test that names the "legitimate independent refresh suppressed once, then runs" case so the boundedness is a guarded invariant rather than incidental, and a line in the docstring noting the fallback cannot distinguish these for no-targeting publishers.
| /** Discard delivery state that outlived the publisher auction which created it. */ | ||
| function prunePendingPublisherBids(now = Date.now()): void { | ||
| for (const [adUnitCode, pendingCode] of pendingPublisherCodes) { | ||
| if (pendingCode.expiresAt <= now) removePendingPublisherBidsForCode(adUnitCode); |
There was a problem hiding this comment.
♻️ refactor — The first loop calls removePendingPublisherBidsForCode(adUnitCode) for each expired code, and that helper does a full scan of pendingPublisherBids (:631-638). But an expired code's bids share its expiresAt, so they're already removed by the second loop (:647-649). The per-code bid scan is therefore redundant, making prune O(expiredCodes × bids) — worst case ~2048² — on every refresh and every auction.
for (const [adUnitCode, pendingCode] of pendingPublisherCodes) {
if (pendingCode.expiresAt <= now) pendingPublisherCodes.delete(adUnitCode);
}Dropping to a bare delete makes prune O(codes + bids); the second loop still reaps the bids.
| pendingPublisherBids.delete(adId); | ||
| pendingPublisherBids.set(adId, pendingBid); | ||
|
|
||
| if (pendingPublisherBids.size > MAX_PENDING_PUBLISHER_BIDS) { |
There was a problem hiding this comment.
📝 note — storePendingPublisherCode (:657-660) evicts the oldest code via removePendingPublisherBidsForCode, which cascades to its bids, but storePendingPublisherBid here evicts a single bid via a bare pendingPublisherBids.delete(oldestAdId) without touching its code. So past the 2048 cap the two maps can drift: a code can survive with its bid evicted.
Consequence: if a slot then reports an hb_adid whose adId was evicted, hasAdId is true so the code fallback is skipped, the bid lookup misses, and the slot runs a redundant TS auction instead of a passthrough delivery. This is always an extra auction, never a wrongful suppression, and only reachable past 2048 concurrent pending bids — and everything is pruned within 5s regardless. Flagging as low-severity/known rather than asking for a change; an optional test around the cap would keep it a documented behavior.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Preserves publisher Prebid ad-unit config across GPT refreshes via request-scoped snapshots, and suppresses duplicate auctions by correlating GPT delivery refreshes to the pending publisher auction. Traced the delivery-correlation state machine, the one-shot completion latch, and the snapshot lifecycle — no blocking defects found. All CI checks pass; the Prebid suite (94 tests) also passes locally. Approving with non-blocking suggestions.
Reviewed diff is JS-only against origin/main (crates/trusted-server-js/lib/src/integrations/prebid/index.ts, its test file, and one added assertion in ad_init.test.ts).
What I verified
- Delivery correlation is one-shot.
publisherDeliverySlotsconsumes pending state only on a match; the early-return paths (adInit bypass, emptytargetSlots, filtered-length mismatch) leave state intact. - Reentrancy is safe.
registerPendingPublisherBidsruns before the publisher'sbidsBackHandler, so a refresh fired from inside that handler observes pending state; the throw path rolls back scoped toregistrationId. - No double GAM request. The
completedlatch plus clearedfallbackTimercovers late Prebid callbacks, throwing targeting, and a synchronousrequestBidsthrow. Scheduling the fallback afterrequestBidscorrectly lets Prebid's own timeout callback win the ordering. - SRA batching preserved. Switching
completeRefreshtooriginalRefresh(slots, opts)keeps the publisher's original list and options in one GAM request. - State rebinding.
installPrebidNpmreassigns the module maps while theremoveAdUnitandpubads.refreshwrappers install once; both read the live bindings, so no stale-map capture across reinstallation. copyParamValueis cycle-safe and passes non-plain prototypes through by reference; publisher mutation-after-auction isolation is covered by tests.
Non-blocking
♻️ refactor
- Codes map bounded by the bids constant:
storePendingPublisherCodecapspendingPublisherCodeswithMAX_PENDING_PUBLISHER_BIDS(index.ts:657). Two maps sharing one constant reads as a copy/paste; addMAX_PENDING_PUBLISHER_CODESor rename to a sharedMAX_PENDING_PUBLISHER_ENTRIES. - File size:
index.tsis now 1328 lines. The delivery-correlation state machine (roughlyindex.ts:626-767) is self-contained and would extract cleanly into a sibling module — follow-up, not this PR.
🤔 thinking
PENDING_PUBLISHER_DELIVERY_TTL_MS = 5000is ungrounded (index.ts:53). This constant is the entire heuristic: a delivery refresh deferred beyond 5s produces a duplicate auction, and a publisher refresh cadence under 5s costs one suppressed auction. Worth deriving it from the configured bidder timeout /installRefreshHandler(timeoutMs), making it injectable throughwindow.__tsjs_prebid, or recording the rationale in a follow-up issue.- Snapshot capture is unguarded on the publisher critical path (
index.ts:906-912).copyParamValuerecurses without a depth cap, so a pathological params tree throws aRangeErrorout ofpbjs.requestBidsand takes down the publisher's whole auction rather than just the snapshot. Suggest wrapping thecapturePublisherAdUnitSnapshotcall in try/catch with alog.warn. - adInit interaction with the stale-
hb_adidrule: TS-owned slots carryhb_adidapplied server-side by adInit. On a publisher delivery refresh that ID is non-empty and unmatched, so the slot falls through to the independent path — the same duplicate-auction class this PR targets. ThepublisherDeliverySlotsdocstring justifies preferring a fresh auction over stale-ID suppression, but naming the adInit case there would save the next reader the trace.
⛏ nitpick
prunePendingPublisherBids(now = Date.now())(index.ts:642): thenowparameter is never supplied by any caller and the function is module-private. Drop it.foldedBidderParams(index.ts:481) callsObject.entrieson an unvalidatedparams.bidderParams; a string value yields index-keyed garbage params. A plain-object guard would make it total.
📝 note
- The new guard
slots !== undefined && targetSlots.length !== slots.length(index.ts:1111) bypasses the auction for the entire call when any entry is a non-object, where the previous code auctioned the valid subset. Conservative and tested — worth calling out in the PR description as an intentional behavior change.
👍 praise
- The test suite is the strongest part of this PR: 94 tests covering LRU eviction, nested-context reentrancy, microtask-deferred refreshes, TTL expiry, late-callback suppression, throwing targeting, and the
removeAdUnitlifecycle. Uncommon rigor for browser glue code. const opts = { ...(requestObj ?? {}) }is a real latent fix — the shim no longer mutates a publisher's reused request options object — and it comes with a regression test.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS (also re-run locally: 94/94 in the Prebid suite)
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