Skip to content

Fixes 30119: Stabilize integration-test CI#30120

Open
harshach wants to merge 35 commits into
mainfrom
harshach/stabilize-flaky-integration-tests
Open

Fixes 30119: Stabilize integration-test CI#30120
harshach wants to merge 35 commits into
mainfrom
harshach/stabilize-flaky-integration-tests

Conversation

@harshach

@harshach harshach commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Describe your changes:

Fixes #30119

This change makes Java, Playwright, and Python integration-test outcomes deterministic without reducing parallel throughput.
It builds each Java runtime once before four concurrent isolation lanes, classifies assertion, setup, infrastructure, missing-report, cancellation, and hung-test outcomes, and retains fail-closed diagnostics.
It also fixes hard-delete cache consistency, stale context/search updates, bulk-update fallback ordering, restore/test-suite update races, and retry-dependent Oracle, Trino, and Playwright readiness checks.
The Playwright zero-retry policy remains shadow-only while stable retry signatures are collected.

Type of change:

  • Bug fix

High-level design:

  • Immutable, checksummed, source-SHA-bound Maven reactor artifact shared by profile lanes
  • Concurrent parallel, global-state, multi-node, and retry-queue Failsafe lanes with unchanged required check names
  • Shared JUnit and Playwright classifiers plus always-uploaded reports, environment data, Maven logs, dumps, and bounded container logs
  • Authoritative hard-delete markers and compare-and-delete invalidation across local/Redis cache layers
  • Conditional context processing, non-overlapping search fallback, durable per-entity relationship revisions for logical test suites, and exact-state polling
  • Transactional per-TestCase and per-logical-TestSuite revisions reject late search publications without database advisory locks
  • No blanket test reruns; shadow telemetry before any zero-retry enforcement

Tests:

Use cases covered

  • Concurrent reads cannot repopulate entities after hard delete
  • Deleted context files reject late background updates
  • Timed-out bulk indexing closes before safe individual fallback
  • Concurrent logical-suite membership updates converge in both TestCase and TestSuite search documents
  • Edited test cases publish fenced logical-suite relationship state instead of stale incremental changes
  • Java lanes fail closed for assertions, setup failures, missing reports, cancellation, and hangs
  • Playwright shards and Python suites preserve their real test outcome and diagnostics
  • Glossary term navigation waits for the populated tree state before deciding whether to expand
  • Oracle lineage, Trino rows, and Lineage UI entities wait for the exact published state

Unit tests

  • Added and updated tests for cache markers, Elasticsearch/OpenSearch bulk sinks, context extraction, entity restore, search updates, and both outcome classifiers
  • Classifier tests: 21 passed
  • Focused Java tests: 258 passed
  • Latest TestCaseRepositoryTest and SearchRepositoryTest: 22 passed
  • Coverage on changed classes: not measured separately

Backend integration tests

  • Added HardDeleteCacheConsistencyIT
  • Full MySQL/Elasticsearch parallel lane: 14,956 tests, 0 failures, 1,380 skipped
  • Logical-suite ordering regressions: 3/3 passed on MySQL/Elasticsearch and PostgreSQL/OpenSearch
  • Edited-TestCase logical-suite regression: prior behavior deterministically failed with Missing fenced relationship update; fixed behavior passed 1/1 against MySQL and Elasticsearch
  • Context Center upload/search regression: 21/21 passed on MySQL/Elasticsearch
  • Hard-delete race regression: 50 repetitions for every Include mode under parallel workers

Python integration tests

  • Latest PR head passed every MySQL and PostgreSQL integration lane: shard 1 and shard 2 on Python 3.10, 3.11, and 3.12
  • Unit/static checks passed on Python 3.10, 3.11, and 3.12
  • Branch workflow validation: MySQL/Elasticsearch and PostgreSQL both passed every lane

Ingestion integration tests

  • Updated Oracle lineage and Trino fixtures with bounded exact-state readiness

Playwright (UI) tests

Manual testing performed

  • actionlint on all 16 changed workflows
  • ShellCheck, bash -n, YAML parsing, and targeted Docker-storage helper validation
  • Maven Spotless apply/check, service build, integration-test compilation, and focused repository suites
  • UI organize-imports, ESLint, and Prettier checks
  • Real MySQL/Elasticsearch positive and negative regression runs for edited logical-suite members
  • Seven local workflow/selector/artifact/gate harnesses

UI screen recording / screenshots:

Not applicable — test synchronization changed, not product UI behavior.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>.
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have documented non-obvious behavior without adding redundant comments.
  • Not applicable: no JSON Schema changes.
  • Not applicable: no product UI changes requiring a recording or screenshots.
  • I have added tests and listed them above.
  • I have added tests that cover the exact race and outcome-classification scenarios being fixed.

Greptile Summary

This PR makes integration-test outcomes deterministic and strengthens search consistency. The main changes are:

  • Shared Java build artifacts and concurrent isolation lanes.
  • Java, Python, and Playwright outcome classifiers with retained diagnostics.
  • Hard-delete cache markers and stale-update protection.
  • Fenced test-suite relationship revisions across service nodes.
  • Bulk search recovery and token-guarded retry processing.
  • More reliable Oracle, Trino, and Playwright readiness checks.

Confidence Score: 4/5

The retry upsert should be fixed before merging.

  • Claim tokens prevent stale workers from deleting or updating newer queue rows.
  • A concurrent ordinary enqueue can still erase stored propagation context.
  • Recovery can therefore leave related search documents stale.

openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java

Important Files Changed

Filename Overview
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java Adds durable revisions and claim-token queue operations, but the retry upsert can still erase propagation context.
openmetadata-service/src/main/java/org/openmetadata/service/search/SearchIndexRetryWorker.java Uses token-guarded completion and failure transitions to preserve superseding retry records.
openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java Adds conservative bulk recovery and relationship-revision fencing.
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java Allocates relationship revisions transactionally and carries them into logical-suite search updates.

Comments Outside Diff (1)

  1. openmetadata-service/src/main/java/org/openmetadata/service/drive/ContextFileExtractionService.java, line 286-296 (link)

    P1 Retry Exhaustion Leaves Stuck State

    Three rapid optimistic-lock conflicts make this helper return false, after which callers abandon the remaining extraction transition. In the result and failure paths, the context file can remain in Analyzing indefinitely instead of reaching a terminal or retryable state.

    Context Used: CLAUDE.md (source)

Reviews (33): Last reviewed commit: "fix(migration): execute retry claim toke..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Context used:

  • Context used - CLAUDE.md (source)
  • Context used - AGENTS.md (source)

@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Jul 16, 2026
Comment thread .github/workflows/playwright-integration-tests-mysql.yml Fixed
Comment thread .github/workflows/playwright-integration-tests-postgres.yml Fixed
@harshach
harshach marked this pull request as ready for review July 16, 2026 07:45
@harshach
harshach requested review from a team, akash-jain-10 and tutte as code owners July 16, 2026 07:45
Copilot AI review requested due to automatic review settings July 16, 2026 07:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

Copilot AI review requested due to automatic review settings July 16, 2026 08:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment on lines +13493 to 13496
+ "ON DUPLICATE KEY UPDATE failureReason = VALUES(failureReason), "
+ "status = VALUES(status), entityType = VALUES(entityType), retryCount = 0, "
+ "claimedAt = NULL, claimToken = NULL",
connectionType = MYSQL)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Preserve queued propagation context

failureReason now contains the durable propagation context, but this conflict update replaces it unconditionally. If a worker claims a propagation retry and another indexing failure enqueues the same entity, the new plain failure reason clears that context and supersedes the worker's claim. The next worker reindexes the primary document but skips propagateEntityAfterRetry, leaving related search documents stale. Preserve or merge the existing propagation context when replacing a queue row.

Context Used: CLAUDE.md (source)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gitar-bot

gitar-bot Bot commented Jul 19, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 9 resolved / 10 findings

Stabilizes integration tests through deterministic outcome classification and robust cache consistency, while addressing search retry races and relationship revision fencing. Consider addressing the potential for stuck states in the context file extraction service due to retry exhaustion.

💡 Edge Case: Embedding generation failure indexes doc without vector

📄 openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/searchIndex/ElasticSearchBulkSink.java:1132-1146

When an entity's state has changed (no reusable cached embedding), enrichWithEmbedding calls generateEmbeddingFields; on a transient failure (e.g. embedding provider unavailable) the catch block returns the original json unchanged and the entity is still indexed via a full index op — stripping/omitting the vector for that document, as the surrounding comment itself warns. The failure is only surfaced in getVectorStats (counted as vectorFailed), not as a reindex failure, so affected docs remain vector-less until the next reindex. Impact is bounded (self-heals on subsequent reindex since no cached embedding will be found), but consider tracking these failures more prominently or preserving any prior vector rather than silently continuing.

✅ 9 resolved
Bug: Unbounded conditional-update retry can spin indefinitely

📄 openmetadata-service/src/main/java/org/openmetadata/service/drive/ContextFileExtractionService.java:266-280
updateFile/updateContent changed from a bounded 3-attempt loop to while (true). The loop only exits on success, a deleted/stale entity (null), or thread interruption. Under sustained write contention on the same context file/content, the extraction worker will retry forever with 10ms sleeps, permanently consuming a thread from the bounded extraction executor and starving other extraction work. Reintroduce an attempt cap (e.g. a larger bound than the old 3) so the loop terminates and the caller can fail-close instead of spinning.

Edge Case: failedRecords==0 branch ignores recorded PROCESS-stage failures

📄 openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java:1903-1908
In addConfirmedPropagationCandidates, the failedRecords == 0 early-return branch propagates every entity as confirmed without consulting failedEntityIds/failedEntityFqns. getStats().getFailedRecords() reflects only SINK-stage failures, while the failure callback also fires (and enqueues to SearchIndexRetryQueue) for PROCESS-stage doc-build failures, which are tracked separately (getProcessStats()). If the catch path is entered (e.g. a flushAndAwait timeout) while sink failedRecords is 0 but a doc-build failure was recorded, that entity is both fanned out via propagation and enqueued for retry — inconsistent with the careful gating used in the failedRecords > 0 branch. Consider excluding entities already present in failedEntityIds/failedEntityFqns even when failedRecords == 0.

Bug: Advisory lock ties up a pooled connection, risks pool starvation

📄 openmetadata-service/src/main/java/org/openmetadata/service/lock/DatabaseAdvisoryLock.java:48-57 📄 openmetadata-service/src/main/java/org/openmetadata/service/lock/DatabaseAdvisoryLock.java:59-73 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java:1221-1234
withLock() opens a JDBI Handle (a pooled connection) and holds it for the entire acquire-poll window (up to LOGICAL_SUITE_LOCK_TIMEOUT = 30s) plus the whole operation. The operation itself (addTestCasesToLogicalTestSuiteLocked → flushInOneTransaction) opens a SECOND pooled connection via Entity.getJdbi().inTransaction(...). So every in-flight logical-suite publication needs two connections, and every waiter busy-polls while holding one. Under a burst of concurrent logical-suite add/delete requests this can exhaust the HikariCP pool; the lock holder may then be unable to obtain the connection it needs to commit its transaction, producing a self-deadlock/starvation rather than clean serialization. Consider acquiring the lock on the same connection/transaction that does the work, or ensuring the pool is sized well above 2×(max concurrent logical-suite ops + waiters), and shortening the wait window.

Bug: Session advisory lock can leak to pooled connection on release failure

📄 openmetadata-service/src/main/java/org/openmetadata/service/lock/DatabaseAdvisoryLock.java:78-86
MySQL GET_LOCK and Postgres pg_try_advisory_lock are SESSION-scoped and are only auto-released when the physical session ends. Because Jdbi here is backed by a pooled DataSource, handle.close() returns the still-open connection to the pool. If release() throws, the RuntimeException is only logged and swallowed, so the advisory lock stays held on that connection when it is handed to the next borrower — leaking the lock onto an unrelated operation and potentially making the key permanently unacquirable until the connection is physically closed. On release failure, evict/discard the underlying connection (or force it closed) instead of returning it to the pool, and treat a false return / exception as a reason to invalidate the connection.

Performance: Postgres lock key hashing can over-serialize distinct suites

📄 openmetadata-service/src/main/java/org/openmetadata/service/lock/DatabaseAdvisoryLock.java:90-93
The Postgres path derives the advisory lock id via hashtextextended(:lockKey, 0), collapsing the string key into a 64-bit space. Two different logical-suite ids that hash to the same value would serialize against each other unnecessarily (correctness is unaffected, only throughput). Collisions are unlikely but worth a comment noting the intentional trade-off, so the behavioral difference from the MySQL named-lock path is documented.

...and 4 more resolved from earlier reviews

🤖 Prompt for agents
Code Review: Stabilizes integration tests through deterministic outcome classification and robust cache consistency, while addressing search retry races and relationship revision fencing. Consider addressing the potential for stuck states in the context file extraction service due to retry exhaustion.

1. 💡 Edge Case: Embedding generation failure indexes doc without vector
   Files: openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/searchIndex/ElasticSearchBulkSink.java:1132-1146

   When an entity's state has changed (no reusable cached embedding), `enrichWithEmbedding` calls `generateEmbeddingFields`; on a transient failure (e.g. embedding provider unavailable) the catch block returns the original `json` unchanged and the entity is still indexed via a full index op — stripping/omitting the vector for that document, as the surrounding comment itself warns. The failure is only surfaced in `getVectorStats` (counted as vectorFailed), not as a reindex failure, so affected docs remain vector-less until the next reindex. Impact is bounded (self-heals on subsequent reindex since no cached embedding will be found), but consider tracking these failures more prominently or preserving any prior vector rather than silently continuing.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Comment on lines 13489 to 13511
@@ -13471,22 +13511,22 @@ void upsert(
@Bind("entityType") String entityType);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Preserve queued propagation context

This conflict update still replaces failureReason unconditionally. If a queued retry contains encoded propagation context and another ordinary failure is enqueued for the same entity, the new reason removes that context. The worker then reindexes the primary document, but getPropagationContext returns empty and the required inherited-field, tag, certification, service-style, or related-entity updates are never replayed. Merge the existing propagation marker into the replacement reason when the new enqueue does not provide one.

Context Used: CLAUDE.md (source)

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

Labels

backend safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stabilize integration-test CI outcomes and recurring races

5 participants