fix: republish pubky identity records - #1271
ben-kaufman wants to merge 2 commits into
Conversation
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
There was a problem hiding this comment.
No findings. Reviewed at c61ede68b as a key-material + dependency-bump change, alongside the iOS twin synonymdev/bitkit-ios#753. c61ede68b "fix: bound identity republishing" landed mid-review, so this covers both commits.
The bounding commit is correct, and I checked the part that decides whether it works at all — what actually happens on timeout, given runSuspendCatching rethrows CancellationException:
- At 5s
withTimeoutOrNullcancels its child withTimeoutCancellationException. bootstrap().republishIdentity()is a real uniffi suspend fn in rc55 — I checked the AAR bytecode, anduniffiRustCallAsynciswithContext(Dispatchers.IO) { suspendCancellableCoroutine { cont -> cont.invokeOnCancellation { ffi_paykit_rust_future_cancel_u8(handle) }; poll(...) } }.Dispatchers.IOcarries no Job, so cancellation propagates and the Rust future is cancelled rather than left running.runSuspendCatching's first catch iscatch (c: CancellationException) { throw c }, so it's rethrown, not swallowed into.onFailure. The "Failed to republish" warn is correctly not logged on timeout.withTimeoutOrNullcatches its own exception by identity and returns null; the function continues normally.finallyunlocks.
So the timeout unwinds cleanly and the mutex can't leak — unlock() in finally (:276) is reached on normal completion, on timeout, on caller cancellation, and on any throwable escaping.
If the timeout fires during the synchronous section instead (initializeOrThrow at :259, the lazy bootstrap() at :266), cancellation is observed at the next suspension point: withContext(Dispatchers.IO) calls ensureActive() on entry and throws before the Rust future is created. Same unwind. initializeOrThrow is a plain non-suspend static so it can't be interrupted mid-way, and bootstrap() is a SynchronizedLazyImpl — if the factory throws, the lazy stays uninitialized and retries next call, so no half-built bootstrap is cached.
Throttle state after a timeout is right too: republishPublicKey and nextIdentityRepublishAt are written before the call (:265-266), the 30-minute success write at :268 never runs, so the next attempt for that key happens ~55s later on the 1-minute retry interval. Not stuck, not skipped.
The new test is a real regression test, not a tautology. assertEquals(5_000L, currentTime - start) pins the timeout to the constant on the virtual clock; verify(bootstrap, times(2)) after the now = 60_000 call proves both that the mutex was released and that the retry interval is a minute — a leaked mutex would make the second call a no-op at tryLock. I confirmed the direction empirically: ran the suite at head (5/5 pass), then replaced withTimeoutOrNull(...) with run and re-ran — timeout and cancellation release publication for retry fails with UncompletedCoroutinesError. Worktree removed, checkout untouched. The cancel = true half would also pass at the old head, so that one guards existing behaviour rather than adding coverage.
Carried over from the first commit — rc54 → rc55 verified against pubky/paykit-rs (29541375..4613beee): the only additions are PubkySessionBootstrap::republish_identity, its FFI wrapper, version bumps, and two dev-deps. No pubky/pkarr bump, no storage or keychain format change, no migration. Tags confirmed with git cat-file -p "<tag>^{commit}:gradle/libs.versions.toml" — v2.3.2 rc8, v2.4.0/v2.4.1 rc31, v2.5.0 rc51, master rc54.
republish_identity takes only a public key — no secret, no session, no re-signing. The identity comes from own keychain, own bootstrap result, or own session, never from the pubkyauth URL, so a remote auth request can't choose the key; invalid input returns null from PubkyPublicKeyFormat.normalized before any network call. The republished packet is pre-signed and pkarr rejects non-most-recent packets on both DHT and relay, so a replayed stale packet can't roll back the user's record and identity A's packet can't be published under identity B. Sign-out and identity-switch clear PUBKY_SECRET_KEY, so the keychain fallback stops republishing a signed-out identity. Users who never opted into Paykit generate no DHT traffic — republish is a no-op without a stored secret or live session. No payment paths touched.
Twin parity vs synonymdev/bitkit-ios#753 — both sides landed the same bounding fix at the same 5s. Android's one-line withTimeoutOrNull is the simpler of the two, and correctly so: iOS had to hand-roll an AsyncStream race because its uniffi binding has no cancellation hook, where the Kotlin binding does. Everything else is present on both: the throttle, the republish in initialize() and activateBootstrapResult, the three approval paths, connectivity-gated poll-start and maintenance triggers, and the network-restore trigger. Android's PubkyRepo layer is absent on iOS by structure, with no safety gap — AppScene passes the same identity source.
One divergence worth knowing, and it does not favour android — see my reply on greptile's blocking thread.
| val handle = handle() | ||
| handle.initialize() | ||
| publishReceiverMarkerIfLiveSessionAvailable(handle) | ||
| republishIdentityIfNeeded(publicKey = result.publicKey) |
There was a problem hiding this comment.
Recording a verified negative here, since this is the one line where android and iOS differ structurally and I want the reason written down before someone "aligns" the two.
This republish is the last statement of activateBootstrapResult, which runs inside activateRegisteredIdentity's try { } finally { if (!activated) clearRegisteredIdentityActivationLocked() } (:371). That rollback is NonCancellable and deletes PAYKIT_SESSION, PUBKY_SECRET_KEY and PAYKIT_SDK_STATE. Because runSuspendCatching deliberately rethrows CancellationException, a cancel escaping the republish would trip it — and on the signup path (PubkyRepo :1033 registerIdentity → :1038 approveRingAuth → :1041 activateRegisteredIdentity) that happens after the homeserver account and the Ring approval are already consumed, so a retry gets a 409 "User already exists".
Two things close it, and it's worth knowing both because they're independent:
- The timeout can't trigger it.
withTimeoutOrNullcatches its ownTimeoutCancellationExceptionby identity check insiderepublishIdentityIfNeeded(:257), so it never escapes to this line. Beforec61ede68bthe call here was unbounded; now the worst case is a 5s delay. - Caller cancellation can't reach it either, at either head.
PubkyRepo.approveSignupAuthgoes throughPubkyService.activateRegisteredIdentity, which is wrapped inServiceQueue.CORE.background { }=withContext(scope.coroutineContext)where that context carries CORE's ownSupervisorJob.withContextwith a Job re-parents the block to it — the same mechanism aswithContext(NonCancellable)— so cancellingviewModelScopeleaves the caller suspended rather than cancelling the block. Nothing exposes a cancel API for that scope.
The conservative version, if you'd rather not lean on the ServiceQueue re-parenting: the window is bounded to 5s instead of unbounded.
The property to preserve: if this call ever moves out of ServiceQueue.CORE.background, or the timeout is removed, the rollback becomes reachable from a cancel again. Cancellation during :1003 handle.initialize() or :1004 publishReceiverMarkerIfLiveSessionAvailable is a pre-existing instance of the same shape on master, untouched by this PR.
For contrast, iOS is safe here for a completely different reason — its republishIdentityIfNeeded is async, not async throws, so it structurally cannot propagate into the equivalent catch.
There was a problem hiding this comment.
Confirmed against the full signup path. Activation stays inside ServiceQueue.CORE.background with its own job, and the republish timeout is handled by withTimeoutOrNull. UI cancellation therefore does not trigger identity rollback during this call. Keeping the existing behavior and cancellation handling, with no additional production change.
Description
This PR keeps the signed Pubky identity record discoverable by periodically republishing it through Paykit 0.1.0-rc55.
Delegated identities without a local key are republished once the app knows their public key after authentication/session restoration. No new persisted identity or throttle state is introduced.
SDK: pubky/paykit-rs#162 — released as https://github.com/pubky/paykit-rs/releases/tag/v0.1.0-rc55.
Design
N/A — no UI changes.
Preview
N/A — no UI changes.
QA Notes
Manual Tests
Automated Checks
PubkyIdentityRepublishTest.kt: success throttling/client reuse, missing-record/error retries, identity changes without session restoration, concurrent triggers, and timeout/cancellation followed by a retry.PubkyServiceTest.ktmocks to keep the existing relay timeout and cancellation tests independent of key conversion.-Dmaven.repo.local=/private/tmp/bitkit-rc55-empty-mavenexcluded local Maven artifacts. Detekt completed successfully; existing complexity/formatting warnings in untouched code are left unchanged.