[review] mgmt + supervisor robustness (command-room scope, reconcile ordering, all-or-nothing puppet pool) #71
Labels
No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
dark/nether-voicebridge#71
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Management-bot and supervisor robustness: commands are accepted from any joined room rather than DMs only while the bot account is shared across bridged rooms (F4-1); a leave-grace timer lock-release race (F4-2); the shared puppet pool runs under
try_join_all, so one bad/revoked token drops every puppet and leaves a Discord-less zombie the supervisor still reports as up (F5-3/F8-2); and reconcile spawns replacement bridges before the removals they replace have actually torn down (F5-5).F4-1 — P2 / LIVE / CONFIRMED
Location:
crates/matrix-rtc/src/mgmt.rs:498@968da57Defect: The command handler accepts commands from ANY joined room, not just DMs — but the bot account is shared with the bridges (joined to every bridged room) and the link probe permanently joins it to arbitrary user-specified rooms, so the documented DM-only self-serve tier (#43, ManagementConfig.open_registration doc: 'direct rooms only') and the usage/refusal reply paths are exposed in every one of those rooms; the Parsed::Usage reply (line 536) is additionally sent before the ban check and outside any cooldown.
Failure scenario: open_registration=true (both auth knobs on, per validate()). Any member of a bridged Matrix voice room — or of any public room a prior link probe joined the bot into — types 'link a b' or 'ban x' in that room: the bridge-bot account replies with usage text / 'That command is operator-only' / '🚫 You are banned' into the call room, once per message with no rate limit (cooldown only gates mutating handlers); self-serve link/unlink/list also execute from there, and approval codes get posted into shared rooms instead of a DM.
Verifier: The command handler (mgmt.rs:498-507) gates only on RoomState::Joined, sender != own_user, and is_operator||open_registration — no DM/is_direct check exists on the command path (the joined_members_count()<=2 check at line 540 covers only Parsed::Unknown, and its comment shows the author knew the bot shares multi-user rooms). The account IS joined to every bridged voice room: the in-code comment at lines 412-415 states the account is shared with the bridges, and /sync is account-scoped with an unfiltered SyncSettings::default(). The link probe's join is explicitly permanent (doc comment lines 997-999: probe failure and unlink both leave the bot joined). With open_registration=true (which validate() at config lib.rs:1422 indeed couples to both auth knobs), any member of such a room triggers: Parsed::Usage replies (line 536) with no ban check and no cooldown (mutate_cooldown is only called inside link/unlink handlers at 1334/1587); operator-only refusals (514) and banned replies (522) into the shared room; and self-serve link/list/unlink executing with room = the shared room, so the approval-code reply (~line 1395) is posted there instead of a DM. This contradicts the documented contract in ManagementConfig.open_registration ("DM the bot (direct rooms only)", config/src/lib.rs:336-337) and the DM-only invite gate (handle_invite rejects non-direct self-serve invites), which the pre-existing bridge joins bypass. The #[cfg(test)] module (1818+) tests only pure helpers (parse_command, cooldown_check, code minting) and pins nothing about room scoping. Every step of the failure scenario re-derives from the actual code; nothing falsifies it. P2 as claimed, so no test_sketch required.
F4-2 — P2 / LIVE / CONFIRMED
Location:
crates/matrix-rtc/src/matrix.rs:229@968da57Defect: Leave-grace timer race: the timer snapshots still_absent under the participants lock, releases it, and only then removes its pending_leaves entry — a reappear that lands between those two lock acquisitions sees had_pending=true and early-returns without re-assigning, while the timer proceeds to router.release() on the stale still_absent=true snapshot, leaving a present participant with no puppet.
Failure scenario: Element Call membership flaps and the user's m.call.member re-appears within microseconds of the leave-grace expiry: timer computes still_absent=true, then apply_member_event inserts the user, finds the pending token, cancels (too late — timer already past its select), and returns early ('puppet never torn down'); the timer then releases the puppet. The user stays in the call with no Discord puppet, and no later membership refresh re-assigns it (appeared requires a 0→1 transition) until they fully leave and rejoin.
Verifier: The race is real in the actual code (crates/matrix-rtc/src/matrix.rs:229-244 vs 156-187). The grace timer snapshots still_absent under the participants lock (229-235), drops it, and only afterwards removes its pending_leaves entry (236) — two separate lock acquisitions with no re-check of the cancellation token or the set before router.release (237-239). A reappear landing between them: apply_member_event inserts the user (appeared=true, 156-162), then finds the token still in pending_leaves (178-181), so had_pending=true and it early-returns WITHOUT router.assign (182-187, on the now-false assumption "the puppet was never torn down"); its cancel() is a no-op since the timer's select already resolved on the sleep arm (222-226). The timer then releases the puppet on the stale still_absent=true snapshot. No self-heal: subsequent m.call.member refreshes have this_user_before=1 so appeared=false (deliberate no-op per the 123-129 comment), and rtc/session.rs only assigns on a NEW track subscription (line 255) while explicitly keeping membership as the owner of release (line 300-304) — in the flap scenario the user never left LiveKit, so no re-subscribe fires. Same snapshot also carries now_empty, so the race can additionally send call_active=false with a participant present. The intent comment at line 228 ("Authoritative re-check under the lock") shows this is a bug, not documented behavior; the crate's test module (line 1177+) covers only resolve_affected_user/device-id/reaper, nothing pins this interleaving. Minor overstatement in the claim only: a #52-style subscriber session rotation or a genuine LiveKit re-subscribe would eventually re-assign, so recovery is not strictly "leave and rejoin" — but the defect (present participant left puppetless, wrongly-lowered call-active, no membership-driven recovery) survives re-derivation. P2 is apt: the window is microseconds wide at grace expiry.
Regression test sketch: Not required (P2/LIVE). If desired: hard to unit-test as-is because apply_member_event takes &Room for resolve_member_name; the interleaving test belongs in matrix.rs's tests module after factoring the timer body's re-check+pending-removal into a lockstep-testable helper — arrange participants={user}, pending_leaves={user:token}, simulate timer having snapshotted still_absent=true, run the reappear path, assert either the timer skips release or the reappear path re-assigns.
F5-3 — P2 / LIVE / CONFIRMED
Location:
crates/bridge/src/supervisor.rs:468@968da57Defect: When the shared puppet pool future ends with a fatal error (SharedPool::run uses try_join_all at puppet.rs:664, so ONE puppet client error drops every other client future and returns), the supervisor only logs and sets pool_done=true — all bridges keep running with the entire Discord side dead, and the process never exits or restarts.
Failure scenario: One puppet bot token is revoked (or its client hits a fatal gateway error) while 8 bridges are live: try_join_all returns Err, every serenity client is dropped (all puppets vanish from Discord), pool_done=true; every bridge keeps its Matrix/LiveKit side up indefinitely, so the daemon looks healthy to systemd/monitoring while no audio can cross in either direction, until a human notices and restarts.
Verifier: Re-derived from code: (1) puppet.rs:664-691 — try_join_all resolves Err on the FIRST puppet client error and SharedPool::run returns that Err (its own doc says "Returns on the first client error"), dropping the pinned clients future and thus every other serenity Client — all puppets disconnect. (2) supervisor.rs:440-470 — the pool_fut wrapper only tracing::error!-logs the Err; the select arm sets pool_done=true and nothing else: no bridge cancellation, no pool_shutdown.cancel(), no restart, no exit. (3) supervisor.rs:511 — the loop only exits when bridges.is_empty() && pool_done, so with 8 live bridges the daemon keeps running indefinitely, Matrix/LiveKit sides up, Discord side dead, looking healthy to systemd. (4) No test pins this as intended (supervisor tests cover only tenant-diff/validate/panic-guard); the comment at supervisor.rs:437-439 assumes the pool "only ends on pool_shutdown", which the error path violates — an oversight, not a documented invariant; CLAUDE.md's lifecycle guidance says nothing endorsing keep-running-on-pool-death. The one live-ish premise — serenity's client.start() returning Err on a revoked token / fatal gateway close (4004) instead of retrying forever — is standard serenity behavior and is exactly the error path the author's own doc anticipates, so no live probe is needed. Severity P2 stands: silent full-Discord outage masked as a healthy process.
Regression test sketch: Not required at P2, but localizable: refactor SharedPool::run's per-client future construction so the client set is injectable, or more cheaply extract the supervisor's pool-arm policy into a testable function; in crates/bridge/src/supervisor.rs tests, mirror run()'s select with a pool_fut that resolves immediately (simulating try_join_all Err) while one never-ending bridge future is in
bridges, and assert the chosen policy fires (e.g. all bridge roots cancelled / supervisor returns) instead of looping forever — currently it would hang, pinning the bug.F8-2 — P2 / LIVE / UNVERIFIED (needs live)
Location:
crates/discord/src/puppet.rs:686@968da57Defect: SharedPool::run drives all puppet clients under future::try_join_all, so ONE client's fatal error (bad/revoked token, fatal gateway error) drops every other puppet's gateway too, and the supervisor (supervisor.rs:468/511) merely sets pool_done=true and keeps the process running — a Discord-less zombie that still claims its bridges are up.
Failure scenario: Pool of 3 tokens, one revoked → that client.start() errors at startup → try_join_all resolves Err and drops the two healthy clients → all puppet gateways close, no anchors/puppets ever join, presence/ghosts never update; the process keeps running (Matrix side alive) until an operator reads the single 'puppet pool exited with error' log line.
Verifier: verifier unavailable
F5-5 — P2 / LIVE / CONFIRMED
Location:
crates/bridge/src/supervisor.rs:385@968da57Defect: reconcile_tenants spawns plan.add entries immediately after only initiating removals (stop_bridge = cancel, not completion), contradicting the ReconcilePlan doc at line 623 ("removals and replacements free resources before additions claim them") — an added tenant that reuses a removed tenant's resources starts while the old bridge is still draining, and validate_desired cannot catch it because the removed tenant's keys are neither in static_keys nor in desired.
Failure scenario: A single tenants.toml edit renames a tenant entry ("a" removed, "b" added) while keeping an explicit matrix.store_path (or the same room/channel): on SIGHUP, spawn_bridge for "b" opens the same SQLite crypto store still held by "a"'s bridge for up to 10s → store open fails (spawn failure, tenant down until next reload) or two matrix-sdk sessions contend on one store — the exact lock/corrupt hazard the duplicate-store_path validation exists to prevent.
Verifier: Re-derived from code: reconcile_tenants (supervisor.rs:373-395) applies plan.add via spawn_bridge in the same synchronous pass after removals were only cancelled (stop_bridge = rb.root.cancel(), line 306-310); all freeing (pool.unregister_channel, running.remove, store-handle drop) is deferred to on_bridge_complete, up to TEARDOWN_TIMEOUT=10s later. The added bridge's future runs resolve_store_device_id + build_client immediately (matrix-rtc/lib.rs:64-96), and build_client unconditionally opens the SQLite store at store_path (matrix.rs:528-531), so a rename that reuses the removed tenant's store (explicit store_path kept, or a slug-colliding rename like "Guest Voice"→"guest_voice" — diff_tenants keys on the raw name string while store_path/device derive from slugify) overlaps two live sessions on ONE store. It is actually worse than claimed: the new bridge reads the old bridge's device_id marker and REUSES the exact device id the draining bridge is still live on (matrix.rs:378-383) — the two hard uniqueness hazards LiveKeys documents (to-device theft, store lock/corrupt) — and the plain-poison/self-heal branches would delete sqlite files the old bridge holds open, violating reset_bot_crypto_store's own SAFETY comment (matrix.rs:318-320). validate_desired provably cannot catch it (supervisor.rs:607: live = static_keys.clone() + desired only; draining tenants checked nowhere). Reachability holds: tenants.toml is a documented operator-edited SIGHUP-reload file. Nothing pins this as intended — the opposite: ReconcilePlan's doc (line 623-624) claims removals free resources before additions, and the replace path explicitly defers respawn to the completion handler ("Never spawn a replacement inline… store handles closed", lines 127-130), a protection the remove→add rename path lacks; tests only cover the pure diff plus an idealized synchronous apply_plan, never the async drain. One phrasing nit in the claim: the second open would not fail inside spawn_bridge (spawn never touches the store); failure surfaces inside the bridge future → "bridge failed" exit with no respawn — same practical outcome (tenant down until next reload). Core defect and P2 severity confirmed.
Regression test sketch: Not required (P2). If wanted: in supervisor.rs tests, a tokio::test that builds a Supervisor with a stub bridge future holding a marker "store lock" (e.g. an exclusive file lock or an Arc flag set while alive, cleared on cancel-completion after a delay), reconciles a rename (remove "a"/add "b" sharing the resource key), and asserts the add is not spawned (or its future not started) until the removed bridge's completion handler ran — i.e. plan.add for resource-overlapping entries must go through the pending_respawn-style deferred path.
Filed from an automated multi-agent code review of
master@968da57(8 scoped finders + adversarial verification). Every finding below was re-derived from the code by an independent verifier. Fixes for the confirmed local-verifiable defects (allocator anchor panic F1-1/F1-2/F1-3/F2-1/F2-2, capture-sink panic F2-7, config env-fallback F5-4, opt-out RMW race F5-2) already landed on branchrefactor/review-and-split.Fixed and merged to master (
5c64a2a,d36e59e,65b1e1d), one commit per finding. F4-2 (leave-grace lock-release race) was deliberately skipped as not small-and-obviously-safe — left for a dedicated change.puppet client failed — other puppets keep serving ... 401: Unauthorized(serenity returns Err, not a hang) and the good puppet serving.Validation: built into
integration/parked-fixes(master + #67 + #68 + #71, merged in that order — the two overlap regions inrun_ghostandon_bridge_completeverified by hand), runner-green (workspace tests + clippy-D warnings), and live-tested on staging with production up:smoke_open_close+audio_d2m_enc(NCC 1.000) +audio_d2m_unenc(0.900) +audio_m2d_enc_freshjoin(0.985) all pass, zero bridge errors, clean ghost teardown (ghost retired → pool drained → all bridges drained cleanly), and each scenario's no-phantom-membership check passed. Shipped in v0.3.0-alpha.15 and deployed to prod.