converge() panics on a History with no events (usize underflow in iteration()) #27

New Issue

2026-08-27T13:20:41Z

logaritmisk commented

2026-08-27 13:20:41 +00:00

Calling converge() on a History that has had no events added panics rather than returning. It panics in both profiles, with a different message in each — and the release one is an out-of-bounds index rather than a clean arithmetic check.

Found against rev 7742b2b891fc341ef13a20d353b9c951790ebfdd.

Reproduction

use trueskill_tt::{ConstantDrift, History, NullObserver};

type Fit = History<i64, ConstantDrift, NullObserver, String>;

#[test]
fn converge_on_an_empty_history() {
    let mut history: Fit = Fit::builder_with_key().score_sigma(5.0).build();

    // No add_events call at all.
    let _ = history.converge();
}

Observed

debug    panicked at src/history.rs:225:22
         attempt to subtract with overflow

release  panicked at src/history.rs:226:42
         index out of bounds: the len is 0 but the index is 18446744073709551615

18446744073709551615 is usize::MAX, which confirms the two are the same fault: with overflow checks on it traps at the subtraction; with them off it wraps and the next line indexes off the end.

Root cause

History::iteration, src/history.rs:220:

fn iteration(&mut self) -> (f64, f64) {
    let mut step = (0.0, 0.0);

    competitor::clean(self.agents.values_mut(), false);

    for j in (0..self.time_slices.len() - 1).rev() {   // <- 225
        for agent in self.time_slices[j + 1].skills.keys() {   // <- 226

self.time_slices.len() - 1 is unguarded. At len() == 0 it underflows.

Note len() == 1 is fine: 1 - 1 == 0 gives an empty range, so only the zero case is affected.

Why this is reachable

It looks like a degenerate case, but it arrives from ordinary data. In ustat the per-layout and per-hole slices each get their own History, and events are only fed to the model when there are ≥2 competitors to compare. A disc golf layout that has only ever been played solo therefore produces a slice with zero rated events — a real row in a real CSV, not a contrived input — and converge() brought the whole boot down.

It is easy to guard at the call site once you know (ustat now does), but the failure gives no hint about the cause: neither message mentions histories, events or time slices, and the two profiles disagree about what went wrong.

Suggested fix

Return early from iteration when there is nothing to iterate:

if self.time_slices.len() < 2 {
    return (0.0, 0.0);
}

which makes converge() on an empty history a no-op reporting converged: true after 0 iterations — arguably the correct answer, since a model with no observations has trivially converged.

If you would rather callers be told explicitly, InferenceError already exists and converge() already returns Result, so an InferenceError::NoEvents variant would work too. Either is better than the current behaviour; the important part is that it should not be an arithmetic fault.

Worth a regression test either way, since the two profiles fail differently and a debug-only test would not have caught the release path.

Calling `converge()` on a `History` that has had no events added panics rather than returning. It panics in **both** profiles, with a different message in each — and the release one is an out-of-bounds index rather than a clean arithmetic check. Found against rev `7742b2b891fc341ef13a20d353b9c951790ebfdd`. ## Reproduction ```rust use trueskill_tt::{ConstantDrift, History, NullObserver}; type Fit = History<i64, ConstantDrift, NullObserver, String>; #[test] fn converge_on_an_empty_history() { let mut history: Fit = Fit::builder_with_key().score_sigma(5.0).build(); // No add_events call at all. let _ = history.converge(); } ``` ## Observed ``` debug panicked at src/history.rs:225:22 attempt to subtract with overflow release panicked at src/history.rs:226:42 index out of bounds: the len is 0 but the index is 18446744073709551615 ``` `18446744073709551615` is `usize::MAX`, which confirms the two are the same fault: with overflow checks on it traps at the subtraction; with them off it wraps and the next line indexes off the end. ## Root cause `History::iteration`, `src/history.rs:220`: ```rust fn iteration(&mut self) -> (f64, f64) { let mut step = (0.0, 0.0); competitor::clean(self.agents.values_mut(), false); for j in (0..self.time_slices.len() - 1).rev() { // <- 225 for agent in self.time_slices[j + 1].skills.keys() { // <- 226 ``` `self.time_slices.len() - 1` is unguarded. At `len() == 0` it underflows. Note `len() == 1` is fine: `1 - 1 == 0` gives an empty range, so only the zero case is affected. ## Why this is reachable It looks like a degenerate case, but it arrives from ordinary data. In [ustat](https://git.aceofba.se/logaritmisk/ustat) the per-layout and per-hole slices each get their own `History`, and events are only fed to the model when there are ≥2 competitors to compare. A disc golf layout that has only ever been played solo therefore produces a slice with zero rated events — a real row in a real CSV, not a contrived input — and `converge()` brought the whole boot down. It is easy to guard at the call site once you know (ustat now does), but the failure gives no hint about the cause: neither message mentions histories, events or time slices, and the two profiles disagree about what went wrong. ## Suggested fix Return early from `iteration` when there is nothing to iterate: ```rust if self.time_slices.len() < 2 { return (0.0, 0.0); } ``` which makes `converge()` on an empty history a no-op reporting `converged: true` after 0 iterations — arguably the correct answer, since a model with no observations has trivially converged. If you would rather callers be told explicitly, `InferenceError` already exists and `converge()` already returns `Result`, so an `InferenceError::NoEvents` variant would work too. Either is better than the current behaviour; the important part is that it should not be an arithmetic fault. Worth a regression test either way, since the two profiles fail differently and a debug-only test would not have caught the release path.

logaritmisk commented

2026-08-27 15:21:26 +00:00

Already fixed — but nothing pinned it, so it does now (eeb43e3).

The guard landed in f4e2922, and git merge-base --is-ancestor 7742b2b f4e2922 confirms the rev you filed against predates it. History::converge returns early on an empty time_slices, and iteration has its own is_empty guard behind that.

Added your exact reproduction as converge_on_an_empty_history_with_owned_keys in tests/degenerate_inputs.rs — the owned-key instantiation, not just the History::default() one that was already there.

Mutation-proved, because a green test proves nothing on its own. Removing both guards reproduces your report verbatim:

debug    panicked at src/history.rs:248:22
         attempt to subtract with overflow

release  panicked at src/history.rs:249:42
         index out of bounds: the len is 0 but the index is 18446744073709551615

Worth recording that the first mutation I tried — removing only iteration's guard — left the test green, because converge's early return means iteration is never reached for an empty history. iteration's guard is belt-and-braces; converge's is the load-bearing one. A regression test that only pinned the former would have been vacuous.

Your point about profiles was right and is why the test lives in tests/degenerate_inputs.rs, whose CI job runs in release too — the release failure is an out-of-bounds index, which no debug_assert would have caught.

Already fixed — but nothing pinned it, so it does now (`eeb43e3`). The guard landed in `f4e2922`, and `git merge-base --is-ancestor 7742b2b f4e2922` confirms the rev you filed against predates it. `History::converge` returns early on an empty `time_slices`, and `iteration` has its own `is_empty` guard behind that. Added your exact reproduction as `converge_on_an_empty_history_with_owned_keys` in `tests/degenerate_inputs.rs` — the owned-key instantiation, not just the `History::default()` one that was already there. **Mutation-proved, because a green test proves nothing on its own.** Removing *both* guards reproduces your report verbatim: ``` debug panicked at src/history.rs:248:22 attempt to subtract with overflow release panicked at src/history.rs:249:42 index out of bounds: the len is 0 but the index is 18446744073709551615 ``` Worth recording that the first mutation I tried — removing only `iteration`'s guard — left the test green, because `converge`'s early return means `iteration` is never reached for an empty history. `iteration`'s guard is belt-and-braces; `converge`'s is the load-bearing one. A regression test that only pinned the former would have been vacuous. Your point about profiles was right and is why the test lives in `tests/degenerate_inputs.rs`, whose CI job runs in release too — the release failure is an out-of-bounds index, which no `debug_assert` would have caught.

logaritmisk closed this issue

2026-08-27 15:21:59 +00:00

logaritmisk referenced this issue from a commit

2026-08-27 15:37:44 +00:00

fix: close out four small issues and pin #27's repro

logaritmisk added the bug label 2026-09-07 13:53:28 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#27