Deny a bunker approval that goes unanswered instead of waiting on it

[kwsantiago]

ServerCallbacks::request_approval requires the implementation to bound its own wait and reject when it expires. keep-cli, keep-web and keep-desktop each enforce that on their own approval channel with a 60 second recv_timeout. keep-mobile called the foreign callback straight through the FFI boundary and returned whatever it returned, so the only thing bounding the wait was the platform's good behavior. keep-android does wait 60 seconds, but nothing on this side required it to.

That matters more than one stalled request. The handler reads one request at a time: Server::run passes a single async closure to handle_notifications and awaits it per notification, so handle_nip46_event runs to completion before the next event is read. An approval that never returned therefore stopped every later request from every client, not just its own.

A synchronous foreign call cannot be interrupted, so the wait is bounded by moving the call to its own thread and timing out the answer. On expiry the request is denied. The thread outlives the timeout: it stays parked in the platform prompt until that returns, and its answer is dropped, which is safe because the channel is created per call, so nothing can read a late answer and no permission state, audit entry or rate limiter sees it.

Because those threads outlive their timeouts, the number of them is capped. The handler reads one request at a time, so more than one is outstanding only when earlier prompts timed out and their threads are still parked, which against a platform that never returns accumulates at roughly one per timeout, each holding a stack, a JVM attachment and a copy of the request. Past the ceiling the answer is a refusal without asking, so a platform that never returns costs a bounded number of threads instead of steady pressure on the whole app. The slot is released by a guard when the platform call ends, however it ends, so a callback that panics does not retire a slot for good.

The spawn itself is a refusal rather than a panic on failure. std::thread::spawn panics when the OS refuses a thread, and that panic unwinds through the handler and out of the notification loop, which stops the bunker while its status still reports running.

The timeout is 60 seconds, matching the other three surfaces, so no answer now means a denial on every surface rather than on three of four. The trait doc states that the implementation is still expected to bound its own wait; this is the backstop for one that does not.

One consequence worth knowing when reading a denial: this timer and the platform's own 60 seconds start at nearly the same moment, so an answer arriving within a few milliseconds of the deadline can be denied here after the operator approved it. Both orderings end in a denial when nobody answers, which is the direction that matters.

Unchanged: the handler still reads one request at a time, so a slow approval still delays later ones. What changes is that the delay is now bounded. Making those concurrent is not a one-line change, because the rate limiters and per-app permission counters currently rely on that ordering, and it is tracked separately.

A refusal now names the pubkey and method that was refused, and reports no answer separately from a callback that panicked, which fail closed at once rather than after the timeout. The app name is deliberately left out of the log: it is remote-derived and unbounded, where the pubkey and method are neither.

Unchanged, and worth stating because the log reads the same either way: a prompt that times out here is recorded in the audit ring on the same branch as a real refusal, so a later read cannot tell "nobody answered" from "the operator said no". Separating them needs a distinct outcome threaded through the handler and is tracked separately.

The timeout is a field rather than the constant read directly, so the test reaches it in 200ms instead of a minute; it is a private field on a private struct, reachable from neither outside the crate nor over FFI. Reverting the timeout makes that test hang past 60 seconds rather than fail, which is the behavior being removed. Removing the ceiling, or leaking the slot on a panicking callback, each fail their own test.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 2 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: d0df6469-3138-4b00-9122-3cc76ee7cb3c

📥 Commits

Reviewing files that changed from the base of the PR and between 7d94e9d and 119307e.

📒 Files selected for processing: 1

• keep-mobile/src/nip46.rs

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}