Skip to content

Commit bfde031

Browse files
committed
Address fourth-round review feedback on backend naming and quantization
- Reject a mediator that is also listed in [auction].providers at startup; the overlap would fire the same provider twice per auction and its demand already flows through the mediation response - Assert in both cardinality enumeration tests that canonical values never exceed the remaining budget (the mediator </body> hold bound invariant) - Document the accepted worst-case rounding haircut on the coarse ladder - Write the spec digest hex into one pre-sized buffer instead of allocating per byte, and defer the ensure() doc to compute_name
1 parent 77f3521 commit bfde031

3 files changed

Lines changed: 66 additions & 10 deletions

File tree

crates/trusted-server-adapter-fastly/src/backend.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
use core::fmt::Write as _;
12
use std::time::Duration;
23

34
use error_stack::{Report, ResultExt as _};
@@ -69,11 +70,11 @@ fn spec_digest_hex(canonical: &str) -> String {
6970
let mut hasher = Sha256::new();
7071
hasher.update(canonical.as_bytes());
7172
let digest = hasher.finalize();
72-
digest
73-
.iter()
74-
.take(SPEC_DIGEST_HEX_LEN / 2)
75-
.map(|byte| format!("{byte:02x}"))
76-
.collect()
73+
let mut hex = String::with_capacity(SPEC_DIGEST_HEX_LEN);
74+
for byte in digest.iter().take(SPEC_DIGEST_HEX_LEN / 2) {
75+
write!(hex, "{byte:02x}").expect("should write hex digit to string");
76+
}
77+
hex
7778
}
7879

7980
/// Default first-byte timeout for backends (15 seconds).
@@ -327,11 +328,10 @@ impl<'a> BackendConfig<'a> {
327328

328329
/// Ensure a dynamic backend exists for this configuration and return its name.
329330
///
330-
/// The backend name is derived from the scheme, host, port, certificate
331-
/// setting, `first_byte_timeout`, and `between_bytes_timeout` to avoid
332-
/// collisions. Different timeout values produce different backend
333-
/// registrations so that a tight deadline cannot be silently widened by an
334-
/// earlier registration.
331+
/// The name is a collision-resistant function of the complete backend spec
332+
/// (see `Self::compute_name`), so different specs — for example, different
333+
/// timeout values — always produce different backend registrations and a
334+
/// tight deadline cannot be silently widened by an earlier registration.
335335
///
336336
/// # Errors
337337
///

crates/trusted-server-adapter-fastly/src/platform.rs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,13 @@ const SUB_QUANTUM_LADDER_MS: [u32; 4] = [200, 150, 100, 50];
196196
/// the greatest rung no larger than the remaining budget, and anything above
197197
/// the top rung clamps to it. Rounding down never extends a transport cap past
198198
/// the remaining budget.
199+
///
200+
/// The rung spacing trades transport window for cardinality: just below a rung
201+
/// the haircut approaches the gap to the rung beneath (worst case ~50%, e.g. a
202+
/// remaining budget of 9,999 ms snaps to 5,000 ms). This is accepted — on the
203+
/// mediator path this value is the effective bound, but a denser ladder would
204+
/// buy back at most half a bucket of transport time at the cost of
205+
/// proportionally more backend names.
199206
const TRANSPORT_TIMEOUT_COARSE_LADDER_MS: [u32; 8] =
200207
[2000, 3000, 5000, 10000, 20000, 30000, 45000, 60000];
201208

@@ -1126,6 +1133,12 @@ mod tests {
11261133
"canonical value {value}ms (from remaining {remaining}ms) is neither a quantum \
11271134
multiple nor a ladder rung"
11281135
);
1136+
// The mediator </body> hold bound relies on canonicalization never
1137+
// extending a transport cap past the wall-clock budget.
1138+
assert!(
1139+
value <= remaining,
1140+
"canonical value {value}ms must not extend past the remaining {remaining}ms budget"
1141+
);
11291142
}
11301143
assert!(
11311144
distinct.len() <= 16,
@@ -1156,6 +1169,12 @@ mod tests {
11561169
"canonical value {value}ms (from remaining {remaining}ms) is neither a quantum \
11571170
multiple nor a ladder rung"
11581171
);
1172+
// The mediator </body> hold bound relies on canonicalization never
1173+
// extending a transport cap past the wall-clock budget.
1174+
assert!(
1175+
value <= remaining,
1176+
"canonical value {value}ms must not extend past the remaining {remaining}ms budget"
1177+
);
11591178
}
11601179
// A budget above the top coarse rung must clamp to it, not open a new
11611180
// bucket per 250ms step.

crates/trusted-server-core/src/auction/orchestrator.rs

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,20 @@ impl AuctionOrchestrator {
209209
}
210210
}
211211

212+
// A provider that is also the mediator would be called twice per
213+
// auction — once in the bidding phase and again as the mediator. The
214+
// mediator's own demand already flows through its mediation response,
215+
// so the overlap is never a legitimate configuration.
216+
if let Some(mediator_name) = &self.config.mediator
217+
&& seen.contains(mediator_name.as_str())
218+
{
219+
return Err(Report::new(TrustedServerError::Configuration {
220+
message: format!(
221+
"Auction mediator `{mediator_name}` is also listed in [auction].providers; a provider may not mediate its own auction"
222+
),
223+
}));
224+
}
225+
212226
for provider_name in self
213227
.config
214228
.providers
@@ -2074,6 +2088,29 @@ mod tests {
20742088
);
20752089
}
20762090

2091+
#[test]
2092+
fn rejects_mediator_also_listed_as_provider() {
2093+
// A provider acting as its own mediator would be called twice per
2094+
// auction (bidding phase, then mediation); its demand already flows
2095+
// through the mediation response, so reject the overlap at startup.
2096+
let config = AuctionConfig {
2097+
enabled: true,
2098+
providers: vec!["prebid".to_string()],
2099+
mediator: Some("prebid".to_string()),
2100+
timeout_ms: 2000,
2101+
..Default::default()
2102+
};
2103+
let orchestrator = AuctionOrchestrator::new(config);
2104+
2105+
let err = orchestrator
2106+
.validate_configured_provider_names()
2107+
.expect_err("should reject a mediator that is also a provider");
2108+
assert!(
2109+
err.to_string().contains("may not mediate its own auction"),
2110+
"should explain the mediator/provider overlap, got: {err}"
2111+
);
2112+
}
2113+
20772114
#[test]
20782115
fn test_orchestrator_is_enabled() {
20792116
let config = AuctionConfig {

0 commit comments

Comments
 (0)