google / google/capsem

[triage:service-04-lock-poisoning-magika-dead-acquire] std::sync::Mutex poisoning on shared caches can wedge routes; plus a pointless magika lock/drop

Open
#143 0 comments 0 reactions 0 assignees View on GitHub
component:service type:bug
Dominant language
Rust
Stars
72
Forks
13
Avg merge
1d 2h
Merged PRs (30d)
5

Description

Imported from Capsem triage report `service-04-lock-poisoning-magika-dead-acquire.md`.

- Severity: `low`
- Category: `bug`
- Area: `capsem-service`
- Location: `crates/capsem-service/src/main.rs:2737-2739` (dead lock/drop), and the ~96 `*.lock().unwrap()` sites on `ServiceState` mutexes (e.g. `:781`, `:2027`, `:2035`)
- Confidence: `verified`

## Summary
Every shared cache and the `magika`, `instances`, `persistent_registry` maps are `std::sync::Mutex`
accessed with `.lock().unwrap()`. If any thread panics while holding one of these guards, the mutex
is poisoned and every later `.lock().unwrap()` on it panics — permanently wedging the affected
routes for the life of the daemon (axum converts the per-request panic to a 500 but never re-arms the
mutex). `magika::Session::identify_file_sync` runs under such a guard inside `spawn_blocking` (a
likely panic site for malformed input), making the magika mutex the most exposed.

Separately, `handle_list_files` acquires the magika mutex and immediately drops it before doing any
work — a dead acquisition that does nothing but contend the lock.

## Evidence
Dead acquire/drop:
```rust
// main.rs:2737-2739
let magika = state.magika.lock().unwrap();
// We can't send MutexGuard across threads; re-acquire inside spawn_blocking
drop(magika);
```
The guard is taken and dropped with no use; the real work re-locks inside `spawn_blocking` via
`&state_clone.magika`. The two lines are pure overhead (and a no-op comment).

Poisoning exposure: `identify_file_sync` (fs_utils.rs:68) does `magika.lock().unwrap()` then calls
into the `magika` crate on arbitrary uploaded/listed file bytes. A panic there poisons the mutex; all
subsequent `handle_list_files` / `handle_download_file` / `handle_upload_file` calls then panic at
`state.magika.lock().unwrap()`.

## Impact
A single panic under any of these locks turns a transient failure into a permanent per-route outage
until the daemon is restarted. Low severity because it requires a panic while the guard is held and
does not crash the whole process, but it is a latent availability cliff on the file routes.

## Suggested fix
- Delete the dead `magika.lock()/drop` pair at main.rs:2737-2739.
- Recover from poisoning instead of `.unwrap()` on the cache/magika mutexes (e.g.
`.lock().unwrap_or_else(|e| e.into_inner())`), or ensure the work under each guard cannot panic.
For `identify_file_sync`, catching the panic (`catch_unwind`) inside the `spawn_blocking` closure
before it can poison the shared `magika` session is the durable fix.

## Triage
Confirmed from the local reviewed report in `/Users/elie/git/capsem/tmp/bugs/service-04-lock-poisoning-magika-dead-acquire.md`. Track implementation in the triage sprint; add regression coverage before fixing.

Contributor guide

Open the contributing guide

Research direction

Start in crates/capsem-service/src/main.rs:2737-2739 and fs_utils.rs:68, then review the cited ServiceState .lock().unwrap() sites and existing route handlers. Reproduce or add regression coverage for mutex poisoning and the dead magika acquisition. Done means the pointless lock/drop is removed, poisoned locks have a defined recovery or panic-safe path, and file routes remain usable after a guarded operation panics.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.