Skip to content

Conversation

@pappz
Copy link
Contributor

@pappz pappz commented Oct 6, 2025

The status cmd will not be blocked by the ICE probe

Refactor the TURN and STUN probe, and cache the results. The NetBird status command will indicate a "checking…" state.

Relays: 
  [stun:192.168.0.10:3478] is Checking...
  [turn:192.168.0.10:3478?transport=udp] is Checking...
  [rel://192.168.6.1] is Available

Describe your changes

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/__

Refactor turn, stun probe and cache the results
Copilot AI review requested due to automatic review settings October 6, 2025 18:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Refactors STUN/TURN probing to be non-blocking and cache results so the client status command can show a "Checking..." state instead of being blocked by ICE probing.

  • Introduces StunTurnProbe with result caching and in-progress signaling.
  • Integrates the new probe into Engine and updates status rendering to show "Checking..." when probes are underway.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
client/status/status.go Adds handling to display "Checking..." when relay probe is in progress.
client/internal/relay/relay.go Adds StunTurnProbe with caching, in-progress sentinel, and refactors probe logic into methods.
client/internal/engine.go Instantiates and uses StunTurnProbe in health probes flow.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +343 to +345
if relay.Error == probeRelay.ErrCheckInProgress.Error() {
available = "Checking..."
} else {
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

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

[nitpick] This relies on string matching an error message to detect the in-progress state, which is brittle. Prefer a structured field (e.g., a status enum or a boolean InProgress) or a stable error code to signal 'checking' across layers.

Copilot uses AI. Check for mistakes.
case <-probeDone:
// when the probe is return fast, return the results right away
return p.getCachedResults(cacheKey, stuns, turns)
case <-time.After(1300 * time.Millisecond):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can leak if any of the other cases are selected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

until 1300 milliseconds

@pappz pappz requested a review from lixmal October 28, 2025 09:47
lixmal
lixmal previously approved these changes Oct 28, 2025
@sonarqubecloud
Copy link

@mlsmaycon mlsmaycon merged commit d7321c1 into main Oct 28, 2025
49 of 51 checks passed
@mlsmaycon mlsmaycon deleted the refactor/do-not-block-probe branch October 28, 2025 15:11
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.

4 participants