-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix verify_size w/ verify_hash set to true in VerifyStore #1273
Conversation
Fixes bug where `verify_size` & `verify_hash` are both set to true, it'd cause small messages to always fail the hash check. fixes TraceMachina#1272
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 LGTMs obtained, and 0 of 2 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, and 1 discussions need to be resolved
a discussion (no related file):
Proof this test catches the failure presented:
failures:
---- verify_size_and_hash_suceeds_on_small_data stdout ----
thread 'verify_size_and_hash_suceeds_on_small_data' panicked at nativelink-store/tests/verify_store_test.rs:374:5:
assertion failed: `(left == right)`: Expected success, got: Err(Error { code: Internal, messages: ["Sender dropped before sending EOF", "During first read of buf_channel::take()", "Failed to collect all bytes from reader in memory_store::update", "---", "Hashes do not match, got: a665a45920422f9d417e4867efdc4fb8a04a1f3fff1fa07e998e86f7f7a27ae3 but digest hash was e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"] })
Diff < left / right > :
<Err(
< Error {
< code: Internal,
< messages: [
< "Sender dropped before sending EOF",
< "During first read of buf_channel::take()",
< "Failed to collect all bytes from reader in memory_store::update",
< "---",
< "Hashes do not match, got: a665a45920422f9d417e4867efdc4fb8a04a1f3fff1fa07e998e86f7f7a27ae3 but digest hash was e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
< ],
< },
>Ok(
> (),
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 LGTMs obtained, and 0 of 2 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, and 1 discussions need to be resolved (waiting on @adam-singer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and 1 discussions need to be resolved
Fixes bug where
verify_size
&verify_hash
are both set to true, it'd cause small messages to always fail the hash check.fixes #1272
This change is