Do not re-run the test a worker crashed on under loadscope/loadfile/loadgroup

[davidheff]

Follows #1328, which fixed the deadlock half of this. One fault in LoadScopeScheduling remains.

The bug

remove_node identifies the test the worker died on but leaves it flagged as pending, so it returns to the workqueue with the rest of its work unit and the replacement worker starts it again. A test that takes the interpreter down with it takes the replacement down too, and the one after that, until --max-worker-restart is spent, at which point the run aborts with the remainder of that work unit never executed.

handle_crashitem has already reported the test as failed by then, so the retry establishes nothing. LoadScheduling pops the crashed item for exactly this reason.

On master, -n1 --dist=loadfile over

# test_a.py
def test_pass_1(): pass
def test_pass_2(): pass

# test_b.py
import os
def test_crash(): os._exit(1)
def test_after(): pass

ends 5 failed, 2 passed, with test_after never run. With this change it ends 1 failed, 3 passed.

The fix

One line in remove_node: mark the crashed test complete before its workload goes back on the queue.

The two tests changed in #1328

test_loadgroup_does_not_hang_after_restart and test_loadgroup_does_not_hang_after_restart2 assert *5 failed* for two-test files, which pins the retry loop rather than the intended outcome. Both now assert *1 failed*1 passed*.

Tests

• test_dsession.py::TestLoadScopeScheduling, the first scheduler-level tests for LoadScopeScheduling. test_remove_node_does_not_requeue_the_crashed_test fails on master. test_node_is_topped_up_until_it_can_report passes on master and is included because the schedule() path fixed in pytest --dist=loadgroup hangs if a crashed worker is restarted - variant 2 #1327 has no scheduler-level test of its own.
• acceptance_test.py::TestNodeFailure::test_loadfile_crashed_worker, the run above. Fails on master.

The full suite passes (one pre-existing non-strict xfail, TestNodeFailure::test_each_multiple "#20: xdist race condition on node restart", xpasses on my machine).

History

This PR originally also dropped completed work units from the requeue and topped a node up to two pending tests. #1328 landed both while it was open, so only the crashed-test retry is left. Rebased and reduced accordingly.

[larsoner]

Is this a dup of #1363 ? If so, review/confirmation that that fixes things would be appreciated!

[davidheff]

Partly, yes. The requeueing of completed work units is the same fault in both, so that half is dead: #1328 landed it while this was open.

I checked the rest against master. The second thing this PR fixed, a node needing two pending tests before it can report, is covered by the double _reschedule in schedule() from #1327 - my own unit test for it passes on master unmodified, so I have dropped the change and kept the test as coverage for that path.

What is left is a third fault neither PR touches. The crashed test is left flagged as pending, so it goes back on the queue and the replacement worker runs it again and dies on it in turn, until --max-worker-restart is exhausted and the run gives up with the rest of that work unit unexecuted. LoadScheduling pops the crashed item; the loadscope family does not. It is visible in #1328's own acceptance tests, which assert *5 failed* for two-test files - that is five workers dying on the same test.

Rebased onto master and reduced to that one line plus tests.