fix(reindex): stop falsely abandoning healthy reindex jobs that hold a live lock

[mohityadav766]

Problem

A distributed search reindex could be marked FAILED with Job abandoned due to server crash or shutdown while it was actually still running and healthy. On large catalogs (long-running reindexes) this aborted the reindex partway and forced a re-run; the app run showed a job-level failure with an empty failed-entities list.

Fixes #29546

Root cause

Orphaned-job detection (JobRecoveryManager.isJobOrphaned) marks a job abandoned when its progress heartbeat (job.updatedAt) is stale (≥ 10 min). When the heartbeat was stale but a valid reindex lock still existed, the old code also treated the job as orphaned ("coordinator may have crashed while holding the lock"), and the periodic OrphanJobMonitor then failed it with the message above.

That branch is wrong. The reindex lock TTL (LOCK_TIMEOUT_MS = 5 min) is shorter than the staleness threshold (ABANDONED_LOCK_THRESHOLD_MS = 10 min), so a crashed coordinator's lock always expires before updatedAt can reach the 10-min threshold. "Valid lock + ≥10-min-stale heartbeat" can therefore only occur when the coordinator is alive and refreshing the lock (every 60 s) while its updatedAt write lagged — e.g. the throttled touchJob write failing, or a single large partition running a long time between completions.

Fix

Treat a still-unexpired reindex lock as proof the coordinator is alive: when updatedAt is stale but a valid lock for the job exists, the job is not orphaned — regardless of heartbeat lag or total job age. A genuine crash/restart expires the lock, so those jobs are still correctly detected and failed.

Extracted a hasLiveCoordinatorLock() helper and replaced the repeated lock-key literal with a constant.

Tests

• JobRecoveryOrphanDetectionTest: stale updatedAt + live (unexpired) lock → not orphaned (previously asserted the opposite).
• JobRecoveryManagerTest: repaired the stale→orphaned case to use a realistic expired lock; added a regression test for a ~2 h reindex with a stale heartbeat but a live lock → not failed.

All green: JobRecoveryManagerTest + JobRecoveryOrphanDetectionTest 31/31. mvn spotless:apply run.

🤖 Generated with Claude Code

Greptile Summary

Inverts the orphan-detection branch for "stale updatedAt + valid lock": a still-unexpired reindex lock is now treated as proof the coordinator is alive rather than evidence it crashed. The fix exploits the invariant that the lock TTL (5 min) is shorter than the staleness threshold (10 min), so a genuinely crashed coordinator's lock always expires before updatedAt can become this stale.

• JobRecoveryManager.isJobOrphaned: replaced the old "valid lock + stale heartbeat → orphaned" branch with a hasLiveCoordinatorLock() helper that returns false (not orphaned) when the lock is unexpired, and extracted the magic "SEARCH_REINDEX_LOCK" string to a constant REINDEX_LOCK_KEY.
• JobRecoveryOrphanDetectionTest: renamed and inverted the assertion of the stale-heartbeat/live-lock test case to match the fixed behaviour.
• JobRecoveryManagerTest: updated the stale-orphaned test to use a realistically expired lock, and added a regression test covering a 2-hour reindex with a 15-minute-stale heartbeat but a lock refreshed 30 seconds ago.

Confidence Score: 5/5

Safe to merge — the change is tightly scoped to a single private method, the invariant it relies on (lock TTL < staleness threshold) is well-established by the surrounding constants, and the fix is validated by both the targeted unit tests and the new integration-level regression test.

The logic inversion is correct: because LOCK_TIMEOUT_MS (5 min) is strictly shorter than ABANDONED_LOCK_THRESHOLD_MS (10 min), a coordinator that crashed cannot still hold a valid lock by the time updatedAt reaches the stale threshold. All five observable scenarios (fresh heartbeat, stale+live-lock, stale+expired-lock, stale+no-lock, stale+different-job-lock) now produce the right outcome, and each is covered by a test.

No files require special attention.

Important Files Changed

Filename	Overview
openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/searchIndex/distributed/JobRecoveryManager.java	Core logic change: isJobOrphaned now returns false when updatedAt is stale but a live lock exists; hasLiveCoordinatorLock extracted as a helper; "SEARCH_REINDEX_LOCK" magic string replaced by REINDEX_LOCK_KEY constant.
openmetadata-service/src/test/java/org/openmetadata/service/apps/bundles/searchIndex/distributed/JobRecoveryManagerTest.java	Existing stale-orphaned test updated to use a realistically expired lock; new regression test added covering a 2-hour reindex with stale heartbeat but live lock — correctly expects 0 orphaned and 0 failed jobs.
openmetadata-service/src/test/java/org/openmetadata/service/apps/bundles/searchIndex/distributed/JobRecoveryOrphanDetectionTest.java	Renamed and inverted the stale-heartbeat/live-lock unit test from asserting orphaned=true to asserting orphaned=false; scenario metadata updated to reflect the live-coordinator context.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[isJobOrphaned called] --> B{updatedAt fresh?\nupdatedAt >= now - 10min}
    B -- Yes --> C[orphaned = false\ncoordinator alive via heartbeat]
    B -- No --> D[hasLiveCoordinatorLock?]
    D --> E{Lock exists AND\nlockInfo.jobId == job.id AND\nlockInfo.expiresAt >= now?}
    E -- Yes --> F["orphaned = false\n(lock TTL 5min < threshold 10min:\ncrashed lock would have expired\nbefore updatedAt went this stale)"]
    E -- No --> G[orphaned = true\nno heartbeat AND no live lock]
    C --> H[return false]
    F --> H
    G --> I[return true → processOrphanedJob]

    style C fill:#90EE90
    style F fill:#90EE90
    style G fill:#FFB6C1
    style I fill:#FFB6C1

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[isJobOrphaned called] --> B{updatedAt fresh?\nupdatedAt >= now - 10min}
    B -- Yes --> C[orphaned = false\ncoordinator alive via heartbeat]
    B -- No --> D[hasLiveCoordinatorLock?]
    D --> E{Lock exists AND\nlockInfo.jobId == job.id AND\nlockInfo.expiresAt >= now?}
    E -- Yes --> F["orphaned = false\n(lock TTL 5min < threshold 10min:\ncrashed lock would have expired\nbefore updatedAt went this stale)"]
    E -- No --> G[orphaned = true\nno heartbeat AND no live lock]
    C --> H[return false]
    F --> H
    G --> I[return true → processOrphanedJob]

    style C fill:#90EE90
    style F fill:#90EE90
    style G fill:#FFB6C1
    style I fill:#FFB6C1

Loading

_{Reviews (1): Last reviewed commit: "Merge branch 'main' into richmond" | Re-trigger Greptile}

[github-actions]

❌ PR checklist incomplete

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

• Linked issue Search reindex incorrectly fails as "Job abandoned due to server crash or shutdown" while still running #29546 is not added to the Shipping project.

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.

[sonarqubecloud]

Quality Gate passed for 'open-metadata-ingestion'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

🟡 Playwright Results — all passed (46 flaky)

✅ 4430 passed · ❌ 0 failed · 🟡 46 flaky · ⏭️ 38 skipped

Shard	Passed	Failed	Flaky	Skipped
🟡 Shard 1	320	0	5	4
🟡 Shard 2	818	0	11	9
🟡 Shard 3	822	0	7	7
🟡 Shard 4	827	0	4	10
🟡 Shard 5	861	0	13	0
🟡 Shard 6	782	0	6	8

🟡 46 flaky test(s) (passed on retry)

• Features/DataAssetRulesEnabled.spec.ts › Verify the Chart Entity Action items after rules is Enabled (shard 1, 1 retry)
• Features/DataAssetRulesEnabled.spec.ts › should enforce single domain selection for glossary term when entity rules are enabled (shard 1, 1 retry)
• Features/EntityRenameConsolidation.spec.ts › Classification - rename then update description should preserve tags (shard 1, 1 retry)
• Features/Pagination.spec.ts › should test API Collection complete flow with search (shard 1, 1 retry)
• Flow/SearchRBAC.spec.ts › a fully denied user sees neither asset type when browsing (shard 1, 2 retries)
• Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
• Features/BulkEditOperationBadges.spec.ts › Glossary bulk edit search filters rows and clear restores them (shard 2, 1 retry)
• Features/BulkImport.spec.ts › Database service (shard 2, 1 retry)
• Features/BulkImport.spec.ts › Database Schema (shard 2, 2 retries)
• Features/Container.spec.ts › Container page children pagination (shard 2, 1 retry)
• Features/Container.spec.ts › expand / collapse should not appear after updating nested fields for container (shard 2, 1 retry)
• Features/Container.spec.ts › Copy column link button should copy the column URL to clipboard (shard 2, 2 retries)
• Features/ContextCenterPermission.spec.ts › user with view-only permission cannot see restore or delete actions on an archived document (shard 2, 1 retry)
• Features/CuratedAssets.spec.ts › Test Search Indexes with display name filter (shard 2, 1 retry)
• Features/GlobalPageSize.spec.ts › Page size should persist across different pages (shard 2, 2 retries)
• Features/GlobalSearchSuggestions.spec.ts › Navigate to column from column suggestion (shard 2, 1 retry)
• Features/KnowledgeCenter.spec.ts › Article mentions in description should working for Knowledge Center (shard 3, 1 retry)
• Features/LandingPageWidgets/FollowingWidget.spec.ts › Check followed entity present in following widget (shard 3, 1 retry)
• Features/Permissions/EntityPermissions.spec.ts › Dashboard allow common operations permissions (shard 3, 1 retry)
• Features/RecentlyViewed.spec.ts › Check Metric in recently viewed (shard 3, 1 retry)
• Features/SearchExport.spec.ts › Export queues a background job and downloads from the jobs tray (shard 3, 1 retry)
• Features/Tasks/TaskNavigation.spec.ts › navigating to /table/TASK-XXXXX should show 404 (invalid URL pattern) (shard 3, 2 retries)
• Features/TestSuiteMultiPipeline.spec.ts › TestSuite multi pipeline support (shard 3, 1 retry)
• Flow/ExploreDiscovery.spec.ts › Should display domain and owner of deleted asset in suggestions when showDeleted is on (shard 4, 1 retry)
• Flow/NestedChildrenUpdates.spec.ts › should add and remove tags to nested column immediately without refresh (shard 4, 1 retry)
• Flow/PersonaFlow.spec.ts › Set default persona for team should work properly (shard 4, 1 retry)
• Flow/ServiceForm.spec.ts › Verify form selects are working properly (shard 4, 1 retry)
• Pages/Entity.spec.ts › Copy entity URL from header (shard 5, 1 retry)
• Pages/Entity.spec.ts › Tag and Glossary Selector should close vice versa (shard 5, 1 retry)
• Pages/Entity.spec.ts › User should be denied access to edit description when deny policy rule is applied on an entity (shard 5, 1 retry)
• ... and 16 more

📦 Download artifacts

How to debug locally

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

[harshach]

@mohityadav766 should this go into 1.13 or 1.12 branch?

[gitar-bot]

Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Prevents healthy reindex jobs from being falsely abandoned by verifying live coordinator locks before failing stale jobs. Ensure the relationship between ABANDONED_LOCK_THRESHOLD_MS and LOCK_TIMEOUT_MS remains explicitly documented to maintain this invariant.

💡 Quality: Orphan-detection correctness depends on cross-class constant coupling

📄 openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/searchIndex/distributed/JobRecoveryManager.java:45 📄 openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/searchIndex/distributed/JobRecoveryManager.java:276-282

isJobOrphaned/hasLiveCoordinatorLock correctness hinges on the invariant ABANDONED_LOCK_THRESHOLD_MS (10 min, in JobRecoveryManager) > the coordinator's LOCK_TIMEOUT_MS (5 min, in DistributedSearchIndexCoordinator.java:63). If a future change raises LOCK_TIMEOUT_MS to ≥10 min (or lowers the staleness threshold), the new branch would silently stop detecting genuinely crashed coordinators that still hold an unexpired lock — those jobs would never be marked orphaned/failed. The two constants live in different classes with no compile-time or test guard linking them; the invariant is only described in a comment.

Suggested: add a guard that ties the two together so a violation fails fast — e.g. a static assertion/test that ABANDONED_LOCK_THRESHOLD_MS > DistributedSearchIndexCoordinator.LOCK_TIMEOUT_MS, or reference the actual coordinator constant when deriving the threshold rather than hardcoding 10 min independently.

Fail fast if the invariant the orphan-detection logic relies on is ever broken.

// In a test or a static initializer block:
static {
  if (ABANDONED_LOCK_THRESHOLD_MS <= DistributedSearchIndexCoordinator.LOCK_TIMEOUT_MS) {
    throw new IllegalStateException(
        "ABANDONED_LOCK_THRESHOLD_MS must exceed the coordinator lock TTL so a live "
            + "lock can be trusted as proof of a live coordinator");
  }
}

🤖 Prompt for agents

Code Review: Prevents healthy reindex jobs from being falsely abandoned by verifying live coordinator locks before failing stale jobs. Ensure the relationship between ABANDONED_LOCK_THRESHOLD_MS and LOCK_TIMEOUT_MS remains explicitly documented to maintain this invariant.

1. 💡 Quality: Orphan-detection correctness depends on cross-class constant coupling
   Files: openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/searchIndex/distributed/JobRecoveryManager.java:45, openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/searchIndex/distributed/JobRecoveryManager.java:276-282

   `isJobOrphaned`/`hasLiveCoordinatorLock` correctness hinges on the invariant `ABANDONED_LOCK_THRESHOLD_MS` (10 min, in JobRecoveryManager) > the coordinator's `LOCK_TIMEOUT_MS` (5 min, in DistributedSearchIndexCoordinator.java:63). If a future change raises `LOCK_TIMEOUT_MS` to ≥10 min (or lowers the staleness threshold), the new branch would silently stop detecting genuinely crashed coordinators that still hold an unexpired lock — those jobs would never be marked orphaned/failed. The two constants live in different classes with no compile-time or test guard linking them; the invariant is only described in a comment.
   
   Suggested: add a guard that ties the two together so a violation fails fast — e.g. a static assertion/test that `ABANDONED_LOCK_THRESHOLD_MS > DistributedSearchIndexCoordinator.LOCK_TIMEOUT_MS`, or reference the actual coordinator constant when deriving the threshold rather than hardcoding 10 min independently.

   Fix (Fail fast if the invariant the orphan-detection logic relies on is ever broken.):
   // In a test or a static initializer block:
   static {
     if (ABANDONED_LOCK_THRESHOLD_MS <= DistributedSearchIndexCoordinator.LOCK_TIMEOUT_MS) {
       throw new IllegalStateException(
           "ABANDONED_LOCK_THRESHOLD_MS must exceed the coordinator lock TTL so a live "
               + "lock can be trusted as proof of a live coordinator");
     }
   }

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact

gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}