Skip to content

fix: warn and clamp LANCE_INITIAL_UPLOAD_SIZE instead of panicking#6389

Open
LuciferYang wants to merge 3 commits intolance-format:mainfrom
LuciferYang:fix/object-writer-panic-to-result
Open

fix: warn and clamp LANCE_INITIAL_UPLOAD_SIZE instead of panicking#6389
LuciferYang wants to merge 3 commits intolance-format:mainfrom
LuciferYang:fix/object-writer-panic-to-result

Conversation

@LuciferYang
Copy link
Copy Markdown
Contributor

@LuciferYang LuciferYang commented Apr 2, 2026

Summary

  • Replace panic!() in initial_upload_size() with a warn-and-clamp fallback when LANCE_INITIAL_UPLOAD_SIZE is set outside the valid [5MB, 5GB] range, so misconfiguration can't crash the process
  • Extract MAX_UPLOAD_PART_SIZE constant for the 5GB upper bound
  • Extract clamp_initial_upload_size as a pure helper and add boundary unit tests

Motivation

Setting LANCE_INITIAL_UPLOAD_SIZE to a value outside the valid range previously crashed the entire process via panic!() — a disproportionate response to a perf-tuning env var misconfiguration. Per review feedback (#6389 (comment)), a crash (or even a propagated Result) forces every caller to handle a purely operator-side mistake. Clamping to the valid range and emitting a single warning lets the workload proceed and surfaces the misconfiguration to operators. This also matches the silent-fallback behavior of the sibling env vars LANCE_UPLOAD_CONCURRENCY and LANCE_CONN_RESET_RETRIES.

What Changed

initial_upload_size(): Return type stays usize. Out-of-range values are clamped into [5MB, 5GB] and a single tracing::warn! is emitted with requested and clamped fields. The existing OnceLock cache guarantees the warning fires at most once per process, so no separate rate-limiting logic is needed. Non-numeric and unset values continue to fall back silently to the 5MB default.

clamp_initial_upload_size(raw) -> (usize, bool): Pure helper extracted for testability. Returns the clamped value and whether clamping occurred.

MAX_UPLOAD_PART_SIZE: New constant for the 5GB upper bound.

Behavioral Equivalence

Input Before After
Env not set 5MB default 5MB default
Non-numeric (e.g. "abc") 5MB default 5MB default
Valid integer in [5MB, 5GB] Returns the value Returns the value
Integer < 5MB panic!() Clamped to 5MB + warn! (once)
Integer > 5GB panic!() Clamped to 5GB + warn! (once)

No API changes — ObjectWriter::new() signature is unchanged.

Test plan

  • New boundary unit tests: below min, min boundary, in-range, max boundary, above max, usize::MAX
  • cargo test -p lance-io --lib object_writer — 7 tests pass
  • cargo clippy -p lance-io --tests -- -D warnings — clean
  • cargo fmt -p lance-io -- --check — clean
  • cargo check --workspace --tests — full workspace compiles

Setting LANCE_INITIAL_UPLOAD_SIZE to an out-of-range value (<5MB or >5GB)
previously crashed the process with panic!(). Now returns an error from
ObjectWriter::new(), letting callers handle it gracefully.

- Change initial_upload_size() return type from usize to Result<usize>
- Cache Result<usize, String> in OnceLock (lance_core::Error is not Clone)
- Store validated upload_size in ObjectWriter struct to avoid repeated
  OnceLock access and eliminate .expect() in next_part_buffer()
- Preserve silent fallback to default for non-numeric values, consistent
  with sibling env vars (LANCE_UPLOAD_CONCURRENCY, LANCE_CONN_RESET_RETRIES)
- Extract MAX_UPLOAD_PART_SIZE constant for the 5GB upper bound
@github-actions github-actions bot added the bug Something isn't working label Apr 2, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 2, 2026

Codecov Report

❌ Patch coverage is 83.72093% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-io/src/object_writer.rs 83.72% 6 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

/// Maximum part size in GCS and S3: 5GB.
const MAX_UPLOAD_PART_SIZE: usize = 1024 * 1024 * 1024 * 5;

fn initial_upload_size() -> Result<usize> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: What do you think of issuing a warning if the variable is misconfigured, and resetting to some reasonable default or clipping to value as needed? That way we don't need to thread the error handling carefully elsewhere. You would have to make sure you only issued the warning once, or once every few seconds, as doing it every time would be annoying.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call — done in a47f8ec. Switched initial_upload_size() back to -> usize and now clamps to [5MB, 5GB] with a single tracing::warn! emitting requested and clamped fields. The existing OnceLock cache gives us the "warn once per process" guarantee for free, so no separate rate-limiter needed.

Also extracted the clamp/was-clamped logic into a pure clamp_initial_upload_size helper and added boundary unit tests (below min, min/max boundaries, in-range, above max, usize::MAX). Behavior is now consistent with the sibling env vars (LANCE_UPLOAD_CONCURRENCY, LANCE_CONN_RESET_RETRIES) that fall back silently on bad input.

Per review feedback on lance-format#6389: an out-of-range perf-tuning env var
shouldn't fail writes. Clamp the value to the valid [5MB, 5GB] range
and emit a tracing::warn! once (the existing OnceLock cache guarantees
the warning fires at most once per process). This also matches the
silent-fallback behavior of the sibling env vars LANCE_UPLOAD_CONCURRENCY
and LANCE_CONN_RESET_RETRIES.
Extract the clamp logic into a pure helper so the boundary policy
([5MB, 5GB]) and was_clamped signal can be unit-tested without fighting
the OnceLock cache in initial_upload_size().
@LuciferYang LuciferYang changed the title fix: replace panic with Result in LANCE_INITIAL_UPLOAD_SIZE validation fix: warn and clamp LANCE_INITIAL_UPLOAD_SIZE instead of panicking Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants