test_sign future-skew assertion fails when the clock crosses a second boundary

[lywinged]

What is wrong

tests/test_sign.py::test_verify_record_future_skew_is_deployment_configurable fails whenever the wall clock crosses a second boundary while it runs. The margin is not the one second it looks like, and the condition is exact rather than mysterious.

record["iat"] = int(time.time()) + 601
with pytest.raises(ValueError, match="max_future_skew_seconds=600"):
    verify_record(record, key_to_jwk(key), max_future_skew_seconds=600)

The test sets iat from int(time.time()), which truncates, while the check reads the clock unrounded. Writing the true time at setup as k + f — k whole seconds, f the fraction — and δ for everything that elapses before verify_record reads the clock (key generation, signing, the call):

iat  = k + 601
age  = (k + f + δ) - (k + 601) = f + δ - 601
raises  ⟺  age < -600  ⟺  f + δ < 1

The tolerance is not one second out of six hundred. It is however much of the current second is left when the test starts, minus its own runtime. f is uniform over [0, 1), so the per-run failure probability is δ — whatever the machine is doing at the time, and not small on a loaded runner.

Deterministic reproduction, patching time.time on the module object (verify_record imports time inside the function, so the module-level patch reaches it). Nothing about the record changes between rows; only where in the second the test begins:

  fraction=0.0     elapsed=0.001  f+elapsed=0.0010  -> raises (green)
  fraction=0.0     elapsed=0.06   f+elapsed=0.0600  -> raises (green)
  fraction=0.5     elapsed=0.001  f+elapsed=0.5010  -> raises (green)
  fraction=0.5     elapsed=0.06   f+elapsed=0.5600  -> raises (green)
  fraction=0.95    elapsed=0.001  f+elapsed=0.9510  -> raises (green)
  fraction=0.95    elapsed=0.06   f+elapsed=1.0100  -> NO RAISE   <-- "DID NOT RAISE ValueError"
  fraction=0.9995  elapsed=0.001  f+elapsed=1.0005  -> NO RAISE
  fraction=0.9995  elapsed=0.06   f+elapsed=1.0595  -> NO RAISE

Observed once in an ordinary full-suite run, which is what prompted tracking it down.

Spec section or file

tests/test_sign.py::test_verify_record_future_skew_is_deployment_configurable, against src/agentrust_trace/sign.py:436.

Expected behavior

The assertion should hold on every run rather than on most of them, and it should still be testing the boundary it names.

Two ways to get there, and they are not equivalent:

Freeze the clock. monkeypatch.setattr(time, "time", lambda: FIXED) for the duration. The local import time in verify_record binds the module object, so patching the module reaches it. The boundary can then be asserted at exactly ±1 second with no race, which is what the test appears to have been written to do.

Widen the margin — +601 becomes something like +660, with the accepting bound moved to match. Removes the race, and stops testing the boundary: an implementation whose comparison is off by a few seconds passes either way.

The first keeps what the test is for. Happy to send it as a PR if that reading is right — a fixture change, no behaviour touched.

Impact

Neither of the two failure modes this section asks about. sign.py is correct: the record really is 601 seconds in the future at the instant it is written and really is 600 by the time it is read, and the comparison does the right thing with both. No conformant implementation produces an invalid record because of this, and no verifier accepts an invalid one.

The cost is to the suite's signal. An intermittent red on a security-relevant freshness bound is the kind of failure that gets re-run rather than read, and a suite with one test that fails for its own reasons is a suite where the next real intermittent failure is assumed to be that one.

Scope is this test alone. Its second assertion (max_future_skew_seconds=602 must not raise) needs f + δ ≥ -1, which always holds. test_disabling_max_age_does_not_disable_future_bound uses +3600 against the 300-second default — a margin of 3300 seconds — so it is unaffected.

---

Generated by Claude Code

[github-actions]

❔ Contributor Check: UNKNOWN

Check	Result
Profile	UNKNOWN
Credential	LOW
Overall	UNKNOWN

Automated check by AgenTrust Contributor Check.

[imran-siddique]

Confirmed, your reading is right, and freeze the clock. Please send it.

Checked against main dba52b3: sign.py computes age = time.time() - iat unrounded while the test writes iat = int(time.time()) + 601, truncating. So the margin is not one second out of six hundred, it is whatever is left of the current second at setup minus the test's own runtime, and f + δ < 1 is exactly the condition. The per-run failure probability being δ rather than something fixed is why this looked like noise instead of a bug.

Freeze rather than widen, for the reason you give. verify_record does a local import time, so a module-level monkeypatch.setattr(time, "time", ...) reaches it, and the boundary can then be asserted at exactly ±1 second with no race. Widening to +660 would remove the race by removing the test: an implementation whose comparison is off by a few seconds passes either way, and that comparison is a security-relevant freshness bound.

Your impact section is the reason this is worth a PR rather than a note. sign.py is correct and no conformant implementation produces or accepts a bad record because of it, so the whole cost is to the suite's signal, and an intermittent red on a freshness bound is precisely the one people re-run instead of read. After that, the next genuine intermittent gets attributed to it.

Two things to carry into the PR:

Assert both edges under the frozen clock, not just the raising one. With the race gone there is nothing stopping the test from pinning the boundary properly: 601 raises, 600 does not, at exactly that second.

Include your reproduction table. Someone will eventually widen a margin somewhere else in this suite to make a flake go away, and a worked example of why that is the wrong move is worth more in the repository than in an issue.

Scope is that test alone, and I agree with your analysis of the neighbours: the 602 assertion needs f + δ ≥ -1, which always holds, and test_disabling_max_age_does_not_disable_future_bound has a 3300-second margin.

The UNKNOWN on the contributor check is the bot, not you. It carries no information.