Fix bug in S3 where it ignores EOF #1178
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained, 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 (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.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 (waiting on @aaronmondal)
There was a problem hiding this comment.
Aww there goes my reason to implement fuzzing lol
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / 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 (20.04), docker-compose-compiles-nativelink (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 2 discussions need to be resolved
nativelink-store/src/s3_store.rs line 422 at r1 (raw file):
.map_ok_or_else(|e| Err(make_err!(Code::Aborted, "{e:?}")), |_| Ok(())), // Stream all data from the reader channel to the writer channel. tx.bind_bufferd(reader_ref)
nit: buffered
nativelink-util/src/buf_channel.rs line 134 at r1 (raw file):
.recv() .await .err_tip(|| "In DropCloserWriteHalf::bind::recv::eof")?;
nit: I believe this error message needs to be adjusted since the logs emitted via tracing already contain the function.
If this manual handling is intentional, we might want to be careful to ensure that we're referencing the correct symbol. Technically we're not raising this message in the implementation of ::eof.
allada
force-pushed
the
fixed-s3-ignoring-eof
branch
from
a749a66 to
16cd589
Compare
Fixes an upload bug where it blindly accepts data without receiving EOF.
allada
force-pushed
the
fixed-s3-ignoring-eof
branch
from
16cd589 to
f3e9863
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks
nativelink-store/src/s3_store.rs line 422 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit:
buffered
Done.
nativelink-util/src/buf_channel.rs line 134 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: I believe this error message needs to be adjusted since the logs emitted via tracing already contain the function.
If this manual handling is intentional, we might want to be careful to ensure that we're referencing the correct symbol. Technically we're not raising this message in the implementation of
::eof.
Done.
There was a problem hiding this comment.
Reviewed 2 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Installation / macos-13, Installation / macos-14, Installation / 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 (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-22.04, windows-2022 / stable
Fixes an upload bug where it blindly accepts data without receiving EOF.
This change is