Conversation
Issue #48 names two targets and only one was fixed in v0.9.0. Its body says `apps/server-frontend/src/api.js` "has its own polling loop with the same shape and deserves the same treatment", and it did: a flat `POLL_INTERVAL` of 400 ms before the second question, so a job the server had already finished cost the browser that much. Worse, after the Rust half landed the browser was waiting twice as long as the CLI for the same job. Same schedule, deliberately: 10 ms doubling to a 200 ms ceiling, mirroring `apps/remote/src/waiting.rs`. `keeps_the_same_schedule_the_rust_client_uses` fails if the two ever drift, which is the failure mode that made this half worth doing rather than leaving as a nicety. Three tests. The first two spy on `setTimeout` to record what the loop asks for and fire it at once, so a schedule spanning seconds of nominal waiting is checked instantly and with no wall clock in the assertions, which is the same property the Rust side gets from its injected `Sleeper`. All three mutations are caught: starting at the ceiling, never growing, and drifting from the Rust ceiling.
Back off in the web client too, on the same schedule as Rust
An archive holding an entry named after the archive, extracted into the archive's own directory, overwrote itself. All three formats; two of the three said "Extracted 1 file(s)" and exited 0 while doing it. [zip] 132 -> 12 bytes [tar] 2048 -> 12 bytes [7z] 115 -> 12 bytes The entry is written onto the file still being read, so the archive is truncated mid-read and what replaces it is whatever fraction the extractor had reached: the contents are lost from the output as much as from disk. What made this indefensible rather than merely unfortunate is that compression has always refused the mirror image, and refuses it even with `--force` (`OutputIsSource`, and an output inside the folder being archived). The same product held two opposite positions on the same question. The planning pass already reads the whole listing before a byte is written, so the check costs one comparison per entry and no extra pass over the file. Two details it would be easy to get wrong, both pinned by tests: - it compares **file identity**, not paths. A hardlink is a second name for one file and never resolves to the same string, which is exactly how `--force` was once able to overwrite its own source on the compression side. Verified against a real hardlink. - it follows the **planned** name, not the archive's spelling, since a rename can land an entry on the archive that the archive's own name does not match. Refused outright rather than made overridable. Nobody agrees to this by asking to extract something, so no flag should unlock it. `WouldOverwriteArchive` is a variant of its own rather than a `Failed` string: the two callers that match on `CompressionError` both have catch-all arms, so nothing breaks, and a caller that wants to offer "extract somewhere else" now has something to match on. Four tests, and all four mutations are caught: removing the guard, comparing paths instead of identity, ignoring the plan, and refusing too much. The last one matters most, since "refuse anything that resembles the archive" would pass the other three and break ordinary extractions. Closes #96
…archive Refuse an entry that would be written over the archive being read
`GET /jobs/{id}` returns a failed job's `error_message`, and that message came
from the engine, which names the file it was working on. For a job that file
is inside the staging directory, so an ordinary upload learned the server's
storage layout:
Compression failed: failed to unpack `/var/lib/collapse/jobs/<uuid>/tree/…`
Reachable with a tar whose second entry has the first, a plain file, for a
parent. The server has no authentication (issue #72, untouched here), so "a
client" is anyone who can reach the port.
Two things were wrong and only one was obvious. The obvious one: unpacking a
tar envelope did not go through `failure_message` at all. The other: the
curation it skipped would not have helped much, because it rewrote one variant
and passed the rest through, on the stated reasoning that they "already read
as a sentence about something the client did".
So the rule is inverted rather than extended. Redact unless there is a curated
sentence. Enumerating the leaky variants would have been the smaller change
and the wrong one: `CompressionError` gains variants, and the next one would
have leaked until somebody noticed. `From<String> for Failure` redacts too, so
a new error path is safe by default rather than safe only if someone remembers.
Redaction is blunt on purpose. The server has no reason to tell a client where
anything lives, so removing every absolute path is correct rather than merely
convenient, and it does not depend on knowing which variant produced the
message or where the staging directory is mounted. Relative paths survive:
those are the client's own entry names.
`Failure { client, log }` splits the two audiences. An operator wants the whole
truth including the path; a client must not have it. Verified end to end
against a running server:
client: Compression failed: failed to unpack `<path>`
log: Compression failed: failed to unpack `/private/tmp/.../tree/…`
One existing test asserted the old policy by name,
`other_engine_errors_are_passed_through_word_for_word`. Its assertions still
passed, since none of its fixtures held a path, but its name and comment
claimed something no longer true. Rewritten rather than left to mislead.
Both mutations are caught: making the redaction a no-op, and letting
`extract_tar` bypass the curation again.
Closes #66
Stop telling clients where this machine keeps things
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.
Completes v0.9.0.
mainalready carries the version bump from the earlier merge; this brings the three PRs that landed after it. Nothing has been tagged or published yet, so 0.9.0 is still free to grow, and it did.Read this first if you are upgrading
Two behaviour changes.
An archive whose listing cannot be read is 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 partial extraction was not a feature: it was the naming layer switching itself off, so every entry written before the fault went to disk unrewritten and unrefused, which on Windows turned
notes.txt:hiddeninto an invisible NTFS stream without the user being asked (#89). Salvaging is still possible through the backends, which take no options.An entry that would be written over the archive being read is refused (#96). All three formats used to overwrite the archive and report success:
Compression has always refused the mirror image, and refuses it even with
--force. Extraction had no equivalent, so the product held two opposite positions on the same question. The check is by file identity, so a hardlink cannot slip past it.No longer leaking, no longer hanging
#66.
GET /jobs/{id}handed a failed job'serror_messagestraight from the engine, which named the file it was working on: for a job, a path inside the staging directory. Reachable with an ordinary upload, and the server has no authentication, so that was anyone who could reach the port. Failures now have two halves: the client's, with every absolute path removed, and the log's, whole.#71. A server that accepted the connection and then said nothing kept the client waiting for as long as the process lived. 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, and a test fails if a total deadline is ever added.
#48. Both poll loops. The Rust client goes from a flat 200 ms to 10 ms doubling to 200 ms: a five byte file measured 236 ms before and 44 ms after, while a 60 MB job taking ~8.3 s is unchanged. The web client had the same shape at 400 ms and now shares the Rust schedule, with a test that fails if the two ever drift.
New public surface
collapse-remotegainsTimeouts,compress_path_with,check_health_withand thewaitingmodule.collapse-coregainsCompressionError::WouldOverwriteArchive. Existing entry points keep their signatures, so the CLI and the desktop are untouched.Error message text changed for remote and server failures, so anything matching on those strings needs updating. The connect timeout is 10 s where ureq's default was 30 s, which is a tightening rather than an addition.
What closes, and what does not
The keywords are in the commit messages, so #48, #66, #89 and #96 close when this lands. Each was checked against the code rather than against a PR body, after an audit of the first attempt at this release caught two keywords that would have closed live bugs.
#66 closes with a successor. Its remaining half, giving
CompressionErrorstructure so callers can tell causes apart, is now #107.#71 does not close and is not meant to. Only its socket half shipped. A server that answers
compressingforever still keeps the client waiting, deliberately, because the limit belongs on the answers and not on the job. What remains there is cancellation.Verification
614 Rust tests and 116 Vitest, on macOS, Linux and Windows.