Skip to content

fix(cluster): aggregate metrics in a deterministic order#771

Open
AysajanE wants to merge 1 commit into
prometheus:mainfrom
AysajanE:northset/M-011
Open

fix(cluster): aggregate metrics in a deterministic order#771
AysajanE wants to merge 1 commit into
prometheus:mainfrom
AysajanE:northset/M-011

Conversation

@AysajanE

@AysajanE AysajanE commented Jul 12, 2026

Copy link
Copy Markdown

Problem

Cluster metric aggregation (lib/metricAggregators.js) sums worker-reported values in the order workers respond to the GET_METRICS_REQ message. Because floating-point addition is not associative, the same set of values summed in different orders can produce slightly different results (e.g. 1.4765105 vs 1.4765104999999998). As reported in #539, this non-determinism can trigger a false "reset" detection in Prometheus and corrupt graphs.

Fix

  • Sort values before summing (sumValues) so the sum and average aggregators produce a reproducible result regardless of the order in which workers respond.
  • Add a unit test that aggregates the same values in two different orders and asserts the results are strictly equal.

Checks (Node 22)

  • npm run test-unit — 543 passing, including the new order-independence test.

Fixes #539


Disclosure: this change was prepared with AI assistance and reviewed by me before submitting. I ran the check above in a network-isolated container and published a signed, re-runnable record of that run, verifiable via GitHub artifact attestation: https://northset-oss.github.io/verification-pilot/. Contributor self-run, not a maintainer verification.

Comment thread lib/metricAggregators.js Outdated
function sumValues(values) {
return values
.slice()
.sort((a, b) => a.value - b.value)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the place or the field that needs to be sorted. The results need to be sorted by threadId at the caller.

And that could be achieved by creating a promise at the point of the message send, and doing a Promise.all() on the results.

Something slightly different might need to be done in worker.js

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — understood. I put determinism in the wrong layer. I will remove the aggregator-level sort and instead collect responses in stable worker-ID order in cluster.js and stable thread-ID order in worker.js, with regression tests that reverse response arrival. I understand this addresses response-order nondeterminism for a stable worker set, not changes caused by workers entering or exiting. Is that the scope you want before I update the branch?

Collect cluster responses in numeric worker-ID order with per-worker promises, and worker-thread responses in numeric thread-ID order. Add reverse-arrival regression coverage for both paths.

Fixes prometheus#539

Signed-off-by: Aysajan Eziz <aeziz@northset.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cluster should always aggregate metrics in a same order

2 participants