Don't set LostConnection on deliberately closed socket by urfeex · Pull Request #593 · UniversalRobots/Universal_Robots_Client_Library

urfeex · 2026-10-12T13:02:08Z

When closing a socket, a pending read might want to set the socket state to LostConnection. This commit only goes to LostConnection if the current state is Connected. This better reflects the socket state machine.

This came up when reviewing #588.

When closing a socket, a pending read might want to set the socket state to LostConnection. This commit only goes to LostConnection if the current state is Connected. This better reflects the socket state machine.

cursor · 2026-10-12T13:02:15Z

Current version of PR was reviewed by /review-bugbot on Oct 12, 12:50. It flagged 4 findings.

Show 4 findings

1. Local benchmark CSV committed

192.168.178.103_cycle_times.csv:1 · High Severity

A new ~15,000-line CSV named after a private LAN address (192.168.178.103) was added at the repository root. This is local RTDE cycle-time capture data, not library source or test fixtures, and should not ship with the project.

2. Machine-specific compile database committed

compile_commands.json:1 · Medium Severity

A compile_commands.json was added at the repo root with absolute paths under /home/feex/git/ur_client_library/build. That file is a per-developer CMake build artifact and will mislead tooling on other machines.

3. Heaptrack profiles committed

heaptrack.rtde_client_example.32011.zst:1 · Medium Severity

Binary heaptrack profile archives for rtde_client_example were added to the repository. These are local profiling captures, not source or test assets.

4. Playbook contradicts package install flag

rt_installation/playbooks/rt_kernel_deb_pkg.yml:5 · Medium Severity

The playbook header states it runs without sudo because rt_kernel_install_packages is false, but the role is invoked with rt_kernel_install_packages: true, which enables apt/dnf install tasks that require become: true.

^{Bugbot on commit 2540f00 is skipped.}

codecov · 2026-10-12T13:03:04Z

Codecov Report

❌ Patch coverage is 86.04651% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.90%. Comparing base (f6a65a8) to head (c44c88d).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/comm/tcp_socket.cpp	89.47%	0 Missing and 4 partials ⚠️
include/ur_client_library/comm/tcp_socket.h	60.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
+ Coverage   83.27%   83.90%   +0.62%     
==========================================
  Files         126      126              
  Lines        8168     8250      +82     
  Branches     3757     3804      +47     
==========================================
+ Hits         6802     6922     +120     
+ Misses        955      915      -40     
- Partials      411      413       +2

Flag	Coverage Δ
python_scripts	`75.90% <ø> (ø)`
start_ursim	`91.89% <ø> (-1.14%)`	⬇️
unit	`61.67% <86.04%> (+2.34%)`	⬆️
ur5-3.14.3	`80.68% <81.39%> (+0.66%)`	⬆️
ur5e-10.11.0	`76.38% <81.39%> (+0.62%)`	⬆️
ur5e-10.12.0	`77.37% <81.39%> (+0.57%)`	⬆️
ur5e-10.7.0	`75.91% <81.39%> (+0.71%)`	⬆️
ur5e-5.9.4	`80.80% <81.39%> (+0.54%)`	⬆️
ur7e-10.13.0	`77.96% <81.39%> (+0.61%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Copilot

🟡 Changes recommended

A stale read can still mark a newly reconnected socket as LostConnection.

1 open finding

Stale read can mark a reconnected connection as lost · New

What changed in this PR

Prevents deliberate socket closure from being reported as a lost connection.

Changes:

Uses compare-and-exchange before setting LostConnection.
Adds a concurrent disconnect/read regression test.

File	Description
`src/comm/tcp_socket.cpp`	Conditionally transitions connected sockets to `LostConnection`.
`tests/test_tcp_socket.cpp`	Tests disconnecting during a pending read.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🟡 Changes recommended

Connection tracking is uninitialized and does not uniquely cover reconnects or rapid connection cycles.

3 open findings

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

The separate generation check and state CAS allow a stale read to mark a newly established connection as lost.

1 open finding

Race between generation check and connection state CAS · New

3 resolved since last review

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Concurrency gaps can still overwrite deliberate closure state or associate a stale generation with a newly connected descriptor.

2 open findings

1 resolved since last review

Race between generation check and connection state CAS

🧠 Review effort: Balanced

  setTargetStateUnlessStopRequested(SocketState::Connected);
-  if (!setupInternal(host, port, max_num_tries, reconnection_time))
+  StateGeneration connection_attempt;
+  if (!setupInternal(host, port, max_num_tries, reconnection_time, connection_attempt))


+  EXPECT_NO_THROW(waitFor([&read_started]() { return read_started.load(); }, std::chrono::seconds(1)));
+  std::this_thread::sleep_for(std::chrono::milliseconds(50));


Copilot

🟡 Changes recommended

Reads can still consume data from a replacement connection, and the regression test does not deterministically exercise a pending read.

3 open findings

🧠 Review effort: Balanced

+    // Only the exact connection on which recv() started may be marked lost. A concurrent close()
+    // changes the state, and a reconnect changes the generation, so neither can be overwritten by
+    // this read's stale result.
+    StateGeneration expected = read_state;
+    state_generation_.compare_exchange_strong(expected, makeStateGeneration(SocketState::LostConnection, read_state));


Don't set LostConnection on deliberately closed socket

2540f00

When closing a socket, a pending read might want to set the socket state to LostConnection. This commit only goes to LostConnection if the current state is Connected. This better reflects the socket state machine.

urfeex requested a review from a team October 12, 2026 13:02

urfeex added the enhancement New feature or request label Oct 12, 2026

Copilot AI balanced review requested due to automatic review settings October 12, 2026 13:02

Copilot started reviewing on behalf of urfeex October 12, 2026 13:02

View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Comment thread src/comm/tcp_socket.cpp Outdated

urfeex added this to the Next release milestone Oct 13, 2026

urfeex marked this pull request as draft October 13, 2026 17:49

urfeex requested a balanced review from Copilot October 14, 2026 12:26

Copilot started reviewing on behalf of urfeex October 14, 2026 12:26

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Comment thread include/ur_client_library/comm/tcp_socket.h Outdated

Comment thread src/comm/tcp_socket.cpp Outdated

urfeex requested a balanced review from Copilot October 14, 2026 12:38

Copilot started reviewing on behalf of urfeex October 14, 2026 12:38

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Comment thread src/comm/tcp_socket.cpp Outdated

track states with connection attempts

afa517d

urfeex force-pushed the tcp_socket_disconnect_vs_lost branch from 5131f1f to afa517d Compare October 14, 2026 12:55

urfeex requested a balanced review from Copilot October 14, 2026 12:55

Copilot started reviewing on behalf of urfeex October 14, 2026 12:56

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Check result of setTargetStateUnlessStopRequested

c44c88d

urfeex requested a balanced review from Copilot October 14, 2026 13:08

Copilot started reviewing on behalf of urfeex October 14, 2026 13:09

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Navigation Menu

Repository navigation

Don't set LostConnection on deliberately closed socket - #593

Don't set LostConnection on deliberately closed socket#593
urfeex wants to merge 3 commits into
UniversalRobots:masterfrom
urfeex:tcp_socket_disconnect_vs_lost

urfeex commented Oct 12, 2026

Uh oh!

cursor Bot commented Oct 12, 2026

1. Local benchmark CSV committed

2. Machine-specific compile database committed

3. Heaptrack profiles committed

4. Playbook contradicts package install flag

Uh oh!

codecov Bot commented Oct 12, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		EXPECT_NO_THROW(waitFor([&read_started]() { return read_started.load(); }, std::chrono::seconds(1)));
		std::this_thread::sleep_for(std::chrono::milliseconds(50));

Conversation

urfeex commented Oct 12, 2026

Uh oh!

cursor Bot commented Oct 12, 2026

1. Local benchmark CSV committed

2. Machine-specific compile database committed

3. Heaptrack profiles committed

4. Playbook contradicts package install flag

Uh oh!

codecov Bot commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Oct 12, 2026 •

edited

Loading