Fix TOCTOU race in removeRun's deletion query (#5031)

[mmido6039]

Fixes #5031

Problem

A run protected by an active RunLock could still be permanently
deleted. removeRun() resolves matched_run_ids from the filter and
lock-checks those specific ids, but the actual deletion query re-ran
process_run_filter() with the original filter instead of reusing the
already-resolved ids.

Root cause

Since each delete commits in its own transaction, there's a window
between the lock check and the deletion query where a run matching
the same filter (e.g. same name, freshly recreated with a new active
lock - as when a store restarts under the same run name) could be
picked up by the deletion query without ever being lock-checked. This
is a follow-up to #1445/#4999, which fixed the lock check itself but
left the deletion query resolving the filter independently, so the
guarantee only held for rows that matched at preflight time.

Fix

Build the destructive query directly from the already lock-checked
matched_run_ids:

q = session.query(Run).filter(Run.id.in_(matched_run_ids))

instead of re-running process_run_filter(). This guarantees only the
exact rows that passed the lock check can be deleted, regardless of
what the filter would match afterward.

Testing

Added test_deletion_query_uses_resolved_ids_not_the_filter_again to
web/server/tests/unit/test_run_removal_lock.py, simulating the exact
race from the issue: resolve and lock-check an unlocked run, then
simulate a concurrent client deleting it and creating a new run under
the same name with an active lock. Confirms the fixed query
(Run.id.in_(matched_run_ids)) correctly excludes the new row, while
explicitly demonstrating that re-running the filter (the old buggy
pattern) would have matched it. All 5 tests in the file pass; no
regressions in the rest of the web unit suite; pycodestyle clean.

[bruntib]

This test duplicates the code that is supposed to be tested. So, the code-base itself is not covered by this test. This is not to best approach, because if the implementation in removeRun() changes, then this query gets out of sync. I know, it's really not elegant to say, but we shouldn't add this test function. If we add a test, then that should simulate the concurrent removal of a run through its name.

[bruntib]

The fix looks good to me, but I have some concerns about the test.

[bruntib]

Thank you, this looks better now. One last request. Could you not to hard-code the run ID here? How can we make sure that the run in question has ID 2 in the long terms? What if the tests are executed in a different order, or another test is added later which also inserts runs? In that case this hard-coded number would break. Maybe, the run could be filtered by its name.

Also, when inserting a new run a few lines below, the ID 3 may conflict with another test in the future. Could we rely on having an autoincrement ID?

Thank you!