Skip to content

keep-android: target DKG cancel by run id and handle Started progress update - #515

Merged
kwsantiago merged 1 commit into
mainfrom
dkg-cancel-run-identity
Aug 24, 2026
Merged

keep-android: target DKG cancel by run id and handle Started progress update#515
kwsantiago merged 1 commit into
mainfrom
dkg-cancel-run-identity

Conversation

@wksantiago

@wksantiago wksantiago commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Closes #514

Summary by CodeRabbit

  • Bug Fixes

    • Improved DKG cancellation so requests target the correct active run.
    • Cancel actions made during the initial connection phase are now preserved and applied once the run starts.
    • DKG state is reliably cleared when the operation finishes.
  • User Interface

    • Added a connecting status display when a DKG run starts.

@wksantiago wksantiago self-assigned this Aug 24, 2026
@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f269d28c-7db2-4da3-a10b-90e1e821a8c9

📥 Commits

Reviewing files that changed from the base of the PR and between e8b8e88 and d58ceb4.

📒 Files selected for processing (3)
  • app/src/main/kotlin/io/privkey/keep/AccountActions.kt
  • app/src/main/kotlin/io/privkey/keep/CreateGroupScreen.kt
  • keep.version

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

DKG cancellation now targets a specific run identifier. Early cancellation requests remain pending until Started provides that identifier. The UI displays Started as a connecting state, and keep.version points to the updated keep-mobile commit.

Changes

DKG cancellation

Layer / File(s) Summary
Track and cancel the active DKG run
app/src/main/kotlin/io/privkey/keep/AccountActions.kt
AccountActions stores the active run identifier, queues early cancellation, calls frostCancelDkg(runId), and clears run state when the job completes.
Handle Started progress and update the pinned mobile version
app/src/main/kotlin/io/privkey/keep/CreateGroupScreen.kt, keep.version
Started displays the connecting status. keep.version pins the updated keep-mobile commit.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to d58ce

The PR is merge-ready after normal checks and review; no actionable merge-blocking risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant CreateGroupScreen
  participant AccountActions
  participant DkgProgressUpdate
  participant KeepMobile
  CreateGroupScreen->>AccountActions: cancelDkg()
  AccountActions->>AccountActions: record pending cancellation
  DkgProgressUpdate->>AccountActions: Started(runId)
  AccountActions->>KeepMobile: frostCancelDkg(runId)
Loading

Suggested reviewers: kwsantiago

Poem

A rabbit tracks the DKG run,
And queues a cancel till IDs come.
Started brings the number bright,
KeepMobile ends the matching flight.
The screen says “Connecting” right.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR implements run-ID cancellation and Started handling, but the required stale-comment update and binding regeneration are not evidenced. Update the stale process-wide cancellation comment and regenerate the keep bindings required by issue #514.
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1 unsupported.) Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the per-run DKG cancellation and Started progress handling changes.
Out of Scope Changes check ✅ Passed The changes are limited to DKG cancellation, Started progress handling, and the required keep.version update.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dkg-cancel-run-identity

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@wksantiago
wksantiago requested a review from kwsantiago August 24, 2026 19:25
@kwsantiago

Copy link
Copy Markdown
Contributor

Verified end to end. Pixel 9a, Android 17: 291 tests, 0 failures, 5 skipped, matching main. Local gate green (unit tests, lintDebug, RNG hygiene guard). CI green.

The pin is right, which is the thing I most wanted to check after #513: d9192f5 is the squash-merge of #979 on keep's main and is reachable through refs/heads, not only a pull ref, so the F-Droid srclib clone path resolves it. Confirmed the bindings actually regenerate against it rather than assuming:

6308: fun `frostCancelDkg`(`runId`: kotlin.ULong)
16379: data class Started(

All three breaks I flagged on #979 are closed, and the stale process-wide-flag comment is rewritten rather than left to mislead.

The cancel race is handled correctly, and the ordering is the reason. cancelDkg sets the pending flag then reads the id; the Started handler sets the id then reads the flag. Opposite orders mean one side always observes the other's write, and the shared compareAndSet makes it fire exactly once. I walked all five interleavings — cancel fully before Started, Started fully before cancel, and the three interleaved orderings — and every one issues exactly one cancel, never zero and never two. The comments on both sides state the ordering dependency explicitly, which matters because someone "tidying" either into a more natural order would silently reintroduce the dropped-cancel window.

Clearing dkgRunId and dkgCancelPending both at run start and in invokeOnCompletion is the right pairing: the start clears anything carried over from a prior run, and the completion hook fires even for an already-cancelled scope, so neither can strand.

Rendering Started with the same string as Connecting is the right call. It carries no user-facing information beyond the id, and Connecting follows immediately, so anything distinct would be a flicker.

Not about this PR: the NDK

Worth recording because it produced a convincingly wrong answer before I chased it. NDK 29.0.14206865, the version app/build.gradle.kts pins, was missing from my SDK. The Rust cross-compile could not run, so Gradle served stale bindings — frostCancelDkg() with no argument and no Started — and the build failed exactly as though this PR did not match its own pin. Reinstalling the pinned NDK and rebuilding produced the correct bindings and a clean run.

Nothing for you to change. Flagging it only because "PR does not compile against its pin" and "local toolchain silently regressed to cached bindings" look identical from the failure output, and the second is the more likely explanation when a pin bump appears to break its own consumer.

No blockers.

@kwsantiago
kwsantiago merged commit 39ab412 into main Aug 24, 2026
4 checks passed
@kwsantiago
kwsantiago deleted the dkg-cancel-run-identity branch August 24, 2026 20:20
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.

Adapt to per-run DKG cancel (frostCancelDkg(runId) + DkgProgressUpdate.Started)

2 participants