feat(supervisor): hand over the tail with the subscription so first lines survive (T16c) - #53
Merged
Conversation
…ines survive (T16c) Capture::start puts the reader threads on the pipes before it returns, and Runner::relay subscribed from inside a tokio::spawn the runtime is free not to poll. A broadcast delivers nothing to a receiver that did not exist when the line was sent, so every line printed in that window reached current.log and never the daemon's ring — permanently, not until something caught up. Those are a service's first lines, the ones that explain a start nobody was watching. Capture::read hands over what the ring holds together with the subscription, without releasing the ring, exactly as ServiceLog::read hands a client its tail and its stream. relay takes it instead of subscribe and records what it was given before it pumps anything. The second half was not in the task and is what makes the first half true. Sink::accept pushed to the ring, released the lock, and only then published, so taking both under one lock would have handed a line over twice: once in the tail and once again on the stream. ServiceLog::record has always published while holding its ring — its doc calls that the property the whole type exists for — and accept now does the same. The lock is held across a broadcast send, which never waits for a receiver, so the reader thread pays a bounded push and a wakeup. Still no test for the race itself, as the task predicted. What is tested is that it has nowhere left to happen: a line recorded before the handover is in the tail and not on the stream, one recorded after is on the stream and not in the tail. Capture::record is #[cfg(test)] and unreachable from the daemon crate, and making it reachable would put test-only code in a shipped one. The module note still said Capture::subscribe was the whole of what a forwarding reader needs. That sentence was the bug, and now says so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A service's first lines reached its file but not the ring a reader was given, because
Capturehanded out a subscription and nothing else: abroadcast::Receivergets nothing sentbefore it existed, so everything the process said between spawn and
relaywas lost to the API.Capture::readnow hands over the ring's contents and the subscription under one lock, theway the daemon's
ServiceLog::readalways has, andRunner::relayrecords the tail before itstarts pumping.
The half the task did not name
Taking both under one lock is only correct if nothing can sit in the ring and not yet on the
channel.
Sink::acceptpushed to the ring, released the lock, and only then broadcast — sothis change alone would have delivered such a line twice: once in the tail, once again a
moment later on the stream.
acceptnow publishes while still holding the ring, which is whatServiceLog::recordhas always done and what its own doc calls "the property the whole typeexists for".
The cost is a lock held across a
broadcast::Sender::send, which never waits on a receiver — asubscriber that has fallen behind is told it lagged. The file's lock, the only one that does
I/O, is unchanged and still the outer one.
Tests
Two, written first and watched to fail:
What is not covered: the race across a real service and
relay.Capture::recordis#[cfg(test)]and the daemon cannot reach it; opening it would put test-only code in a shippedcrate, which
standards/rust.mdforbids. The limit is written into the roadmap rather than leftimplied.
The module doc's opening sentence — "
Capture::subscribeis the whole of what they need fromhere" — was the bug stated as documentation. It now says why it is not.
Checks
cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --all --check, rustdoc with-D warnings, supervisor 68 lib tests + 4 suites, daemon all 12 suites. CI green on all ten jobs(the first bench (ubuntu) failure was the known M3 warm-start bimodality: a rerun of the same
commit moved the median from 11118 ms to 7037 ms with nothing changed).
Phase 1 is now 15/15.