Published before it was finished

Background

router = APIRouter()

@router.get("/items/{id}")
def read_item(id: int):
    return {"id": id}

app = FastAPI()
app.include_router(router, prefix="/api")

The cached object lives on a small internal class called _IncludedRouter: a dataclass, one instance per included router, holding the flattened list plus the version that list was built for. It has two methods that build and cache such a list, effective_candidates for ordinary routes and effective_low_priority_routes for routes that are tried last. Both changed in the same way, and both share one lock.

fastapi/routing.py:1571

@dataclass
class _IncludedRouter(BaseRoute):
    original_router: "APIRouter"
    include_context: _RouterIncludeContext

The version is not a plain counter on one router. It is a sum over the subtree, so a change to any nested router changes the version every ancestor sees.

fastapi/routing.py:2530

    def _get_routes_version(self, seen: set[int] | None = None) -> int:
        if seen is None:
            seen = set()
        router_id = id(self)
        if router_id in seen:
            return self._routes_version
        seen.add(router_id)
        version = self._routes_version
        for route in self.routes:
            if isinstance(route, _IncludedRouter):
                version += route.original_router._get_routes_version(seen)
        return version

Six methods bump a router's own counter: add_route, add_websocket_route, add_api_route, add_api_websocket_route, include_router, and frontend, which mounts a static frontend build as low-priority routes. All six are construction-time APIs. Nothing on the request path bumps the counter, which is why a retained cache is built once, on the first request, and then left alone.

Here is the rebuild before the change. Three steps, and every one of them writes to shared state.

fastapi/routing.py, before the change

        self._effective_candidates = []
        candidates = self.original_router.routes
        for route in candidates:
            ...
            self._effective_candidates.append(route_context)
        self._effective_candidates_version = routes_version
        return self._effective_candidates

Intuition

The pull request body is two sentences and a link. The reasoning is in the linked discussion, and the reported symptoms go past "two threads corrupted a list".

The discussion is titled "Router-tree lazy rebuild (0.137.0+) is not thread-safe: valid routes can 404 under concurrent first requests". Its author reports three effects. The first is the one that reaches a user. The third column is my reading of how long each lasts, not something the report states.

For the second effect the reporter counted 283, 166, 226, 247, and 375 cached entries across five runs of an application with 120 routes. The list never shrinks back, because nothing rebuilds it once the version has been stamped.

How two threads get there

sequenceDiagram
    autonumber
    participant A as Thread A
    participant C as the cache. shared state
    participant B as Thread B

    Note over A,B: before the fix. no lock. list published empty
    A->>C: version differs. start rebuilding
    A->>C: self._effective_candidates = []
    B->>C: version still differs. start rebuilding too
    A->>C: append route 1 of 120
    B->>C: self._effective_candidates = []
    Note over C: Thread A's work is discarded mid flight
    B->>C: append route 1 of 120
    A->>C: append into the list B just replaced
    A->>C: set version to current
    B->>C: reads a list that is neither complete nor its own
    B--)B: a valid route 404s

Step 8 is the one to look at. Thread A stamps the version while thread B is still appending. From then on the version says current, so every later reader takes the fast path and trusts a list that two threads are still writing to. Step 10 is what a user sees.

Two changes, and which one is the fix

The diff does two separable things, and they are easy to conflate.

The first is a lock, with the version re-checked after acquiring it. That is the standard double-checked pattern: the check outside the lock keeps a warm cache from paying for synchronization at all. It is not free, since computing the version walks the subtree, but it costs no lock.

The second is that the list is built into a local variable and assigned to the attribute in one statement, once complete. This is the change that fixes the bug, and the reason is that a reader on the fast path never takes the lock. A lock alone would still leave that reader looking at a partially filled list. Building locally means the attribute only ever holds a finished list.

Before: the container is shared while filling

After: the container is private while filling

Code walkthrough

The flow, end to end. A request needs the flattened routes. The method compares the subtree version against the cached one. On a match it returns the cached list and stops. On a mismatch it takes the lock, checks again in case another thread just did the work, builds a fresh list locally, publishes it, stamps the version, and returns what it built.

Two files: the fix, and a test file that gains two tests.

1. The whole method

fastapi/routing.py:1587

    def effective_candidates(self) -> list["_EffectiveRouteContext | _IncludedRouter"]:
        routes_version = self.original_router._get_routes_version()
        if routes_version == self._effective_candidates_version:
            return self._effective_candidates
        with self._effective_routes_lock:
            routes_version = self.original_router._get_routes_version()
            if routes_version == self._effective_candidates_version:
                return self._effective_candidates
            effective_candidates: list[_EffectiveRouteContext | _IncludedRouter] = []
            for route in self.original_router.routes:
                # elided: builds a child branch for a nested router, or an
                # effective route context for a plain route
                ...
            self._effective_candidates = effective_candidates
            self._effective_candidates_version = routes_version
            return effective_candidates

Four things to name: the duplicated check, the local list, the publish-then-stamp order, and the fact that the method returns the local rather than reading the attribute back.

2. The lock field

fastapi/routing.py:1575

+    _effective_routes_lock: Any = field(
+        default_factory=threading.Lock, repr=False, compare=False
+    )

default_factory gives each instance its own lock, so two routers never contend. The two keyword arguments matter because the bare @dataclass generates both a __repr__ and an __eq__.

repr=False keeps the lock out of the string form, which would otherwise print a memory address that changes every run. compare=False matters more: the generated __eq__ compares the tuple of fields, and a threading.Lock only ever equals itself. Left in, two _IncludedRouter instances with identical routers and contexts would always compare unequal, because their locks are different objects.

3. Checking the version twice

The check at the top runs without the lock, and the same check runs again immediately after acquiring it. The two do different jobs. The first is the fast path: a warm cache computes the subtree version, compares it, and returns, without touching the lock. The second covers the window between failing the first check and getting the lock, during which another thread may have finished the whole rebuild.

Without the second check, every thread that queued on the lock would rebuild the list again in turn, each one throwing away the work of the thread before it.

4. Publish, then stamp

This is the ordering from the edge case above, and it is the reason the unlocked fast path is safe. The invariant the code maintains is: if the version says current, the list is complete.

The first three rows are states this code produces. The fourth is the counterfactual, and the only one where the reader has no way back.

5. Returning the local

Both methods end with return effective_candidates, the local variable, rather than return self._effective_candidates.

At this commit that makes no observable difference. The return value is evaluated while the thread still holds the lock, and both writes to those attributes happen under that same lock, so no other thread can have replaced either one in between. Returning the local would still hold if something later wrote the attribute without the lock. Whether that was the reason, the record does not say.

6. The nested-router asymmetry

Inside the rebuild, a nested router becomes a fresh _IncludedRouter object, constructed on the spot. The two methods then do opposite things with it, so read this before either one.

effective_candidates appends the child into the list it publishes, so the child survives. Callers reach it through the parent's cached list and call its own effective_candidates, which fills the child's own cache. So a nested router's cache is reused across requests. It is discarded whenever the parent rebuilds, because the parent then constructs a new child object, and the old one becomes garbage along with its warm cache and its lock.

effective_low_priority_routes does not keep the child at all. It builds one, calls the child's method once, extends the flattened result into its own list, and drops the child. So that recursion always runs against a cold cache.

So one nested router is represented by more than one _IncludedRouter. Two persist: include_router appends one into the parent's routes, which is the object the isinstance check reads during a rebuild, and the rebuild then constructs a second and stores it in the parent's candidate cache. A third exists only while a low-priority rebuild is in flight, and is dropped when it ends. None of them share a cache or a lock.

That is also why the recursion cannot deadlock. threading.Lock is not reentrant, so a thread that holds one and tries to acquire the same one again blocks forever. Here the parent holds its own lock while the child acquires the child's, and those are different objects. The throwaway child's lock is one no other thread can reach at all.

7. The two tests

tests/test_router_include_context.py:903

def test_included_router_candidate_cache_is_thread_safe():
    router = APIRouter()
    route_count = 120
    thread_count = 6

The test uses the same shape as the discussion's example code, 120 routes and six threads, driven through the internal API instead of test clients. It releases the six threads simultaneously through a threading.Barrier, and each calls effective_candidates() on a cold cache. Then four assertions, at lines 929 to 932:

    assert len(results) == thread_count
    assert all(result is results[0] for result in results)
    assert len(results[0]) == route_count
    assert TestClient(app).get("/items/0").json() == {"index": 0}

The second is the interesting one. It uses is, not ==, so it demands that all six threads received the same list object. The third pins the count at exactly 120, which is the assertion the duplication would break. The fourth checks that routing still works afterwards.

On the pre-fix code this test can pass. It is a real race and nothing in it forces a schedule, so a run where the threads do not interleave would report green. The discussion's author ran the equivalent script and recorded corruption in five runs out of five, with the counts listed above, which is the only frequency evidence either the report or this page has.

tests/test_router_include_context.py:935

def test_included_router_low_priority_cache_rechecks_version_after_lock(monkeypatch):

The second test targets the branch that only exists because of this change: the early return inside the lock. It holds the lock on the main thread, starts a worker, waits on an Event to prove the worker has already failed the outside check, then sets the version to current and releases the lock. The worker acquires it, re-checks, finds the version now matches, and returns early.

That test fails deterministically on the pre-fix code, and not because of timing: the lock attribute does not exist there, so reaching for it raises AttributeError on every run.

The file holds 40 tests in total, counted with grep -c "^def test_\|^async def test_".

8. What the change does not do

The discussion's third symptom, leaking global warning filters, is not fixed here. Building a model field opens warnings.catch_warnings(), which saves the process-global filter list and restores it on exit. Overlapping builds save and restore each other's copies, so the last restore wins and entries leak.

Serializing one router's rebuild removes those overlapping builds. It does not make catch_warnings thread-safe, and model fields are built on other paths too. The maintainer wrote in the discussion: "For the warnings, it's also pretty much harmless and that will be refactored either way so it should be fine."

Quiz

Five questions about why the change is shaped the way it is. Click an option to see the answer.