test(ui): ban_removal_set_terminates_on_a_cycle hangs instead of failing (3 orphans burned 8 days of CPU on nova)

[sanity]

ban_removal_set_terminates_on_a_cycle (ui/src/components/members.rs) is designed to hang rather than fail if the cycle guard regresses:

(Test hangs rather than fails if this regresses, which is still a loud CI signal.)

It is not a loud signal in practice. On nova I found three orphaned copies of the river_ui test binary running this exact test, each burning ~96% of a core for 8 days (PPID 1 — their cargo parents had long since exited):

    PID     ELAPSED CMD
3530769  8-07:08:40 .../worktrees/feat-impersonation/target/debug/deps/river_ui-<hash> ban_removal_set_terminates_on_a_cycle
4010245  8-06:08:48 .../worktrees/feat-impersonation/target/debug/deps/river_ui-<hash> ban_removal_set_terminates_on_a_cycle
4091413  8-05:57:10 .../worktrees/feat-impersonation/target/debug/deps/river_ui-<hash> ban_removal_set_terminates_on_a_cycle

I killed them (they were 3/16 cores of permanent load on the dev box). The binaries were built from the feat-impersonation worktree, so I have not confirmed whether ban_removal_set on main also fails to terminate — the branch's version may be the one that regressed. Either way there are two problems:

1. A hang is a bad failure mode for this assertion. In CI it burns the job's full timeout; locally it leaves an orphan spinning indefinitely with no output. Neither is loud. Prefer making non-termination detectable: give ban_removal_set an explicit visited-set (or an iteration cap of members.len()) and assert the walk completes and returns the expected set. A test that can only signal by never finishing cannot distinguish "regressed" from "still running".
2. Something apparently did hang, at least on that branch — worth confirming whether the cycle guard on main is intact.

Suggested fix: add the visited guard to ban_removal_set itself (the UI mirror walks half-applied or hostile state, so it cannot rely on verify having rejected circular invite chains), and keep the test as a normal terminating assertion.

[AI-assisted - Claude]

[sanity]

Issue Priority Assessment

Generated: 2026-08-14T01:23:50.907043441+00:00
Issue hash: 2651ad22

Value Assessment

The code on main today (ban_removal_set, members.rs:768-789) already has the removed.insert(*child) visited-set guard the issue asks for — the walk uses a stack/removed HashSet with insert gating pushes, so it cannot loop on a cycle. This means the underlying cycle guard the issue worried might be missing is intact on main; the reporter explicitly said they hadn't confirmed whether main (vs. the feat-impersonation branch they found the orphans on) had this same regression, and it evidently didn't — this looks like a branch-local bug, not a main defect.

What's still true and unaddressed is problem #1: the test itself (ban_removal_set_terminates_on_a_cycle, line 4287) is written to hang forever if the guard ever regresses, rather than failing with a bounded timeout or an explicit iteration cap. That's a real, if narrow, test-hygiene gap — a future edit to this function that reintroduces an infinite loop would silently spin instead of failing CI, and locally could again leak an orphaned process indefinitely (exactly what happened on nova, burning a core for over a week per orphan). But note the function under test is not itself broken today; only the test's failure mode is fragile.

Impact is low: this is a single unit test in River's UI crate affecting CI hygiene and dev-box resource use, not user-facing behavior — no River users are affected, and it doesn't block any roadmap work. It's not urgent, but it's a cheap, well-scoped fix (bound the test with a timeout or an explicit visited-set/iteration-cap assertion rather than relying on the production code's own guard to prevent a hang) that would prevent a repeat of the exact orphaned-process CPU leak already observed. I'd rate this low priority — worth a quick fix when someone's touching this file, not worth interrupting other work for.

Cost Assessment

Cost Assessment

Relevance check: The core diagnosis in the issue is partly stale. On main today, ban_removal_set (ui/src/components/members.rs:768-789) already has the visited-set guard the issue asks for — removed.insert(*child) gates every push onto the walk stack, so the function structurally terminates on a cycle. The issue's author explicitly flagged they hadn't confirmed whether main was affected (only a feat-impersonation worktree build was seen hanging), and it appears main was never broken — the guard already existed when this test was written. So there is no production code change needed for ban_removal_set itself. What remains actionable is exactly the secondary point: the test ban_removal_set_terminates_on_a_cycle (members.rs:4287-4297) still relies on "hang forever" as its only regression signal, with a comment acknowledging this is intentional but weak. That test-hygiene gap is real and matches the issue's complaint about orphaned processes being a bad failure mode.

Scope: Single file, single test function. No contract, delegate, or wire-format code is touched — this is purely a UI-crate unit test change in ui/src/components/members.rs. No migration concerns (per wasm-safety.md/delegate-migration.md) since members.rs under ui/ isn't part of delegate/contract WASM.

Complexity: Low. The fix is to replace "call the function and hope it returns" with a bounded assertion — e.g., run the walk with an explicit iteration cap (members.len() as suggested) inside the test, or spawn the call on a thread and join with std::thread::Builder::spawn + a timeout, panicking if exceeded. Since the guard already exists in the production function, the simplest version is to just assert the returned set is correct (the existing assert! lines already do this) and additionally assert the walk didn't exceed a bound — mostly a matter of picking the idiom the codebase prefers for a "must terminate" test. No tricky edge cases: the existing ban_chain() test fixture and hand-forged cycle are already correctly constructed.

External dependencies / Risk / Testing effort: None — no external services, approvals, or coordination needed. Risk of regression is minimal since this only touches one test's internals, not shared helpers. Testing effort is trivial: modify the one test, run cargo test -p river-ui ban_removal_set to confirm it still passes and terminates quickly. Overall this is a small, low-risk, sub-hour task; the only judgment call is picking a timeout/cap mechanism idiomatic to this codebase (no existing precedent for thread-timeout tests was found in a quick scan, so a simple iteration-cap-based assertion inside the test is the more consistent choice than introducing threading).

---

Generated by issue-prioritizer

[sanity]

🤖 Automated ranking by Torvan. Not a maintainer's assessment of this issue.

Measure	Score
Value	0.253
Cost	0.147
Priority	316 of 593

Value and cost run 0 to 1 across the 593 open issues in the freenet repositories: value 1.00 is the most valuable, cost 0.00 the cheapest. Issues are compared against each other rather than scored in isolation. The queue alternates between the most valuable work and the best value for the effort, so neither expensive-but-important nor cheap-and-quick work crowds the other out.

Ranked 2026-08-31, and liable to change. Adding information here, whether that is scope, a reproduction, or a correction to this assessment, causes the issue to be reassessed and this comment updated.