Worker lifecycle issues with lib/worker.js and lib/cluster.js

[jdmarshall]

AI caught that we don't see a close event if a worker is terminated. Thinking about this code has revealed some more issues.

We cannot tell if a worker has gone away

Problem is that nodejs/node#59053 means that we never get the Worker object and therefore we cannot determine if the worker is dead, except by controlling the top level application workflows that are responsible for creating Workers in the first place, and that seems a bit mad especially given how many libraries there are that already do that.

And if the broadcast channel hasn't closed, then we don't know when one of our siblings has exited. If a worker terminates after it has been announced, then all subsequent GET_METRICS_REQ calls get an unfulfilled promise and you get no more metrics at all.

I believe we will have to do some sort of supervisor functionality in the main thread. But I'm not aware of a way to enumerate all of the workers that are currently running.

We don't remember historical values from workers and simply return no answer if any are missing

Every time a worker dies, counts get screwed up.

There may be some argument for keeping the last value recorded from a worker. Probably not gauges, but maybe everything else.

Because the Worker feature is going to be new to all users, we can ask the users to do a bit of additional bookkeeping to use this
functionality as table stakes for using this feature at all. We still have the luxury of adding that ask without listing it as a breaking change in the process. And it's a non-breaking change for Cluster if we ship it in v0.16 because dead workers has always been a fatal error there.

##Tasks

[ ] Document that reloading prom-client has undefined behavior in production mode.
[ ] Add debugging output (or console.warn?) to warn about existing listeners being found on initial call
[x] Simplify the promise logic in clusterMetrics() and workerMetrics() and store a promise for in-flight calls
[x] Implement dead workers support in the aggregator - aggregate all 'sum' type metrics to keep stats from backtracking
[x] Add a shutdown function for syncing data
[x] Add a shutdown event to worker.js, which flushes a 'swan song' last metric update to the aggregator to be retained in perpetuity
[x] Add a shutdown event to cluster.js, which flushes a 'swan song' last metric update to the aggregator to be retained in perpetuity
[x] Implement an ACK for the flush messages, because BroadcastChannel is complete Fire and Forget and process.exit() is likely to cause it to abort.
[x] Update the examples to match the new logic

[jdmarshall]

This is... frustrating. The API is so sparse that I don't see a way to do this short of doing a full distributed computing messaging protocol complete with ACKs and timeouts to work out if a worker has permanently disappeared.

Even in the main thread there is no way to enumerate the running workers. BroadcastChannel is completely fire and forget. Unlike the HTML5 version, there is no source object to interrogate or call functions on. They have to be explicitely closed in order to be in any other observable state. The only error callback is for if the message cannot be encoded for delivery.

[jdmarshall]

My first thought was move the _REQ event to the worker-specific channel, where arguably it might belong anyway, but there's still no hint on a BroadcastChannel that the other listeners have gone away, so I don't think that will work.

I hate the idea of a heartbeat function. Judging how long to wait for a heartbeat to arrive to be timely but not overbearing is a matter of fine tuning, and will break down if long tasks appear on any worker. And could lead to a bit of split brain behavior if we remove a thread from the list for non-responsiveness after an outlier request hits, only for it to recover in a few seconds.

But as the code as written and the philosophy generally seems to be, we would rather report no results than to periodically report incomplete results. Of course every time a worker dies, we are going to be reporting incomplete results anyway because the Registry doesn't accumulate old results. That behavior predates my intervention with the code, and I conserved it because I didn't have any strong opinions of my own at the time as the new guy.

And if we are being polled every N seconds there are some nasty epicycles that can hit with other periodic tasks.

So, we need to have a conversation about whether to preserve values from the last time a worker was seen or not.

Separate from that, I believe each GET_METRICS_REQ should generate an acknowledgement. And every time we get a message from a channel, we update the last seen time. If it gets too stale then we need to either filter it from the expected results or regurgitate the last known metrics.

[jdmarshall]

@krajorama FYI: Draft document of the changes that will go into this PR and the ones that will not

main...jdmarshall:prom-client:concurrencyFixes

At first I thought I could do a 'retained mode' to report old data for workers that are slow to respond or don't at all, but in the light of day and a fresh caffeine dose I think that's too ambitious by half. I have some ideas for 1.0 to perhaps mark specific workers as 'flaky' so that the users can opt-in to retaining data for a subset of workers.

[milcho0604]

Two things from poking at this. The first is just a bug report; the second is an option with a support-policy price tag attached, so I have put that price up front.

The registry has no reachable removal path

lib/worker.js:248 removes a worker when its BroadcastChannel emits close:

workerChannel.addEventListener('close', () => {
	workers.delete(workerName);
});

BroadcastChannel has no close event — only the method. MessagePort does dispatch one, which is probably where the expectation comes from, but the two are separate implementations: BroadcastChannel is an EventTarget over a native broadcast handle, and its whole prototype is constructor, name, close, postMessage, ref, unref, onmessage, onmessageerror (it is not an EventEmitter, so there is no .on either). Neither addEventListener('close') nor an assigned onclose fires for a peer exiting, throwing or being terminated, nor for the channel closing itself, on 22.23.2 and 26.5.1. MessagePort does fire in the same harness, so it is not the harness.

lib/worker.js:249 is the only place anything is removed from workers, so the map only grows. After a worker dies its slot stays and Promise.all never completes, so every later scrape runs into the watchdog at lib/worker.js:92: with one announced worker terminated, two successive workerMetrics() calls each rejected after ~5.00s with Operation timed out. 1 outstanding responses. The returned promise does settle — but from then on it settles as a rejection, every time.

A liveness probe, if the price is acceptable

The price first, because I think it is the deciding factor rather than a footnote: this needs process.on('workerMessage'), which is a shared, process-wide listener on a Stability 1.1 API, and postMessageToThread landed in 22.5.0 / 20.19.0 while engines: ^22 accepts 22.0–22.4. Feature-detecting it does not help — on the versions without it the leak simply stays. So it is really a minimum-Node decision. If that is not something you want to spend, I would drop this half and leave the section above as the bug report.

If it is: postMessageToThread(threadId, message, timeout) needs only a threadId, and workers already stores one (lib/worker.js:245). Probing from the primary and from a sibling worker, 5 trials per case on 22.23.2 / 24.11.0 / 26.5.1, 20 ms probe timeout:

target	result
live, listening	resolves, sub-millisecond
live, blocking the loop 4s	ERR_WORKER_MESSAGING_TIMEOUT, only after the timeout
terminated	ERR_WORKER_MESSAGING_FAILED, sub-millisecond, whatever the timeout
threadId that never existed	ERR_WORKER_MESSAGING_FAILED

Relevant to the heartbeat concern: a blocked worker and a dead one come back as different error codes rather than as the same silence, so eviction does not need a "how long is too long" threshold. It works worker→worker and worker→primary, so it does not assume the collector is thread 0.

Two windows blur it, and neither is fatal on its own:

• Startup. A live worker that has not installed its listener yet also answers FAILED, so announcing before installing it is a race rather than a guaranteed failure — with the listener deferred by setTimeout(0) I got FAILED 16/20 on 22 and 20/20 on 24 and 26, while deferring by process.nextTick or setImmediate never lost the race. Installing the listener before the ANNOUNCEMENT removes it, and that ordering is under this library's control.
• Shutdown. A probe that lands mid-teardown returns TIMEOUT, not FAILED. I measured how long that lasts: for process.exit(), an uncaught throw, and terminate(), the next probe already returns FAILED, median 21–22 ms after the exit was triggered, 5 trials each on both versions. So it is milliseconds, not scrapes.

So FAILED is actionable and TIMEOUT is not evidence of anything. That suits the exception path: at the watchdog, probe only the threadIds still in responseHandlers and drop the ones answering FAILED.

Things I would want settled before writing that:

• The watchdog callback calls done() immediately, which drops the request, so probing there means restructuring that path — and waiting for a probe extends the scrape past 5s by the probe timeout.
• Every user workerMessage listener also receives the probe, and if any of them throws, the sender gets ERR_WORKER_MESSAGING_ERRORED instead. Distinguishable, but it makes the probe only as reliable as unrelated listeners in the process.
• lib/worker.js:236 drops a duplicate announcement by name. If a worker is replaced under the same name, evicting the stale entry afterwards would leave the live replacement unregistered, so eviction and re-announcement have to be considered together.

Scope

This is detection only. It would make scrapes recover after a worker dies; it does nothing for retaining a dead worker's last values, and aggregate counters can still step backwards when one disappears, since Registry.aggregate() builds a fresh registry from the snapshots it is given. If the bar for this issue is counter continuity rather than "scrapes keep working", it is not enough on its own.

Happy to write the eviction part if you want it, or to leave it entirely if concurrencyFixes already covers this ground — I would rather not duplicate what you have in flight.

[jdmarshall]

The reason postMessage is not going to suffice here, is because the whole notion of Workers is that you put tasks on them that may block the event loop for an indeterminate amount of time. As configured, we are hoping that the loop is blocked for less than 5 seconds but we don't know if that's even reasonable.

I've elected not to go the retained route for storage space reasons. Any scrape where any of the children block for longer than 5 seconds will continue to time out as before. What I have done so far:

• Unwound the promise spaghetti in workerMetrics() and clusterMetrics() to be pure async/await logic, which avoids Kernighan's Law problems. Only the message handlers deal with resolve/reject, the main function just deals with Promise.all
• created util.waitFor() to either wait for a promise or a timeout. Removed some duplicated code between cluster.js and worker.js
• Added type attribute to getMetricsAsArray() and getMetricsAsJSON() that can filter the metrics by the aggregation type
• Implemented a shutdown() function on worker.js, that sends a message to the collector with the sum metrics and waits for an acknowledgement or a timeout before shutting down.
• On the collection thread, in-flight requests store the promise for the collection task, and shutdown() blocks on all requests until they settle
• On a goodbye message, WorkerRegistry aggregates the received metrics with an array of previously received metrics and then overwrites the stored data with the new data, and then deletes the worker from the worker list on the same tick.
• On a scrape, the WorkerRegistry grabs the list of known workers, and a snapshot of the old data on the same tick. So there should never be any skew or double-counting of values, but there may still be timed out scrapes if a worker exits mid-scrape.
• Moar debug lines
• Copy-catted shutdown logic back to cluster.js
• started documenting the lifecycle monitoring requirements of relying on this new feature

Any work we do to reduce scrape time decreases the window of possibility for that error.

Because we aggregate the aggregations, the space overhead of the data is based on the cardinality of the sum metrics across all workers. That gets expensive if you are recording per-worker values. If you're doing that, you should probably not be using cluster.js or worker.js in the first place and instead scraping all of the isolates directly.

Left to do is:

• complain about removing listeners, and update the docs