Skip to content

Core: add minWinningBidCacheTTL so winning bids don't expire (#12987)#14536

Open
vasujain00 wants to merge 5 commits intoprebid:masterfrom
vasujain00:feature/min-winning-bid-cache-ttl-codex
Open

Core: add minWinningBidCacheTTL so winning bids don't expire (#12987)#14536
vasujain00 wants to merge 5 commits intoprebid:masterfrom
vasujain00:feature/min-winning-bid-cache-ttl-codex

Conversation

@vasujain00
Copy link

Summary

Adds minWinningBidCacheTTL config option to prevent winning bids (that have had targeting set) from expiring, fixing "cannot find ad" / expired render errors when using GPT lazy load with scroll-based render.

Problem

Setting minBidCacheTTL causes "cannot find ad" errors in lazy-load scenarios. The ad is fetched from GPT, but the scroll milestone for render takes a long time. Winning bids expire and are dropped from memory before GPT can render them.

Solution

  • minWinningBidCacheTTL – When set, overrides minBidCacheTTL for bids that have had targeting set (winning bids sent to the ad server).
  • If unset, behavior is unchanged (all bids use minBidCacheTTL).
  • Set to Infinity (or a large value) to effectively never expire winning bids.

Usage

pbjs.setConfig({
  minBidCacheTTL: 30,
  minWinningBidCacheTTL: Infinity  // keep winning bids indefinitely for lazy-load GPT
});

Changes

  • src/bidTTL.ts – Added minWinningBidCacheTTL config, getMinWinningBidCacheTTL(), and getEffectiveMinBidCacheTTL(bid)
  • src/auction.ts – Use effective TTL per bid; refresh bid TTL when targeting is set
  • src/auctionManager.js – Use effective TTL when calculating auction TTL

Testing

  • gulp lint – passed
  • gulp test --file test/spec/auctionmanager_spec.js – 91 tests passed

Labels

feature | minor


Fixes #12987

- Updated auction and auctionManager to utilize getEffectiveMinBidCacheTTL for determining bid TTL based on targeting status.
- Added minWinningBidCacheTTL configuration to allow for differentiated TTL handling for winning bids.
- Enhanced bidTTL module with new functions to support effective TTL calculations.
- Updated tests to cover new TTL logic and configurations, ensuring correct behavior for bids with and without targeting set.
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c06c4650ea

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +45 to +49
ttl: (au) => au.end.then(() => {
const bids = au.getBidsReceived();
if (bids.length === 0) {
const minTTL = getMinBidCacheTTL();
return minTTL == null ? null : minTTL * 1000;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Refresh auction TTL when targeting status is updated

The auction-level TTL is computed once inside au.end.then(...), so it snapshots each bid’s status at auction end; however bids are marked BID_TARGETING_SET later via setStatusForBids when setTargetingForGPT runs. Because _auctions.refresh() is never triggered on that status transition, auctions can still expire using the pre-targeting TTL (for example minBidCacheTTL/bid TTL) instead of minWinningBidCacheTTL, which reintroduces the lazy-load renderAd/"cannot find ad" failure this change is trying to prevent when all bids have short TTLs.

Useful? React with 👍 / 👎.

@patmmccann
Copy link
Collaborator

@vasujain00 thanks for the feature! could you link a docs pull?

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 22521425597

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 57 of 57 (100.0%) changed or added relevant lines in 4 files are covered.
  • 46 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+33.2%) to 96.328%

Files with Coverage Reduction New Missed Lines %
src/adloader.js 9 76.92%
src/utils.js 37 90.51%
Totals Coverage Status
Change from base Build #3: 33.2%
Covered Lines: 217375
Relevant Lines: 225662

💛 - Coveralls

@monis0395
Copy link
Collaborator

Hi, I am a little caught up in some tasks will review this tom.

Copy link
Collaborator

@dgirardi dgirardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even addressing the refresh issue, I have some concerns with this:

  • the current logic picks the maximum between minBidCacheTTL and the bid's own .ttl. If we need this feature it should mean that either the bid's .ttl is inaccurate, or that rendering will not yield revenue. Calling out vendors that need to fix their TTL and working around the problem by overriding it using the bidResponse event might be a better way to address this. There's also an open issue #13838 on how to make refreshing easier.
  • a bid that was sent to the ad server (targeting set) is not a winning bid; unfortunately we can't really know which bids should be "saved" as we know what won only by the time we need to render it. In the general case, any bid may win at any point in the future, since even those that were not selected for targeting may still be picked from cache later. And likewise bids that were used for targeting may never win. So I don't think there's a clean way to do this besides fixing bids' TTLs.

I can see how this can still be useful but I think the setting should be called something else, maybe minTargetedBidCacheTTL.

auction.callBids();
await auction.end;
const shortTtlBid = auctionManager.getBidsReceived().find(b => b.ttl === 10);
auctionManager.setStatusForBids(shortTtlBid.adId, BID_STATUS.BID_TARGETING_SET);
Copy link
Collaborator

@dgirardi dgirardi Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the bot is correct; when the TTL is calculated the bid is fresh (not targeted yet) and there's no refresh when targeting is set. This test should await clock.tick() before marking the bid as targeted.

Scratch that, I missed that you do in fact refresh - but the bot still seems right, the auction may hold on to the bid but be removed from auctionManager.

@olafbuitelaar
Copy link
Contributor

any concerns about SSP's validating server side if the bid has been expired and hence wouldn't attribute revenue (if the bid was rendered after the TTL), because the bid was expired on their end?

@patmmccann
Copy link
Collaborator

we're having some debate on if winning is the correct terminology here

perhaps minSentBidCacheTTL or minPassedBidCacheTTL or targeted?

@patmmccann
Copy link
Collaborator

any concerns about SSP's validating server side if the bid has been expired and hence wouldn't attribute revenue (if the bid was rendered after the TTL), because the bid was expired on their end?

yes we certainly wouldnt want to make this default behavior. I think docs pr here should explain to the publisher the choice they need to make when expiring bids. When bids expire after targeting but before rendering, should they hold a new auction, do nothing, or just render? When bids are missing from the cache bc they are doing memory saving techniques, they should also be making an explicit choice.

@vasujain00
Copy link
Author

vasujain00 commented Mar 12, 2026

@patmmccann Docs: add minWinningBidCacheTTL / targeted-bid cache TTL to setConfig
#6479

@github-actions
Copy link

Tread carefully! This PR adds 6 linter errors (possibly disabled through directives):

  • src/auctionManager.js (+6 errors)

@monis0395
Copy link
Collaborator

@vasujain00 There are some unresolved comments please resolve those

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Core: don't expire winning bids

7 participants