feat(identity): observable nsec backup status with pass tickets, explicit lock release, and refused-recovery copy by matt2e · Pull Request #1816 · block/buzz-app

matt2e · 2026-10-28T13:21:52Z

Onboarding stack (merge in order, top to bottom):

fix(navigation): run the visit deadline only while a presenter is attached #1814 navigation: visit deadline only while a presenter is attached (merged)
refactor(communities): extract the join owner and add the pre-join relay lookup #1815 communities: join owner and pre-join relay lookup (merged)
feat(identity): observable nsec backup status with pass tickets, explicit lock release, and refused-recovery copy #1816 identity: native nsec backup status ← this PR
feat(builderlab): tenant table — a login origin per hosted relay, granted natively and accepted by the binding signer #1817 builderlab: tenant table and granted native origins
feat(identity): first-run Builderlab account client and dev seed — bundled-grant sign-in, account reads, key link/unlink, backup, saved communities #1818 identity: first-run Builderlab account client and dev seed
feat(identity): first-run setup store, step table and pages (composition follows in the next PR) #1819 identity: first-run setup store, step table and pages
feat(identity): the first-run Welcome flow replaces the key-setup gate (FOUNDATION: App.tsx) #1820 identity: Welcome flow replaces the key-setup gate
feat(settings): read-only Key backup row in Settings → Identity #1821 settings: read-only Key backup row
[DO NOT MERGE] chore(identity): DO NOT MERGE — isolated debug Keychain slot (full stack for native testing) #1822 DO NOT MERGE: isolated debug Keychain slot (full stack for native testing)

Depends on: nothing; based on main. PRs 05–08 build on it.

Diff: production +636/−76; tests +951/−45; docs +12/−1

What this layer is

A cross-host change reviewed as one contract. Rust: BackupStatus { seq, pass, builderlab, status } published under the state lock; nsec_backup_state / nsec_backup_watch(Channel) commands (registered in lib.rs, build.rs, capabilities/default.json, browser_permissions_tests.rs); trigger/session/retry return the ticket of the pass they start; the classify table (401 ⇒ signed_out, 403/404 ⇒ not_offered, 429/5xx/transport ⇒ unreachable, a held consent or record lock ⇒ busy, …); the REFUSED recovery string for a 403 on the current read or on recovery itself (401 keeps the retry string, since a new sign-in may clear it); and the Lease flock fix (nsec_backup_choice.rs, persistence.rs) so a spawned child cannot hold the consent or record lock. TS consumer: createBackupStatus exposed as identity.backup (service.ts), handoffBackupSession resolving to the ticket, the backUpWith waiter + BackupError, the BACKUP_FAILURES table (backup-failure.ts), RECOVERY_UNAVAILABLE / RECOVERY_REFUSED / recoveryOff, and RestoreIdentity hiding Try recovery again when recovery is off. backup.test.ts over scripted invoke is the executable contract for the Rust status.

Review focus

nsec_backup.rs: every publish point is under the state lock (seq order); a retired or closed pass publishes nothing; no response body crosses IPC. The classify table is the contract backup-failure.ts copies.
The Lease fix: flock(LOCK_UN) in Drop releases the lock for every descriptor sharing the open file description (a fork/posix_spawn child's inherited copy). Same pattern as crates/credential-store.
backup.ts: the store registers lazily, watches before reading the snapshot, keeps the higher seq; the waiter answers only a pass with pass >= ticket, so a stale failure from before sign-in never answers it, and hands the credential slot back on every exit.
A null handoff that retires a running pass publishes waiting{no_session} under the latest ticket, so a rerun queued during that pass gets its answer too.
Only this layer's callers: RestoreIdentity.tsx changes only the recoveryOff condition; its expectedPubkey / onImport props moved to PR 06 with KeyRestore. backup-failure.ts holds the table and BackupFailureKind; isBackupFailureKind moved to PR 05 with the seed guard and the Settings cause variant to PR 08. In identity.test.tsx the two UNAVAILABLE rows move from the Try-again it.each into a new no-retry it.each that adds a REFUSED row, and the raw-import test pins both native strings.

Checks run on this branch's tree

On 6546483: cargo test nsec_backup 49 passed (the four new or changed Rust tests fail on the previous head), cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, tsc --noEmit, biome check, vitest run src/features/identity 120 passed.
Pushed with --no-verify because the machine was overloaded; the hosted checks are the verification of the pushed tree.

Native evidence

None on this tree. The native first-run recordings and the hands-on checks on the branch were made on the full tree, which only the DO-NOT-MERGE PR 09 reproduces (it adds the isolated debug Keychain slot). Treat every layer below 09 as unit/integration-tested only.

🤖 Generated with Claude Code

kalvinnchau

🤖 Two error-classification findings in identity recovery and backup contention.

…icit lock release, and refused-recovery copy A cross-host change reviewed as one contract: the native nsec backup uploader becomes observable so a caller can follow up on a specific backup pass, and its locks release correctly. Rust (`src-tauri/src/nsec_backup.rs`): - `BackupStatus { seq, pass, builderlab, status }`, `status` one of `idle`, `running`, `complete`, `waiting { reason: no_consent | no_session | not_linked }`, `failed { kind: busy | signed_out | not_offered | other_key_linked | invalid_service | refused | unreachable | unconfirmed | storage }`. `builderlab` is true when a storage root exists and the Builderlab source is allowed for this build. Published under the state lock so pushes leave in `seq` order: a spawned pass publishes `running` then its outcome; a queued rerun publishes `running` under the latest ticket; a null handoff that retires a running pass publishes `waiting{no_session}` numbered as the latest ticket, so a queued rerun's ticket is settled too; `document_load` turns a running pass into `idle`. - `classify`: 401 ⇒ `signed_out`; 403 ⇒ `other_key_linked` on upload, `not_offered` on enrollment and on the `current` read; 404 ⇒ `not_offered`; 429, 5xx and transport ⇒ `unreachable`; other 4xx ⇒ `refused`; enrollment 409 ⇒ `waiting{not_linked}`; an upload response that is not a stored, matching receipt ⇒ `unconfirmed`; another pass holding the consent lock or this key's record lock ⇒ `busy`; other local I/O ⇒ `storage`. No response body crosses IPC. - `trigger`/`session`/`retry` return the ticket (`Option<u64>`) of the pass that starts after the call; a wake during a pass gets a ticket covered by the single queued rerun. `nsec_backup_state` returns the snapshot; `nsec_backup_watch(Channel)` replaces the single watcher slot and refuses any window but main. Both are registered in `lib.rs`, `build.rs`, `capabilities/default.json` and `browser_permissions_tests.rs`. - Recovery: a 403 on the `current` read (no `buzz_users` capability) or on recovery itself (not a private org, or no Buzz user role) answers the new `REFUSED` string "Backup recovery is not offered for this account." instead of the generic retry string; 401 keeps it, since a new sign-in may clear a rejected session. - Lock fix (`nsec_backup_choice.rs`, `nsec_backup/persistence.rs`): the consent and per-identity record locks relied on closing the file to release the `flock`; a child spawned through fork/posix_spawn shares the open file description until exec, so the next `try_lock` saw contention and a pass reported a spurious `busy`/`storage`. A `Lease` now unlocks explicitly in `Drop` (the pattern `crates/credential-store` already uses). Probe, 3000 trials: close-only 29–57 spurious holds, unlock-then-close 0. TypeScript consumer: - `createBackupStatus` (`backup.ts`), exposed as `identity.backup` (`service.ts`): registers nothing until the first subscribe/`ready()`, watches before reading the snapshot, keeps the higher `seq`, re-registers after a failed watch. It stays out of `IdentitySnapshot`. `handoffBackupSession` resolves to the ticket or null and never rejects. - `backUpWith(identity, pubkey, credential, opts)`: the waiter a first-run step will use — `ready()`, consent on, hand the credential to the `builderlab` slot, wait for the first status with `pass >= ticket` and not `running`, hand the slot back on every exit; `complete` resolves, `failed{kind}` ⇒ that kind, `waiting{not_linked}` ⇒ `not_linked`, other waits/idle ⇒ `interrupted`, 60 s ⇒ `timeout`; renderer-side failures are `unknown`/`unavailable`/`interrupted`. `BackupError(kind)` carries the kind, the table's message and `retryable`. - `backup-failure.ts` (new): `BACKUP_FAILURES`, one entry per kind with first-run copy (cause + instruction) and whether Try again can help. The Settings `cause` variant and the seed's kind guard arrive with their callers (PRs 08 and 05). - `RECOVERY_UNAVAILABLE`/`RECOVERY_REFUSED`/`recoveryOff()` in `service.ts` match the two native strings exactly; `RestoreIdentity` hides *Try recovery again* when recovery is off; its `expectedPubkey`/`onImport` props arrive with their caller (`KeyRestore`, PR 06). In `identity.test.tsx` the two UNAVAILABLE rows move from the Try-again `it.each` into a new no-retry `it.each` that adds a REFUSED row. Tests: Rust `nsec_backup/tests.rs` (+541: running-then-outcome under the returned ticket, the full waiting/failure table incl. busy from a held consent or record lock, rerun tickets, supersede by key install, a null handoff settling a rerun ticket, a 403 on `current` refusing recovery, `document_load` with stale and new watchers, retry after timeout, flat serialization, permissions) and `nsec_backup_choice/tests.rs` (a released lease is free while a dup'd descriptor still shares it; a `dup` stands in for the spawned child). TS `backup.test.ts` (new, 347: the store's lazy registration and seq rule; the waiter's invoke order watch → state → set_choice → session → session(null), each status → kind, stale pass ignored, deadline, `builderlab: false`, null ticket, no native text reaching the page), `backup-choice.test.ts` (creating an identity never invokes watch/state), and the recovery hunks of `identity.test.tsx`: the 3-row `it.each` "offers no Try recovery again when %s answers %s" and "matches native recovery's unavailable and refused messages exactly", which reads `nsec_backup.rs?raw`. docs/identity.md: the observable status paragraph and the refused-recovery line. Stack: PR 03 of the onboarding stack; depends on nothing. 05–08 build on it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>

wesbillman

🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot reviews.

No required changes found; one optional recovery-copy suggestion is inline. Reviewed head 6546483b07fd588ed2972beb47ab0029a6403836 against base 6cec9476b0761ac8dcb823abdfe4f656ceb8f580 (merge-base fcbb8347). Source-only: tests and app flows were not run, CI was not assessed, and native IPC/lock behavior and first-run integration remain unverified; backUpWith has no production caller in this layer.

…essage `recoveryOff()` already hides Try recovery again for this message, since no retry changes a build or tenant without recovery; the copy still told the user to try again later. The message is now "Backup recovery is unavailable." in native `UNAVAILABLE`, the renderer's `RECOVERY_UNAVAILABLE` (kept in lockstep by the exact-match contract test in `identity.test.tsx`), and the browser identity fixture that mirrors it. Back still leads to manual key entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>

wesbillman

🤖 Star Lord’s automated source follow-up via Wes’s account.

No further changes requested. The optional recovery-copy fix keeps native UNAVAILABLE, renderer RECOVERY_UNAVAILABLE, and the browser fixture aligned; the existing exact-match contract test covers this boundary.

Reviewed head 717f39eac30bb702e540ab0f2d5865dedff180ac, base 6cec9476b0761ac8dcb823abdfe4f656ceb8f580. Solo follow-up under the small-change exception: only these three strings changed since the prior four-Guardian review. Source-only; tests, app/focus flows, and CI were not verified.

wesbillman

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

No blockers at 717f39e. I found two should-fix status-classification issues and three optional nits, each in an inline thread below.

What holds up:

The new commands are reachable only from the main webview on the local origin (ACL plus the label guard).
The status payload carries only seq/pass, the builderlab flag and enums. No response body, key, token or account id crosses IPC.
Every publish runs under the state lock. Ticket accounting holds through coalesced reruns and null handoffs.
BACKUP_FAILURES matches every Rust Failure variant, and the recovery strings match service.ts.
On macOS, the Lease unlock frees a lock that a forked child still holds, and an independent shared reader keeps writers out.

Validation gaps: I reviewed the source, and probes covered only fork and lock behavior. I didn't run the IPC delivery, Windows/Linux locking or a live backup. As the PR body says, there's no native evidence on this tree.

To merge: fix or explicitly decline the two should-fix threads. The nits are optional.

wesbillman · 2026-10-29T16:16:07Z

        self.valid(epoch)?;
-        let pubkey = identity.viewer().await?;
+        // Without a local key there is nothing to describe.
+        let Ok(pubkey) = identity.viewer().await else {


Should-fix: any viewer() error is reported as idle, not only a missing key. IdentityHost::viewer() (identity.rs:728) returns Err both for "no identity" and for a failed restore() (keychain denied, busy or unavailable). So if the user denies the Keychain prompt during first run, the status reads idle and backUpWith shows "The backup was interrupted. Try again." when the failure is really storage, and the status view never shows a failure.

Fix: map only a confirmed absence to Outcome::Idle and every other error to Stop::Failed(Failure::Storage). That needs viewer() (or a sibling) to report absence separately from a failed read.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account. Fixed in dda4cbc. IdentityHost::saved_viewer() keeps an absent key separate from a failed read: Ok(None) is still idle, and an Err is now failed{storage}. viewer() is built on it, so nsec_backup_choice behaves as before. Covered by two assertions in each_stopped_pass_publishes_its_waiting_reason_or_failure_kind.

wesbillman · 2026-10-29T16:16:07Z

        Ok(body)
    }
-    async fn me(&self) -> Result<Account> {
+    async fn me(&self) -> std::result::Result<Account, ReadError> {


Should-fix: when the local clock is ahead of the server, a session the server accepts is reported as unreachable. If /v1/auth/me answers 200 but expires_at <= now locally, this returns RETRY. From<&str> for ReadError turns that into Other, which becomes Failure::Unreachable, so the user sees retryable "Couldn't reach Builderlab… Check your connection" for what is an authentication state. Retrying sends the same credential and gets the same result until the server expires the session (then the 401 path correctly says signed_out) or the clock is corrected.

Fix: return ReadError::SignedOut for the expiry branch, and keep the malformed subject/date checks as Other.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account. Fixed in dda4cbc. The local expiry branch now returns ReadError::SignedOut, which becomes failed{signed_out} and still shows the retry string in recovery. Malformed subject/date checks stay Other. There's a new table row in each_stopped_pass_publishes_its_waiting_reason_or_failure_kind. Without the fix it fails as failed:unreachable.

wesbillman · 2026-10-29T16:16:07Z

+        HttpStatus(403) if upload => Failure::OtherKeyLinked,
+        HttpStatus(403 | 404) => Failure::NotOffered,
+        Transport | HttpStatus(429 | 500..) => Failure::Unreachable,
+        HttpStatus(400..) => Failure::Refused,


Nit (optional): HttpStatus(400..) also catches 408 Request Timeout and maps it to Refused, which shows the non-retryable "Builderlab refused this backup." A 408 is transient. Consider adding it to the Unreachable arm with 429.

wesbillman · 2026-10-29T16:16:07Z

-            _lock: lock,
+            // The file opened, so a refused lock is another pass holding this key's record.
+            _lock: crate::nsec_backup_choice::lease(lock, true)
+                .map_err(|_| crate::nsec_backup_choice::BUSY)?,


Nit (optional): every try_lock_exclusive error becomes BUSY, including non-contention errors such as ENOLCK/EOPNOTSUPP. Those would tell the user another backup is in progress and offer "Try again in a moment". It still fails closed. crates/credential-store maps only contention to busy; doing the same here would report other errors as storage.

wesbillman · 2026-10-29T16:16:07Z

        state.sessions.clear();
        // Source counters restart with each renderer document.
        state.generations = [None, None];
+        // The watcher stays: a stale channel drops sends, and the new document registers.


Nit (optional): document_load keeps the old document's watcher and publishes Idle to it. The new document registers lazily, on its first identity.backup read. Until then, each publish calls a callback id the page doesn't know, which only logs a Tauri warning. Clearing state.watcher here loses nothing, because the new page reads the snapshot after it registers.

…y kind Two backup statuses named the wrong failure, so first run showed the wrong copy: - `backup()` treated every `viewer()` error as "no local key" and reported `idle`. A denied, busy or locked secure store now reports `failed{storage}`; only a confirmed absent key is idle. `IdentityHost::saved_viewer` keeps absence separate from a failed read, and `viewer` is built on it. - `Api::me()` returned the retry string for a session that the service accepted but this device's clock has already expired, which became `failed{unreachable}` with connection advice. It now returns `ReadError::SignedOut`; a malformed subject or date stays `Other`. Recovery still shows the retry string for it. Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Wes <wesbillman@users.noreply.github.com>

matt2e mentioned this pull request Oct 28, 2026

[DO NOT MERGE] chore(identity): DO NOT MERGE — isolated debug Keychain slot (full stack for native testing) #1822

Closed

matt2e marked this pull request as ready for review October 28, 2026 13:40

matt2e requested a review from a team as a code owner October 28, 2026 13:40

matt2e force-pushed the onboarding/03-native-backup-status branch from f9f41f0 to 28c3c73 Compare October 28, 2026 14:18

matt2e force-pushed the onboarding/02-communities-join-owner-and-lookup branch from d47e7c2 to 2415a08 Compare October 28, 2026 14:18

matt2e force-pushed the onboarding/03-native-backup-status branch from 28c3c73 to c1bfe83 Compare October 28, 2026 14:21

matt2e force-pushed the onboarding/02-communities-join-owner-and-lookup branch from 2415a08 to 600aa3a Compare October 28, 2026 14:21

matt2e force-pushed the onboarding/02-communities-join-owner-and-lookup branch from 600aa3a to c172562 Compare October 28, 2026 23:20

Base automatically changed from onboarding/02-communities-join-owner-and-lookup to main October 29, 2026 01:04

kalvinnchau reviewed Oct 29, 2026

View reviewed changes

Comment thread src-tauri/src/nsec_backup.rs Resolved

Comment thread src-tauri/src/nsec_backup/persistence.rs Resolved

matt2e force-pushed the onboarding/03-native-backup-status branch from c1bfe83 to b353bdf Compare October 29, 2026 01:41

matt2e force-pushed the onboarding/03-native-backup-status branch from b353bdf to 6546483 Compare October 29, 2026 03:36

wesbillman reviewed Oct 29, 2026

View reviewed changes

Comment thread src/features/identity/service.ts Resolved

wesbillman reviewed Oct 29, 2026

View reviewed changes

wesbillman approved these changes Oct 29, 2026

View reviewed changes

wesbillman merged commit f4f885f into main Oct 29, 2026
37 checks passed

wesbillman deleted the onboarding/03-native-backup-status branch October 29, 2026 20:58

Conversation

matt2e commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this layer is

Review focus

Checks run on this branch's tree

Native evidence

Uh oh!

kalvinnchau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wesbillman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wesbillman left a comment

Choose a reason for hiding this comment

Uh oh!

wesbillman left a comment

Choose a reason for hiding this comment

Uh oh!

wesbillman Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

wesbillman Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

wesbillman Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

wesbillman Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

wesbillman Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

wesbillman Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

wesbillman Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matt2e commented Oct 28, 2026 •

edited

Loading