Repository navigation
Download remote files: downloadFile() and downloadTo(), digest-verified and atomic (#147) - #180
Conversation
…ed and atomic (#147) client.downloadFile(remote, local) and client.file(remote).downloadTo(local) copy a remote file to a local one through the WinRM connection: the counterpart of uploadFile(), with the same guarantees. - A PowerShell probe reports the size and the SHA-256 digest of the file, opening it with the share mode of the reads: certutil -hashfile, which the uploads use, fails with a sharing violation on a file another process holds open for writing (measured). - An identical local file is not transferred again: the call returns 0. - The content streams into <name>.<random>.part next to the destination, with bounded memory, is verified against the probed digest, fsync'ed, and moved onto the destination with ATOMIC_MOVE: a failure or a timeout never leaves a truncated file, and the timeout message tells how many bytes were transferred. - An existing directory as the destination receives the file under its remote name. A remote-file script whose start is rejected by the host's operation quota is now retried like the file-transfer steps, within the timeout (RemoteFiles.start). Co-Authored-By: Claude Opus 5.5 <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 620f5b8b90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ame, retry deadline - A Publication handshake makes the final move and the caller's timeout mutually exclusive: a timeout is only reported when the destination was left untouched, and a deadline that fires while the verified file is moved into place lets the move, and the download, complete. - The probe reports the bytes it hashed (the stream position after ComputeHash) instead of the length read beforehand, so size and digest always describe the same bytes; the integrity check also compares the received byte count. - The staging file keeps at most 64 UTF-16 units of the destination's name, so a long name still fits the 255-unit (NTFS) and 255-byte (ext4) limits. - A quota retry of a remote-file script is only attempted when its pause ends before the timeout: a streaming terminal never starts a command after its deadline. - New test for the common refresh case (an existing, different local file is replaced); ATOMIC_MOVE replaces on Windows too (MoveFileEx with MOVEFILE_REPLACE_EXISTING). Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 468e032db9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The pause before a quota retry can overrun its planned length (a GC pause, a descheduled thread): check the timeout again after it, so a streaming terminal never starts a command after its deadline. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dbe72199b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tegrity wording - The local name for a directory destination is taken after the last \ or / only; a name that still contains a colon (an alternate data stream, whose stream name alone would collide, or a drive-relative path such as C:a.txt, which a Windows client resolves outside the directory) is refused before anything is sent. - A replaced file keeps its POSIX permissions: the staging file gets them before any byte is written, so a 0600 file stays private throughout. Windows keeps the directory's inherited ACL (documented). - The docs now state the integrity guarantee precisely: the local copy is exactly the bytes the probe hashed, one consistent version of the file, never a mix of two. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 090dd7833f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…eview) A chmod after creation leaves a window in which another user can open the file under the umask-default mode and keep the descriptor, then read what is written later. The POSIX permissions of the replaced file are now a creation attribute: the umask can only narrow that mode, and the exact permissions are restored right after, before any byte is written. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39e5c2d41e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Closes #147. This is the third step of the remote file access family, after #146 (merged in #177) and #145 (merged in #179). #148 (CLI) comes next.
What changed
Downloads: the counterpart of
uploadFile(...), with the same guarantees.cp. A remote name with a colon (an alternate data stream, or a drive-relative path) is refused there: the caller names the local file. The destination's directory is created when needed.offset(...)/length(...)/maxBytes(...)do not apply, the same as fordigest(...).How it works
RemoteFiles.probeScript) reports the size and the SHA-256 digest of the file as one base64 line (an 8-byte size, then the digest). The size is the number of bytes hashed, so size and digest always describe the same bytes. It opens the file exactly like the reads, with a share mode that tolerates other writers.openStream()into<name>.<random>.partnext to the destination, the name cut to 64 characters so a long one still fits the file-name limits. Memory stays bounded.fsync'ed (FileChannel.force) and moved onto the destination withATOMIC_MOVE, which replaces an existing file on Windows too (MoveFileExwithMOVEFILE_REPLACE_EXISTING). A replaced file keeps its POSIX permissions: the staging file is created with them (a creation attribute, never a laterchmodthat a reader could race).The destination is never seen truncated. On a digest mismatch, a read error or a timeout, it keeps its previous content and the
.partfile is deleted. The local copy is always exactly the bytes the probe hashed: one consistent version of the file, never a mix of two. A change that reaches bytes not transferred yet fails the integrity check.The timeout is a wall-clock deadline for the whole download. When it fires, the message says how far it got:
Downloads are not resumable. That was decided in the issue, and the Javadoc and
file-transfers.mdboth say so.Quota retry. It lives in
RemoteFiles.start(), reusingisRetryableQuotaRejection,QUOTA_RETRIESandQUOTA_RETRY_DELAY_MILLIS. So every remote-file script (reads,info(),list(), the download's two scripts) retries a start that the host's operation quota rejected. A retry is only attempted when its pause ends before the timeout.Where this departs from the issue
certutil -hashfilefails withERROR_SHARING_VIOLATIONon a file another process holds open for writing, while the read'sFile.Open(..., 'ReadWrite')opens it fine. A certutil probe would make downloads fail on exactly the service logsopenStream()can read. SoShellFileCopy.parseAnyDigestis not used.digestHexisn't either: it takes abyte[], which would break the bounded-memory requirement for the local digest.file-transfers.mdandfiles.md.Worth a reviewer's eye
The deadline and the final move exclude each other. The blocking terminals run in a worker that
Utils.executecancels when the deadline fires, and the caller gets theWinRMTimeoutExceptionright away. A smallRemoteFile.Publicationhandshake (twosynchronizedmethods) decides between the worker's move and the caller's timeout:The worker, usually blocked in a socket read, notices the cancellation at its next step and deletes its
.partfile, so that cleanup happens after the call returns.@SuppressFBWarnings("AT_NONATOMIC_64BIT_PRIMITIVE")onRemoteFile. SpotBugs treats any class with ajava.util.concurrent.atomiclocal as multithreaded code, and then flags theoffset/length/maxBytessetters. TheAtomicLongprogress counters are needed for a race-free read of the progress in the timeout message.RemoteFileis documented as not thread-safe.Permissions on replace. POSIX permissions are carried over, so a
0600file stays0600. Windows ACLs are not: the new file gets the directory's inherited ACL, as documented. Java has noReplaceFile, andsetAclwould turn inherited entries into explicit ones.Two local-filesystem guards.
Files.createDirectoriesis only called when the directory is missing: on JDK 11 and 17 it rejects an existing symbolic link to a directory, such as/tmpon macOS. Checked in the JDK sources; fixed in 21.NoSuchFileException, wrapped in the usualWinRMClientException.Tests
RemoteFileTest(FakeWsmanServer):600 of 1000 bytes transferred, the destination keeps its previous content, and the.partfile disappears once the abandoned transfer stops;Publication: abandoning and publishing exclude each other;a.txt:meta) or a drive-relative path is refused for a directory destination, with nothing sent.RemoteFilesScriptTest(localpowershell.exe): the probe reports the size and digest, including for an empty file and for a file held open for writing by another process.WinRMLiveTest:uploadFile→downloadFileround trip, with every byte value under a non-ASCII name, into a directory, then skipped on a second download;Verification
mvn clean verify siteon JDK 17: 305 tests pass; 0 checkstyle, PMD and SpotBugs findings.-DargLine=-Xmx32m -Dwinrm.live.download.mib=64)Docs
file-transfers.md: new Downloading a file section covering how it works, the guarantees, "not resumable", the timeout, the measured throughput, and SMB for bulk data.files.md: short Downloading to a local file section,downloadToin the timeout list, and the quota retry.index.md,preparing-the-host.md,timeouts-and-errors.mdandmigrating-from-winrm4j.md.🤖 Generated with Claude Code