A corrupt or future ci_wait_started silently disables the CI wait budget #473

New issue

Closed

opened 2026-08-28 07:08:53 +00:00 by mph · 2 comments

mph commented

2026-08-28 07:08:53 +00:00

Owner

Found triaging IW-437 (raised in phase 3 code review; pre-existing, deliberately not fixed there).

CiGate.startOfWait (core/model/CiGate.scala:352-361) has two ways to make the total timeout stop enforcing, both silent:

UNPARSEABLE VALUE. A present but unparseable ci_wait_started is swallowed (catch case _: Exception => None) and replaced by now. Every resumed invocation then recomputes startedAtMs as approximately now, so the budget resets instead of expiring.
FUTURE VALUE. A parseable but future ci_wait_started makes (now - startedAtMs) negative, which never exceeds timeoutMs.

Either yields an unbounded wait, with no warning to the operator. The wait that was supposed to be capped simply never caps.

AGE. This is unchanged from baseline 81826a3. That revision's startOfWait already carried the identical catch case _: Exception => None plus .getOrElse(now) fallback, and a far-future ci_deadline produced the same negative-elapsed result via (deadline - timeoutMs). IW-437 phase 3 renamed the field and changed how the start is derived; it neither introduced nor widened this behaviour. The scaladoc documents the fallback deliberately: "Absent, or unreadable, means this invocation is the beginning of the wait."

THE DECISION TO MAKE. Absent is legitimately the beginning of a wait. Present-but-corrupt is not the same thing, and neither is present-but-future. What should each do - fail loud, or fall back to a bounded value? The "a typo must fail loudly" principle IW-423 raised for --needs-attention argues for the former on a corrupt value.

RELATED. Nothing enforces the schema's declared date-time format on this field, which is how an unparseable value reaches the card in the first place - see the companion issue on timestamp validation. That makes case 1 reachable through an ordinary bad write, not only through file corruption.

Found triaging IW-437 (raised in phase 3 code review; pre-existing, deliberately not fixed there). CiGate.startOfWait (core/model/CiGate.scala:352-361) has two ways to make the total timeout stop enforcing, both silent: 1. UNPARSEABLE VALUE. A present but unparseable ci_wait_started is swallowed (catch case _: Exception => None) and replaced by `now`. Every resumed invocation then recomputes startedAtMs as approximately now, so the budget resets instead of expiring. 2. FUTURE VALUE. A parseable but future ci_wait_started makes (now - startedAtMs) negative, which never exceeds timeoutMs. Either yields an unbounded wait, with no warning to the operator. The wait that was supposed to be capped simply never caps. AGE. This is unchanged from baseline 81826a3. That revision's startOfWait already carried the identical `catch case _: Exception => None` plus `.getOrElse(now)` fallback, and a far-future ci_deadline produced the same negative-elapsed result via (deadline - timeoutMs). IW-437 phase 3 renamed the field and changed how the start is derived; it neither introduced nor widened this behaviour. The scaladoc documents the fallback deliberately: "Absent, or unreadable, means this invocation is the beginning of the wait." THE DECISION TO MAKE. Absent is legitimately the beginning of a wait. Present-but-corrupt is not the same thing, and neither is present-but-future. What should each do - fail loud, or fall back to a bounded value? The "a typo must fail loudly" principle IW-423 raised for --needs-attention argues for the former on a corrupt value. RELATED. Nothing enforces the schema's declared date-time format on this field, which is how an unparseable value reaches the card in the first place - see the companion issue on timestamp validation. That makes case 1 reachable through an ordinary bad write, not only through file corruption.

mph referenced this issue

2026-08-28 07:09:59 +00:00

ReviewStateValidator does not enforce the schema's date-time format on any timestamp field #474

mph commented

2026-08-28 07:10:08 +00:00

Author

Owner

The companion issue referenced above is #474 — enforce the schema's declared date-time format in ReviewStateValidator, for every timestamp field on the card.

The companion issue referenced above is #474 — enforce the schema's declared `date-time` format in `ReviewStateValidator`, for every timestamp field on the card.

mph referenced this issue

2026-08-28 07:22:08 +00:00

IW-437: The phase CI gate finishes a phase without manual intervention #475

mph referenced this issue

2026-08-30 19:26:06 +00:00

phase-pr treats 'no pipeline yet' as 'no CI' and passes the gate before the pipeline is created #485

mph referenced this issue

2026-09-01 14:03:27 +00:00

phase-pr pushes a commit after reading CI, so its green can describe a stale head #491

mph referenced this issue from a commit

2026-09-02 06:22:12 +00:00

fix(ci-gate): bound the wait when the card's start cannot be trusted

mph referenced this issue

2026-09-02 06:22:57 +00:00

Squash the batch-implement blockers: nine fixes for the phase gate, the CI gate and the review-state card #501

mph commented

2026-09-02 20:55:14 +00:00

Author

Owner

Fixed in PR #501 (merge d1eba9c4). CiGate.startOfWait now returns a bounded start plus a warning: an unparseable value falls back to now, a future value is clamped to now, and ci-gate, phase-pr and phase-merge print the reason to stderr. An absent value is unchanged and silent. The wait is bounded in every case.

Fixed in PR #501 (merge d1eba9c4). `CiGate.startOfWait` now returns a bounded start plus a warning: an unparseable value falls back to now, a future value is clamped to now, and `ci-gate`, `phase-pr` and `phase-merge` print the reason to stderr. An absent value is unchanged and silent. The wait is bounded in every case.

mph closed this issue

2026-09-02 20:55:14 +00:00

mph referenced this issue

2026-09-02 20:55:14 +00:00

ReviewStateValidator does not enforce the schema's date-time format on any timestamp field #474