feat: enhance BasicHost shutdown

[Smartdevs17]

What was wrong?

Tests were failing with "Task was destroyed but it is pending!" warnings due to lack of proper resource cleanup in Host and Network components. Existing implementations of BasicHost and Swarm did not fully handle the shutdown of background tasks and services (like mDNS, Identify) during teardown.

Issue #92

How was it fixed?

Implemented explicit async def close() methods for BasicHost and Swarm (Network).

• BasicHost: Added cleanup logic to stop background services (mDNS, bootstrap, UPnP), cancel all pending identify tasks using trio.CancelScope, and close the underlying network.
• Swarm: Ensured close() method satisfies the INetworkService interface and properly cleans up connections and listeners.
• Tests: Updated HostFactory to automatically call close() on hosts during teardown, resolving cleanup issues in all tests using these factories. Added explicit cleanup to test_basic_host.py.

To-Do

• Clean up commit history
• Add or update documentation related to these changes
• Add entry to the release notes

[Smartdevs17]

@acul71 @yashksaini-coder @sumanjeet0012 Hey everyone, I've just pushed a fix for the "Task destroyed but pending" warnings we were seeing in the tests.

I implemented proper close() methods for Host and Network to ensure we clean up all background tasks (like identify streams and mDNS) during teardown. I also updated the test factories so this cleanup happens automatically in our test suite.

Everything is passing locally with no warnings now. PR is up.

[acul71]

Hello @Smartdevs17 thanks for this PR.

Full review here:

AI Pull Request Review: #1208 — feat: enhance BasicHost shutdown

PR: libp2p/py-libp2p#1208
Author: @Smartdevs17
Branch: feat/add-close-methods → main
Related issue: #92 (Closing network resources in tests, adding close functionality)

---

1. Summary of Changes

This PR enhances BasicHost shutdown to fix "Task was destroyed but it is pending!" warnings in tests by ensuring proper resource cleanup in Host and Network components.

What changed:

• BasicHost.close() — Implemented full cleanup: stop mDNS and bootstrap, remove UPnP port mappings (with error handling), cancel all in-flight identify tasks via a new _identify_scopes: dict[ID, trio.CancelScope], then close the underlying network.
• Identify task lifecycle — Each identify task is now started under a trio.CancelScope stored in _identify_scopes. On close(), all scopes are cancelled so identify tasks exit cleanly; the task’s finally removes its scope from the dict (with a peer_id match to avoid races).
• Tests — HostFactory.create_batch_and_listen now closes all yielded hosts in a finally block. test_basic_host.py::test_swarm_stream_handler_no_protocol_selected explicitly calls await host.close() in a finally so the host is closed even when the test raises.

Files touched:

• libp2p/host/basic_host.py — close() implementation, _identify_scopes, and identify task spawning/entry changes.
• tests/core/host/test_basic_host.py — try/finally and await host.close() in one test.
• tests/utils/factories.py — HostFactory.create_batch_and_listen teardown calls await host.close() for each host.

Note: The PR description mentions ensuring Swarm.close() satisfies INetworkService; the diff does not modify libp2p/network/swarm.py. Swarm already implements async def close() (see swarm.py around line 804) and INetwork (and thus INetworkService) already declare close(). So this PR focuses on BasicHost and test cleanup only.

Breaking changes / deprecations: None. This adds and uses existing close() APIs.

---

2. Branch Sync Status and Merge Conflicts

Branch Sync Status

• Status: ℹ️ Ahead of origin/main
• Details: Branch is 3 commits ahead of origin/main (0 behind). branch_sync_status.txt: 0 3. This is expected for a feature branch.

Merge Conflict Analysis

• Conflicts detected: ✅ No conflicts
• Details: Test merge with origin/main completed with "Already up to date." No conflicting files. The PR branch can be merged cleanly into origin/main.

---

3. Strengths

• Correct targeting of issue Closing network resources in tests, adding close functionality #92 — Addresses both “cleanup in tests” and “add close methods to host and network” in line with the issue and maintainer guidance.
• Structured identify cancellation — Using trio.CancelScope per identify task and cancelling them in close() is the right pattern for trio; it avoids leaving system tasks pending and ensures deterministic teardown.
• Race-safe scope cleanup — In _identify_task_entry, the scope is removed from _identify_scopes only when self._identify_scopes.get(peer_id) is cancel_scope, which avoids races if the same peer is re-scheduled while the previous task is still finishing.
• Centralized test cleanup — Adding await host.close() in HostFactory.create_batch_and_listen’s finally fixes cleanup for all tests that use this factory without touching each test individually.
• UPnP cleanup — Closing the host now removes UPnP port mappings and logs failures instead of failing the whole close, which is appropriate for best-effort cleanup.
• Docstring — BasicHost.close() has a short, clear docstring.

---

4. Issues Found

Critical

• Newsfragment missing (BLOCKER)
  • File: newsfragments/
  • Issue: There is no newsfragment for issue Closing network resources in tests, adding close functionality #92. The PR references issue Closing network resources in tests, adding close functionality #92 and the To-Do lists “Add entry to the release notes” as done, but no file matching 92.<type>.rst exists under newsfragments/.
  • Suggestion: Add a newsfragment, e.g. newsfragments/92.feature.rst, with a short user-facing sentence (e.g. “BasicHost and tests now perform proper shutdown and resource cleanup, resolving pending task warnings.”) and ensure the file ends with a newline. This is required for approval per project policy.

Major

• PR does not reference the issue in a way that auto-closes it

  • Issue: The PR body says “Issue Closing network resources in tests, adding close functionality #92” but does not use “Fixes Closing network resources in tests, adding close functionality #92” or “Closes Closing network resources in tests, adding close functionality #92”.
  • Suggestion: Add “Fixes Closing network resources in tests, adding close functionality #92” (or “Closes Closing network resources in tests, adding close functionality #92”) in the PR description so the issue is linked and closed on merge.

• Inconsistent trailing whitespace in basic_host.py

  • Lines: Around 851–855 (after self.mDNS.stop() and self.bootstrap.stop()).
  • Issue: Blank lines contain trailing spaces (visible in diff: ).
  • Suggestion: Remove trailing spaces so the file matches project style (e.g. ruff/black). This may be reported by lint in CI.

Minor

• close() could document order of operations

  • File: libp2p/host/basic_host.py
  • Lines: 845–871
  • Issue: The docstring is minimal; the order (services → UPnP → identify → network) is important for correct teardown.
  • Suggestion: Optionally add a one-line note in the docstring that cleanup order is intentional (e.g. stop services first, then cancel identify, then close network).

• Double merge commits on branch

  • Issue: The branch history includes two merge commits from main (817f6d3, 6f8fb31). Not a code issue but may warrant a squash or rebase for a cleaner history before merge, depending on project preferences.

---

5. Security Review

• Unvalidated input: No new external input; cleanup uses existing host state.
• Subprocess / temp files / OS: Only existing UPnP and network teardown; no new usage.
• Keys / crypto: Not modified.
• Sensitive data in logs: Only existing debug/warning patterns; the new “Error removing UPnP mappings during close” logs an exception message, which is consistent with current practice.
• Conclusion: No new security concerns identified. Security impact: None.

---

6. Documentation and Examples

• Docstrings: BasicHost.close() has a brief docstring; consider adding a note on cleanup order (see Minor above).
• API/release notes: Missing newsfragment (see Critical). No README or tutorial changes required for this internal/behavioral improvement.
• Examples: No new public API surface; no new example needed.

---

7. Newsfragment Requirement

Status: ❌ BLOCKER — missing newsfragment and optional issue link

• Severity: CRITICAL / BLOCKER
• Issue:
  1. No newsfragment file for issue Closing network resources in tests, adding close functionality #92 under newsfragments/.
  2. PR does not use “Fixes Closing network resources in tests, adding close functionality #92” / “Closes Closing network resources in tests, adding close functionality #92” in the description (only “Issue Closing network resources in tests, adding close functionality #92”).
• Impact: PR cannot be approved until:
  1. The PR description includes “Fixes Closing network resources in tests, adding close functionality #92” (or “Closes Closing network resources in tests, adding close functionality #92”).
  2. A valid newsfragment is added: newsfragments/92.feature.rst (or appropriate type such as 92.bugfix.rst if the project treats this as a bugfix). Content: short ReST user-facing description; file must end with a newline.
• Suggestion:
  • Add “Fixes Closing network resources in tests, adding close functionality #92” to the PR description.
  • Create newsfragments/92.feature.rst with content like:
    BasicHost and test infrastructure now perform proper shutdown and resource cleanup, eliminating "Task was destroyed but it is pending!" warnings.
  • Ensure the file ends with a newline.
• Action required: Address the above before merge.

---

8. Tests and Validation

Intent: The PR adds explicit host cleanup in one test and in HostFactory.create_batch_and_listen, which should resolve pending-task warnings across tests that use the factory.

Local verification (with project venv activated):

• Lint (make lint): Failed with exit code 127 — pre-commit: command not found in the environment used. No lint output could be evaluated; recommend CI or an environment with pre-commit installed.
• Typecheck (make typecheck): Failed with exit code 127 — same pre-commit dependency. Not run.
• Tests (make test): ✅ Passed. Run with source venv/bin/activate && make test. Result: 2164 passed, 16 skipped in ~91s. Full output in downloads/AI-PR-REVIEWS/1208/test_output.log.
• Docs (make linux-docs): ✅ Passed. Run with source venv/bin/activate && make linux-docs. Sphinx HTML build succeeded; doctests: 109 tests, 0 failures. Full output in downloads/AI-PR-REVIEWS/1208/docs_output.log.

Conclusion: With the project virtual environment activated, the test suite and documentation build complete successfully. Lint and typecheck were not run successfully (pre-commit missing in this environment). The code changes are validated by the passing test run.

---

9. Recommendations for Improvement

1. Add newsfragment and “Fixes Closing network resources in tests, adding close functionality #92” (mandatory).
2. Remove trailing whitespace in basic_host.py in the new close() block.
3. Optionally document in the docstring that cleanup order (services → UPnP → identify → network) is intentional.
4. Consider squashing or rebasing the two merge-from-main commits if the project prefers a linear history.

---

10. Questions for the Author

1. Was the intention to leave Swarm unchanged in this PR (i.e. “Ensured close() satisfies INetworkService” means it was already correct), or did you plan a separate follow-up for Swarm?
2. For UPnP, was it deliberate to only remove mappings when self.upnp.get_external_ip() is truthy (i.e. only if we previously discovered an external IP), and to iterate get_addrs() (which includes p2p suffix) and use value_for_protocol("tcp")? Just to confirm this matches how mappings were added.
3. Do you want idempotent close() (e.g. set self.mDNS = None after stop, or guard with a “closed” flag) so that multiple close() calls are safe, or is it acceptable to document that close() should be called at most once?

---

11. Overall Assessment

• Quality rating: Good — design and implementation are solid; missing newsfragment and small style/consistency items prevent “Excellent”.
• Security impact: None.
• Merge readiness: Needs fixes — add newsfragment, add “Fixes Closing network resources in tests, adding close functionality #92”, and fix trailing whitespace (and optionally docstring/history).
• Confidence: High in the code and design; medium in full CI results due to environment limitations during review.

---

Review generated using the py-libp2p AI Pull Request Review Prompt. Review context: PR branch checked out, branch sync and merge conflict check performed, issue #92 and PR description/comments read. Tests and docs run with project venv activated (make test: 2164 passed, 16 skipped; make linux-docs: build and doctests passed). Lint and typecheck failed in this environment (pre-commit not found).