Skip to content

fix: republish pubky identity records - #1271

Open
ben-kaufman wants to merge 2 commits into
masterfrom
codex/republish-pubky-identity
Open

ben-kaufman wants to merge 2 commits into
masterfrom
codex/republish-pubky-identity

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Description

This PR keeps the signed Pubky identity record discoverable by periodically republishing it through Paykit 0.1.0-rc55.

  • Reuses the existing foreground maintenance loop and checks on network restoration, identity activation, and auth approval. A locally held identity can be republished before restoring its session.
  • Reuses the SDK client cache, allows one publication at a time, and throttles successful publication to once every 30 minutes. Missing records or failures may retry after one minute on the next eligible trigger.
  • Keeps publication best-effort with a five-second attempt deadline: failures do not fail authentication, external coroutine cancellation propagates, and no identity record is reconstructed or changed.
  • Adds no background service or UI. Payment polling intervals and public/private payment behavior are unchanged.

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

  • 1. Signed-in Profile → background and reopen Bitkit: profile/session remains available; repeated foreground transitions do not repeatedly republish a successful record within 30 minutes.
  • 2. Scanner → approve a Pubky auth request: approval still completes when identity republishing fails.
  • 3. Background Bitkit: periodic identity maintenance stops; foreground/network restoration resumes eligible maintenance.

Automated Checks

  • Added five tests in PubkyIdentityRepublishTest.kt: success throttling/client reuse, missing-record/error retries, identity changes without session restoration, concurrent triggers, and timeout/cancellation followed by a retry.
  • Updated PubkyServiceTest.kt mocks to keep the existing relay timeout and cancellation tests independent of key conversion.
  • Kotlin compilation and all 2,547 unit tests passed with rc55 resolved from GitHub Packages; -Dmaven.repo.local=/private/tmp/bitkit-rc55-empty-maven excluded local Maven artifacts. Detekt completed successfully; existing complexity/formatting warnings in untouched code are left unchanged.
  • Staged whitespace checks passed; independent correctness/quality review completed. No live DHT outage or battery benchmark is claimed.

@greptile-apps

greptile-apps Bot commented Sep 15, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 3/5

The PR is not safe to merge until republication is bounded away from critical initialization and authorization paths and concurrent identity triggers are retained.

Findings

  1. P1 Publication Can Block Authentication
  2. P1 Concurrent Identity Trigger Is Lost

Summary

This PR upgrades Paykit and adds best-effort republication of existing Pubky identity records.

  • Adds normalized-key throttling, bootstrap-client reuse, and serialization for republication.
  • Triggers republication during SDK initialization, identity activation, authorization, foreground maintenance, and network restoration.
  • Adds unit coverage for throttling, retries, identity changes, and concurrent triggers.
  • The inline remote call can block initialization and authorization, while the non-blocking mutex can discard a newly activated identity's only immediate publication request.

Diagram

sequenceDiagram
    participant Trigger as Foreground/Auth/Activation
    participant SDK as PaykitSdkService
    participant Lock as Republish mutex
    participant DHT as Pubky network
    Trigger->>SDK: republishIdentityIfNeeded(key)
    SDK->>Lock: tryLock()
    alt another publication is active
        Lock-->>SDK: false
        SDK-->>Trigger: return without pending retry
    else lock acquired
        SDK->>DHT: republishIdentity(key)
        DHT-->>SDK: success, missing record, or failure
        SDK->>SDK: set 30-minute or 1-minute throttle
        SDK->>Lock: unlock()
        SDK-->>Trigger: continue initialization/auth
    end
Loading

Reviews (1) · Last reviewed commit: "fix: republish pubky identity records"

Comment thread app/src/main/java/to/bitkit/services/PaykitSdkService.kt Outdated
Comment thread app/src/main/java/to/bitkit/services/PaykitSdkService.kt
@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Regtest APK

Built from c61ede6 (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. At 5s withTimeoutOrNull cancels its child with TimeoutCancellationException.
  2. bootstrap().republishIdentity() is a real uniffi suspend fn in rc55 — I checked the AAR bytecode, and uniffiRustCallAsync is withContext(Dispatchers.IO) { suspendCancellableCoroutine { cont -> cont.invokeOnCancellation { ffi_paykit_rust_future_cancel_u8(handle) }; poll(...) } }. Dispatchers.IO carries no Job, so cancellation propagates and the Rust future is cancelled rather than left running.
  3. runSuspendCatching's first catch is catch (c: CancellationException) { throw c }, so it's rethrown, not swallowed into .onFailure. The "Failed to republish" warn is correctly not logged on timeout.
  4. withTimeoutOrNull catches its own exception by identity and returns null; the function continues normally.
  5. finally unlocks.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The timeout can't trigger it. withTimeoutOrNull catches its own TimeoutCancellationException by identity check inside republishIdentityIfNeeded (:257), so it never escapes to this line. Before c61ede68b the call here was unbounded; now the worst case is a 5s delay.
  2. Caller cancellation can't reach it either, at either head. PubkyRepo.approveSignupAuth goes through PubkyService.activateRegisteredIdentity, which is wrapped in ServiceQueue.CORE.background { } = withContext(scope.coroutineContext) where that context carries CORE's own SupervisorJob. withContext with a Job re-parents the block to it — the same mechanism as withContext(NonCancellable) — so cancelling viewModelScope leaves 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants