Prepare and finish step will now fail when phase fails

[happz]

This was not automatic! Both steps failed and stopped running their phases when a phase raised an exception and/or returned an outcome with an exception attached. These two conditions are no longer enough as we are expecting plugins that do not have any relevant exception, instead they gracefully deliver results reporting failed outcomes.

The patch teaches prepare and finish to recognize "negative" outcomes, FAIL and ERROR, and react as if a phase raised an exception, i.e. by reporting the error and stopping the run.

Pull Request Checklist

• implement the feature
• write the documentation

[gemini-code-assist]

Code Review

This pull request correctly extends the failure conditions for prepare and finish steps to include FAIL and ERROR outcomes, not just exceptions. The implementation in tmt/queue.py introduces a mechanism to stop the task queue. My review includes a performance suggestion for the queue implementation and points out a bug in the finish step where a continue statement delays the queue termination, which should be removed for immediate failure handling.

[thrix]

Clean, focused fix. Verified all changes against the actual branch code.

This directly addresses the design gap we identified during #4660 review - FAIL/ERROR results from plugins were silently recorded without stopping the prepare/finish steps. Now _is_failed() checks for both exceptions AND negative result outcomes, and queue.stop() + break halts execution immediately.

Key points verified:

• _is_failed() correctly checks bool(exceptions) or any(result in (ERROR, FAIL))
• preparations_applied only increments after the _is_failed() check passes
• The continue after exception processing was correctly replaced with fall-through to the failure check
• The queue's _keep_running flag acts as a safety net alongside the consumer's break
• Final guard changed from if exceptions to if _is_failed() to catch both failure modes

[vaibhavdaren]

Apart from the PR. @thrix - Your code review setup is too cool now. Need a demo in the next hacking session ^_^ .

[skycastlelily]

LGTM^^, just a nitpick about duplicate-ish:
do you consider create a shared _is_failed function,say: def _is_failed(exceptions: list, results: list) -> bool ,maybe in tmt/steps/init.py?

[happz]

LGTM^^, just a nitpick about duplicate-ish: do you consider create a shared _is_failed function,say: def _is_failed(exceptions: list, results: list) -> bool ,maybe in tmt/steps/init.py?

I hope there will be more opportunities to deduplicate code once other steps get on board of producing results.

[thrix]

Re-Review: Prepare and finish step will now fail when phase fails

Summary

The PR correctly addresses a gap where prepare and finish steps would only stop on exceptions, but not on gracefully reported FAIL/ERROR outcomes. The implementation adds a stop() mechanism to Queue and teaches both steps to recognize negative outcomes.

Previous review issues (the stale continue in finish, pop(0) performance) have been addressed or acknowledged (#4668).

Verified Observations

1. Unused return value from Queue.stop() (minor)

stop() returns self[:] (remaining tasks), but all 6 call sites discard the return value. If it's intended for future use, a comment would help; otherwise it could return None.

2. Dual stopping mechanisms in Queue.run() — redundant but harmless

Queue.run() has two independent stopping checks after each task: the pre-existing if failed_tasks: return and the new if not self._keep_running: return. When outcome.exc is set, the consumer calls queue.stop() AND breaks, but queue.run() would have also stopped via failed_tasks. This is defense-in-depth — fine, but worth noting for future refactoring.

3. _is_failed() duplication across prepare and finish

The two closures are near-identical, differing only in their results reference (self.results vs local results). As @skycastlelily noted and the author acknowledged, deduplication can wait for more steps to adopt results.

4. Logic flow in prepare and finish is correct

The _is_failed() check sits at the for outcome loop body level, meaning it runs after every successfully completed outcome (not just ones with exceptions). This correctly catches outcomes that report FAIL/ERROR results without raising exceptions — which is the whole point of this PR.

Verdict

No blocking issues. The core logic is sound and well-structured. The PR achieves its goal cleanly.

Generated-by: Claude Code

[LecrisUT]

Functionally looks good, but have questions about the longevity of these patches

[thrix]

Re-Review: Prepare and finish step will now fail when phase fails

Reviewed against latest commit 4e28278.

Summary

The PR correctly closes the gap where prepare and finish steps only stopped on exceptions, not on gracefully reported FAIL/ERROR outcomes. The _is_failed() helper and break statements ensure execution halts immediately when negative outcomes are detected.

Changes since last review

Commit 4e28278 added a TODO comment on the failed_tasks check in Queue.run(), acknowledging planned refactoring in #4668.

Observations

1. queue.stop() is effectively a no-op (minor)

Every call to queue.stop() is immediately followed by break. The break exits the consumer's for loop, which triggers GeneratorExit in the queue.run() generator — terminating it at the yield line before _keep_running is ever checked. This means the stop() method and _keep_running flag are unused infrastructure. The actual stopping is done entirely by break. This is acknowledged as future work in the TODO referencing #4668.

2. Queue.stop() return value is unused (minor)

All 4 call sites discard the return value (self[:]).

3. _is_failed() duplication (acknowledged)

Near-identical closures in prepare (self.results) and finish (local results). Deduplication deferred until more steps adopt results.

Verdict

No blocking issues. The core logic — detecting FAIL/ERROR outcomes and halting the step — is correct. The stop() infrastructure is forward-looking and tracked in #4668.

Generated-by: Claude Code