[Phlabry]

Part of #245. Does not close it: this is the relay side only, the live clustering itself is still open.

Why

The relay held one TCP connection, so a flight could feed iMotions or the Python logger, never both. #245 names this as the blocker for live performance groups, and it also forced a choice on every real flight: record the biometrics study properly, or collect ML data.

What it does

The TCP thread now keeps an optional second connection, the mirror, and writes every packet to both from one shared list, so the two sinks cannot drift apart. In the GUI: a second address box with Connect mirror / Disconnect mirror, placeholder 127.0.0.1:5001 to match data_logger.py's default port.

The mirror's socket is owned by its own writer thread, reached through a bounded queue with a non-blocking push, so the TCP thread never waits on it. Connecting, writing and failing all happen off that thread. What is not promised is every copy: a full queue drops packets, reported once when the mirror falls behind and once when it catches up. That covers #245's "a slow refit never blocks packet forwarding to iMotions".

docs/adr/0003 records why the mirror hangs off the primary connection rather than the relay holding two equal ones, and what would justify revisiting that.

Refactor

Packet building moved out of the thread into packets_for_fields. Both destinations are written from its output, and the 4, 8 and odd-length mappings became testable without a socket. I verified the extraction is byte-identical to the old inline code for every field count from 0 to 19, against a transcription of the previous implementation.

Testing

21 tests, 13 new, suite runs in about 2 seconds.

• the_python_mirror_receives_the_same_packets_as_imotions asserts both sinks get byte-identical output
• imotions_keeps_streaming_when_the_mirror_cannot_connect points the mirror at a dead port
• imotions_survives_the_python_side_dying_mid_stream kills Python mid-flight
• a_python_process_that_stops_reading_is_dropped_not_waited_on accepts the connection then never reads, and checks the mirror is dropped on the write timeout while iMotions keeps receiving
• a_long_run_reaches_both_sinks_in_order pushes 800 packets to each and checks for drift
• plus reconnect, disconnect-clears-status, and address validation cases

End to end against the real logger: I drove this code into a running data_logger.py and got five correct CSV rows with pilot_id and flight_id populated, while the iMotions stand-in received the same bytes.

Mutation checked the three load-bearing tests by breaking the code deliberately. Each failure was caught by its intended test and nothing else.

Not verifiable here: anything needing live X-Plane or iMotions. The buttons compile and are wired, but nobody has clicked them against real hardware.

Known limitations, all deliberate

• The mirror needs Connect TCP first, because it lives inside that connection. With no iMotions, point Connect TCP straight at the Python listener as before.
• No automatic reconnect. If Python restarts, press Connect mirror again. Note that a reconnect starts a new flight_id unless --flight-id was passed, same trap as Record pilot_id and flight_id in the logged CSV #219.
• A connect attempt blocks the TCP thread for up to 500ms. Packets queue and flush after, nothing is lost, and it only happens on a button press.
• Send Packet reaches both sinks, so it checks both links at once and writes a row of zeros to the logger as well.

[jmacd867]

Thanks for this, it's a clean piece of work. The tests are thorough, and checking that the packets_for_fields extraction is byte-identical to the old code was a good call. I pulled the branch, cargo test passes (21 tests), and I traced the changes through. Comments below, most important first.

Should fix before merge

1. "Never slows iMotions" isn't true as written. ADR 0003 says "The mirror can never slow the recording" and README.md:128 says the mirror "never affects the iMotions stream". But the TCP thread writes each packet to iMotions, then to the mirror, before moving on to the next one (relay/src/state.rs:725-729). So a stalled Python reader holds up the next iMotions write by up to 100ms, and a connect holds the thread for up to 500ms. That's around 10 samples arriving late. If iMotions timestamps packets on arrival (our timestamp field is empty), those samples are out of step with the biometrics. The ADR also contradicts itself a few sentences later, where it says a connect blocks for 500ms.

Two ways to fix it:

• Reword the ADR and README to give the actual limits instead of "never". This is the minimum.
• If we really want "never", give the mirror its own writer thread with a bounded queue, and drop mirror packets when the queue is full.

2. A slow reader may never get dropped. MIRROR_WRITE_TIMEOUT applies to each individual write call, and write_all keeps retrying after a partial write. A Python process that reads slowly, rather than stopping, could keep every call under 100ms. In that case the mirror is never dropped and iMotions runs at Python's pace. a_python_process_that_stops_reading_is_dropped_not_waited_on only covers a reader that never reads, and its < 20s bound (relay/src/update.rs:696) is loose enough to hide a slowdown. Could you add a test with a reader that drains slowly? A time limit on the whole mirror_send would close this.

Worth addressing

3. Document what the Python side has to do. If the clustering process stops reading during a slow refit, the mirror is dropped and doesn't come back on its own. So whatever replaces data_logger.py has to read the socket on its own thread. A line in the ADR's Consequences section or in inference/README.md would save the next person from finding this out mid-flight.

Smaller things

• "Sample" has two meanings. GLOSSARY.md defines a Sample as one complete set of telemetry values, but PAIRED_SAMPLES (state.rs:198) and eight_fields_become_one_packet_per_sample_in_order (state.rs:893) use it for a single iMotions entry. Meanwhile state.rs:163 and send_sample use the glossary meaning. A different word for the iMotions one would keep them apart.
• One name for the second destination. It's currently "mirror", "Python stream" (UI and event log), "Python mirror" (thread logs), "Python side" and "sink". I'd pick one, probably "mirror", and add it to GLOSSARY.md.
• mirror_row builds its text_input inline (relay/src/view.rs:228). CONTRIBUTING.md asks for a separate helper for each element, like tcp_addr_input.
• connect_mirror re-parses the address that validate_tcp_addr has already checked, and copies its error message. One of the two could go.
• Optional: the event-name list now exists in three places (the 4-field branch, the 8-field branch and PAIRED_SAMPLES). Also, about 8 tests in update.rs repeat the same connect-plus-mirror setup and could share a fixture.

Checked and looks good

• Packets queue rather than drop during a mirror connect.
• Disconnecting iMotions also clears the mirror, and stale MirrorConnected messages are discarded.
• packets_for_fields matches the old inline code.
• "Connect TCP" is required before "Connect Python", and "Send Packet" reaches both destinations.

Like you said, none of this has been run against live X-Plane or iMotions.

[Phlabry]

All of it addressed. Thanks, both of the first two were right and the second one was the important one.

1. "Never slows iMotions". You were right that it was false and that the ADR contradicted itself two paragraphs later. I took the second option rather than rewording, because of your point 2: the first real consumer of this mirror is a clustering process doing periodic refits, which is exactly a slow reader, so the pacing case is the expected usage rather than an edge case.

The mirror's socket now belongs to its own writer thread, and the TCP thread reaches it through a bounded queue (256 packets, about 1.5 seconds) with a non-blocking try-send. Connecting happens on that thread too. What is guaranteed now is narrow and true: the TCP thread never waits on the mirror. What is not guaranteed is delivery of every copy, and the ADR says so instead of claiming "never".

New test connecting_the_mirror_does_not_pause_the_imotions_stream points the mirror at a blackhole address and asserts iMotions keeps receiving. It passes in 0.03s, and fails at 513ms if I move the connect back onto the TCP thread, so it holds the line you identified.

2. Slow reader never dropped. Fixed by the same change, and a_slow_python_reader_does_not_pace_imotions covers it: a reader draining slower than the relay writes, with iMotions asserted to receive every packet and the relay asserted to report the problem. Worth knowing what I found while writing it: the socket buffers absorb far more than the queue does, so a slow reader only produces backpressure after a few hundred KB. Below 1000 samples nothing happens at all. The outcome is drops first, then the write timeout closing the mirror.

Also replaced the loose < 20s bound you flagged, and the fixed-tick pump_update waits, with a pump_until helper that waits on a condition. The old fixed waits made failed_connect_surfaces_an_error_and_allows_a_retry flaky once the suite had a heavier test in it, which is my doing. 23 tests, stable over six consecutive runs.

3. What the Python side must do. In the ADR's Consequences and in inference/README.md: keep reading the socket, on its own thread if the reader also does slow work, because stopping loses copies and then the mirror, which does not reconnect by itself.

Smaller things. All taken. PAIRED_SAMPLES is now IMOTIONS_EVENTS with a comment on why it is not a Sample in the glossary sense, and the test name follows. One name throughout: "mirror" in code, "Python mirror" in the UI and logs, and a Mirror entry in GLOSSARY.md. mirror_row now composes mirror_addr_input, mirror_status_element and two button helpers, per CONTRIBUTING. connect_mirror no longer repeats the hint text.

Left alone: the event-name list in three places and the test fixture duplication. Both are fair, but I would rather not grow this diff further. Happy to do either in a follow-up.

Re-verified end to end against a running data_logger.py after the rewrite, three samples in and three correct rows out.

[jmacd867]

#248 has merged, so this needs a rebase on main. It already conflicts in GLOSSARY.md and inference/README.md after #244. I test-merged it with #248, and three things will bite that git won't flag:

1. A test fails. a_malformed_mirror_address_is_refused_before_the_thread_sees_it asserts the error contains "Invalid IP address", but #248 changed the message to "Invalid address. Enter a literal IP and port, for example :5050". With both merged: 22 pass, 1 fails.
2. The mirror box would show the iMotions hint. mirror_connect reuses validate_tcp_addr, so a bad Python address would get "for example :5050". Could each box supply its own example, for instance by passing the hint into validate_tcp_addr? That fixes the test too.
3. The conflicts in state.rs and view.rs are just constants added in the same spot. Keep both blocks in state.rs. In view.rs, keep only DEFAULT_MIRROR_PLACEHOLDER, because #248 removed DEFAULT_TCP_PLACEHOLDER and the input now uses IMOTIONS_ADDR_HINT from state.rs.

Also from the rebase:

• The README still says inference/Data/data_logger.py. On main it's now inference/pilot_inference/capture/data_logger.py.
• There's a second ADR 0003 in progress, 0003-live-clustering-runs-in-a-python-sidecar.md. Whichever lands second needs renumbering.
• The PR body says Connect Python and the README says Connect mirror. Which is the button called?

[Phlabry]

Rebased onto main after #246 and #248. #248 renamed the validation message, so one of my tests asserted text that no longer exists, now fixed. 23 tests pass. Ready for another look.

[jmacd867]

Thanks for this. The writer-thread rewrite is the right fix: the TCP thread now only does a non-blocking try_send, connecting happens off it, and connecting_the_mirror_does_not_pause_the_imotions_stream pins that down. I pulled c394be7 and cargo test passes 23/23 on three runs in a row. The data_logger path and the test that asserted the old error text are both fixed.

A few things left, most important first.

Should fix before merge

1. A bad mirror address still gets the iMotions example. mirror_connect (state.rs:906) checks the address with validate_tcp_addr first, so a typo in the Python box shows "for example :5050". The mirror's own message in connect_mirror (state.rs:419) is never reached. The test only checks for "literal IP and port", so it doesn't catch this. Passing the example into validate_tcp_addr, or having each caller add its own, would fix it, and the test could then assert the mirror error doesn't mention iMotions.

2. "Sink" in the tests. The new GLOSSARY entry lists "sink" under Avoid, and docs/agents/domain.md applies that to test names: a_long_run_reaches_both_sinks_in_order, "the two sinks disagree", "the sinks diverged over a long run", and the comment above that test. Something like a_long_run_reaches_imotions_and_the_mirror_in_order would do. The local sink buffer in the slow-reader test is fine.

3. The queue comment promises more than the code does. MIRROR_QUEUE_CAPACITY says the queue rides out "a clustering refit without losing anything". It only fills while a write is stuck, and a stuck write closes the mirror after 100ms. So a refit that stops reading loses the mirror, not just some copies. The ADR's "first lose copies and then the mirror" overstates the first phase the same way. Also, "the oldest behaviour is to drop new packets" reads garbled.

Worth addressing

• Stale text from before the rewrite.
  • The doc on mirror_connect (state.rs:902) still says "The mirror lives inside the TCP thread".
  • The PR body still says Connect Python and that a connect blocks the TCP thread for up to 500ms.
  • The slow-reader test comment says 20ms a line, but the reader sleeps 1ms.
• A dead writer is logged as "behind". mirror_send treats every try_send error as a full queue, so if the writer thread has exited, the log first says "Python mirror is behind... iMotions is unaffected" before saying it stopped. Matching TrySendError::Disconnected separately would fix that.
• README:130, "iMotions keeps recording at full rate". The tests only use a plain listener standing in for iMotions, so this one should be marked as unverified on real iMotions, per AGENTS.md.

Smaller, take or leave

• MIRROR_WRITE_TIMEOUT has no doc comment, unlike the constants around it.
• MirrorLagging also carries the "caught up" message. Something like MirrorStatus would cover both.
• mirror_send and drain_mirror_events mostly work on Mirror's own fields and would read well as methods on it.
• After disconnecting or reconnecting the mirror, the old writer keeps sending its queued packets to the old socket until the queue drains. That's harmless, but worth a line in the ADR if it's intended.
• The time limit on the whole mirror_send from my first review wasn't added. That's fine now that it's off the iMotions thread, but the < 20s bound in a_python_process_that_stops_reading_is_dropped_not_waited_on is still loose.

The event-name list and the test fixtures are fine as a follow-up, like you said. Not checked against live X-Plane or iMotions.

[Phlabry]

Good catch on all of it. Your review landed about six minutes before I pushed the rebase, so most of it was already done, and one thing was not.

Already in, from the rebase:

1. The failing test. I hit the same thing and fixed it, though I asserted on "literal IP and port" rather than the full string.
2. Both conflicts resolved exactly as you describe, both constant blocks kept in state.rs, only DEFAULT_MIRROR_PLACEHOLDER kept in view.rs.

• The README path is now pilot_inference.capture.data_logger, as a python -m command.

Fixed now, in 201c300:

2. The hint leak. validate_tcp_addr takes the example for the box being validated, and MIRROR_ADDR_HINT sits next to IMOTIONS_ADDR_HINT in state.rs, so the mirror input and its error both say 127.0.0.1:5001. Unlike the iMotions hint it is a real working example, since the Python listener normally runs on the relay's machine. The test now asserts the mirror error names 5001 and does not mention iMotions at all, and it fails if I point the call back at the iMotions hint.

• The button is Connect mirror. The PR body was stale, now corrected along with the description of the writer thread.

On the ADR number: nothing else in any open PR adds one, so yours is unpushed. Mine is in review, so I will keep 0003 and yours can be 0004. Happy to flip if you would rather the sidecar decision took the lower number, it is a file rename either way.

23 tests, green over four consecutive runs.

[jmacd867]

Approving. The writer thread and the per-box hints fix the things that mattered, and CI passes. The remaining points from my 22:59 review are wording and comments, fine as a follow-up. The one I'd do first is the MIRROR_QUEUE_CAPACITY comment, since whoever writes the clustering side will read it.