New Module: Prebid DOOH creative approval - #4871
Conversation
…ached state on invalid or unavailable responses
| m.storeApprovalRefreshFallbacks(cfg, refreshes) | ||
| } | ||
| m.refreshes.finish(refreshes) | ||
| }() |
There was a problem hiding this comment.
If runApprovalRefresh panics and the recover() fires, it calls storeApprovalRefreshFallbacks. If that function also panics, the deferred anonymous function exits via a new unrecovered panic — skipping m.refreshes.finish(refreshes). The buffered-channel slot is never drained, the creative IDs are never removed from inFlight, and every subsequent claim() for those IDs treats them as already-in-flight. Once all slots fill, claim() returns capacityAvailable=false permanently — blocking all creative-approval refreshes for the lifetime of the process.
Fix: wrap storeApprovalRefreshFallbacks in its own recover, or ensure m.refreshes.finish(refreshes) runs in a separate deferred call that cannot be skipped by a secondary panic.
|
|
||
| type moduleConfig struct { | ||
| Enabled bool `json:"enabled,omitempty"` | ||
| Platforms []string `json:"platforms,omitempty"` |
There was a problem hiding this comment.
normalizePlatforms rejects any value other than "dooh" (returning a config error), but no hook handler reads cfg.Platforms at dispatch time. Platform detection happens solely via payload.Request.DOOH != nil. The intended per-platform filtering mechanism is entirely missing from the hook logic.
This means an operator who configures platforms: ["dooh"] believing the module will activate only for DOOH requests gets no such filtering — and the restrictive validation will reject any future platform extension (e.g. CTV) by config error rather than at runtime.
The platforms field should either be wired into the hook dispatch logic or removed from the config until it is implemented.
| base.Headers = overlay.Headers | ||
| } | ||
| if overlay.TimeoutMS != nil { | ||
| base.TimeoutMS = *overlay.TimeoutMS |
There was a problem hiding this comment.
When an account config JSON contains {"endpoint": ""}, applyAccountConfig writes base.Endpoint = "". HandleProcessedAuctionHook hits cfg.Endpoint == "" and emits the warning "DOOH creative approval endpoint is not configured" — text that reads as a global misconfiguration rather than a deliberate per-account override — then returns without setting module context. HandleAllProcessedBidResponsesHook finds isModuleContextActive false and exits early, passing every DOOH bid through with zero approval checking.
No metric, no clearly-labelled log, and no error distinguish this bypass from a missing global endpoint. An accidental empty string in account config silently kills enforcement with no audit trail.
|
|
||
| claimed, capacityAvailable := m.refreshes.claim(refreshes) | ||
| if !capacityAvailable { | ||
| return nil |
There was a problem hiding this comment.
When all goroutine slots are running, claim() returns (nil, false) and scheduleApprovalRefresh returns nil (no warnings appended). The hook sets each creative's status to approvalStatusPending, needsApprovalFilter sees non-approved statuses and triggers filterResponsesByApproval, which removes those bids. result.Warnings is empty so operators see bids disappear with no diagnostic signal.
Because no fallback is written to cache, every subsequent auction for the same creatives repeats the cycle until a slot eventually frees — causing sustained silent revenue loss under load.
Fix: append a warning to the hook result when creatives are dropped due to capacity, so operators can observe the condition in PBS logs.
|
|
||
| func (m *Module) storeApprovalRefreshFallbacks(cfg moduleConfig, refreshes []approvalRefresh) { | ||
| for _, refresh := range refreshes { | ||
| id := refresh.Creative.CreativeApprovalID |
There was a problem hiding this comment.
On provider error, storeApprovalRefreshFallbacks writes fallback status to cache with cfg.PendingTTLSeconds (default 60 s) unconditionally:
if err := m.cache.set(id, refresh.FallbackStatus, cfg.PendingTTLSeconds); err != nil {
When refresh.FallbackStatus == approvalStatusApproved, this overwrites a previously-approved creative's cache entry with a 60 s TTL instead of cfg.ApprovedTTLSeconds (default 3600 s). During any provider outage, the entire pool of previously-approved creatives expires every 60 seconds and triggers a retry wave — maximising load precisely when the provider is struggling to recover.
The correct helper already exists and is used on line 128 for valid responses:
ttlForStatus(cfg, status)
Fix: replace cfg.PendingTTLSeconds on this line with ttlForStatus(cfg, refresh.FallbackStatus).
| if !ok { | ||
| warnings = append(warnings, "bid skipped from approval lookup because it is missing creative id") | ||
| continue | ||
| } |
There was a problem hiding this comment.
When Bid.CrID == "", collectCreativeApprovals emits "bid skipped from approval lookup because it is missing creative id" and excludes the bid from the creatives map. approvalCandidateForBid then returns approvalCandidate{Exempt: false, CreativeApprovalID: ""}. Since statuses[""] is never set to approvalStatusApproved, filterResponsesByApproval removes the bid silently — no further warning, no entry in result.Warnings.
The word "skipped" is misleading: it implies the bid passes through, not that it will be removed. Operators cannot distinguish pending-lookup bids from CrID-missing bids in diagnostics.
Fix: treat CrID == "" as exempt (bypass the filter), or emit an explicit result.Warnings entry at the point of removal. Document the behaviour in the README.
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| responseBody, err := io.ReadAll(io.LimitReader(resp.Body, 1024*1024)) |
There was a problem hiding this comment.
On every non-2xx response (rate-limit 429, WAF 500, verbose stack trace), io.ReadAll(io.LimitReader(resp.Body, 1024*1024)) reads and heap-allocates up to 1 MB before the status-code guard triggers and discards the bytes. Under sustained provider errors every auction goroutine repeats this allocation.
Fix: check resp.StatusCode immediately after the response arrives (before reading the body), and drain/close the body only on the error path.
| } | ||
|
|
||
| statuses, warnings := m.resolveApprovalStatuses(cfg, miCtx.AccountID, payload.Responses) | ||
| result.Warnings = append(result.Warnings, warnings...) |
There was a problem hiding this comment.
HandleAllProcessedBidResponsesHook calls resolveApprovalStatuses (pass 1 via collectCreativeApprovals), needsApprovalFilter (pass 2), and filterResponsesByApproval inside the mutation closure (pass 3). Passes 2 and 3 each re-call approvalCandidateForBid for every bid, redoing isBidderExempt and creativeApprovalID (a SHA-256 hash) per bid.
A single-pass design — computing removal candidates during the statuses-building loop and using a non-empty candidates map as the needsApprovalFilter gate — eliminates one full traversal and all duplicate hash and exemption computations.
| } | ||
| } | ||
|
|
||
| func applyAccountConfig(base moduleConfig, data json.RawMessage) (moduleConfig, error) { |
There was a problem hiding this comment.
Every DOOH auction with a per-account module config triggers applyAccountConfig, which calls normalizeModuleConfig: URL parsing via url.ParseRequestURI, normalizePlatforms (allocates seen-map + slice), normalizeExemptBidders (allocates second map + slice), and cfg.Headers iteration — all deterministic for a given account config blob.
Consider memoising the result keyed on the raw JSON bytes of the account config so normalisation runs once per unique config rather than once per auction.
| return normalized | ||
| } | ||
|
|
||
| func isBidderExempt(cfg moduleConfig, bidder string) bool { |
There was a problem hiding this comment.
isBidderExempt iterates the ExemptBidders slice with strings.EqualFold on every per-bid call. With 10 exempt bidders, 10 non-exempt bidders, and 50 bids each, this is ~4 900 string comparisons per auction.
Since ExemptBidders is fixed at config normalisation time, convert it to map[string]struct{} inside normalizeExemptBidders (with lowercase keys) and replace the loop with a single map lookup.
| for id := range creativesByID { | ||
| ids = append(ids, id) | ||
| } | ||
| sort.Strings(ids) |
There was a problem hiding this comment.
The sorted slice is used only as keys into a map and as input to scheduleApprovalRefresh, whose claim() deduplicates by ID regardless of order. Removing sort.Strings saves one allocation and O(n log n) work per auction with no change in behaviour. If deterministic ordering is needed for test assertions, sort only in test fixtures.
Summary
This PR adds a new
prebid.doohcreativeapprovalmodule for Digital Out-of-Home requests. The module gives publishers the ability to approve or reject bidder creatives before they can compete in an auction.The module is configured through host and publisher account settings. Host config enables the module, defines hook execution, cache limits, concurrency, and default refresh behavior. Publisher account config supplies the approval endpoint, request headers, status TTLs, and optional bidder exemptions.
This is a step toward better support for publisher-controlled creative review in Prebid Server. DOOH publishers may need to review creative content for venue requirements, screen policies, brand safety, or operational restrictions before allowing it to play. This module provides a configurable approval layer without making PBS the durable source of truth for publisher approval state.
What Changed
prebid.doohcreativeapprovalmodule and registered it in the module builder.processed_auction_requesthook.all_processed_bid_responseshook before winner selection.bid.crid.approved,rejected, andpendingstatuses.Notes
PBS is not the durable approval system. Each PBS process keeps an in-memory cache, while the publisher approval service remains the long-term source of truth.
A first-seen creative is treated as
pendingand excluded from the current auction while PBS starts a background lookup. Later auctions use the returned status.Status TTLs control when PBS attempts a refresh; they do not discard the last-known status. If the publisher approval API is unavailable, previously approved creatives continue serving and previously rejected or pending creatives remain filtered. Creatives with no usable prior status remain pending until a successful response is received.
Cache contents and refresh coordination are local to each PBS process. The module does not add a cache inspection or invalidation API.