Skip to content

fix(service-discovery): drain bootstrap futures on shutdown#2823

Merged
vladopajic merged 4 commits into
masterfrom
copilot/bugfix-auto-advertise-on-start
Jul 14, 2026
Merged

fix(service-discovery): drain bootstrap futures on shutdown#2823
vladopajic merged 4 commits into
masterfrom
copilot/bugfix-auto-advertise-on-start

Conversation

Copilot AI commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

auto-advertise on start could leave a service-discovery RPC stream alive after teardown because bootstrap futures were only signaled for cancellation, not drained. This change makes shutdown wait for those bootstrap cancellations to complete so the associated streams close before tracker checks run.

  • Root cause

    • ServiceDiscovery.stop() cleared serviceBootstrapFuts after cancelSoon()
    • bootstrap refresh RPCs could still be unwinding while the test moved on
    • one LPChannel could remain open long enough to poison later tests
  • Change

    • convert bootstrap shutdown from fire-and-forget cancellation to drained cancellation
    • preserve the existing raises: [] shutdown contract by waiting through noCancel
    • converted other future cancellations to cancelAndWait to have same pattern in this proc

Affected Areas

  • Gossipsub

  • Transports

  • Peer Management / Discovery

  • Protocol Logic

  • Build / Tooling

  • Other

Compatibility & Downstream Validation

Reference PRs / branches / commits demonstrating successful integration:

  • Nimbus:
    N/A

  • Waku:
    N/A

  • Codex:
    N/A

Impact on Library Users

No API change. Internal shutdown behavior is stricter: service-discovery now waits for bootstrap work to finish cancelling before completing stop, which removes a teardown race without changing steady-state protocol behavior.

Risk Assessment

Low risk.

  • shutdown-only change in service-discovery
  • no wire format or protocol semantics change
  • possible minor increase in stop latency when bootstrap futures are active
  • reduces leaked-resource risk during teardown

References

  • Issue: bug: auto-advertise on start
  • vacp2p/nim-libp2p#2822

Additional Notes

This is intentionally narrow in scope: only bootstrap future teardown changed. Other service-discovery background loops keep their existing shutdown behavior.

Copilot AI changed the title [WIP] Fix issue with open streams after auto-advertise test fix(service-discovery): drain bootstrap futures on shutdown Jul 13, 2026
Copilot AI requested a review from vladopajic July 13, 2026 19:14
@vladopajic
vladopajic marked this pull request as ready for review July 13, 2026 19:22
@vladopajic
vladopajic requested review from a team, gmelodie and richard-ramos July 13, 2026 19:22
@vladopajic
vladopajic enabled auto-merge July 13, 2026 19:22
@codecov-commenter

codecov-commenter commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.06%. Comparing base (3a59e0e) to head (2cca903).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2823      +/-   ##
==========================================
- Coverage   81.07%   81.06%   -0.02%     
==========================================
  Files         171      171              
  Lines       31135    31134       -1     
  Branches       12       13       +1     
==========================================
- Hits        25244    25239       -5     
- Misses       5891     5895       +4     
Files with missing lines Coverage Δ
libp2p/protocols/service_discovery.nim 87.79% <100.00%> (-0.08%) ⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.


disco.serviceBootstrapFuts.cancelSoon()
disco.serviceBootstrapFuts = @[]
let serviceBootstrapFuts = move disco.serviceBootstrapFuts

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

interesting trick

# stops the advertiser maintenance loop before draining advertiser tasks,
# so shutdown cannot spawn new registration work while cleanup is running
if not disco.advertiserMaintenanceLoop.isNil():
if not disco.advertiserMaintenanceLoop.isNil:

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.

nit?

Suggested change
if not disco.advertiserMaintenanceLoop.isNil:
if not disco.advertiserMaintenanceLoop.isNil():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

adjective-like getters should not use ()

@vladopajic
vladopajic added this pull request to the merge queue Jul 14, 2026
@github-project-automation github-project-automation Bot moved this from new to In Progress in nim-libp2p Jul 14, 2026
Merged via the queue into master with commit 0fa4c94 Jul 14, 2026
38 checks passed
@vladopajic
vladopajic deleted the copilot/bugfix-auto-advertise-on-start branch July 14, 2026 13:01
@github-project-automation github-project-automation Bot moved this from In Progress to done in nim-libp2p Jul 14, 2026
gmelodie pushed a commit that referenced this pull request Jul 14, 2026
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Vlado Pajić <vlado@brrrrr.at>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: done

Development

Successfully merging this pull request may close these issues.

bug: auto-advertise on start

5 participants