cmd/router: cache Context Match responses per spec §Caching#410
cmd/router: cache Context Match responses per spec §Caching#410ohalushchak-exadel wants to merge 4 commits into
Conversation
![aao-ipr-bot[bot]](https://avatars.githubusercontent.com/u/134922?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean cache, one spec hole. The mechanism is right — the block is that cache_ttl=0 silently violates a normative MUST in the exact feature this conformance series exists to certify.
The block
cache_ttl == 0 is treated as "use default," not "disable caching." router/context_cache.go:140 (Put only overrides when resp.CacheTTL > 0) maps a received zero to the 5-minute default. The field's own normative doc (tmproto/types_gen.go:140) says: "the router MUST use this value instead of its default. Set to 0 to disable caching (e.g., when targeting configuration has just changed)." The router does neither for 0.
Failure mode, one sentence: a non-Go TMP provider (Python/TS) that just changed targeting sends "cache_ttl": 0 on the wire to force no-caching, and the router caches its now-stale offers for 5 minutes and serves them to every user on that placement — the exact scenario the parenthetical was written to prevent.
Both experts flagged this independently:
code-reviewer: High — "serving exactly the stale offers the field exists to prevent."ad-tech-protocol-expert: conformance violation with cited divergence. Its key correction: the PR frames this as a sender problem ("Go can't emit 0"), but the ambiguity is on the receive side — aCacheTTL intcannot represent the tri-state {absent, explicit-0, positive}, so even a perfectly conformant non-Go provider's explicit 0 is indistinguishable from a Go provider's omission.
The PR's own justification doesn't survive contact with this repo's rules. docs/sdk-typing-policy.md:68-69: "Use a pointer if the schema marks the field nullable, gives omission semantics, or callers need to distinguish missing from zero." cache_ttl has omission semantics (omit → default) distinct from explicit-0 (disable). Per the repo's own accepted policy it MUST be a pointer. The cache_ttl=1 "nearly-disabled" workaround reinterprets a published contract at the provider's expense — at ad-request QPS a 1s window still serves thousands of stale offers.
What flips this to approve — either:
- Make the wire field
*int(schema + regen oftmproto/types_gen.goperdocs/sdk-typing-policy.md) and branch inPut/fanOutContexton nil (default) vs*0(skip the Put — actually don't cache) vs*n>0(override); or - A recorded maintainer decision that shipping this deviation pre-publish is acceptable, with the spec issue filed — not a code comment framing it as a neutral design choice.
I lean 1. The int→*int change is source-breaking for tmproto Go consumers, but it's a single targeted field and the typing policy's "no sweeping pointer changes" rule is satisfied. If it's better as its own PR, say so and we sequence it before the image publish — but the conformance PR can't land the violation it's certifying against.
Things I checked
- Deep-clone completeness.
cloneOffer(router/context_cache.go:214) covers every non-scalarOfferfield —SellerAgent,Brand,Price,CreativeManifest,Macros— matchingtmproto/types_gen.go:194-202exactly.cloneContextResponsecoversOffersand top-levelSignals; scalars ridedst := *src. Clone-on-both-read-and-write is the right call given theSellerAgent-stamp note attypes_gen.go:196. The isolation test atcontext_cache_test.go:471proves it. - Concurrency.
mudiscipline is clean; entries are immutable after insert (Putoverwrites the map slot with a fresh clone), so readingentry.responseoutside the lock inGet(context_cache.go:120) is safe. Loop varpis per-iteration;cmReqis read-only in the goroutines. 32-goroutine-racetest atcontext_cache_test.go:628. - Cache-hit bookkeeping. Hit path skips
RecordSuccess/duration observation correctly (no call was made), and offer counting is not lost —AddOffersruns post-merge inHandleContextMatchregardless of hit/miss. Verified bycode-reviewer. - Cache key. Dropping
package_idsis spec-correct given the per-user invariant attypes_gen.go:130("MUST NOT vary by user");ad-tech-protocol-expertconfirmed provider-scoped filtering happens after the check. - Circuit-open serving and the 24h clamp are spec-consistent — bounded staleness, provider chose the TTL.
- Env wiring (
cmd/router/main.go),CacheConfig(serverconfig.go:34), bad-value handling, and docs (network-surface.md:221-222) all correct. Conventional-commit type is right; no breaking wire symbol removed.
Follow-ups (non-blocking — file as issues)
- TTL clamp defeated by overflow.
context_cache.go:141:time.Duration(resp.CacheTTL)*time.Secondoverflows int64 forcache_ttl> ~9.2e9 before themin(..., MaxContextCacheTTL)clamp, wrapping negative → already-expired entry. Benign (entry just isn't cached) but defeats the defense-in-depth the comment claims. Clamp in seconds first, then convert. Signalsis a shallow clone.context_cache.go:205-208copies the top-level map but nestedmap[string]any/[]anyvalues stay shared. Not currently exploited — the merger only reads them into a fresh map — but the doc promises callers can "freely mutate," which isn't true for nested Signals. Latent.
Minor nits (non-blocking)
- Dead assignment.
router/router.go:383cached.RequestID = cmReq.RequestIDhas no observable effect —mergeContextResponsessetsmerged.RequestID = requestID(router.go:613) and never readsres.response.RequestID. Drop it or keep for symmetry, but the comment oversells it.
The test-plan checkboxes are all checked, including the two e2e cache-behavior items — notable, given they can't cover the one path (cache_ttl=0 from a non-Go provider) that's actually broken. Resolve the block and this ships.
Fixes the request-changes review on #410: the previous implementation treated any received cache_ttl of zero as "use the router default TTL" because Go's int + omitempty conflates absent-field with present-zero at unmarshal time. Non-Go providers that send an explicit cache_ttl=0 on the wire (spec §Caching: "0 disables caching, e.g. when targeting configuration has just changed") would have their now-stale offers cached for 5 minutes anyway — exactly the scenario the field exists to prevent. The fix mirrors adcp-go's own SDK typing policy (docs/sdk-typing-policy.md): fields with omission-vs-zero semantics distinct at the wire level MUST be pointer types. Changes: - internal/generate/go-overlays.json — force *int for ContextMatchResponse.cache_ttl - tmproto/types_gen.go — regenerated: CacheTTL *int - targeting/contextagent/handler.go — assign &ttl when the agent wants to override; nil otherwise - router/context_cache.go Put branches on the tri-state: * nil → use configured default * *0 → provider disabling caching; entry NOT stored * *n > 0 → override, clamped to MaxContextCacheTTL * *n < 0 → nonsensical; fall back to default rather than store an already-expired entry Also addresses the two non-blocking follow-ups from the same review: - Overflow-safe clamp: seconds are clamped BEFORE conversion to time.Duration, so a pathologically large cache_ttl (past ~9.2e9s) no longer overflows the int64 multiplication and silently drops the entry. TestContextCache_TTLOverflowSafe locks it in. - Dead RequestID assignment on cache hit removed. The merger (mergeContextResponses) already sets the response's RequestID from the incoming request, so any overwrite inside fanOutContext was observationally dead. Test additions: - TestContextCache_AbsentTTLUsesDefault — nil pointer → default - TestContextCache_ExplicitZeroTTLDisablesCaching — cache_ttl=0 entry not stored - TestContextCache_NegativeTTLUsesDefault — defensive fallback - TestContextCache_TTLOverflowSafe — 1<<62 seconds still stores at MaxContextCacheTTL, not silently dropped - TestRouterContextMatch_ProviderDisablesCaching — end-to-end: three successive requests all fan out because cache_ttl=0 skips the Put
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
2 similar comments
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
There was a problem hiding this comment.
Request changes. The cache key drops the two request dimensions this repo's own targeting engine uses to scope offers, so within the TTL the router serves one seller's offers to a different seller on the same placement — a cross-seller disclosure. security-reviewer: High, with a reproducible path.
The engineering around the cache_ttl tri-state is genuinely good work — the *int typing, the overflow-safe clamp, the deep-clone of Offer, the six-case wire round-trip test. The blocker is the key, not the mechanism.
The block — cache key omits seller_agent_url and country
contextCacheKey keys on {property_rid, placement_id, provider_id} (router/context_cache.go:194), read at the fan-out short-circuit (router/router.go Get) and written after fresh fill (router.go Put). But the provider's offer set is seller- and geo-scoped:
targeting/engine.go:246—ActivePackages(ctx, canonicalSeller, req.PropertyID, country, req.PlacementID, e.now()). Differentseller_agent_url→ different authorized packages → different offers (brand, price, creative manifest). An unregistered seller fails safe to an empty active set (engine.go:242).seller_agent_urlis a required request field (tmproto/validate.go).
The PR's rationale — "the same packages are evaluated for every user on a given placement" — conflates per-viewer invariance with per-seller invariance. Offers are invariant across viewers; they are not invariant across sellers. This is the precise gap.
Failure mode for adopters, one sentence: premium seller S_A queries {property_rid=X, placement=P}, the router caches S_A's authorized offers under X\0P\0provider, and for the rest of the TTL (default 5 min, up to 24h) any other seller S_B querying the same {X, P} is served S_A's offers verbatim — competitor brands, pricing, and creative manifests — with no provider round-trip. The mirror case is fill denial: an empty-set seller poisons the entry first, and legitimate S_A gets served empty offers until the entry expires. mergeContextResponses does not re-filter cached offers against the current caller, so nothing downstream contains the leak.
This is MUST-FIX category 2 (data exposure / missing tenant scope on a router endpoint that scopes by tenant). The seller is the tenant dimension here, and /tmp/context takes it straight from an unauthenticated request field.
The fix is small: add the canonicalized seller_agent_url (same urlcanon.Canonicalize the engine uses at engine.go:238) and the geo/country component to contextCacheKey. If the external spec §Caching genuinely defines the key as placement-only, then the spec is under-specified against this repo's observable per-seller offer scoping — reconcile that upstream before shipping the placement-only key; don't ship the leak to match a spec line.
ad-tech-protocol-expert could not verify the spec's cache-key text from source (docs/trusted-match/specification.mdx lives in adcontextprotocol/adcp; schemas are download-on-demand at pinned 3.1.3), so the premise that a placement-only key is spec-faithful is currently unverified. That unverified premise is exactly what the security finding contradicts.
Things I checked
- Generated type is overlay-driven, not hand-edited.
tmproto/types_gen.goCacheTTL int→*intreproduces from the single overlay line ininternal/generate/go-overlays.json:48;internal/generate/schema.goappliesfo.Type+*forPointerand retainsomitempty(notrequired). Correct escape hatch. - No wire break. Field stays an optional JSON integer with
omitempty. The change only makes the spec'scache_ttl:0disable signal emittable — Go'sint+omitemptycould never marshal it. Non-breaking; no conventional-commit!needed. - Tri-state is faithful to the generated schema doc ("MUST use when present; set to 0 to disable"): nil→default, 0→don't store (
Putreturns atcontext_cache.go:154-156), >0→clamp, <0→default.TestContextMatchResponse_CacheTTLWireBehaviorlocks all six marshal/unmarshal cases. - No loop-variable capture bug.
go.modpins go 1.25;pis per-iteration andcmRespis declared inside the closure, so&cmRespis a distinct per-goroutine allocation. - Cache-before-signing is not an auth bypass. Signing is for the router's outbound provider call; a cache hit makes no outbound call.
HandleContextMatchdoes no inbound signature verification, so nothing is skipped. (security-reviewerQ2: clean.) - Metric cardinality bounded — labels only ever
providerIDfromProviderSet.ID;property_rid/placement_idare never labels. - NUL key separator is safe —
validateSafeIDrejects all control bytes (< 0x20) inproperty_rid/placement_id, so no NUL injection or separator collision. - Sub-second RESPONSE_TTL regression fix is correct (
targeting/contextagent/handler.go): the guard is on truncatedsecs > 0, so a 500ms TTL omits the field and gets the router default rather than emittingcache_ttl=0(accidental disable).
Follow-ups (non-blocking — file as issues)
- Unbounded cache growth.
security-reviewer: Medium.entrieshas no max-size cap and no janitor; expired entries evict only on a same-key refetch (context_cache.go:122-127). A caller varyingplacement_id(charset-validated but otherwise unconstrained) grows the map without bound → eventual OOM. Add an entry cap with LRU/random eviction plus a periodic sweep. Becomes higher severity if/tmp/contextis exposed without upstream rate-limiting. 86400s"schema-enforced maximum" is unverified and self-contradicted.context_cache.go:18-19calls the 24h clamp "schema-enforced," buttargeting/contextagent/handler.go:133(kept by this same PR) sayscache_ttl"has no spec-level maximum." Both can't hold. The clamp is harmless defense-in-depth; soften the comment to "router-imposed ceiling" unless the 3.1.3context-match-response.jsonactually declaresmaximum: 86400.Signalsis a shallow clone despite the docstring's "deep-clone on read AND write" promise.cloneContextResponsecopies the map withmaps.Copy(context_cache.go:239-242); nestedanyvalues (oftenmap[string]any/[]anyfrom JSON) stay shared. No current caller mutates nested signal values, so it's latent — but the docstring atcontext_cache.go:41-43overpromises. Either deep-clone or scope the comment toOffers.
Minor nits (non-blocking)
- PR description contradicts the shipped code. The body's first design note says "
cache_ttl=0treated as 'use default', not 'disable caching'." The code does the opposite, correctly (Putreturns without storing onsecs == 0). The body is stale from the pre-*intdesign; update it so a future reader doesn't "fix" correct code back to the bug. - Line-number reference will drift.
context_cache.go:216points attmproto/types_gen.go:196for the SellerAgent-stamp note; a symbol reference survives regeneration, a line number won't.
Fix the cache key and I'll re-review. Everything else here is sound.
Fixes the High-severity cross-tenant offer disclosure Argus flagged on the second review of #410. The cache key was {property_rid, placement_id, provider_id}, but this repo's targeting engine (targeting/engine.go: ActivePackages(ctx, canonicalSeller, propertyID, country, placementID, ...)) scopes the active package set per seller and per country. Different sellers on the same placement have different authorized offers; keying on placement alone lets seller A's cached response be served to seller B for the whole TTL window — leaking competitor brands, pricing, and creative manifests across tenants. The fix extends the cache key with canonicalized seller_agent_url (same urlcanon.Canonicalize the engine applies) and country (cmReq.Geo["country"] extracted the same way engine.go does), so the router keys the same offer set the provider computes. Adopters see this as an end-to-end test at router_test.go: TestRouterContextMatch_CacheIsolatesAcrossSellers, plus TestRouterContextMatch_CacheIsolatesAcrossCountries for the symmetric country dimension. Unit-level partition tests cover both. Also folds in the three non-blocking follow-ups the reviewer flagged in the same review, since they touch the same file: - MaxEntries cap on the cache with expired-first-then-oldest eviction, so a caller varying placement/seller/country cannot grow the map without bound. Default 10000, overridable via TMP_ROUTER_CACHE_MAX_ENTRIES. Overwrites of an existing key skip the sweep/evict path so an in-place update doesn't churn the cache. - Signals shallow-clone docstring scoped to what the code actually provides (top-level map allocation, nested any values shared) — the prior "callers may freely mutate any field" was overpromising. Nothing in the merger mutates nested signal values today; a general deep-copy would need a JSON round-trip. - The stale "cache_ttl has no spec-level maximum" comment in contextagent/handler.go is now correct (schema has maximum: 86400, verified against adcp/schemas/trusted-match/context-match-response.json). - Line-number reference to tmproto/types_gen.go:196 replaced with a symbol reference (grep-able across regenerations). Config surface addition: - ServerConfig.Cache.MaxEntries (int, JSON: max_entries) - TMP_ROUTER_CACHE_MAX_ENTRIES env var Pre-commit code review caught two docstring inaccuracies (fixed) and one asymmetry note on canonicalizeSellerForCache (documented).
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
2 similar comments
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
Adds a per-provider Context Match response cache keyed on
{property_rid, placement_id, provider_id}, exactly matching the
recommended cache key in the spec. The default TTL is 5 minutes
(spec SHOULD); a provider's cache_ttl in the response overrides the
default (spec MUST), clamped to the 24h schema ceiling as a
defense-in-depth against upstream validation slips.
Cache-first flow inside fanOutContext: hit → return cached response
with the current request's RequestID stamped on, no health check /
signing / dial. Miss → fall through to the existing fan-out path,
then write the fresh response into the cache before returning.
Design notes:
- CacheTTL=0 in a response is treated as "use default" rather than
"disable caching". Go's int + omitempty conflates absent-field
from present-zero, and a Go-based provider can never explicitly
emit 0 (json.Marshal drops it). Providers that need to force
cache invalidation should return cache_ttl=1. Documented at the
Put call site.
- Cache hit skips the circuit-breaker gate: a warm response is
still useful during a provider outage and the TTL bounds
staleness.
- Every read AND write clones the response, and clones deeply
enough that Offer's inner pointer/slice/map fields (SellerAgent,
Brand, Price, CreativeManifest, Macros) are isolated from cache
entries. This defends against the router MAY stamp SellerAgent
after fan-out pattern the tmproto doc mentions, which under a
shallow clone would silently corrupt cache entries.
Config surface:
- ServerConfig.Cache.{Disabled, DefaultTTLSeconds}
- TMP_ROUTER_CACHE_DISABLED — turn caching off entirely
- TMP_ROUTER_CACHE_DEFAULT_TTL_SEC — override the 5-minute default
Metrics:
- tmp_context_cache_hits_total{provider}
- tmp_context_cache_misses_total{provider}
Test coverage:
- Cache unit tests: nil-safe, deep-clone isolation, TTL expiration
(injected clock), provider TTL override, TTL clamp to max,
zero-TTL-uses-default, per-provider key partitioning, metrics
counts, concurrent-safe under race detector (32 goroutines).
- End-to-end: repeat Context Match hits cache and does not re-hit
the provider; provider cache_ttl override survives past the
router's default TTL.
- Env-merge coverage for both cache env vars, including bad-value
ignore.
Refs AI-3719.
Fixes the request-changes review on #410: the previous implementation treated any received cache_ttl of zero as "use the router default TTL" because Go's int + omitempty conflates absent-field with present-zero at unmarshal time. Non-Go providers that send an explicit cache_ttl=0 on the wire (spec §Caching: "0 disables caching, e.g. when targeting configuration has just changed") would have their now-stale offers cached for 5 minutes anyway — exactly the scenario the field exists to prevent. The fix mirrors adcp-go's own SDK typing policy (docs/sdk-typing-policy.md): fields with omission-vs-zero semantics distinct at the wire level MUST be pointer types. Changes: - internal/generate/go-overlays.json — force *int for ContextMatchResponse.cache_ttl - tmproto/types_gen.go — regenerated: CacheTTL *int - targeting/contextagent/handler.go — assign &ttl when the agent wants to override; nil otherwise - router/context_cache.go Put branches on the tri-state: * nil → use configured default * *0 → provider disabling caching; entry NOT stored * *n > 0 → override, clamped to MaxContextCacheTTL * *n < 0 → nonsensical; fall back to default rather than store an already-expired entry Also addresses the two non-blocking follow-ups from the same review: - Overflow-safe clamp: seconds are clamped BEFORE conversion to time.Duration, so a pathologically large cache_ttl (past ~9.2e9s) no longer overflows the int64 multiplication and silently drops the entry. TestContextCache_TTLOverflowSafe locks it in. - Dead RequestID assignment on cache hit removed. The merger (mergeContextResponses) already sets the response's RequestID from the incoming request, so any overwrite inside fanOutContext was observationally dead. Test additions: - TestContextCache_AbsentTTLUsesDefault — nil pointer → default - TestContextCache_ExplicitZeroTTLDisablesCaching — cache_ttl=0 entry not stored - TestContextCache_NegativeTTLUsesDefault — defensive fallback - TestContextCache_TTLOverflowSafe — 1<<62 seconds still stores at MaxContextCacheTTL, not silently dropped - TestRouterContextMatch_ProviderDisablesCaching — end-to-end: three successive requests all fan out because cache_ttl=0 skips the Put
… nits Self-review of the prior commit surfaced a real regression I introduced along with two safety gaps worth closing in the same push: **Regression** — a deploy with 0 < RESPONSE_TTL < 1s (e.g. 500ms) would flip from "cache for 5 min" to "never cache" because int(h.responseTTL.Seconds()) truncates to 0 and, under the new tri-state semantics, the router now reads cache_ttl=0 as the spec's disable-caching signal. The guard is now on the truncated seconds value, not on the raw duration: sub-second TTLs omit the field so the router's default applies (matching the pre-existing behavior). **cloneContextResponse** — the struct copy shared CacheTTL's *int with the cached entry. Nothing mutates *resp.CacheTTL today but the file's own doc comment promises callers can mutate any field of a cache-hit copy without corrupting the entry. Deep-clone the pointer so that stays honest. **Wire round-trip coverage** — the new *int type on ContextMatchResponse.CacheTTL is the load-bearing behavior of the whole PR-D fix. Add a tmproto-level test that pins down all six cases (marshal nil → omitted, *0 → 0, *n → n; unmarshal absent → nil, 0 → *int(0), n → *int(n)). Any future codegen or JSON-tag drift that regresses these fails the test loudly. **Portability** — replaced 1 << 62 in the overflow-clamp test with math.MaxInt so the test compiles on 32-bit builds (adcp-go is 64-bit in practice but non-64-bit builds shouldn't break `go test ./...`). Follow-up on non-blocking review nits noted for future PRs: - Signals is still a shallow clone (nested any values shared). Documented as latent; will surface as a fix if any caller ever mutates nested values, which nothing does today.
Fixes the High-severity cross-tenant offer disclosure Argus flagged on the second review of #410. The cache key was {property_rid, placement_id, provider_id}, but this repo's targeting engine (targeting/engine.go: ActivePackages(ctx, canonicalSeller, propertyID, country, placementID, ...)) scopes the active package set per seller and per country. Different sellers on the same placement have different authorized offers; keying on placement alone lets seller A's cached response be served to seller B for the whole TTL window — leaking competitor brands, pricing, and creative manifests across tenants. The fix extends the cache key with canonicalized seller_agent_url (same urlcanon.Canonicalize the engine applies) and country (cmReq.Geo["country"] extracted the same way engine.go does), so the router keys the same offer set the provider computes. Adopters see this as an end-to-end test at router_test.go: TestRouterContextMatch_CacheIsolatesAcrossSellers, plus TestRouterContextMatch_CacheIsolatesAcrossCountries for the symmetric country dimension. Unit-level partition tests cover both. Also folds in the three non-blocking follow-ups the reviewer flagged in the same review, since they touch the same file: - MaxEntries cap on the cache with expired-first-then-oldest eviction, so a caller varying placement/seller/country cannot grow the map without bound. Default 10000, overridable via TMP_ROUTER_CACHE_MAX_ENTRIES. Overwrites of an existing key skip the sweep/evict path so an in-place update doesn't churn the cache. - Signals shallow-clone docstring scoped to what the code actually provides (top-level map allocation, nested any values shared) — the prior "callers may freely mutate any field" was overpromising. Nothing in the merger mutates nested signal values today; a general deep-copy would need a JSON round-trip. - The stale "cache_ttl has no spec-level maximum" comment in contextagent/handler.go is now correct (schema has maximum: 86400, verified against adcp/schemas/trusted-match/context-match-response.json). - Line-number reference to tmproto/types_gen.go:196 replaced with a symbol reference (grep-able across regenerations). Config surface addition: - ServerConfig.Cache.MaxEntries (int, JSON: max_entries) - TMP_ROUTER_CACHE_MAX_ENTRIES env var Pre-commit code review caught two docstring inaccuracies (fixed) and one asymmetry note on canonicalizeSellerForCache (documented).
9f81e08 to
125fdf9
Compare
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
1 similar comment
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
1 participant
Summary
Implements the router-side Context Match response cache defined in the spec at
docs/trusted-match/specification.mdx:747-756. Adds a per-provider cache keyed on{property_rid, placement_id, provider_id}, default 5-minute TTL, honoring provider-suppliedcache_ttlwhen present (clamped to the 24h schema ceiling).Last non-blocked pre-image-publish spec gap in the router series (PR-A #405 TLS, PR-C #407 registry sync, PR-E #409 latency+SNI regression tests — all merged).
Design notes worth calling out
cache_ttl=0treated as "use default", not "disable caching". Go'sint+omitemptyconflates absent-field with present-zero, so a Go-based provider can never explicitly emit0on the wire (json.Marshaldrops it). The majority case is genuinely absent → use default. Providers that need to force cache invalidation should returncache_ttl=1. Documented at thePutcall site. Open follow-up: file a spec issue asking for an explicitno_cache: truefield to remove the ambiguity for statically-typed impls.OffercarriesSellerAgent,Brand,Price,CreativeManifest, andMacros— all pointer/slice/map. The tmproto docstring onSellerAgentsays "the router MAY stamp this field from its cached package→seller map" — a shallow clone would let that stamp corrupt cache entries. Deep-clone removes the failure mode upfront. Regression test proves the isolation.package_idsper spec. The spec's cache-key definition explicitly omits it because "the same packages are evaluated for every user on a given placement." A publisher varyingpackage_idsfor the same{property, placement}violates that assumption — comment at the check site names it.Config surface
TMP_ROUTER_CACHE_DISABLEDfalseTMP_ROUTER_CACHE_DEFAULT_TTL_SECcache_ttl300JSON:
cache: { disabled: bool, default_ttl_seconds: int }onServerConfig.Metrics
tmp_context_cache_hits_total{provider}tmp_context_cache_misses_total{provider}Provider labels are bounded (spec limits
provider_idto^[A-Za-z0-9_]+$max 64), and only ever populated fromProviderSet.ID— never from request-derived strings.Test plan
go test ./router/... -race— passes; new tests cover nil-safety, deep-clone isolation, TTL expiration (injected clock), provider TTL override, TTL clamp to max, zero-TTL-uses-default, per-provider key partitioning, metrics counts, and 32-goroutine concurrent accessgo test ./cmd/router/... -race— passes; new env-merge testscache_ttloverride survives past the router's default TTLgolangci-lint run --new-from-rev origin/main— 0 issues on both modulesgo vetcleanRefs
adcp-gorouter to spec-conformance for community-facing OCI image publish