Skip to content

Make local install failures actionable - #543

Open
sdairs wants to merge 2 commits into
stack/463-parse-local-versionsfrom
stack/462-actionable-install-failures
Open

Make local install failures actionable#543
sdairs wants to merge 2 commits into
stack/463-parse-local-versionsfrom
stack/462-actionable-install-failures

Conversation

@sdairs

@sdairs sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • preserve directory paths and OS causes for local install directory creation failures
  • extract ClickHouse tarballs in-process, reject unsafe paths and link-based binary entries, and validate gzip CRC/size before publishing a binary
  • document the required install examples, destination, download hosts and size, and bare-server bootstrap path concisely

Review resolution

Issue #462 explicitly requires the destination, network hosts, approximate download size, and bare local server start route. The earlier #507 quality concern applied to broad and duplicated wording; this version puts agent context first, condenses the required facts into one download section, omits the stable example, and tests only the required help fragments instead of snapshotting the full rendered output.

Archive I/O failures now retain their underlying source, and corrupt or truncated gzip streams fail before .clickhouse.extracting is renamed. The focused regressions also assert that the partial file is cleaned up.

Tests

  • cargo test -p clickhousectl version_manager::install::tests:: -- --nocapture
  • cargo test -p clickhousectl local::cli::tests::install_help_covers_install_requirements
  • cargo test -p clickhousectl
  • cargo fmt --all --check
  • cargo clippy -p clickhousectl --all-targets -- -D warnings
  • cargo build -p clickhousectl

Closes #462

@sdairs

sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Carry-over review from PR #507 (old PR for #462). Good parts of this PR: Error::CreateDir { path, source } with path + OS cause, in-process extraction with entry-path-traversal and symlink/hardlink rejection, partial-file (.clickhouse.extracting) → rename publish, and well-structured help. But there is one blocker and one content conflict, both carry-overs from #507's review:

  1. BLOCKER — corrupted gzip downloads are accepted as valid installs. This is exactly the bug macroscope flagged (High) on Make local install failures and prerequisites actionable #507, and sdairs confirmed the fix there: "Fixed… extract_tarball_auto now drains GzDecoder to gzip EOF before returning success, so CRC/ISIZE corruption and truncated trailers fail before the staged binary can be committed. Added focused corrupted-CRC and truncated-trailer regressions." In this PR, extract_tarball_auto (crates/clickhousectl/src/version_manager/install.rs:~279-419) never validates the gzip trailer: the tar entry iterator stops at the first zero block before the deflate stream ends, and flate2::GzDecoder only checks CRC/ISIZE when a read drives it past stream end — which never happens. This was verified empirically against this repo's pinned flate2/tar versions with logic mirroring this PR: CRC-corrupted and truncated-trailer archives both return Ok. Since install_resolved skips detect_binary_version when exact_version_known, a corrupted download is renamed into place and committed unexecuted. Fix: drain the decoder to EOF (e.g. archive.into_inner() copied to a sink) before renaming into place, and port Make local install failures and prerequisites actionable #507's regression tests corrupted_gzip_crc_is_not_committed and truncated_gzip_trailer_is_not_committed.

  2. Resolve the help-content conflict explicitly. On Make local install failures and prerequisites actionable #507, sdairs wrote: "worth a second pass review. help changes were very low quality." The recorded resolution: "agent context now precedes concise install examples led by latest; download internals, duplicated server-start guidance, the stable example, and the unnecessary broad help test were removed." This PR re-adds every one of those categories: the CLICKHOUSE DOWNLOAD internals block (destination, ~150 MB, hosts), the BOOTSTRAP ALTERNATIVE section, and a full-rendered-help snapshot test install_help_is_complete_and_stable. Tension: issue Make local install failures and prerequisites actionable #462's acceptance criteria literally ask for examples, destination, hosts, download size, and the bootstrap alternative — so the content may be justified. If it is, state in the PR description why the earlier "very low quality" verdict no longer applies (and make the content genuinely concise); otherwise trim to Make local install failures and prerequisites actionable #507's final EXAMPLES-only form and drop/narrow the snapshot test to assert only required substrings.

  3. Minor: archive failures go through Error::Extract(String) rather than a typed #[source] variant (loses the structured error chain Make local install failures and prerequisites actionable #507 has); no failure-path assertion that .clickhouse.extracting is cleaned up.

@sdairs
sdairs force-pushed the stack/462-actionable-install-failures branch from ab1c3cb to 25f9cd5 Compare August 26, 2026 18:58
@sdairs
sdairs force-pushed the stack/462-actionable-install-failures branch from 25f9cd5 to 0876e61 Compare August 26, 2026 19:01
@sdairs
sdairs force-pushed the stack/462-actionable-install-failures branch 3 times, most recently from b231485 to b0bbfc9 Compare August 26, 2026 19:10
@sdairs
sdairs force-pushed the stack/462-actionable-install-failures branch 2 times, most recently from 0474136 to f4b53de Compare August 26, 2026 19:20
@sdairs
sdairs force-pushed the stack/462-actionable-install-failures branch 2 times, most recently from 6d4ef31 to c5655d2 Compare August 26, 2026 19:28
@sdairs
sdairs force-pushed the stack/462-actionable-install-failures branch 2 times, most recently from 004186a to d559866 Compare August 26, 2026 19:34
@sdairs
sdairs force-pushed the stack/462-actionable-install-failures branch from d559866 to e517999 Compare August 26, 2026 19:36

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit e517999. Configure here.

Comment thread crates/clickhousectl/src/version_manager/install.rs
@sdairs
sdairs force-pushed the stack/462-actionable-install-failures branch from e517999 to 7633a1f Compare August 26, 2026 20:10
@sdairs
sdairs force-pushed the stack/462-actionable-install-failures branch from 7633a1f to 1b73479 Compare August 26, 2026 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make local install failures and prerequisites actionable

1 participant