Auction orchestration

[ChristianPavilonis]

No description provided.

[jevansnyc]

Confirming "providers" means wrappers?

[jevansnyc]

mediator = final ad-server?

[jevansnyc]

timeout meaning the time for PBS, APS etc to come back to TS right? This does not include any GAM RT's or think time?

[jevansnyc]

Better way to say is that "Both return their bids within a time window" as they'll never be simultaneous

[jevansnyc]

This may be addressed elsewhere, but AFAIK APS doesn't return a clear text creative bid, so we'll need to understand what the decryption of the TAM value looks like

[jevansnyc]

Let's discuss this on a call as it's not really a realistic scenario where PBS comes back with nothing and I don't know any pubs interested in waterfalls these days. We also need to make sure we are considering bid floors set by publishers here. Maybe I'm missing context?

[aram356]

Code review from senior developer perspective - adding inline comments on specific concerns.

[aram356]

🔧 Critical: Global Static Singleton Initialization Order Risk

File: crates/common/src/auction/mod.rs:27-72

The get_orchestrator() function takes settings but only uses them on first call. Subsequent calls ignore the settings parameter, which could cause confusion or bugs if called with different settings.

static GLOBAL_ORCHESTRATOR: OnceLock<AuctionOrchestrator> = OnceLock::new();

pub fn get_orchestrator(settings: &Settings) -> &'static AuctionOrchestrator {
    GLOBAL_ORCHESTRATOR.get_or_init(|| { ... })
}

Suggestion: Either:

• Make the signature get_orchestrator() -> Option<&'static AuctionOrchestrator> with separate init_orchestrator(settings)
• Or document clearly that settings are only used on first call
• Consider using OnceLock::get() to check initialization status before calling

[aram356]

🔧 Critical: Memory Leak in Route Registration

File: crates/common/src/integrations/prebid.rs:245

let static_path: &'static str = Box::leak(script_path.clone().into_boxed_str());

This leaks memory. While the comment says it's "safe because config lives for app lifetime," this is still a code smell and will show up in memory profilers.

Suggestion: Use a different pattern:

• Store the path in the struct and return a reference
• Use a static string cache/interning library
• Or if truly needed, use once_cell::sync::Lazy with a HashSet<String> for interned strings

[aram356]

🔧 Important: Raw Provider Reference Storage Pattern

File: crates/common/src/auction/orchestrator.rs:308-313

pending_requests.push((
    provider.provider_name(),
    pending,
    start_time,
    provider.as_ref(),  // Raw reference stored alongside owned data
));

Storing provider.as_ref() alongside the PendingRequest is risky. While it works here because providers is borrowed for the function duration, this pattern is fragile. If refactored, it could cause use-after-free.

Suggestion: Clone the provider Arc and store that instead, or restructure to avoid storing the provider reference:

pending_requests.push((
    provider.clone(),  // Arc<dyn AuctionProvider>
    pending,
    start_time,
));

[aram356]

🔧 Important: Hash-Based "Randomness" Behavior Undocumented

File: crates/common/src/integrations/gam.rs:326-333

let should_gam_win = {
    let mut hasher = DefaultHasher::new();
    slot_id.hash(&mut hasher);
    let hash_val = hasher.finish();
    (hash_val % 100) < self.config.win_rate as u64
};

Using DefaultHasher for deterministic behavior based on slot_id means the same slot will always have the same outcome. This may be intentional for testing consistency, but:

1. DefaultHasher is not guaranteed stable across Rust versions
2. This behavior isn't documented

Suggestion:

• Document this clearly as intentional (e.g., "deterministic for reproducible tests")
• Or use a seeded RNG if actual randomness per-request is desired
• Consider using ahash or another stable hasher if determinism is required

[aram356]

🔧 Important: Error Handling Timing Inconsistency

File: crates/common/src/auction/orchestrator.rs:346-358

match provider.parse_response(response, response_time_ms) {
    Ok(auction_response) => { ... }
    Err(e) => {
        log::warn!("Provider '{}' failed to parse response: {:?}", provider_name, e);
        responses.push(AuctionResponse::error(provider_name, response_time_ms));
    }
}

When a provider fails to parse a response, an error response is added with response_time_ms calculated from the original start time. This conflates "time to receive response" with "time including parse failure."

Suggestion: Track response receipt time separately from total processing time, or add metadata distinguishing network time from processing time:

AuctionResponse::error(provider_name, response_time_ms)
    .with_metadata("error_phase", json!("parse"))

[aram356]

⛏️ Minor: OrchestrationResult Helper Methods Position

File: crates/common/src/auction/orchestrator.rs:773-792

The impl OrchestrationResult block is placed after the #[cfg(test)] module (~360 lines of tests), which is unusual and makes the code harder to navigate.

// Current structure:
impl AuctionOrchestrator { ... }  // line 35-409

#[cfg(test)]
mod tests { ... }  // line 412-771

impl OrchestrationResult { ... }  // line 773-792  ← buried after tests

Suggestion: Move impl OrchestrationResult immediately after the struct definition (around line 33) for better readability and discoverability.

[aram356]

💡 Architecture Suggestion: Builder Pattern for AuctionRequest

File: crates/common/src/auction/mod.rs:167-230

Building AuctionRequest in convert_tsjs_to_auction_request is verbose (~60 lines). A builder pattern would improve ergonomics and reduce boilerplate:

// Current approach - verbose
Ok(AuctionRequest {
    id: Uuid::new_v4().to_string(),
    slots,
    publisher: PublisherInfo {
        domain: settings.publisher.domain.clone(),
        page_url: Some(format!("https://{}", settings.publisher.domain)),
    },
    user: UserInfo {
        id: synthetic_id,
        fresh_id,
        consent: None,
    },
    device,
    site: Some(SiteInfo { ... }),
    context: HashMap::new(),
})

// With builder - cleaner
AuctionRequest::builder()
    .id(Uuid::new_v4())
    .slots(slots)
    .publisher(&settings.publisher.domain)
    .user(synthetic_id, fresh_id)
    .device_from_request(&req)
    .build()

This would also make it easier to add optional fields in the future without changing all call sites.