fix(analytics/prebid): accumulate all bidWon events before emitting witness

[dyachoksa]

Summary

Fixes the data-loss bug where only the first bidWon event per Prebid auction was captured on multi-ad-unit pages (issue #69).

Root cause: trackBidWon cleared the pending bidWinTimeout, emitted the witness immediately, and deleted the auction from the in-memory map. Every subsequent bidWon for the same auctionId hit the "Missing 'auctionEnd' event" guard and was silently dropped.

Fix: trackBidWon now only accumulates events; the bidWinTimeout timer in trackAuctionEnd is the sole emit point. After the timeout fires, all collected bidWon events are included in the witness payload as an array.

Changes

lib/addons/prebid/analytics.ts

• AuctionItem interface — added bidWonEvents: any[] accumulator field.
• trackBidWon — stripped to auction.bidWonEvents.push(event) + auction.missed = true propagation when called with missed=true. No more timeout clear, no early emit, no map deletion.
• trackAuctionEnd setTimeout callback — reads auction.bidWonEvents and auction.missed at fire time (so late-arriving missed=true propagation is respected), emits the witness with the full array, then removes the auction from the map.
• toWitness — second parameter changed from bidWonEvent: any | null to bidWonEvents: any[]; properties.bidWon is now always an array of { bidderCode, adUnitCode, cpm } objects (empty array when no bidWon arrived within the timeout window).

lib/addons/prebid/analytics.test.ts

• All toWitness(…, null) call sites updated to toWitness(…, []).
• trackBidWon describe block uses jest.useFakeTimers() / jest.runAllTimersAsync() to drive the deferred emit.
• setHooks "missed bidWon" test migrated to fake timers.
• New test: two trackBidWon calls for the same auction produce exactly one witness call with a two-element bidWon array.
• Added bidWonEvents: [] assertion to the trackAuctionEnd stored-auction test.

Wire format change

properties.bidWon changes from { message, bidderCode, adUnitCode, cpm } | null to Array<{ bidderCode, adUnitCode, cpm }>. The companion backend change (handling the array and assigning win=true per entry) should be deployed first, per the coordination note in the issue.

Test plan

• All 53 existing unit tests pass
• New multi-bidWon accumulation test passes
• Deploy backend change first (tracked in companion issue)
• Manual end-to-end: two-ad-unit page with Prebid debugging injecting wins for both ad units → one POST /witness with properties.bidWon.length === 2

[jrosendahl-opt]

lgtm

[eugenedorfman-optable]

Quick review:

Fix is correct: single emit point in trackAuctionEnd's timeout, trackBidWon reduced to accumulator, missed flag re-read at fire time. Resolves the multi-ad-unit data loss.

Issues

• bidWonAt semantics changed. Now ~auctionEnd + bidWinTimeout instead of actual win time. Fix: capture at push time — auction.bidWonEvents.push({ ...event, _receivedAt: new Date() }) and use earliest in payload.
• Stale jsdoc on trackBidWon (analytics.ts:377-382) and toWitness (analytics.ts:429-435) — describe old behavior.

Nits

• Eviction race (pre-existing): cleanupOldAuctions doesn't clearTimeout, leaks no-op timers.
• filteredEvent in trackBidWon (analytics.ts:385-392) is now log-only; simplify.
• Variable shadowing of auction inside the timeout closure at analytics.ts:359-367; rename to storedAuction.

Tests

Good: fake timers replace flaky sleep, new multi-bidWon test covers the bug.

Missing:

• Late trackBidWon(_, true) flips missed/optableLoaded on emitted payload.
• No-bidWon timeout case → bidWon: [], bidWonAt: null.
• Auction deleted from internal map after timeout.

Coordination

Wire format break (bidWon: {…}|null → array) - backend should be deployed first, as noted. No SDK consumer references properties.bidWon.

Verdict

Approve after doc refresh + bidWonAt capture-at-push fix.

[mosherBT]

10s timeout on this means that unless configured differently it will always wait 10s? I am worried we will lose impressions of people leaving the page in that time. I may be misunderstanding

[mosherBT]

Does this need to be any? can we define the type

[eugenedorfman-optable]

10s timeout on this means that unless configured differently it will always wait 10s? I am worried we will lose impressions of people leaving the page in that time. I may be misunderstanding

Hi @mosherBT your understanding is correct. A better alternative design would be to send a single auctionEnd event and then follow up with potentially several bidWon events sending each event as soon as it arrives. Then aggregate on the backend. That would be a timeout-less design, but would require somewhat significant changes on the backend. Currently we'd propose to reduce the default delay to 5s maybe to mitigate and allow some auction packets be lost on some sessions, then update the backend in a backward compatible manner.

[mosherBT]

Approving I think this is good to merge. The timeout I still think we should consider fixing. I wonder if there is prebid auction timeouts that we could leverage instead of a hard set default. Either way I think it is a fine step to have a timeout

[jrosendahl-opt]

we should add an onpageunload event that flushes the cache before the user navigates away.

[eugenedorfman-optable]

@sergeydyachok-optable let's look at John's suggestion tomorrow please as a separate PR - it'd be a good enhancement to the SDK