Merge pull request #176 from FoggedLens/no-429-splitting

No 429 splitting, bring back query reduction

DEVELOPER.md

@@ -297,26 +297,23 @@ These are internal app tags, not OSM tags. The underscore prefix makes this expl
 - **Sandbox (OSM API)**: Zoom ≥ 13 (stricter due to bbox limits)
 
 **Smart error handling & splitting:**
-- **50k node limit**: Automatically splits query into 4 quadrants, recursively up to 3 levels deep
-- **Timeout errors**: Also triggers splitting (dense areas with many profiles)  
-- **Rate limiting**: Extended backoff (30s), no splitting (would make it worse)
-- **Surgical detection**: Only splits on actual limit errors, not network issues
+- **50k node limit**: Automatically splits query into 4 quadrants, recursively up to 3 levels deep. This is a deterministic result-set size limit, so shrinking the queried area is the correct fix.
+- **Timeout errors**: Treated as a plain retryable network error — no splitting. Timeouts usually indicate server load or an expensive query, not "too much area", so firing off more sub-requests would only add load to an already-struggling server.
+- **Rate limiting (HTTP 429)**: Extended backoff (30s), no splitting (would make it worse). Still surfaces the same "slow" status indicator as the node-limit-splitting case, so the user sees consistent feedback either way.
+- **Surgical detection**: Only splits on the actual 50k node limit, not timeouts or other network issues.
 
-**Query optimization & deduplication:**
-- **Pre-fetch limit**: 4x user's display limit (e.g., 1000 nodes for 250 display limit)
-- **Profile deduplication**: Automatically removes redundant profiles from queries using subsumption analysis
-- **User-initiated detection**: Only reports loading status for user-facing operations  
-- **Background operations**: Pre-fetch runs silently, doesn't trigger loading states
 
-**Profile subsumption optimization (v2.1.1+):**
-To reduce Overpass query complexity, profiles are deduplicated before query generation:
+**Query optimization & deduplication (in `OverpassService._buildQuery`):**
+Profiles are deduplicated before query generation via subsumption analysis:
 - **Subsumption rule**: Profile A subsumes profile B if all of A's non-empty tags exist in B with identical values
 - **Example**: `Generic ALPR` (tags: `man_made=surveillance, surveillance:type=ALPR`) subsumes `Flock` (same tags + `manufacturer=Flock Safety`)
 - **Result**: Default profile set reduces from ~11 to ~2 query clauses (Generic ALPR + Generic Gunshot)
-- **UI unchanged**: All enabled profiles still used for post-query filtering and display matching
+- **UI unchanged**: All enabled profiles still used for post-query filtering and display matching (see `NodeProviderWithCache._matchesAnyProfile`), independent of which clauses were sent to Overpass
+- **Safety fallback**: if dedup would remove every profile (e.g. all profiles have only empty tags), the original full list is used instead of sending an empty query
 
 **Why this approach:**
-Dense urban areas (SF, NYC) with many profiles enabled can easily exceed both 50k node limits and 25s timeouts. Profile deduplication reduces query complexity by ~80% for default setups, while automatic splitting handles remaining edge cases. Surgical error detection avoids unnecessary API load from network issues.
+Dense urban areas (SF, NYC) with many profiles enabled can easily exceed both 50k node limits and 45s timeouts. Profile deduplication reduces query complexity by ~80% for default setups. Surgical error detection avoids unnecessary API load from network issues.
+
 
 ### 6. Uploader Service Architecture (Refactored v1.5.3)
 

lib/dev_config.dart

@@ -99,13 +99,10 @@ const Duration kMarkerTapTimeout = Duration(milliseconds: 250);
 const Duration kMapLongPressTimeout = Duration(milliseconds: 600); // Duration to trigger "add node here" on empty map area
 const Duration kDebounceCameraRefresh = Duration(milliseconds: 500);
 
-// Pre-fetch area configuration
-const double kPreFetchAreaExpansionMultiplier = 3.0; // Expand visible bounds by this factor for pre-fetching
 const double kNodeRenderingBoundsExpansion = 1.3; // Expand visible bounds by this factor for node rendering to prevent edge blinking
 const double kRouteProximityThresholdMeters = 500.0; // Distance threshold for determining if user is near route when resuming navigation
 const double kResumeNavigationZoomLevel = 16.0; // Zoom level when resuming navigation
-const int kPreFetchZoomLevel = 10; // Always pre-fetch at this zoom level for consistent area sizes
-const int kMaxPreFetchSplitDepth = 3; // Maximum recursive splits when hitting Overpass node limit
+
 
 // Data refresh configuration
 const int kDataRefreshIntervalSeconds = 60; // Refresh cached data after this many seconds
@@ -165,11 +162,11 @@ const int kAbsoluteMaxTileCount = 50000;
 const int kAbsoluteMaxZoom = 23;
 
 // Node icon configuration
-const double kNodeIconDiameter = 18.0;
-const double _kNodeRingThicknessBase = 2.5;
-const double kNodeDotOpacity = 0.3; // Opacity for the grey dot interior
+const double kNodeIconDiameter = 17.0;
+const double _kNodeRingThicknessBase = 2.6;
+const double kNodeDotOpacity = 0.28; // Opacity for the grey dot interior
 const Color kNodeRingColorReal = Color(0xFF3036F0); // Real nodes from OSM - blue
-const Color kNodeRingColorMock = Color(0xD0FFFFFF); // Add node mock point - white
+const Color kNodeRingColorMock = Color(0xE044BB55); // Add node mock point - white
 const Color kNodeRingColorPending = Color(0xD09C27B0); // Submitted/pending nodes - purple
 const Color kNodeRingColorEditing = Color(0xD0FF9800); // Node being edited - orange
 const Color kNodeRingColorPendingEdit = Color(0xD0757575); // Original node with pending edit - grey

lib/services/map_data_provider.dart

@@ -54,8 +54,8 @@ class MapDataProvider {
     required List<NodeProfile> profiles,
     UploadMode uploadMode = UploadMode.production,
     int maxResults = 0, // 0 = no limit for offline downloads
-    int maxTries = 3,
   }) async {
+
     if (AppState.instance.offlineMode) {
       throw OfflineModeException("Cannot fetch remote nodes for offline area download in offline mode.");
     }

lib/services/node_data_manager.dart

@@ -215,7 +215,18 @@ class NodeDataManager extends ChangeNotifier {
     }
   }
 
-  /// Fetch nodes with automatic area splitting if needed
+  /// Fetch nodes, splitting the query area into quadrants only when Overpass
+  /// reports the query would exceed its hard 50k node limit ([NodeLimitError]).
+  ///
+  /// Splitting is deliberately NOT used for other failure modes:
+  /// - [RateLimitError] (HTTP 429): the server explicitly asked us to slow
+  ///   down, so firing off up to 4x more requests would be counterproductive.
+  ///   We back off instead, while still surfacing the "slow" status
+  ///   indicator to the user (same UI feedback as the splitting case).
+  /// - Timeouts / other transient failures now surface as [NetworkError]
+  ///   (see [OverpassService]) and are not caught here at all — they're
+  ///   handled by the normal retry/backoff/fallback logic and reported as a
+  ///   plain timeout/error status by the caller.
   Future<List<OsmNode>> fetchWithSplitting(
     LatLngBounds bounds, 
     List<NodeProfile> profiles, {
@@ -238,7 +249,7 @@ class NodeDataManager extends ChangeNotifier {
       return nodes;
       
     } on NodeLimitError {
-      // Hit node limit or timeout - split area if not too deep
+      // Hit the deterministic 50k node limit - split area if not too deep
       if (splitDepth >= maxSplitDepth) {
         debugPrint('[NodeDataManager] Max split depth reached, giving up');
         return [];
@@ -254,13 +265,20 @@ class NodeDataManager extends ChangeNotifier {
       return _fetchSplitAreas(bounds, profiles, splitDepth + 1, isUserInitiated: isUserInitiated);
       
     } on RateLimitError {
-      // Rate limited - wait and return empty
-      debugPrint('[NodeDataManager] Rate limited, backing off');
+      // Rate limited - back off without splitting the area. Splitting here
+      // would multiply request volume against a server that just told us to
+      // slow down. We still show the same "slow" status indicator so the
+      // user gets feedback, but there is no recursive quadrant fetching.
+      debugPrint('[NodeDataManager] Rate limited, backing off (no area splitting)');
+      if (isUserInitiated) {
+        NetworkStatus.instance.setSplitting();
+      }
       await Future.delayed(const Duration(seconds: 30));
       return [];
     }
   }
 
+
   /// Fetch data by splitting area into quadrants
   Future<List<OsmNode>> _fetchSplitAreas(
     LatLngBounds bounds, 

lib/services/overpass_node_limit_exception.dart

@@ -1,23 +0,0 @@
-/// Exception thrown when Overpass API returns an error indicating too many nodes were requested.
-/// This typically happens when querying large areas that would return more than 50k nodes.
-class OverpassNodeLimitException implements Exception {
-  final String message;
-  final String? serverResponse;
-
-  OverpassNodeLimitException(this.message, {this.serverResponse});
-
-  @override
-  String toString() => 'OverpassNodeLimitException: $message';
-}
-
-/// Exception thrown when Overpass API rate limits the request.
-/// Should trigger longer backoff delays, not area splitting.
-class OverpassRateLimitException implements Exception {
-  final String message;
-  final String? serverResponse;
-
-  OverpassRateLimitException(this.message, {this.serverResponse});
-
-  @override
-  String toString() => 'OverpassRateLimitException: $message';
-}

lib/services/overpass_service.dart

@@ -32,7 +32,20 @@ class OverpassService {
   String get _primaryEndpoint => _endpointOverride ?? defaultEndpoint;
 
   /// Fetch surveillance nodes from Overpass API with retry and fallback.
-  /// Throws NetworkError for retryable failures, NodeLimitError for area splitting.
+  ///
+  /// Throws:
+  /// - [NodeLimitError] when the query would exceed Overpass's hard 50k node
+  ///   limit. This is the *only* case where the caller should split the
+  ///   query area into smaller regions, since it's a deterministic function
+  ///   of how much data lives in the requested bounds.
+  /// - [RateLimitError] when rate limited (HTTP 429 or equivalent). Callers
+  ///   should back off, not split/retry — splitting would only amplify load
+  ///   on a server that just told us to slow down.
+  /// - [NetworkError] for timeouts and other retryable HTTP/network failures.
+  ///   Timeouts are usually a sign of server load or an expensive query, not
+  ///   "too much area" — retrying with a hail of smaller sub-requests makes
+  ///   the problem worse, so timeouts are treated like any other transient
+  ///   network failure (handled by the existing retry/backoff/fallback logic).
   Future<List<OsmNode>> fetchNodes({
     required LatLngBounds bounds,
     required List<NodeProfile> profiles,
@@ -70,22 +83,16 @@ class OverpassService {
 
       final errorBody = response.body;
 
-      // Node limit error - caller should split area
+      // Node limit error - a deterministic result-set size limit. This is
+      // the one case where splitting the query area into smaller regions is
+      // the correct fix, since it directly reduces nodes-per-request.
       if (response.statusCode == 400 &&
           (errorBody.contains('too many nodes') && errorBody.contains('50000'))) {
-        debugPrint('[OverpassService] Node limit exceeded, area should be split');
+        debugPrint('[OverpassService] Node limit exceeded (50k), area should be split');
         throw NodeLimitError('Query exceeded 50k node limit');
       }
 
-      // Timeout error - also try splitting (complex query)
-      if (errorBody.contains('timeout') ||
-          errorBody.contains('runtime limit exceeded') ||
-          errorBody.contains('Query timed out')) {
-        debugPrint('[OverpassService] Query timeout, area should be split');
-        throw NodeLimitError('Query timed out - area too complex');
-      }
-
-      // Rate limit
+      // Rate limit - back off, don't split or hammer the server with more requests.
       if (response.statusCode == 429 ||
           errorBody.contains('rate limited') ||
           errorBody.contains('too many requests')) {
@@ -93,6 +100,17 @@ class OverpassService {
         throw RateLimitError('Rate limited by Overpass API');
       }
 
+      // Timeout / runtime limit exceeded - treat as a plain retryable network
+      // error. This is usually server load or query cost, not "area too
+      // big" — splitting into up to 64 sub-requests would only make load
+      // on the server worse.
+      if (errorBody.contains('timeout') ||
+          errorBody.contains('runtime limit exceeded') ||
+          errorBody.contains('Query timed out')) {
+        debugPrint('[OverpassService] Query timed out');
+        throw NetworkError('Query timed out');
+      }
+
       throw NetworkError('HTTP ${response.statusCode}: $errorBody');
     } catch (e) {
       if (e is NodeLimitError || e is RateLimitError || e is NetworkError) {
@@ -108,9 +126,24 @@ class OverpassService {
     return ErrorDisposition.retry;
   }
 
-  /// Build Overpass QL query for given bounds and profiles
+  /// Build Overpass QL query for given bounds and profiles.
+  ///
+  /// Profiles are deduplicated first: if one profile's non-empty tags are a
+  /// subset of another's (with identical values), the broader profile's query
+  /// clause already returns every node the narrower one would, so the
+  /// narrower clause is redundant and can be dropped. This directly reduces
+  /// query size/complexity without changing the result set — client-side
+  /// filtering (matching returned nodes against the full profile list) still
+  /// happens independently, so nothing is lost.
   String _buildQuery(LatLngBounds bounds, List<NodeProfile> profiles) {
-    final nodeClauses = profiles.map((profile) {
+    final profilesToQuery = _deduplicateProfilesForQuery(profiles);
+
+    if (profilesToQuery.length < profiles.length) {
+      debugPrint(
+          '[OverpassService] Deduplicated ${profiles.length} profiles to ${profilesToQuery.length} for query efficiency');
+    }
+
+    final nodeClauses = profilesToQuery.map((profile) {
       // Convert profile tags to Overpass filter format, excluding empty values
       final tagFilters = profile.tags.entries
           .where((entry) => entry.value.trim().isNotEmpty)
@@ -131,6 +164,74 @@ out ids;
 ''';
   }
 
+  /// Deduplicate profiles for Overpass queries by removing profiles that are
+  /// subsumed by others. A profile A subsumes profile B if all of A's
+  /// non-empty tags exist in B with identical values — meaning every node
+  /// matching B's filter also matches A's filter, so B's clause is redundant.
+  List<NodeProfile> _deduplicateProfilesForQuery(List<NodeProfile> profiles) {
+    if (profiles.length <= 1) return profiles;
+
+    final result = <NodeProfile>[];
+
+    for (final candidate in profiles) {
+      // Skip profiles that only have empty tags - they would match
+      // everything and shouldn't be treated as subsuming or subsumable.
+      final candidateNonEmptyTags = candidate.tags.entries
+          .where((entry) => entry.value.trim().isNotEmpty)
+          .toList();
+
+      if (candidateNonEmptyTags.isEmpty) continue;
+
+      // Check if any existing profile in our result subsumes this candidate
+      bool isSubsumed = false;
+      for (final existing in result) {
+        if (_profileSubsumes(existing, candidate)) {
+          isSubsumed = true;
+          break;
+        }
+      }
+
+      if (!isSubsumed) {
+        // This candidate is not subsumed, so add it. But first, remove any
+        // existing profiles that this candidate subsumes (candidate is
+        // broader, so those clauses are now redundant).
+        result.removeWhere((existing) => _profileSubsumes(candidate, existing));
+        result.add(candidate);
+      }
+    }
+
+    // Safety check: if dedup removed everything (e.g. all-empty-tag
+    // profiles), fall back to the original list rather than sending an
+    // empty query.
+    return result.isNotEmpty ? result : profiles;
+  }
+
+  /// Check if [broaderProfile] subsumes [specificProfile]: true if all
+  /// non-empty tags in [broaderProfile] exist in [specificProfile] with
+  /// identical values.
+  bool _profileSubsumes(NodeProfile broaderProfile, NodeProfile specificProfile) {
+    final broaderTags = Map.fromEntries(
+      broaderProfile.tags.entries.where((entry) => entry.value.trim().isNotEmpty),
+    );
+    final specificTags = Map.fromEntries(
+      specificProfile.tags.entries.where((entry) => entry.value.trim().isNotEmpty),
+    );
+
+    // If broader has no non-empty tags, it doesn't subsume anything (it
+    // would match everything, which isn't a meaningful "broader filter").
+    if (broaderTags.isEmpty) return false;
+
+    // If broader has more non-empty tags than specific, it can't subsume it.
+    if (broaderTags.length > specificTags.length) return false;
+
+    for (final entry in broaderTags.entries) {
+      if (specificTags[entry.key] != entry.value) return false;
+    }
+
+    return true;
+  }
+
+
   /// Parse Overpass JSON response into OsmNode objects
   List<OsmNode> _parseResponse(String responseBody) {
     final data = jsonDecode(responseBody) as Map<String, dynamic>;
@@ -173,7 +274,9 @@ out ids;
   }
 }
 
-/// Error thrown when query exceeds node limits or is too complex - area should be split
+/// Error thrown when a query would exceed Overpass's 50k node limit.
+/// The caller should split the query area into smaller regions to resolve this
+/// — it's a deterministic function of how much data lives within the bounds.
 class NodeLimitError extends Error {
   final String message;
   NodeLimitError(this.message);
@@ -189,7 +292,7 @@ class RateLimitError extends Error {
   String toString() => 'RateLimitError: $message';
 }
 
-/// Error thrown for network/HTTP issues - retryable
+/// Error thrown for network/HTTP issues (including timeouts) - retryable
 class NetworkError extends Error {
   final String message;
   NetworkError(this.message);

test/services/overpass_service_test.dart

@@ -99,6 +99,129 @@ void main() {
     });
   });
 
+  group('profile deduplication (subsumption)', () {
+    test('a specific profile subsumed by a generic one is dropped', () async {
+      final generic = NodeProfile(
+        id: 'generic-alpr',
+        name: 'Generic ALPR',
+        tags: const {
+          'man_made': 'surveillance',
+          'surveillance:type': 'ALPR',
+        },
+      );
+      final specific = NodeProfile(
+        id: 'flock',
+        name: 'Flock',
+        tags: const {
+          'man_made': 'surveillance',
+          'surveillance': 'public',
+          'surveillance:type': 'ALPR',
+          'surveillance:zone': 'traffic',
+          'camera:type': 'fixed',
+          'manufacturer': 'Flock Safety',
+          'manufacturer:wikidata': 'Q108485435',
+        },
+      );
+
+      stubOverpassResponse([]);
+
+      await service.fetchNodes(bounds: bounds, profiles: [generic, specific]);
+
+      final captured = verify(
+        () => mockClient.post(any(), body: captureAny(named: 'body')),
+      ).captured;
+      final query = (captured.last as Map<String, String>)['data']!;
+
+      // Only the generic clause should remain.
+      expect(query, contains('["man_made"="surveillance"]["surveillance:type"="ALPR"]'));
+      expect(query, isNot(contains('manufacturer')));
+      expect(query, isNot(contains('Flock Safety')));
+
+      // Only one node clause total.
+      final nodeClauseCount = RegExp(r'node\[').allMatches(query).length;
+      expect(nodeClauseCount, equals(1));
+    });
+
+
+    test('unrelated profiles are all kept', () async {
+      final alprGeneric = NodeProfile(
+        id: 'generic-alpr',
+        name: 'Generic ALPR',
+        tags: const {
+          'man_made': 'surveillance',
+          'surveillance:type': 'ALPR',
+        },
+      );
+      final gunshotGeneric = NodeProfile(
+        id: 'generic-gunshot',
+        name: 'Generic Gunshot Detector',
+        tags: const {
+          'man_made': 'surveillance',
+          'surveillance:type': 'gunshot_detector',
+        },
+      );
+
+      stubOverpassResponse([]);
+
+      await service.fetchNodes(bounds: bounds, profiles: [alprGeneric, gunshotGeneric]);
+
+      final captured = verify(
+        () => mockClient.post(any(), body: captureAny(named: 'body')),
+      ).captured;
+      final query = (captured.last as Map<String, String>)['data']!;
+
+      expect(query, contains('["surveillance:type"="ALPR"]'));
+      expect(query, contains('["surveillance:type"="gunshot_detector"]'));
+    });
+
+    test('full default profile set reduces to the two generic clauses', () async {
+      final profiles = NodeProfile.getDefaults();
+
+      stubOverpassResponse([]);
+
+      await service.fetchNodes(bounds: bounds, profiles: profiles);
+
+      final captured = verify(
+        () => mockClient.post(any(), body: captureAny(named: 'body')),
+      ).captured;
+      final query = (captured.last as Map<String, String>)['data']!;
+
+      final nodeClauseCount = RegExp(r'node\[').allMatches(query).length;
+      expect(nodeClauseCount, equals(2));
+
+      expect(query, contains('["man_made"="surveillance"]["surveillance:type"="ALPR"]'));
+      expect(query, contains('["man_made"="surveillance"]["surveillance:type"="gunshot_detector"]'));
+      // Brand-specific tags should not appear anywhere in the reduced query.
+      expect(query, isNot(contains('manufacturer')));
+      expect(query, isNot(contains('ShotSpotter')));
+    });
+
+    test('profiles with only empty tags do not subsume or break the query', () async {
+      final emptyTagsProfile = NodeProfile(
+        id: 'empty',
+        name: 'Empty',
+        tags: const {'camera:mount': ''},
+      );
+      final normalProfile = NodeProfile(
+        id: 'normal',
+        name: 'Normal',
+        tags: const {'man_made': 'surveillance'},
+      );
+
+      stubOverpassResponse([]);
+
+      await service.fetchNodes(bounds: bounds, profiles: [emptyTagsProfile, normalProfile]);
+
+      final captured = verify(
+        () => mockClient.post(any(), body: captureAny(named: 'body')),
+      ).captured;
+      final query = (captured.last as Map<String, String>)['data']!;
+
+      expect(query, contains('["man_made"="surveillance"]'));
+    });
+  });
+
+
   group('response parsing — constraint detection', () {
     test('nodes referenced by a way are constrained', () async {
       stubOverpassResponse([
@@ -255,27 +378,28 @@ void main() {
       );
     });
 
-    test('response with "timeout" throws NodeLimitError', () async {
+    test('response with "timeout" throws NetworkError (not split)', () async {
       stubErrorResponse(400, 'runtime error: timeout in query execution');
 
       await expectLater(
         () => service.fetchNodes(
             bounds: bounds, profiles: profiles, policy: const ResiliencePolicy(maxRetries: 0)),
-        throwsA(isA<NodeLimitError>()),
+        throwsA(isA<NetworkError>()),
       );
     });
 
-    test('response with "runtime limit exceeded" throws NodeLimitError',
+    test('response with "runtime limit exceeded" throws NetworkError (not split)',
         () async {
       stubErrorResponse(400, 'runtime limit exceeded');
 
       await expectLater(
         () => service.fetchNodes(
             bounds: bounds, profiles: profiles, policy: const ResiliencePolicy(maxRetries: 0)),
-        throwsA(isA<NodeLimitError>()),
+        throwsA(isA<NetworkError>()),
       );
     });
 
+
     test('HTTP 429 throws RateLimitError', () async {
       stubErrorResponse(429, 'Too Many Requests');