Skip to content

fix(tests): unblock CI by stopping Testcontainers' ES wait from probing HTTPS#23

Merged
ANcpLua merged 4 commits into
mainfrom
fix/ci-unblock-orphan-tests-xmlcontent
May 16, 2026
Merged

fix(tests): unblock CI by stopping Testcontainers' ES wait from probing HTTPS#23
ANcpLua merged 4 commits into
mainfrom
fix/ci-unblock-orphan-tests-xmlcontent

Conversation

@ANcpLua
Copy link
Copy Markdown
Owner

@ANcpLua ANcpLua commented May 16, 2026

Summary

  • Testcontainers.Elasticsearch 4.11's built-in wait strategy probes ES with HttpWaitStrategy().UsingTls(configuration.TlsEnabled).
  • ElasticsearchConfiguration.TlsEnabled is false only when BOTH xpack.security.enabled AND xpack.security.http.ssl.enabled are explicitly "false". The fixture only set the first → TlsEnabled returned true → probe spoke HTTPS to a plain-HTTP server forever → WaitStrategy.WaitUntilAsync timed out at Testcontainers' 1-hour default.
  • Result: every PaperlessServices.Tests.Integration.* test failed with Collection fixture type 'SharedContainerFixture' threw in InitializeAsync / System.TimeoutException. Same failure was reproducing on main (see runs 25952721854, 25951301510) — this is the persistent CI red.
  • One-line fix: add xpack.security.http.ssl.enabled=false to the ES container env. TlsEnabled now returns false, the built-in wait probes HTTP, fixture initialises in seconds.

Verification

  • Built and ran PaperlessServices.Tests integration suite locally (Testcontainers 4.11 on ES 9.4.1 / MinIO 2025-09-07 / RabbitMQ 4.3.0):
    • Before: killed after 4 minutes still hung at SharedContainerFixture.InitializeAsync (Task.WhenAll of three StartAsync() calls).
    • After: 13/13 passed in 57s 314ms.
  • REST fixture (SharedRestContainerFixture) is unaffected — it already supplies an explicit UntilMessageIsLogged("started") wait strategy, which causes the builder to skip the buggy default.

Test plan

Notes for parallel work

Two other sessions are touching WorkerTestBase.cs:

  • chore/coverage-to-100-plus-services-kickoff reverts the three image tags back to the older versions (ES 9.1.3, MinIO 2024-11-07, RabbitMQ 4.0-alpine) and reintroduces the deprecated ElasticsearchBuilder() parameterless ctor (CS0618).
  • chore/ci-replace-pnpm-action-with-corepack carries the same fixture revert.

This PR is the forward-compatible fix: it keeps the bleeding-edge image tags and the modern ElasticsearchBuilder(image) ctor, and unblocks the wait strategy with a single additive env var. If this lands first, the fixture-revert chunks in those parallel PRs become unnecessary churn — leave only their non-overlapping work.

🤖 Generated with Claude Code

…h wait uses HTTP

Testcontainers.Elasticsearch 4.11's built-in wait strategy probes ES via
`new HttpWaitStrategy().UsingTls(configuration.TlsEnabled)`. TlsEnabled is
the AND of two env vars: `xpack.security.enabled` AND
`xpack.security.http.ssl.enabled`, both explicitly "false". The fixture
only set the first, so TlsEnabled returned true, the wait probed HTTPS,
ES (security disabled) only answered plain HTTP, and the probe never
satisfied — every PaperlessServices integration test failed with
`System.TimeoutException` after Testcontainers' 1-hour default.

Adding the second env var makes TlsEnabled return false, the wait
probes HTTP, and the fixture completes in seconds.

Verified locally on ES 9.4.1 / MinIO 2025-09-07 / RabbitMQ 4.3.0 with
Testcontainers 4.11 — 13/13 integration tests pass in 57s (vs. hanging
forever before).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 16, 2026 06:37
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6b12985f-69ac-408e-b084-017bb35a211d

📥 Commits

Reviewing files that changed from the base of the PR and between d2a65ee and 6ae05bc.

📒 Files selected for processing (1)
  • PaperlessServices.Tests/Integration/WorkerTestBase.cs
📜 Recent review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Build & Test (backend)
🧰 Additional context used
📓 Path-based instructions (2)
PaperlessServices.Tests/Integration/**/*.cs

📄 CodeRabbit inference engine (CLAUDE.md)

Mock ITextSummarizer in integration tests with FakeTextSummarizer; do not use the real Gemini API even with placeholder keys

Files:

  • PaperlessServices.Tests/Integration/WorkerTestBase.cs
**/*.cs

📄 CodeRabbit inference engine (.editorconfig)

**/*.cs: Do not qualify event, field, method, and property access with 'this.' in C# code
Use predefined types (like int, string) instead of BCL types (like Int32, String) in C#
Always use parentheses for clarity in arithmetic and relational binary operators in C#
Require explicit accessibility modifiers for non-interface members in C#
Prefer coalesce expressions, collection initializers, null propagation, and object initializers in C#
Prefer auto-properties and compound assignments in C#
Prefer conditional expressions over assignment and return statements in C#
Mark fields as readonly when possible in C#
Treat all unused parameters as code quality issues in C#
Use explicit types instead of 'var' in C# unless type is apparent
Prefer expression-bodied members for accessors, indexers, lambdas, methods, and properties in C#
Use pattern matching instead of 'as' with null checks and 'is' with cast checks in C#
Prefer switch expressions over traditional switch statements in C#
Use conditional delegate calls in C# to avoid null reference exceptions
Prefer static local functions over instance local functions in C#
Use modifier order: public, private, protected, internal, file, static, extern, new, virtual, abstract, sealed, override, readonly, unsafe, required, volatile, async in C#
Always use braces for code blocks in C#
Prefer file-scoped namespaces in C#
Prefer implicit object creation when type is apparent in C#
Prefer index and range operators in C#
Place 'using' directives outside of namespace declarations in C#
Always place opening brace on new line before catch, else, finally, and members in C#
Indent block contents and case contents in C#
Add space after comma, before and after binary operators, and after colon in inheritance clause in C#
Do not add space after dot, cast, or before open square brackets in C#
Name interfaces with 'I' prefix using PascalCase in C#
Name types (classes, structs, enums) using PascalCase in C#
Name non-field members (properties, events...

Files:

  • PaperlessServices.Tests/Integration/WorkerTestBase.cs
🔇 Additional comments (4)
PaperlessServices.Tests/Integration/WorkerTestBase.cs (4)

35-38: Comment claims implementation details that don't match Testcontainers.Elasticsearch.

The comment asserts ElasticsearchConfiguration.TlsEnabled evaluates via an AND of two env vars. Per the Testcontainers.Elasticsearch module, there's no explicit TlsEnabled property with that logic—the library determines TLS based on container security state, not a direct AND of these flags. The env var addition is correct; the explanatory comment is speculative and may mislead future maintainers.

Suggested replacement:

-		// Required so Testcontainers' ElasticsearchConfiguration.TlsEnabled evaluates to false
-		// (it AND-s xpack.security.enabled with xpack.security.http.ssl.enabled). Without this,
-		// the built-in wait strategy probes HTTPS while ES listens on plain HTTP, and hangs.
+		// Disables HTTPS on Elasticsearch. Without this, the built-in wait strategy
+		// may probe HTTPS while ES listens on HTTP (due to xpack.security.enabled=false),
+		// causing startup hangs.
		.WithEnvironment("xpack.security.http.ssl.enabled", "false")

195-198: Stated 30s budget is not enforced for the final search.

The comment on lines 195-197 documents a "30s overall budget," but the final SearchAsync at line 252 uses only the caller's cancellationToken. If ES is slow, this final call can run unboundedly past the advertised timeout.

If preserving "found nothing" semantics for assertions is the goal, capture lastResponse during the loop and return it on timeout instead of issuing an unbound final call:

+	SearchResponse<T>? lastResponse = null;

 	while (!overallLinked.Token.IsCancellationRequested)
 	{
 		try
 		{
 			SearchResponse<T> response = await client.SearchAsync<T>(configureSearch, overallLinked.Token);
+			lastResponse = response;

 			if (response.Documents.Count > 0)
 			{
 				return response;
 			}
 		}
 		...
 	}

-	// Final attempt with the caller's token only so the assertion sees real
-	// "found nothing" data rather than a TaskCanceledException at the wait boundary.
-	return await client.SearchAsync<T>(configureSearch, cancellationToken);
+	if (lastResponse is not null)
+	{
+		return lastResponse;
+	}
+	cancellationToken.ThrowIfCancellationRequested();
+	throw new TimeoutException("Timed out waiting for Elasticsearch search results.");

Also applies to: 250-252


213-222: LGTM!


224-248: LGTM!


📝 Walkthrough

Summary by CodeRabbit

  • Tests
    • Test container config explicitly disables HTTP-layer TLS for Elasticsearch to prevent protocol misdetection during test startups.
    • xUnit runner parallelism changed to "conservative" to reduce concurrency and improve stability.
    • Integration test polling for search results improved: increased overall timeout, upfront index refresh, and stronger cancellation handling to reduce flakes and preserve expected assertions.

Walkthrough

Disable Elasticsearch HTTP TLS probing in the Testcontainers fixture; rework WaitForSearchResultsAsync to use a 30s overall timeout linked to the caller token with cancellation-aware refresh/search/delay and a final search using the caller token; set xUnit parallelAlgorithm to conservative.

Changes

Test runtime and polling updates

Layer / File(s) Summary
Elasticsearch Testcontainers HTTP TLS flag
PaperlessServices.Tests/Integration/WorkerTestBase.cs
SharedContainerFixture sets xpack.security.http.ssl.enabled = false so Testcontainers' TLS detection/wait logic does not probe HTTPS against a plain-HTTP Elasticsearch.
WaitForSearchResultsAsync polling behavior
PaperlessServices.Tests/Integration/WorkerTestBase.cs
Default overall timeout raised to 30s; overall CancellationTokenSource linked to caller token used; upfront Indices.RefreshAsync added; SearchAsync and polling Task.Delay are cancellation-aware and wrapped to break cleanly on cancellation; final search after loop uses only the caller's cancellation token.
xUnit runner parallelism
PaperlessServices.Tests/xunit.runner.json
xUnit parallelAlgorithm changed from aggressive to conservative.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the primary fix: disabling Testcontainers' HTTPS probing by setting xpack.security.http.ssl.enabled to false, addressing the root cause of CI failures.
Description check ✅ Passed The description provides thorough technical context: root cause analysis of the TlsEnabled logic, reproduction steps, the specific fix applied, local verification with test results, and notes on parallel work and forward compatibility.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ci-unblock-orphan-tests-xmlcontent
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/ci-unblock-orphan-tests-xmlcontent

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Unblocks PaperlessServices.Tests integration CI by ensuring Testcontainers’ Elasticsearch wait strategy probes Elasticsearch over plain HTTP (not HTTPS) when the container is configured with security/TLS disabled.

Changes:

  • Adds xpack.security.http.ssl.enabled=false to the Elasticsearch Testcontainers environment in the shared integration-test fixture.
  • Documents why the extra env var is required (to keep ElasticsearchConfiguration.TlsEnabled evaluating to false and avoid HTTPS probing hangs).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codacy-production
Copy link
Copy Markdown

codacy-production Bot commented May 16, 2026

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · -2 duplication

Metric Results
Complexity 0
Duplication -2

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

Copy link
Copy Markdown

@codacy-production codacy-production Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

The PR successfully addresses the CI blockage by setting 'xpack.security.http.ssl.enabled' to 'false' in the Elasticsearch container configuration within 'WorkerTestBase.cs'. This ensures that the Testcontainers library's built-in wait strategy probes via plain HTTP, preventing the System.TimeoutException previously encountered during container initialization. Codacy analysis indicates the changes are up to standards with no new issues, clones, or complexity increases.

Test suggestions

  • Elasticsearch container in WorkerTestBase initializes successfully without timing out
  • PaperlessServices.Tests.Integration suite passes successfully

TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@PaperlessServices.Tests/Integration/WorkerTestBase.cs`:
- Around line 35-38: Remove the inaccurate reference to
ElasticsearchConfiguration.TlsEnabled and replace the existing comment above the
.WithEnvironment("xpack.security.http.ssl.enabled", "false") call with a
concise, accurate explanation that setting xpack.security.http.ssl.enabled=false
disables HTTPS for Elasticsearch and that without it the wait strategy may probe
HTTPS while Elasticsearch is listening on HTTP (when
xpack.security.enabled=false), which causes the startup hang; keep the
.WithEnvironment("xpack.security.http.ssl.enabled", "false") line unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 018aeec7-1b12-4585-b41e-9585bdaec617

📥 Commits

Reviewing files that changed from the base of the PR and between 9da5146 and 86b123f.

📒 Files selected for processing (1)
  • PaperlessServices.Tests/Integration/WorkerTestBase.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: copilot-pull-request-reviewer
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Build & Test (backend)
🧰 Additional context used
📓 Path-based instructions (2)
PaperlessServices.Tests/Integration/**/*.cs

📄 CodeRabbit inference engine (CLAUDE.md)

Mock ITextSummarizer in integration tests with FakeTextSummarizer; do not use the real Gemini API even with placeholder keys

Files:

  • PaperlessServices.Tests/Integration/WorkerTestBase.cs
**/*.cs

📄 CodeRabbit inference engine (.editorconfig)

**/*.cs: Do not qualify event, field, method, and property access with 'this.' in C# code
Use predefined types (like int, string) instead of BCL types (like Int32, String) in C#
Always use parentheses for clarity in arithmetic and relational binary operators in C#
Require explicit accessibility modifiers for non-interface members in C#
Prefer coalesce expressions, collection initializers, null propagation, and object initializers in C#
Prefer auto-properties and compound assignments in C#
Prefer conditional expressions over assignment and return statements in C#
Mark fields as readonly when possible in C#
Treat all unused parameters as code quality issues in C#
Use explicit types instead of 'var' in C# unless type is apparent
Prefer expression-bodied members for accessors, indexers, lambdas, methods, and properties in C#
Use pattern matching instead of 'as' with null checks and 'is' with cast checks in C#
Prefer switch expressions over traditional switch statements in C#
Use conditional delegate calls in C# to avoid null reference exceptions
Prefer static local functions over instance local functions in C#
Use modifier order: public, private, protected, internal, file, static, extern, new, virtual, abstract, sealed, override, readonly, unsafe, required, volatile, async in C#
Always use braces for code blocks in C#
Prefer file-scoped namespaces in C#
Prefer implicit object creation when type is apparent in C#
Prefer index and range operators in C#
Place 'using' directives outside of namespace declarations in C#
Always place opening brace on new line before catch, else, finally, and members in C#
Indent block contents and case contents in C#
Add space after comma, before and after binary operators, and after colon in inheritance clause in C#
Do not add space after dot, cast, or before open square brackets in C#
Name interfaces with 'I' prefix using PascalCase in C#
Name types (classes, structs, enums) using PascalCase in C#
Name non-field members (properties, events...

Files:

  • PaperlessServices.Tests/Integration/WorkerTestBase.cs

Comment on lines +35 to +38
// Required so Testcontainers' ElasticsearchConfiguration.TlsEnabled evaluates to false
// (it AND-s xpack.security.enabled with xpack.security.http.ssl.enabled). Without this,
// the built-in wait strategy probes HTTPS while ES listens on plain HTTP, and hangs.
.WithEnvironment("xpack.security.http.ssl.enabled", "false")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify Testcontainers.Elasticsearch TlsEnabled logic

# Search Testcontainers .NET repository for ElasticsearchConfiguration implementation
rg -nP --type=cs -A10 -B2 'class ElasticsearchConfiguration|TlsEnabled.*get' --iglob '*elasticsearch*'

Repository: ANcpLua/Paperless

Length of output: 43


🌐 Web query:

Testcontainers.Elasticsearch 4.11 ElasticsearchConfiguration TlsEnabled implementation

💡 Result:

In Testcontainers for.NET (including version 4.11.0), there is no explicit property named TlsEnabled within an ElasticsearchConfiguration class [1][2]. The Testcontainers Elasticsearch module automatically configures the container to use HTTPS by default for modern Elasticsearch versions (8.x and later), which are secure by default [2][3]. Because the container runs with TLS enabled,.NET clients attempting to connect will typically fail unless the client is configured to trust the self-signed certificate generated by the container [2]. To handle this in your.NET application, you should: 1. Create an ElasticsearchClientSettings instance [2]. 2. Set the ServerCertificateValidationCallback delegate to CertificateValidations.AllowAll to bypass certificate validation for your test environment [2]. Example implementation: var clientSettings = new ElasticsearchClientSettings(new Uri(_elasticsearchContainer.GetConnectionString)); clientSettings.ServerCertificateValidationCallback(CertificateValidations.AllowAll); var client = new ElasticsearchClient(clientSettings); If you need to disable security (and thus TLS) for specific testing scenarios, you can typically do so by modifying the environment variables of the container (e.g., setting xpack.security.enabled to false), though this is generally discouraged in favor of proper certificate handling [3][4][5]. Recent updates to the.NET module have improved support for handling these configurations, including returning appropriate HTTP/HTTPS connection strings based on the security state [4].

Citations:


Remove the inaccurate comment about ElasticsearchConfiguration.TlsEnabled and provide a correct explanation.

The comment references ElasticsearchConfiguration.TlsEnabled and describes an "AND" logic between two environment variables that does not match Testcontainers.Elasticsearch 4.11 implementation. TlsEnabled is not an explicit property that evaluates based on those flags. The environment variables set here control Elasticsearch security configuration directly. Replace the comment with a simpler, accurate explanation:

// xpack.security.http.ssl.enabled=false disables HTTPS for Elasticsearch.
// Without this, the built-in wait strategy probes HTTPS while ES listens on HTTP (due to xpack.security.enabled=false), causing hangs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@PaperlessServices.Tests/Integration/WorkerTestBase.cs` around lines 35 - 38,
Remove the inaccurate reference to ElasticsearchConfiguration.TlsEnabled and
replace the existing comment above the
.WithEnvironment("xpack.security.http.ssl.enabled", "false") call with a
concise, accurate explanation that setting xpack.security.http.ssl.enabled=false
disables HTTPS for Elasticsearch and that without it the wait strategy may probe
HTTPS while Elasticsearch is listening on HTTP (when
xpack.security.enabled=false), which causes the startup hang; keep the
.WithEnvironment("xpack.security.http.ssl.enabled", "false") line unchanged.

ANcpLua and others added 2 commits May 16, 2026 08:58
…llection serializes

xUnit v3's parallelAlgorithm: aggressive submits every test to the
parallel thread pool simultaneously, ignoring collection boundaries —
even tests sharing `[Collection(SharedContainerCollection.Name)]` end
up running in parallel against the same Elasticsearch instance.

Result: OcrIntegrationTests.ProcessMultipleDocuments_Concurrently
spam-indexes documents while
SearchIndexIntegrationTests.MultipleDocuments_SearchCorrectly is
polling for its own write to become visible. On a slow CI disk, the
write/refresh cycle stalls and the poll's 10s cap is reached — the
test fails deterministically with TaskCanceledException at exactly
10s 047ms (visible in run 25955174937).

Conservative scheduling keeps tests in the SharedContainer collection
sequential, eliminating the cross-test ES write contention. Local
13/13 integration tests now run in 37s (down from 57s under
aggressive — less contention even when nothing fails).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… slow single call

The polling helper used a single 10s linked-token across every SearchAsync
call. On GitHub-hosted runners the first search after index creation can
spend several seconds priming Lucene query caches even after the
preceding Refresh.True has returned — eating the budget and surfacing as
the deterministic 10s 047ms TaskCanceledException at line 90 of
SearchIndexIntegrationTests.MultipleDocuments_SearchCorrectly.

Two changes:

- bump the default overall timeout from 10s → 30s. CI runners are slower
  than local dev machines and 10s was set when the suite was running on
  beefier hardware. 30s is still tight for a happy-path search.
- catch OperationCanceledException inside the loop and exit cleanly so
  the caller's final attempt (with the caller's own token) decides
  pass/fail. Previously a cancelled poll surfaced as
  TaskCanceledException at the helper boundary, hiding whether the doc
  was actually missing or just not yet visible.

Local 5/5 SearchIndexIntegrationTests pass in 41s on ES 9.4.1.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 16, 2026 07:08
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@PaperlessServices.Tests/Integration/WorkerTestBase.cs`:
- Around line 195-198: The final SearchAsync call can run beyond the intended
overall timeout because it only uses cancellationToken; fix by enforcing the
timeout for the entire method: create a CancellationTokenSource with the
computed timeout (timeout ??= TimeSpan.FromSeconds(30)) and link it with the
incoming cancellationToken (e.g., using
CancellationTokenSource.CreateLinkedTokenSource), then pass the linked token
into the final SearchAsync calls instead of plain cancellationToken; apply the
same change to the other occurrences mentioned (the similar SearchAsync at the
232–234 region) to preserve the "final response" semantics while ensuring the
30s budget is honored.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 77a6fbe9-a636-4123-9e61-daf7b4a19e9c

📥 Commits

Reviewing files that changed from the base of the PR and between be2e216 and d2a65ee.

📒 Files selected for processing (1)
  • PaperlessServices.Tests/Integration/WorkerTestBase.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: copilot-pull-request-reviewer
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Build & Test (backend)
🧰 Additional context used
📓 Path-based instructions (2)
PaperlessServices.Tests/Integration/**/*.cs

📄 CodeRabbit inference engine (CLAUDE.md)

Mock ITextSummarizer in integration tests with FakeTextSummarizer; do not use the real Gemini API even with placeholder keys

Files:

  • PaperlessServices.Tests/Integration/WorkerTestBase.cs
**/*.cs

📄 CodeRabbit inference engine (.editorconfig)

**/*.cs: Do not qualify event, field, method, and property access with 'this.' in C# code
Use predefined types (like int, string) instead of BCL types (like Int32, String) in C#
Always use parentheses for clarity in arithmetic and relational binary operators in C#
Require explicit accessibility modifiers for non-interface members in C#
Prefer coalesce expressions, collection initializers, null propagation, and object initializers in C#
Prefer auto-properties and compound assignments in C#
Prefer conditional expressions over assignment and return statements in C#
Mark fields as readonly when possible in C#
Treat all unused parameters as code quality issues in C#
Use explicit types instead of 'var' in C# unless type is apparent
Prefer expression-bodied members for accessors, indexers, lambdas, methods, and properties in C#
Use pattern matching instead of 'as' with null checks and 'is' with cast checks in C#
Prefer switch expressions over traditional switch statements in C#
Use conditional delegate calls in C# to avoid null reference exceptions
Prefer static local functions over instance local functions in C#
Use modifier order: public, private, protected, internal, file, static, extern, new, virtual, abstract, sealed, override, readonly, unsafe, required, volatile, async in C#
Always use braces for code blocks in C#
Prefer file-scoped namespaces in C#
Prefer implicit object creation when type is apparent in C#
Prefer index and range operators in C#
Place 'using' directives outside of namespace declarations in C#
Always place opening brace on new line before catch, else, finally, and members in C#
Indent block contents and case contents in C#
Add space after comma, before and after binary operators, and after colon in inheritance clause in C#
Do not add space after dot, cast, or before open square brackets in C#
Name interfaces with 'I' prefix using PascalCase in C#
Name types (classes, structs, enums) using PascalCase in C#
Name non-field members (properties, events...

Files:

  • PaperlessServices.Tests/Integration/WorkerTestBase.cs
🔇 Additional comments (1)
PaperlessServices.Tests/Integration/WorkerTestBase.cs (1)

35-38: No actionable issue in this hunk.

Comment on lines +195 to +198
// 30s overall budget: GitHub-hosted runners are markedly slower than local
// dev machines and the first SearchAsync after index creation can spend
// several seconds priming query caches even after Refresh.True returns.
timeout ??= TimeSpan.FromSeconds(30);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Timeout contract is no longer enforced after loop exit.

The method states a 30s overall budget, but the final SearchAsync uses only cancellationToken, so it can run past the budget and recreate CI tail latency/hangs under ES slowness.

Proposed fix (preserve “final response” semantics without exceeding budget)
 public async Task<SearchResponse<T>> WaitForSearchResultsAsync<T>(
 	Action<SearchRequestDescriptor<T>> configureSearch,
 	CancellationToken cancellationToken,
 	TimeSpan? timeout = null,
 	TimeSpan? pollInterval = null)
 {
 	// 30s overall budget: GitHub-hosted runners are markedly slower than local
 	// dev machines and the first SearchAsync after index creation can spend
 	// several seconds priming query caches even after Refresh.True returns.
 	timeout ??= TimeSpan.FromSeconds(30);
 	pollInterval ??= TimeSpan.FromMilliseconds(100);

 	ElasticsearchClient client = Services.GetRequiredService<ElasticsearchClient>();
 	using CancellationTokenSource overallCts = new(timeout.Value);
 	using CancellationTokenSource overallLinked =
 		CancellationTokenSource.CreateLinkedTokenSource(overallCts.Token, cancellationToken);
+	SearchResponse<T>? lastResponse = null;

 	while (!overallLinked.Token.IsCancellationRequested)
 	{
 		try
 		{
 			SearchResponse<T> response = await client.SearchAsync<T>(configureSearch, overallLinked.Token);
+			lastResponse = response;

 			if (response.Documents.Count > 0)
 			{
 				return response;
 			}
 		}
 		catch (OperationCanceledException) when (overallLinked.Token.IsCancellationRequested)
 		{
 			break;
 		}

 		try
 		{
 			await Task.Delay(pollInterval.Value, overallLinked.Token);
 		}
 		catch (OperationCanceledException)
 		{
 			break;
 		}
 	}

-	// Final attempt with the caller's token only so the assertion sees real
-	// "found nothing" data rather than a TaskCanceledException at the wait boundary.
-	return await client.SearchAsync<T>(configureSearch, cancellationToken);
+	// Return the most recent real response if available, keeping the overall budget strict.
+	if (lastResponse is not null)
+	{
+		return lastResponse;
+	}
+
+	// If cancellation came from caller, preserve that behavior.
+	cancellationToken.ThrowIfCancellationRequested();
+	throw new TimeoutException("Timed out waiting for Elasticsearch search results.");
 }

Also applies to: 232-234

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@PaperlessServices.Tests/Integration/WorkerTestBase.cs` around lines 195 -
198, The final SearchAsync call can run beyond the intended overall timeout
because it only uses cancellationToken; fix by enforcing the timeout for the
entire method: create a CancellationTokenSource with the computed timeout
(timeout ??= TimeSpan.FromSeconds(30)) and link it with the incoming
cancellationToken (e.g., using CancellationTokenSource.CreateLinkedTokenSource),
then pass the linked token into the final SearchAsync calls instead of plain
cancellationToken; apply the same change to the other occurrences mentioned (the
similar SearchAsync at the 232–234 region) to preserve the "final response"
semantics while ensuring the 30s budget is honored.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment on lines 6 to 8
"parallelizeTestCollections": true,
"parallelAlgorithm": "aggressive",
"parallelAlgorithm": "conservative",
"maxParallelThreads": "4x",
Comment on lines +195 to +204
// 30s overall budget: GitHub-hosted runners are markedly slower than local
// dev machines and the first SearchAsync after index creation can spend
// several seconds priming query caches even after Refresh.True returns.
timeout ??= TimeSpan.FromSeconds(30);
pollInterval ??= TimeSpan.FromMilliseconds(100);

ElasticsearchClient client = Services.GetRequiredService<ElasticsearchClient>();
using CancellationTokenSource cts = new(timeout.Value);
using CancellationTokenSource linked =
CancellationTokenSource.CreateLinkedTokenSource(cts.Token, cancellationToken);
using CancellationTokenSource overallCts = new(timeout.Value);
using CancellationTokenSource overallLinked =
CancellationTokenSource.CreateLinkedTokenSource(overallCts.Token, cancellationToken);
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d2a65ee804

ℹ️ 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".

Comment on lines +232 to 234
// Final attempt with the caller's token only so the assertion sees real
// "found nothing" data rather than a TaskCanceledException at the wait boundary.
return await client.SearchAsync<T>(configureSearch, cancellationToken);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor polling timeout on final Elasticsearch search

When the 30s budget expires during SearchAsync, the new OperationCanceledException handler breaks out of the loop and then issues a final SearchAsync with only the caller token. If the caller token is still active, this extra request is no longer bounded by the polling timeout and can block for the transport/request timeout, so a method documented as waiting "until results are found or timeout occurs" can run much longer than timeout in failing CI scenarios.

Useful? React with 👍 / 👎.

…sync polls

SearchIndexService writes documents with Refresh.True (`?refresh=true`),
which the Elastic 9.x docs describe as forcing a refresh of the affected
shards before the index call returns. In practice, on GitHub-hosted
ubuntu-latest runners the per-document refresh has been observed to not
fully propagate before the first SearchAsync hits the index — the doc is
visible to a real-time GET (used by WaitForDocumentAsync, which passes
on CI) but invisible to _search (used by WaitForSearchResultsAsync,
which fails).

Calling client.Indices.RefreshAsync at the start of the polling helper
forces an explicit index-level refresh. It's idempotent: on local dev
machines where the per-document refresh already settled, this is a fast
no-op; on a slow CI runner it converts a deterministic empty-result
flake (MultipleDocuments_SearchCorrectly was failing at exactly
10s 047ms with TaskCanceledException, then post-timeout-bump at 30s
with "collection is empty") into a passing search.

The refresh failure is caught and ignored — the polling loop below is
the actual correctness boundary; the refresh is best-effort warmup.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ANcpLua ANcpLua merged commit e8c71ad into main May 16, 2026
5 of 6 checks passed
@ANcpLua ANcpLua deleted the fix/ci-unblock-orphan-tests-xmlcontent branch May 16, 2026 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants