Skip to content

Add sanity check to Sync packet handeling: to not remove connections the client didn't know about at time of packet construction#150

Open
nazzleflack wants to merge 2 commits intorancher:mainfrom
nazzleflack:main
Open

Add sanity check to Sync packet handeling: to not remove connections the client didn't know about at time of packet construction#150
nazzleflack wants to merge 2 commits intorancher:mainfrom
nazzleflack:main

Conversation

@nazzleflack
Copy link

@nazzleflack nazzleflack commented Mar 5, 2026

Issue: none

Problem

In case of multiple concurrent requests being processed remotedialer sometimes returns a sync from client error to an api client.

How to reproduce:

  1. clone repo
  2. edit types.go and reduce SyncConnectionsInterval and SyncConnectionsTimeout to time.Second / 60 or similar
  3. edit remotedialer/dummyserver to not return any http body (commenting out line 24)
  4. execute dummy server and client examples
  5. send obscene amounts of http requests through remotedialer (parallelism is important here)
set c 1000; while true;
  set res "$(for i in (seq 1 $c);
    curl -0 "http://localhost:8123/client/foo/http/localhost:8125" -s -o - 2>/dev/zero& end; wait)";
    echo "'$res' $(echo -n "$res" | wc -c)";
  if [ -z "$res" ]; echo "no errors"; else date; break; end;
  sleep 2
end

sometimes this works just fine, while other times a lot of Get "http://localhost:8125": sync from client messages get returned by remotedialer.

This is not the case if one sets SyncConnections{Interval,Timeout} to very large numbers (= removing sync all together)- doing so fixes the error without leaving stale connections.

Part of the problem (the part i address in this PR) was the implementation of the sync process.
Is was implemented in a way in which the client sends the server a list of all connection IDs the client still has open and the server closes the ones it has the client doesn't. This enabled the possibility of the client not yet knowing about a new connection, sending a Sync packet and the server then removing this connection before it had a chance of reaching the client.

Solution

To resolve this specific case, I've added a additional field to the Sync Packet. It sends the highest (and therefore latest) connection the client has received at time of crafting the packet. The server then only closes connections that are closed on the client AND older than the clients knowledge at time of crafting the packet.

This DOES NOT completely eliminate the issue at hand though it greatly decreased the chance of it occurring.

I am convinced the rest of the issue is caused by the server sending connections out of order, i.e. connectionIDs not being linear when received or processed by the client, though I have not been able to come up with anything.

CheckList

  • Test

@nazzleflack nazzleflack requested a review from a team as a code owner March 5, 2026 11:28
@nazzleflack nazzleflack changed the title Add sanity check to sync packets to not remove connections the client has never seen Add sanity check to Sync packet handeling: to not remove connections the client didn't know about at time of packet construction Mar 5, 2026
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

edit types.go and reduce SyncConnectionsInterval and SyncConnectionsTimeout to time.Second / 60 or similar
edit remotedialer/dummyserver to not return any http body (commenting out line 24)
execute dummy server and client examples

Can you perhaps add a test that does this? You may need to change some consts and functions to vars that can be modified by the test harness, but if this is reproducible - we should add a test to reproduce it.

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