fix: answer requests the state machine was about to drop - #450
Merged
Conversation
The waker carries a single bit of information: the main loop should stop waiting on the current transition and step the machine. The loop treats it that way already, discharging it with `try_recv` on every iteration, but the handlers filled it with a blocking `send().await` on a one-slot channel. That made a full slot mean "wait until the loop drains me", which the loop cannot do while a handler is running. Two accepted requests reaching a handler between two transitions were enough to park the agent for good: the first took the slot, the second waited on a loop that would not run again until the handler it was blocking returned. Sending without waiting says what was meant. An occupied slot already carries the message, so there is nothing to wait for.
While a state waits on its transition, the loop raced the sleep and the waker against a
future that both received a request and handled it. Whenever the sleep or the wake won,
`select` dropped that future wherever it happened to be, and a request suspended on a slow
server went down with it: its reply channel was dropped unsent, and the caller was left
holding a receiver nobody would ever send on.
Racing the receive alone settles it. Dropping a pending receive loses nothing, so the
request is only taken off the channel once it is going to be answered, and the handling
runs where nothing can cancel it. The wait itself now outlives the requests answered
against it, so a query no longer restarts a poll interval, and a request that redirects the
machine ends the wait it was made against instead of relying on a wake up to do it.
Two smaller repairs come along, both of which cost a request its answer:
- An accepted request kept its state change even when the reply could not be delivered.
Tolerating a client that hung up is right; throwing away the local install it just
asked for is not.
- `Addr` treated a closed reply channel as unreachable and panicked the caller's task. A
dead state machine is a runtime condition, and `TransitionError::CommunicationFailed`
already describes it. The remaining `unreachable!` covers a mismatched response variant,
which really cannot happen.
The tests hold a probe at the server and wake the machine while the answer is outstanding,
which is what a poll timer firing mid-request looks like in production.
`Download` and `DirectDownload` each hand-rolled the shape the main loop just had fixed: race the download against a future that receives a request and handles it, and let `select` drop whichever loses. A handler suspended when the download finished went the same way its counterpart in the loop did, taking the caller's reply with it. Neither state can reach that today. Both are non-preemptive, so every handler they see answers without suspending, and the drop has nothing to interrupt. That makes this a repair of the shape rather than of an observed failure, which is also why one preemptive state away it would have become a live bug in a place nobody was looking. `handle_communication_while` now owns the race for all three sites. It goes a step further than the loop: the download keeps being polled while a request is answered, so a query no longer stalls a transfer, and when the download wins the race `select` hands back the unfinished handler to be driven to completion rather than dropped. A request that redirects the machine outranks work that is no longer wanted, which matches the reply the client was already given. Both states shrink to building their work future and handing it over, and `start_download` takes the update package by reference, dropping two of its three clones.
Every `request_*` method opened its own reply channel, sent the message, awaited the answer, and then matched four arms to unwrap it. Five copies, which is how the response matching had drifted apart in the first place, and it meant each new request type had to remember the closed-channel arm or reintroduce a panic. A `From<RecvError>` impl beside the existing `From<SendError>` one lets `?` carry the closed channel, and a private `request` holds the plumbing. What is left in each method is the part that actually differs: the message it sends and the response variant it expects.
otavio
force-pushed
the
fix/state-machine-request-handling
branch
from
September 10, 2026 14:48
24cc9c7 to
94e3bff
Compare
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
Four fixes to how the state machine takes requests off its channel and answers them. Three of the four cost a caller its reply — it is left holding a receiver nobody will ever send on — and one could park the agent outright.
The commits
fix: signal the waker without blocking— the waker carries one bit: stop waiting on the current transition and step the machine. The loop already treats it that way, discharging it withtry_recveach iteration, but handlers filled it with a blockingsend().awaiton a one-slot channel. A full slot therefore meant "wait until the loop drains me", which the loop cannot do while a handler is running. Two accepted requests arriving between two transitions were enough to park the agent for good.fix: answer requests that are still being handled when a wait ends— while a state waited on its transition, the loop raced the sleep and the waker against a future that both received a request and handled it. When the sleep or the wake won,selectdropped that future wherever it stood, and a request suspended on a slow server went down with it. Racing the receive alone settles it: dropping a pending receive loses nothing, so a request only leaves the channel once it is going to be answered, and the handling runs where nothing can cancel it.Two smaller repairs ride along, both of which also cost a request its answer: an accepted request kept its state change even when the reply could not be delivered (tolerating a client that hung up is right, discarding the local install it just asked for is not), and
Addrtreated a closed reply channel asunreachable!and panicked the caller's task, whereTransitionError::CommunicationFailedalready describes that runtime condition.fix: share one request-handling race across the download states—DownloadandDirectDownloadeach hand-rolled the shape the loop had just had fixed, with the same drop. Neither can reach it today: both are non-preemptive, so every handler they see answers without suspending. This is a repair of the shape rather than of an observed failure — which is also why one preemptive state away it would have become a live bug somewhere nobody was looking.handle_communication_whilenow owns the race for all three sites, and goes further than the loop did: the download keeps being polled while a request is answered, so a query no longer stalls a transfer, and when the download wins the race the unfinished handler is driven to completion rather than dropped.refactor: collapse the repeated request plumbing in Addr— five copies of "open a reply channel, send, await, match four arms to unwrap", which is how the response matching had drifted apart to begin with. AFrom<RecvError>impl beside the existingFrom<SendError>lets?carry the closed channel, and a privaterequestholds the plumbing; each method keeps only the message it sends and the response variant it expects.Tests
The new tests in
states/machine/tests.rshold a probe at the server and wake the machine while an answer is outstanding, which is what a poll timer firing mid-request looks like in production.