client: drop poison panics and surface invite parse errors#139
Conversation
|
Audit note: this PR and #138 both edit Planned merge order: #138 first, then rebase this PR on top of main. The rebase will replace the Also noting for a follow-up (out of scope here): Generated by Claude Code |
Fixes two robustness footguns in willow-client that were both surfaced by the code-quality review under #108. Issue #114: three sites in joining.rs and listeners.rs acquired the join_links Mutex via .lock().unwrap(). If any prior caller had panicked while holding the guard, the lock would be poisoned and every future caller — including the gossip listener that processes inbound JoinRequest messages — would panic itself, permanently disabling that path. Switch the Arc<Mutex<Vec<JoinLink>>> from std::sync::Mutex to parking_lot::Mutex (added at workspace level and pulled into crates/client only) and drop the .unwrap() calls. parking_lot does not poison on holder panic, so subsequent acquisitions succeed. The new test join_links_lock_survives_panic_in_holder demonstrates this. Issue #115: accept_invite previously used unwrap_or_else(|_| Uuid::new_v4()) and unwrap_or_else(|_| ChannelId::new()) to coerce invalid invite fields into freshly minted IDs. The result was a split-brain where the joining peer thought it had joined a server that did not exist for any other peer. Add a ClientError::MalformedInvite(String) variant (the first typed error in the crate, derived via thiserror), validate the server id up front, and propagate create_channel errors via ? from inside the registry mutation closure. The new test malformed_invite_server_id_is_rejected builds a real invite encrypted for the test client, tampers the embedded server_id to "not-a-uuid", re-encodes it, and asserts that accept_invite returns ClientError::MalformedInvite mentioning server_id. Validation: cargo fmt --check, cargo clippy --workspace -- -D warnings, cargo test -p willow-client -p willow-state -p willow-channel -p willow-network, and the full just check-wasm cohort (-p willow-identity -p willow-state -p willow-channel -p willow-messaging -p willow-crypto -p willow-transport -p willow-common -p willow-network -p willow-client -p willow-web) all pass with zero warnings. Closes #114 Closes #115 Progresses #108 https://claude.ai/code/session_018TTGhL645aTR4RZWrRBLPS
57bdb86 to
9518c59
Compare
Summary
Two small robustness fixes in
willow-client, both pulled from the review under #108:std::sync::Mutex::lock().unwrap()onjoin_linksso a panic while holding the guard no longer poisons the lock and takes down every future caller (including the gossip listener that handlesJoinRequest). TheArc<Mutex<Vec<JoinLink>>>now usesparking_lot::Mutex, which is poison-free by construction.parking_lotis added once at the workspace level and pulled intocrates/clientonly.accept_invitepreviously usedunwrap_or_else(|_| Uuid::new_v4())/unwrap_or_else(|_| ChannelId::new())to silently invent fresh IDs when an invite'sserver_idor a channel name failed to parse. That split-brained the joiner from every other peer. IntroduceClientError::MalformedInvite(String)(a newthiserrorvariant — the first typed error in the crate) and propagate parse / create-channel failures with?, so the caller sees a clearanyhow::Errorthat downcasts toClientError::MalformedInviteinstead of a phantom server membership.Scope is strictly
crates/client/plus the twoCargo.tomlentries forparking_lotandthiserror. No other crates touched.Closes #114
Closes #115
Progresses #108
Test plan
cargo fmt --checkcargo clippy --workspace -- -D warnings— clean, zero warningscargo test -p willow-client— 68 tests pass including the two new regression tests:join_links_lock_survives_panic_in_holder— spawns aspawn_blockingtask that pushes a link, panics while holding the guard, then asserts the main task can still read/writejoin_linkswithout panicking.malformed_invite_server_id_is_rejected— builds a real invite encrypted for the test client, tampers the embeddedserver_idto"not-a-uuid", re-encodes, and assertsaccept_invitereturnsClientError::MalformedInvitementioningserver_id.cargo test -p willow-state -p willow-channel -p willow-network— all upstream dependents still green.cargo check --target wasm32-unknown-unknownfor the fulljust check-wasmcohort (-p willow-identity -p willow-state -p willow-channel -p willow-messaging -p willow-crypto -p willow-transport -p willow-common -p willow-network -p willow-client -p willow-web) — clean, verifyingparking_lotbuilds on WASM.https://claude.ai/code/session_018TTGhL645aTR4RZWrRBLPS