fix: stopping a sync manager cancels its in-flight tick instead of waiting it out (#1552) - #1562
Open
raman325 wants to merge 5 commits into
Open
fix: stopping a sync manager cancels its in-flight tick instead of waiting it out (#1552)#1562raman325 wants to merge 5 commits into
raman325 wants to merge 5 commits into
Conversation
…iting it out (#1552) async_stop awaited every in-flight tick, and after #1551 a tick wedged in a provider call can hold SYNCING for three operation budgets in sequence. An entry unload or reload waited that out per lock with nothing on screen. Stop now cancels the ticks it finds and waits only for them to unwind. Cancelling mid-write is safe: the lock's turn is released in the write path's finally, and the coordinator records a pending write only once the call has returned. A cancelled tick drops SYNCING back to OUT_OF_SYNC so a restarted manager does not skip the slot forever. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Entire-Checkpoint: 70804ed5d9a4
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1562 +/- ##
=======================================
Coverage 99.22% 99.22%
=======================================
Files 66 66
Lines 8739 8773 +34
Branches 522 522
=======================================
+ Hits 8671 8705 +34
Misses 68 68
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Review found that a plain cancel could land between a provider's delete and add, skip the native-user rollback, or skip the Z-Wave value DB reconcile. The write and the verification read now run on their own eager Home Assistant task under asyncio.shield: the tick can be cancelled at once while the device operation finishes on its own timeouts. The initial tick in async_start gets its own task too, so _tick_tasks never holds a task the manager does not own (it used to register Home Assistant's entity-add task, harmless only while stop merely awaited). Tests share one blocking-stub helper; the vacuous cancel test is folded into the wedged-call test, which now also checks that the write lands after stop returned. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Entire-Checkpoint: d8e9758ac485
Review of the shield design found three hazards in work nobody owns: the orphaned write kept the old provider instance's mutex while a reload built a new instance with a fresh one, so two writes could interleave on one physical lock; the orphan's exception reached no handler; and Home Assistant shutdown still waited on the tracked task. Stop now waits up to STOP_GRACE_SECONDS for in-flight ticks and cancels only what is still running. A healthy write answers well inside the window, which keeps delete-then-add sequences, user rollbacks, and the Z-Wave cache repair whole; a call still running past it is presumed wedged, and the cancel then costs nothing the wedge had not already taken. Nothing outlives the stop. The test helper now owns both events and the remaining hand-rolled pause stubs in the touched files use it. A new test pins the grace: stop is still waiting a tenth of a second in, and the write lands once the device answers. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Entire-Checkpoint: c1087dc5b5c9
…celled stop honest A tick queued behind a long read can get the lock's turn late in the stop's window; a grace counted from the stop would then cut it mid-sequence. BaseLock now records when the current holder took the turn, and async_stop gives whoever holds it a whole grace from that point. A tick still queued is cancelled without harm. asyncio.wait leaves its tasks alone when the waiter is cancelled, unlike the gather it replaced, so a cancelled stop now cancels its ticks before unwinding. The SYNCING reset on cancel is gone: no production path restarts a stopped manager, so it defended nothing. The native-user rollback catches BaseException so a cancelled write still deletes the user it created. Slot removal runs its entity removers concurrently so several wedged locks in one slot pay one grace, not one each. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Entire-Checkpoint: 4237374f74c6
… stays off the cancel path The stop grace is now a loop: the deadline moves whenever one of this manager's ticks holds the lock's turn and took it after the deadline was set, so a hand-off from another holder still gives the tick a whole window, and a turn held by someone else never extends the wait. BaseLock records the holder alongside the time. Cancel-all lives in a finally so a cancelled stop takes its ticks with it without a separate branch. The native-user rollback goes back to except Exception. On cancellation it would have awaited one more exchange with a lock just presumed wedged, while still holding its turn inside the operation budget, which held the stop for the whole budget through that path. The user it would delete is found by tag and reused on the next write. A stop that cancels the first tick while async_start awaits it no longer surfaces as the caller's cancellation. Prose that restated the grace's rationale in four places now points at STOP_GRACE_SECONDS. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Entire-Checkpoint: a56d5dd55130
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
SlotSyncManager.async_stopawaited every in-flight tick with an unboundedgather. After #1551 a tick behind a provider call that never answers sits through three operation budgets in sequence (availability probe, mutex wait, operation), so an entry unload or reload waited up to that long per lock with nothing on screen.Stop now gives in-flight ticks a bounded grace (
STOP_GRACE_SECONDS, 30 s) and cancels whatever is still running, then waits for the cancelled ticks to unwind. Nothing outlives the stop. The grace is what keeps a healthy write whole: several providers write a credential as a delete followed by an add, others roll back a half-made native user or repair the Z-Wave driver cache only after the call returns, and a cancel landing inside any of those would leave the door worse than the wait. The grace is measured from when the tick took the lock's turn, not from the stop: a tick queued behind a long read may get its turn late in the window, and a grace counted from the stop would cut it mid-sequence.BaseLockrecords who holds the turn and since when, and stop moves its deadline whenever one of its own ticks holds the turn and took it after the deadline was set, so the holder always gets a whole window while a turn held by someone else never extends the wait. A tick still queued for the turn is cancelled without harm. A call that has held the turn a whole grace without answering is presumed wedged; the tick unwinds through the write path'sfinally, which releases the turn, and a reload builds a fresh manager that reads the slot before it writes. Stop logs at info when it has to cancel.Smaller changes that fell out of review:
asyncio.waitleaves its tasks alone when the waiter is cancelled, unlike thegatherit replaced.async_startis awaiting it no longer surfaces as the caller's cancellation, which would have aborted Home Assistant's entity-add batch. The first tick does no I/O today, so this is a guard._set_credentialdeliberately staysexcept Exception. Rolling back on cancellation would be one more exchange with a lock just presumed wedged, taken while still holding its turn, which would hold the stop for the full budget through that path. The user it would delete is found by tag and reused on the next write.async_startruns as its own task. Awaited inline it registered Home Assistant's entity-add task in the set that stop may cancel.A provider-level alternative was considered and left for a follow-up: give
BaseLocka stop deadline that reschedules the current holder's ownasyncio.timeout, so refreshes and service calls are bounded uniformly and stop needs no grace machinery. It would still cut a sequence at the deadline, and it reaches into the rate limiter, so it is out of scope here.Two earlier revisions (plain cancel, then
asyncio.shieldaround the lock call) were reviewed and reworked. Plain cancel tore delete-then-add writes; shield orphaned work that kept the old provider instance's mutex across a reload, left its exception unhandled, and gave Home Assistant shutdown something new to wait on. The commit history keeps the reasoning.Tests: a new test wedges the provider call, shrinks the grace, and asserts stop returns within a second, the tick is cancelled, and the lock's turn is released. It failed before the fix (stop never returned). Further tests pin the grace itself (stop is still waiting a tenth of a second in, the write lands, and no state is written after stop began), the turn-relative grace (a tick that gets the turn late in the window is not cancelled), the cancelled-stop propagation,
async_starttracking only its own task, and concurrent slot removal. The existing tests that pinned the old "stop waits for the tick" contract now assert the new one, and the touched files share one blocking-stub helper. Four mutations (no cancel, zero grace, grace counted from stop only, cancelled stop leaving ticks alone) were each killed. Full suite green, coverage stays at 100%.Type of change
Additional information
🤖 Generated with Claude Code