Compare the timeout tiers exactly, not approximately (#727)

docs/developers-guide.md

@@ -6972,12 +6972,60 @@ into seconds before the second arrives; summed the other way round the
 nanosecond accumulator overflows and the whole duration is refused, which would
 be this contract refusing a configuration nextest runs quite happily.
 
+### The tiers are compared exactly, not approximately
+
+Every tier comparison is a sum, and a sum is exact only if every term is. One
+`float` among them converts the whole of it back, and the conversion is silent.
+So `seconds` returns a `fractions.Fraction`, and so does everything the
+comparisons add to it: the watchdog budget read from a workflow, the job
+ceiling converted from `timeout-minutes`, and the five allowances and margins
+declared in `timeout_budgets.py`.
+
+The reason is the range. humantime reaches 2**64 seconds and a double holds 53
+bits of significand, so above 2**53 it cannot represent two budgets a second
+apart. `18446744073709551614s` and `18446744073709551615s` are both inputs in
+the estate differential and both convert to the same double, so an ordering
+assertion between them compares equal and passes whichever way round it is
+written. The nanosecond end makes the same point: a tenth of a second has no
+exact double, so a budget assembled from tenths and one written as a decimal
+would differ by a rounding error rather than by anything anyone configured.
+
+A float is still what a reader wants to see in a message, so `display_seconds`
+returns one. The two are behind different names on purpose: a caller chooses
+which it wants rather than getting the lossy one by default.
+
+None of this can be demonstrated on the budgets this repository configures.
+They are minutes and seconds, nowhere near either end, and they never will be
+otherwise, so a contract resting on the real files would pass with every term a
+float. `timeout_exactness_test.py` therefore drives the three compositions the
+ordering contract evaluates at two to the sixtieth, where neighbouring doubles
+are 256 seconds apart and a one-second difference is lost outright rather than
+only on one side of a tie. Each case asserts the collapse alongside the
+ordering, so a case that stopped exercising the loss fails rather than passing
+quietly, and one further case asserts that the values actually in force arrive
+exact, so a float reintroduced on the live path is caught without waiting for a
+budget nobody will set.
+
+Each of the eight terms was reverted to a float in turn and every one failed a
+case naming it. Two of them, the outside-work allowance and the ceiling margin,
+are added by the same function and fail the same ordering case, which is why
+the constants are also asserted one by one under their own names: the report
+then says which of the two moved.
+
+The watchdog reading is the one place a float still appears, and deliberately.
+`Fraction` has no notion of `nan` or `inf` and raises on both, which would turn
+a workflow interpolating an expression to `inf` into unreadable text rather
+than the named refusal that case deserves. So the text is parsed as a float,
+checked for finiteness and sign, and then converted from the text rather than
+from the float, which keeps a tenth exactly a tenth. Nothing is compared
+against the float on the way through.
+
 The port's scope is narrow and deliberately so. `nextest_durations` owns one
 thing: turning the text of a nextest duration into seconds exactly as
 `humantime` would, and refusing what `humantime` refuses. It is a
 workflow-contract helper, not a repository-wide duration parser. Its call-sites
 are `nextest_budgets.py`, which reads `.config/nextest.toml` budgets,
-`timeout_ordering_test.py`, and the two test modules that drive the reading
+`timeout_ordering_test.py`, and the three test modules that drive the reading
 directly. Nothing outside `tests/workflow_contracts` imports it, and nothing
 inside should grow a second duration reader beside it. humantime's unit table
 sits beside it in `nextest_units.py`, and humantime's accumulator in

tests/workflow_contracts/coverage_lane_multi_step_test.py

@@ -14,6 +14,7 @@
 Run via ``make test-workflow-contracts``.
 """
 
+import fractions
 import typing as typ
 
 import pytest
@@ -125,15 +126,15 @@ def test_two_coverage_steps_in_one_job_are_judged_together() -> None:
             workflow="ci.yml",
             job="build-test",
             step="cover one",
-            watchdog=1800.0,
-            job_timeout=60 * 60.0,
+            watchdog=fractions.Fraction(1800),
+            job_timeout=fractions.Fraction(60 * 60),
         ),
         CoverageLane(
             workflow="ci.yml",
             job="build-test",
             step="cover two",
-            watchdog=2700.0,
-            job_timeout=60 * 60.0,
+            watchdog=fractions.Fraction(2700),
+            job_timeout=fractions.Fraction(60 * 60),
         ),
     )
 

tests/workflow_contracts/coverage_lane_reading_test.py

@@ -9,6 +9,7 @@
 Run via ``make test-workflow-contracts``.
 """
 
+import fractions
 import typing as typ
 
 import pytest
@@ -167,14 +168,21 @@ def test_the_required_ceiling_sums_the_watchdogs_and_adds_the_margin() -> None:
     the margin changes nothing observable. Both terms are therefore
     driven with controlled numbers.
     """
-    assert required_ceiling([1800.0, 2700.0]) == pytest.approx(
-        4500.0 + OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + CEILING_MARGIN_SECONDS
+    assert required_ceiling([
+        fractions.Fraction(1800),
+        fractions.Fraction(2700),
+    ]) == fractions.Fraction(4500) + OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + (
+        CEILING_MARGIN_SECONDS
     ), "two steps need the sum of their budgets, not the larger of them"
-    assert required_ceiling([1800.0]) == pytest.approx(
-        1800.0 + OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + CEILING_MARGIN_SECONDS
+    assert (
+        required_ceiling([fractions.Fraction(1800)])
+        == fractions.Fraction(1800)
+        + OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS
+        + CEILING_MARGIN_SECONDS
     ), "one step needs its own budget, the allowance and the margin"
-    assert required_ceiling([]) == pytest.approx(
-        OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + CEILING_MARGIN_SECONDS
+    assert (
+        required_ceiling([])
+        == OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + CEILING_MARGIN_SECONDS
     ), "the margin is a term of its own, not a fraction of the others"
 
 

tests/workflow_contracts/coverage_lanes.py

@@ -6,6 +6,7 @@
 no module outgrows the 400-line limit the Python lint gate enforces.
 """
 
+import fractions
 import typing as typ
 
 from lane_environment import WatchdogValueError, nextest_profile_of, watchdog_of
@@ -27,10 +28,11 @@ class CoverageLane(typ.NamedTuple):
         The job the step belongs to.
     step : str
         The step's declared name.
-    watchdog : float or None
-        The watchdog budget in seconds, or None when the job sets none
+    watchdog : fractions.Fraction or None
+        The watchdog budget in seconds exactly, or None when the job
+        sets none
         and so inherits the action's 1,800 s default.
-    job_timeout : float or None
+    job_timeout : fractions.Fraction or None
         The job's ``timeout-minutes`` in seconds, or None when it
         declares none and so inherits GitHub's six-hour default.
     condition : tuple[object, object]
@@ -47,8 +49,8 @@ class CoverageLane(typ.NamedTuple):
     workflow: str
     job: str
     step: str
-    watchdog: float | None
-    job_timeout: float | None
+    watchdog: fractions.Fraction | None
+    job_timeout: fractions.Fraction | None
     condition: tuple[object, object] = (None, None)
     nextest_profile: str | None = None
 
@@ -169,7 +171,12 @@ def _lanes_in_job(
         message = f"{workflow}:{job_name}: {error}"
         raise WatchdogValueError(message) from error
     raw_timeout = job.get("timeout-minutes")
-    timeout = None if raw_timeout is None else float(raw_timeout) * 60.0
+    # Read from the text and multiplied exactly. This ceiling is
+    # compared against a sum of watchdog budgets and two allowances, so
+    # a float here would discard the exactness the other terms carry;
+    # the minute-to-second conversion is itself a term of that
+    # comparison rather than a display detail.
+    timeout = None if raw_timeout is None else fractions.Fraction(str(raw_timeout)) * 60
     return [
         CoverageLane(
             workflow=workflow,

tests/workflow_contracts/lane_environment.py

@@ -12,6 +12,7 @@
 enforces.
 """
 
+import fractions
 import math
 import typing as typ
 
@@ -22,7 +23,7 @@ def watchdog_of(
     document: dict[str, typ.Any],
     job: dict[str, typ.Any],
     step: dict[str, typ.Any],
-) -> float | None:
+) -> fractions.Fraction | None:
     """Return the watchdog budget in force for one step.
 
     All three levels are read, innermost first, as GitHub resolves them.
@@ -143,7 +144,7 @@ class WatchdogValueError(ValueError):
     """
 
 
-def _budget_from(raw: object) -> float | None:
+def _budget_from(raw: object) -> fractions.Fraction | None:
     """Return the resolved watchdog budget, or None when none is set.
 
     This reads the one declaration :func:`_declared_in_scope` chose, so
@@ -166,8 +167,11 @@ def _budget_from(raw: object) -> float | None:
 
     Returns
     -------
-    float or None
-        The budget in seconds, or None when the source sets none.
+    fractions.Fraction or None
+        The budget in seconds exactly, or None when the source sets
+        none. Exact because it is compared against a sum of budgets
+        read from three files, and one float among those terms loses
+        the whole comparison silently.
 
     Raises
     ------
@@ -180,6 +184,12 @@ def _budget_from(raw: object) -> float | None:
     text = str(raw).strip()
     if not text:
         return None
+    # Parsed as a float first and converted afterwards. `Fraction` has
+    # no notion of `nan` or `inf`: it raises on both, which would make
+    # them unreadable text rather than the named refusal below, and a
+    # workflow interpolating an expression to `inf` is exactly the case
+    # that refusal exists to name. The float here is a parser, not a
+    # value: nothing is compared against it before it becomes exact.
     try:
         seconds = float(text)
     except ValueError as error:
@@ -198,4 +208,4 @@ def _budget_from(raw: object) -> float | None:
             f"ceiling arithmetic and failing there"
         )
         raise WatchdogValueError(message)
-    return seconds
+    return fractions.Fraction(text)

tests/workflow_contracts/nextest_budgets.py

@@ -18,19 +18,19 @@
 """
 
 import tomllib
+import typing as typ
 from itertools import starmap
 
-from nextest_durations import (
-    NextestConfigurationError,
-    UnboundedTestError,
-    seconds,
-)
+from nextest_durations import NextestConfigurationError, UnboundedTestError, seconds
 from timeout_budgets import (
     CAPPED_PROFILE,
     NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS,
     TERMINATION_SAFETY_MARGIN_SECONDS,
 )
 
+if typ.TYPE_CHECKING:
+    import fractions
+
 
 #: One value-and-unit pair of a humantime duration. nextest parses its
 #: durations with `humantime` through `humantime_serde`, which reads a
@@ -176,7 +176,7 @@ def _multiplier_of(path: str, multiplier: object) -> int:
     raise NextestConfigurationError(message)
 
 
-def _budget_of(path: str, value: object) -> float:
+def _budget_of(path: str, value: object) -> fractions.Fraction:
     """Return the per-test budget one ``slow-timeout`` declares.
 
     Parameters
@@ -188,8 +188,8 @@ def _budget_of(path: str, value: object) -> float:
 
     Returns
     -------
-    float
-        The budget in seconds.
+    fractions.Fraction
+        The budget in seconds, exactly.
 
     Raises
     ------
@@ -229,7 +229,7 @@ def _budget_of(path: str, value: object) -> float:
     return seconds(period) * _multiplier_of(path, multiplier)
 
 
-def largest_test_allowance(config_text: str) -> float:
+def largest_test_allowance(config_text: str) -> fractions.Fraction:
     """Return the longest a single test may run, in seconds.
 
     nextest warns once per ``period`` and terminates after
@@ -245,8 +245,8 @@ def largest_test_allowance(config_text: str) -> float:
 
     Returns
     -------
-    float
-        The longest per-test budget.
+    fractions.Fraction
+        The longest per-test budget, exactly.
 
     A ``slow-timeout`` that names no ``terminate-after`` raises
     :class:`UnboundedTestError` from :func:`_budget_of` rather than
@@ -294,7 +294,7 @@ def bounds_a_single_test(config_text: str, profile: str = "default") -> bool:
     return isinstance(table, dict) and table.get("terminate-after") is not None
 
 
-def grace_period(config_text: str) -> float:
+def grace_period(config_text: str) -> fractions.Fraction:
     """Return the longest grace period the configuration names, in seconds.
 
     Read from the configuration rather than fixed, so a profile that
@@ -308,7 +308,7 @@ def grace_period(config_text: str) -> float:
 
     Returns
     -------
-    float
+    fractions.Fraction
         The largest configured grace period, or nextest's default.
     """
     periods = [
@@ -320,7 +320,7 @@ def grace_period(config_text: str) -> float:
     return max(periods, default=NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS)
 
 
-def termination_allowance(config_text: str) -> float:
+def termination_allowance(config_text: str) -> fractions.Fraction:
     """Return the time nextest may take to stop the run, in seconds.
 
     Two terms, not one. Hitting the whole-run budget starts nextest's
@@ -331,7 +331,8 @@ def termination_allowance(config_text: str) -> float:
     the second is a fixed margin for the teardown and report writing
     that follow it. A single floor over the two would absorb every grace
     period below the margin, so raising one would look free until the
-    run it cancelled.
+    run it cancelled. Both terms are exact so that the sum is: a float
+    in either would convert the whole of it back, silently.
 
     Parameters
     ----------
@@ -340,13 +341,15 @@ def termination_allowance(config_text: str) -> float:
 
     Returns
     -------
-    float
-        The grace period plus the safety margin.
+    fractions.Fraction
+        The grace period plus the safety margin, exactly.
     """
     return grace_period(config_text) + TERMINATION_SAFETY_MARGIN_SECONDS
 
 
-def global_timeout(config_text: str, profile: str = CAPPED_PROFILE) -> float | None:
+def global_timeout(
+    config_text: str, profile: str = CAPPED_PROFILE
+) -> fractions.Fraction | None:
     """Return one profile's whole-run budget, or None when it sets none.
 
     Read from the named profile's own table alone. nextest's profiles
@@ -367,7 +370,7 @@ def global_timeout(config_text: str, profile: str = CAPPED_PROFILE) -> float | N
 
     Returns
     -------
-    float or None
+    fractions.Fraction or None
         The whole-run budget in seconds, or None when that profile
         declares none.