laoc/stash-bookmark
Watch
1
0
Fork
You've already forked stash-bookmark
 0
Code Issues Pull requests 1 Projects Releases Packages Wiki Activity

Launch the signer intent on the main thread (relay AUTH crash) #5

Closed
laoc wants to merge 0 commits from fix/auth-crash into main
pull from: fix/auth-crash
merge into: laoc:main
Conversation 1 Commits - Files changed -
laoc commented 2026-08-06 04:38:43 +00:00
Owner
Copy link

Reported against 0.11.1 on 2026-08-06: "I get a crash when trying to AUTH to a relay." The video shows Amber's prompt for bread.nostrsms.com, the user accepting, and Stash simply gone.

Mechanism

ActivityResultLauncher.launch() is main-thread-only. Callers do not all arrive on the main thread:

  • a save comes from viewModelScope — main, fine
  • a NIP-42 AUTH challenge is answered from the SDK's own worker, because rust-nostr calls our custom signer itself

Measured on device with a thread-recording signer against the auth relay:

Instr: androidx.test.runner.AndroidJUnitRunner   mainLooper=false
DefaultDispatcher-worker-2                       mainLooper=false
DefaultDispatcher-worker-2                       mainLooper=false

So AmberSigner answers AUTH by touching the Activity Result API from a background thread. Every Amber user meeting an auth-demanding relay hits it.

Why the existing test never caught it

AuthRelayE2ETest covers AUTH and has always passed — it signs with a local key and never reaches the launcher at all. The launcher is the part that has the thread requirement.

The fix

Marshal to main inside ActivityResultSignerLauncher, which owns the Activity Result contract, rather than at each call site — a future signer path would otherwise have to remember the rule on its own.

Testing

SignerLaunchThreadE2ETest drives the real ActivityResultSignerLauncher from Dispatchers.Default (the AUTH shape, asserted as a precondition) with a fake inner ActivityResultLauncher that records the thread.

Red-first, with the fix removed as a verified-landed mutation (1+/1−):

without fix:  1 test, 0 passed, 1 failed
              "was called on DefaultDispatcher-worker-1 (mainLooper=false)"
with fix:     1 test, 1 passed
full suite:   expected=44 started=44 passed=44 failed=0

I first wrote this test with a fake SignerIntentLauncher — which replaced the very class being fixed and so could not see it. Retargeted at the production class.

Not a regression from the 0.11.1 speed work

NIP-42 support landed in b90b53f (2026-08-04) and shipped in 0.11.0; SignerIntentLauncher is byte-identical between 0.11.0 and 0.11.1. AUTH-with-Amber has been broken since the feature shipped — the speed changes did not cause it.

What is NOT proven

I have not reproduced the user's crash with real Amber — no signer app on the emulator. What is measured is the off-main call and the main-thread requirement; the causal link to their specific stack trace is inference. Their logcat would confirm it, and they offered one.

Reported against 0.11.1 on 2026-08-06: *"I get a crash when trying to AUTH to a relay."* The video shows Amber's prompt for `bread.nostrsms.com`, the user accepting, and Stash simply gone. ### Mechanism `ActivityResultLauncher.launch()` is **main-thread-only**. Callers do not all arrive on the main thread: - a save comes from `viewModelScope` — main, fine - a **NIP-42 AUTH challenge is answered from the SDK's own worker**, because rust-nostr calls our custom signer itself Measured on device with a thread-recording signer against the auth relay: ``` Instr: androidx.test.runner.AndroidJUnitRunner mainLooper=false DefaultDispatcher-worker-2 mainLooper=false DefaultDispatcher-worker-2 mainLooper=false ``` So `AmberSigner` answers AUTH by touching the Activity Result API from a background thread. Every Amber user meeting an auth-demanding relay hits it. ### Why the existing test never caught it `AuthRelayE2ETest` covers AUTH and has always passed — it signs with a **local key** and never reaches the launcher at all. The launcher is the part that has the thread requirement. ### The fix Marshal to main inside `ActivityResultSignerLauncher`, which owns the Activity Result contract, rather than at each call site — a future signer path would otherwise have to remember the rule on its own. ### Testing `SignerLaunchThreadE2ETest` drives the **real** `ActivityResultSignerLauncher` from `Dispatchers.Default` (the AUTH shape, asserted as a precondition) with a fake inner `ActivityResultLauncher` that records the thread. Red-first, with the fix removed as a verified-landed mutation (1+/1−): ``` without fix: 1 test, 0 passed, 1 failed "was called on DefaultDispatcher-worker-1 (mainLooper=false)" with fix: 1 test, 1 passed full suite: expected=44 started=44 passed=44 failed=0 ``` I first wrote this test with a fake `SignerIntentLauncher` — which replaced the very class being fixed and so could not see it. Retargeted at the production class. ### Not a regression from the 0.11.1 speed work NIP-42 support landed in `b90b53f` (2026-08-04) and shipped in **0.11.0**; `SignerIntentLauncher` is byte-identical between 0.11.0 and 0.11.1. AUTH-with-Amber has been broken since the feature shipped — the speed changes did not cause it. ### What is NOT proven I have not reproduced the user's crash with real Amber — no signer app on the emulator. What is measured is the off-main call and the main-thread requirement; the causal link to their specific stack trace is inference. **Their logcat would confirm it**, and they offered one.
Launch the signer intent on the main thread
All checks were successful
build / build (pull_request) Successful in 6m56s
Details
0955d3d950
laoc commented 2026-08-06 10:33:26 +00:00
Author
Owner
Copy link

Closed because it has landed, not because it was dropped: fix/auth-crash is an ancestor of main (ff39aee) via #6. Verified with git merge-base --is-ancestor.

The branch is deliberately left in place. The relay-AUTH crash is live on the published 0.11.0/0.11.1, and main now also carries three features that have not been through a phone. If a 0.11.2 hotfix carrying only the crash fix turns out to be the right move, this one commit (0955d3d) is what it is built from — branch from the pre-merge main (5290607) and take it. Closing the PR does not remove that option; deleting the branch would.

Closed because it has landed, not because it was dropped: `fix/auth-crash` is an ancestor of `main` (`ff39aee`) via #6. Verified with `git merge-base --is-ancestor`. **The branch is deliberately left in place.** The relay-AUTH crash is live on the published 0.11.0/0.11.1, and `main` now also carries three features that have not been through a phone. If a 0.11.2 hotfix carrying only the crash fix turns out to be the right move, this one commit (`0955d3d`) is what it is built from — branch from the pre-merge `main` (`5290607`) and take it. Closing the PR does not remove that option; deleting the branch would.
laoc closed this pull request 2026-08-06 10:33:27 +00:00
All checks were successful
build / build (pull_request) Successful in 6m56s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
laoc/stash-bookmark!5
No description provided.