Croway opened a new pull request, #29337: URL: https://github.com/apache/camel/pull/29337
JIRA: [CAMEL-26648](https://issues.apache.org/jira/browse/CAMEL-26648), [CAMEL-26671](https://issues.apache.org/jira/browse/CAMEL-26671) The `mina-sftp` counterpart of #29273 (CAMEL-26656, `sftp`). It resolves CAMEL-26648 (both symptoms) and CAMEL-26671. `readLock=changed` now reads the file attributes with the request that `existsFile` uses, so the `fastExistsCheck` branch that never acquires the lock goes away, and a failed whole listing is now wrapped as the listing one entry at a time already is. **Merge after #29273:** the second commit rewords the `fastExistsCheck` description shared with `sftp`, which only holds for `sftp` once #29273 is merged. ## What was wrong - **CAMEL-26671, `existsFile`:** `MinaSftpOperations.existsFile` sent `stat`, which follows a symbolic link, so a dangling link counted as missing, while the directory listing shows it. It also returned `false` for every `IOException`: a status of the server such as `PERMISSION_DENIED`, and an I/O or session failure. For `fileExist=Fail|Ignore|Move`, `tempFileName`, `tempPrefix` or a producer `doneFileName`, such an error meant "no such file". - **CAMEL-26671, `readLock=changed`:** `MinaSftpChangedExclusiveReadLockStrategy` listed the whole parent directory on every check (1000 files: 3 OPENDIR and 2,218 entries listed for one consumed file). - **CAMEL-26648:** with `fastExistsCheck=true`, the read lock listed the path of the file itself. The strategy was written for JSch, whose `ls` accepts a file path. MINA SSHD sends OPENDIR for it, the server refuses it for a regular file, and the `UncheckedIOException` thrown by the lazy `readDir` escaped. The file was never consumed: each poll logged a begin failure, and `readLockTimeout` did not help. - **CAMEL-26648, listing errors:** the same lazy `readDir` made `listFiles(String)`, which catches only `IOException`, let a failed OPENDIR escape as `UncheckedIOException`. With `stepwise=false` and the whole listing (no `maxMessagesPerPoll`, or `preSort`, `doneFileName`, `eagerMaxMessagesPerPoll=false`), a missing directory failed every poll, also with `ignoreFileNotFoundOrPermissionError=true`. The listing one entry at a time (CAMEL-26635) already wrapped it, so with `maxMessagesPerPoll` the same directory was ignored. ## What changed - `existsFile` sends a single `lstat` of the path, where `SSH_FX_NO_SUCH_FILE` means `false`. MINA SSHD reports a status sent by the server as an `org.apache.sshd.sftp.common.SftpException`, and an I/O or session failure as another `IOException`, so the two are told apart by type (no heuristic needed, unlike JSch). - On any other status (`PERMISSION_DENIED`, `OP_UNSUPPORTED`, `FAILURE`, `BAD_MESSAGE`, a later-version status such as `NO_SUCH_PATH`), it lists the directory of the file and compares the names literally. The listing stops at the matching entry and always closes its handle. A missing directory means `false`; another failure of the listing fails the check. - An I/O or session failure throws `GenericFileOperationFailedException` (with the `IOException` as cause, so `isTransportError` still sees it), instead of answering `false`. - `readLock=changed` reads the size and modification time with the same `lstat` on every check, through the new public `MinaSftpOperations.getFileAttributes(String)`, with and without `fastExistsCheck`. It returns a small `MinaSftpOperations.FileAttributes` record (length, modification time in millis as `MinaSftpRemoteFile`), as `SftpOperations.FileAttributes` does in #29273. Operations that are not `MinaSftpOperations` keep the old listing. - **Symbolic links:** `lstat`, not `stat`, on purpose, as in #29273. MINA SSHD and OpenSSH list a link with its own attributes, so the read lock keeps comparing the link's own size and modification time, as before, and the fallback listing gives the same answer. - Third commit: `listFiles(String)` catches `IOException | UncheckedIOException` and throws `GenericFileOperationFailedException`, exactly as `listFiles(String, Predicate)` does, so `ignoreCannotRetrieveFile` (`NO_SUCH_FILE`, `PERMISSION_DENIED`) applies to both listings the same way. The two comments that said `readDir` does not throw for a directory that does not exist are corrected: it only sends OPENDIR once iterated. - Second commit: the shared `fastExistsCheck` description (camel-ftp-common, used by ftp, ftps, sftp and mina-sftp) says that the option makes no difference for the two SFTP components, as #29273 announced. The json of the four components, the catalog and the endpoint DSL are regenerated. The `sftp` half of that sentence holds once #29273 is merged, so this commit should go in after it. ## Tests New `MinaSftpExistsFileIT`, against the embedded MINA SFTP server, which counts requests and listed entries with an `SftpEventListener` and can answer a request type with an error status: - `existsFile`: existing file, missing file, directory, missing directory, in a 10-file and a 1000-file directory: exactly 1 LSTAT and no other request each. A dangling link exists. - Fallback: LSTAT answered with each of the 5 statuses above, and LSTAT and STAT both refused on the file: the answers are still right, from the listing. LSTAT and OPENDIR both refused: the check fails. The connection dropped during LSTAT: the check fails with an I/O cause, and does not list the directory. - Producer: `fileExist=Fail` into a 1000-file directory costs 2 LSTAT and no listing; `fileExist=Fail` on a dangling link fails and writes nothing through the link. - New `MinaSftpConsumerMissingDirectoryIT`, `stepwise=false` on a missing directory, for the whole listing, the whole listing with `preSort`, and the listing one entry at a time: with `ignoreFileNotFoundOrPermissionError=true` the polls complete without failure; without it each poll fails with a `GenericFileOperationFailedException` carrying `SSH_FX_NO_SUCH_FILE`, not an `UncheckedIOException`. - `readLock=changed`, both `fastExistsCheck` values, 1000 files: the only OPENDIR is the poll, fewer than 1000 entries listed. `fastExistsCheck=true` with `delete=true` consumes and deletes the file (CAMEL-26648). A symbolic link is consumed with both values. A server that refuses LSTAT still gets the file consumed through the listing. A file that grows before each of the first three checks is consumed complete (driven from the server listener, no sleeps). Evidence: - origin/main: `Tests run: 19, Failures: 17`. With the fix: `Tests run: 19, Failures: 0`. The 2 that pass on main are guards: the symbolic link with `fastExistsCheck=false` and the read lock fallback when LSTAT is refused, which the old listing also handled. - `MinaSftpConsumerMissingDirectoryIT` on origin/main: `Tests run: 6, Failures: 4` (both whole-listing variants of both tests, with `UncheckedIOException`; the listing one entry at a time already passes). With the fix: `Tests run: 6, Failures: 0`. - The earlier reproducers, on the fix: a dangling link exists (1 LSTAT); an existing file whose STAT is refused exists (LSTAT, then the listing); `readLock=changed` on 1000 files: 1 OPENDIR and 214 entries (was 3 OPENDIR and 2,218); `fastExistsCheck=true`: consumed, 0 failures (was 10 begin failures in 3 s and never consumed). - Whole `camel-mina-sftp` module (`mvn verify`, unit tests and all embedded-server ITs): 53 unit tests and 328 ITs, 0 failures. One error, `MinaSftpLargeFileIT.testZeroByteFile`: the embedded server did not send its banner within 30 s on a loaded machine; that class passes 12/12 when rerun. After the rebase on the latest main, the unit tests, `MinaSftpExistsFileIT`, `MinaSftpLargeFileIT` and `SftpChangedReadLockIT` pass again. The `camel-ftp-common` and `camel-ftp` unit tests (190) pass with the second commit. With the third commit: the unit tests (53), `MinaSftpExistsFileIT`, `MinaSftpConsumerMissingDirectoryIT` and the listing ITs (`*NotStepwise*IT`, `MinaSftpErrorHandlingIT`, `MinaSftpListFilesIT`, `MinaSftpConsumerMaxMessagesPerPollListingIT`), 51 ITs, pass. ## Docs - `mina-sftp-component.adoc`: a new section "Checking whether a file exists" (one request per check, `fastExistsCheck` making no difference, links not followed, the fallback and when the check fails). The catalog copy is refreshed. - The listing fix needs no doc or upgrade-guide line: `ignoreFileNotFoundOrPermissionError` now does what it documents with the whole listing, as it already did with the listing one entry at a time, and no option or default changes. - 4.24 upgrade guide: its own section for `camel-mina-sftp`, next to the TCP_NODELAY one, so it merges mechanically with the `sftp` section of #29273: a dangling link now exists (for example `fileExist=Fail` now fails on it), and a status other than "no such file" now falls back to the listing, and fails the check when that fails too, where before the file counted as missing. ## Backport Not proposed for camel-4.18.x / camel-4.22.x. A regular file on a server that answers `lstat` gets the same answers as before, with fewer requests; but a dangling link now exists and an error now fails the check or falls back to the listing, which needs the upgrade-guide note. On 4.18.x, mina-sftp still uses the `readLock=changed` strategy of camel-ftp, shared with JSch, so a backport there would be different code. _Claude Code on behalf of Federico Mariani_ -- 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]
