Repository navigation
CAMEL-25728: camel-file - keep a done file with a fixed name for the files a poll left - #28035
Merged
davsclaus merged 2 commits intoOct 18, 2026
Conversation
…files a poll left With doneFileName=ready the consumer only picks up the files of a directory while the done file exists, and the last file of the batch (CamelBatchComplete) deletes it. The batch is the poll: with maxMessagesPerPoll the files beyond the limit, and a file whose processing could not start (read lock not acquired), stay in the directory, but the done file is gone, so they are left until a new done file is written. The poll now remembers the files it left (beyond the limit, or not started), per done file, and the last file of the batch does not delete the done file while there are any. The next poll only consumes these files, so a file written to the directory meanwhile still waits for a new done file, and the done file is deleted after its last file as before (or by the next poll if these files are gone). A file that was processed and failed is not left: it is handled as before. To know the files beyond the limit, such a consumer checks every file of the directory also with the eager limit, and then cuts the list to the same files the eager limit takes. The state is in memory and cleared on stop. Dynamic done files, noop and recursive=true are unchanged. The FTP, SFTP, SMB and Azure Files consumers share this code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
reviewed
Oct 18, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Thanks @allthingssecurity.
The problem is real: with a fixed done file, the last file of the poll deletes it, so the files beyond maxMessagesPerPoll, or a file whose read lock was not acquired, sit there until the next done file. Keeping the done file while a poll left files is a reasonable fix. I traced it through the eager and non-eager limits, the not-started path, the shutdown drain and the browse consumer, and the bookkeeping holds up. ftp/sftp/smb/azure-files go through the same canPollMoreFiles, isMatched and GenericFileOnCompletion, so they are consistent too.
My main concern is the gate that only admits the left files while the done file is kept. The PR lists "a producer must not rewrite a done file that is still there" as a limit, but Camel's own file producer does exactly that: to("file:bar?doneFileName=done") deletes and rewrites done after every file it writes (GenericFileProducer.writeDoneFile). With that producer feeding from("file:bar?doneFileName=done&maxMessagesPerPoll=N"), the files written while the consumer drains are not admitted, and the done file the producer just rewrote to announce them is deleted after the last left file. They are then stranded until the producer writes again, while on main the next poll picks them up. So for the most common Camel-to-Camel setup this widens the problem instead of closing it. Could the gate be lifted when the done file was rewritten (its timestamp changed; FTP listings may only have minute resolution, though), or dropped so the done file is simply kept while files are left? Main already consumes files written after the done file as long as it is there. A test with a Camel producer writing the done file during the drain would pin this.
Second, GenericFilePollingConsumer forces maxMessagesPerPoll=1, so pollEnrich and ConsumerTemplate.receive with a fixed done file now take this path on every receive and keep the done file across receives (on main the first received file deletes it). That is probably an improvement, but it is a behaviour change without a test, and the upgrade-guide entry should mention it.
A small mismatch with the description: when files are left and the last file of the poll fails, it is not retried under the same done file, because the next poll admits only the left files (main does retry it; testFileThatAlwaysFailsWithFilesLeftDoesNotHoldDoneFile pins this). Fine with me, but the description says otherwise.
Claude Code on behalf of Claus Ibsen
Contributor
|
🧪 CI tested the following changed modules:
🔁 2 tests passed only after a retry on JDK 25 (2 retried attempts) Recovered flaky tests on JDK 25 (2)
🔬 Scalpel shadow comparison — Scalpel: 489 of 706 tested, 25 compile-only — current: 493 all testedMaveniverse Scalpel detected 489 affected modules (current approach: 493). Skip-tests mode would test 489 modules (4 direct + 487 downstream), skip tests for 25 (generated code, meta-modules) Modules only in current approach (4)
Modules Scalpel would test (489)
Modules with tests skipped (25)
Build reactor — dependencies compiled but only changed modules were tested (4 modules, 53.8s total)Total reactor time: 53.8s
Top 20 slowest modules:
|
…, list one file past the limit Review follow-up: - the kept done file no longer admits only the files left: the polls consume every file of the directory while it is there, as they do whenever a done file is there. A Camel producer with doneFileName rewrites the done file after each file; with the gate its files written during the drain were rejected and then stranded when the done file was deleted. A failed last file is now tried again with the files left, as on main. - with the eager limit, one file past maxMessagesPerPoll is listed (instead of every file) to know that the poll leaves files; when the listing stops there, the done files of the files listed are kept, and a kept done file is only deleted as gone after a complete listing. - the done file kept for a file that was not started is the one of the file as polled, not of the file the preMove bound to the exchange. - upgrade guide: pollEnrich and ConsumerTemplate.receive keep the done file until the last file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Thanks for the thorough review. All three points are addressed in e47aeb8 (details in the inline replies):
On the description mismatch: with the gate removed, a last file that fails while files are left is tried again under the same done file, as on main. Tests: 14 in Claude Code on behalf of allthingssecurity |
gnodet-bot
reviewed
Oct 18, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Well-structured fix for a real gap: done files for subdirectories that have been fully polled were not cleaned up when a GenericFileKeptDoneFiles strategy was in use. The core logic for the single-directory case looks correct.
Two substantive concerns before this merges:
1. Recursive-directory done-file leak under persistent truncated listings
GenericFileKeptDoneFiles.removeUnpolled (line 63, !listingComplete branch) calls kept.removeAll(listedDoneFiles) — which removes done files only for the entries that appeared in the current (truncated) listing. For a subdirectory that was emptied in a previous poll cycle, its done file will never appear in future truncated listings; the pruning never fires. As long as listings remain persistently cut (directory always returns more than maxMessagesPerPoll), that orphan done file stays on disk indefinitely. This is an edge case, but the fix specifically targets done-file hygiene — it's worth a comment in the code and, ideally, a test.
2. startBatch in the incomplete-listing path is a no-op
When listingCut=true, startDoneFileBatch(kept, files, false) is called (line 182), which ends up in startBatch(doneFiles, false) → kept.removeAll(listedDoneFiles). The caller then immediately calls listedDoneFiles.forEach(kept::keep) (line 188), which re-adds every entry that was just removed. Net effect: removeAll + addAll of the exact same set = no change. The comment in startBatch claims it "removes done files corresponding to listed files that are no longer needed" but it cannot — any entry removed by removeAll is put right back by the caller's forEach. Consider whether this branch needs a different strategy.
Minor:
- Comment at line 278 ("the file as polled: the begin strategy may pre move it, and then binds the pre moved file to the exchange") reads as a sentence fragment — the leading verb is missing.
- PR description vs test count: the description mentions 11 tests; the diff for
FileConsumerDoneFileNameFilesLeftTest.javacontains 14@Testmethods. The description would benefit from an update. handleDoneFilerollback path (line 178): consider whether the done-file tracking inGenericFileOnCompletionshould also reflect themoveFailedscenario — currently it's only triggered on rollback whenmoveFailedis configured. IfmoveFailedis absent, a failed exchange skips done-file tracking entirely; that may be intentional but worth a code comment.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
approved these changes
Oct 18, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Thanks @allthingssecurity.
Re-review of e47aeb8 against the commit I reviewed (f959c79). All three earlier points are fixed:
- The admission gate is gone (
GenericFileDoneFileAdmissionremoved,isMatcheduntouched). While the done file is kept, the polls consume every file of the directory, as main does whenever the done file is there.testFileWrittenByCamelProducerWhileFilesAreLeftIsConsumedpins the Camel producer case that rewrites the done file. - The polling consumer (
pollEnrich,ConsumerTemplate.receive) is covered bytestConsumerTemplateReceivesEveryFileOfTheDoneFile, and the 4.24 upgrade guide now says the done file is kept until the last file is received. Listing one file past the limit, instead of the whole directory, keeps the cost of that path small. - The not-started path now keeps the done file of the file as polled, captured before
processExchange, so a failed download afterpreMoveno longer points at<preMoveDir>/ready.testDownloadFailingAfterPreMoveKeepsTheDoneFileOfTheDirectorycovers it.
The description mismatch I mentioned is gone too: with the gate dropped, a last file that failed is retried with the files left, and testFileThatAlwaysFailsWithFilesLeftDoesNotHoldDoneFile asserts the second attempt.
I traced the eager cut (N+1 listed, cut before sorting, so the same files the eager limit takes on main, with the extra file released from the in-progress and eager idempotent repositories), the non-eager cut, the batch stopped at shutdown, the exactly-N case (complete listing, done file deleted as before) and the browse consumer (custom processor, so no kept done files). ftp, sftp, smb and azure-files reach this through canPollMoreFiles and GenericFileConsumer.processExchange, and none of them overrides poll or processBatch. The docs change is mirrored in the catalog copy.
About the gnodet-bot review: the recursive point is not a regression. On main, only the directory of the last file of the batch loses its done file, so the done files of the other directories already stay. Here they are deleted once a complete listing finds their directory empty. The removeAll point is right but harmless (nit inline). The test count in the description is now correct.
Good to merge from my side once CI is green.
Claude Code on behalf of Claus Ibsen
allthingssecurity
deleted the
camel-file-done-file-kept-while-files-left
branch
October 19, 2026 12:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
CAMEL-25728
With a done file of a fixed name (
doneFileName=ready) the file consumer only picks up the files of a directory while the done file exists, andGenericFileOnCompletion.handleDoneFiledeletes it when the last file of the batch (CamelBatchComplete, CAMEL-5883) is committed. The batch is the poll, not the directory. Files the poll did not take then stay without their done file, and are left until a new done file is written, without a log:maxMessagesPerPoll, the files beyond the limit;Change: the poll keeps the done file of every file it did not take (beyond the limit, or not started; for a file that was not started, the file as polled, not the one a
preMovebound to the exchange), inGenericFileKeptDoneFiles(package-private), and the last file of the batch does not delete a kept done file. While it is kept, the polls consume every file of the directory, as they do on main whenever the done file is there, including a file written meanwhile (for example by a Camel producer withdoneFileName, which rewrites the done file after each file). The done file is then deleted after the last file of a poll that leaves nothing, as before, or by the next poll if a complete listing finds none of its files. A file that was taken and failed is handled as on main: the done file is deleted after the last file of the poll unless that file failed, and a failed file is tried again while the done file is there, so a file that always fails cannot hold the done file. With exactlymaxMessagesPerPollfiles nothing is left and nothing changes. Dynamic done files (${file:name}.done) andnoop=truebehave as before.pollEnrichandConsumerTemplate.receive(one file per call) keep the done file until the last file is received; on main the first received file deleted it. The FTP, SFTP, SMB and Azure Files consumers share these classes (GenericFileConsumer,GenericFileOnCompletion).Cost: with
eagerMaxMessagesPerPoll=true(the default), such a consumer lists one file pastmaxMessagesPerPollto know that the poll leaves files, and cuts the list to the same files the eager limit takes (no Exchange for the extra one). When the listing stops there, the done files of the files listed are kept (withrecursive=truethe files of another directory may not have been listed), and a kept done file is only deleted as gone after a complete listing.Limits (in memory, cleared on stop): after a restart a kept done file is treated as any done file found. A file that can never be started (for example behind a stale lock file) keeps the done file until it is gone or can be started.
Tests:
FileConsumerDoneFileNameFilesLeftTest(14 tests). On main six fail:maxMessagesPerPoll=2with three files (mock://limit ... Expected: <3> but was: <2>); the same witheagerMaxMessagesPerPoll=false(mock://noteager ... <3> but was: <2>); the read lock of b not acquired the first time (mock://notstarted ... <3> but was: <2>);maxMessagesPerPoll=1with x written while the first file is routed (mock://gate ... Expected: <4> but was: <1>); the same with x written by a Camel producer withdoneFileName=ready, which rewrites the done file (mock://producer ... <4> but was: <1>);ConsumerTemplate.receiveBodythree times on a, b, c (expected: <[A, B, C]> but was: <[null, C]>).testDownloadFailingAfterPreMoveKeepsTheDoneFileOfTheDirectory(a download failing afterpreMove) pins that the done file kept is the one of the polled directory: with the pre-moved file it deletedwork/ready.maxMessagesPerPollfiles (done file deleted; a file written meanwhile not consumed); a file failing once waits for the next done file; a last file failing once keeps the done file and is retried with the file written meanwhile; a file that always fails does not block the next batch (d andreadyagain), also withmaxMessagesPerPoll(where it is tried again with the file left, as on main);${file:name}.doneunchanged. Tests that expect a file not to be consumed check it once the done file is gone (synchronous routes), so they do not depend on timing.org.apache.camel.component.file.**) 474 run, 12 skipped, 0 failures; camel-ftp with embedded FTP and SFTP servers: 17 done-file, maxMessagesPerPoll, consumer-template and pollEnrich ITs (24 tests, includingSftpSimpleConsumeRecursiveIT),RemoteFileIgnoreDoPollErrorTestandSftpPreSortTest(14 tests), 0 failures. Formatter and import-sort checks pass.What is not changed: with asynchronous routing a poll can still run while the last file is completing (the same window as on main).
What was not run: the full root build; the whole camel-ftp suite; the camel-smb and camel-azure-files tests.
Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.(I built and tested camel-file, camel-core, camel-ftp-common and camel-ftp, including the formatter and import-sort plugins. I did not run the full root build.)
AI-assisted contributions
Co-authored-bytrailers) 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-Bytrailer.Claude Code on behalf of allthingssecurity
🤖 Generated with Claude Code