Flaky test: chat_delegate::tests::clear_ensure_subscription_sent_is_scoped_to_supplied_vk

[sanity]

Problem

Observed during the PR #294 review: components::app::chat_delegate::tests::clear_ensure_subscription_sent_is_scoped_to_supplied_vk (ui/src/components/app/chat_delegate.rs) failed once in a parallel full-suite cargo test -p river-ui --bins run, then passed in isolation, passed with --test-threads=1, and passed on a re-run.

This is pre-existing and unrelated to PR #294 — that PR does not touch chat_delegate.rs.

Hypothesis

Classic shared-global-mutable-state flake: the test asserts on a global signal/state that another test mutates concurrently. The pass-in-isolation / pass-with---test-threads=1 behaviour points at cross-test interference rather than a logic bug in the test itself.

Suggested fix

Isolate the global state the test depends on (per-test reset, or a test-local scope), or serialize the affected tests. Per the project's "flaky tests are broken tests" rule this should be fixed at the root, not retried.

[AI-assisted - Claude]

[sanity]

Issue Priority Assessment

Generated: 2026-05-26T16:01:36.223247703+00:00
Issue hash: 8f52c2c5

Value Assessment

The issue is already fixed. All three tests that touch ENSURE_SUBSCRIPTION_SENT now acquire a shared DEDUP_TEST_LOCK mutex, which serializes them against each other and eliminates the cross-test interference. This fix is already present in the current main branch.

Relevance: This issue is no longer relevant — it has already been fixed. The current code at ui/src/components/app/chat_delegate.rs:723 defines a DEDUP_TEST_LOCK: std::sync::Mutex<()> and all three tests that access the shared ENSURE_SUBSCRIPTION_SENT global acquire it as their first action (lines 747, 769, 803). This is precisely the "serialize the affected tests" fix that the issue's suggested fix recommended. The fix is already merged into main.

User Impact: This was a test infrastructure issue, not a user-facing bug. Users of River were never directly affected; the only impact was on developers whose CI might occasionally report a spurious failure, eroding confidence in the test suite.

Strategic Alignment: The project's "flaky tests are broken tests" rule makes test reliability a first-class concern, so fixing this was aligned with project standards. However, since it's already fixed, there is no remaining work to do here.

Blocking Factor and Urgency: Since the fix is already in place, this issue is neither blocking anything nor time-sensitive. It should be closed as resolved. The commit that introduced DEDUP_TEST_LOCK appears to have landed after PR #294, addressing the root cause the issue diagnosed correctly.

This issue can be closed as already fixed.

Cost Assessment

The serialization mechanism (DEDUP_TEST_LOCK) looks complete — all three tests acquire it, and reset_ensure_subscription_dedup() is only called from within those guarded tests. The flake is genuine but may be a one-time event or caused by some other parallel test mutating adjacent global state (e.g., ROOMS or another signal) that indirectly triggers ensure_room_subscription_once in a non-test path, populating ENSURE_SUBSCRIPTION_SENT outside the lock.

Assessment:

This is a low-cost fix. The issue is already partially addressed — the three tests in the dedup group already use a DEDUP_TEST_LOCK static Mutex to serialize themselves, and each calls reset_ensure_subscription_dedup() at the start. The serialization pattern is correct and complete for interference between the three tests in the group. The one remaining risk is interference from non-test production code paths: if some other test elsewhere in the binary triggers ensure_room_subscription_once concurrently (which inserts into ENSURE_SUBSCRIPTION_SENT without holding DEDUP_TEST_LOCK), it could corrupt the dedup state mid-test. However, since the global is a static LazyLock<Mutex<HashSet<...>>> with no wasm/async paths reachable from a native test binary, and the production insertion sites (line 904, line 924) would only be invoked from WASM signal callbacks that don't fire in native tests, this race is unlikely outside WASM.

The scope is a single file (ui/src/components/app/chat_delegate.rs) and at most a handful of lines. The fix could be as simple as verifying the lock discipline is complete, or adding a brief investigation to confirm whether any other test in the binary touches ENSURE_SUBSCRIPTION_SENT without holding DEDUP_TEST_LOCK. If such a site is found, the fix is to add DEDUP_TEST_LOCK.lock() there too, or to ensure those paths can't fire in native test runs.

Technically this is trivial — no algorithm changes, no new abstractions, no protocol or contract changes. The only complexity is identifying the exact source of interference, which requires a short investigation (grepping for ENSURE_SUBSCRIPTION_SENT usages in test contexts, reviewing what tests exist in the binary). Given the serialization infrastructure already exists and is clearly documented, a skilled developer could resolve this in under an hour. There are no external dependencies, no third-party services, and no coordination requirements.

Risk is negligible. The change touches only test-infrastructure code (#[cfg(test)] blocks or test module setup), so production behavior is unaffected. The fix itself — strengthening lock discipline or isolating state — cannot introduce regressions in production code. Testing effort is minimal: the fix is validated simply by running cargo test -p river-ui --bins repeatedly to confirm the flake is gone.

---

Generated by issue-prioritizer

[sanity]

The racing tests are now serialized by DEDUP_TEST_LOCK in chat_delegate.rs, and the tests added in #364 acquire the same lock. No test-reachable unguarded path remains, so closing as resolved. [AI-assisted - Claude]