logaritmisk/trueskill-tt
1
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Activity

Draws with the default p_draw = 0.0 silently produce NaN skills and report converged: true #8

New Issue
Closed
opened 2026-08-04 19:14:12 +00:00 by logaritmisk · 1 comment
logaritmisk commented 2026-08-04 19:14:12 +00:00
Owner
Copy Link
Copy Source

Severity: critical. Silent data corruption on a default-configuration happy path, in release builds, with no error and no panic.

Reproduction

All of these run clean and return NaN (verified on main @ 2b5d3b1, --release):

// 1. record_draw on a default history
let mut h = History::default();
h.record_draw(&"a", &"b", 1).unwrap();
let r = h.converge().unwrap();
// h.current_skill("a") == Some(Gaussian { pi: NaN, tau: NaN })
// r.log_evidence == NaN
// r.converged   == true      <-- reports success

// 2. the fluent builder
h.event(1).team(["a"]).team(["b"]).draw().commit().unwrap();
// -> NaN

// 3. any 3+ team event with a single winner, because the losers tie
Game::ranked(&teams, Outcome::winner(0, 3), &GameOptions::default())
// -> all three posteriors NaN

Outcome::winner(w, n) for n >= 3 assigns rank 0 to the winner and rank 1 to everyone else (src/outcome.rs:35), so every free-for-all with more than two teams hits this on default options.

Root cause

Two independent defects that compound.

(a) The tie branch divides by zero when margin == 0. Game::likelihoods sets margin = 0.0 whenever p_draw == 0.0 (src/game.rs:395-404), and tie is set from equal results (src/game.rs:394). In v_w (src/lib.rs:147-158) the tie branch computes:

let alpha = (-margin - mu) / sigma;   // margin == 0 -> alpha == -mu/sigma
let beta  = ( margin - mu) / sigma;   // margin == 0 -> beta  == -mu/sigma  (identical)
let v = (pdf(alpha) - pdf(beta)) / (cdf(beta) - cdf(alpha));   // 0.0 / 0.0 = NaN

alpha == beta exactly, so both numerator and denominator are exactly zero. The NaN propagates through trunc → approx → the diff chain → every skill in the slice.

HistoryBuilder::default() uses p_draw: P_DRAW where P_DRAW = 0.0 (src/lib.rs:53), and GameOptions::default() does the same — so the default configuration is exactly the broken one.

(b) NaN is then misreported as convergence. tuple_gt (src/lib.rs:183) is t.0 > e || t.1 > e. NaN comparisons are always false, so tuple_gt((NaN, NaN), eps) == false, and History::converge (src/history.rs:441-448) both exits its loop immediately and sets converged = !tuple_gt(step, opts.epsilon) → true. A history that has been poisoned end-to-end reports a successful convergence in 1 iteration.

Defect (b) is worth fixing on its own: it turns any NaN anywhere in the engine into a false success signal.

Why it wasn't caught

Every existing draw test passes p_draw > 0.0 — test_1vs1_draw uses 0.25 (src/game.rs:726), test_1vs1vs1_draw likewise. Game::ranked_with_arena has a debug_assert! for exactly this ("draw must be > 0.0 if there are teams with draw", src/game.rs:205-212), so debug-build tests would catch it — but the public Game::ranked constructor does not perform that check, and nothing on the History ingestion path does either.

Fix

Decide the intended semantics first — the two options are not equivalent:

  1. Reject. Return InferenceError when an event contains tied teams and p_draw == 0.0, from Game::ranked, History::add_events, and History::record_draw. Promotes the existing debug_assert! to a real check. Simple, but makes Outcome::draw and Outcome::winner(_, n>=3) unusable on default options — arguably correct, since a zero draw probability genuinely says "draws cannot happen".
  2. Handle the degenerate limit. Define the margin == 0 tie case analytically (the limit of the two-sided truncation as the window closes) rather than evaluating 0/0, so a draw with p_draw == 0 is treated as the point-observation "these two performances are equal".

Option 1 plus a clear error message is the smaller change and matches the assert that is already written down. Option 2 makes the default configuration do something sensible instead of erroring, which is friendlier for Outcome::winner(0, n).

Independently of the choice, guard the convergence check against NaN so a poisoned run can never report converged: true — e.g. treat a non-finite step as "not converged" and surface it (InferenceError, or a converged: false report with the non-finite step recorded).

Acceptance

  • A draw on a default-configured History either errors cleanly or yields finite posteriors — never NaN.
  • Outcome::winner(0, 3) on GameOptions::default() likewise.
  • A test asserts that a NaN step is never reported as converged: true.
  • Regression tests covering record_draw, EventBuilder::draw(), Outcome::draw, and Outcome::winner(_, n) for n in 3..=5, all with p_draw == 0.0.
**Severity: critical.** Silent data corruption on a default-configuration happy path, in release builds, with no error and no panic. ## Reproduction All of these run clean and return NaN (verified on `main` @ 2b5d3b1, `--release`): ```rust // 1. record_draw on a default history let mut h = History::default(); h.record_draw(&"a", &"b", 1).unwrap(); let r = h.converge().unwrap(); // h.current_skill("a") == Some(Gaussian { pi: NaN, tau: NaN }) // r.log_evidence == NaN // r.converged == true <-- reports success // 2. the fluent builder h.event(1).team(["a"]).team(["b"]).draw().commit().unwrap(); // -> NaN // 3. any 3+ team event with a single winner, because the losers tie Game::ranked(&teams, Outcome::winner(0, 3), &GameOptions::default()) // -> all three posteriors NaN ``` `Outcome::winner(w, n)` for `n >= 3` assigns rank 0 to the winner and rank 1 to *everyone else* (`src/outcome.rs:35`), so every free-for-all with more than two teams hits this on default options. ## Root cause Two independent defects that compound. **(a) The tie branch divides by zero when `margin == 0`.** `Game::likelihoods` sets `margin = 0.0` whenever `p_draw == 0.0` (`src/game.rs:395-404`), and `tie` is set from equal results (`src/game.rs:394`). In `v_w` (`src/lib.rs:147-158`) the tie branch computes: ```rust let alpha = (-margin - mu) / sigma; // margin == 0 -> alpha == -mu/sigma let beta = ( margin - mu) / sigma; // margin == 0 -> beta == -mu/sigma (identical) let v = (pdf(alpha) - pdf(beta)) / (cdf(beta) - cdf(alpha)); // 0.0 / 0.0 = NaN ``` `alpha == beta` exactly, so both numerator and denominator are exactly zero. The NaN propagates through `trunc` → `approx` → the diff chain → every skill in the slice. `HistoryBuilder::default()` uses `p_draw: P_DRAW` where `P_DRAW = 0.0` (`src/lib.rs:53`), and `GameOptions::default()` does the same — so the default configuration is exactly the broken one. **(b) NaN is then misreported as convergence.** `tuple_gt` (`src/lib.rs:183`) is `t.0 > e || t.1 > e`. NaN comparisons are always false, so `tuple_gt((NaN, NaN), eps) == false`, and `History::converge` (`src/history.rs:441-448`) both exits its loop immediately and sets `converged = !tuple_gt(step, opts.epsilon)` → `true`. A history that has been poisoned end-to-end reports a successful convergence in 1 iteration. Defect (b) is worth fixing on its own: it turns *any* NaN anywhere in the engine into a false success signal. ## Why it wasn't caught Every existing draw test passes `p_draw > 0.0` — `test_1vs1_draw` uses `0.25` (`src/game.rs:726`), `test_1vs1vs1_draw` likewise. `Game::ranked_with_arena` has a `debug_assert!` for exactly this ("draw must be > 0.0 if there are teams with draw", `src/game.rs:205-212`), so debug-build tests would catch it — but the public `Game::ranked` constructor does not perform that check, and nothing on the `History` ingestion path does either. ## Fix Decide the intended semantics first — the two options are not equivalent: 1. **Reject.** Return `InferenceError` when an event contains tied teams and `p_draw == 0.0`, from `Game::ranked`, `History::add_events`, and `History::record_draw`. Promotes the existing `debug_assert!` to a real check. Simple, but makes `Outcome::draw` and `Outcome::winner(_, n>=3)` unusable on default options — arguably correct, since a zero draw probability genuinely says "draws cannot happen". 2. **Handle the degenerate limit.** Define the `margin == 0` tie case analytically (the limit of the two-sided truncation as the window closes) rather than evaluating `0/0`, so a draw with `p_draw == 0` is treated as the point-observation "these two performances are equal". Option 1 plus a clear error message is the smaller change and matches the assert that is already written down. Option 2 makes the default configuration do something sensible instead of erroring, which is friendlier for `Outcome::winner(0, n)`. Independently of the choice, **guard the convergence check against NaN** so a poisoned run can never report `converged: true` — e.g. treat a non-finite step as "not converged" and surface it (`InferenceError`, or a `converged: false` report with the non-finite step recorded). ## Acceptance - A draw on a default-configured `History` either errors cleanly or yields finite posteriors — never NaN. - `Outcome::winner(0, 3)` on `GameOptions::default()` likewise. - A test asserts that a NaN step is never reported as `converged: true`. - Regression tests covering `record_draw`, `EventBuilder::draw()`, `Outcome::draw`, and `Outcome::winner(_, n)` for `n in 3..=5`, all with `p_draw == 0.0`.
logaritmisk commented 2026-08-04 20:10:59 +00:00
Author
Owner
Copy Link
Copy Source

Fixed in f4e2922 (merged to main).

Took option 1 — reject — since it implements the intent the debug_assert! already stated. Validation sits in add_events_with_prior, the chokepoint every ingestion route reaches, because record_draw builds its results directly and never goes through Outcome; a check on the typed path alone would have missed it.

Defect (b) is fixed independently and matters on its own: a non-finite step now ends the loop and returns InferenceError::NonFiniteResult instead of being read as convergence. step_is_finite / step_converged replace bare !tuple_gt(..).

Consequence worth restating: Outcome::winner(w, n) for n >= 3 ties every loser, so those events now require a positive p_draw. They previously returned NaN, so nothing that worked has stopped working.

Covered by tests/degenerate_inputs.rs — record_draw, EventBuilder::draw(), Outcome::draw, and winner(_, n) for n in 3..=5, all at p_draw == 0.0, plus the positive-p_draw paths staying finite. These run in release too, where the original debug_assert! was compiled out.

Option 2 (defining the degenerate tie analytically, so a draw with p_draw == 0 means "these performances were equal") remains available if you'd rather the default configuration accept draws than reject them. Worth a fresh issue if so.

Fixed in f4e2922 (merged to `main`). Took option 1 — reject — since it implements the intent the `debug_assert!` already stated. Validation sits in `add_events_with_prior`, the chokepoint every ingestion route reaches, because `record_draw` builds its results directly and never goes through `Outcome`; a check on the typed path alone would have missed it. Defect (b) is fixed independently and matters on its own: a non-finite step now ends the loop and returns `InferenceError::NonFiniteResult` instead of being read as convergence. `step_is_finite` / `step_converged` replace bare `!tuple_gt(..)`. Consequence worth restating: `Outcome::winner(w, n)` for `n >= 3` ties every loser, so those events now require a positive `p_draw`. They previously returned NaN, so nothing that worked has stopped working. Covered by `tests/degenerate_inputs.rs` — `record_draw`, `EventBuilder::draw()`, `Outcome::draw`, and `winner(_, n)` for n in 3..=5, all at `p_draw == 0.0`, plus the positive-`p_draw` paths staying finite. These run in release too, where the original `debug_assert!` was compiled out. Option 2 (defining the degenerate tie analytically, so a draw with `p_draw == 0` means "these performances were equal") remains available if you'd rather the default configuration accept draws than reject them. Worth a fresh issue if so.
logaritmisk added the bugnumerics labels 2026-09-07 13:53:07 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
api
Shape or ergonomics of the public surface
 blocked
Waiting on another issue or an external decision
 breaking
Resolving it changes the public API; wants a major bump
 bug
Behaviour is wrong, including silently wrong
 decision
Needs a call to be made, not code to be written
 docs
Documentation, including doc comments and the README
 enhancement
New capability the crate does not have
 infra
CI, release process, packaging
 numerics
Floating-point accuracy, cancellation, convergence maths
 performance
Speed or memory
 refactor
Internal cleanup with no intended behaviour change
 testing
Test coverage and test infrastructure
No labels bug numerics
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logaritmisk
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#8