Skip to content

DKG: scope frost_cancel_dkg to a per-run identity - #979

Merged
kwsantiago merged 2 commits into
mainfrom
dkg-cancel-run-identity
Aug 24, 2026
Merged

DKG: scope frost_cancel_dkg to a per-run identity#979
kwsantiago merged 2 commits into
mainfrom
dkg-cancel-run-identity

Conversation

@wksantiago

Copy link
Copy Markdown
Contributor

Fixes #969.

frost_cancel_dkg set one process-wide flag with no run identity, so a cancel issued for a run that has since finished could abort a later run. Each frost_run_dkg now mints a monotonic run_id and its own cancel flag, registered for the run's lifetime and cleared on every exit path. The id reaches the UI via a new DkgProgressUpdate::Started { run_id }, and frost_cancel_dkg(run_id) flips the flag only when it matches the active run.

@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 57 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 09b66478-f984-484f-8fbe-8978bdea4f89

📥 Commits

Reviewing files that changed from the base of the PR and between 750d8f5 and d55f769.

📒 Files selected for processing (2)
  • keep-mobile/src/lib.rs
  • keep-mobile/src/types.rs

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 17:28
@kwsantiago

Copy link
Copy Markdown
Contributor

The Rust is right. Local gate clean: cargo fmt --check, clippy --all-targets -D warnings, 324 tests including the new cancel_targets_run_by_id.

Per-run identity is the correct fix rather than, say, clearing the flag more carefully, and the details hold up. The Drop impl deregisters dkg_active before releasing dkg_in_flight, so a next run cannot observe a stale identity through the window between the two — reversing those two lines would reintroduce a narrower version of the same bug. Mutex poisoning is handled uniformly with unwrap_or_else(|p| p.into_inner()) rather than unwrap(), so a panic in one run does not wedge cancellation for every later one. The test covers all four cases that matter: mismatched id ignored, matching id honoured, stale id not hitting the next run, and no-active-run as a no-op.

This breaks keep-android at the next pin bump, in two places

Both are compile-time, which is the good outcome — they fail loudly rather than silently — but they are worth naming now so the companion PR is not discovered by a red build.

AccountActions.kt:433 calls keepMobile.frostCancelDkg() with no argument; the new signature takes run_id.

CreateGroupScreen.kt:972, dkgStatusText, is an exhaustive when over DkgProgressUpdate with no else branch, so adding Started makes it non-exhaustive and it stops compiling. That missing else is a feature here: it guarantees the new variant gets a deliberate rendering decision instead of being silently swallowed.

Worth flagging that keep's android CI job asserts the bindings generate, it does not compile keep-android against them, so CI here will stay green and the break only surfaces when keep.version moves.

The Kotlin side is also not a mechanical fix. It has to capture run_id from Started and thread it to cancelDkg(), which means holding per-run state the current code does not have. AccountActions.kt:71 also carries a comment describing the old process-wide flag that will need rewriting.

One thing I checked and it is not a regression: a cancel pressed before Started arrives is a no-op under the new scheme, since there is no id to target yet. The old code reset dkg_cancel to false at the top of frost_run_dkg, so an early cancel was discarded there too. Same window, same outcome, just now visible in the API rather than hidden in a flag reset. Given AccountActions.kt:485 synthesises a Connecting state before the run actually starts, the UI does show a cancellable state during that window, so it may be worth having the Kotlin queue a pending cancel and apply it on Started — but that is a keep-android decision and not a blocker here.

No blockers on this PR.

@wksantiago

Copy link
Copy Markdown
Contributor Author

@kwsantiago — filed the keep-android companion you flagged: privkeyio/keep-android#514. It tracks all three breaks (the no-arg frostCancelDkg() call site, capturing run_id from Started, and the else-less when in dkgStatusText), the stale process-wide-flag comment, and the keep.version/bindings bump. It also carries your optional UX note about queuing a cancel pressed during the synthesised Connecting window and applying it on Started.

On re-review: nothing has changed here since your pass. Both commits (ddd0a80, d55f7697) predate your review, HEAD is still d55f7697, CI is green (incl. android), and it's mergeable/clean. So there's no new delta to look at — no re-review needed unless you want a second set of eyes. The keep-android side is blocked on this merging so its pin can move from the placeholder to #979's squash-merge SHA.

@kwsantiago
kwsantiago merged commit d9192f5 into main Aug 24, 2026
12 checks passed
@kwsantiago
kwsantiago deleted the dkg-cancel-run-identity branch August 24, 2026 18:23
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.

DKG: cancel flag has no run identity, can abort the wrong session

2 participants