Split TCP RR infrastructure from the workload by usamasaqib · Pull Request #1905 · DataDog/lading

usamasaqib · 2026-06-01T14:36:31Z

What does this PR do?

This PR refactors the tcp_rr workload to split the infrastructure elements into a separate file. This will allow subsequent workloads to reuse the same thread and flow management code.

Motivation

Related issues

Additional Notes

PR stack
[1] #1905 <-- This
[2] #1906

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1785a95bb9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-06-01T14:41:07Z

+
+/// Control-channel handshake: server writes `flows` to the accepted control
+/// connection as a 2-byte big-endian `u16` and closes; client reads the same
+/// 2 bytes after connecting. Internal protocol — no magic / version byte.


Use ASCII-only documentation

This newly added doc comment contains a non-ASCII em dash, and /workspace/lading/AGENTS.md requires US-ASCII only in code and documentation. There are multiple new occurrences in this file, so replace these punctuation characters with ASCII equivalents before landing.

Useful? React with 👍 / 👎.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f74a091536

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-06-02T09:08:09Z

+                // against a generator that connects but never reads.
+                conn.set_write_timeout(Some(HANDSHAKE_TIMEOUT))
+                    .expect("set_write_timeout on accepted TcpStream must succeed");
+                conn.write_all(&flows_bytes)?;


Clean up workers when control handshake fails

If the accepted control connection closes or resets before the 2-byte handshake is written (for example, the generator is killed right after connecting, or a probe hits the control port), this ? returns from run_server before shutdown_flag is set and before the worker/metrics handles are joined. Dropping those JoinHandles detaches the data listener threads, leaving sockets and metrics work running even though the blackhole task reported an error, which can make subsequent runs in the same process collide with the still-bound data port.

Useful? React with 👍 / 👎.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 86bb9725e0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-06-02T10:12:10Z

+                registry
+                    .reregister(&mut flow.stream, flow.token, interest)
+                    .expect("reregister of a live, owned flow must succeed");


Handle poll registry errors without panicking

When mio returns an io::Error from reregister (and the same new expect pattern is used for deregister just below), this worker thread panics instead of converting the flow to an error/removal path. In a live run, a registry error would stop that thread's event loop and only surface as ThreadPanicked after shutdown, which can corrupt the experiment's results; it also violates the repository rule in AGENTS.md that recoverable errors must not panic.

Useful? React with 👍 / 👎.

We do not consider this a recoverable error. A live flow must always allow reregistration.

Could we bubble up the error instead? I see that this function is called from the tcp_rr (blackhole+generator), we could make aply_action return a result and if it's an error, we log and exit lading gracefully.

preinlein

Is there any way to include some tests for things in here?

It's hard to review with confidence given that I'm not familiar enough with the inner workings of neper/etc.

preinlein · 2026-06-16T13:39:02Z

+                registry
+                    .reregister(&mut flow.stream, flow.token, interest)
+                    .expect("reregister of a live, owned flow must succeed");


Could we bubble up the error instead? I see that this function is called from the tcp_rr (blackhole+generator), we could make aply_action return a result and if it's an error, we log and exit lading gracefully.

preinlein · 2026-06-16T13:45:21Z

+        match net::TcpStream::connect(params.control_addr) {
+            Ok(mut conn) => {
+                conn.set_read_timeout(Some(HANDSHAKE_TIMEOUT))
+                    .expect("set_read_timeout on connected TcpStream must succeed");


rather than expect here, could we bubble up the error?

preinlein · 2026-06-16T13:49:13Z

+    Ok(())
+}
+
+fn client_thread_main(


can we make this function bubble up errors as well

preinlein · 2026-06-16T13:54:31Z

+        }
+    }
+
+    // All data listeners are up. Open control port so the generator can


I wonder if we can refactor this method so that we don't need the clippy/lint exception for "method too long" here. Can we refactor up to this point into a function called prepare_data_listeners or something akin to that?

Done in 64a17d8

preinlein · 2026-06-16T13:55:03Z

+    let control_listener = ctrl_res?;
+    control_listener
+        .set_nonblocking(true)
+        .expect("failed to set control listener nonblocking");


Let's do a pass thru the PR and remove all these expects and instead change it to bubble up errors.

The expects are there because they are meant to be invariants. If these operations do not succeed then the workload will not behave as designed. So this operation must always suceed.

And why do we need a panic / how does graceful error handling not work here?

We can just have lading exit gracefully with an error, no?

We can. I'll make the changes.

preinlein · 2026-06-16T13:56:41Z

+    }
+    drop(control_listener);
+
+    if generator_connected {


It feels like everything above this could be another function called wait_for_generator or something along those lines.

Done in 64a17d8

preinlein · 2026-06-16T14:12:54Z

+fn set_nodelay_mio(stream: &TcpStream, no_delay: bool) {
+    let sock = socket2::SockRef::from(stream);
+    if let Err(e) = sock.set_tcp_nodelay(no_delay) {
+        trace!("failed to set TCP_NODELAY: {e}");


is this problematic? Are we ok with a silent error here?

Its been changed to a warn!. It is okay for it to be just a log for now since this feature is not available on all kernel versions, and I am not sure how to best handle it yet. So just logging on continuing seems fine for now.

It can totally just be a log and not an error that gets bubbled up. If we do that, let's just add a comment saying why it's ok to have a silent error and if there any consequences for this not working as expected (or being available like you mentioned)

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 64a17d8f86

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-08-28T11:00:22Z

+                conn.set_read_timeout(Some(HANDSHAKE_TIMEOUT))
+                    .expect("set_read_timeout on connected TcpStream must succeed");
+                let mut buf = [0u8; HANDSHAKE_LEN];
+                conn.read_exact(&mut buf)?;


Retry incomplete control handshakes

When the TCP connection succeeds but the control peer closes, resets, or times out before sending both bytes—for example, when the target proxy accepts before its blackhole connection is ready—this ? exits run_client immediately instead of using the existing five-minute retry window. Treat handshake read failures like connection failures and retry until the deadline or shutdown so transient startup races do not abort the experiment.

Useful? React with 👍 / 👎.

usamasaqib · 2026-09-09T11:52:12Z

All the comments about bubbling up the errors are addressed in
#1928

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3db8a76a45

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-09-09T12:19:53Z

+    let mut conn = net::TcpStream::connect(control_addr)?;
+    conn.set_read_timeout(Some(HANDSHAKE_TIMEOUT))?;
+    let mut buf = [0u8; HANDSHAKE_LEN];
+    conn.read_exact(&mut buf)?;


Yield the runtime during handshake retries

When a control peer accepts the connection but delays or never sends the two-byte response, this synchronous connect/read_exact blocks a Tokio worker for five seconds, after which the caller uses std::thread::sleep and immediately retries without reaching an .await. With at least as many affected tcp_rr generators as runtime workers, the blackhole, experiment timer, and shutdown tasks cannot run until the five-minute deadline. Use Tokio networking and timers, or move each blocking attempt to spawn_blocking, so startup retries yield the runtime.

Useful? React with 👍 / 👎.

We do not need to worry about this case since both the generator and blackhole are written to be well behaved.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5cdeda358a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-09-09T12:29:20Z

+                conn.write_all(&flows_bytes)?;
+                info!("generator connected from {peer}, sent flows={flows}, data threads running");
+                return Ok(true);


Keep accepting until the client acknowledges the handshake

Although the client now retries incomplete reads, the final server code treats a successful local write_all as a completed handshake and returns, after which run_server drops the control listener. If the target proxy resets or loses the response after those two bytes enter the server's socket buffer but before the generator reads them, every retry is refused and the generator waits until the five-minute deadline. Add a client acknowledgement or otherwise keep accepting handshake attempts until completion is confirmed; this server-side listener closure is fresh evidence that the previously requested client retry cannot recover from this case.

Useful? React with 👍 / 👎.

usamasaqib added 3 commits June 1, 2026 15:24

refactor rr

62943cb

do not swallow poll registry errors

1046f0d

coordinate number of flows between generator and blackhole

1785a95

usamasaqib requested a review from a team as a code owner June 1, 2026 14:36

This comment has been minimized.

Sign in to view

chatgpt-codex-connector Bot reviewed Jun 1, 2026

View reviewed changes

usamasaqib added the no-changelog label Jun 2, 2026

usamasaqib added 2 commits June 2, 2026 11:04

remove non ascii chars from comments

db754ba

fmt

f74a091

chatgpt-codex-connector Bot reviewed Jun 2, 2026

View reviewed changes

ensure shutdown flag is set on error paths

86bb972

chatgpt-codex-connector Bot reviewed Jun 2, 2026

View reviewed changes

usamasaqib mentioned this pull request Jun 2, 2026

TCP CRR workload #1906

Open

preinlein reviewed Jun 16, 2026

View reviewed changes

decompose run_server

64a17d8

chatgpt-codex-connector Bot reviewed Aug 28, 2026

View reviewed changes

usamasaqib added 2 commits September 2, 2026 14:10

Merge branch 'main' into usama.saqib/rr-refactor

cd76b4e

update h2 due to security advisory

3def8b6

chatgpt-codex-connector Bot reviewed Sep 9, 2026

View reviewed changes

ecapsulate control handshake

5cdeda3

usamasaqib force-pushed the usama.saqib/rr-refactor branch from 3db8a76 to 5cdeda3 Compare September 9, 2026 12:23

chatgpt-codex-connector Bot reviewed Sep 9, 2026

View reviewed changes

Uh oh!

Conversation

usamasaqib commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Related issues

Additional Notes

Uh oh!

This comment has been minimized.

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

preinlein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

usamasaqib commented Sep 9, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

usamasaqib commented Jun 1, 2026 •

edited

Loading