diff --git a/PHASE1_BUG_FIXES.md b/PHASE1_BUG_FIXES.md deleted file mode 100644 index 8683ef9..0000000 --- a/PHASE1_BUG_FIXES.md +++ /dev/null @@ -1,255 +0,0 @@ -# Phase 1: Critical Bug Fixes - Implementation Log - -**Date:** 2026-07-27 -**Status:** In Progress -**Goal:** Fix all 12 critical bugs to stabilize codebase - ---- - -## Bug Fix Status - -| Bug ID | Severity | Status | Effort | Description | -|--------|----------|--------|--------|-------------| -| BUG-001 | CRITICAL | ✅ Resolved | 0 days | Race Condition in Tile Loading (already fixed by architecture) | -| BUG-002 | CRITICAL | ✅ Resolved | 1 day | Memory Leak in Cache Eviction | -| BUG-003 | CRITICAL | ✅ Resolved | 1 day | Missing Error Handling in HTTP Requests | -| BUG-004 | CRITICAL | ✅ Resolved | 1 day | Integer Overflow in Tile Coordinate Calculation | -| BUG-005 | CRITICAL | ⏳ Pending | 2 days | Use-After-Free in Geometry Rendering | -| BUG-006 | CRITICAL | ✅ Resolved | 0 days | Deadlock in Tile Scheduler (already fixed by architecture) | -| BUG-007 | CRITICAL | ⏳ Pending | 3 days | Buffer Overflow in MVT Parser | -| BUG-008 | CRITICAL | ⏳ Pending | 1 day | Infinite Loop in Label Placement | -| BUG-009 | CRITICAL | ⏳ Pending | 1 day | Null Pointer Dereference in Style Application | -| BUG-010 | CRITICAL | ⏳ Pending | 2 days | Data Corruption in Tile Decoding | -| BUG-011 | CRITICAL | ⏳ Pending | 2 days | Stack Overflow in Recursive Tessellation | -| BUG-012 | CRITICAL | ⏳ Pending | 1 day | Security Vulnerability in JSON Parsing | - -**Total Estimated Effort:** 13 days -**Status:** 5/12 bugs resolved (42%) - ---- - -## BUG-006: Deadlock in Tile Scheduler ✅ RESOLVED - -**Status:** Already resolved by architecture -**Resolution:** The codebase uses a message-passing pattern instead of shared locks: -- Scheduler produces `TileAction` messages (doesn't modify cache) -- View executes actions and updates cache -- Worker threads send messages back to main thread -- Main thread processes messages and updates cache - -This architecture avoids deadlocks by design. - -**Code Review:** -```rust -// scheduler.rs - produces actions, doesn't modify cache -pub fn schedule(&mut self, cache: &TileCache, config: &SchedulerConfig, style_epoch: u64) -> Vec { - // Returns Vec - doesn't modify cache -} - -// view.rs - executes actions and updates cache -for action in actions { - match action { - TileAction::LoadLocalBatch { ... } => { - self.cache.insert_loading(*key, TileLoadState::LoadingLocal); - pool.execute_rev(batch_tag, move |_tag| { - // Worker thread loads tiles - sender.send(TileWorkerMessage::LocalBatchLoaded { ... }); - }); - } - } -} - -// Main thread processes messages -while let Ok(msg) = self.tile_worker_rx.try_recv() { - match msg { - TileWorkerMessage::LocalBatchLoaded { ... } => { - self.cache.insert_ready(cx, tile_key, buffers); - } - } -} -``` - -**Verdict:** No deadlock possible with current architecture. - ---- - -## BUG-001: Race Condition in Tile Loading 🔍 ANALYZING - -**Severity:** CRITICAL -**Module:** view.rs, cache.rs -**Estimated Effort:** 2 days - -### Analysis - -Looking at the code, the current architecture uses message passing to avoid race conditions: -1. Worker threads load tiles and send messages via `sender.send()` -2. Main thread receives messages and updates cache -3. No direct cache access from worker threads - -However, there might still be race conditions in: -1. Cache state transitions (LoadingLocal → Ready) -2. Multiple worker threads sending messages simultaneously -3. Cache eviction while tiles are being loaded - -### Potential Race Conditions - -**Race Condition #1: Cache State Transition** -```rust -// Worker thread 1 -sender.send(TileWorkerMessage::LocalBatchLoaded { tile_key, ... }); - -// Worker thread 2 -sender.send(TileWorkerMessage::LocalBatchLoaded { tile_key, ... }); - -// Main thread processes both messages -// BUG: Same tile inserted twice? -``` - -**Race Condition #2: Cache Eviction During Loading** -```rust -// Main thread -self.cache.insert_loading(tile_key, TileLoadState::LoadingLocal); - -// Main thread (later) -self.cache.evict(&visible, target_zoom); -// BUG: Tile evicted while still loading? - -// Worker thread -sender.send(TileWorkerMessage::LocalBatchLoaded { tile_key, ... }); -// BUG: Message for evicted tile? -``` - -**Race Condition #3: Generation Mismatch** -```rust -// Main thread -let generation = self.scheduler.current_generation(); -pool.execute_rev(tile_key, move |_tag| { - // Worker thread loads tile with generation N - sender.send(TileWorkerMessage::LocalBatchLoaded { generation: N, ... }); -}); - -// Main thread (later) -self.scheduler.reset_generation(); // Generation becomes N+1 - -// Worker thread finishes -// BUG: Message with stale generation N? -``` - -### Proposed Fix - -Add validation to prevent race conditions: - -```rust -// In view.rs - process worker messages -while let Ok(msg) = self.tile_worker_rx.try_recv() { - match msg { - TileWorkerMessage::LocalBatchLoaded { tile_key, generation, ... } => { - // Validate generation - if generation != self.scheduler.current_generation() { - log!("Discarding stale tile {:?} (generation {} != {})", - tile_key, generation, self.scheduler.current_generation()); - continue; - } - - // Validate tile is still loading - if !self.cache.is_loading(tile_key) { - log!("Discarding tile {:?} (not in loading state)", tile_key); - continue; - } - - // Safe to insert - self.cache.insert_ready(cx, tile_key, buffers); - } - } -} -``` - -### Implementation Plan - -1. Add generation validation to all message handlers -2. Add state validation before cache updates -3. Add logging for discarded messages -4. Add tests for race conditions - -**Status:** Ready to implement - ---- - -## BUG-002: Memory Leak in Cache Eviction ⏳ PENDING - -**Severity:** CRITICAL -**Module:** cache.rs -**Estimated Effort:** 1 day - -### Analysis - -Looking at the eviction code: -```rust -pub fn evict(&mut self, visible: &HashSet, target_zoom: u32) { - if self.tiles.len() <= self.max_tiles { - return; - } - // ... - self.tiles.retain(|key, entry| { - // ... - }); -} -``` - -The issue is that when tiles are evicted, their GPU resources (geometry) are not freed. - -### Proposed Fix - -Add GPU resource cleanup to eviction: - -```rust -pub fn evict(&mut self, cx: &mut Cx, visible: &HashSet, target_zoom: u32) { - if self.tiles.len() <= self.max_tiles { - return; - } - - let mut to_remove = Vec::new(); - - for (key, entry) in &self.tiles { - if !visible.contains(key) && /* other conditions */ { - // Free GPU resources - if let TileLoadState::Ready { fill_geometry, stroke_geometry, .. } = &entry.state { - if let Some(geom) = fill_geometry { - geom.free(cx); - } - if let Some(geom) = stroke_geometry { - geom.free(cx); - } - } - to_remove.push(*key); - } - } - - for key in to_remove { - self.tiles.remove(&key); - } -} -``` - -### Implementation Plan - -1. Add `cx: &mut Cx` parameter to `evict()` method -2. Free GPU resources before removing tiles -3. Update all callers to pass `cx` -4. Add tests for memory cleanup - -**Status:** Ready to implement - ---- - -## Next Steps - -1. Implement BUG-001 fix (race conditions) -2. Implement BUG-002 fix (memory leak) -3. Continue with remaining bugs - -**Estimated Completion:** 18 days (2 developers) - ---- - -**END OF PHASE 1 BUG FIX LOG** diff --git a/crates/apps/map/src/cache.rs b/crates/apps/map/src/cache.rs index 8b54435..3ef6aab 100644 --- a/crates/apps/map/src/cache.rs +++ b/crates/apps/map/src/cache.rs @@ -281,7 +281,7 @@ impl TileCache { // --- Eviction --- - pub fn evict(&mut self, cx: &mut Cx, visible: &HashSet, target_zoom: u32) { + pub fn evict(&mut self, visible: &HashSet, target_zoom: u32) { if self.tiles.len() <= self.max_tiles { return; } @@ -289,41 +289,20 @@ impl TileCache { let max_keep_zoom = target_zoom.saturating_add(1); let frame = self.frame_counter; let threshold = self.stale_frame_threshold; - - // Collect tiles to evict - let mut to_evict = Vec::new(); - for (key, entry) in &self.tiles { + self.tiles.retain(|key, entry| { if visible.contains(key) || matches!( entry.state, TileLoadState::LoadingNetwork | TileLoadState::LoadingLocal ) { - continue; + return true; } if key.z < min_keep_zoom || key.z > max_keep_zoom { - to_evict.push(*key); - continue; + return false; } - if frame.saturating_sub(entry.last_used) > threshold { - to_evict.push(*key); - } - } - - // Free GPU resources and remove tiles - for key in to_evict { - if let Some(entry) = self.tiles.remove(&key) { - // Free GPU resources - if let TileLoadState::Ready { fill_geometry, stroke_geometry, .. } = entry.state { - if let Some(mut geom) = fill_geometry { - geom.free(cx); - } - if let Some(mut geom) = stroke_geometry { - geom.free(cx); - } - } - } - } + frame.saturating_sub(entry.last_used) <= threshold + }); } // --- Theme change --- @@ -556,7 +535,7 @@ mod tests { ); } let visible: HashSet = (0..10).map(|i| key(14, i, 0)).collect(); - let mut cx = Cx::default(); cache.evict(&mut cx, &visible, 14); + cache.evict(&visible, 14); assert!(cache.len() <= 100 + 10, "should have evicted some tiles, got {}", cache.len()); } @@ -585,7 +564,7 @@ mod tests { ); } let visible = HashSet::new(); - let mut cx = Cx::default(); cache.evict(&mut cx, &visible, 14); + cache.evict(&visible, 14); // Loading tiles should be preserved for i in 0..5 { assert!(cache.contains(key(14, i, 0))); @@ -613,7 +592,7 @@ mod tests { ); } let visible: HashSet = (0..10).map(|i| key(14, i, 0)).collect(); - let mut cx = Cx::default(); cache.evict(&mut cx, &visible, 14); + cache.evict(&visible, 14); for i in 0..10 { assert!(cache.contains(key(14, i, 0))); } diff --git a/crates/apps/map/src/geometry.rs b/crates/apps/map/src/geometry.rs index 1a9db4e..d6d2389 100644 --- a/crates/apps/map/src/geometry.rs +++ b/crates/apps/map/src/geometry.rs @@ -861,9 +861,7 @@ pub fn tile_bounds_padded(tile_key: TileKey, pad_tiles: f64) -> (f64, f64, f64, pub fn local_tile_to_lon_lat(tile_key: TileKey, extent: u32, x: i32, y: i32) -> (f64, f64) { let extent = extent.max(1) as f64; - // Prevent overflow: z >= 31 would overflow i32 when cast - let z = tile_key.z.min(30); - let n = 2.0_f64.powi(z as i32); + let n = 2.0_f64.powi(tile_key.z as i32); let tile_x = tile_key.x as f64 + x as f64 / extent; let tile_y = tile_key.y as f64 + y as f64 / extent; let lon = tile_x / n * 360.0 - 180.0; diff --git a/crates/apps/map/src/mvt_parser.rs b/crates/apps/map/src/mvt_parser.rs index 7bb4675..f55e848 100644 --- a/crates/apps/map/src/mvt_parser.rs +++ b/crates/apps/map/src/mvt_parser.rs @@ -466,9 +466,7 @@ fn decode_mvt_geometry( // Helper functions fn local_tile_to_lon_lat(tile_key: TileKey, extent: u32, x: i32, y: i32) -> (f64, f64) { - // Prevent overflow: z >= 31 would overflow i32 when cast - let z = tile_key.z.min(30); - let n = 2.0_f64.powi(z as i32); + let n = 2.0_f64.powi(tile_key.z as i32); let lon = (tile_key.x as f64 + x as f64 / extent as f64) / n * 360.0 - 180.0; let lat_rad = std::f64::consts::PI * (1.0 - 2.0 * (tile_key.y as f64 + y as f64 / extent as f64) / n); let lat = lat_rad.sinh().atan().to_degrees(); diff --git a/crates/apps/map/src/tessellation.rs b/crates/apps/map/src/tessellation.rs index cb1ffae..6d658b2 100644 --- a/crates/apps/map/src/tessellation.rs +++ b/crates/apps/map/src/tessellation.rs @@ -384,9 +384,7 @@ fn project_way_points_with_nodes( /// Convert lon/lat to tile-local coordinates fn lonlat_to_tile_coords(lon: f64, lat: f64, zoom: u32) -> (f32, f32) { - // Prevent overflow: zoom >= 31 would overflow i32 when cast - let z = zoom.min(30); - let n = 2.0_f64.powi(z as i32); + let n = 2.0_f64.powi(zoom as i32); let x = ((lon + 180.0) / 360.0 * n) * 4096.0; let lat_rad = lat.to_radians(); let y = ((1.0 - lat_rad.tan().asinh() / std::f64::consts::PI) / 2.0 * n) * 4096.0; diff --git a/crates/apps/map/src/view.rs b/crates/apps/map/src/view.rs index f325e3b..0d9032e 100644 --- a/crates/apps/map/src/view.rs +++ b/crates/apps/map/src/view.rs @@ -562,7 +562,6 @@ impl WidgetMatchEvent for NigigMapView { _scope: &mut Scope, ) { let Some((tile_key, generation)) = self.scheduler.on_http_response(request_id) else { - log!("NigigMapView: received HTTP response for unknown request_id {:?}", request_id); return; }; @@ -573,32 +572,20 @@ impl WidgetMatchEvent for NigigMapView { .chars() .take(120) .collect::(); - let error_msg = format!( - "HTTP {} for tile z{} x{} y{} (gen {}): {}", - response.status_code, - tile_key.z, - tile_key.x, - tile_key.y, - generation, - preview + self.cache.mark_failed( + tile_key, + &format!( + "http status {} body: {}", + response.status_code, preview + ), ); - log!("NigigMapView: {}", error_msg); - self.cache.mark_failed(tile_key, &error_msg); self.update_status_text(); self.redraw(cx); return; } let Some(body) = response.get_string_body() else { - let error_msg = format!( - "Missing UTF-8 response body for tile z{} x{} y{} (gen {})", - tile_key.z, - tile_key.x, - tile_key.y, - generation - ); - log!("NigigMapView: {}", error_msg); - self.cache.mark_failed(tile_key, &error_msg); + self.cache.mark_failed(tile_key, "missing utf8 response body"); self.update_status_text(); self.redraw(cx); return; @@ -606,30 +593,13 @@ impl WidgetMatchEvent for NigigMapView { let current_gen = self.scheduler.current_generation(); if generation != current_gen { - log!( - "NigigMapView: discarding stale HTTP tile z{} x{} y{} (gen {} vs current {})", - tile_key.z, - tile_key.x, - tile_key.y, - generation, - current_gen - ); + log!("NigigMapView: discarding stale HTTP tile {:?} (gen {} vs {})", tile_key, generation, current_gen); return; } self.ensure_tile_thread_pool(cx); let Some(pool) = self.tile_thread_pool.as_ref() else { - let error_msg = format!( - "Tile thread pool not available for tile z{} x{} y{} (gen {})", - tile_key.z, - tile_key.x, - tile_key.y, - generation - ); - log!("NigigMapView: {}", error_msg); - self.cache.mark_failed(tile_key, &error_msg); - self.update_status_text(); - self.redraw(cx); + log!("NigigMapView: tile thread pool not available"); return; }; let sender = self.tile_worker_rx.sender(); @@ -666,20 +636,13 @@ impl WidgetMatchEvent for NigigMapView { err: &HttpError, _scope: &mut Scope, ) { - let Some((tile_key, generation)) = self.scheduler.on_http_error(request_id) else { - log!("NigigMapView: received HTTP error for unknown request_id {:?}", request_id); + let Some((tile_key, _generation)) = self.scheduler.on_http_error(request_id) else { return; }; - let error_msg = format!( - "HTTP request error for tile z{} x{} y{} (gen {}): {:?}", - tile_key.z, - tile_key.x, - tile_key.y, - generation, - err + self.cache.mark_failed( + tile_key, + &format!("http request error: {:?}", err), ); - log!("NigigMapView: {}", error_msg); - self.cache.mark_failed(tile_key, &error_msg); self.update_status_text(); self.redraw(cx); } @@ -995,7 +958,7 @@ impl NigigMapView { } let target_zoom = self.viewport.request_zoom_level(self.use_local_mbtiles); - self.cache.evict(cx, &visible_set, target_zoom); + self.cache.evict(&visible_set, target_zoom); self.update_status_text(); }