Repository navigation
fix(blocks): report frames blocksd declines as declined, not delivered - #423
Draft
hyperb1iss wants to merge 4 commits into
Draft
hyperb1iss wants to merge 4 commits into
hyperb1iss wants to merge 4 commits into
Conversation
A backend could report a frame as sent, suppressed, or failed, but not as declined: the transport delivered it and the device refused it. A backend in that position had to call the frame delivered, which counts an undelivered frame as sent, or call it failed, which feeds the lifecycle reconnect path for a session that is fine. DeviceWriteOutcome and DeviceDeliveryStatus gain a Declined variant, and DeviceDeliveryAck::declined builds its acknowledgement: transport started, zero completed bytes, no error. The output queue counts it in a new transport_declined counter, outside frames_sent, transport_failed, and errors_total. It never becomes a retained failure, so lifecycle never sees it, and it neither extends nor resets a failure streak. A declined payload still has to reach the device. Dedupe lets an identical follow-up frame go out, replacing a declined payload is not a coalesced drop, and the retained-frame path retries it the way it retries a failed one, at the normal render cadence. Display transports only complete or fail, so a declined display ack fails like a suppressed one. The daemon's device_metrics topic carries transport_declined, defaulted so older snapshots still decode. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
blocksd answers each grid and key frame with an acknowledgement, and the backend turned a negative one into Ok(()), so the output pipeline counted a frame the device never took as delivered. Blocks was the one backend whose acknowledgement said something the device did not do. The write path now returns a typed outcome: Sent for an accepted frame, Declined for a negative acknowledgement, and an error for transport and protocol failures. write_colors_shared_outcome hands that outcome to the output queue, which counts the frame as declined instead of sent. A decline still keeps the connection, as before, so the next frame goes out on the same socket and nothing reconnects; only real failures drop it. Direct writes through write_colors, such as identify flashes, still fold a decline into Ok(()) the way other backends fold suppression, so a single deferred frame does not abort the flash. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
/diagnose listed transport_started, transport_completed, and transport_failed per device but not transport_declined, so a Blocks lane whose frames blocksd declined showed started attempts that never ended in either column. DiagnoseDeviceOutputItem gains transport_declined, filled from the queue counter through the daemon's device metrics. The field defaults, so the OpenAPI schema marks it optional and the regenerated Python client still decodes a daemon that predates it. The field's doc states the accounting as it is: once nothing is in flight, started equals completed plus declined plus the failed attempts that reached the transport. transport_failed also counts attempts a lane rejects before any I/O, which USB, OpenRGB, Govee, and WLED do, so started equals completed plus failed plus declined only where no attempt is rejected early. Blocks is one of those lanes, and its end-to-end test asserts the exact identity. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
blocksd answers each binary frame with 0x01 when it accepts the frame and 0x00 when it declines it, as spec 30 documents and its server writes. The backend read any byte other than 0x01 as a decline, so a stream that had fallen out of step with blocksd looked like a device politely refusing frames and kept the connection. write_frame_binary now maps 0x01 to accepted and 0x00 to declined, and fails on any other byte with an error naming it. That error takes the existing failure path, the same one a malformed key frame acknowledgement takes: the backend drops the connection and reconnects on the next connect. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
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
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.
When blocksd declined a frame, the Blocks backend logged it at debug level and returned success, so the output pipeline counted a frame the device never took as delivered. Blocks was the one backend whose acknowledgement said something untrue, and Spec 85's cut receipts need every backend's acknowledgement to be authoritative. After this PR, a decline is its own outcome: it is never counted as delivered, never counted as a failure, never disconnects the device, and the next frame goes out normally.
💡 What changes
Declinedin its write outcome and delivery status, plusDeviceDeliveryAck::declined(transport started, zero bytes completed, no error). The API had no way to say "the device refused this one frame" before: a write could only be sent, suppressed, or failed, and the only retryable error,Timeout, would have been untrue.write_colors_shared_outcome. On the binary grid path,0x01is accepted and0x00is declined, matching blocksd (blocksd/api/server.py:204) and Spec 30; any other byte is a protocol error that takes the existing reconnect path. The key-frame path already validated a JSON acknowledgement and now maps a refusal the same way.transport_declinedcounter and leavesframes_sent,transport_failed,errors_total,last_error, and every failure streak alone, so device lifecycle never sees it. A frame after a decline goes out even if it is identical, and replacing a declined frame is not counted as a dropped frame.device_metricsWebSocket topic and the/diagnosedevice output item both carrytransport_declined, defaulted so older payloads decode; the OpenAPI schema marks it optional and the regenerated Python model types itint | Unset.🎯 Behavior to know about
In a static scene, the existing retry for failed frames now also re-sends a declined frame, at the normal render cadence (Blocks stays capped at 25 FPS). Before, a declined frame was counted as delivered and never retried. No sleeps, backoff, or throttling were added.
Direct writes used by identify flashes still fold a decline into success, on purpose: one deferred frame should not abort a flash, and Govee and WLED already treat suppression the same way there. Those writes bypass the delivery counters.
For Blocks, once nothing is in flight, started equals completed plus declined plus failed attempts that reached the transport, and the end-to-end test asserts it. The identity does not hold for every backend, because USB, OpenRGB, Govee, and WLED reject some attempts before any I/O, which count as failed but never as started; the field's doc states the scoped version.
🧪 Validation
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)blocks_backend_testsbackend_manager_teststransport_declinedis 1 and sent, failed, and errors are 0; after an accepted frame,frames_sentis 1device_metrics_tests,ws_protocol_tests,openapi_tests,rest_wire_contract_testsjust python-verify,just python-generate-check-D warnings --all-targets(types withschema, core, daemon),cargo check --workspace, rustfmtAn independent review traced every path from a Blocks write to the counters, lifecycle, telemetry, and
/diagnose, and confirmed the protocol mapping against blocksd's source and Spec 30. It found no defect; the socket fixtures and fresh builds it could not run in its sandbox were run separately and pass.🔍 Review focus
output_queue.rsaccounting and the retry predicate. A decline must stay out of every success and failure counter while still making the frame eligible to go out again.Declined. Display output treats it as a protocol failure, the same boundary it already applies to a suppressed acknowledgement; Blocks has no display path.🤖 Generated with Claude Code