Skip to content

fix: cancel queued advertisement after service destruction - #73

Merged
bwp91 merged 4 commits into
homebridge:beta-1.3.11from
danieloleary:codex/fix-immediate-destroy-race
Aug 8, 2026
Merged

fix: cancel queued advertisement after service destruction#73
bwp91 merged 4 commits into
homebridge:beta-1.3.11from
danieloleary:codex/fix-immediate-destroy-race

Conversation

@danieloleary

Copy link
Copy Markdown

Closes #18.

What was happening

Calling advertise() and immediately calling destroy() could leave startup work waiting in the responder queue.

By the time that work ran, the service—and sometimes its mDNS server—had already been shut down. CIAO would still begin probing and could throw ERR_SERVER_CLOSED or retain a ghost advertisement.

This is the race originally described by @bauer-andreas in #18.

What changed

A destroyed service can no longer enter or continue the advertisement pipeline:

  • destroy() marks the service as destroyed before queued work can resume.
  • The responder checks that state before probing.
  • It checks again after probing, before announcing.
  • If shutdown fails, the destroyed state is rolled back so the caller can try again.

Normal advertising, unpublishing, retries, and duplicate-destroy behavior are unchanged.

Testing

The regression test reproduces the old behavior: three probes, one announcement, and a service left behind after destruction.

With this fix:

  • Both destroy timing windows are covered.
  • Fifty repeated advertise/destroy cycles leave no probes, announcements, services, or timers behind.
  • Failed shutdown and duplicate destruction retain their existing behavior.
  • All 108 tests pass on Node 22, 24, and 26.
  • Lint, TypeScript build, and open-handle checks pass.

Release note

Fixed a race that could continue advertising a service after it had been destroyed.

@github-actions github-actions Bot added the latest label Aug 8, 2026
@bwp91

bwp91 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Nice work — this is a clean fix for a genuinely awkward race, and I'm happy to take it.

I checked it out and ran it rather than reading alone: lint, build and all 108 tests pass here too, and it merges into the current beta-1.3.11 branch with no conflicts.

The part I looked hardest at is the ordering in destroy(), because that is where this kind of fix usually goes wrong:

const endPromise = this.end();
this.destroyed = true;

Calling end() first means its own assert(!this.destroyed, "Cannot end destroyed service!") still passes, and flipping the flag synchronously before the await is what actually closes the window. Getting those two the other way round would have traded one bug for a louder one. Rolling the flag back if end() rejects is the right call too.

isDestroyed() rather than reaching into private state is the right shape, and I like that the test drives fifty advertise/destroy cycles and asserts nothing is left behind — that is a much better guarantee than a single-shot reproduction.

One thing before merge: beta-1.3.11 has moved on since you branched (a few audit fixes plus two changes for #72). It merges cleanly, but could you rebase onto it so the history stays linear? The changelog also wants a bullet matching the commit subject exactly, at the bottom of the pending section:

- fix: cancel queued advertisement after service destruction (#73)

CI compares those character-for-character, so it is worth copying rather than retyping.

Thanks for picking up #18 — that one has been sat there a while.

@danieloleary
danieloleary changed the base branch from latest to beta-1.3.11 August 8, 2026 17:37
@danieloleary
danieloleary force-pushed the codex/fix-immediate-destroy-race branch from eb9e17e to 17320bc Compare August 8, 2026 17:38
@github-actions github-actions Bot added the beta label Aug 8, 2026
@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 31269916575

Warning

No base build found for commit 3ce0cd7 on beta-1.3.11.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 48.192%

Details

  • Patch coverage: 15 of 15 lines across 2 files are fully covered (100%).

Uncovered Changes

No uncovered changes found.

Coverage Regressions

Requires a base build to compare against. How to fix this →


Coverage Stats

Coverage Status
Relevant Lines: 3178
Covered Lines: 1713
Line Coverage: 53.9%
Relevant Branches: 1688
Covered Branches: 632
Branch Coverage: 37.44%
Branches in Coverage %: Yes
Coverage Strength: 134.38 hits per line

💛 - Coveralls

@bwp91 bwp91 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@bwp91
bwp91 merged commit 4bb9531 into homebridge:beta-1.3.11 Aug 8, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition when unpublishing service immediately after advertising

3 participants