allthingssecurity opened a new pull request, #26881: URL: https://github.com/apache/camel/pull/26881
# Description [CAMEL-25016](https://issues.apache.org/jira/browse/CAMEL-25016) With `noop=true` or `idempotent=true`, a remote file consumer (FTP, FTPS, SFTP, SMB, Azure Files) never retried a file whose download failed. The idempotent key is added while polling (`idempotentEager=true` is the default). When the retrieve then failed, for example on a socket timeout, a connection reset or `426 Transfer aborted`, `GenericFileConsumer.processExchange` only removed the file from the in-progress repository, reported the exception and returned `true`. So `processBatch` counted the file as started and did not remove the key: it only removes the key of the files for which `processExchange` returned `false`, which CAMEL-21947 added for the read-lock-not-acquired case. `GenericFileOnCompletion`, which would remove the key on rollback, is only registered after a successful retrieve. Later polls skipped the file because its key was present, and only a single WARN was logged. With the default memory repository this lasted until a restart. With a persistent idempotent repository (JDBC, Infinispan, Hazelcast, ...) the file was never consumed again. Consumers that are not idempotent were not affected, because the file was removed from the in-progress repository and was picked up on the next poll. The local file component is not affected in practice either, because `FileOperations.retrieveFile` always returns true. This change handles a failed or ignored retrieve the same way as a `begin` that failed, following CAMEL-21947: - `processStrategy.abort(...)` is called with the retrieved target file. It releases the exclusive read lock, deletes a partial local work file and releases the retrieved resources; - the file is removed from the in-progress repository, as before; - the eagerly added idempotent key is removed for the original file, by `absoluteFileName` or the `FILE_IDEMPOTENT_KEY` snapshot, the same way `GenericFileOnCompletion.processStrategyRollback` does. The key cannot be taken from the exchange file here, because with `preMove`, `GenericFileRenameProcessStrategy.begin` has bound the pre moved file to the exchange, while the key was added for the original path; - `processExchange` returns `false`, so the file counts as not started; - the failure is still reported to the exception handler, with the same message as before. Failures after the file was retrieved are unchanged, because `GenericFileOnCompletion` commits or rolls back. `tryRetrievingFile`, which the browse support in `GenericFileEndpoint` also uses, is unchanged. Before this change the read lock was not released on a failed retrieve either. That has no lasting effect for the built-in read locks of the remote components (`none`, `changed`, `rename`). The idempotent read locks (`readLock=idempotent`, `idempotent-changed`, `idempotent-rename`) only exist in the file component (`FileProcessStrategyFactory`), where the retrieve does not fail. The `abort` call matters for a custom `exclusiveReadLockStrategy` bean, and it keeps the failed retrieve on the same abort path as a failed `begin`. Behaviour changes, also in the 4.23 upgrade guide: - A file whose download fails is now retried on the next poll also by idempotent and `noop=true` consumers, as it already was by the other consumers. - A failed retrieve no longer counts as a polled message, the same as a file whose read lock was not acquired. A poll in which no file could be downloaded counts as idle, which matters for `backoffIdleThreshold`, `sendEmptyMessageWhenIdle` and `greedy`. Tests: new `FileConsumerRetrieveFailureTest` in camel-core, next to the other file consumer tests. The retrieve of the file component cannot fail, so the test plugs a `FileOperations` whose first `retrieveFile` fails into the real `FileConsumer` through the protected `FileEndpoint.newFileConsumer` hook. `FileConsumer` shares `GenericFileConsumer` with the remote consumers, so this covers the same code path as a failed FTP download: - `noop=true` (eager idempotent key), the retrieve throws: the file is consumed on the next poll. This is the production scenario. - `idempotent=true` with `preMove`, the retrieve throws: the key of the original file is removed, and the file is consumed after it is moved back from the pre move directory. - `readLock=idempotent`, the retrieve throws, and the retrieve returns false with `ignoreCannotRetrieveFile` returning true: the file is consumed on the next poll. These cover the abort path, which releases the read lock (as it would for a custom `exclusiveReadLockStrategy` bean). They are not a production scenario, because the file component's retrieve does not fail. Without the fix all 4 fail: ``` java.lang.AssertionError: mock://result Received message count. Expected: <1> but was: <0> org.opentest4j.AssertionFailedError: The eager idempotent key of the original file should be removed ==> expected: <0> but was: <1> ``` Without the removal of the original file's key in `processExchange`, only the `preMove` case fails, with the second message, because `processBatch` then removes the key of the pre moved file. With the fix all 4 pass. `*File*,*ReadLock*` in camel-file and camel-core: 22 + 552 tests, 0 failures (16 skipped). I did not add a camel-ftp test. Its embedded FTP server (`FtpServerTestSupport` with `FtpEmbeddedService` from camel-test-infra-ftp) needs no Docker, but its dependencies (Apache FtpServer, jsch, camel-test-infra-ftp) were not available in my offline build, and the FTP tests are integration tests (`*IT`, run by failsafe). An IT with `noop=true` and a custom operations class that fails the first download would follow `FromFtpNoopIT`. Found with a TLA+ model of the file consumer's poll, read lock and idempotent repository, then reproduced against the real classes: with `noop=true` or `idempotent=true`, the file is now processed on the next poll after a failed retrieve. Before, it was never processed. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built and tested the affected modules, including the formatter and import-sort plugins. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
