CAMEL-24618: Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest by gnodet · Pull Request #26120 · apache/camel

gnodet · 2026-09-04T09:43:27Z

Problem

SpringFileAntPathMatcherRemoteFileFilterTest.testAntPatchMatcherFilter is flaky with two distinct failure modes:

expected <1> but was <9> — state contamination: The FTP consumer URI had no delete option, so consumed files remained on the FTP server. Since FtpServiceExtension.afterAll() is a no-op (the FTP server is never stopped/reset), files accumulated across test retries, causing the mock to receive duplicate messages.
expected <1> but was <0> — timing race: The consumer had initialDelay=2000ms, and with the default MockEndpoint.resultWaitTime (10s), the assertion could expire before the first poll completed on slow CI environments.

Fix

Add delete=true to the FTP consumer URI in the Spring XML context — files are deleted after processing, preventing accumulation across retries
Change initialDelay from 2000 to 0 — the consumer starts polling immediately
Set resultWaitTime(30000) on the mock endpoint — generous timeout for slow CI

utafrali

The three-line test/XML fix (delete=true, initialDelay=0, 30s wait) is correct and targeted, but the PR bundles unrelated CI/Sonar/Scalpel changes — several of which hardcode gnodet/camel and gnodet_camel project keys, drop a security hardening flag, and remove an INFRA-27808 gate. Split the PR and revert the fork-specific configuration before this can be merged.

utafrali · 2026-09-04T09:51:48Z

          -Dsonar.pullrequest.base=${{ env.pr_base_ref }}
          -Dsonar.pullrequest.key=${{ env.pr_number }}
-          -Dsonar.pullrequest.github.repository=apache/camel
+          -Dsonar.pullrequest.github.repository=gnodet/camel


This hardcodes -Dsonar.pullrequest.github.repository=gnodet/camel, -Dsonar.projectKey=gnodet_camel, and -Dsonar.organization=gnodet. If this lands on apache/camel, sonar analysis will publish to your personal fork's Sonar project instead of apache_camel. These look like local testing overrides that shouldn't be committed. Revert to apache/camel, apache_camel, apache.

utafrali · 2026-09-04T09:51:48Z

 jobs:
  sonar:
-    # Temporarily disabled until SonarCloud quality gate is adjusted (INFRA-27808)
    if: >


Removing github.repository == 'apache/camel' from the condition means the workflow will now attempt to run on every fork that receives a workflow_run event. Combined with the hardcoded gnodet/camel project key below, this looks like fork-local plumbing rather than an intentional main-branch change. Keep the repository guard.

utafrali · 2026-09-04T09:51:48Z

 jobs:
  build:
-    if: github.repository == 'apache/camel'
+    if: github.repository == 'apache/camel' || github.repository == 'gnodet/camel'


github.repository == 'apache/camel' || github.repository == 'gnodet/camel' hardcodes a personal fork into an apache/camel workflow. If you need to test on your fork, use github.repository_owner != '' or drop the guard locally, not in the upstream file.

utafrali · 2026-09-04T09:51:48Z

-
+      - name: Fetch base branch for Scalpel change detection
+        run: |
+          git fetch --deepen=200 2>/dev/null || true


The persist-credentials: false line was removed from the checkout step. This is a hardening default (OSSF Scorecard recommends it for actions/checkout) — restore it unless there is a specific reason later steps need the credential helper.

utafrali · 2026-09-04T09:51:48Z

  build:
-    # Temporarily disabled until SonarCloud quality gate is adjusted (INFRA-27808)
-    if: false && github.repository == 'apache/camel'
+    if: true


if: false && ... was replaced by if: true, dropping both the INFRA-27808 gate and the github.repository == 'apache/camel' guard in one go. The linked ticket comment above still says the workflow is disabled pending SonarCloud quality-gate work — either update/remove the comment and confirm INFRA-27808 is resolved, or keep the repo guard.

utafrali · 2026-09-04T09:51:48Z


        <!-- use myFilter as filter to allow setting ANT paths for which files to scan for -->
-        <endpoint id="myFTPEndpoint" uri="ftp://admin@localhost:${SpringFileAntPathMatcherRemoteFileFilterTest.ftpPort}/antpath?password=admin&amp;recursive=true&amp;delay=10000&amp;initialDelay=2000&amp;filter=#myAntFilter"/>
+        <endpoint id="myFTPEndpoint" uri="ftp://admin@localhost:${SpringFileAntPathMatcherRemoteFileFilterTest.ftpPort}/antpath?password=admin&amp;recursive=true&amp;delay=10000&amp;initialDelay=0&amp;delete=true&amp;filter=#myAntFilter"/>


The URI change itself is the right fix, but consider adding readLock=changed (or readLock=none with a comment explaining why it's safe here) alongside delete=true. Without a read lock, on a slow FTP handshake the consumer can begin reading a file that the producer is still writing — historically a source of a different flake class in the FTP itests. If you've already ruled this out for this test, leave it, but a one-line XML comment near delete=true explaining the state-contamination fix would help the next reader.

utafrali · 2026-09-04T09:51:48Z

    void testAntPatchMatherFilter() throws Exception {

        result.expectedBodiesReceived(expectedBody);
+        result.setResultWaitTime(30000);


Prefer the timed overload MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS) in the assertion below instead of calling setResultWaitTime(30000). It reads more clearly at the assertion site and keeps the wait budget next to the assertion it applies to. Per the project guidelines, mock-based waits should use MockEndpoint's native timed API rather than setter side-effects.

utafrali · 2026-09-04T09:51:48Z

  if [ -n "$prId" ]; then
-    echo "Fetching PR #${prId} diff..."
-    diff_body=$(fetchDiff "$prId" "$repository")
+    echo "Computing diff against origin/${GITHUB_BASE_REF:-main}..."


Switching PR diff computation from the GitHub API to a local git diff merge-base..HEAD is a meaningful behavior change (previously the diff matched exactly what GitHub shows on the PR; now it includes any local merge commits or force-push artifacts differently). Worth calling out in the PR description and testing on a PR that has been rebased/force-pushed — the two diffs can diverge when the base branch has moved forward and the PR hasn't been rebased.

utafrali · 2026-09-04T09:51:48Z

        <groupId>eu.maveniverse.maven.scalpel</groupId>
        <artifactId>extension3</artifactId>
-        <version>0.3.0</version>
+        <version>0.3.3</version>


Bumping Scalpel from 0.3.0 to 0.3.3 is unrelated to the flaky-test fix advertised in the PR title. Please split: (1) the CAMEL-24618 test fix, (2) the Scalpel bump + shadow-comparison wiring, (3) the sonar/pr-build workflow changes. Right now this PR mixes three independent changes under a title that only names one.

utafrali · 2026-09-04T09:51:48Z

@@ -0,0 +1,255 @@
+#


A new 255-line workflow (full-test-suite.yml) is landing under a PR titled "Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest". Split this out — it deserves its own PR, its own JIRA (or none if infra-only), and its own review. Bundling it here hides it from reviewers looking at the ticket scope.

github-actions · 2026-09-04T10:44:03Z

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

First-time contributors require MANUAL approval for the GitHub Actions to run
You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

github-actions · 2026-09-04T11:04:16Z

🧪 CI tested the following changed modules:

tests/camel-itest

🔬 Scalpel shadow comparison — Scalpel: 1 tested, 0 compile-only — current: 0 all tested

Maveniverse Scalpel detected 1 affected modules (current approach: 0).

⚠️ Modules only in Scalpel (1)

camel-itest

Skip-tests mode would test 1 modules (1 direct + 0 downstream), skip tests for 0 (generated code, meta-modules)

Modules Scalpel would test (1)

camel-itest

ℹ️ Shadow mode — Scalpel observes but does not affect test execution. Learn more

⚙️ View full build and test results

Fix two flakiness root causes: 1. State contamination (expected <1> but was <9>): The FTP consumer had no delete option, so files accumulated across test retries since FtpServiceExtension.afterAll() is a no-op and the FTP server directories are never reset. Added delete=true to the FTP consumer URI so files are removed after processing. 2. Timing race (expected <1> but was <0>): The consumer had initialDelay=2000ms which, combined with the default resultWaitTime, could cause the mock assertion to expire before the first poll completed on slow CI. Changed initialDelay to 0 and set an explicit resultWaitTime of 30 seconds.

gnodet · 2026-09-04T11:05:38Z

Addressed all review comments:

Unrelated CI/Sonar/Scalpel changes: The PR was rebased onto upstream/main — the diff now contains only the 2 test files. The earlier review was on a stale base that included fork-local CI changes.
Use timed assertIsSatisfied: Replaced setResultWaitTime(30000) + result.assertIsSatisfied() with MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS) — clearer and keeps the timeout at the assertion site.
Add explanatory XML comment: Added a comment above the endpoint explaining why delete=true is needed (state contamination from FtpServiceExtension never resetting the FTP directory).
readLock=changed: Not needed here — the producer writes via template.sendBodyAndHeader() which completes synchronously before the consumer polls. The consumer's initialDelay=0 + delay=10000 means the first poll happens after context start, by which point all 5 files are fully written. No partial-read risk.

gnodet marked this pull request as draft September 4, 2026 09:50

utafrali reviewed Sep 4, 2026

View reviewed changes

gnodet force-pushed the fix/CAMEL-24618 branch 2 times, most recently from 91325e3 to cbefb7f Compare September 4, 2026 10:43

github-actions Bot added the tests label Sep 4, 2026

gnodet force-pushed the fix/CAMEL-24618 branch from cbefb7f to 33bcf38 Compare September 4, 2026 11:05

apupier approved these changes Sep 4, 2026

View reviewed changes

apupier requested a review from utafrali September 4, 2026 11:54

Uh oh!

Conversation

gnodet commented Sep 4, 2026

Problem

Fix

Uh oh!

utafrali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnodet commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Sep 4, 2026 •

edited

Loading