Preserve Prebid ad units across GPT refreshes

[ChristianPavilonis]

Summary

• Preserve request-scoped Prebid bidder params, client bids, and banner zones so later GPT refreshes can reconstruct complete ad units even when pbjs.adUnits is empty.
• Suppress duplicate auctions caused by synchronous or shortly deferred, targeting-correlated GPT delivery refreshes while keeping unrelated and expired refreshes on the independent auction path.
• Stacked on Add Prebid error diagnostics #893 (add-prebid-error-diagnostics).

Changes

File	Change
crates/trusted-server-js/lib/src/integrations/prebid/index.ts	Capture isolated publisher ad-unit snapshots, protect them from synthetic refresh units, materialize refresh units from exact-code snapshots, and correlate delivery refreshes through publisher GPT targeting with a bounded one-shot context.
crates/trusted-server-js/lib/test/integrations/prebid/index.test.ts	Add regressions for request-scoped units, nested snapshot isolation, malformed bids, synthetic recursion, mixed GAM-only delivery lists, delayed targeted refreshes, context expiry, nested callbacks, and independent refreshes.

Closes

Closes #911

Test plan

• cargo test-fastly && cargo test-axum
• cargo clippy-fastly && cargo clippy-axum
• cargo fmt --all -- --check
• JS tests: cd crates/trusted-server-js/lib && npx vitest run (421 passed; 74 focused Prebid tests)
• JS format: cd crates/trusted-server-js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve
• Other: npm 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

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses tracing macros (not println!) — no logging was added
• New code has tests
• No secrets or credentials committed

[aram356]

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 artifacts fails because ERROR_TYPE_HTTP_STATUS is unused at crates/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]

Missing issue

[aram356]

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): capturePublisherAdUnitSnapshot returns a truthy snapshot for any unit with a non-empty code, and serverSideBidderParamsForRefresh/clientSideBidsForRefresh return early on presence, never reaching the findRefreshAdUnit fallback. 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, with pbjs.adUnits still holding the folded params in both. See the inline comment on serverSideBidderParamsForRefresh (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 survive pbjs.removeAdUnit() and keep reaching /auction forever, 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 stale hb_adid/hb_cache_path and a persisting ts_initial=1 (index.ts:643).
• Required integration CI is failing: prepare integration artifacts fails 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_MS is 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-less setTargetingForGPTAsync() — the canonical Prebid call — retains via the undefined branch.

  Two deterministic signals are already reachable from this file:

  1. hb_adid correlation: after setTargetingForGPTAsync, the GPT slot carries hb_adid for the winning bid. slot.getTargeting is already on RefreshGptSlot and already used at index.ts:1029. Recording the pending auction's winner and treating a slot as delivery iff its hb_adid matches an unconsumed pending adId is time-free and naturally per-slot.
  2. auctionId: the chained handler at index.ts:857 takes ...args and discards them; Prebid passes (bidResponses, timedOut, auctionId). Keying contexts by auctionId gives 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 bidsBackHandler frame, so a publisher setting targeting in a setTimeout/.then() misses it. The test at index.test.ts:1912 currently asserts the resulting duplicate auction as intended behavior (index.ts:889).
• setTargetingForGPTAsync(null, matcher) never retains: only undefined is special-cased, but null is 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 hasOnlyValidExplicitSlots guard short-circuits before consumption (index.ts:1005).

⛏ nitpick

• Orphaned and stale docs: inserting findRefreshSnapshot detached the pbjs.adUnits doc comment from findRefreshAdUnit, and the docblocks at index.ts:496-498 and :532-537 still describe pbjs.adUnits as 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

[prk-Jr]

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 on pbjs.removeAdUnit, so a second installPrebidNpm() call double-wraps and fires removePublisherState twice 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 + publisherDeliverySlots solve a genuinely tricky reentrancy problem (nested bidsBackHandlers, 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-fastly via 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 lint and npm run format clean)
• GitHub Integration Tests: PASS (per gh pr checks)
• Note: PR currently shows mergeStateStatus: CONFLICTING against main — needs a rebase/merge resolution before merge regardless of review outcome.

[prk-Jr]

🤔 thinking — installPrebidNpm() reads whatever is currently on pbjs.removeAdUnit as originalRemoveAdUnit and wraps it. If installPrebidNpm() ever runs twice (re-init), it wraps the already-wrapped function, so removePublisherState fires twice per real removeAdUnit call. Harmless today (deletes are no-ops on missing keys), but worth a one-line guard if double-init becomes a real scenario:

const prebidWithRemoveAdUnit = pbjs as unknown as { removeAdUnit?: RemoveAdUnit; __tsRemoveAdUnitWrapped?: boolean };
if (!prebidWithRemoveAdUnit.__tsRemoveAdUnitWrapped) {
  const originalRemoveAdUnit = prebidWithRemoveAdUnit.removeAdUnit;
  if (typeof originalRemoveAdUnit === 'function') {
    prebidWithRemoveAdUnit.removeAdUnit = function (adUnitCode?: string | string[]) {
      const result = originalRemoveAdUnit.call(this, adUnitCode);
      removePublisherState(adUnitCode);
      return result;
    };
    prebidWithRemoveAdUnit.__tsRemoveAdUnitWrapped = true;
  }
}