Pin timeout and retry semantics; establish that fallback fails closed

[jeremymanning]

Closes the last unpinned part of the execution contract, so 06_failure_policy can be written as a real acceptance spec.

Everything here was already implemented. None of it was asserted anywhere, so none of it was safe to write down. Measuring it found two defects and one thing I had assumed was missing but is deliberately absent.

Retry was double-counted

Task.fail() increments retry_count, and it was called twice for one failure: once by the retry handler in _execute_task_with_resources, again when the gathered result was processed. A step with max_retries: 0 reported one retry.

Measured, before and after the fix:

max_retries	reported retries (before)	after
0	1	0
1	1	0
2	2	1
3	—	2

max_retries bounds attempts, not retries

With the count honest, the semantics are visible: max_retries: 2 is two attempts and therefore one retry. 0 and 1 are both a single attempt.

The name and the behaviour disagree. Correcting it would shift every existing pipeline's retry budget, so the behaviour is pinned by tests and the mismatch recorded rather than silently changed. Worth a decision, not a drive-by.

Timeouts

Enforced via asyncio.wait_for, and retried like any other failure — so worst-case wall time is roughly timeout × max_retries. Measured: timeout: 1 with max_retries: 2 takes ~2.7s, not 1s. That is the kind of thing you want to know before setting these numbers, so there is a test asserting the multiplication actually happens.

StepResult.timed_out now distinguishes a timeout from an ordinary failure, because "raise the timeout" and "fix the step" are different responses.

Fallback: nothing to record, because nothing falls back

I said last time that fallback events had "no emission point". The truth is better: there is no fallback on the canonical path at all. ModelRegistry.select_model raises NoEligibleModelsError rather than substituting a model the pipeline did not ask for. models/model_selector.py does have fallback strategies — and nothing on the canonical path uses it.

That is correct rather than an omission, so it is pinned as a test rather than "fixed". Quietly selecting an unrequested model is exactly how a cost-control policy gets bypassed: the pipeline believes it ran on the free model it asked for. Any future fallback has to record what it did and must not cross the free/paid boundary silently.

A gap recorded rather than closed

fail is the default policy, but it fires only for a step that raised. A tool returning {"success": False} without raising leaves its task completed, so the run continues past a step the author asked to abort on.

Making the policy consult StepResult.success closes it in one line. I tried it, and reverted it: the policy aborts by raising, so the run then produced no result document at all — discarding the trace precisely when it is most wanted. Two golden tests caught it immediately.

Closing this properly means the execution loop stops scheduling rather than throwing, and still returns a PipelineResult. That is a change to the loop, not a predicate, and it deserves its own PR. ADR 0001 records it and a test pins the current behaviour, so closing it later is a visible change rather than a silent one.

Numbers

	before	after
Blocking suite	506 passed	515 passed, 13 skipped, 0 failed, 0 xfailed
Collection	3301	3310
ruff / uv lock --check / git diff --check / docs drift	—	clean

Nine tests: timeout fires, is distinguishable, and is not set for a step that finishes in time; retries bounded, max_retries: 0 is one attempt, and the timeout×retry multiplication is real; continue lets the run finish while still reporting failure; the non-raising fail-fast gap; and selection failing closed.

[jeremymanning]

CI 9/9 green.

	main	this PR
failed	475	474
passed	1845	1846
errors	189	189
deselected	605	614

deselected +9 is this PR's new tests, excluded from the legacy selection by construction.

The −1 failed / +1 passed is within the noise floor and should not be read as an improvement. #443 established that this suite is not deterministic: test_analytics_result_creation and test_is_running_real flip between runs on PRs that touch nothing near them, giving a run-to-run swing of at least ±2. A delta of 1 is smaller than that.

I mention it because I have twice reported a legacy delta as signal when it was not, and a −1 in the favourable direction is exactly the case where it is tempting to stop checking.