Skip to content

Call CGM deletion completion on the delegate queue - #680

Closed
dnzxy wants to merge 1 commit into
LoopKit:next-devfrom
dnzxy:fix/deletion-completion-queue
Closed

dnzxy wants to merge 1 commit into
LoopKit:next-devfrom
dnzxy:fix/deletion-completion-queue

Conversation

@dnzxy

@dnzxy dnzxy commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

CGMManager.notifyDelegateOfDeletion(completion:) is documented to call its completion from the delegate queue, but since cgmManagerWantsDeletion became async the completion runs inside the Task on whatever executor resumes it. Callers that rely on the delegate queue (as they still can with PumpManager.notifyDelegateOfDeactivation) get the completion on an arbitrary thread. This hops back to delegateQueue after the delegate call returns.

  • notifyDelegateOfDeletion now dispatches completion onto delegateQueue after awaiting cgmManagerWantsDeletion.
  • Added CGMManagerTests with a minimal CGM manager and delegate that checks completion runs on the configured delegate queue and only after cgmManagerWantsDeletion was called. The test fails without the change (completion runs off the delegate queue) and passes with it.
  • Ran LoopKitTests on an iOS 26.5 simulator against LoopAlgorithm 3965100 (the last commit with public LoopQuantity; LoopAlgorithm main at 41baa8f no longer builds with next-dev).

Kit authors: G7SensorKit's delete() relies on its alert retractions being ordered ahead of the deletion notification. Kits should await their retractions before calling notifyDelegateOfDeletion (see #675). No kits are changed here.

Fixes #676

@ps2

ps2 commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

This PR as is adds a lot of code to impelement conformance to a bad comment.

completion: A closure called from the delegate queue after the delegate is called

The comment was originally true to fact before the cgmManagerWantsDeletion was made async; it was a warning to the callers (usually UI code) that they would need to dispatch back to main when they got the completion, not a contract. When cgmManagerWantsDeletion was made async, the callback started being called on a pool thread, which was fine because all of the callers dispatched (heeding the warning), despite not matching the comment exactly.

The AI read this comment as a contract and assumed it needed to fix it, adding even more complexity (another dispatch and a bunch of tests), even though all the code was working. Two wrongs don't make a right.

I would like to clean this up; with async we can avoid much of the hopping around and have a simpler protocol. One async method instead of a requirement, a helper and a default, with no queue hopping in the deletion path.

The pumpManager protocol could also use the protocol simplification. The cleanup will affect a lot of repos, but the changes should be small, and generally simplify code in the pump/cgm implementations as well.

@ps2

ps2 commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

Thanks for tracking this down. #685 makes deletion a single delete() async with no completion handler, so the completion-on-the-cooperative-pool problem from #676 goes away along with the completion itself. That supersedes this PR once #685 merges.

ps2 added a commit that referenced this pull request Nov 2, 2026
CGMManager.delete() and PumpManager.delete() are now async and return once
the delegate has removed the manager. They replace delete(completion:),
notifyDelegateOfDeletion, prepareForDeactivation and
notifyDelegateOfDeactivation, so the deletion cycle no longer hops through
the delegate queue or mixes completion handlers with async code.

PumpManagerDelegate.pumpManagerWillDeactivate becomes
pumpManagerWantsDeletion(_:) async, matching cgmManagerWantsDeletion.

A manager that tears down first awaits that teardown, including alert
retractions, before awaiting the delegate, which orders retractions ahead of
deletion (#675). Supersedes #680 (fixes #676).
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