Stability & latency hardening: bounded stalls, MoQ stale-reset, truthful keyframes - #17
Merged
Merged
Conversation
…n demux Every uni stream now opens with a UniHello tag frame (Desktop/Sync/ Audio). The accepting side runs one accept_uni owner per connection that reads the tag and routes the stream to the consumer registered for it via Connection::uni_streams — desktop and sync can share a connection without racing the accept queue. PROTOCOL_VERSION bumps to 3; v2 peers won't interop on uni streams.
…roll; scope input display - mailbox::Sender notifies on drop — a consumer parked in recv() used to sleep forever after the producer's task ended (verified hang). Regression test: parked_recv_wakes_when_last_sender_drops. - X11 scroll clamped to 32 clicks/event: remote deltas are unbounded f64s; a huge one looped billions of paired XTEST calls, wedging the injector. - Input events naming a display other than the session's are dropped un-acked — the grant scope checked the hello's display but per-event display_id was never enforced. - Frame streams write the UniHello::Desktop tag; the client claims tagged streams via conn.uni_streams instead of accept_uni.
…nk streams - resolve_under(): the canonicalized deepest-existing ancestor of every destination must stay under the canonical sync root — a symlinked component (managed dotfiles, nix dirs) can no longer redirect pulls, the .rds-sync journal, or assembly outside the root. Offers are refused up front; the journal dir resolves the same way. - check_rel_path refuses .rds-sync as a first component — the journal namespace is not a peer-writable path. - Assembly's temp file is dest + '.rds-part' (appended suffix, no aliasing a peer's literal *.rds-part name) created exclusively after unlinking any stale/planted one. - ChunkHdr.len is validated against the manifest before sizing the receive buffer — a forged u32 no longer forces a multi-GiB alloc. - Chunk streams carry the UniHello::Sync tag; the receiver consumes them via the connection demux. - E2E: symlink_escape_refused (pull/push/journal-redirect), forged_chunk_len_rejected, .rds-sync namespace refusal.
The freshness check ran under a read lock that was dropped before the store — two concurrent valid PUTs could regress the snapshot. The write lock now covers verify and store.
Both consume uni streams; the v3 demux routes Desktop- and Sync-tagged streams to their own consumer. Proves the fix for the pre-v3 race where two accept_uni callers could swallow each other's streams.
…og; C5/C6 checklists - architecture.md documents the UniHello tag + uni_streams demux and the two-phase (lexical + resolved) sync confinement. - deployment.md and the agent unit note that PrivateTmp hides /tmp/.X11-unix — desktop capture needs the abstract socket, a user-level agent, or PrivateTmp dropped. - CHANGELOG records protocol v3, the confinement/alloc/scroll/display fixes and the mailbox liveness fix. - C5/C6 manual checklists filled honestly: the review they anticipated happened and found the bugs these commits fix.
- uni_demux now drops every registered sender on exit — a dead connection ends its UniStreams inboxes (recv → None) instead of leaving consumers parked forever. rds-sync's receive loop and the desktop frame task both rely on None as end-of-stream. - mailbox::Sender::drop used to notify while its own Arc still counted, so a receiver woken cross-thread could read a stale strong_count, re-park, and hang — the same bug the wake was meant to close. An explicit AtomicUsize sender count now falls before notify_one; recv checks it instead of strong_count. - x11 scroll comment corrected: f64::min ignores NaN, so a NaN delta caps at MAX_SCROLL_CLICKS rather than producing zero clicks. - Regression tests: uni_demux.rs (recv → None on conn death, claim on a dead conn ends immediately) and parked_recv_survives_drop_wake_race (multi-thread drop/wake stress). Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…deny policy cargo-machete flagged 13 declared dependencies that nothing imports: serde (agent/desktop/relay), bytes (cli/relay), rand (relay), tracing (discovery, bench), proptest (sync dev-dep), rds-core (bench), async-trait (desktop), thiserror and tracing-subscriber (net). All removed; the workspace still builds and tests green under --locked. deny.toml gains the two licenses the iroh transitive tree actually ships (Unlicense, CDLA-Permissive-2.0) and ignores the two unmaintained advisories that have no upgrade path (atomic-polyfill via rustls, paste via iroh-quinn) — with a comment explaining the bounds so the ignore can't silently absorb future advisories. `cargo deny check bans licenses advisories sources` and `cargo audit` now pass locally; `cargo machete` is clean. Generated with Devin Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ning
Replaces the hand-rolled workflow with callers of
NDDev-OpenNetwork/ci-workflows @ a7b90bd (release 0.1.26):
- ci.yml → rust-ci: locked build, fmt --check, clippy across five
feature lanes (default, noq/owned-relay, metrics, x11, desktop),
and a ubuntu+macos test matrix running the workspace suite plus
feature tests.
- supply-chain.yml → rust-supply-chain: cargo-deny, cargo-audit and
cargo-machete on every PR/push and weekly, closing the gap where
deny.toml documented a check no workflow ran.
- codeql.yml → public-codeql: rust (build-mode none) + actions
analysis; enables code scanning, which default setup reports as
not-configured.
Top-level permissions remain {}; caller jobs declare only the
scopes their callee needs; ubuntu-latest everywhere (public repo).
actionlint clean; zizmor 1.26.1 pedantic clean.
Generated with Devin
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Tagging X.Y.Z runs resolve → authorize (release environment) → release-supply-chain, which validates the release contract (VERSION file == tag, CHANGELOG heading) and publishes the source archive, SPDX SBOM, SHA256SUMS and SLSA/SBOM attestations. Adds VERSION (0.1.0, matching Cargo.toml) and a tag ruleset (refs/tags/X.Y.Z: creation open, deletion + rewrite denied), applied to the repo as ruleset 23828256. Generated with Devin Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Mirrors the estate convention: grouped minor/patch cargo updates and action pinning updates with a 7-day cooldown so fresh releases get a bake-in window. Generated with Devin Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with Devin Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
YAML `>-` folding only joins lines at the scalar's own indentation; the extra-indented `--features`/`-- -D warnings` continuations kept their line breaks, so bash executed `--features` as a command and the clippy job exited 127. Flatten every continuation to one line. Generated with Devin Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… test Path pinning caps concurrent multipath paths at one, but it does not suppress iroh's direct-path probes at candidates learned through in-band address exchange. Whether probes land inside the test window is platform timing — macOS runners observed 4 direct datagrams, Linux none. The counter is correct to count them; asserting zero tested a transport-timing property, not counter accuracy. The C7 gate stands on the relay side: payload datagrams, sent and received bytes and seen paths must all register via=relay. Generated with Devin Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
New improvement work stands on both pending PRs: the v3 demux + confinement hardening and the ci-workflows adoption. CHANGELOG keeps both Unreleased entries; the merged lock re-adds rds-sync as an rds-desktop dev-dep (the coexistence test needs it) while keeping the machete removals. Generated with Devin Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The demux read each UniHello tag inline, so a peer that opened a uni stream and never tagged it blocked routing of every stream queued behind it for the connection's lifetime. Each accepted stream now gets its own tag-read task bounded by a 10s timeout — a stalled tag costs one task seconds, not the connection's routing. Route removal on a failed send is also channel-identity checked now: a stale sender's failure cannot clobber a kind that was re-claimed by a new uni_streams() call. Regression test: stalled_tag_does_not_block_routing.
The decoder lived in a function-static Mutex shared by every session — two concurrent viewers would cross-contaminate each other's H.264 reference chains. Decode state now lives in a per-session Delivery owned by the connection's frame task. A decode failure now requests an IDR (rate-limited at 500ms) so a corrupt stretch resyncs instead of decoding deltas against a broken reference until the next periodic keyframe — without letting the stretch turn into an IDR storm. Session ack and per-frame header/body reads are bounded at 30s and frame payloads at 32 MiB, so a peer that opens a tagged stream and drips bytes parks seconds of budget, not a task forever.
…ority A frame that went stale while its stream was still sending used to finish on the wire — bytes the client would drop anyway, consuming path capacity the fresher frame needs. The writer now selects between the payload send and the next queued frame: a stale delta in flight is reset mid-write (MoQ-style), an in-flight keyframe always finishes (the chain behind it depends on it), and the producer's IDR flag is re-armed after a reset since the client's delta chain just broke. The collapse is extracted into a decode-aware helper applied after dequeue AND after the pacing wait, and empty encode-failure placeholders are skipped instead of sent as undecodable payloads. Frame streams now carry an explicit constant priority (i32::MAX/2 — above QUIC's default, strictly below control). Escalating per-frame priorities, which starved in-flight sends, are not reintroduced.
The keyframe flag was seq % 240, but intra_frame_period defaulted to auto — periodic IDRs never fired on schedule, so the flag could mark deltas as keyframes. The collapse trusts that flag to decide what may be superseded, so a wrong flag breaks decode chains. The flag is now read off the emitted Annex-B NAL units (type 5), and the intra period is pinned at 240 frames (~8s at 30fps) as a bound on undecodable time. set_bitrate was a silent no-op: OpenH264 has no live rate setter, so the adaptive controller's decisions never reached the encoder. Changes past a 15% deadband now queue a lazy rebuild whose first frame is a real IDR with fresh SPS/PPS — exactly the resync a rate shift wants; smaller steps ride the token-bucket pacing alone.
manifest_of() loaded whole files into memory; manifest_of_reader / manifest_of_path stream through StreamCDC instead — memory bounded at one max-size chunk, with a regression test proving identical cut points to the slice chunker. Manifest scans, journal open/verify and assembly now run on spawn_blocking; chunk reads use tokio::fs. Verified chunks are stored by a dedicated blocking-pool sink behind a bounded queue, so disk latency never parks an async worker or the wire pipeline. Completion is counted on the wire (the peer sends exactly the Need set) instead of the sink's asynchronously lagging counter, which used to leave the receive loop parked in a 300s stall after the last chunk arrived. Every protocol read, uni-stream wait and chunk body is bounded by a 300s stall. store() returns whether the chunk was newly present and skips the tmp-write+rename for duplicates; destination seeding gates on metadata before scanning and hashes streamed.
…anic A peer that opened a bidi stream and never wrote its StreamHello parked a task per stream for the connection's lifetime — the read is now bounded at 15s. Every state mutex (authz state, watcher, grant id, active grants) now recovers from poisoning via into_inner — the guarded data is plain state whose invariants a panic cannot tear, so one panicked holder can no longer deny service to every later connection. Both unreachable!() arms on the request path now write an explicit HelloAck::Error refusal before bailing — a request path must never panic if its coupling assumption breaks.
/v1/registry PUT did read-verify-drop-write: two racing valid PUTs could interleave so the older snapshot won. verify_fresh and the store now run under one write lock, matching /v1/revocations. Regression test concurrent_registry_puts_cannot_regress races eight snapshots and asserts the newest lands. All service locks (registry, revocations, rate limiter, metrics) now recover from poisoning — they guard plain data a panic cannot tear, so one panicked holder must not 500 every later directory request.
Relay: a connection that never opened its control stream or sent Register parked a task per connection; both waits are bounded at 15s. CLI: endpoint.connect could stall inside hole punching/relay fallback — now bounded at 30s; every HelloAck wait and the ping echo bounded at 15s, so a silent peer fails fast instead of hanging the command.
The desktop media section claimed freshness was enforced "rather than by stream reset" and that frame priorities were removed — both changed this pass: stale frames reset mid-send, frame streams carry a constant priority below control (escalating per-frame priorities that starved in-flight sends stay removed). The sync and stability sections now describe streamed manifests, the journal sink, wire-counted completion, and the per-stage stall bounds.
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.
Summary
Stability/latency hardening pass over the whole workspace, built on the
review-hardening base (#15) with the CI/CD branch (#16) merged in —
this PR contains both, so landing it lands all three.
Transport (
rds-net) — the uni demux read eachUniHellotaginline: a peer that opened a stream and never tagged it stalled routing
of every stream behind it. Each accepted stream now gets its own
tag-read task under a 10s bound; failed-send route removal is
channel-identity checked so a stale sender can't clobber a re-claimed
inbox. Regression test
stalled_tag_does_not_block_routing.Desktop media — the doc claimed freshness was enforced "rather than
by stream reset"; now it actually resets: a frame that goes stale while
its stream is still sending is abandoned mid-write (MoQ-style), the
producer's IDR flag re-arms, and undecodable queued deltas are drained.
Keyframes still always finish. Frame streams carry a constant priority
below control (escalating per-frame priorities, which starved in-flight
sends, stay removed). The decoder moved from a function-static Mutex —
which cross-contaminated reference chains between sessions — to
per-session state, and decode failure auto-requests an IDR, rate-limited
against storms. Session ack + frame header/body reads bounded at 30s,
payloads at 32 MiB.
Codec — the
keyframeflag wasseq % 240whileintra_frame_perioddefaulted toauto: the flag could lie to thecollapse that trusts it. It is now read off the emitted Annex-B NALs;
the period is pinned at 240 (~8s at 30fps) as a bound on undecodable
time.
set_bitratewas a silent no-op — OpenH264 has no live ratesetter — so changes past a 15% deadband now lazily rebuild the encoder
(whose first frame is a real IDR).
Sync — manifests stream through
StreamCDC(memory bounded at onemax-size chunk; proven identical cut points to slice chunking); manifest
scans, journal open/verify and assembly run on the blocking pool;
verified chunks go through a bounded blocking-pool sink; every protocol
read/chunk body is stall-bounded at 300s. Completion counts chunks on
the wire rather than the sink's lagging counter — the old condition
parked the receive loop 300s after the last chunk arrived.
Agent / discovery / relay / CLI — stream hello 15s, registry PUT
verify+store under one write lock (concurrent-PUT regression test),
service locks recover from poisoning, request paths refuse with
HelloAck::Errorinstead ofunreachable!, relay register 15s, CLIconnect 30s + acks 15s.
Test plan
cargo fmt --check— cleancargo build --locked --workspace --all-targets— cleancargo clippy --workspace --all-targets --locked -D warnings— all 4 lanes (default, noq/owned-relay, metrics, x11)cargo test --workspace --locked— all targets green, incl. 12/12 sync e2e in ~25s (previously 7×300s stalls)cargo deny check/cargo audit/cargo machete— cleanstalled_tag_does_not_block_routing,concurrent_registry_puts_cannot_regress,streaming_manifest_matches_slice,keyframe_flag_matches_emitted_nals,bitrate_change_rebuilds_with_idrGenerated with Devin