Skip to content

Make delete-by-query conflict retry tests deterministic and guarantee writer cleanup #328

Description

@niemyjski

Observed during the production-readiness audit of #314

The full Release build succeeded (zero warnings/errors), but this run failed one existing test:

https://github.com/FoundatioFx/Foundatio.Repositories/actions/runs/35742581329/job/106795844454

  • Commit: 4fd1a96411277d2a2865f94c2cd3580fa4e9158e
  • Test: RepositoryTests.RemoveAllAsync_DeleteByQuery_AccumulatesDeletedCountAcrossConflictRetries
  • Expected deleted count: 1000; actual: 998.
  • Log: RemoveAll: 998 records removed after 11 attempts (2 unresolved version conflicts).
  • Suite result: 843 total, 842 passed, 1 failed, 0 skipped.

A subsequent full run passed after adding the independent multi-get cache regressions; the delete-by-query implementation and these tests were unchanged:
https://github.com/FoundatioFx/Foundatio.Repositories/actions/runs/35745192085

Why the current fixture is not deterministic

The tests introduced with #291 start StartConflictWriterAsync, await RemoveAllAsync, and only then cancel the writer. The writer repeatedly patches surviving documents until cancellation. Contrary to the test comment, catching DocumentNotFoundException only logs and continues; it does not stop the writer. A bounded delete retry loop therefore cannot guarantee complete deletion while that writer remains active.

The one-pass warning test also uses Task.Delay(100) to claim a guaranteed conflict. Elapsed time does not establish that a write occurred between the delete-by-query snapshot and its delete operation.

Writer shutdown is not protected by finally: if the operation under test throws, the writer may outlive the test and interfere with subsequent shared-index tests until its timeout.

This is a test-contract/scheduling problem, not evidence that the production retry budget should be increased or made unbounded. Returning a partial count and warning after exhausted retries is the documented contract.

Required repair

  1. Replace timing-dependent conflict generation with a controlled request/response sequence or an explicit synchronization seam. Prove that the first attempt has a conflict before allowing the writer to stop; make the successful-retry case quiescent before the succeeding attempt. A scripted transport fixture is appropriate for retry accounting, retry-budget, and warning assertions; retain a real-Elasticsearch integration case for ordinary deletion.
  2. Assert cumulative deletion across multiple actual attempts, exact retry-budget behavior, and one-time notification side effects. Do not weaken the expected count, skip tests, or rely on larger delays/timeouts/retry counts.
  3. Make persistent-conflict tests deterministic independently of the successful-convergence tests.
  4. Always cancel and await any writer in finally, including failure/cancellation paths.

Verification

Run the repository's CI command with the documented Elasticsearch services running:

dotnet test --solution ./Foundatio.Repositories.slnx --configuration Release --no-build --report-github

Before that command, build the same Release solution. Repeat the retry tests/full CI runs to check stability, and demonstrate that deliberately broken count accumulation or retry-budget handling causes the deterministic tests to fail. Confirm no background writer remains after a failed test.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions