[Sirajmx]

Problem

GET /products (and everything reading the catalog) served the static 13-product default catalog even after a real ad-server sync (GAM/FreeWheel/S3) had run. ProductSetupFlow.state.products is per-flow-instance and never outlives the request, and the catalog service's in-memory cache is per-worker-process (infra/docker/Dockerfile runs uvicorn with --workers 2), so at most one of two worker processes ever saw synced data. Docs claimed GAM/FreeWheel support that the code didn't deliver.

Fix

Three layers, all required together for a real GAM deployment to actually work:

1. Catalog-serving layer — synced products now persist under a dedicated synced_product:{id} storage key (deliberately separate from the generic product:{id} key negotiation_service/media_kit_service already write, which is a narrower proposal-time snapshot) and are served fresh from storage on every catalog read when a real ad server is configured, matching how synced packages already worked. Stale products no longer re-seeded by a sync run are pruned instead of lingering as phantom inventory. CSV-mode and no-ad-server-configured behavior is unchanged.
2. GAM client layer — GAMRestClient.connect() called googleapiclient.discovery.build("admanager", "v1", ...) with no discoveryServiceUrl. GAM self-hosts its own discovery document rather than registering centrally, so every real connection raised UnknownApiNameOrVersion and silently degraded to mocks. Fixed by pointing build() at admanager.googleapis.com/$discovery/rest?version=v1 directly.
3. Deployment layer — infra/docker/Dockerfile only installed the redis extra; the gam extra (googleads/google-api-python-client/google-auth) was never in the shipped production image, so even with (1) and (2) fixed, the container would ImportError before ever reaching GAM. Fixed by installing ".[redis,gam]".

All three verified together end to end against a real GAM network with real service-account credentials, rebuilding the real Dockerfile from scratch with zero manual installs: list_inventory() returned 100 real ad units, POST /packages/sync completed with zero warnings, GET /products served 50 real synced products.

Not yet verified: FreeWheel's real-data path — no real FreeWheel credentials available. Its list_inventory() looks architecturally sound (real OAuth 2.1 MCP tool call) but unproven live.

Reconciliation with main

This branch had drifted behind main. Merged origin/main in, resolving conflicts in CHANGELOG.md, interfaces/api/deps.py, interfaces/api/main.py, interfaces/cli/main.py, and services/catalog_service.py — all were additive overlaps (new unrelated routes/fixes landing alongside this branch's own catalog work), not competing logic changes.

Reconciling against main's newer default_price_floor_cpm setting (AI-8) surfaced one real gap: tests/unit/test_synced_product_catalog.py's hand-rolled settings fixture predated that field, so the second sync in the prune test crashed before pruning ever ran — masking as a "pruning doesn't work" failure when the actual cause was an unrelated settings-fixture gap. Fixed by adding the field to the fixture.

Testing

• Full suite green post-merge: 1726 unit+integration+smoke tests passed, 28 skipped (env-gated), 0 failed.
• Booted the real server and confirmed /health and /products respond correctly post-merge.
• No merge-conflict markers remain; diffed the merge result against an independent resolution to confirm consistency.

[Kishore-2572]

Live-tested the safety-critical part given this codebase's history with #100: simulated an ad-server call failing mid-fetch with a real product already persisted — confirmed the real product survives, the failure path never reaches the pruning logic.

Closed the cross-repo gap live: drove the real unmodified persistence/serving functions with realistic fake GAM inventory into a real DB, started a real server against it, confirmed GET /products returns real synced products not the static catalog, then got a correct quote through real buyer-agent code end to end.

GAM discovery fix and Dockerfile gam extra both confirmed in code. Full suite: 1726 passed, 0 failed, matches the PR's numbers. Approving on substance — no blocking issues found.

[aleksUIX]

A successful sync that returns no inventory deletes the stored products, and the next catalog read serves the static defaults again. sync_from_ad_server still calls _finish_sync on that path, the prune removes every synced_product key, and an empty key set is treated as never synced. I ran a synthetic GAM sync: one real product was served, then an empty success made GET /products return the 13 demo products. A failed sync keeps the real product. Can we record a completed sync, including an empty one, and return an empty catalog?

[Sirajmx]

Good catch, thanks for catching this! Fixed — a sync completing with zero items now serves an honest empty catalog instead of falling back to the demo one, via a synced_catalog:completed marker tracking whether a real sync has ever run.

Also found and fixed a related issue while verifying: a sync that crashes partway through persisting items was leaving the already-persisted new items in storage despite being reported as failed. Now rolled back precisely on failure.

Both covered by new tests, verified end-to-end. Commits: 4751ceb, dec2b49.

[aleksUIX]

An empty successful sync now stays empty. A finished real sync sets synced_catalog:completed, including when the ad server returns no items, and the catalog read returns that empty catalog instead of the static defaults. The new test covers a one-product sync followed by an empty success.

[atc964]

Really solid work, Siraj. The three-layer diagnosis (catalog read path, GAM discovery URL, missing Docker extra) is convincing, and verifying all three together against a live GAM network from a scratch-built image is exactly the standard we want. I checked the Redis backend's keys() prefix handling specifically since the in-memory test double would mask a prefix bug there, and it's clean; hybrid routing also puts synced_product:* in Postgres, which is the right durability call. The rollback tests proving prior-value restoration (not blind deletion) are a nice touch.

One fix I'd like before merge, three things to track:

1. (this PR) The _finish_sync docstring says it "runs unconditionally, including... (or the real sync raised)" and that a transient failure "reverts the catalog to the honest static-default fallback", but the except block in sync_from_ad_server returns before _finish_sync, and its own comment says the opposite (correctly!). The implemented keep-last-good behavior is the right one; the docstring describes the failure mode this PR exists to prevent. Could you reword it so the next reader doesn't "fix" the code to match it?

2. (decision/follow-up) This PR is the moment real floors start flowing to anonymous surfaces: GET /products publishes ext.floor_cpm and the ungated MCP list_products returns floor_cpm directly, and with #92 clamping quotes to the floor, that's publishing the seller's reserve price. The exposure predates your change (CSV mode already does it), so it doesn't block here, but we should make a deliberate call (keep as published rate card vs trim floor_cpm from public mappers) before a real network runs in prod. We're tracking the decision on our side.

3. (follow-up) CSV mode still goes down the real sync path, so a CSV-mode sync persists synced_product:* keys and sets the never-cleared completed marker that the CSV read path ignores. If a deployment later switches to GAM/S3, those dormant CSV-era products get served as the synced catalog until the first sync under the new config, and with the scheduler off by default that window is unbounded. Skipping persist/marker when ad_server_type == "csv" would close it cheaply.

4. (follow-up, noting only) With two workers and the scheduler enabled, both workers sync on the same interval from boot, and the new rollback can restore "prior" values captured before a concurrent run's writes. It self-heals next cycle, but a lock: key (hybrid already routes those to Redis) or jitter would make it boring. Related small one: the synced read path is keys() plus one get per product per catalog read, the Redis backend uses KEYS rather than SCAN/MGET, and ProductDefinition(**data) will 500 every catalog endpoint if a single stored record ever drifts from the model; a per-key try/log/skip would degrade more gracefully.

Also a two-second one: the media_kit.py comment saying the catalog "is never persisted to storage" is now stale.

Approving with the docstring fix. The multi-worker read-consistency design is right: fresh storage read per call, no cache to invalidate, every consumer goes through the one accessor. I grepped main for bypasses and found none.

[Kishore-2572]

Thanks Siraj, I re-reviewed 8c1bfa6 end to end: a real seller server with SQLite, syncs through the real ProductSetupFlow, and a quote from real buyer-agent code. The suite passes (1689; 1782 merged with main).

Verified: an empty sync serves an empty catalog, mid-loop crashes roll back to prior values, products removed upstream are pruned, a failed sync keeps the last good catalog, and buyer-agent lists and quotes synced products. The docstring fixes are in.

Blocking

GAM syncs only the first 100 ad units, and the prune deletes the rest. GAMAdServerClient.list_inventory calls list_ad_units(page_size=100) once and drops the next-page token. With the real adapter and 150 ad units, the sync requested 1 page, kept 100 products and pruned the 50 previously synced ones. The tests use _FakeAdServerClient, so they can't catch it. Please follow nextPageToken until it's empty.

Should fix (here or tracked)

• One stored record that no longer matches ProductDefinition returns 500 on both GET /products and POST /api/v1/quotes. Suggest skipping and logging per record.
• Every synced GAM product is priced at default_price_floor_cpm ($10 base / $8.50 floor), with inventory_type guessed from the name.
• A failure in the packages loop leaves that run's new products stored, while the sync is reported as failed.

atc964's open follow-ups still apply: the floor is shown to anonymous callers, CSV mode persists synced keys, and two workers can race.

[Sirajmx]

Thanks Kishore, confirmed both and fixed in 712ee92 and 1da6d9f.

• GAM paging: list_inventory dropped nextPageToken. That predates this PR, but fix: real, storage-backed catalog for synced ad servers (AI-6) #117 is what makes it the served catalog. It now pages until the token runs out, the sync requests the whole inventory, and that also lifts the same 100-row cap on S3. Tests cover multi-page reads, limit across pages and a repeated token.
• Packages-loop failure: this run's products are now rolled back too, with a regression test.
• Default pricing and the type guessed from the name: these are by design for GAM, since ad units carry no price (AI-8). Operators set real prices with the rate card (Rate card is writable but is not read by the quote, deal-booking or negotiation pricing paths #69) and correct the type with the AI-14 override.
• Per-record skip on drifted records: that one goes with atc964's follow-ups.

[Kishore-2572]

Re-reviewed at 67be66c and tested end to end on a real seller server (SQLite), with syncs through the real ProductSetupFlow and real adapters. Approving.

Verified

Item	Result
GAM paging	250 ad units → all 3 pages read, 250 products stored; a failure on a later page keeps the last good catalog; a repeating page token terminates
Packages-loop failure	This run's products are rolled back; only the prior catalog remains
CSV sync (limit=None)	Real server, CSV mode: /packages/sync → synced, inventory-sync trigger → success (15 items)
CSV catalog past 100 rows	Real server, 150-row CSV: GET /products → 150, row 150 served and quotable
Empty sync / failed sync	Empty catalog served honestly; a failed sync keeps the last good catalog
Cross-repo	Real buyer-agent code lists synced products and gets a quote

Negative controls on the new tests (paging, packages rollback, CSV limit=None, CSV catalog) all fail when the fix is reverted.

CI

• All 5 checks green on 67be66c, including Docker Build.
• Merged with current main: no conflicts; 1797 unit + 39 integration tests pass on Python 3.11 and 3.12; ruff check and ruff format --check clean.