Panic: user-provided comparison function does not correctly implement a total order

[golfdish]

A note for the community

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

Problem

Starting in 0.43, Vector panics on encountering certain distribution metric data samples, in this instance when writing to a prometheus_remote_write sink:

thread 'vector-worker' panicked at library/core/src/slice/sort/shared/smallsort.rs:865:5:
user-provided comparison function does not correctly implement a total order
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::slice::sort::shared::smallsort::panic_on_ord_violation
   3: core::slice::sort::unstable::quicksort::quicksort
   4: vector::sinks::util::statistic::DistributionStatistic::from_samples
   5: <vector::sinks::prometheus::remote_write::request_builder::RemoteWriteEncoder as vector::sinks::util::encoding::Encoder<alloc::vec::Vec<vector_core::event::metric::Metric>>>::encode_input
   6: vector::sinks::util::builder::SinkBuilderExt::request_builder::{{closure}}::{{closure}}
   7: tokio::runtime::task::raw::poll
   8: tokio::runtime::scheduler::multi_thread::worker::Context::run_task
   9: tokio::runtime::task::raw::poll

I believe this is because release 0.43.0 updated the underlying Rust version from 1.80 to 1.81:

• commits between 0.42.0 -> 0.43.0
• includes: chore(deps): Bump Rust version to 1.81.0 #21509

Rust 1.81 introduced the following backwards-incompatible breaking change to how user-provided sorting functions are handled:

• Improve Ord violation help rust-lang/rust#128273
• discussed in more detail here: New panics from sort detecting Ord/PartialOrd violations rust-lang/rust#129561

As we can see in the current code, the sorting function passed into Vec::sort_unstable_by in statistic.rs is:

                bins.sort_unstable_by(|a, b| {
                    a.value.partial_cmp(&b.value).unwrap_or(Ordering::Equal)
                });

Per the docs, partial_cmp implements a partial order on a type, which is insufficient for functions passed into sort_unstable_by as of Rust 1.81. Per the current docs,

If the comparison function compare does not implement a total order, the function may panic

Unfortunately I haven't yet been able to capture any payloads that look like a smoking gun for triggering this panic, and have no way of reliably reproducing it.

Configuration

sinks:
    mimir:
      type: prometheus_remote_write
      inputs:
        - vector_public_metrics_statsd_mimir_safe
        - vector_public_metrics_routed.prometheus
      endpoint: "http://mimir-nginx.o11y.svc:80/api/v1/push"
      healthcheck:
        enabled: false
      buffer:
        type: disk
        max_size: 1073741824
        when_full: drop_newest
      request:
        retry_attempts: 1
        retry_max_duration_seconds: 3

Version

vector 0.47.0 (x86_64-unknown-linux-musl 3d5af22 2025-05-20 13:53:41.638057046)

Debug Output

Example Data

No response

Additional Context

No response

References

1. chore(deps): Bump Rust version to 1.81.0 #21509
2. Improve Ord violation help rust-lang/rust#128273
3. New panics from sort detecting Ord/PartialOrd violations rust-lang/rust#129561

[thomasqueirozb]

Hi, thanks for the report and all the details - this flew under the radar during the rust upgrade. Do you have example inputs which will make Vector panic to validate a potential fix?

Also, just so I'm sure I fully understood this: is the fix basically to just replace .partial_cmp with .cmp .total_cmp inside .sort_unstable* functions/closures?

[golfdish]

Do you have example inputs which will make Vector panic to validate a potential fix?

I haven't been able to isolate any end-to-end inputs that cause these crashes, but I've reproduced the panic with a unit test. I'm far from fluent in Rust, so excuse the messiness here, but this test (using a sequence taken from a reddit post on the topic) passes locally for me, and results in the same panic message:

    #[test]
    #[should_panic]
    fn test_from_samples_panics_using_partial_cmp() {
        let v: Vec<f64> = vec![
            85.0, 47.0, 17.0, 34.0, 18.0, 75.0, f64::NAN, f64::NAN, 22.0, 41.0, 38.0, 72.0, 36.0, 42.0,
            91.0, f64::NAN, 62.0, 84.0, 31.0, 59.0, 31.0, f64::NAN, 76.0, 77.0, 22.0, 56.0, 26.0, 34.0,
            81.0, f64::NAN, 33.0, 92.0, 69.0, 27.0, 14.0, 59.0, 29.0, 33.0, 25.0, 81.0, f64::NAN, 98.0,
            77.0, 89.0, 67.0, 84.0, 79.0, 33.0, 34.0, 79.0
        ];
        let rates: Vec<u32> = std::iter::repeat([1]).flatten().take(v.len()).collect();
        let foo: Vec<(f64, u32)> = v.into_iter().zip(rates).collect();
        let ssamples = samples(&foo);
        DistributionStatistic::from_samples(&ssamples, &[0.0, 1.0]);
    }

Also, just so I'm sure I fully understood this: is the fix basically to just replace .partial_cmp with .cmp .total_cmp inside .sort_unstable* functions/closures?

I haven't bothered forking to put up a PR for the one-line diff, but I have verified that replacing partial_cmp with total_cmp fixes this, yes (i.e. the above test fails).

[golfdish]

@thomasqueirozb Just checking in, should I fork and push a PR for this? It'd be very useful to us to have this fixed so we're not locked to 0.42 in production.

[thomasqueirozb]

@thomasqueirozb Just checking in, should I fork and push a PR for this? It'd be very useful to us to have this fixed so we're not locked to 0.42 in production.

This was on my personal todo list before the next release :)

PR is up with the fix. Tomorrow we should have a nightly version if you want to try to deploy it before the actual release comes out. These "user-provided comparison function" crashes should not happen anymore in any source/sink

[golfdish]

Awesome! We're running v0.50.0 in prod now. Thanks so much! 🙏