Skip to content

Conversation

pappz
Copy link
Contributor

@pappz pappz commented Sep 25, 2025

Describe your changes

When the client sends a signaling offer or answer, the Signal server may return an error if the message cannot be delivered. The client then checks the error type to decide how to proceed:

  • If the error suggests the remote peer is unavailable, the client waits for the peer instead of retrying immediately.
  • If the method is not implemented, then switch back to the original retry mechanism (backward compatibility case)
  • If the error indicates another error, the client retries by sending a new offer.

Whenever a peer reconnects to the signaling server and no active peer connection exists, it must send a new offer. This ensures that any signaling messages lost while the peer was offline are recovered and the connection can be re-established.

The NB_DISABLE_SEND_WITH_DELIVERY_CHECK: true|false option can be used to enforce that clients always use the server’s original methods.

For the review, keep in mind that these Signal server changes must support both old and new clients, and old clients must still connect properly with each other.

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

@Copilot Copilot AI review requested due to automatic review settings September 25, 2025 10:22
Copy link
Contributor

@Copilot 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

This PR refactors the signaling mechanism by adding a new delivery check method and simplifying the reconnection logic. The changes introduce a synchronous delivery check for signal messages and remove the complex exponential backoff mechanism in favor of a simpler timer-based approach.

Key changes:

  • Adds a new SendWithDeliveryCheck RPC method that returns an error when the remote peer is not connected
  • Replaces complex exponential backoff reconnection logic with a fixed timer-based approach
  • Updates client code to use the new delivery check method for offer/answer signaling

Reviewed Changes

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

Show a summary per file
File Description
shared/signal/proto/signalexchange.proto Adds new SendWithDeliveryCheck RPC method definition
signal/server/signal.go Implements SendWithDeliveryCheck method with delivery validation
shared/signal/proto/signalexchange_grpc.pb.go Generated gRPC client/server code for new method
shared/signal/proto/signalexchange.pb.go Generated protobuf code with empty response type
shared/signal/client/grpc.go Implements client-side SendWithDeliveryCheck method
shared/signal/client/client.go Adds SendWithDeliveryCheck to Client interface
client/internal/peer/signaler.go Updates signaling to use new delivery check method
client/internal/peer/handshaker.go Minor logging reorder and code cleanup
client/internal/peer/guard/guard.go Simplifies reconnection logic with fixed timer approach
client/internal/peer/conn.go Adds initial offer sending on connection open

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

@pappz pappz marked this pull request as ready for review September 29, 2025 19:40
@pappz pappz requested a review from hakansa October 7, 2025 13:49
hakansa
hakansa previously approved these changes Oct 7, 2025
@pappz pappz changed the title [client, server] Refactor/reducate signaling [client, server] reducate signaling Oct 8, 2025
@pappz pappz changed the title [client, server] reducate signaling [client, server] Refactor/reduce signaling Oct 8, 2025
@mlsmaycon mlsmaycon changed the title [client, server] Refactor/reduce signaling [client, signal] Refactor/reduce signaling Oct 8, 2025
Copy link

sonarqubecloud bot commented Oct 9, 2025

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.

3 participants