Investigate memory leaks in Cloudflare

[JPeer264]

Description

There are quite some memory leaks in Cloudflare. One of the biggest one is that the CloudflareClient can't be garbage collected and stays in memory forever, this is actually working as expected, as usually we only have one client and it is usually never removed.

In Cloudflare Workers we actually don't know how long isolates live. So when a lot of requests come in rather fast the request is actually done, but the client is still active, which takes some memory - this could lead to OOM of the worker, as it is limited currently to 128MB per worker.

Possible solution

We don't know how long an isolate will live, but we do now how long a request takes (as we can hook into all waitUntil). As an isolate can't be reused it is guaranteed that a new request will spawn a new isolate and therefore a new client. With that it is safe to assume that we can mark an client as "disposed" so it can be garbage collected.

Repro

https://github.com/JPeer264/temp-cloudflare-leak

There is a reproduction showing the memory leak over time. There is also an experimental fix applied, which disposes clients.

[linear]

JS-1785

[jasnell]

... it is guaranteed that a new request will spawn a new isolate and therefore a new client

This is not correct. We do not spawn new isolates for every request and a worker should never be written to depend on whenever we might spawn a new isolate.

In production, any single isolate can serve multiple requests. The infrastructure can spin up new copies of the worker as necessary based on the workload being handled at any given time but that's dynamic and not really observable.

but we do now how long a request takes (as we can hook into all waitUntil). ...

Not really sure hat the as we can hook into all waitUntil means here. The waitUntil mechanism does not tell you how long a request takes. All it allows you to do is temporarily extend the lifespan of the request context to wait for some post-request tasks to finish. But that time period is finite and if the tasks do not complete within a define time period they are dropped entirely and the request context is destroyed. This has nothing to do with the isolate lifetime.

A worker isolate will generally only go away if there's a fatal error or a resource constraint (like the isolate heap memory limit) is exceeded. This is not somethng that is predictable or should ever be relied on.

With that it is safe to assume that we can mark an client as "disposed" so it can be garbage collected.

If a client is expected to be used for only a single request then you'll need to be careful to ensure that nothing in the global scope, and nothing that outlives the request lifespan holds on to a reference. You need to ensure that the references are cleared and never rely on the isolate being terminated to determine the lifespan of the client.

[JPeer264]

We do not spawn new isolates for every request and a worker should never be written to depend on whenever we might spawn a new isolate.

Thanks a lot for the corrections - that makes a lot of sense. I then actually misread the docs - there I assumed "user code" is kinda equivalent to requests.

The infrastructure can spin up new copies of the worker as necessary based on the workload

Would this mean that also the memory is copied over or would it be a fresh copy?

The waitUntil mechanism does not tell you how long a request takes. All it allows you to do is temporarily extend the lifespan of the request context to wait for some post-request tasks to finish.

That is correct, but we would know when a promise inside a waitUntil ends - by the end of all promises of all called waitUntils of a given request ends, we could safely release the our client (we do have a mechanism to detect that already for our last flush).

If a client is expected to be used for only a single request then you'll need to be careful to ensure that nothing in the global scope, and nothing that outlives the request lifespan holds on to a reference. You need to ensure that the references are cleared and never rely on the isolate being terminated to determine the lifespan of the client.

Yap, we actually keep the client, as it was not designed initially like that. But this is something we have to add.

[jasnell]

Would this mean that also the memory is copied over or would it be a fresh copy?

Fresh instance.

That is correct, but we would know when a promise inside a waitUntil ends - by the end of all promises of all called waitUntils of a given request ends, we could safely release the our client (we do have a mechanism to detect that already for our last flush).

It depends. Let's say you have a waitUntil(promise.finally(() => {})) where the promise is expected to a couple minutes to to resolve... the finally will never trigger because the wait time exceeds the grace period we allow for waitUntil tasks. The waitUntil only waits so long, after which the work is just dropped on the floor in order to prevent a request from being stuck around indefinitely.

[JPeer264]

In the case of a waitUntil being stuck forever after the max wait time the request context is destroyed and therefore all its references as well right? In that case we would be safe of not cleaning up everything, if not I think this would be hard to solve.