Parallelize quadrant fetching and add smart 429 retry#114
Parallelize quadrant fetching and add smart 429 retry#114dougborg wants to merge 2 commits intoFoggedLens:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Overpass-based node fetching performance and resilience by parallelizing quadrant fetches during splitting and adding adaptive rate-limit handling based on Overpass /api/status.
Changes:
- Parallelize quadrant fetching via
Future.wait()while limiting total concurrency with a resizable async semaphore. - Add
/api/statussupport inOverpassServiceto derive slot count and to poll until a slot is available for smarter 429 retries. - Add DI hooks and new unit tests covering splitting, semaphore init, and rate-limit behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/services/overpass_service.dart |
Injects HTTP client and adds /api/status helpers (getSlotCount, waitForSlot) to support adaptive concurrency + 429 retry. |
lib/services/node_data_manager.dart |
Introduces _AsyncSemaphore, rate-limit retry behavior, and parallel quadrant fetching in _fetchSplitAreas. |
lib/services/node_spatial_cache.dart |
Adds a forTesting() constructor to support test setup. |
test/services/node_data_manager_test.dart |
Adds extensive tests for splitting geometry, status parsing/polling, retries, and semaphore init deduplication. |
COMMENT |
Adds an unrelated note/snippet that doesn’t appear connected to the PR’s purpose. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
If Overpass /api/status ever returns Rate limit: 0 (or parsing yields a non-positive value), the semaphore would block all run() calls forever. Clamp both the constructor and resize() to a minimum of 1. Addresses Copilot review feedback on PR FoggedLens#114. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6b28f20 to
9a58036
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Introduces a unified service policy system (ServiceType, ServicePolicy, ServicePolicyResolver, ServiceRateLimiter) that resolves compliance rules per-URL, covering OSMF official services and third-party tile providers. Custom/self-hosted endpoints get permissive defaults. Key changes: - ServicePolicyResolver maps URLs to policies (OSM tile server, Nominatim, editing API, Overpass, Bing, Mapbox, custom) - Nominatim: adds 1-req/sec rate limiting and client-side result caching with 5-minute TTL as required by Nominatim usage policy - OSM tile server: blocks offline tile downloads (explicitly prohibited by tile usage policy) with user-facing dialog - Attribution dialog: adds tappable license link to openstreetmap.org/copyright when using OSM-based tile providers (ODbL requirement) - OSM editing API: enforces max 2 concurrent download threads via semaphore - 23 unit tests covering policy resolution, URL template parsing, custom overrides, rate limiting, and all policy values Builds on top of PR FoggedLens#123 (UserAgentClient) and PR FoggedLens#127 (NetworkTileProvider). Compatible with PR FoggedLens#114 (Overpass parallelization) — Overpass policy defers to NodeDataManager's _AsyncSemaphore. https://claude.ai/code/session_01XyRTrax1tmtjcuT7CMoJhD
9a58036 to
48e19ec
Compare
Introduces a unified service policy system (ServiceType, ServicePolicy, ServicePolicyResolver, ServiceRateLimiter) that resolves compliance rules per-URL, covering OSMF official services and third-party tile providers. Custom/self-hosted endpoints get permissive defaults. Key changes: - ServicePolicyResolver maps URLs to policies (OSM tile server, Nominatim, editing API, Overpass, Bing, Mapbox, custom) - Nominatim: adds 1-req/sec rate limiting and client-side result caching with 5-minute TTL as required by Nominatim usage policy - OSM tile server: blocks offline tile downloads (explicitly prohibited by tile usage policy) with user-facing dialog - Attribution dialog: adds tappable license link to openstreetmap.org/copyright when using OSM-based tile providers (ODbL requirement) - OSM editing API: enforces max 2 concurrent download threads via semaphore - 23 unit tests covering policy resolution, URL template parsing, custom overrides, rate limiting, and all policy values Builds on top of PR FoggedLens#123 (UserAgentClient) and PR FoggedLens#127 (NetworkTileProvider). Compatible with PR FoggedLens#114 (Overpass parallelization) — Overpass policy defers to NodeDataManager's _AsyncSemaphore. https://claude.ai/code/session_01XyRTrax1tmtjcuT7CMoJhD
48e19ec to
c901c8c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When Overpass queries hit the 50k node limit, _fetchSplitAreas() splits the area into 4 quadrants. Previously these were fetched sequentially — at max split depth (3) that's up to 64 serial HTTP requests, which is the primary cause of the slow "splitting" network status users report. This change: - Parallelizes quadrant fetches with Future.wait() instead of a for-loop - Adds a dynamic concurrency limiter (_AsyncSemaphore) sized from the Overpass /api/status slot count, so we never exceed the server's per-IP rate limit - Replaces the "sleep 30s and give up on 429" behavior with smart retry: polls /api/status until a slot is available (modeled after OSMnx and OSMPythonTools), then retries up to 2 times - Resizes the semaphore on retry with the latest observed slot count, adapting to changing server conditions Refactors for testability: - HTTP client injection in OverpassService (following RoutingService pattern) - DI constructors + forTesting() factories on NodeDataManager and NodeSpatialCache - splitBounds made static + @VisibleForTesting for direct unit testing 18 new tests covering: splitBounds geometry, getSlotCount/waitForSlot parsing, fetchWithSplitting (happy path, split, max depth, rate limit retry + cap), partial/total quadrant failure, recursive splitting, and semaphore init deduplication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the user pans/zooms mid-fetch, queued sub-requests now bail out instead of blocking the semaphore. Each getNodesFor() call increments _fetchGeneration; fetchWithSplitting checks _isStale(generation) at 6 cooperative checkpoints (before semaphore, inside lambda, before splitting, before/after waitForSlot, top of _fetchSplitAreas). Null generation (offline download, existing tests) is never stale, so there is zero regression risk for non-production-path callers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c901c8c to
9bebba6
Compare
Closes #109
Summary
Future.wait()instead of sequential for-loop — at max split depth (3), up to 64 requests can overlap instead of running one-by-one_AsyncSemaphore) sized from Overpass/api/statusslot count, so we never exceed the server's per-IP rate limit/api/statusuntil a slot is available, retries up to 2 times, and resizes the semaphore with the latest observed slot countWhat changed
lib/services/overpass_service.dartOverpassService({http.Client? client})) following theRoutingServicepattern — enables mock-based testinggetSlotCount()— queries/api/statusand parsesRate limit: N, falls back to 4waitForSlot()— polls/api/statusuntil"slots available now"appears, parsing"in N seconds"for smart delay, bounded bymaxWait(default 2 min)lib/services/node_data_manager.dartNodeDataManager._({OverpassService?, NodeSpatialCache?})+NodeDataManager.forTesting()factory_AsyncSemaphore— resizable async semaphore;resize()wakes queued waiters when capacity increases; capacity clamped to >= 1 to prevent deadlock; usesQueuefor O(1) dequeue_semaphoreInitFuturecaches the init Future so concurrent calls get the same instancefetchWithSplittingnow wrapsfetchNodesinsemaphore.run()and handlesRateLimitErrorby polling + retrying (max 2)_fetchSplitAreasnow usesFuture.wait()for parallel quadrant fetchingsplitBoundsrenamed from_splitBounds, madestatic+@visibleForTestingfor direct unit testing_fetchGenerationint incremented eachgetNodesFor()call_isStale(int? generation)checked at 6 cooperative checkpoints throughout the fetch pipeline (before semaphore, inside lambda, before splitting, before/afterwaitForSlot, top of_fetchSplitAreas)nullgeneration (offline download,map_data_provider.dart) is never stale — zero regression risklib/services/node_spatial_cache.dartNodeSpatialCache.forTesting()constructortest/services/node_data_manager_test.dart(new)23 tests using
mocktail+fake_async:FakeAsync)Design notes
The semaphore is safe without locks because Dart's event loop is single-threaded: microtasks from
Completer.complete()drain sequentially before new events can enterrun(). Each woken waiter re-checkswhile (_current >= _maxConcurrent)before proceeding./api/statusis not polled on every requestThe
/api/statusendpoint is only hit in two situations:getSlotCount()is called during semaphore initialization, and the result is cached via_semaphoreInitFuture ??=. All subsequent requests reuse the cached semaphore instance with no network call.waitForSlot()is called inside theon RateLimitErrorhandler, and only up to 2 times per request (capped byrateLimitRetries). This is where we poll until a slot opens up, then resize the semaphore with the fresh count.Stale fetch cancellation avoids ~95% of wasted work
We can't cancel in-flight HTTP requests (the
httppackage has noCancelToken), but the generation counter prevents queued/pending work from starting. Since only 4 requests run concurrently while up to 60 may be queued, this captures the vast majority of the benefit without changing the HTTP layer.Test plan
flutter analyze— 0 issuesflutter test— 23/23 pass in node_data_manager_testOverpass semaphore: N slotsWaiting N seconds for slotappears and request retries successfully🤖 Generated with Claude Code