fix: republish pubky identity records - #753
ben-kaufman wants to merge 5 commits into
Conversation
|
jvsena42
left a comment
There was a problem hiding this comment.
No findings. Reviewed at d76e9eb2 as a key-material + dependency-bump change, alongside the android twin synonymdev/bitkit-android#1271. d76e9eb2 "fix: bound identity republishing" landed mid-review, so this covers both commits.
The bounding commit is correct. The extra machinery relative to android's one-line withTimeoutOrNull(5.seconds) is justified rather than gratuitous: the uniffi binding has no cancellation hook (a withUnsafeContinuation poll loop, freeFunc only after completion), so the FFI genuinely ignores cancellation as the comment at :382 says. A withTaskGroup race wouldn't work — the group awaits all children on scope exit, so the caller would block for the full publication anyway. Detaching the publication and racing a signal is the only way to release the caller early while keeping the in-flight guard tied to the real publication.
Things I specifically tried to break and couldn't:
isRepublishingIdentityleak. On the timeout pathpublication.cancel()(:393) is ignored, so the publication runs to completion and only then clears the flag viadefer(:403) — held for exactly the real FFI duration. That duration is bounded:republish_identityisresolve(CacheOnly)→resolve(NetworkOnly)→publish, and pkarr 8.0.0publishis atokio::join!of relay (reqwest,config.request_timeout) and DHT (mainline, 2s per-request). paykit-ffi never tunes the pkarr builder, so it runs on pkarr's 2s default. Worst case is seconds, not the process lifetime, so republishing can't be permanently disabled.- Deadline starvation.
deadlineis actor-isolated butTask.sleepsuspends without holding the actor, and every Rust call goes throughuniffiRustCallAsync'swithUnsafeContinuation, which releases it. The only actor-blocking work is millisecond-scale synchronous FFI/keychain calls. The 5s bound is honoured to within those. - Stream race.
Continuation.finish()is idempotent; a stream finished before iteration returns immediately (the desired fast path for the :401 early return); nothing is yielded so buffering is moot.deadlineis only cancelled in thedefer, i.e. after the loop already exited, so it always fires within the timeout while the loop runs. The thirdfinish()at :395 is redundant but harmless. - Caller cancellation. AsyncStream iteration terminates on cancellation and the wrapper returns promptly. Unstructured
Task {}children don't inherit cancellation — the explicitdefercancels are what make this work, and they do. - Lock hold.
PaykitSdkOperationLockis held across the wrapper atactivateBootstrapResult:1183 (from importSession :458, signUp :475, activateRegisteredIdentity :501, signIn :522, completeAuth :570). That hold was previously the full FFI duration and is now ≤5s; the background publication doesn't take the lock. Strictly shorter than before. - AppScene reorder. Purely ordering.
currency.refresh()is non-throwing, catches internally, and is bounded byURLSessiontimeouts, so it can't drop the republish. One nuance:scenePhase == .activeis now evaluated after the fetch, so backgrounding mid-fetch skips the republish until the next foreground trigger. Not a defect.
Carried over from the first commit — the dependency bump rc54 → rc55 is additive (only republish_identity, its FFI wrapper, version bumps and two dev-deps; no pubky/pkarr bump, no storage format change). republish_identity takes only a public key — no secret, no session, no re-signing — and the key is derived locally and normalized before any network call. The republished packet is pre-signed and pkarr rejects lower-seq packets, so publishing key B's record from a device holding key A is inert. cachedBootstrap is stateless w.r.t. sessions. Sign-out clears .paykitSession and .pubkySecretKey, so a signed-out device has no identity to republish. No payment paths touched.
Twin parity vs synonymdev/bitkit-android#1271 — both sides landed the same bounding fix at the same 5s, shaped to their platform. Everything material is present on both: the throttle (30 min / 60 s / identity-change bypass / single-flight), the republish in initialize() and activateBootstrapResult, the three approval paths, connectivity-gated poll-start and maintenance triggers, and the network-restore trigger (iOS gates on scenePhase == .active, android on the polling job being active — same meaning). Android's PubkyRepo layer is absent on iOS by structure; AppScene passes pubkyProfile.publicKey, the same identity source, so there's no safety gap.
One genuine divergence worth knowing about, and it favours iOS — see my reply on greptile's publication-blocking thread for the test-coverage note.
jvsena42
left a comment
There was a problem hiding this comment.
236c8d068 closes the coverage gap I noted. Test-only — git diff --quiet d76e9eb2 HEAD -- Bitkit is clean, so everything I verified at d76e9eb2 stands unchanged.
The added assertion is not a tautology. With defer { isRepublishingIdentity = false } (PubkyService.swift:403) removed, the first publication has already returned true so nextIdentityRepublishAt is now + 1800 and the identity matches; every retry call then enters republishIdentity, hits the guard !isRepublishingIdentity at :401 with the flag still held, and returns before reaching bootstrap().republishIdentity at :415. publicKeys.count stays 1, the loop spins to its deadline, and :109 fails 1 != 2. So it discriminates exactly the property that was missing.
The poll loop rather than my two-line suggestion is the right call, and worth recording why: finished.fulfill() fires inside the stub before the publication Task has resumed on the actor to run the defer, so a single retry call could legitimately observe the flag still held and early-return. Asserting directly would have been racy against ordering the code doesn't guarantee. Swapping bootstrap.operation at :102 before the retry is also necessary — otherwise the second publication re-fulfils started/finished and XCTest's over-fulfil check fails.
Determinism is at least as good as the existing test: no wall-clock reads, no sleeps, ContinuousClock is monotonic, and the only timing assumption is that an already-runnable Task gets its actor turn within 1s, which is the same class of bound as the 20ms and 1s deadlines already at :93 and :99. Each iteration's 5s deadline Task is cancelled by the wrapper's defer at :394, so nothing accumulates across iterations.
The caller-cancellation case is still uncovered — the android twin has both halves, iOS has retry only. That was the optional half of my note and I'm not asking for it.
Nothing further from me on this PR.
|
Applied the relevant part of João's Android emulator findings in d6bfdf0. SDK initialization now schedules identity publication without waiting for it, and the five-second caller deadline logs when it expires. iOS already allows the native publication to finish after that deadline and keeps the single-flight guard until completion, so it did not need Android's separate network-operation timeout change. Auth cancellation checks are unchanged. All 1,300 unit tests and the 130 focused identity/auth/profile tests pass, along with SwiftFormat and the final independent review. |
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 1 file.
Retest suggested: item 1 (SDK initialization now starts identity republication without waiting; Manual Test 1 exercises this reopen path).
Counterpart synonymdev/bitkit-android#1271: equivalent.
Findings:
1 inline (non-blocking)
Audit:
Already done in comment.
QA: sim-1 iPhone 17 Pro simulator on iOS 26.5, exact-head E2E regtest build d6bfdf0
-
✅ passed: the profile and session survived three background/foreground cycles; successful publication remained throttled to one attempt.
-
✅ passed: Scanner authorization reached the success screen after the forced missing-record publication failure.
-
✅ passed: no retry occurred during 75 seconds in the background; foregrounding immediately resumed eligible maintenance.
Tip
Test 1 worth a journey:
- Create and sign into a Bitkit profile
- Send Bitkit to the background
- Reopen Bitkit and verify the profile remains available
- Repeat the background and foreground transition three times
- Verify successful identity publication is not repeated
Test 2 worth a journey:
- Create and sign into a local Bitkit profile
- Restart the local Pubky testnet with an empty identity-record cache
- Open Scanner and paste a Pubky authorization request
- Tap Authorize
- Verify authorization succeeds
Test 3 worth a journey:
- Create and sign into a Bitkit profile
- Make the profile identity service unavailable
- Foreground Bitkit and verify one maintenance attempt occurs
- Send Bitkit to the background for more than 60 seconds
- Verify no maintenance attempt occurs while backgrounded
- Foreground Bitkit and verify eligible maintenance resumes
Coverage:
QA: 3 of 3 Manual Tests passed
Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer test · retest · audit (author or owner)
| } | ||
|
|
||
| func initialize() async throws { | ||
| Task { await republishIdentityIfNeeded() } |
There was a problem hiding this comment.
PaykitSdkService.initialize() now starts republication in an unstructured Task, but no test calls this initializer. Every case in PubkyIdentityRepublishTests invokes the helper or auth entry points directly. Reverting this line to the awaited call would leave the suite green and restore up to five seconds of startup delay. Could we add an initializer-level regression test that stalls republication and verifies SDK initialization proceeds before the publication gate opens?
Simulator test:
|
| # | Scenario | Result |
|---|---|---|
| 1 | Cold launch, twice (after a fresh install and after a simulator reboot) | ✅ One Republished Pubky identity per launch |
| 2 | Three background/foreground cycles, 8s in background each, within the 30-minute window | ✅ No extra republish. The throttle held and the profile stayed available |
| 3 | App left in the foreground for 30 minutes | ✅ The maintenance loop republished 30m28s after the previous success |
| 4 | Main scanner → paste a pubkyauth://signin_grant request (private random relay secret) → Authorize |
✅ Reached "Authorization Successful" within 2s of the tap. The republish on this path was inside the throttle window, so it returned immediately |
| 5 | Startup cost compared with earlier builds on the same simulator that have no republishing | ✅ No regression. "Scene active" to "Paykit session restored": 3.1s and 4.0s on this branch vs 4.5s and 4.7s before. Contacts loaded at 4.8s and 6.5s vs 6.3s and 7.5s |
Cold launch 1 (fresh install)
[2026-09-16 16:34:56.371 UTC] INFOℹ️: Scene phase changed: active - AppScene
[2026-09-16 16:35:00.360 UTC] INFOℹ️: Paykit session restored for pubkyxgpi8a4epnmb9z5m9bokaf5w6hm1myffk4odmo1gbb3ip6cpf71o - PubkyProfileManager
[2026-09-16 16:35:01.402 UTC] WARN⚠️: Stopped waiting for Pubky identity republishing - PaykitSdkService [PubkyService.swift: republishIdentityIfNeeded(publicKey:now:timeout:) line: 401]
[2026-09-16 16:35:02.446 UTC] DEBUG: Republished Pubky identity - PaykitSdkService [PubkyService.swift: republishIdentity(publicKey:now:) line: 429]
[2026-09-16 16:35:02.824 UTC] DEBUG: Loaded 1 SDK contact records - ContactsManager
Background/foreground cycles (no republish logged)
[2026-09-16 16:35:45.559 UTC] INFOℹ️: Scene phase changed: background - AppScene
[2026-09-16 16:35:52.449 UTC] INFOℹ️: Scene phase changed: active - AppScene
[2026-09-16 16:36:07.998 UTC] INFOℹ️: Scene phase changed: background - AppScene
[2026-09-16 16:36:15.836 UTC] INFOℹ️: Scene phase changed: active - AppScene
[2026-09-16 16:36:31.370 UTC] INFOℹ️: Scene phase changed: background - AppScene
[2026-09-16 16:36:39.203 UTC] INFOℹ️: Scene phase changed: active - AppScene
grep -c "Pubky identity" stayed at 2 for the whole session: the cold-start warning and its republish.
Cold launch 2 (after sim reboot) and the 30-minute periodic republish
[2026-09-16 16:38:16.050 UTC] INFOℹ️: Scene phase changed: inactive - AppScene
[2026-09-16 16:38:16.746 UTC] INFOℹ️: Scene phase changed: active - AppScene
[2026-09-16 16:38:19.882 UTC] INFOℹ️: Paykit session restored for pubkyxgpi8a4epnmb9z5m9bokaf5w6hm1myffk4odmo1gbb3ip6cpf71o - PubkyProfileManager
[2026-09-16 16:38:21.387 UTC] WARN⚠️: Stopped waiting for Pubky identity republishing - PaykitSdkService [PubkyService.swift: republishIdentityIfNeeded(publicKey:now:timeout:) line: 401]
[2026-09-16 16:38:21.593 UTC] DEBUG: Loaded 1 SDK contact records - ContactsManager
[2026-09-16 16:38:22.640 UTC] DEBUG: Republished Pubky identity - PaykitSdkService [PubkyService.swift: republishIdentity(publicKey:now:) line: 429]
[2026-09-16 16:39:16.405 UTC] INFOℹ️: Scene phase changed: active - AppScene <- paste permission alert
[2026-09-16 16:39:16.733 UTC] DEBUG: Showing sheet pubkyAuthApproval after delay - SheetViewModel
<- Authorize tapped ~16:39:34.7, success screen by 16:39:36.7
[2026-09-16 17:08:50.573 UTC] DEBUG: Republished Pubky identity - PaykitSdkService [PubkyService.swift: republishIdentity(publicKey:now:) line: 429]
Non-blocking notes
1. A misleading WARN is logged on every cold start.
PaykitSdkService.initialize() runs Task { await republishIdentityIfNeeded() } (PubkyService.swift:363) with the default 5s deadline. Nothing awaits that task, but the deadline still fires and logs Stopped waiting for Pubky identity republishing. Both cold launches logged it. In launch 2 the wait began at ~16:38:16.39 (warning time minus 5s), before the scene became active, so it was the initialize() task and not a real caller. Someone reading logs will take it for a stalled caller. Consider calling republishIdentity(publicKey:now:) directly from that fire-and-forget task so it skips the deadline wrapper.
2. The first publication of a launch takes longer than the 5s cap.
Both cold launches took about 6.0s and 6.25s, measured from when the wait began to Republished Pubky identity. In the tested flows this didn't matter: startup doesn't wait on it, and the approval arrived inside the throttle window. But when an auth approval is the first eligible trigger (for example, the poll loop skipped republishing because network.isConnected was false, or a failed attempt's 1-minute retry has come due), Authorize will stall for the full 5s before approving. Consider whether the approval path needs to wait at all, or whether a shorter deadline is enough there.
Not covered on the simulator
- Network-restoration trigger (
handleNetworkChange): the simulator shares the host's network, so connectivity can't be toggled from here. - Approval while republishing fails: there was no way to make pkarr publication fail without changing host networking. The unit tests cover it.
- Companion-claim (
bitkit://pubky-auth/setup) and Ring approvals, and delegated identities without a local key.
Local build note for reviewers: the rc54 → rc55 bump fails the first incremental build with the stale paykitFFI.h .pcm error. just clean modules fixes it (for XcodeBuildMCP's DerivedData, set BITKIT_DERIVED_DATA_PATH to that path).



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.
Linked Issues/Tasks
Design
N/A — no UI changes.
Screenshot / Video
N/A — no UI changes.
QA Notes
Manual Tests
Automated Checks
PubkyIdentityRepublishTests.swiftcover success throttling/client reuse, missing-record/error retries, identity changes, concurrent triggers, slow-call deadlines/retries, signing-identity publication before ordinary/companion auth approval even on publication failure, and cancellation during publication across ordinary/companion/Ring approval paths.