Copilot commented on code in PR #891:
URL:
https://github.com/apache/arrow-rs-object-store/pull/891#discussion_r4471451381
##########
src/local.rs:
##########
@@ -982,41 +985,66 @@ fn hard_link(original: &std::path::Path, link:
&std::path::Path, fsync: bool) ->
/// directory after — the rename, so a successful return is durable. The file
is always closed
/// before the rename (checking for close errors): required for NFS error
detection and for some
/// FUSE filesystems (e.g. Blobfuse) that only commit the data on close.
+///
+/// Returns the file's metadata. A failure before the rename succeeds removes
the staging file.
+/// After a successful rename its path may already belong to a new upload, so
it is left alone.
fn finish_staged_rename(
file: File,
src: &std::path::Path,
dest: &std::path::Path,
fsync: bool,
-) -> Result<()> {
- sync_and_close(file, src, fsync)?;
- rename(src, dest, fsync).map_err(|source| Error::UnableToRenameFile {
source })?;
- Ok(())
+) -> Result<Metadata> {
+ let closed = staged_metadata(&file, src).and_then(|metadata| {
+ sync_and_close(file, src, fsync)?;
+ Ok(metadata)
+ });
+ let err = match closed {
+ Ok(metadata) => match rename(src, dest, fsync) {
+ Ok(()) => return Ok(metadata),
+ Err(RenameError::Fsync(source)) => {
+ return Err(Error::UnableToRenameFile { source }.into());
+ }
+ Err(RenameError::Rename(source)) => Error::UnableToRenameFile {
source }.into(),
+ },
+ Err(e) => e,
+ };
+ let _ = std::fs::remove_file(src); // Attempt to cleanup
+ Err(err)
Review Comment:
`finish_staged_rename` attempts to delete `src` on error, but if
`staged_metadata(&file, src)` fails, the `File` handle is still open when
`remove_file(src)` runs (the file is only dropped after returning). On Windows
this typically prevents deletion, reintroducing a staging-file leak
specifically on metadata failures. Refactor so the file is closed/dropped
before attempting `remove_file` in all pre-rename error paths (e.g., get
metadata, then always `sync_and_close(file, ...)` or explicitly `drop(file)`
before cleanup when metadata retrieval fails).
##########
src/local.rs:
##########
@@ -654,7 +639,11 @@ impl ObjectStore for LocalFileSystem {
Ok(_) => match rename(&staged, &to, fsync) {
Ok(_) => return Ok(()),
Err(source) => {
- let _ = std::fs::remove_file(&staged); //
Attempt to clean up
+ // Once renamed, `staged` may already be a
new upload's file.
+ if let RenameError::Rename(_) = source {
+ let _ = std::fs::remove_file(&staged);
// Attempt to clean up
Review Comment:
New behavior depends on distinguishing `RenameError::Rename` vs
`RenameError::Fsync` to avoid deleting `staged` after a successful rename.
There’s now no test that exercises the `RenameError::Fsync` path (rename
succeeds, directory fsync fails) to validate that `staged` is *not* removed.
Consider adding an injectable hook/abstraction for `fsync_parent_dir` in tests
(or a test-only fault injector) so you can deterministically force an fsync
failure after rename and assert the safety property.
##########
src/local.rs:
##########
@@ -982,41 +985,66 @@ fn hard_link(original: &std::path::Path, link:
&std::path::Path, fsync: bool) ->
/// directory after — the rename, so a successful return is durable. The file
is always closed
/// before the rename (checking for close errors): required for NFS error
detection and for some
/// FUSE filesystems (e.g. Blobfuse) that only commit the data on close.
+///
+/// Returns the file's metadata. A failure before the rename succeeds removes
the staging file.
+/// After a successful rename its path may already belong to a new upload, so
it is left alone.
fn finish_staged_rename(
file: File,
src: &std::path::Path,
dest: &std::path::Path,
fsync: bool,
-) -> Result<()> {
- sync_and_close(file, src, fsync)?;
- rename(src, dest, fsync).map_err(|source| Error::UnableToRenameFile {
source })?;
- Ok(())
+) -> Result<Metadata> {
+ let closed = staged_metadata(&file, src).and_then(|metadata| {
+ sync_and_close(file, src, fsync)?;
+ Ok(metadata)
+ });
+ let err = match closed {
+ Ok(metadata) => match rename(src, dest, fsync) {
+ Ok(()) => return Ok(metadata),
+ Err(RenameError::Fsync(source)) => {
+ return Err(Error::UnableToRenameFile { source }.into());
+ }
+ Err(RenameError::Rename(source)) => Error::UnableToRenameFile {
source }.into(),
+ },
+ Err(e) => e,
+ };
+ let _ = std::fs::remove_file(src); // Attempt to cleanup
+ Err(err)
}
/// Like [`finish_staged_rename`] but publishes via a hard link
(`PutMode::Create` semantics): the
-/// staging file is linked to `dest` and then removed. Returns
[`Error::AlreadyExists`] if `dest`
-/// already exists.
+/// staging file is linked to `dest` and then removed, whether or not linking
succeeds. Returns
+/// [`Error::AlreadyExists`] if `dest` already exists.
fn finish_staged_hard_link(
file: File,
src: &std::path::Path,
dest: &std::path::Path,
fsync: bool,
-) -> Result<()> {
- sync_and_close(file, src, fsync)?;
- match hard_link(src, dest, fsync) {
- Ok(()) => {
- let _ = std::fs::remove_file(src); // Attempt to cleanup
- Ok(())
- }
- Err(source) => match source.kind() {
- ErrorKind::AlreadyExists => Err(Error::AlreadyExists {
+) -> Result<Metadata> {
+ let linked = staged_metadata(&file, src).and_then(|metadata| {
+ sync_and_close(file, src, fsync)?;
+ hard_link(src, dest, fsync).map_err(|source| match source.kind() {
+ ErrorKind::AlreadyExists => Error::AlreadyExists {
path: dest.to_str().unwrap().to_string(),
source,
- }
- .into()),
- _ => Err(Error::UnableToRenameFile { source }.into()),
- },
- }
+ },
+ _ => Error::UnableToRenameFile { source },
+ })?;
Review Comment:
`dest.to_str().unwrap()` can panic for non-UTF8 paths. This can crash
callers when interacting with valid-but-non-UTF8 filesystem paths. Prefer a
non-panicking conversion such as `dest.to_string_lossy().to_string()` (or
change the error to carry a `PathBuf` if the error type allows).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
