Refuse a non-finite min_duration_ms (#3287) - #3369
Conversation
NaN fails every comparison including the sign test, +Infinity is not negative, and an overflowing literal parses to +Infinity rather than failing to bind. All three were accepted as floors, matched no row, and returned the filtered-no-matches status naming a floor that was never a number - a refusal rendered as an answer.
|
Reviewed. The fix is correct and matches the PR description precisely:
One gap worth considering before/after merge: the PR body specifically identifies the web endpoint's Not a blocker — the validator fix closes the hole for every caller regardless — but it's the one place the test suite doesn't yet match the PR's own narrative about where this was reachable from. |
The sibling lane reached this defect independently and its version carries a positive control mine did not: a finite floor on the same read still returns a page, so the three refusal assertions are not passing against a read that refuses everything. It also documents the mechanism, including the 1e400 overflow, in the summary a caller reads rather than in a comment inside the method. Keeps the no-matches MESSAGE assertion beside its status-token one: the status is the branch taken, the sentence is what the caller reads, and the two can drift.
|
Re-reviewed at the current head (after the "better-instrumented form of the same fix" commit) — confirms the analysis in the earlier review comment still holds:
One gap from the earlier review is still unaddressed as of the latest commit: |
1 participant
ValidateMinMstested onlyminMs is < 0. NaN fails that like it fails every comparison,+Infinityis not negative, and an oversized literal such as1e400parses, to+Infinity, rather than being rejected as too large — so all four spellings arrived as accepted floors. None can ever match a run, so the read then returned the filtered-no-matches status, which names the floor it applied and reads as a true negative about the window. A refusal rendered as an answer, in the one validator whose whole job is refusing values that cannot mean what they say.All of them are reachable from the web surface:
double.TryParseunderNumberStyles.Floataccepts the named literals regardless of the style flags, and the overflow case parses rather than failing. Measured, all four.!double.IsFiniteis tested ahead of the sign so-Infinityis refused for what it is rather than for its sign.0remains a real value — it admits every row and ranks the page by duration — and the negative message keeps its wording, which pins on both SKUs assert on.Two lanes reached this independently, and this carries the better-instrumented half of each
The #3287 lane found it from the same review comment and had already written a fix on its own branch. Its version is better in two ways and both are taken here rather than defended against:
<summary>, where a caller reading the API sees it, rather than in a comment inside the method body.What is kept from the other version is one extra assertion: the refusal must not contain the no-matches message either, asserted beside the status-token check. The status is the branch taken and the sentence is what the caller actually reads, and the two can drift.
Pinned end-to-end on both SKUs over NaN,
+Infinityand-Infinity, with the finite positive control. The assertions that discriminate are the two absences: before this, the value passed validation and fell into the no-matches branch, so asserting only the lack of a payload would have been green on the defect.Found by the review bot on #3363 and merged past — this is that finding, repaired.