Skip to content

socket export: the race's peer connects once a racer answered its last connection; a failed job reports the peer's verdict - #1083

Merged
Japabu merged 4 commits into
mainfrom
socket-wake
Oct 28, 2026
Merged

Japabu merged 4 commits into
mainfrom
socket-wake

Conversation

@Japabu

@Japabu Japabu commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

netstack_holders_exported_to_children's race (tests/guest/rust-tests/src/bin/socket_export.rs, race) stalled now and then on a macOS host: 9 stalls in 64 soak batches across three revisions (orchestrator's soak, evidence pr-1074 r4). issues/two-holders-racing-on-an-exported-socket-can-both-stall.md suspected a lost wake between two holders of one socket queue.

What it actually is

No wake is lost in ToyOS. The harness's peer made the race's two connects back to back. QEMU's user network listens on the forwarded port with a queue of one. A macOS host resets a connect that finds that queue full. When QEMU's loop has not accepted the first connection yet, as happens under TCG at netstack's mDNS claim, the second connect() fails with ECONNRESET. The peer's thread ended there. The guest's second racer waited, correctly, for a connection that never came. The job's failure dropped the peer's result, so the harness reported a silent guest after its 700+ s ceiling.

Observed (evidence below):

  • A packet dump of a stalled round shows one SYN on the guest's wire for the round's two connects. The previous round shows both.
  • The peer thread's own result for that run is a racer's connect: Connection reset by peer (os error 54).
  • netstat -Lan reads QEMU 11.1.1's forwarded listener as maxqlen 1. With QEMU suspended, the first connect sits unaccepted (1/0/1) and the second is reset with error 54, every time.
  • A plain macOS listener with backlog 1 that never accepts resets every connect after the first. With backlog 8 none are reset.
  • In every one of the 9 soak stalls and all 3 of mine, netstack's mDNS claim line comes at the race. Inference, not measured: the claim is when QEMU's loop is busiest (probe answers, IGMP report, announcement), which delays slirp's accept.

Inference, not measured here: a Linux host drops a SYN to a full queue rather than resetting it (tcp_abort_on_overflow off by default), so its client retransmits. That would explain why the stall was seen on macOS only.

Change

  • The race's peer connects one at a time, the second once a racer has written its line on the first: it waits on an event the guest answers, with no delay and no retry. The two datagrams still go at once. Coverage: the connection half usually has both racers blocked in accept when the first connection lands (each sends racing before it calls accept, so the peer cannot know; likely under TCG, as before this change), and the one that does not take it must wait on its bell again and be woken by the second. The datagram half still has two datagrams waiting together for two holders.
  • On a failed job, both peer-driven parts report what the peer's thread had come to (peer_verdict), so a step the peer could not take is named as the peer's.
  • The issue is deleted; the queue-of-one fact moves to SOCKET_EXPORT_PORT's comment.
  • Reviews r1 and r2: the holder's half of the bell protocol gets a type and a host model. look hands each attempt a Drain spent once; spending it gives the Drained that Drained::accept and Drained::receive send their request with. A raw ipc send of ACCEPT or RECEIVE, which netstack's server needs public, bypasses the token; review catches it. look is generic over the end's bell (EndBell in production), so a host test runs two holders over one queue against a modelled server. The node's datagram test gains a third arrival. No change in what the SDK sends or when.

The cause needed no guest, netstack or kernel change; the SDK change is the review's coverage of the contract below. The test's environment and targets are unchanged: QEMU x86-64 netcase.toml, user network with one forwarded port.

Checks

Evidence: https://github.com/ToyOSOrg/evidence/tree/a5c49b359811d9d1a19a76906a41195ebdfcdd32/pr-1083/r1 (README lists each file, command and exit code). Host: aarch64 Mac, QEMU 11.1.1, x86-64 guest under TCG, test alone, three boots in parallel.

  • cargo run -- --ci host at this tree: exit 0, 104 steps green.
  • The committed test alone at bde3e08ba: 40/40 exit 0. Run without --nocapture, so these logs do not show whether the race met the claim.
  • The discriminating guest check, the same 40-round race loop per boot (scratch instrumentation, not committed), --nocapture, same session and setup:
    • head bde3e08ba + loop: 12/12 boots pass, 480 races; netstack's mDNS claim fell inside the rounds in 10 boots.
    • base, the whole change reverted + the same loop: 6/6 boots fail. Four peers end on ECONNRESET at connect. Two end on EINVAL from set_read_timeout straight after a connect; that this is the same reset is inferred, not measured. Two earlier base loop boots stalled at rounds 7 and 4, and the plain base test stalled on its first run.
  • Mechanism, deterministic: a suspended QEMU's forwarded port resets the second connect every time; QEMU's listener reads maxqlen 1.

Round 3 (reviews r1 and r2), evidence https://github.com/ToyOSOrg/evidence/tree/b7dfd4045520217c8ea54f21cd17d25d62f90c9f/pr-1083/r3, at head b6a717df8 on 717baf3ac; every log begins with that HEAD and an empty git status --porcelain:

  • cargo run -- --ci host: exit 0, 104 steps green.
  • Negative controls, each a checked patch applied, run and reverted byte-identical, with a clean status after: look reading the bell after an ask answered nothing fails the SDK model test ("holder 1 waits for good with [2] queued"); re-arming a datagram holder only on a take fails the node test, (2, 2) against (2, 3).
  • Type records: a second drain.now() in one attempt is E0382; a raw ipc::signal of RECEIVE compiles.
  • netstack_holders_exported_to_children once with the SDK rebuilt into the guest: exit 0. The 40-round loop was not rerun: since r1 the harness changed only in comments, and the SDK sends the same requests in the same order.
  • After review r3, head 91b62446b moves #[must_use] from Drain to Drained, so a bare drain.now();, which reads the bell and sends nothing, warns (recorded). (cd sdk/toyos && cargo check && cargo test) at that head: no warnings, all tests pass. No guest rerun: an attribute changes no code. Evidence: https://github.com/ToyOSOrg/evidence/tree/60a99591018817e70c42bcd5ee493d6be07ddeb4/pr-1083/r3.
  • Round 2's controls ran on a socket.rs that differs from 9a26dea93 by doc comments only (r2-control-trees.diff). Round 2's host and guest runs recorded no tree; the runs above at a recorded head supersede them.

No metal run: the behaviour belongs to the host and QEMU's user network, not to hardware.

Risks

  • The connection half no longer has two connections waiting at once. It usually has both racers blocked in accept when the first lands, and the loser re-waiting on its bell until the second. Two connections arriving in one pass is covered on the host (every_holder_takes_from_one_queue_and_the_loser_finds_nothing).
  • A peer failure is now named, but the job still waits out its liveness ceiling before the harness reports it: the job's wait cannot be ended from the peer's thread.
  • kernel: a pipe keeps its bytes in 4 KiB frames it takes as it fills #1074's evidence README cites issues/two-holders-of-one-socket-queue-can-wait-for-good.md, a record on that branch that this mechanism explains; it should not land.

The wake contract for several holders of one queue

  1. The server records, per holder, whether it has signalled that holder since its last request.
  2. After every queue change and every answered request, the server signals each holder not yet signalled, while the queue is not empty.
  3. The server takes every request as a look, whatever it answers, and re-arms its holder (server half). The holder reads its signal once, immediately before its request, after every step that can fail, and never after the request (holder half).

A coalesced event bit is enough only with 1 to 3. One event shared by all holders, or a server that signals only when items arrive, can lose a waiter: with three items arriving together and two holders, the third is never signalled.

What checks each part, after this PR:

Part Check Negative control
1, 2, and 3's server half: the listener system/netstack/node/tests/listeners.rs every_holder_takes_from_one_queue_and_the_loser_finds_nothing (the P3 step: both holders rung after looks) not run in this PR
1, 2, and 3's server half: the datagram socket system/netstack/node/tests/datagram.rs every_holder_receives_from_one_queue_each_datagram_once_and_whole, now with a third arrival after a holder's empty receive: (2, 3) re-arming only on a receive that takes a datagram fails it, (2, 2)
3's holder half: read once, before the request types, partly: within one attempt the Drain is spent once; a Drained cannot be made outside socket.rs; stream::accept and recv_from send through Drained::accept/Drained::receive. Review covers the rest: a raw ipc send of ACCEPT/RECEIVE, or another read of the bell inside socket.rs, compiles a second drain.now() in one attempt: E0382 (recorded); the raw-ipc bypass compiles, with warnings (recorded)
3's holder half: look's loop, never reading after the ask sdk/toyos/src/net/socket.rs the_holder_that_found_nothing_takes_the_next_arrival: two holders, one queue, a modelled server keeping rules 1 to 3; the second arrival lands between the server's "nothing" and the holder reading it look reading the bell after an ask answered nothing fails it: "holder 1 waits for good with [2] queued"
3's holder half: drain after every fallible step (the region in stream::accept) inspection only none
The whole contract over the kernel's real bells and pollers the guest race (netstack_holders_exported_to_children) the base control above

The host tests are independent of the bell's mechanism: the node's Bell trait and the SDK's look over a Bell trait. They carry the contract through the IPC migration; the guest race remains the end-to-end check.

🤖 Generated with Claude Code

@Japabu

Japabu commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Review r1: bde3e08ba against origin/main b0164b262

Architectural assessment

This is a harness-only change: tests/toyos.rs, plus deleting the issue. No guest, SDK, netstack or kernel code changes. It touches the right place, because the defect was in the peer, not in ToyOS. Production code is unchanged. tests/toyos.rs gains 25 lines and loses 10, and the 27-line issue is deleted. The durable fact (the host forward's queue of one) moves to the forward's constant, as issues/README.md requires, and git grep bde3e08ba finds no remaining citation of the slug or path.

The causal chain holds. I checked each link in the published evidence.

  1. The old peer connected twice back to back: the removed collect in the diff.
  2. QEMU's forward listener has a queue of one: host/maxqlen.out, 0/0/1 under QEMU 11.1.1.
  3. A full queue on macOS resets the next connect: host/backlog.out (backlog 1 resets connects 1 to 3, backlog 8 resets none) and host/stopped.out (with QEMU stopped, the queue reads 1/0/1 and the second connect gets error 54).
  4. That reset happened in the stalled runs: logs/base-loop-run2-pcap.log:380, soak/baseloop-1.log:398 and soak/baseloop-3.log:506 each end with Ok(Err("a racer's connect: Connection reset by peer (os error 54)")).
  5. Only one connection reached the guest: logs/base-loop-run2.pcap.txt shows rounds 1 to 3 with two SYNs each, and round 4 with one SYN (62868), then the peer's FIN about 0.8 ms after the handshake (the first stream dropped by the ?). The guest's step lines end with parent accepted and no racer accepted.
  6. The fix removes the cause: the second connect now starts only after read_to_end saw the racer's FIN. For that to happen the guest must have received slirp's SYN, so slirp has already accepted the first connection and the host queue is empty.

The discriminator is 12/12 head loop boots (480 races, with the claim inside the rounds in 10) against 6/6 failures of the reverted base, in one session. That is a proper whole-change control. The 9 earlier soak stalls in #1074 are attributed to the same cause by their identical symptom; that is an inference, but a well-founded one.

The test keeps its subject, and arguably sharpens it. In the old version's passing rounds, the pcap shows both SYNs about 20 µs apart, so the guest usually saw two queued connections at once and each holder simply took one. Now the first connection lands alone. If both racers are in accept, both are told, one takes it, and the other must be answered Nothing, wait on its bell again and be woken by the second arrival. That re-arm is exactly the lost-wake hazard the record was worried about. The datagram half is unchanged.

The wait is an event with a failure timeout. The peer waits for the racer's FIN through read_to_end under set_read_timeout(CEILING) (120 s per read), inside the job's 300 s ceiling. There is no sleep and no retry. connect has no timeout of its own, but by step 6 above the queue is empty when it runs, and the job ceiling bounds it anyway.

The Linux inference is labelled ("Inference, not measured here") in the PR, and the commit and the source comment make no claim about Linux.

Findings

BLOCKER (PR record, no code change). PR description, "The wake contract for several holders of one queue": the claim that the contract "is modeled on the host" by the two cited tests, and that "these tests, rather than the guest race, should carry it through the IPC migration", is false for clause 3.

  • Clause 3 has two halves. The netstack server half ("a request is a look, whatever it answers, so it re-arms the holder") is in netstack. The holder half (consume the signal immediately before the request, and only then) is sdk/toyos/src/net/socket.rs:222 look, together with the drain() calls in stream::accept (sdk/toyos/src/net/stream.rs:748) and recv_from.
  • No host test covers that holder half. system/netstack/node/tests/* drives Node directly and never runs look. The guest race is the only check of the holder side with two holders, so recording it as dispensable for the migration would let that half go unguarded.
  • The datagram test is also weaker than stated. system/netstack/node/tests/datagram.rs:336 stops at Ok(None) / "nothing left, no ring". It never delivers again to show that the holder answered nothing is rung by the next arrival. The listener test does show this (listeners.rs:545, the P3 step, (2, 3)).
  • Consequence: this is the record the orchestrator will use to decide what guards the contract during the IPC migration.
  • Fix: restate that the netstack tests model clauses 1 and 2 and the server's re-arm on a look, and that the holder's clear-before-ask is exercised only by the guest race. Clauses 1 to 3 themselves read correctly against listeners.rs:8-12 and look. The cheapest way to close the datagram gap is a third lan.deliver after the Ok(None), asserting rings() == (1, 3). That is optional for this PR, since the contract statement is a proposal and not part of the change.

SUGGESTION. PR description, "Coverage": "the connection half still has both racers blocked in accept when the first connection lands" overstates it. Each racer sends racing before it calls accept (socket_export.rs, race), so the peer cannot know that both are blocked. That is likely under TCG, not guaranteed, and the same was true before this change. "Usually both" would be accurate.

SUGGESTION. PR description and commit: "That is when QEMU's loop is busiest (probe answers, IGMP report, announcement)" is an inference from the correlation with mDNS. It is not measured, so label it as one. The causal chain does not depend on it.

SUGGESTION. tests/toyos.rs:5443-5445: the inner comment "The race: both processes say they are about to block, then two connections, one at a time, and two datagrams at once, each taken once" repeats the function's doc comment just above. Deleting it loses nothing.

SUGGESTION. tests/toyos.rs:5350-5352: "QEMU's user network listens there" sits on the guest-port constant, while the queue of one belongs to the host-side listener. "Listens on the host's port" would remove the ambiguity. Alternatively, the sentence could move to the connect loop, whose shape it explains.

peer_verdict (tests/toyos.rs:5495) is correct. If the peer fails just after is_finished, the verdict says "still waiting", which is acceptable for a diagnostic. Dropping an unfinished handle detaches the thread, as the old code already did.

Verdict

The code is ready, and the record can close: its exit ("the cause found and fixed") is met and evidenced. The remaining blocker is the PR description's account of which tests carry the wake contract, which belongs to the orchestrator.

SEND BACK

Japabu added a commit that referenced this pull request Oct 28, 2026
…ecord goes back as base had it

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Japabu

Japabu commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Review r2: 9a26dea93 (focused on the r1 follow-up)

Architectural assessment

r1's blocker is resolved in substance. The holder half of the contract now has a host model, the datagram test has its third arrival, and both negative controls are real and discriminating. The PR body no longer says the guest race is dispensable. The SDK change is the right owner for the holder half: look and the request closures are where the bell is read. Requests and their order are unchanged. stream::accept and recv_from still drain immediately before signal/signal_with (the receiver drain.now() is evaluated before the call). look_now still runs ask under the end's turn, and the wait still uses its own poller outside the turn.

Size: about +55 / -20 production lines in sdk/toyos/src/net/socket.rs (Drain, Drained, the Bell trait, and EndBell, which is mostly the old look body moved), about +110 lines of SDK test and +2 node test lines.

Bell trait. Its only reason to exist is the test. It is a seam, though, not added machinery: one production impl, monomorphised, and the code is moved rather than new. No smaller seam lets the host run the real look. It earns its place, narrowly. It separates the retry loop from the poller mechanism, and the IPC migration is going to replace that mechanism.

The SDK model test is faithful, not right by construction. I traced it. Server::ask clears told for every ask, whatever it takes, then calls tell. tell rings each holder that has not been told while the queue is not empty. That is Holders::asked followed by Holders::tell(ready) (system/netstack/node/src/holders.rs:73,77-83; datagram.rs:205-210; listeners.rs:188,220). The after_nothing arrival lands after the server has answered and before look sees WouldBlock, which is the real window. The test supplies its own ask closure, so it checks look's loop and not the production closures. The table scopes it that way, correctly. The mutation Err(WouldBlock) => bell.drain() fails it with the expected panic (mutation-drain-after-ask.log, exit 101, restored identical).

The datagram test's (2, 3) follows netstack's actual rule. Trace: second takes "one" and is re-told, (1,2). FIRST takes "two"; nothing is left, so no ring. Second's empty receive still calls asked, so told is cleared. "three" then rings both: (2,3). The control that calls asked only on Ok(Some(_)) gives (2,2), so it discriminates exactly the empty-look re-arm (mutation-rearm-only-on-take.log, exit 101, restored identical).

A refused call leaves the ring in the bell. stream::open reserves the slot, creates the region and shares it before it calls ask (stream.rs:812-823). If any of those fails, the Drain is dropped unspent and the error is not WouldBlock, so look returns it with the bell untouched. recv_from has no fallible step before its drain. Leaving "drain after every fallible step" as inspection only is acceptable. The steps that would need a test are kernel calls (SharedMemory::create, share), so no cheap host test reaches them. The new type already confines the read: stream.rs cannot touch the listener's bell except through the Drain.

Findings

BLOCKER. The type-level claim is stated more strongly than the types enforce. This affects the PR table row "3's holder half: read once, before the request", whose control column says "does not compile"; the commit message ("A request without a read, and a second read after the request, no longer compile"); and the doc comment at sdk/toyos/src/net/socket.rs:219-222 ("Only the [Drained] it is spent for sends an accept or a receive").

  • ACCEPT and RECEIVE are pub const (socket.rs:57,67), and ipc::signal/ipc::signal_with are free functions. So ipc::signal(end.connection, RECEIVE) inside recv_from's closure, or ipc::signal_with(listener, super::socket::ACCEPT, [region]) in stream::accept, compiles: a request with no read. At most an unused drain parameter draws a lint.
  • In socket.rs, self::drain(end.bell) after the request also compiles (the closure parameter only shadows the name), as does constructing Drained(()) directly. A second read is impossible only in stream.rs, which has no bell handle.
  • What the types do guarantee is narrower: inside one attempt the Drain can be spent only once, a Drained cannot be made outside socket.rs, and the request helpers consume it.
  • The "does not compile" control has no recorded attempt.
  • Consequence: this is the same class as r1's blocker. The record the orchestrator will use to decide what guards the contract through the IPC migration reports compile-time enforcement that does not exist.
  • Fix: narrow all three statements to what holds, for example "ACCEPT and RECEIVE are sent through the Drained a single-use Drain yields; a raw ipc::signal with those types bypasses it and is caught in review". Change the control column to "none (type)", or name the misuse that really fails to compile (a second drain.now()).

BLOCKER (record, small). The PR says round 2's cargo run -- --ci host ran at "head 9a26dea93". The evidence README says "the r2 tree before commit". ci-host-2.log ran at 08:54-09:01 and r2-guest.log at 09:02. The later mutation runs (09:52) show a tree whose Drain::now still carried a doc comment ("Reads the bell empty. Call it after every step…") that the commit no longer has, so the tree changed after both runs. Comment-only edits need no rerun, but the record has to say so: diff the tested tree against 9a26dea93 and state that the difference is comments only (or rerun if it is not). The README also names ci-host.log, while the file is ci-host-2.log.

SUGGESTION. socket.rs:235-241: Drained::signal/signal_with take any msg_type, so the token says nothing about which request it licenses. Drained::receive(self, connection) and Drained::accept(self, listener, region) would fix the message type, read better at both call sites, and make the narrowed doc claim concrete. A #[must_use] on Drain is cheap too. Neither makes a raw-ipc bypass impossible, so the wording fix above stands either way.

SUGGESTION. socket.rs:219: "One request's leave to read the bell" reads ambiguously ("leave" as permission). "One request's read of the bell" is plainer.

Verdict

The code and tests are sound. The design, request order and refusal behaviour are unchanged, and the model and both controls hold up. Both blockers are corrections to the record and to one doc comment. The orchestrator owns the PR text; the commit message and the comment belong to the implementer.

SEND BACK

Japabu and others added 3 commits October 28, 2026 10:14
…t connection, and a failed job reports the peer's verdict

netstack_holders_exported_to_children's race stalled now and then on a
macOS host, and the record blamed a lost wake between the two holders.
No wake is lost. The harness's peer made its two race connects back to
back. QEMU's user network listens on a forwarded port with a queue of
one (QEMU 11.1.1, `netstat -Lan` reads maxqlen 1). A macOS host resets a
connect that finds that queue full, so when slirp has not yet accepted the
first connection, the second connect fails with ECONNRESET (os error 54).
That happens when QEMU's loop is busy: under TCG, every observed stall fell
at netstack's mDNS claim. The guest's racer then waits, correctly, for a
connection that never comes. The peer's thread ended with the error, but
the job's failure dropped the peer's result, so the ceiling reported a
silent guest.

Evidence, in the PR's evidence: a packet dump of a stalled round carries
one SYN for the round's two connects; the peer's own error is
"a racer's connect: Connection reset by peer (os error 54)"; a suspended
QEMU's forwarded port holds one connection unaccepted (1/0/1) and resets
the second connect; a backlog-1 listener on macOS does the same.

The peer now connects one at a time, the second once a racer has written
its line on the first. That is an event the guest answers, not a delay.
The datagrams still go two at once. A job that fails reports what the
peer's thread had come to, so a step the peer could not take is named as
the peer's, not the guest's.

Closes issues/two-holders-racing-on-an-exported-socket-can-both-stall.md;
the queue-of-one fact moves to SOCKET_EXPORT_PORT's comment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ers over one queue are modelled on the host on both sides

The holder half of the several-holder wake contract had no host check:
consuming the signal immediately before the request and never after it.
That half is `look` together with the drain calls in `stream::accept` and
`recv_from`, and only the guest race exercised it.

- `look` hands each attempt a single-use `Drain`. Spending it reads the
  bell and yields the `Drained` that alone sends an ACCEPT or a RECEIVE.
  A request without a read, and a second read after the request, no longer
  compile. The drain stays inside the request closures, after every step
  that can fail, so a call refused before its request still leaves the ring
  in the bell.
- `look` is generic over the end's bell (`EndBell` in production), so a host
  test runs it for two holders over one queue against a modelled server
  that keeps netstack's per-holder rule. The second arrival lands after the
  server answers holder 1 "nothing" and before holder 1 reads that answer.
  Negative control: `look` reading the bell after an ask answered nothing
  fails the test with "holder 1 waits for good with [2] queued".
- The node's datagram test gains a third arrival after a holder's empty
  receive: both holders are rung, the one that found nothing included.
  Negative control: re-arming a holder only on a receive that takes a
  datagram fails it, (2, 2) against (2, 3).
- The harness's inner comment that repeated its doc comment is gone, and
  the queue of one is said to be the host port's.

The commit before this one says the mDNS claim is when QEMU's loop is
busiest. That is an inference from the correlation, not a measurement.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d what its type guarantees is said exactly

The previous commit said a request without a read, and a second read after
it, no longer compile. That is stronger than the types. ACCEPT and RECEIVE
are public, because netstack's server parses them, so a raw `ipc::signal` of
either compiles. Inside socket.rs, a hand-built `Drained` or another read of
the bell compiles too.

What the types now guarantee:
- `look` hands each attempt a `Drain` that is spent once. A second
  `drain.now()` in one attempt is E0382, recorded in the PR's evidence.
- Spending it gives the `Drained` that `Drained::accept` and
  `Drained::receive` send their request with. Those name the message
  themselves, where `signal`/`signal_with` took any message type.
- A `Drained` cannot be made outside socket.rs, and stream.rs reaches the
  listener's bell only through its `Drain`.
- The bell is read in one place, `EndBell::drain`; the free `drain` is gone.

A raw `ipc` send of ACCEPT or RECEIVE bypasses the token. The doc comment
says so; review catches it, and the recorded attempt compiles with an
unused-variable warning.

`Drain` is `#[must_use]`. Rebased onto 717baf3.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Japabu

Japabu commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Review r3: b6a717df8 on 717baf3ac (focused on r2's two blockers)

Architectural assessment

Nothing about the design changes. The r3 commit is net -1 line in sdk/toyos/src/net/socket.rs and stream.rs. The free drain is folded into EndBell::drain. Drained::signal/signal_with become Drained::accept(listener, region) and Drained::receive(connection), which name their message type themselves. The doc comment is narrowed. Drain gets #[must_use]. The requests and their order are unchanged: recv_from still drains and then sends RECEIVE bare, and stream::accept still drains inside open's closure, after the slot and the region are made, and then sends ACCEPT with the region.

The rebase is clean. git range-diff b0164b262..9a26dea93 717baf3ac..262855361 shows both earlier commits identical (=). The only change between the bases is one line of CLAUDE.md, so the rebase brings no new defect.

Blocker 1 (type claim): resolved

Why the constants stay public. system/netstack/src/serve.rs:51 imports ACCEPT and RECEIVE. asked matches on them (serve.rs:963,974), and the server's tests use them, including the check that message types are distinct across protocols (serve.rs:1077). Keeping them private to socket.rs would mean the SDK exporting the server's frame classification instead, i.e. moving netstack's asked into the SDK. Even then, ipc::signal(conn, 7) with a literal would still compile. No representation makes a raw ipc send unrepresentable while ipc::signal is a free function over u32. Narrowing the claim is the right fix. Restructuring to win a marginal guarantee is not.

What the types now guarantee, checked:

  • Drain::now(self) consumes the token, so a second read in one attempt is E0382. This is recorded: compile-fail-second-read.log, check exit 101, and the error points at socket.rs:221, which matches the head.
  • Drained(()) has a private field, so only socket.rs and its test module can build one.
  • Both production requests go through Drained::accept (stream.rs:750) and Drained::receive (socket.rs:449).
  • The only read of a socket bell is EndBell::drain (socket.rs:272-275). The other read_nonblock calls in stream.rs:393,417 read a stream's own doorbells, not a socket bell.

The bypass is recorded too: compiles-raw-bypass.log, exit 0, with unused-variable and dead-code warnings. Both compile attempts ran on 262855361 plus the uncommitted r3 change, and the README says so. The stated difference from the head is one doc line. The cited line numbers (221, 237, 446-449) match the head, and a doc line cannot change a type error. I accept them.

The doc comment (socket.rs:213-217) now states exactly the single-use token, what it yields, the rule to spend it after every fallible step, and the raw-ipc bypass. It does not claim more than that.

The commit message of b6a717df8 states the guarantees exactly and names the bypass. It also says plainly that the previous commit's "no longer compile" sentence was overstated. That false sentence stays in 262855361's message on landing, but the next commit corrects it explicitly, which is an honest record.

The PR table reads "types, partly" and lists what review covers: a raw ipc send, or another bell read inside socket.rs. The control column records both attempts. It is accurate.

Blocker 2 (tested tree): resolved

  • r3-ci-host.log begins with ## HEAD b6a717df8…, git status --porcelain: '' and ends with Host(None): 104 step(s), all green and exit 0. It includes the_holder_that_found_nothing_takes_the_next_arrival, every_holder_receives_from_one_queue_each_datagram_once_and_whole and every_holder_takes_from_one_queue_and_the_loser_finds_nothing, all ok. The FAILED lines in the log belong to ci's own loom model controls (=== [ci] control wake-fence-off and the others), each of which reaches its verdict.

  • r3-guest.log has the same header. toyos and netstack are recompiled for the guest. The test is netstack_holders_exported_to_children on netcase.toml x86-64: PASS, exit 0. Both racers took one connection and one datagram.

  • Controls:

    • r3-drain-after-ask: mutation exit 101 with "holder 1 waits for good with [2] queued".
    • r3-rearm-only-on-take: mutation exit 101 with (2, 2) against (2, 3).

    Both logs begin at the head with a clean status, and each says the file was restored byte-identical with a clean status afterwards.

  • r2-control-trees.diff: r2's control socket.rs differs from 9a26dea93 only in the Drain/Drain::now doc comments. Its datagram.rs is identical (diff exit 0). The r2 host and guest runs are superseded rather than relabelled. The ci-host-2.log naming is corrected.

The reason for not rerunning the 40-round loop holds. Since r1, tests/toyos.rs changed only in comments (262855361: the SOCKET_EXPORT_PORT doc and a removed inner comment), and the SDK's requests are unchanged in kind and order.

Findings

No blockers.

SUGGESTION. sdk/toyos/src/net/socket.rs:217: the #[must_use] on Drain never fires. A Drain is only ever constructed as ask's argument (socket.rs:283), and ignoring a closure parameter draws unused_variables, not must_use. This corrects my own r2 suggestion. The value that matters is a discarded Drained. drain.now(); on its own reads the bell and sends nothing, which loses the ring: the server still counts the holder as told, so it does not ring it again for the waiting item. Moving the attribute to Drained (socket.rs:228) catches that statement. The message could be along the lines of "the bell was read for a request: send it".

SUGGESTION (evidence README). The r3 README gives the guest and control runs by test name and result but does not give the invocations. r1's README did. They can be worked out from the logs (--lib, --test datagram, the filtered tests/toyos.rs run), but one line each makes the record complete.

Verdict

Both r2 blockers are resolved. The claim now matches what the types enforce, both compile attempts are recorded, and every rerun is pinned to b6a717df8 with a clean tree.

LAND

…ent leave to read it

The `#[must_use]` on `Drain` never fired. A `Drain` exists only as `ask`'s
argument, and an ignored parameter draws `unused_variables` instead.

The value that matters is the `Drained`. `drain.now();` on its own reads the
bell and sends nothing. That loses the ring, because the server still counts
the holder as told and does not ring it again for the waiting item. With the
attribute on `Drained`, that statement warns "the bell was read for a
request: send it" (recorded in the PR's evidence).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Japabu
Japabu marked this pull request as ready for review October 28, 2026 09:38
@Japabu
Japabu added this pull request to the merge queue Oct 28, 2026
Japabu added a commit that referenced this pull request Oct 28, 2026
…ecord goes back as base had it

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merged via the queue into main with commit de417c2 Oct 28, 2026
10 checks passed
@Japabu
Japabu deleted the socket-wake branch October 28, 2026 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant