Skip to content

[client] Fix shutdown blocking on stuck ICE agent close#4780

Merged
lixmal merged 1 commit intomainfrom
timeout-ice-agent-close
Nov 13, 2025
Merged

[client] Fix shutdown blocking on stuck ICE agent close#4780
lixmal merged 1 commit intomainfrom
timeout-ice-agent-close

Conversation

@lixmal
Copy link
Copy Markdown
Collaborator

@lixmal lixmal commented Nov 13, 2025

Describe your changes

In some cases, the ICE agent will block on Close() indefinitely, blocking the shutdown or restart process.
To avoid that, we add a 3-second timeout after which we abandon waiting for the close operation and proceed with shutdown.

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced connection termination robustness with timeout protection for ICE agent shutdown. Prevents the system from hanging indefinitely during network connection cleanup. If shutdown operations exceed timeout limits, the system logs a warning and continues with resource cleanup to ensure proper termination.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 13, 2025

Walkthrough

The change introduces a timeout mechanism to the ThreadSafeAgent.Close() method in the ICE agent module. A new constant defines the maximum wait duration for Close operations. The Close method now executes the underlying agent's Close concurrently with timeout protection, logging a warning if the timeout elapses before completion.

Changes

Cohort / File(s) Summary
ICE Agent Timeout Protection
client/internal/peer/ice/agent.go
Added iceAgentCloseTimeout constant and modified ThreadSafeAgent.Close() to execute agent closure with timeout, logging a warning and proceeding with cleanup if timeout elapses.

Sequence Diagram

sequenceDiagram
    actor Caller
    participant TSA as ThreadSafeAgent
    participant Agent as ICE Agent
    participant Timer

    Caller->>TSA: Close()
    TSA->>Agent: Start Close() (async)
    TSA->>Timer: Start timeout
    par
        Agent-->>TSA: Close() result (or hangs)
    and
        Timer-->>TSA: Timeout elapsed
    end
    alt Close completes first
        TSA-->>Caller: Return Close() error
    else Timeout occurs first
        rect rgba(255, 193, 7, 0.2)
            TSA->>TSA: Log warning
        end
        TSA-->>Caller: Return nil (proceed with cleanup)
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Focus on verifying that the timeout duration is appropriate for ICE agent closure operations.
  • Confirm the concurrent execution pattern correctly handles both success and timeout paths.

Poem

🐰 A timeout guard now shields the close,
No more the agent's endless froze,
With gentle warnings when it stalls,
The cleanup dance in timeout's halls! 🕐

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding a timeout to prevent ICE agent close from blocking the shutdown process.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The pull request description addresses the core issue and marks it as a bug fix, but lacks an issue ticket reference and doesn't explain why documentation is not needed.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch timeout-ice-agent-close

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link
Copy Markdown

@lixmal lixmal merged commit 2795703 into main Nov 13, 2025
37 of 39 checks passed
@lixmal lixmal deleted the timeout-ice-agent-close branch November 13, 2025 12:24
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