Repository navigation
Engineering quality pass: bugs, security, performance, dead code - #20
Merged
Merged
Conversation
The core config tests saved and deleted files in the user's real ~/Library/Application Support/dev.brolink.BroLink, and every test that built a Service, a HostApp or a ClientApp read that install's host.toml and client.toml (and would have rewritten them had a test changed a setting). data_dir() now honours BROLINK_DATA_DIR, and the workspace's .cargo/config.toml sets it to target/brolink-data for cargo test and cargo run. A shipped binary never sees the variable. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
a2f2ebe loosened the rule from "whois says the same user" to "whois knows the peer at all", so the relay VPS (a tagged node, whose own user is tagged-devices) would answer the Mac. That also admitted every machine shared in from another tailnet: it could pair with Sunshine through /v1/pin, sleep or restart the PC, read and write its clipboard, and POST /v1/update with any executable whose SHA-256 it sent alongside. The service now admits the account it is signed in as. A tagged node has no account, so it admits members of its own tailnet: the login that names a personal tailnet, or the users of an organisation's domain. Refusals are cached like approvals, and logged as before 3.x. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The viewer tells a new copy on the host from an old one by the clipboard's sequence number, and only Windows has one: elsewhere read() reported 0 every time, so after the first poll no copy on a Mac or on the Linux VPS desktop ever reached the viewer. The sequence number is now derived from the text itself. The host also opened and dropped an arboard::Clipboard for every read and write. On X11 the selection belongs to that handle, and dropping the last one discards the text unless a clipboard manager takes it within 100 ms, so text pasted in from the viewer could be gone before the desktop pasted it. One handle now lives for the whole process. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
apply() only rolled back when Windows refused to create the new process at all. A replacement that started and then crashed (panic = "abort", a missing DLL) left the PC with the new file in place and no service: the old one exits right after apply() returns, and nothing starts the new one again until the next logon, which on an unattended PC is never. The new copy waits for the old one's port, so a healthy one is still running a moment after launch. apply() now waits two seconds and, if the child has already exited, moves it aside and restores the old executable while the old service keeps serving. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
http::request, request_with and the GitHub fetch all connected to the first address a name resolved to and gave up if it failed. GitHub's download hosts resolve to several addresses, and a network with an IPv6 address but no IPv6 route lists an unusable one first. A shared connect() now walks the list. request() also goes through request_with() instead of duplicating it, and takes its Host header from the connected peer. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
build.rs wrote "MIT license" into brolink-host.exe's LegalCopyright string. BroLink is GPL-3.0-or-later, as is the moonlight-common-c it links, and that string is what Explorer, Task Manager and installers show. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Service::status() asked machine_name() for the name on every request, and off Windows that runs `scutil --get ComputerName` or `hostname`. The window polls /v1/status every second and every other machine's lobby every three, so the service spawned a process one to two times a second for a name that almost never changes. It is now read at start and then once a minute with the other slow probes. status() went from 3.4 ms to 0.65 µs a call on this Mac (200 calls, test build). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
decode() hands VideoToolbox a pointer to a Decoded on its own stack as the frame refcon, and the output callback writes the picture through it. That is only sound while every callback for the frame runs before decode() returns. Synchronous decode does call it before returning, but a decoder is allowed to hold a frame back and emit it during a later call, which would write through a stack frame that no longer exists. decode() now calls VTDecompressionSessionWaitForAsynchronousFrames after a successful submit, as FFmpeg's VideoToolbox hwaccel does. Also drops what was never used: VideoToolbox::full_range (always false), the width and height every decoder constructor ignored, and OpenH264's intermediate chroma buffer, which cost a copy of the UV plane per frame on Windows and Linux. nal_units and interleave_uv are private to the platform that uses them. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
While Sunshine reports no audio device, the service calls take_over_engine() on every sixth tick, and install_helpers() rewrote take-over-engine.ps1, .vbs and .cmd and ran `schtasks /Create /F` each time. The files are now written only when they differ from what this build carries, and the logon task is registered once per process. brand.rs also read VerQueryValue's UTF-16 strings and translation pair by turning a pointer into a Vec<u8> into a &[u16], which assumes an alignment a byte buffer does not promise. They are read unaligned now. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
connect() retried server_info() eight times, 700 ms apart and up to 6 s each, without looking at the cancel flag, so a Cancel while the PC was not answering waited out the whole retry. retry() now checks the attempt between tries. The Live stream's codec label said HEVC whenever the PC offered it and the setting allowed it, but Windows and Linux mask HEVC out of the formats they ask for, so they streamed H.264 under an HEVC label. The choice now also asks whether this machine decodes HEVC. Removes what never had an effect: stream::Settings::remote (every stream is flagged local, see the packet-size note in session.rs), with its BROLINK_TEST_REMOTE hook, lan_like_stream() and tailscale::overlay_or_lan(), and fixes two doc comments that sat on the wrong items. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Session's own mouse, key, text and scroll methods duplicated Input's; every caller goes through Session::input(). - tailscale::Status::windows_peers, superseded by machine_peers since every OS can share. - http::serve (serve_with_peer_check is the one the service uses) and nvhttp::Client::pair (the app pairs through pair_cancellable) are now test-only helpers. - ffi declarations for constants and moonlight functions BroLink never uses (STREAM_CFG_REMOTE/AUTO, COLOR_RANGE_FULL, the AVC reference invalidation flag, CONN_STATUS_OKAY, LiGetPendingVideoFrames, LiRequestIdrFrame), and the module-wide allow(dead_code) that hid them. - audio::Player's decoded counter and its packets()/decoded() accessors, which only the tests read; the test checks the queue instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
path::effective took the measured path and ignored it: since 3.3 Auto asks for the same quality whatever the route. The parameter made every caller build a Path it did not need, and the test looped over paths that could not change the answer. Also removes a doc line left over from a deleted function in stream.rs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The Wake-on-LAN step printed `Step "Enabling Wake-on-LAN on '<name>'"`. The name was escaped for a single-quoted literal, but this string is double-quoted: a `"` in the adapter's name ended it, a syntax error that fails the entire elevated script, and `$x` or `$(...)` in the name was expanded or run. Both names are now assigned once as single-quoted literals and used through variables. Also drops a second, undocumented BROLINK_DUMP_SETUP hook that did what BROLINK_DUMP_SETUP_SCRIPT does, fixes two test helpers whose doc comments were swapped, and a stale task reference in migrate.rs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The Windows setup script is generated, so a quoting slip in Rust shows up only on a real PC. The Windows job now dumps it for every plan shape (fresh install, migration, each with and without dry run), with an exe path and adapter names that need quoting, and runs it, install-host.ps1 and take-over-engine.ps1 through Windows PowerShell's own parser. The macOS and Linux jobs and the release workflow listed four crates by name and so never ran brolink-ui's tests; they use --workspace now. The Linux job and the node Dockerfile stop installing libssl-dev and libpulse-dev, which nothing links (there is no openssl or libpulse crate in Cargo.lock). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
build.rs globbed every .c file under the vendored src/ and enet/, so it also built four files nothing links: ConnectionTester.c and SimpleStun.c (public helpers BroLink never calls), RecorderCallbacks.c (only for LC_DEBUG_RECORD_MODE) and ENet's compress.c (moonlight never enables compression). They are gone, and build.rs lists the sources it compiles, so a re-vendor has to decide about each new file. VERSION now records the upstream commits of moonlight-common-c and its ENet and nanors submodules, everything trimmed, and the two local patches with their reasons (the ENet control-channel timeout for relayed streams, and the 150 Mbps bitrate latch), plus how to re-vendor. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
brolink-stream declared serde but serialises nothing; its only JSON lives in brolink-core. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The thread that owns the cpal stream woke every 50 ms to look at a stop flag for the whole of every session, and kept the device open up to 50 ms after the player was dropped. It now blocks on a channel whose sender the player holds, so it wakes exactly once, at the end. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The lobby asked egui for a new frame every 400 ms, and the unified app's service poller asked for another after every one-second poll, whether or not anything had changed. Workers already repaint when discovery, a connection attempt or an update moves, so the lobby's own timer only has to age notices: it is now one second, and the poller repaints only when the status, error, paired clients or gamepad state it read differ from before. Measured on this Mac with the release viewer (brolink-client, lobby open and idle, 60 s after a 12 s warm-up): 0.58 s of CPU at 400 ms, 0.39 s at 1 s, from about 1.0% to 0.65% of a core. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ROADMAP still listed clipboard sync under Later; it shipped in 3.1, and 3.1 to 4.0 had no section at all. README called the stream "BroLink's stream protocol" (it is GameStream, through moonlight-common-c and Sunshine), left out deploy/native, and now states the control API's account rule including tagged servers. MACOS.md still described a Windows-only machine list; WINDOWS.md's route table lacked /v1/display. CONTRIBUTING explains BROLINK_DATA_DIR, where the vendored library is described, and the stream_real variables for streaming from another machine. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
install-macos.sh and install-macos-release.sh replaced /Applications/BroLink.app but left the launchd job running the old binary: replacing a bundle does not stop a process, and the agent's KeepAlive restarts only after a failure. The Mac's sharing service then answered on the previous version until the next login, and every manual install needed a `launchctl kickstart`. Both scripts now kickstart the agent when launchd has it loaded. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Every decoded frame is checked for being entirely black (the stream's black-picture diagnosis). The check walked the planes byte by byte with a short-circuiting all(), which cannot be vectorised, so a frame that really was black, the case the check exists for, cost the decoder thread 2.5 ms at 3024x1964: 15% of a core at 60 fps, on the latency path. Each row is now reduced to its maximum (and minimum, for chroma), which the compiler vectorises, still stopping at the first row with a visible pixel. Release build, 3024x1964 NV12, 200 runs: a black frame 2.47 ms -> 143 µs, a frame with a picture near the top 8.1 µs -> 0.34 µs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The unified window renders the 64-pixel logo for the client's header, again for the host panel's, and on Windows and Linux once more for the window icon, all on the UI thread before the first frame. Each render is a Lanczos resize of the 1024-pixel logo, about 6.6 ms in a release build. Rendered sizes are now kept, so every render after the first of a size costs a 16 KB copy (3.5 µs). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
bundle-macos.sh fell back to the viewer-only brolink-client binary when no brolink-host was built, which produced a BroLink.app that could not share its machine. Nothing builds the bundle that way; it now asks for the host build instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…etup - The 100.64.0.0/10 test lived in core's wake.rs and again in the host service, with the IPv6 variant only in the service. Both are now tailscale::is_tailnet and is_tailnet_ip. - wake.rs and virtual_display.rs each carried the same powershell() runner; setup.rs, with the other PowerShell helpers, has the one. - brolink-host and the development viewer each built their own window options, and only the viewer's explained the vsync-off choice they share. brolink_client::native_options() is used by both, and the host no longer depends on egui and egui-wgpu directly. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Since 4.0 the unified window runs on every OS, and with it update::spawn. The updater assumes a Mac throughout: it swaps an app bundle and pulls brolink-host.exe out of the Windows zip with /usr/bin/unzip. On a Windows PC with another Windows PC in the tailnet it would fetch the release and then fail to push, again every three minutes, racing the Mac that does the same job. Off macOS spawn() now only says where updates come from. Also corrects the module docs that still called the repository private. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The rebinding guard allowed loopback and Tailscale IPv4 in the Host header while the peer check itself admits Tailscale IPv6 too, so a client reaching the service over IPv6 was told browser access is disabled. Both now use tailscale::is_tailnet_ip. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The handover server polls a non-blocking listener so it can notice a stop. On macOS a socket accepted from a non-blocking listener inherits O_NONBLOCK, so the timeouts set on it did nothing: a read before the PC's request arrived failed at once, and writing the ~20 MB executable failed with WouldBlock as soon as the send buffer filled. The PC then got a truncated file, the script's SHA-256 check refused it, and the install through the stream could not complete from a Mac. Accepted connections are now switched back to blocking. The new test writes 16 MB through a connection accepted this way; it fails without the change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
setup::run starts `brolink-host --setup-elevated --local-app-data <dir>` through `Start-Process -ArgumentList @(...)`. Start-Process joins that array with spaces and quotes nothing (as its documentation says), so for a Windows user whose profile folder contains a space the elevated process got `--local-app-data C:\Users\Ada` plus a stray `Lovelace\...` argument, which clap rejects, and setup failed. The folder now carries its own double quotes, without a trailing backslash that would escape the closing one. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
tailscale netcheck's preferred DERP region moves between runs on the same network (this Mac's service.log: Paris, Chicago, Paris within an hour), and every move logged a full network line. The window keeps the last 80 lines, so the flips pushed out what mattered. The line is now logged when UDP, the address families, the NAT's hardness or port mapping change; the report itself still carries the current region. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The control service caches a whois answer for a minute and logged the peer on every refresh. A machine polling /v1/status every few seconds therefore wrote "hermes (…) asked" once a minute for as long as it was up: 2,707 of 2,810 lines in this Mac's service.log, and the whole of the 80 lines the window shows. Answers are now remembered for an hour, and a peer is logged when it is first seen, when its answer changes, or after an hour without asking. The one-minute trust in an answer is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
When the Mac's default output changes during a stream, cpal 0.18's CoreAudio backend reroutes the default-device stream itself and reports ErrorKind::DeviceChanged through the error callback. BroLink logged that as a warning (28 of them in this Mac's panel.log) and kept the old device's name in Output::Playing, so the stats said "audio → WH-1000XM4" while the sound played on AirPods. The callback now takes the new default's name for a DeviceChanged and still warns about anything else. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This Mac's panel.log has "host update attempt 1/3: connection closed" followed by "host update for Hermes: this PC already runs BroLink Host 4.0.2": the first POST landed, Hermes answered and handed over to the new executable, the reply was lost with the old process, and the retry got 409 NotNewer, which the lobby showed as "Could not update Hermes". After a dropped connection or on a retry, push_host now asks the host's /v1/status (for up to 15 s, while it restarts) and treats a version at or past the release as success. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A failed whois is not cached, so a peer polling every three seconds logged the same failure twenty times a minute and emptied the window's 80-line log. The same failure is now logged once until another one takes its place. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The black-picture report runs PowerShell with WMI queries and a screen sample on the PC, then asks the engine's web API for its config and log (curl, up to 8 s each). The Mac gave the whole thing 25 s and this Mac's panel.log has three "display report: timed out", after which the notice never says why the picture is black. The report now gets 60 s; it is asked for once per stream, off the UI thread. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
stream_real now ends the app it launched (not one it resumed) before it asserts, so a live run does not leave a Desktop session open on the PC. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Since 4.0 a Windows or Linux machine is as likely as a Mac to be the one asking, but the service still logged "the Mac sent the new BroLink Host", "sleep requested from the Mac", "the Mac turned the HDR desktop off" and "a Mac can only reach this PC through a relay". Those lines are now OS-neutral, and a power, update or display change names the tailnet address that asked for it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The discovery thread asked for a frame after every three-second scan, and the netcheck and peer-relay probes asked for one after each of their runs, whether or not anything had changed. The probes now only fill their slots; the scan that picks their results up redraws the window when the machines, their status, the relays or this machine's network differ from the last scan. The time of the scan alone does not count. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.
An engineering pass over every crate, the scripts, the deploy kits and CI: security and correctness bugs fixed with regression tests, measured performance work, dead code and dependencies removed, the vendored moonlight-common-c trimmed and documented, and stale docs brought up to date. No version bump, no licence change, no change to the control API's JSON,
client.toml,host.tomlor the update protocol (a 4.0.2 node can update to this).Security
whoiscould name was admitted, including machines shared in from other tailnets: they could pair, sleep/restart the PC, read/write its clipboard and POST/v1/updatewith any executable. Now: same user id, or for a tagged node (the relay VPS) the members of its own tailnet (CurrentTailnet.Name). Tested against the shapes of the real tailnet (owner,tagged-devicesrelay, shared-in user).$expanded,"broke the script). New CI step dumps the generated setup script for every plan shape (with hostile names) and parses it with Windows PowerShell's parser, plusinstall-host.ps1andtake-over-engine.ps1.Start-Process -ArgumentListdoes not quote array elements, so--local-app-dataarrived split and setup failed.~/Library/Application Support/dev.brolink.BroLink(BROLINK_DATA_DIR, set forcargo test/cargo runin.cargo/config.toml).Bugs
WaitForAsynchronousFramesguarantees it cannot outlive the frame.unzip, bundle swap).brolink-host.execlaimed an MIT licence; update downloads tried only the first resolved address; IPv6 tailnet Host headers were refused; brand.rs read UTF-16 from unaligned pointers.launchctl kickstartthe service after replacing the bundle.service.log; DERP-region flips and whois failures also flooded it. Service log lines are OS-neutral and name the peer that asked.Performance (measured)
/v1/statuson Mac/Linux: 3.4 ms → 0.65 µs (noscutil/hostnamespawn per request).schtasks /Createevery 30 s on a PC without audio; audio output thread no longer polls every 50 ms.Cleanup
VERSIONrecords upstream commits (+ ENet/nanors submodules), what was trimmed, and both local patches (ENet control timeout 15/30 s; 150 Mbps bitrate latch).windows_peers,overlay_or_lan,lan_like_stream,Settings::remote, unused FFI declarations,path::effective's ignored argument), unused deps (streamserde, hostegui/egui-wgpu), duplicate helpers (tailnet check, PowerShell runner, window options), a duplicate env hook.Verification
cargo fmt --check,cargo clippy --workspace --all-targets -D warnings,cargo test --workspacegreen on macOS; CI green on Windows/macOS/Linux.🤖 Generated with Claude Code