Skip to content
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

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

allada
Copy link
Member

@allada allada commented Aug 22, 2024

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 Reviewable

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
Copy link
Member Author

@allada allada left a 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(
>    (),
 )

Copy link
Member Author

@allada allada left a comment

Choose a reason for hiding this comment

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

+@adam-singer

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)

Copy link
Member

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@allada allada merged commit c21d59f into TraceMachina:main Aug 22, 2024
27 checks passed
@allada allada deleted the fix-verify-store branch August 22, 2024 03:52
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.

VerifyStore fails small messages when verify_hash and verify_size are both set
2 participants