Scaffolding: Federation sync app creation#298
Conversation
klpoland
added
feature
klpoland
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
I'll add more comments after those are addressed to reduce the noise |
| asset=asset, | ||
| asset_type=asset_type, | ||
| ) | ||
| await push_asset_updated_to_peers(http, config, payload) |
There was a problem hiding this comment.
Stale skip still notifies peers
High Severity
After a Redis event, handle_redis_asset_event always calls push_asset_updated_to_peers, even when FederatedAssetIndexer.apply_asset_event returns early because the event is stale. Peers can receive deletes or older updates that the local indexer intentionally ignored, causing cross-site index drift (for example after bootstrap sets a newer in-memory event_at).
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d0608d2. Configure here.
| body = payload.model_dump(mode="json") | ||
| path = payload.asset_type.webhook_path | ||
| for peer in config.peers: | ||
| url = peer_webhook_url(peer, path) |
There was a problem hiding this comment.
Outbound sync ignores peer registry
Medium Severity
push_asset_updated_to_peers always uses static config.peers URLs from federation.toml, while site-hello updates PeerRegistry with each peer’s declared sync_service_url. Outbound webhooks never read the registry, so post-hello URL data is unused.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d0608d2. Configure here.
| def _is_stale(self, site_name: str, uuid: UUID, event_at: datetime) -> bool: | ||
| key = doc_id(site_name, uuid) | ||
| prev = self._last_event.get(key) | ||
| return bool(prev is not None and event_at <= prev) |
There was a problem hiding this comment.
Naive-aware datetime compare crash
Medium Severity
Stale-event dedupe compares event_at from Redis (datetime.fromisoformat without normalizing timezone) against bootstrap/webhook timestamps that are timezone-aware UTC. If the gateway publishes naive ISO timestamps, Python raises TypeError on comparison and can crash indexing or the Redis subscriber loop.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d0608d2. Configure here.
| config = request.app.state.config | ||
| allowed = {peer.name for peer in config.peers} | {config.site.name} | ||
| if payload.site_name not in allowed: | ||
| raise HTTPException(status_code=403, detail="Unknown origin site") |
There was a problem hiding this comment.
Peers may spoof local site
Medium Severity
Inbound asset webhooks allow payload.site_name to match the local config.site.name as well as configured peers. A peer (or any caller that can reach the route) can index documents under the home site identity, which local changes are meant to supply only via Redis and gateway export—not via peer POST bodies.
Reviewed by Cursor Bugbot for commit ce44ac9. Configure here.
| asset_type.value, | ||
| exc, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Bootstrap misses validation errors
Medium Severity
bootstrap_gateway_exports catches only httpx.HTTPError around fetch_peer_export_list. Pydantic validation failures, unexpected response shapes (TypeError), or ValueError from indexing propagate and can abort all of run_bootstrap, skipping later peer pulls and register_with_peers, with only a top-level lifespan log if the exception reaches startup.
Reviewed by Cursor Bugbot for commit ce44ac9. Configure here.
|
|
||
| app = FastAPI(title="SDS Federation Sync", root_path="/sync", lifespan=lifespan) | ||
| app.include_router(health_router) | ||
| app.include_router(webhooks_router, prefix=API_PREFIX) |
There was a problem hiding this comment.
Sync URL path mismatch
High Severity
Outbound peer webhooks target {sync_service_url}/api/v1/..., which includes a /sync segment when sync_service_url is configured as documented. The running app registers routes at /api/v1 and /health only; root_path="/sync" does not serve those paths. Requests to /sync/api/v1/... hit uvicorn without a matching route unless a proxy strips /sync, so peer POSTs can 404 while tests pass via a /sync mount.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3a7c399. Configure here.
| app.state.fed_indexer, | ||
| stop, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Bootstrap misses live Redis events
Medium Severity
Startup fully awaits federation bootstrap before creating the Redis subscriber task. Gateway events published on federation:events during bootstrap are never consumed, so metadata changes in that window can stay missing from the local index and peers until a later update.
Reviewed by Cursor Bugbot for commit 3a7c399. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 8 total unresolved issues (including 7 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5f40bf1. Configure here.
| def _is_stale(self, site_name: str, uuid: UUID, event_at: datetime) -> bool: | ||
| key = doc_id(site_name, uuid) | ||
| prev = self._last_event.get(key) | ||
| return bool(prev is not None and event_at <= prev) |
There was a problem hiding this comment.
Equal timestamps treated as stale
Medium Severity
Stale-event dedupe uses event_at <= prev, so an event with the same timestamp as the last applied one is skipped entirely. Bootstrap applies many documents with one shared event_at, so a later Redis or webhook update at that same instant can be dropped and never written to OpenSearch.
Reviewed by Cursor Bugbot for commit 5f40bf1. Configure here.
2 participants
In this PR:
federation/(v1) application with:services:bootstrap.py: for initialization of federated data from home site and peers (including when new peer is added)fed_index.py: for handling (external peer) asset indexing on webhook eventslocal_events.py: for handling redis dispatches to fetch local datapeer registry.py: for registering new peers and holding site information from site hellospeer_sync.py: for packaging and sending asset data to peer sites (through their webhook routes)routes:health.py: (stub for now) for pinging site health (is connection established?)webhooks.py: for receiving payloads FROM peers to index into LOCAL federated asset indices as well as asite-hellohook to confirm federation between peersschemasandmodels: for type checking and providing data structures to federation configs, events, and documents (how asset docs are represented byFederatedDatasetDoc,FederatedCaptureDocmay warrant further consideration re: anticipating schema flexibility)mainapp that subscribes to redis events for picking up signals from gateway (handled in PR Scaffolding: Federation gateway setup #299)Note
Medium Risk
Adds new cross-site sync and OpenSearch indexing with webhook allowlisting; operational correctness depends on gateway Redis events and export API contracts staying in sync with this service.
Overview
Introduces a new
federation/FastAPI sync service (mounted at/sync) that keeps federated dataset/capture metadata in sync across SDS sites.On startup it bootstraps by pulling gateway export lists for the local site and configured peers, indexing into OpenSearch (
fed-datasets/fed-captures), then registers with peers viasite-hello. A background Redis subscriber onfederation:events:{site}reacts to gateway change notifications: resolves public assets from the local gateway export API, updates the local federated index, and fans outdataset-updated/capture-updatedwebhooks to peers (optional peer CA for HTTPS).Inbound
/api/v1/webhook/*routes accept peer events only from allowlistedsite_namevalues, with Pydantic contracts aligned to gateway federation serializers./healthaggregates Redis, OpenSearch, subscriber, and gateway-export probes.Repo wiring adds federation-specific pre-commit (ruff/pyrefly/deptry), Docker Compose for local/prod sync containers, dev mTLS cert scripts, and a broad pytest suite including a two-site in-process mesh.
Reviewed by Cursor Bugbot for commit 5f40bf1. Bugbot is set up for automated code reviews on this repo. Configure here.