Let PrimaryClient take a port and read package length safely

[urrsk]

Fake-server tests had to bind the real primary port because PrimaryClient always connected to UR_PRIMARY_PORT. Another listener made TCPServer retry the bind forever, hanging the tests. PrimaryClient now takes an optional port, and the fake-server fixtures use dedicated test ports.

getPackageLength() also copies the length bytes before the endian swap, so an unaligned position in the byte stream is no longer read through a cast.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.43%. Comparing base (00893ff) to head (d6e3b54).
⚠️ Report is 2 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
- Coverage   88.63%   88.43%   -0.21%     
==========================================
  Files           2        2              
  Lines         440      441       +1     
==========================================
  Hits          390      390              
- Misses         50       51       +1     

Flag	Coverage Δ
python_scripts	75.90% <ø> (ø)
start_ursim	91.34% <ø> (-0.26%)	⬇️

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]

Copilot review overview

🟡 Changes recommended

Fixed ports retain the test-hang risk, and the constructor change breaks shared-library ABI compatibility.

Review effort: Balanced
Findings: 1 · 2

Open (3)

• Preserve ABI with a separate three-argument constructor · New
• Use an ephemeral port to prevent fixture bind hangs · New
• Use an ephemeral port while preserving reconnect same-port behavior · New

What changed in this PR

Adds configurable PrimaryClient ports for isolated fake-server tests and safely reads potentially unaligned package lengths.

Changes:

• Adds an optional PrimaryClient port.
• Moves fake-server tests to dedicated ports.
• Replaces an unaligned cast with memcpy.

File	Description
primary_client.h	Exposes configurable port API.
primary_client.cpp	Connects using the supplied port.
package_header.h	Safely reads package lengths.
test_primary_client.cpp	Uses a fake-server test port.
test_primary_client_reconnect.cpp	Uses a test port for reconnect tests.

---

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

[Copilot]

Copilot review overview

🟡 Changes recommended

The unaligned package-length behavior needs a focused regression test.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Preserve ABI with a separate three-argument constructor
• Add regression test for unaligned packet length reads · New

Resolved since last review (2)

• Use an ephemeral port while preserving reconnect same-port behavior
• Use an ephemeral port to prevent fixture bind hangs

[Copilot]

Copilot review overview

🔵 Needs a closer look

The replacement reconnect server can still retry indefinitely when its released port cannot be rebound.

Review effort: Balanced
Findings: None

Resolved since last review (2)

• Preserve ABI with a separate three-argument constructor
• Add regression test for unaligned packet length reads

Previously missed (1)

In code that hasn't changed since last review

Prevent replacement server from hanging on fixed-port rebind

tests/​test_primary_client_reconnect.cpp:182

The replacement server still binds a previously released fixed port with TCPServer's default unlimited retries. This port remains unreserved for the entire reconnect/stop interval, so another concurrent test or process can claim it; on platforms where immediate rebinding is delayed, the same bind can also fail temporarily. In either case this constructor can hang forever, preserving the failure mode the PR is intended to remove. Give FakePrimaryServer a finite bind-attempt option and use it here (or otherwise keep/reserve the port) so a rebind failure terminates the test instead of blocking indefinitely.

[urrsk]

Addressed the previously missed rebind hang in 4590227.

The replacement server in PrimaryClientReconnectTest.stop_not_blocked_by_stuck_reconnect_thread was binding a port that had been free for the whole stop interval, and TCPServer retries that bind forever. FakePrimaryServer now takes a bind retry limit, and that replacement server uses 5 attempts, 100 ms apart, so a collision fails the test instead of hanging.

#582 (review)

[Copilot]

Copilot review overview

🟡 Changes recommended

The same-port restart test remains unreliable on Windows because the port can outlive the brief retry period in TCP TIME_WAIT.

Review effort: Balanced
Findings: 1

Open (1)

• Windows test flakes when restarting server on a port in TIME_WAIT · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

The constructor change removes the existing shared-library symbol, breaking binary compatibility.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Windows test flakes when restarting server on a port in TIME_WAIT

Previously missed (1)

In code that hasn't changed since last review

Use the supplied notifier or document it as unused

include/​ur_client_library/​primary/​primary_client.h:73

This new parameter documentation promises that the supplied notifier receives lifecycle events, but the constructor marks the parameter unused and passes the separate default-constructed notifier_ member to the pipeline. Either initialize/use the supplied notifier or document that this parameter is retained as an unused compatibility argument.

This issue also appears on line 76 of the same file.

[mergify]

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.