Skip to content

Fix flaky tests: prevent auto-ingest signal for Documents without pdf_file#1252

Merged
JSv4 merged 1 commit intomainfrom
fix/flaky-tests-empty-pdf-signal
Apr 14, 2026
Merged

Fix flaky tests: prevent auto-ingest signal for Documents without pdf_file#1252
JSv4 merged 1 commit intomainfrom
fix/flaky-tests-empty-pdf-signal

Conversation

@JSv4
Copy link
Copy Markdown
Collaborator

@JSv4 JSv4 commented Apr 14, 2026

Summary

  • Fixes 2 intermittently failing backend tests on main caused by the Document post_save signal triggering ingest_doc synchronously during test setup
  • Root cause: TransactionTestCase + CELERY_TASK_EAGER_PROPAGATES=True means Document.objects.create() immediately runs the full ingestion pipeline via on_commit. When pdf_file is empty (or not yet saved), chunked_parser.py:165 opens MEDIA_ROOT directory instead of a file → IsADirectoryError
  • Fix: set processing_started=timezone.now() on Document creation to skip the process_doc_on_create_atomic signal, since these tests don't need automatic ingestion

Affected tests

  • test_structural_annotations_graphql_backwards_compat.py::StructuralRelationshipGraphQLBackwardsCompatibilityTests::test_structural_relationships_accessible_after_migration
  • test_multimodal_integration.py::TestDoclingImageExtraction::test_parse_pdf_with_embedded_image

These failures are also blocking PRs #1236, #1237, and #1239.

Test plan

  • Backend CI pytest passes (the two previously failing tests should now pass consistently)
  • No other tests regress

…_file

TransactionTestCase + CELERY_TASK_EAGER_PROPAGATES causes Document post_save
signal to synchronously run ingest_doc. When pdf_file is empty/not yet saved,
chunked_parser opens MEDIA_ROOT directory instead → IsADirectoryError.

Set processing_started on Document creation to skip the signal, since these
tests don't need automatic ingestion.

Fixes intermittent failures in:
- test_structural_relationships_accessible_after_migration
- test_parse_pdf_with_embedded_image
@claude
Copy link
Copy Markdown

claude bot commented Apr 14, 2026

Code Review

Overall this is a clean, minimal, and well-targeted fix. The root cause analysis in the PR description is accurate and the solution follows the established pattern already used in worker_uploads/tasks.py, corpuses/models.py, and the _create_doc_bypass_pipeline helper in test_caml_pipeline_coverage.py.

Correctness

The fix is correct. The signal guard in opencontractserver/documents/signals.py is:

if created and not instance.processing_started:

Setting processing_started=timezone.now() on creation directly trips this guard, preventing ingest_doc from being queued before pdf_file is attached. For the multimodal integration tests, the parser is subsequently called directly (parser.parse_document(...)) after the file is saved — so the tests still exercise the full parsing pipeline as intended. For the structural annotations tests, no ingestion is needed at all since annotations are created manually.

Suggestions

1. Consider making the signal guard more defensive (separate PR / non-blocking)

The underlying issue is that the signal fires with an empty pdf_file, causing chunked_parser.py:165 to open MEDIA_ROOT as a directory. A belt-and-suspenders fix in the signal handler itself would protect production code from the same IsADirectoryError if a Document is ever created without pdf_file in a non-test context:

# In signals.py process_doc_on_create_atomic
if created and not instance.processing_started:
    if not instance.pdf_file:  # Guard: no file to ingest yet
        return
    # ... queue tasks

This is a separate concern from the test fix and shouldn't block this PR, but it would make the system more resilient.

2. Consider a shared test utility (non-blocking)

A _create_doc_bypass_pipeline helper already exists in test_caml_pipeline_coverage.py with a clear docstring explaining the pattern. If more tests need this in the future, consolidating into a shared opencontractserver/tests/helpers.py or test_utils.py would prevent the pattern from being cargo-culted without the guard comment. Not required for this fix, just something to keep in mind.

3. Minor: processing_status is left at its default

In worker_uploads/tasks.py, documents bypassing the pipeline are created with both processing_started=timezone.now() and processing_status=DocumentProcessingStatus.COMPLETED. The test documents here leave processing_status at its default (likely PENDING). This is fine for these tests since they don't assert on status, but it's worth being aware of if any future test or assertion in these classes checks that field.

Summary

The fix is correct, minimal, well-commented, and consistent with the established codebase pattern. The two suggestions above are non-blocking improvements for future consideration. Good work diagnosing the race condition.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@JSv4 JSv4 merged commit da14090 into main Apr 14, 2026
10 checks passed
@JSv4 JSv4 deleted the fix/flaky-tests-empty-pdf-signal branch April 14, 2026 04:58
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.

1 participant