[PM-39992] Wipe IPC Crypto Session When Destination is Unreachable

[quexten]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-39992
Related clients PR: bitwarden/clients#21653

📔 Objective

Currently there is a few bugs around disconnects with the IPC layer. For the crypto layer, if one side disconnects, and then comes back with a fresh crypto state, the first message will be dropped. Subsequently, a re-establishment of the session will happen. We do not want the first message to be dropped because this already leads to some breakages of the intended use-cases. Namely, downstream in the clients layer, the connection confirms the connect worked correctly by starting a version request. Currently, this fails every time after a restart of the desktop app, because of the above issue.

There are a few ways to fix this. In this PR the fix chose is to propagate through the unreachability error throughout the SDK. The transport layer can elect to inform the SDK that the destination is unreachable. In this case, the SDK crypto layer will discard the crypto session. The next time the destination is available again, a new handshake will happen.

This also prevents stale crypto sessions from sticking around forever.

The PR provides a test case for the above scenario.

🚨 Breaking Changes

[github-actions]

🔍 SDK Breaking Change Detection

SDK Version: km/cleanup-session (ad8a5c2)

⚠️ If breaking changes are detected, a corresponding pull request addressing them must be ready for merge in the affected client repository.

Client	Status	Details
typescript	✅ No breaking changes detected	Compilation passed with new SDK version - View Details
android	✅ No breaking changes detected	Compilation passed with new SDK version - View Details

---

Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm.
Results update as workflows complete.

[codecov]

Codecov Report

❌ Patch coverage is 85.59671% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.31%. Comparing base (74d2677) to head (e797cca).

Files with missing lines	Patch %	Lines
...n-ipc/src/crypto_provider/noise/crypto_provider.rs	63.63%	16 Missing ⚠️
crates/bitwarden-shared-unlock/src/tests.rs	95.09%	8 Missing ⚠️
...es/bitwarden-ipc/src/wasm/communication_backend.rs	50.00%	6 Missing ⚠️
crates/bitwarden-ipc/src/error.rs	57.14%	3 Missing ⚠️
crates/bitwarden-shared-unlock/src/follower.rs	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1229      +/-   ##
==========================================
+ Coverage   85.21%   85.31%   +0.09%     
==========================================
  Files         471      471              
  Lines       64893    65085     +192     
==========================================
+ Hits        55301    55525     +224     
+ Misses       9592     9560      -32     

☔ View full report in Codecov by Harness.
📢 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

Reviewed the IPC error-classification refactor that replaces the binary is_fatal() with a three-way ErrorKind (Fatal / Unreachable / Other) and wipes the Noise crypto session when a destination is unreachable. The change spans bitwarden-ipc (error model, crypto provider, client, WASM backend, traits) and bitwarden-shared-unlock (follower error handling plus a new encrypted reconnect test). The core reconnection behavior is well tested, but one send-failure branch diverges from both its comment and the prior propagation semantics.

Code Review Details

• ⚠️ : Recoverable (ErrorKind::Other) transport-send failures are logged then swallowed, returning Ok(()) and saving the session; the comment claims they are "still surfaced" and the prior code propagated all send failures. This also makes the Other branch in ipc_client.rs::send unreachable for transport-send errors.
  • crates/bitwarden-ipc/src/crypto_provider/noise/crypto_provider.rs:286

[claude]

⚠️ IMPORTANT: Recoverable send failure is swallowed, not surfaced — comment contradicts behavior.

Details and fix

The comment says "Every other recoverable send failure is still surfaced," but the ErrorKind::Other arm only logs and then falls through to sessions.save(...) and Ok(()). The message was never delivered, yet send() returns Ok(()).

Previously the code used communication.send(...).map_err(...)?, so any send failure (including recoverable ones) propagated to the caller. This change silently converts recoverable transport-send failures into success.

It also makes the ErrorKind::Other branch in ipc_client.rs::send (the tracing::warn! + SendError::Other mapping) unreachable for transport-send errors, since they no longer escape this function.

If the intent is to surface it (matching the sibling Fatal/Unreachable arms and the comment), add the missing return:

ErrorKind::Other => {
    error!(
        "Recoverable error sending message to {:?}: {:?}",
        destination, e
    );
    return Err(e);
}

If swallowing is intentional, the comment should be corrected and the ipc_client.rs Other handling reconciled.

[dani-garcia]

Platform changes lgtm

[eligrubb]

Looks good! enum > bool, always.