feat(serf-compio)!: API parity with serf-reactor (+ 3 real defects fixed)#97
Open
al8n wants to merge 6 commits into
Open
feat(serf-compio)!: API parity with serf-reactor (+ 3 real defects fixed)#97al8n wants to merge 6 commits into
al8n wants to merge 6 commits into
Conversation
OsResolver and DnsResolver both declare Address = HostAddr<SmolStr>, but the Transport impls demanded A: Data and HostAddr has no Data impl — the orphan rule makes one impossible. So neither resolver could supply an advertise address, and the declared default A = HostAddr<SmolStr> was dead. A is the caller's unresolved address domain, resolved to a SocketAddr once at construction and never encoded, so the bound bought nothing. Re-exports JoinFailed and DEFAULT_JOIN_DEADLINE, which appeared in the public API but could not be named, and corrects the doc claiming A constrains join's seeds — it does not, and joining by hostname while advertising a resolved address is deliberately allowed.
…expose the SWIM knobs UnreliableTransport::Datagram is the default and is documented as encrypted and attested, but the QUIC driver sent every gossip packet over a plain UDP socket and never consulted the setting — so a node configured for datagram gossip got plaintext UDP instead, silently. It now queues datagrams on the connection and falls back to UDP only where the status says to. The transports gained the seven SWIM timing knobs the reactor has; without them EndpointOptions took no override and failure detection could not be tuned at all. Joins now clamp their deadline to the coordinator's exchange deadline, so a join can no longer report JoinAllFailed before the caller's deadline elapses. Adds SerfError::LeaveFarewellUndelivered, the rustls backend features, the test-only message dropper, and the re-exports for MaybeResolved and DnsError, which the public signatures already named but callers could not.
… to the OS DnsResolver::resolve swallowed every tcp_query error, including TimedOut, and escalated into the unbounded OS resolver — so a slow or hostile nameserver could burn the per-query TCP budget and then hang bootstrap in unbounded OS DNS, defeating the timeout contract. A timed-out query now returns the error, matching serf-reactor.
The QUIC driver imports ExchangeId, but it was gated on the tcp feature alone, so a quic-without-tcp build failed to compile. Gate it on either transport, as serf-reactor does.
…rs, idempotent shutdown The handle gains the advertise address-domain parameter it was missing, so it now mirrors serf-reactor's shape (minus the runtime parameter compio does not need). Adds the tcp/tls/quic constructors and their *_with_rng variants, so a node no longer needs the ten-argument turbofish. shutdown() is now idempotent: a second or concurrent call awaits teardown and returns Ok once the sockets are released, rather than failing immediately while the port is still bound. Ports serf-reactor's unit suites (resolvers, transports, options, the parked key-response path) adapted to the !Send model, and forwards compio's rustls backend features through the serf umbrella.
Two QUIC scenarios failed under coverage instrumentation and on Windows while passing on fast hardware: the membership always converged, but the assertions raced the event delivery. Five hook checks asserted a notify_* callback synchronously right after polling the snapshot, but the callback lands on a separate path that can lag snapshot publication — a gap QUIC's handshake widens. They now poll until the hook fires, bounded by the same window, the way the reactor's suite does. The parked-key test released B's acknowledgement after a fixed 300 ms, which could fall outside the key query's response window under load. The acknowledgement now completes on the next runtime turn, tied to the rotation rather than a wall-clock offset, so park and ack scale together — mirroring the reactor's persistence-worker mechanism.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## refactor/sansio #97 +/- ##
===================================================
- Coverage 95.19% 95.05% -0.14%
===================================================
Files 94 94
Lines 18899 19555 +656
===================================================
+ Hits 17990 18588 +598
- Misses 909 967 +58
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
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.
What
Brings serf-compio to public-API parity with serf-reactor. The two crates wrap the same
serf-protomachine and are meant to be feature-equivalent siblings differing only in runtime model (Send/agnosticvs!Send/compio) — they had drifted badly. An audit found 14 divergences; three of them were real defects, not just missing surface.Breaking changes are intentional and were authorized.
The three defects (fixes, not parity surface)
1. compio sent QUIC gossip as plaintext UDP.
UnreliableTransport::Datagramis the default and is documented "encrypted + attested", but compio's QUIC driver ignored the setting and sent every gossip packet over a plain UDP socket — so a node configured for datagram gossip silently got unencrypted UDP. The driver now queues datagrams on the connection and falls back to UDP only where the send status directs. Mutation-verified: forcing gossip back onto UDP fails the datagram test while theUdp-opt-out test still passes — the two together are the discriminator.2. The
dnsandgetifsresolvers could not build a node — in both crates.OsResolver/DnsResolverdeclareAddress = HostAddr<SmolStr>, but theTransportimpls demandedA: Data, andHostAddrhas noDataimpl (the orphan rule makes one impossible). So the resolvers were unusable for the advertise address and the declared defaultA = HostAddr<SmolStr>was dead. The bound was gratuitous —Ais resolved to aSocketAddrat construction and never encoded. Removed. Verified: a node now builds and joins overA = HostAddr<SmolStr>, and re-adding the bound reproduces the exact user-facing error.3. compio's DNS resolver defeated its own timeout.
DnsResolver::resolveswallowed everytcp_queryerror includingTimedOutand escalated into the unbounded OS resolver — a slow or hostile nameserver could burn the TCP budget then hang bootstrap. Now a timed-out query surfaces the error, matching the reactor. This was caught by a ported reactor unit test (failed pre-fix, passes post-fix).A fourth, smaller one: compio's
quic-only build didn't compile (ExchangeIdgated ontcpalone while the QUIC driver imports it). Fixed toany(tcp, quic), matching the reactor; acargo hack --each-featuresweep found no siblings.Parity surface
Serf<I>→Serf<I, A>(breaking) — the handle regains the advertise address-domain brand; compio drops only the reactor'sR(compio is the runtime).joinstays generic over anyResolver— joining by hostname while advertising a resolvedSocketAddris deliberately kept, and the reactor's doc comment that wrongly claimed otherwise was corrected in both crates.tcp/tls/quic+*_with_rng, replacing the ten-argumentSerf::new::<..>turbofish.shutdown()(breaking behavior) — a second/concurrent call now awaits teardown and returnsOkonce the port is released, proven by a rebind assertion.SerfError::LeaveFarewellUndelivered, thetls-rustls-ring/-aws-lc-rsfeatures, thetest-gated message dropper, and re-exports ofMaybeResolved/DnsError/JoinFailed/DEFAULT_JOIN_DEADLINE(the first two already appeared in compio's public signatures yet were unnameable). Theserfumbrella now forwards compio's rustls features.!Send; skipped only the genuinely reactor-specific ones (Send/Syncassertions, runtime-generic scaffolding).Verification
--test-threads=1CI shape.--no-default-features --features quic-rustls-ringbuild now compiles;cargo hack --each-featureclean.-D warningsclippy (including the literal ci-compio feature string),cargo fmt --all --check,cargo docfor both crates and the umbrella at--all-features, and the umbrella smoke test: all green.Note
Delegate::message_dropper()is honored only on TCP in both crates (TLS could support it; QUIC would need aserf-protoaddition). Mirrored the reactor rather than diverging — a candidate follow-up.Stacked on #96 (now merged). Held for review — not auto-merged.
🤖 Generated with Claude Code