Add ttl to fetch promises, check metadata rows before fetching #80
Conversation
WalkthroughThe changes in this pull request involve enhancements to error handling, request management, and variable ordering across four files: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Mosaic
participant Viewer
participant Fetcher
User->>Viewer: Move map
Viewer->>Mosaic: Update hash with new coordinates
Mosaic->>Fetcher: Request tile
Fetcher->>Fetcher: Check if request is in queue
alt Request in progress
Fetcher-->>Mosaic: Return existing tile
else New request
Fetcher->>Fetcher: Add request to queue with timeout
Fetcher-->>Mosaic: Return new tile
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range comments (2)
src/titiler_fetcher.mjs (1)
Line range hint
7-11: Consider adjusting concurrency based on timeout patterns.The combination of CPU-based concurrency (
numCPUs) and fixed timeouts might lead to suboptimal resource utilization. If requests frequently time out at 10 minutes, you might want to:
- Implement adaptive concurrency based on timeout patterns
- Add monitoring to correlate timeouts with concurrency levels
- Consider implementing a circuit breaker pattern for failing endpoints
This would help maintain optimal throughput while preventing resource exhaustion.
Also applies to: 37-38
src/mosaic_viewer.ejs (1)
Line range hint
57-59: Consider adding error handling for tile loading.Given that the PR aims to address 503/504 errors, consider adding error handling for the tile sources:
map.addSource("mosaic", { type: "raster", url: "<%= baseUrl + '/tiles/tilejson.json' %>", tileSize: 256, + maxRetries: 3, // Retry failed tile requests + timeout: 10000 // 10 second timeout per tile request }); map.addSource("mosaic-clusters", { type: "vector", tiles: ["<%= baseUrl + '/clusters/{z}/{x}/{y}.mvt' %>"], minzoom: 0, maxzoom: 30, + maxRetries: 3, + timeout: 10000 });Also consider adding error event listeners:
map.on('error', (e) => { console.error('Map error:', e.error); // Implement appropriate error handling/user notification });Also applies to: 64-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
src/mosaic.mjs(1 hunks)src/mosaic_viewer.ejs(2 hunks)src/titiler_fetcher.mjs(1 hunks)
🧰 Additional context used
🪛 Biome
src/mosaic.mjs
[error] 174-174: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
src/mosaic_viewer.ejs (1)
41-41: Verify the coordinate order change impact.
The change in coordinate order from [zoom, lng, lat] to [zoom, lat, lng] in URL hash parsing could potentially break bookmarked URLs or external systems relying on the previous order. However, this change aligns better with the common convention of specifying coordinates as (lat, lng).
✅ Verification successful
Coordinate order change is safe within this file's scope
The coordinate order change is localized to mosaic_viewer.ejs and maintains internal consistency:
- URL hash parsing:
[zoom, lat, lng](line 41) - Map initialization:
center: [lng, lat](line 45) - Hash generation:
#${zoom}/${lat}/${lng}(line 99)
No other files in the codebase reference or depend on this URL hash format, making this a safe, self-contained change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other files that might rely on the URL hash format
# Look for patterns that might parse or generate map URLs
# Search for URL hash patterns with coordinates
rg -A 2 "location.hash.*zoom.*lat.*lng" --type js --type html
# Search for mapboxgl map initialization patterns
rg "new mapboxgl.Map\(" -A 5 --type js --type html
Length of output: 625
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (1)
src/titiler_fetcher.mjs (1)
Inconsistent Error Handling for 503/504 Status Codes
The
fetchTileandfetchTileMetadatafunctions handle 404 and 500 errors by returning null but do not account for 503/504 errors. Please update the error handling to consistently manage these additional status codes.🔗 Analysis chain
Line range hint
16-35: Verify error handling consistency across fetch functionsBoth
fetchTileandfetchTileMetadatahave similar error handling patterns, but they might need to be updated to handle 503/504 errors consistently.Also applies to: 63-71
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent error handling patterns across fetch functions # Search for error handling patterns in fetch functions echo "Analyzing error handling patterns in fetch functions..." ast-grep --pattern 'catch (err) { if ($$$) { return null; } else { throw err; } }'Length of output: 219
🧰 Tools
🪛 Biome
[error] 37-37: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/titiler_fetcher.mjs(1 hunks)
🧰 Additional context used
🪛 Biome
src/titiler_fetcher.mjs
[error] 37-37: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🔇 Additional comments (1)
src/titiler_fetcher.mjs (1)
46-46:
Add specific error handling for timeout and HTTP errors
Given that this PR aims to address 503/504 errors, we should add specific error handling for both timeout and HTTP errors to:
- Properly log and monitor these specific error cases
- Potentially implement retry logic for recoverable errors
const request = tileRequestQueue
- .add(() => fetchTile(url), { priority: z, timeout: FETCH_QUEUE_TTL })
+ .add(() => fetchTile(url), { priority: z, timeout: FETCH_QUEUE_TTL })
+ .catch(error => {
+ if (error.name === 'TimeoutError') {
+ console.error(`Tile request timed out after ${FETCH_QUEUE_TTL}ms: ${url}`);
+ throw new Error(`Tile request timeout: ${url}`);
+ }
+ if (error.response?.statusCode === 503 || error.response?.statusCode === 504) {
+ console.error(`Server error ${error.response.statusCode} for URL: ${url}`);
+ throw new Error(`Server error ${error.response.statusCode}: ${url}`);
+ }
+ throw error;
+ })
.finally(() => {Likely invalid or redundant comment.
…g for UUID checks
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
src/mosaic.mjs(1 hunks)src/titiler_fetcher.mjs(2 hunks)
🧰 Additional context used
🪛 Biome
src/titiler_fetcher.mjs
[error] 37-37: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🔇 Additional comments (5)
src/titiler_fetcher.mjs (3)
46-46: Well-implemented timeout mechanism
The addition of the timeout parameter is a good solution for preventing hanging requests and addressing the 503/504 errors mentioned in the PR objectives.
37-37: 🧹 Nitpick (assertive)
Modernize parseInt usage for better consistency
Use Number.parseInt instead of the global parseInt for better alignment with ES2015+ practices.
-const FETCH_QUEUE_TTL = parseInt(process.env.TILE_FETCH_TTL_MS, 10) || 1000 * 60 * 10; // 10 minutes default
+const FETCH_QUEUE_TTL = Number.parseInt(process.env.TILE_FETCH_TTL_MS, 10) || 1000 * 60 * 10; // 10 minutes defaultLikely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 37-37: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
47-53: Verify the impact of silent failure on timeout
The error handling distinguishes between timeout and other errors, which is good. However, the current implementation silently continues execution after logging the error, potentially masking issues from upstream error handlers.
Let's verify how this error handling impacts the upstream code:
src/mosaic.mjs (2)
174-176: 🧹 Nitpick (assertive)
LGTM! Metadata row validation improved.
The optional chaining operator improves the safety of metadata fetching by properly handling null/undefined rows. This aligns with the PR objective of checking metadata rows before fetching.
However, consider adding error logging when skipping invalid rows to help with debugging:
if (row?.uuid) {
metadataByUuid[row.uuid] = await getGeotiffMetadata(row.uuid);
+ } else {
+ console.warn('Skipping invalid row: missing uuid', { row });
}Line range hint 154-164: Verify TTL implementation for request deduplication.
The request deduplication in requestCachedMosaic512px is good, but there's no visible TTL implementation. According to the PR objectives, we should add TTL to fetch promises. Consider:
- Adding a timeout to remove stale requests from
activeMosaicRequests - Implementing request cancellation for timed-out requests
Let's check if TTL is implemented elsewhere:
✅ Verification successful
TTL Implementation Confirmed
The TTL functionality for request deduplication is implemented in src/titiler_fetcher.mjs with FETCH_QUEUE_TTL.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for TTL implementation in related files
# Expected: Find TTL constants or timeout logic
# Search for TTL-related constants
rg -i "ttl|timeout|expire"
# Look for Promise timeout implementations
ast-grep --pattern 'Promise.race([
$_,
new Promise((_, reject) => setTimeout(reject, $_))
])'
Length of output: 998
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
README.md(1 hunks)src/titiler_fetcher.mjs(3 hunks)
🔇 Additional comments (4)
src/titiler_fetcher.mjs (3)
15-16: LGTM! Well-structured timeout configuration.
The implementation follows modern JavaScript practices and provides a reasonable default timeout value.
78-78: LGTM! Improved status code check.
The array includes method is the correct way to check for status codes.
21-21: LGTM! Verify timeout behavior across different network conditions.
The timeout configuration is correctly implemented using got's timeout object structure.
README.md (1)
123-140: LGTM! Good documentation of environment variables.
The new section clearly documents all environment variables, including the newly added timeout configurations that align with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/mosaic.mjs(2 hunks)
🔇 Additional comments (2)
src/mosaic.mjs (2)
174-176: LGTM - Optional chaining implementation
This improvement was already covered in a past review comment and has been implemented correctly.
228-229: Verify the impact of metadata filtering on mosaic generation
The addition of metadata filtering improves robustness but could affect mosaic generation if many tiles are filtered out. Let's verify the typical ratio of null metadata in the production environment.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Filtering null metadata is a valid and safe approach
The codebase shows a robust metadata handling system with proper error handling and caching:
- Null metadata only occurs in exceptional cases (404/500 errors from titiler)
- The system has built-in retry mechanisms and request queuing
- Metadata is cached and invalidated properly when images are deleted
- The filtering is part of a larger error-handling strategy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the frequency of null metadata in production
# Expected: Low percentage of null metadata to ensure mosaic quality
# Search for logging statements related to metadata
rg "metadata|getGeotiffMetadata" -A 5
# Look for error handling in metadata fetching
ast-grep --pattern 'async function getGeotiffMetadata($_) {
$$$
catch($_) {
$$$
}
}'
Length of output: 12466
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (1)
src/mosaic.mjs (1)
Line range hint
238-257: Validate metadata structure before sortingThe sorting logic assumes the existence of
uploaded_at,file_size, andgsdfields in metadata. Consider adding validation to prevent runtime errors:filteredTiles.sort((a, b) => { + // Validate required metadata fields + const validateMetadata = (meta) => { + return meta.uploaded_at && + typeof meta.file_size !== 'undefined' && + typeof meta.gsd !== 'undefined'; + }; + + if (!validateMetadata(a.meta) || !validateMetadata(b.meta)) { + console.warn('Invalid metadata structure detected, falling back to default order'); + return 0; + } + // 1. Prefer the latest image const dateA = new Date(a.meta.uploaded_at); const dateB = new Date(b.meta.uploaded_at);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
src/mosaic.mjs(2 hunks)src/titiler_fetcher.mjs(3 hunks)
🔇 Additional comments (3)
src/titiler_fetcher.mjs (2)
21-21: LGTM: Timeout configuration is properly implemented
The timeout configuration follows got's recommended structure for request timeouts.
50-50: Verify the impact of the priority calculation change
The priority calculation has been changed from z to Math.pow(2, z), which exponentially increases priority with zoom level. While this might be intentional, it represents a significant change in queue behavior.
Let's analyze the impact:
✅ Verification successful
Priority calculation change is intentional and well-documented
The change from priority: z to priority: Math.pow(2, z) was introduced in commit a5bf65d with the message "Adjust tile fetching priority calculation to use exponential scaling". This is an intentional improvement to the queue prioritization system:
- Higher zoom levels now get exponentially higher priority (e.g., zoom 15 gets priority 32768 vs zoom 14 with priority 16384)
- This ensures more detailed tiles (higher zooms) are fetched more aggressively
- The change was part of a larger mosaic rendering improvement PR (Improve mosaic rendering and fetching #79) that specifically mentioned "Add zoom priorities to fetch tiles queue, higher zoom has higher priority"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all other uses of the queue priority in the codebase
rg -l "priority.*z" --type js
# Look for any documentation or comments about queue priority
rg -l "priority.*zoom" --type md
Length of output: 66
Script:
#!/bin/bash
# Let's try a broader search for priority-related code in JavaScript files
rg "priority.*" --type js -A 2 -B 2
# Also search for queue-related configurations
rg "queue" --type js -A 2 -B 2
# Look for any configuration files that might define queue behavior
fd -e json -e js -e yaml -e yml | xargs rg "queue|priority" -A 2 -B 2
Length of output: 4253
Script:
#!/bin/bash
# Let's search specifically in the source files for tile fetching and queue initialization
rg "new PQueue|PQueue.*configure" --type js -A 5 -B 5
# Search for any comments or documentation about tile fetching priority
rg "tile.*priority|zoom.*priority" --type js -A 2 -B 2
# Look for the history of this file to understand the change
git log -p src/titiler_fetcher.mjs
Length of output: 11245
src/mosaic.mjs (1)
174-176: 🧹 Nitpick (assertive)
Consider adding TTL to metadata cache
While the null check is good, the PR objectives mention adding TTL to fetch promises. Consider implementing TTL for the metadata cache to prevent serving stale data.
Let's check if TTL is implemented elsewhere:
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range comments (2)
__tests__/mosaic.mjs (2)
Line range hint
179-278: Consider extracting snapshot paths to constantsThe snapshot file paths are hardcoded in multiple places. Consider extracting them to constants for better maintainability.
Consider creating a constants section at the top of the file:
const SNAPSHOT_PATHS = { MOSAIC_2X_14: './__tests__/[email protected]', MOSAIC_2X_13: './__tests__/[email protected]', MOSAIC_2X_12: './__tests__/[email protected]', MOSAIC_1X_15: './__tests__/[email protected]', MOSAIC_2X_11: './__tests__/[email protected]' };Then use these constants in the test cases:
if (process.env.UPDATE_SNAPSHOTS) { - await fs.promises.writeFile("./__tests__/[email protected]", tile.image.buffer); + await fs.promises.writeFile(SNAPSHOT_PATHS.MOSAIC_2X_14, tile.image.buffer); }
Line range hint
179-278: Consider creating a helper function for snapshot managementThe snapshot update logic is duplicated across multiple test cases. Consider extracting it to a helper function.
Consider creating a helper function:
async function updateSnapshot(path, buffer) { if (process.env.UPDATE_SNAPSHOTS) { await fs.promises.writeFile(path, buffer) .catch(err => console.error(`Failed to write snapshot to ${path}:`, err)); } }Then use it in the test cases:
- if (process.env.UPDATE_SNAPSHOTS) { - await fs.promises.writeFile("./__tests__/[email protected]", tile.image.buffer); - } + await updateSnapshot(SNAPSHOT_PATHS.MOSAIC_2X_14, tile.image.buffer);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (7)
__tests__/[email protected]is excluded by!**/*.png,!**/*.png__tests__/[email protected]is excluded by!**/*.png,!**/*.png[email protected]is excluded by!**/*.png,!**/*.png[email protected]is excluded by!**/*.png,!**/*.png[email protected]is excluded by!**/*.png,!**/*.png[email protected]is excluded by!**/*.png,!**/*.png[email protected]is excluded by!**/*.png,!**/*.png
📒 Files selected for processing (2)
README.md(1 hunks)__tests__/mosaic.mjs(3 hunks)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~140-~140: After the verb ‘keep’, the progressive form of ‘fetch’ should be used.
Context: ... FETCH_QUEUE_TTL_MS | Time to keep fetch promises in memory (ms). Older promises...
(KEEP_SEEING)
dqunbp
deleted the
20161-oam-mosaic-and-cluster-endpoints-give-503-504-answers
branch
https://kontur.fibery.io/Tasks/Task/oam-Mosaic-and-cluster-endpoints-give-503-504-answers-20161
Summary by CodeRabbit
New Features
OAM_LAYER_IDwith a default value ofopenaerialmap.Improvements