Bound the server's answers, never the job - #100
Merged
Conversation
`wait_for_completion` had no deadline, no maximum number of polls and no per-request timeout, so a server that accepted the connection and then said nothing kept the client waiting for as long as the process lived. Measured against v0.7.0 in the issue: still polling after 60 s, nothing printed, nothing written, killed by the harness. The limit added here is on the **server's responsiveness**, and deliberately not on the job. Compression is allowed to take as long as it takes; what is not allowed is silence on an open socket. There is no total deadline on the poll loop, and `a_long_job_is_never_cut_short_while_the_server_keeps_answering` fails if one is ever added: the mistake is easy to make and would cut off exactly the users who need the server most. `Timeouts` carries connect (10 s), read and write (30 s each) on one shared agent. The values are generous on purpose. Hitting one has to be unambiguous evidence that something is wrong rather than evidence that we were impatient, because the only thing the user gets is the message. Two properties were verified against ureq rather than assumed, and both matter: - `timeout_read` and `timeout_write` are **per socket operation**, not per response. A body dribbled out over 2.7 s in fast chunks passes a 1 s read timeout untouched, so a large upload or download that keeps moving never trips one. A test pins it. - ureq separates "no socket could be opened" (`ConnectionFailed` for both a refused connection and a connect timeout, `Dns` for a name that will not resolve) from "the socket was open and the exchange broke" (`Io`). That separation becomes `RemoteError::Unresponsive`, kept apart from `Unreachable` because the two want different things from whoever reads them: a wrong address or a server that is down, against a server that is up and stuck. Its message says the job may still be running on the far side and that the archive was not downloaded, which is the true and useful thing to say. `Timeouts` is injectable through `compress_path_with` and `check_health_with`, following `collapse_core::extract_with`: the defaults suit every front-end, and a suite cannot wait 30 seconds to prove what happens after 30 seconds of silence. Both existing entry points keep their signatures, so the CLI and the desktop are untouched. Four tests, each checked against a broken implementation: dropping the timeouts makes the silent-server case take 5 s and fail, reporting a quiet socket as merely unreachable fails the classification case, and a total deadline on the loop fails the long-job case. Closes #71
`a_body_delivered_in_slow_pieces_is_not_mistaken_for_silence` failed on the macOS leg. Not the code: the test. It trickled a body in 60 ms pieces against a 200 ms read timeout, and `thread::sleep` promises a floor rather than a ceiling, so a shared runner turning one 60 ms gap into 250 ms was enough. A margin that looked generous on a quiet laptop was 3x. The same mistake was latent in two more cases, which passed only because they got lucky: they used one short timeout for everything, including the cases where the stub has to answer *inside* it. So there are two helpers now, and the split is the point. `firing_timeouts` keeps the short read for the cases where the timeout firing is the thing being proved. `patient_timeouts` gives a second, at least a 25x margin over every nominal gap, for the cases where the point is that nothing fires. The long-job case grows from eight `compressing` answers to twelve, so it still outlives the timeout it is measured against: 1710 ms of waiting against 1000 ms. The trickle case sends 30 pieces 40 ms apart, ~1200 ms in total, which is past the read timeout, so a per-response limit would still fire where a per-read one does not. The suite goes from 0.95 s to 1.77 s. Worth it: six consecutive local runs are identical, and all three mutations are still caught (no timeouts, a quiet socket misreported as unreachable, and a total deadline on the loop). This is real-socket behaviour, so unlike the poll schedule it cannot be moved onto a virtual clock. Margin is the only defence available.
Merged
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.
Fixes #71, on top of #99.
The poll loop had no deadline, no maximum number of polls and no per-request timeout. A server that accepted the connection and then said nothing kept the client waiting for as long as the process lived: the issue measured it still polling after 60 s, having printed nothing and written nothing, killed by the harness.
The distinction this is built around
The limit is on the server's answers. It is not on the job.
Compression is allowed to run for as long as it takes. What is not allowed is silence on an open socket. So there is deliberately no total deadline on the poll loop, and a test fails if one is ever added:
That mistake is an easy one to make while "fixing #71", and it would cut off exactly the users who need a remote server most.
Values
connectreadwriteGenerous on purpose. Hitting one has to be unambiguous evidence that something is wrong, not evidence that we were impatient, because the message is the only thing the user gets.
Two things verified against ureq rather than assumed
timeout_readis per socket operation, not per response. This is the one that would have quietly broken large transfers if I had guessed wrong. Probed directly:So a 500 MB archive on a slow link is fine as long as bytes keep moving.
a_body_delivered_in_slow_pieces_is_not_mistaken_for_silencepins it.ureq already separates the two failures worth telling apart:
That becomes
RemoteError::Unresponsive, kept apart fromUnreachable: a wrong address or a server that is down is a different diagnosis from a server that is up and stuck. Its message says the job may still be running on the far side and that the archive was not downloaded, which is the true and useful thing to say.Shape
Timeoutsis injectable throughcompress_path_withandcheck_health_with, followingcollapse_core::extract_with. The defaults suit every front-end, and a suite cannot wait 30 seconds to prove what happens after 30 seconds of silence. Both existing entry points keep their signatures, so the CLI and the desktop are untouched.Tests
Four new, 19 in the file, running in 0.95 s. Each was checked against a broken implementation:
Unreachablea_long_job_is_never_cut_short_while_the_server_keeps_answeringdrives a stub that answers every poll at once but reportscompressingeight times, so the client legitimately waits 910 ms against a 200 ms read timeout.Counts: 600 Rust and 113 Vitest, 485 offline.