Conversation
`wait_for_completion` slept a fixed 200 ms before asking a second time, so a job the server had already finished still cost the caller that much of this function sleeping. Measured with the real binary against a real server, a five byte file: before: 236, 233, 237 ms after: 44, 44, 42 ms The wait now starts at 10 ms and doubles to the old 200 ms ceiling, so the schedule is 10, 20, 40, 80, 160, 200, 200, ... A long job is unaffected by design: it reaches the ceiling after 310 ms and stays there, which the same measurement confirms on a 60 MB incompressible input taking around 8.3 s (8347, 8550, 8562 before against 8271, 8479, 8297 after). The whole price is about three extra requests over the life of a job of any real length. The schedule is a pure function in `protocol` rather than a counter inside the loop, matching the split the rest of that module already has, so it can be checked without a server and without waiting for it. Tests: four in tests/protocol.rs pinning the first wait, the doubling, the ceiling, monotonicity and saturation on a nonsense input; one in tests/client.rs driving a stub that reports `compressing` three times, which takes 627 ms if the fixed interval comes back and around 70 ms as it stands. Closes #48
…ested
The backoff was only reachable through a socket and a real `thread::sleep`, so
the one test covering it drove a stub server and asserted on a wall clock with
a 400 ms margin. That proves the fixed interval has not come back and very
little else: it cannot say what the schedule *is*, and it gets slower and
flakier the more of it you try to cover.
`waiting` now holds the loop behind two one-method traits, `Sleeper` and
`Poller`, with `RealSleeper` and a private `HttpPoller` as the production
pair. Between them a test can state the thing no stub server can state
deterministically: a job that takes exactly N milliseconds. The fake sleeper
accumulates virtual time instead of spending it and the fake job reads that
accumulation to decide whether it has finished.
`wait_for` returns `Waited { polls, slept }`, its own account of what it did.
That is the part that keeps the tests honest: asserting on the return value is
asserting on the subject, while reaching into a fake to count calls is
asserting on the fake, and passes when the fake is wrong.
The schedule moves out of `protocol` with it. That module answers "what does
this JSON mean"; how often to ask is a different question and now has its own
file.
Ten cases in tests/waiting.rs, running in microseconds. All five mutations I
tried are caught by several of them each: starting at the ceiling, never
growing, sleeping before asking, not counting the settling poll, and removing
the ceiling.
Two of them exist to be honest rather than flattering. The backoff is not
uniformly faster: a job finishing just after the ramp is asked again a whole
ceiling later, so at 199 ms the old schedule finished at 200 and this one
finishes at 310. The bands are narrow, the worst case measured across 0 to
2000 ms is 110 ms, and it is bounded by one ceiling. Both the bound and the
gain are pinned.
The stub-server test stays. It is the only thing proving `RealSleeper` and
`HttpPoller` are actually wired in rather than the policy being right in
isolation.
Back off from a short first wait instead of a flat 200 ms
`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.
Bound the server's answers, never the job
`plan_for` swallowed a listing failure and carried on with the identity plan. The comment justifying that was right about the message and wrong about the timing: the extractor fails **while streaming**, so by the time it notices it has written every entry before the fault, and written them with no plan at all. No rewriting, no refusal, no collision check. Reproduced, same two entries, twice: intact listing report Ok(1) extract Err ON DISK [] unreadable report Err extract Err ON DISK [notes.txt:hidden, second.txt] On Windows `notes.txt:hidden` is not a file name, it is the `hidden` alternate data stream of `notes.txt`: the write succeeds, the bytes land where no listing shows them, and the user was told there was nothing to answer for. Issue #63's harm, performed without consent, arranged by appending one bad 512 byte header to a tar. The message half of the old argument turned out to be the weak half. Both passes go through the same parser, so they say close to the same thing; in the reproduction they differed only in where they were cut off. What this costs is partial recovery: a truncated tar used to hand back whatever preceded the damage and now hands back nothing. Deliberate, and not actually gone, since the backends take no options and never come through the planning pass. `recovering_from_a_damaged_archive_is_still_possible_through_ the_backend` pins that so the capability does not look deleted. Checked before committing to it that the listing parser is not stricter than the extractor for archives that are merely unusual: a tar with no end-of-archive marker and a zip with junk appended both list fine, so this does not start refusing good archives. Also makes the refusal readable, which is not scope creep but the consequence of the change: it is now the whole of what the user gets for a damaged archive, and the tar crate embeds the bytes it choked on. before: Compression failed: numeric field did not have utf-8 text: <4 bad bytes> when getting cksum for <100 more> after: Compression failed: this archive could not be read, so nothing was extracted: numeric field did not have utf-8 text: when getting cksum for Narrower than issue #66, which is about callers telling causes apart. This is only about what is fit to show a person. One desktop assertion moves with it, since the 7z message is now prefixed with what happened before why. Closes #89
Stop extracting an archive whose listing cannot be read
…landed All eight version strings and the four lockfiles. The release guard checks two of them (issue #77), so the rest are by hand and by eye. Minor rather than patch because v0.9.0 changes observable behaviour: an archive whose listing cannot be read is now refused outright instead of extracting whatever preceded the damage. It also adds public surface to collapse-remote (`Timeouts`, `compress_path_with`, `check_health_with`, and the `waiting` module). Not a bump: `bad.zip` is removed. It is 24 bytes of junk from a shell check whose `cd` failed, so the file landed in the repository root instead of the scratch directory, and `git add -A` swept it into #101. A release branch should be a bump and nothing else, so this is called out rather than hidden: it is a deletion of something that was never meant to exist.
Release 0.9.0
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.
Everything on
devsince v0.8.0: three PRs in the remote and extraction paths, plus the version bump. Version strings and lockfiles are already at 0.9.0, which is what the release guard checks before it will publish.Read this first if you are upgrading
One behaviour change. An archive whose listing cannot be read is now refused outright, where it used to extract whatever preceded the damage and then fail. A truncated tar that handed back three of its five files now hands back none.
That was deliberate. The partial extraction was not a feature, it was the naming layer switching itself off: every entry written before the fault went to disk with no rewriting, no refusal and no collision check, which on Windows turned
notes.txt:hiddeninto an invisible NTFS stream without the user ever being asked (#89, and #63's harm without consent).Salvaging is still possible and is not gone: the backends (
extract_tarand friends) take no options and never go through the planning pass. It is simply no longer whatextractdoes by default.Faster, and no longer able to hang
#48. The poll loop slept a flat 200 ms before asking a second time, so a job the server had already finished still cost the caller that much. Measured with the real binary against a real server:
Not uniformly faster, and that is pinned rather than glossed: a job finishing just after the ramp is asked again a whole ceiling later, worst case 110 ms, bounded by one interval.
#71. No deadline, no maximum polls, 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: still polling after 60 s, nothing printed, nothing written. Now bounded by connect 10 s and read/write 30 s.
The limit is on the server's answers, never on the job. Compression may run for hours; what is not allowed is silence on an open socket. A test fails if a total deadline is ever added, because that mistake would cut off exactly the users who need a remote server most.
Unresponsiveis a new error kept apart fromUnreachable, because "the address is wrong or the server is down" and "the server is up and stuck" want different things from whoever reads them. Its message says the job may still be running on the far side.Two smaller changes worth knowing
Error message text changed for remote failures, so anything matching on those strings will need updating. And the connect timeout is now 10 s where ureq's default was 30 s, which is a tightening rather than a pure addition.
New public surface
collapse-remotegainsTimeouts,compress_path_with,check_health_withand thewaitingmodule. The existing entry points keep their signatures, so the CLI and the desktop are untouched.Also
bad.zip, 24 bytes of junk accidentally committed in #101, is removed.Verification
604 Rust tests and 113 Vitest, on macOS, Linux and Windows. Two ureq behaviours were verified against the crate rather than assumed, one of which would have broken large transfers if guessed wrong.
What this does and does not close
An audit of this release caught two closing keywords that would have closed live bugs. Only one issue is genuinely finished.
Closes #89
#71 stays open, deliberately. Its title is "the poll loop has no upper bound, so a stuck job hangs the caller forever" and its primary reproduction is a server that answers
compressingon every poll. That case is still unbounded, and it is unbounded by decision: the limit belongs on the server's answers, not on the job, because a genuine compression may run for hours. What landed is the other half, a server that goes silent on an open socket. The source says so itself atapps/remote/src/waiting.rs:99, and a live stub confirms it: the v0.9.0 client polled such a server until a 45 s guard killed it.#48 stays open too. Its body names a second target:
apps/server-frontend/src/api.js"has its own polling loop with the same shape and deserves the same treatment". Only the Rust client was fixed; the web app still sleeps a flatPOLL_INTERVAL = 400.Both will be narrowed to what actually remains rather than left implying nothing happened.