fix cleaning mixed tests

[JoanFM]

Summary

Fixes a latent bug where two benchmark_type: "mixed" benchmarks sharing the same (setup, dataset_name) crashed on assert benchmark_type == "read-only" inside ro_benchmark_reuse, because the second test inherited the first one's populated setup_details["env"] and got routed through the reuse branch.

The fix is two-part: a refactor of the post-spin-up publish to shared_env, plus a new pre-iteration teardown for mixed groups. (An initial one-line fix avoided the assertion crash but broke the cross-type handoff — see "Why the bug report's one-liner isn't enough" below.)

The bug

mixed semantically means "this workload may mutate state, so each test in the group must get a fresh Redis env." The framework respected that for the cross-type handoff (mixed → read-only on the same dataset) but not for the intra-group case.

Triggers when a plan satisfies all of:

1. Contains at least one read-only and at least one mixed benchmark → sets reuse_mixed = True.
2. Contains two or more mixed benchmarks sharing the same (setup, dataset_name).

Symptom:

File "redisbench_admin/run_remote/run_remote.py", line 786, in run_remote_command_logic
    ) = ro_benchmark_reuse(
File "redisbench_admin/run_remote/run_remote.py", line 1896, in ro_benchmark_reuse
    assert benchmark_type == "read-only"
AssertionError

Real-world repro on RediSearch master: search-expire-doc-1000-seconds.yml and search-expire-write-read-concurrent.yml are both mixed, both target dataset_name: "5200K-docs-union-iterators.idx10-expire-doc-1000s", share the same 8-setup list, and the surrounding search*.yml glob includes plenty of read-only tests — whichever of the two mixed tests runs second in its group hits the assertion.

Why the bug report's one-liner isn't enough

The original proposal was a single line: after publishing the mixed env to shared_env, clear setup_details["env"] so the next iteration takes the spin-up branch. That avoids the assertion crash — but run_local.py:736's per-iteration teardown uses setup_details["env"] is None as the "tear down Redis" signal:

if setup_details["env"] is None:
    if args.keep_env_and_topo is False:
        teardown_local_setup(redis_conns, redis_processes, setup_name)

So the one-liner also causes the per-iteration teardown to kill the Redis whose env was just published to shared_env. The subsequent read-only group inherits a dict pointing to dead processes and crashes with ConnectionRefusedError. The integration test in this PR caught that regression on the existing test_run_local_dataset_reuse_memtier scenario (one mixed + read-only on the same dataset, which works on master today). The bug report's "the handoff still works because shared_env holds the env" assertion didn't account for the per-iteration teardown also killing the processes the env references.

The same loop in run_remote.py:1391 already has an is_shared check that protects the handoff there, but the inter-iteration teardown story is still needed for cleanliness; the fix is applied symmetrically.

The fix (two parts)

1. save_env_for_cross_type_reuse just publishes, no clearing

Pulled the publish into a named helper in both files. It writes setup_details["env"] into shared_env[env_key] when reuse_mixed=True and leaves setup_details["env"] populated. The env stays alive across the inner loop iteration (and the existing end-of-group logic preserves it for the cross-type handoff).

2. New tear_down_previous_mixed_env_if_needed helper, called at the top of each inner-loop iteration

For mixed groups with reuse_mixed=True, this helper looks at setup_details["env"] from the previous iteration. If populated, it tears down that Redis (via teardown_local_setup / shutdown_remote_redis), removes the corresponding entry from shared_env (the Redis it references is now dead), and clears setup_details["env"] — so the dispatcher takes the spin-up branch.

No-op cases (all important):

• First iteration in a group — setup_details["env"] is None, nothing to tear down.
• Read-only types — read-only tests are meant to reuse env within a group (the original RO optimization predating reuse_mixed).
• --keep-env-and-topo — user explicitly opted out of teardown.

The result is the correct semantics:

• Each mixed test runs against a fresh Redis env.
• The last mixed test's env stays alive at end-of-group (because the helper is a no-op when env is None — the last iteration doesn't tear itself down).
• End-of-group logic (run_local.py:752, equivalent in run_remote.py) sees the env in shared_env, marks is_shared=True, and preserves it.
• The read-only group on the same (setup, dataset) picks it up via the existing shared_env[env_key] handoff at the top of the next outer-loop iteration.
• The ro_benchmark_reuse call site is no longer reached on the mixed path, so the assert benchmark_type == "read-only" invariant is preserved as a belt-and-braces check.

Applied symmetrically to run_remote.py (shutdown_remote_redis for the teardown, with the extra skip_remote_db_setup no-op case for the remote-specific opt-out).

Tests

Integration tests (run in CI alongside the existing dataset-reuse tests)

• tests/test_run_local.py::test_run_local_mixed_env_no_leak — drives run_local_command_logic end-to-end with a new plan (tests/test_data/mixed_env_leak/*.yml) containing two mixed benchmarks plus one read-only benchmark, all on the same (setup="oss-standalone", dataset_name="mixed-env-leak"). Pre-fix this would crash on the assertion. Post-fix the run completes with exit code 0; each mixed test gets a fresh spin-up and the read-only test reuses the env from the last mixed test. Uses memtier_benchmark, which CI already installs from apt.
• tests/test_run_remote.py::test_run_remote_mixed_env_no_leak — same plan, driven through run_remote_command_logic. Gated behind RUN_REMOTE_TESTS=1 like the existing test_run_remote_dataset_reuse_memtier.

Unit tests (fast feedback for the helpers)

• test_save_env_for_cross_type_reuse_publishes_to_shared_env / _disabled_is_noop / _read_only_is_noop (remote-specific) / _mixed_without_reuse_mixed_is_noop — lock in the publish semantics.
• test_tear_down_previous_mixed_env_clears_and_removes_from_shared_env / _noop_on_first_iteration / _noop_for_read_only / _noop_when_keep_env_and_topo — lock in the four key invariants of the teardown helper, using monkeypatch to intercept the actual teardown calls.

Verification done locally

• New integration test passes (memtier_benchmark available locally).
• Mixed → read-only handoff verified by running vanilla-memtier-load.yml + vanilla-memtier-query{,2}.yml end-to-end → exit code 0.
• test_run_local_dataset_reuse_memtier fails locally on vanilla-memtier-monitor-input.yml because the apt-installed memtier_benchmark 2.1.4 on my machine doesn't support --monitor-input; CI uses a newer build and the test passes there (last master run: 206 passed, 1 skipped).
• black --check clean on all four touched files.

Out of scope

Two mixed benchmarks sharing (setup, dataset) each pay their own fresh spin-up — that's the contract of mixed. If a benchmark author wants to share state between two mixed tests (e.g. warm-up + measured-run), a separate explicit opt-in (e.g. share_env_with: <prev-test-name> in clientconfig) would be cleaner than the implicit state-leak the old code produced. Flagged for future work.

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.53%. Comparing base (913f4c9) to head (ce2911b).

Files with missing lines	Patch %	Lines
redisbench_admin/run_remote/run_remote.py	83.33%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
+ Coverage   47.28%   47.53%   +0.25%     
==========================================
  Files          74       74              
  Lines        8868     8907      +39     
==========================================
+ Hits         4193     4234      +41     
+ Misses       4675     4673       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.