Fix(raster): cancel in-flight raster tile requests when source URL changes - issue #7149 by pcardinal · Pull Request #7214 · maplibre/maplibre-gl-js

pcardinal · 2026-03-03T23:06:42Z

Launch Checklist

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Document any changes to public APIs.
Post benchmark scores.
Add an entry to CHANGELOG.md under the ## main section.

Summary

This PR fixes a raster source lifecycle issue where calling RasterTileSource.setUrl could leave old tile requests running.
When the URL changes, in-flight tile requests are now aborted deterministically through TileManager.abortAllRequests, with cache-side support in TileCache.
The change ensures in-flight raster requests are cancelled before reloading source metadata.

Root cause
When a raster source URL was updated, the source reload path did not consistently cancel all active requests tied to the previous URL/state.

Changes included

Updated RasterTileSource.setUrl to trigger request cancellation through the tile manager before reload.
Updated request-cancellation behavior in TileManager.abortAllRequests.
Updated cache-level request cancellation in TileCache.abortAllRequests to ensure out-of-view pending raster requests are aborted.

Tests added/updated

raster_tile_source.test.ts - setUrl aborts in-flight raster tile requests through TileManager
tile_manager.test.ts - coverage for TileManager.abortAllRequests
tile_cache.test.ts - coverage for TileCache.abortAllRequests

How to validate locally
npm run test-unit -- src/source/raster_tile_source.test.ts
npm run test-unit -- src/tile/tile_manager.test.ts
npm run test-unit -- src/tile/tile_cache.test.ts

Impact

No public API changes.
Internal behavior change only for raster request cancellation on URL update.
Prevents stale requests/flicker and reduces unnecessary network activity.

Issue
Fixes #7149

codecov · 2026-03-03T23:48:48Z

Codecov Report

❌ Patch coverage is 87.09677% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.73%. Comparing base (0cac8ee) to head (266ef76).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/source/vector_tile_source.ts	83.78%	6 Missing ⚠️
src/tile/tile_manager.ts	87.50%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7214      +/-   ##
==========================================
+ Coverage   92.45%   92.73%   +0.28%     
==========================================
  Files         288      288              
  Lines       24043    24095      +52     
  Branches     5093     5107      +14     
==========================================
+ Hits        22228    22344     +116     
+ Misses       1815     1751      -64

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

src/source/raster_tile_source.ts

pcardinal · 2026-03-07T15:38:40Z

Vector and raster tiles are adressed
If this works, the title of this issue may need to be changed.

src/tile/tile_manager.ts

src/source/vector_tile_source.ts

src/source/raster_tile_source.ts

HarelM · 2026-03-07T16:08:58Z

Thanks for the time spent on this!
I haven't reviewed the test code yet, let's make sure we address the production code comments first.

src/ui/events.ts

src/tile/tile_manager.ts

src/tile/tile_cache.ts

src/source/vector_tile_source.ts

src/tile/tile_manager.ts

HarelM · 2026-03-10T09:33:19Z

I haven't reviewed the tests yet as I want to make sure the business logic is correct first.
Once the code looks good I'll do a second review for the tests.
Thanks for taking the time to open this PR!

src/tile/tile_manager.ts

src/source/vector_tile_source.ts

HarelM · 2026-03-17T21:45:45Z

src/source/vector_tile_source.ts

+            }
+
+            // Keep all callers waiting for a reload while this tile is already loading.
+            // Resolve/reject cascades so every queued caller settles when the queued reload completes.


This is like the 3-5 time I'm reading this and it still isn't clear what's happening here.
I think the comment is explaining it , but I still find this code combersome.
Is there a better way to manage a queue instead of this obscure promise inside promise thing?
It's also might be better to change the name of tile.reloadPromise to something like reloadResolveReject as it's not really promise but something that can resolve or reject a promise, and I think it adds to the confusion (to be fair I think I gave it that "bad" name).

Thanks for calling this out. I agree the previous implementation was hard to read.

I refactored the reload-while-loading flow to use an explicit queue of waiters instead of chaining resolve/reject callbacks through nested promises. This makes the behavior easier to follow:

when a tile is already loading, each caller is added to a queued waiters list

when loading finishes, we run at most one queued reload

then we resolve or reject all queued callers together

I also renamed the tile field from reloadPromise to queuedReloadWaiters, since it stores resolver pairs rather than an actual Promise.

Behavior is unchanged, but the control flow and naming are now much clearer. I validated this with the targeted unit tests for both:

single reload while loading

multiple queued reload callers resolved by one queued worker reload

HarelM · 2026-03-17T21:48:33Z

src/source/vector_tile_source.ts

    }

    async abortTile(tile: Tile): Promise<void> {
+        tile.aborted = true;


I wonder why aborted is not a state of the tile and instead it's a boolean field, but that is probably outside the scope of this PR...

aborted is a boolean (not a TileState) because it represents a different axis of information than state:

state (state: TileState;) models data/render lifecycle, not request cancellation
It answers: “what data status does this tile currently have?” (loading, loaded, reloading, unloaded, errored, expired) in tile.ts:47.

aborted (aborted: boolean;) is a transient control flag for in-flight async work
It answers: “should the current async load result be ignored/cancelled?” It is checked right after worker response/error handling in vector_tile_source.ts:248 and vector_tile_source.ts:264, and set in abort path at vector_tile_source.ts:347.

A tile can keep a meaningful render state while a request is aborted
For example, when reloading an already renderable tile, aborting should not necessarily force a new lifecycle state; it may keep prior usable data and just stop/ignore the current fetch. The code explicitly only forces unloaded in some cases (e.g. when it had no renderable data before), see vector_tile_source.ts:249 and vector_tile_source.ts:265.

This pattern is used across the manager as request-level metadata
(method) TileManager.abortAllRequests(): void marks tile.aborted = true independently of lifecycle state in tile_manager.ts:136, which reinforces that cancellation is orthogonal to the main tile state machine.

So in short: state is “what the tile is,” aborted is “what to do with the current request.” Keeping them separate avoids state explosion and preserves correct rendering behavior during cancellations.

HarelM · 2026-03-17T21:50:14Z

src/source/vector_tile_source.ts

    async abortTile(tile: Tile): Promise<void> {
+        tile.aborted = true;
+        if (!tile.hasData()) {
+            tile.state = 'unloaded';


Wouldn't this be a reaction to the below abort call from the abort controller? Is this mandatory to set this?

Setting tile.state = 'unloaded' here is intentional and complementary to AbortController.abort(). The controller only cancels the in-flight async work; it does not guarantee the tile lifecycle state is updated consistently. Marking non-renderable tiles as unloaded ensures we do not keep a loading/aborted tile in an inconsistent state, while preserving renderable tiles (hasData()) to avoid flicker.

HarelM · 2026-03-17T21:57:45Z

src/tile/tile_manager.test.ts


    });

+    test('falls back to cache miss after out-of-view cache reset', () => {


I think this is another test not related to this PR, right?

Removed. This test seems unrelated to the PR scope since it exercises TileManager cache reset behavior on reload, not the abortPendingTileRequests path.

HarelM · 2026-03-17T21:58:28Z

src/tile/tile_manager.test.ts


+    test('calls abortTile before unloadTile for unfinished tile', () => {
+        const tileID = new OverscaledTileID(0, 0, 0, 0, 0);
+        const calls: string[] = [];


Same as previous comment with the spies.

which comment ?

HarelM · 2026-03-17T21:59:26Z

src/tile/tile_manager.test.ts

+        tileManager._paused = false;
+        tileManager.transform = new MercatorTransform() as any;
+
+        (tileManager as any)._dataHandler({dataType: 'source', abortPendingTileRequests: true});


Please use public APIs only, this as any is a sign of a bad test.

I cleaned up the two neighboring unit tests to use public APIs only and removed the private/internal access patterns introduced in this thread.

What was changed:

Refactored the test at tile_manager.test.ts:537 to stop calling private handlers and stop using internal state mutation.

Refactored the adjacent test at tile_manager.test.ts:571 with the same approach.

Replaced direct private calls and state poking with public behavior via source events:

onAdd(...)

update(transform)

getSource().fire(new Event('data', ...))

Validation:

Targeted test run for “ignores content events after abortPendingTileRequests until metadata arrives” passed.

Targeted test run for “forwards sourceDataChanged and shouldReloadTileOptions to reload” passed.

Scope note:

I only changed code authored in this conversation, as requested, and left other existing as any usages untouched.

HarelM · 2026-03-17T22:00:33Z

src/tile/tile_manager.test.ts


+    test('ignores content events after abortPendingTileRequests until metadata arrives', () => {
+        const tileManager = createTileManager({});
+        const abortAllSpy = vi.spyOn(tileManager, 'abortAllRequests');


I'm not sure that's the right way to test how this class behaves. It's best to either mock other classes this class uses or check events and other public APIs.

Thanks for the feedback. I updated the two latest tests to avoid asserting TileManager internals and to focus on observable behavior.

test('ignores content events after abortPendingTileRequests until metadata arrives'

test('forwards sourceDataChanged and shouldReloadTileOptions to reload

What changed:

Removed spies on TileManager methods (abortAllRequests, reload, update).

Kept event-driven setup through public source data events.

Asserted behavior through public/collaborator effects instead:

loaded() transitions after abortPendingTileRequests

reload behavior inferred from Source.loadTile call count changes

forwarding of shouldReloadTileOptions verified via Source.shouldReloadTile arguments

I reran the two updated tests, and both pass.

HarelM · 2026-03-17T22:01:12Z

src/tile/tile_manager.test.ts

+        expect(updateSpy).toHaveBeenCalledTimes(1);
+    });
+
+    test('forwards sourceDataChanged and shouldReloadTileOptions to reload', () => {


See my previous comment, this test mocks too much of the class under test.

see previous comments R568 and R576

HarelM · 2026-03-17T22:03:06Z

src/tile/tile_manager.test.ts

+        const nonLoadingTile = tileManager.addTile(new OverscaledTileID(1, 0, 1, 1, 1));
+        nonLoadingTile.state = 'loaded';
+
+        const abortTileSpy = vi.spyOn(tileManager._source, 'abortTile');


I would feel better to simply check the state of the tiles and avoid making sure some internal methods are called, as those can change in the future.

Done! I've refactored both tests to remove the internal spy assertions and focus only on observable behavior—the tile states.

The tests now:

First test: Directly assert that loadingTile.aborted is true and nonLoadingTile.aborted is not—checking only the public state changes

Second test: Assert that tile.aborted is true after calling abortAllRequests()—verifying the end result without coupling to internal method calls

This approach follows the project guidelines : "Prefer assertions on public behavior (emitted events, public state, return values) over private/internal method calls." The tests are now more resilient to future implementation refactors.

HarelM · 2026-03-17T22:04:18Z

src/tile/tile_manager.test.ts

+        tile.state = 'loaded';
+        tile.abortController = {abort: vi.fn()} as unknown as AbortController;
+
+        const abortTileSpy = vi.spyOn(tileManager._source, 'abortTile');


If the outcome is that the tile is aborted by calling abortAllRequests I think that's a good enough test, no need to drill down to the exact method that was called.

see previous comment "Comment on line R2711"

HarelM reviewed Mar 4, 2026

View reviewed changes

src/source/raster_tile_source.ts Outdated Show resolved Hide resolved

pcardinal force-pushed the issue_7149 branch from 98e1862 to 5246f5a Compare March 6, 2026 17:03

HarelM reviewed Mar 7, 2026

View reviewed changes

src/tile/tile_manager.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 7, 2026

View reviewed changes

src/tile/tile_manager.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 7, 2026

View reviewed changes

src/tile/tile_manager.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 7, 2026

View reviewed changes

src/source/vector_tile_source.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 7, 2026

View reviewed changes

src/source/vector_tile_source.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 7, 2026

View reviewed changes

src/source/vector_tile_source.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 7, 2026

View reviewed changes

src/source/vector_tile_source.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 7, 2026

View reviewed changes

src/source/raster_tile_source.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 7, 2026

View reviewed changes

src/source/raster_tile_source.ts Outdated Show resolved Hide resolved

pcardinal force-pushed the issue_7149 branch from 0415cfd to fc68d0f Compare March 10, 2026 00:45