Skip to content

Fixes #30491: Bound async DB fan-out and prepare JDK 25 runtime#30492

Open
harshach wants to merge 16 commits into
mainfrom
harshach/jetty-virtual-threads-db-pool
Open

Fixes #30491: Bound async DB fan-out and prepare JDK 25 runtime#30492
harshach wants to merge 16 commits into
mainfrom
harshach/jetty-virtual-threads-db-pool

Conversation

@harshach

@harshach harshach commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Describe your changes:

Fixes #30491

I bounded top-level DB-heavy AsyncService work because unbounded request fan-out could exhaust the shared HikariCP pool; the change also exposes background executor limits, adds queue metrics and a connection-budget ledger, upgrades HikariCP to 7.1.0, removes unused pool keys, and moves Alpine server runtimes to JDK 25 while keeping Java 21 bytecode and enableVirtualThreads disabled by default.

Type of change:

  • Improvement

High-level design:

The new fair-semaphore executor view acquires permits inside virtual threads, queues excess operations without rejection, and offers 0 as an unbounded rollback setting.
Internal fire-and-forget work and nested continuations stay on the raw executor to prevent permit deadlocks, while startup injects explicit defaults into the remaining DB-using background executors.
The legacy minimal-ubuntu image remains on JDK 21 because its Ubuntu Xenial base cannot provide JDK 25.

Tests:

Use cases covered

  • Top-level async DB work never exceeds its configured concurrency limit
  • Excess work queues FIFO and eventually executes without rejection
  • A throwing task releases its permit
  • Setting the async limit to 0 restores unbounded behavior
  • Background worker and lifecycle-lane overrides preserve existing defaults

Unit tests

  • Added and updated unit tests for the changed executor and configuration logic
  • 31 focused tests passed across BoundedAsyncExecutorTest, AsyncServiceTest, BackgroundExecutorsConfigurationTest, GenericBackgroundWorkerTest, OrderedLaneExecutorTest, and LoggingConfigurationYamlTest

Backend integration tests

  • Existing full integration suite exercised; no new API endpoints
  • Environment-invalid result: MinIO returned HTTP 507 under disk pressure, and transient Docker/Testcontainers timing failures passed in focused reruns

Ingestion integration tests

  • Not applicable (no ingestion changes)

Playwright (UI) tests

  • Not applicable (no UI changes)

Manual testing performed

  • mvn spotless:apply
  • mvn -q dependency:get -Dartifact=com.zaxxer:HikariCP:7.1.0
  • Installed openjdk25-jre from alpine:3 and verified OpenJDK 25.0.3

UI screen recording / screenshots:

Not applicable.

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 commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
  • For UI changes: I attached a screen recording and/or screenshots above.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.

Improvement

  • I have added tests around the new logic.
  • I updated the developer documentation.

Greptile Summary

This PR bounds DB-heavy asynchronous work and updates related runtime configuration.

  • Adds a semaphore-backed executor view and routes top-level DB-heavy operations through it.
  • Exposes concurrency limits and queue metrics for asynchronous and background executors.
  • Updates search-reindex timeout coordination, HikariCP, server runtime images, tests, and connection-budget documentation.

Confidence Score: 3/5

The PR is not yet safe to merge because a timed-out reindex can continue performing database and search operations after cancellation is reported.

The admission wait is now separated from execution time, but future cancellation only interrupts the worker; its broad per-entity exception handler consumes interruption-related failures and continues processing later entities.

Files Needing Attention: openmetadata-service/src/main/java/org/openmetadata/service/resources/search/SearchResource.java

Important Files Changed

Filename Overview
openmetadata-service/src/main/java/org/openmetadata/service/util/BoundedAsyncExecutor.java Adds fair semaphore admission, cancellation of interrupted submitted Futures, queue metrics, and delegate lifecycle forwarding.
openmetadata-service/src/main/java/org/openmetadata/service/util/AsyncService.java Exposes raw and bounded views over the shared virtual-thread executor and registers concurrency gauges.
openmetadata-service/src/main/java/org/openmetadata/service/resources/search/SearchResource.java Delays the reindex execution timeout until admission, but timeout cancellation can be swallowed by the per-entity loop so work continues past the deadline.
openmetadata-service/src/main/java/org/openmetadata/service/config/BackgroundExecutorsConfiguration.java Adds validated configuration for change events, lifecycle lanes, activity tracking, and background job workers.
openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplication.java Initializes asynchronous concurrency budgets before repositories and passes configured worker limits into managed services.

Sequence Diagram

sequenceDiagram
  participant API as SearchResource
  participant Bound as Bounded executor
  participant Worker as Reindex worker
  participant Watchdog as Timeout watchdog
  participant DB as Database
  participant Search as Search backend
  API->>Bound: submit reindex
  API->>Watchdog: submit timeout monitor
  Bound->>Worker: admit after permit
  Worker->>Watchdog: signal executionStarted
  loop Each entity
    Worker->>DB: load entity
    Worker->>Search: delete and recreate document
  end
  Watchdog->>Worker: cancel(true) after timeout
  Worker-->>Worker: catch exception and continue loop
Loading

Reviews (16): Last reviewed commit: "Merge origin/main into harshach/jetty-vi..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Context used (3)

@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Jul 26, 2026
@harshach
harshach marked this pull request as ready for review July 26, 2026 16:56
@harshach
harshach requested review from a team, akash-jain-10 and tutte as code owners July 26, 2026 16:56
Copilot AI review requested due to automatic review settings July 26, 2026 16:56

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.

Pull request overview

This PR introduces a bounded “DB-heavy” async execution lane to prevent unbounded fan-out from exhausting the shared HikariCP pool, while also exposing configurable background executor limits and preparing Alpine-based server images for a JDK 25 runtime (keeping Java 21 bytecode and virtual threads disabled by default).

Changes:

  • Add a bounded async executor view for top-level DB-heavy operations (with Micrometer gauges for active/queued/limit) and switch key async entry points to use it.
  • Add explicit configuration blocks for async DB limits and background executor parallelism/lanes/permits/workers; initialize these budgets early during app startup.
  • Upgrade HikariCP to 7.1.0 and update Alpine runtime images to install openjdk25-jre.

Reviewed changes

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

Show a summary per file
File Description
pom.xml Bumps HikariCP version to 7.1.0.
openmetadata-service/src/main/java/org/openmetadata/service/util/BoundedAsyncExecutor.java Adds a semaphore-gated ExecutorService view for bounded concurrency.
openmetadata-service/src/main/java/org/openmetadata/service/util/AsyncService.java Adds bounded executor lane, config-driven initialization, and Micrometer gauges.
openmetadata-service/src/main/java/org/openmetadata/service/security/auth/UserActivityTracker.java Makes DB permit semaphore configurable and adds explicit initialization.
openmetadata-service/src/main/java/org/openmetadata/service/resources/search/SearchResource.java Routes reindex worker to bounded lane; keeps watchdog on raw lane.
openmetadata-service/src/main/java/org/openmetadata/service/resources/lineage/LineageResource.java Routes lineage export async work to bounded lane.
openmetadata-service/src/main/java/org/openmetadata/service/resources/glossary/GlossaryTermResource.java Routes glossary move async work to bounded lane.
openmetadata-service/src/main/java/org/openmetadata/service/resources/EntityResource.java Routes async delete/restore and bulk async ops to bounded lane.
openmetadata-service/src/main/java/org/openmetadata/service/resources/columns/ColumnResource.java Routes async column import/update to bounded lane.
openmetadata-service/src/main/java/org/openmetadata/service/resources/apps/AppResource.java Routes async app delete to bounded lane.
openmetadata-service/src/main/java/org/openmetadata/service/resources/ai/AuditPackGenerator.java Routes audit pack generation to bounded lane.
openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplicationConfig.java Adds new config sections: asyncOperations and backgroundExecutors.
openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplication.java Initializes async/background budgets early and wires configurable background job workers.
openmetadata-service/src/main/java/org/openmetadata/service/jobs/GenericBackgroundWorker.java Makes background job worker pool size configurable.
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestSuiteRepository.java Routes async test suite deletion to bounded lane.
openmetadata-service/src/main/java/org/openmetadata/service/events/lifecycle/OrderedLaneExecutor.java Adds configurable lane count support (0 = auto).
openmetadata-service/src/main/java/org/openmetadata/service/events/lifecycle/EntityLifecycleEventDispatcher.java Initializes ordered lanes with configurable lane count.
openmetadata-service/src/main/java/org/openmetadata/service/events/EventFilter.java Makes change-event ForkJoinPool parallelism configurable.
openmetadata-service/src/main/java/org/openmetadata/service/config/BackgroundExecutorsConfiguration.java New config class for background executor limits.
openmetadata-service/src/main/java/org/openmetadata/service/config/AsyncOperationsConfiguration.java New config class for bounded async DB-heavy operations.
openmetadata-service/src/test/java/org/openmetadata/service/util/BoundedAsyncExecutorTest.java Adds unit tests for bounded executor semantics.
openmetadata-service/src/test/java/org/openmetadata/service/util/AsyncServiceTest.java Adds test coverage for bounded-vs-raw executor behavior.
openmetadata-service/src/test/java/org/openmetadata/service/jobs/GenericBackgroundWorkerTest.java Adds test for configurable worker pool sizing.
openmetadata-service/src/test/java/org/openmetadata/service/events/lifecycle/OrderedLaneExecutorTest.java Adds test for configured lane-count override behavior.
openmetadata-service/src/test/java/org/openmetadata/service/config/LoggingConfigurationYamlTest.java Adds openmetadata-h2-test.yaml to logging YAML validation list.
openmetadata-service/src/test/java/org/openmetadata/service/config/BackgroundExecutorsConfigurationTest.java Adds tests for new background/async config defaults.
docker/docker-compose-quickstart/Dockerfile Switches Alpine runtime to install openjdk25-jre.
docker/development/Dockerfile Switches Alpine runtime to install openjdk25-jre.
DEVELOPER.md Documents per-pod database connection budgeting and relevant config keys.
conf/openmetadata.yaml Adds asyncOperations/backgroundExecutors config blocks; cleans up DB pool comments/keys.
conf/openmetadata-h2-test.yaml Mirrors new config blocks and DB pool comment cleanup for test config.

Comment on lines 65 to 69
public static synchronized AsyncService getInstance() {
if (instance == null) {
instance = new AsyncService();
LOG.warn("AsyncService not initialized, using defaults");
initialize(new AsyncOperationsConfiguration());
}
Comment on lines +58 to 63
public static synchronized void initialize(AsyncOperationsConfiguration config) {
if (instance == null) {
instance = new AsyncService(config);
instance.registerMetrics();
}
}
Comment on lines +126 to 132
private static int resolveLaneCount(int configuredLaneCount) {
int resolvedLaneCount = configuredLaneCount;
if (resolvedLaneCount == 0) {
resolvedLaneCount = computeAutomaticLaneCount();
}
return resolvedLaneCount;
}
@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

🔴 Playwright Results — workflow failed

Validated commit 055af7a32ea30da9a99b7c49eff5d4f087ba63dd in Playwright run 30277157338, attempt 1.

✅ 1187 passed · ❌ 1 failed · 🟡 1 flaky · ⏭️ 15 skipped · 🧰 0 lifecycle flaky

Pipeline and setup failures (3)

  • Playwright coverage validation found 184 missing, 0 unexpected, 0 duplicate-plan, and 0 duplicate-execution test ID(s).
  • Shard chromium-06 did not upload a usable Playwright results artifact.
  • Shard chromium-06 test execution finished with status failure without a reported test failure.

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 56m 49s

⏱️ Max setup 3m 33s · max shard execution 21m 0s · max shard-job elapsed before upload 24m 48s · reporting 8s

🌐 233.22 requests/attempt · 2.31 app boots/UI scenario · 23.41% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 23.41% (convergence target: at most 15%).
  • Browser traffic was 233.22 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.31 per UI scenario (3580 boots / 1548 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 184 0 0 0 0 0
✅ Shard chromium-02 162 0 0 2 0 0
🔴 Shard chromium-03 168 1 0 3 0 0
✅ Shard chromium-04 177 0 0 0 0 0
✅ Shard chromium-05 155 0 0 5 0 0
⛔ Shard chromium-06
🟡 Shard chromium-07 143 0 1 3 0 0
✅ Shard data-asset-rules-01 61 0 0 0 0 0
✅ Shard domain-isolation-01 14 0 0 0 0 0
✅ Shard global-state-01 23 0 0 0 0 0
✅ Shard ingestion-01 28 0 0 0 0 0
✅ Shard ingestion-02 33 0 0 0 0 0
✅ Shard reindex-01 2 0 0 0 0 0
✅ Shard search-01 10 0 0 0 0 0
✅ Shard search-rbac-01 27 0 0 2 0 0

Genuine Failures (failed on all attempts)

Features/ContextCenterPermission.spec.tsuser with createAll permission sees Add Memory button but no row edit/delete actions or modal action buttons, and can create a memory (shard chromium-03)
�[31mTest timeout of 180000ms exceeded.�[39m
🟡 1 flaky test(s) (passed on retry)
  • Features/Glossary/GlossaryHierarchy.spec.tsshould move term with children to different glossary (shard chromium-07, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

Copilot AI review requested due to automatic review settings July 26, 2026 17:44
@harshach
harshach requested a review from a team as a code owner July 26, 2026 17:44

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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

openmetadata-service/src/main/java/org/openmetadata/service/resources/search/SearchResource.java:1103

  • The watchdog timeout starts counting immediately after submission, but the bounded executor may spend significant time queued waiting for a permit. That means a reindex job can be cancelled before it ever starts running, undermining the new “queues without rejection” behavior. Consider measuring the timeout from when the worker actually begins (e.g., CountDownLatch/CompletableFuture signaled at the top of the worker) or using a two-phase timeout: wait-for-start + run-timeout.
    // The watchdog waits on the worker future and must not consume a DB-task permit.
    AsyncService.getInstance()
        .getExecutorService()
        .submit(
            () -> {
              try {
                future.get(timeoutMinutes, TimeUnit.MINUTES);
              } catch (TimeoutException e) {
                future.cancel(true);
                LOG.error(
                    "Reindex job timed out after {} minutes and was cancelled", timeoutMinutes);
              } catch (Exception e) {

Comment on lines +43 to +55
private void runWithPermit(Runnable command) {
try {
semaphore.acquire();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
return;
}
try {
command.run();
} finally {
semaphore.release();
}
}

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.

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

openmetadata-service/src/main/java/org/openmetadata/service/util/BoundedAsyncExecutor.java:52

  • runWithPermit drops non-Future Runnable tasks if the permit wait is interrupted (it just returns). That can silently lose work in cases like shutdownNow() interrupting a virtual thread blocked on Semaphore.acquire(), and more generally violates the expectation that execute() tasks either run or are explicitly rejected/cancelled.

Consider looping on InterruptedException and only exiting early when the delegate is shutting down or when the wrapped command is a cancelled Future. Preserve the interrupted flag before returning/running so callers can observe it if needed.

  private void runWithPermit(Runnable command) {
    try {
      semaphore.acquire();
    } catch (InterruptedException e) {
      cancelIfFuture(command);
      Thread.currentThread().interrupt();
      return;
    }

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.

Pull request overview

Copilot reviewed 42 out of 42 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

openmetadata-service/src/main/java/org/openmetadata/service/util/BoundedAsyncExecutor.java:51

  • If the delegate interrupts a task while it's blocked on permit acquisition, runWithPermit returns and the task never runs. For submit(...) this is surfaced as a cancelled Future, but for plain execute(Runnable) it's currently silent; adding an explicit note here will help avoid accidental misuse of execute(...) for important work.

Comment thread conf/openmetadata.yaml
Comment on lines +787 to +791
asyncOperations:
# Max concurrently RUNNING DB-heavy async tasks (async delete/restore, bulk tag/asset
# ops, glossary move, column/lineage CSV, app delete, audit packs). Excess tasks queue
# (parked virtual threads, FIFO) — nothing is rejected. 0 = unbounded.
maxConcurrentDbTasks: ${ASYNC_OPERATIONS_MAX_CONCURRENT_DB_TASKS:-25}

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.

Pull request overview

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

page.getByTestId('searchBox').press('Enter'),
]);
hasSubmittedSearch = true;
await submitSearch(crypto.randomUUID().replaceAll('-', ''));
Comment on lines 84 to 86
static void submit(UUID reportId) {
AsyncService.getInstance().execute(() -> run(reportId));
AsyncService.getInstance().getBoundedExecutorService().submit(() -> run(reportId));
}

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.

Pull request overview

Copilot reviewed 42 out of 42 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

openmetadata-service/src/main/java/org/openmetadata/service/resources/ai/AuditPackGenerator.java:86

  • submit(...) returns a Future that is ignored here, which adds overhead (creates a FutureTask) and introduces cancellation semantics that nobody observes. Since this is fire-and-forget dispatch, prefer execute(...) (or store/handle the Future if cancellation/report-state handling is intended).
  static void submit(UUID reportId) {
    AsyncService.getInstance().getBoundedExecutorService().submit(() -> run(reportId));
  }

openmetadata-service/src/main/java/org/openmetadata/service/util/BoundedAsyncExecutor.java:28

  • getQueuedCount() currently relies on Semaphore#getQueueLength(), which is an estimate. Add an explicit queued-task counter so the queue gauge reflects the actual number of tasks waiting for a permit.
    openmetadata-service/src/main/java/org/openmetadata/service/util/BoundedAsyncExecutor.java:49
  • Permit-wait interruption should decrement the queued counter to avoid permanently inflating the exported queue metric, and successful acquisition should decrement before running the command.
    openmetadata-service/src/main/java/org/openmetadata/service/util/BoundedAsyncExecutor.java:72
  • Queue size should be derived from the explicit queued-task counter rather than Semaphore#getQueueLength() to keep the metric deterministic.

Comment on lines +18 to +22
import java.util.concurrent.AbstractExecutorService;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;

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.

Pull request overview

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

Comment on lines +114 to +121
Entity.getCollectionDAO()
.relationshipDAO()
.insert(
broken.getTestDefinition().getId(),
broken.getId(),
Entity.TEST_DEFINITION,
Entity.TEST_CASE,
Relationship.CONTAINS.ordinal());
Comment on lines +653 to +658
JsonNode node = findNode(fetchGlossaryGraph(glossaryId), termId);
assertNotNull(node.get("id"), () -> "Term " + termId + " should be projected to RDF");
assertEquals(
glossaryId.toString(),
node.path("glossaryId").asText(null),
() -> "Term " + termId + " should carry its RDF glossary projection");

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.

Pull request overview

Copilot reviewed 43 out of 43 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/search/TestCaseStaleDefinitionReindexIT.java:121

  • cleanupBrokenTestCase() re-inserts the CONTAINS relationship in the opposite direction (TEST_DEFINITION -> TEST_CASE) from what was deleted (TEST_CASE -> TEST_DEFINITION). EntityRelationshipDAO.insert(fromId, toId, fromEntity, toEntity, ...) is directional, so this does not restore the broken relationship and can leave cleanup incomplete/flaky.
      Entity.getCollectionDAO()
          .relationshipDAO()
          .insert(
              broken.getTestDefinition().getId(),
              broken.getId(),
              Entity.TEST_DEFINITION,
              Entity.TEST_CASE,
              Relationship.CONTAINS.ordinal());

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.

Pull request overview

Copilot reviewed 45 out of 45 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/RdfGlossaryGraphIT.java:654

  • awaitTermInGraph can throw a NullPointerException when findNode(...) returns null: node.get("id") is dereferenced before asserting the node exists. This makes the Awaitility poll fail with an exception instead of retrying until the term appears in the RDF graph.
              JsonNode node = findNode(fetchGlossaryGraph(glossaryId), termId);
              assertNotNull(node.get("id"), () -> "Term " + termId + " should be projected to RDF");

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.

Pull request overview

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

Comments suppressed due to low confidence (2)

openmetadata-service/src/main/java/org/openmetadata/service/resources/search/SearchResource.java:1133

  • awaitExecutionStartOrCompletion(...) polls indefinitely with no upper bound. Once the reindex worker is submitted to the bounded executor, admission can be delayed arbitrarily under sustained saturation; this helper should enforce a timeout (and propagate TimeoutException) so the watchdog can cancel queued work.
  static void awaitExecutionStartOrCompletion(CountDownLatch executionStarted, Future<?> future)
      throws InterruptedException {
    while (executionStarted.getCount() > 0 && !future.isDone()) {
      executionStarted.await(EXECUTION_START_POLL_MILLIS, TimeUnit.MILLISECONDS);
    }
  }

openmetadata-service/src/test/java/org/openmetadata/service/resources/search/SearchResourceAsyncTest.java:92

  • This test calls SearchResource.awaitExecutionStartOrCompletion(...); with the production fix adding a timeout parameter, update the test invocation to pass a deadline and keep the intent (exit promptly once the future is cancelled).

Comment on lines 1102 to 1106
() -> {
try {
awaitExecutionStartOrCompletion(executionStarted, future);
future.get(timeoutMinutes, TimeUnit.MINUTES);
} catch (TimeoutException e) {
Comment on lines +51 to +58
Future<?> watchdog =
watchdogExecutor.submit(
() -> {
watchdogStarted.countDown();
SearchResource.awaitExecutionStartOrCompletion(executionStarted, reindexTask);
reindexTask.get(100, TimeUnit.MILLISECONDS);
return null;
});

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.

Pull request overview

Copilot reviewed 48 out of 48 changed files in this pull request and generated 1 comment.

Comment on lines +91 to +101
private void registerMetrics() {
Gauge.builder(ACTIVE_TASKS_METRIC, this, AsyncService::getActiveDbTaskCount)
.description("Running DB-heavy asynchronous operations")
.register(Metrics.globalRegistry);
Gauge.builder(QUEUED_TASKS_METRIC, this, AsyncService::getQueuedDbTaskCount)
.description("Queued DB-heavy asynchronous operations")
.register(Metrics.globalRegistry);
Gauge.builder(TASK_LIMIT_METRIC, this, service -> service.maxConcurrentDbTasks)
.description("Configured DB-heavy asynchronous operation limit")
.register(Metrics.globalRegistry);
}

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.

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

openmetadata-service/src/main/java/org/openmetadata/service/resources/search/SearchResource.java:1105

  • The watchdog waits indefinitely in awaitExecutionStartOrCompletion(...) before starting the future.get(timeoutMinutes, ...) timer. Under saturation of the bounded DB lane, this allows reindex requests to exceed the configured timeout by an arbitrary admission delay (and keeps an extra watchdog task alive the whole time). Consider enforcing a single end-to-end deadline that includes permit/admission wait, cancelling the job if it cannot start within the configured timeout.
              try {
                awaitExecutionStartOrCompletion(executionStarted, future);
                future.get(timeoutMinutes, TimeUnit.MINUTES);

openmetadata-ui/src/main/resources/ui/playwright/utils/domain.ts:300

  • Same issue as assignDomainWidget: relying on clickAvailableWidgetAction(addBtn, editBtn) can wait on a hidden DOM-first button and time out even though the other action is visible. Prefer clicking the first visible button.

const editBtn = page.getByTestId('edit-domain');
const isAdd = await addBtn.isVisible();
await (isAdd ? addBtn : editBtn).click();
await clickAvailableWidgetAction(addBtn, editBtn);
@sonarqubecloud

Copy link
Copy Markdown

@gitar-bot

gitar-bot Bot commented Jul 27, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 2 resolved / 3 findings

Bounds top-level asynchronous database operations and prepares the Alpine runtime for JDK 25. Consider correcting the jackson-bom dependency comment referencing a future CVE identifier.

💡 Quality: Misleading jackson comment: '3.1.5 adds CVE-2026-59889'

📄 pom.xml:800-806

The comment on the jackson-bom bump reads "Force patched tools.jackson 3.x ... 3.1.5 adds CVE-2026-59889", which literally states the upgrade introduces a vulnerability — contradicting the stated intent of forcing a patched version. This will mislead future maintainers about why the version is pinned. Reword to say 3.1.5 fixes/addresses CVE-2026-59889.

Clarify that 3.1.5 remediates the CVE rather than introducing it.
<!-- Force patched tools.jackson 3.x (jsonschema2pojo-core pulls 3.0.2 transitively).
     3.1.5 fixes CVE-2026-59889. jsonschema2pojo-core is scope=provided so none of this
     reaches the runtime classpath, but Snyk's Maven scanner walks the provided tree. -->
✅ 2 resolved
Edge Case: Reindex timeout now counts bounded-lane queue wait time

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/search/SearchResource.java:952-966
In reindexEntities, the worker was moved from the raw executor to the bounded executor (getBoundedExecutorService()), but the watchdog's future.get(timeoutMinutes, MINUTES) clock still starts at submission time. When the 25-permit bounded lane is saturated by other DB-heavy async operations, the reindex worker parks on the semaphore before it starts running, so the timeout can elapse and future.cancel(true) fires while the job is still queued — cancelling it before any work is done. Previously the worker started immediately on the unbounded raw lane. Consider measuring the timeout from the moment the worker actually begins executing (e.g., record a start latch inside the worker and have the watchdog wait on execution start before arming the timeout), or document that the timeout now includes admission wait.

Quality: Cleanup in finally can throw and mask test failure

📄 openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/search/TestCaseStaleDefinitionReindexIT.java:107-119
The previous cleanup wrapped deletion in a best-effort try/catch; the new finally block calls relationshipDAO().insert(...) and the SDK hard-delete without any guard. If the re-insert (e.g. relationship row already exists) or the delete throws, the exception propagates out of finally and masks any assertion failure from the try block, producing a misleading test error. Wrap the cleanup so it does not suppress the primary result, or log/swallow cleanup exceptions rather than letting them replace the real failure.

🤖 Prompt for agents
Code Review: Bounds top-level asynchronous database operations and prepares the Alpine runtime for JDK 25. Consider correcting the jackson-bom dependency comment referencing a future CVE identifier.

1. 💡 Quality: Misleading jackson comment: '3.1.5 adds CVE-2026-59889'
   Files: pom.xml:800-806

   The comment on the jackson-bom bump reads "Force patched tools.jackson 3.x ... 3.1.5 adds CVE-2026-59889", which literally states the upgrade introduces a vulnerability — contradicting the stated intent of forcing a patched version. This will mislead future maintainers about why the version is pinned. Reword to say 3.1.5 fixes/addresses CVE-2026-59889.

   Fix (Clarify that 3.1.5 remediates the CVE rather than introducing it.):
   <!-- Force patched tools.jackson 3.x (jsonschema2pojo-core pulls 3.0.2 transitively).
        3.1.5 fixes CVE-2026-59889. jsonschema2pojo-core is scope=provided so none of this
        reaches the runtime classpath, but Snyk's Maven scanner walks the provided tree. -->

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 1106 to 1109
} catch (TimeoutException e) {
future.cancel(true);
LOG.error(
"Reindex job timed out after {} minutes and was cancelled", timeoutMinutes);

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 Cancellation does not stop reindex

When a reindex operation exceeds its execution timeout during a blocking database or search operation, future.cancel(true) interrupts the worker, but the per-entity catch (Exception) handles the interruption-related failure as an ordinary entity failure and continues the loop. Database and search writes can therefore continue after the watchdog reports that the job was cancelled.

Knowledge Base Used: Search Indexing and Event/Notification Sync

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.

Pull request overview

Copilot reviewed 48 out of 48 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

openmetadata-service/src/main/java/org/openmetadata/service/resources/search/SearchResource.java:1106

  • The watchdog can block indefinitely in awaitExecutionStartOrCompletion() if the reindex worker never gets admitted to the bounded executor (e.g., persistent backlog). This makes the effective timeout unbounded under load. Consider bounding the admission wait (e.g., by timeoutMinutes) so the watchdog eventually cancels the job rather than waiting forever.
            () -> {
              try {
                awaitExecutionStartOrCompletion(executionStarted, future);
                future.get(timeoutMinutes, TimeUnit.MINUTES);
              } catch (TimeoutException e) {

openmetadata-service/src/main/java/org/openmetadata/service/resources/search/SearchResource.java:1133

  • awaitExecutionStartOrCompletion() currently waits without any upper bound, so a queued reindex job can leave the watchdog stuck forever (and leak a virtual thread) if it never starts. Add a timed overload and use it from the watchdog to cap admission wait and surface a TimeoutException.
  static void awaitExecutionStartOrCompletion(CountDownLatch executionStarted, Future<?> future)
      throws InterruptedException {
    while (executionStarted.getCount() > 0 && !future.isDone()) {
      executionStarted.await(EXECUTION_START_POLL_MILLIS, TimeUnit.MILLISECONDS);
    }
  }

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bound async database fan-out before enabling virtual threads

2 participants