Repository navigation
fix(input): release host capture on pump abort and cap blocksd replies - #374
Merged
Merged
Conversation
The pump released host screen, audio, and interaction capture only after its worker loop exited normally. When shutdown passed its one second deadline and aborted the worker, or the worker panicked, that release never ran and capture sessions stayed open with no pump left to close them. A HostCaptureDemand guard now owns the capture demand state for the worker's lifetime and reconciles every domain to inactive on drop. It is declared before the DeviceInputDemand guard, so an unwinding or aborted worker withdraws device input before releasing host capture, the same order as the clean exit. Abort is asynchronous, so the release runs when the runtime drops the cancelled task, which can be after shutdown has already returned its deadline error. A new test aborts the worker while screen and interaction capture are active and checks both are released, with a probe source confirming device input was withdrawn first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The request connection read blocksd replies with an uncapped read_line, bounded only by the two second request timeout, so a peer that sent bytes without a newline could grow the reply buffer without limit inside that window. The hand framing behind the event stream's 64 KiB cap now lives in a shared read_line_capped helper that both connections use. It buffers at most a given number of bytes of a line and discards a longer one through its newline, leaving the reader framed at the next line. Replies cap at 1 MiB. The largest is discover_response at about 230 bytes per block; a full topology (blocksd counts blocks in seven bits) runs about 28 KiB, and about 215 KiB with every name and version string at its 127-character limit and fully escaped. Losing a discover reply hides every block, so the cap covers several worst-case topologies. An oversized reply fails its request with an error naming the cap. The connection stays framed, so the next request reads its own reply, and the backend still reconnects after any failed request. Framing unit tests cover reassembly across refills, the exact cap boundary, an oversized line, and an unterminated tail. A connection test pads a pong past the cap, checks the request fails, and checks the next ping on the same connection succeeds. A backend test pads a key frame acknowledgement past the cap and checks the write fails and the backend reconnects. Spec 30 records both caps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 10 billable files and costs up to $2.50. Or wait 59 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (10)
Comment |
Bring in main now that the device input base this branch stacked on has landed, so the branch can merge on its own. The merge was clean. Co-Authored-By: Claude Opus 5.5 <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.
What this changes
Two small robustness gaps that turned up while hardening #323, neither of them new:
HostCaptureDemandguard now does the same for host capture, declared so it drops afterDeviceInputDemand, matching the clean path's order.read_line, bounded only by its timeout. feat(input): route ROLI touch and button input from blocksd #323's 64 KiB event-stream framing moves into a sharedblocks/framing.rshelper (read_line_capped), and the request connection now uses it with a 1 MiB cap. An oversized reply fails its request with a clear error and is discarded through its newline, so the next request reads its own reply. The backend still reconnects after any failed request, as before.Why 1 MiB
The largest blocksd reply is
discover_response, at about 230 bytes per block. blocksd counts a topology's blocks in 7 bits (at most 127), so a full topology is about 28 KiB. With every name and version string at its 127-character limit and fully escaped, it's about 215 KiB. A lost discover reply hides every block, so the cap leaves a wide margin. Spec 30 §4.3 and its failure table record both caps.Verification
just verifypasses locally (Rust fmt + lint + test)just denypasses (required for dependency or license changes)just ui-testandjust ui-buildpass (required forcrates/hypercolor-ui/)just sdk-lint,just sdk-check, andjust sdk-buildpass (required forsdk/)just python-verifypasses (required forpython/)just compat-checkpasses (required fordata/drivers/vendors/*.toml)just docs-buildpasses (required for docs or README changes)cd docs && zola checkpasses (required for docs link/content changes)scripts/orpackaging/)just e2e-buildpasses with the normal Servo stack (required for daemon/UI/effect integration changes)just e2e-build-cpupasses when validating the CPU smoke fallbackjust e2epasses against the Servo stack (required for end-to-end behavior changes; starts daemon/browser)input_publicationtests, serializeddevice::blocksunit tests,blocks_backend_tests,blocks_input_tests-D warningson core and daemon, all targets;cargo fmt --check;cargo check --workspace --all-targetsThe new tests are
aborted_pump_worker_releases_host_capture_demand,an_oversized_reply_fails_its_request_and_the_next_request_resyncs,blocks_backend_fails_an_oversized_reply_and_reconnects, and three framing unit tests. An independent verifier passed both commits. It mutation-checked the guard test twice: without the guard, screen capture staysActive, and with the two guards swapped, the probe sees device input still demanded at release. It also confirmed the extracted framing reproduces #323's event-stream behavior line for line.Notes for reviewers
The guard runs the capture setters from
Drop. If the worker panicked inside one of those setters and the release panicked again during unwinding, the process would abort. That can't be made safe from a destructor, because any panic during unwinding aborts. The setters returnResult, so this needs a setter that panics on deactivate.🤖 Generated with Claude Code