Deadlock between segment refresh and segment reload due to inconsistent lock/semaphore acquisition order in `BaseTableDataManager`

[shuturmurgh]

Summary

BaseTableDataManager acquires the per-segment lock and the server-wide SegmentReloadSemaphore in opposite orders on the refresh and reload paths:

Path	Order	Location on master (7f79c65)
replaceSegmentInternal → doReplaceSegment (refresh)	segment lock → semaphore	BaseTableDataManager.java:537 then :556
reloadSegment (reload)	semaphore → segment lock	BaseTableDataManager.java:1106 then :1108

When a refresh and a reload target the same segment, this deadlocks permanently. Because max.parallel.refresh.threads defaults to 1 (HelixInstanceDataManagerConfig:259), the single permit is then held forever and every segment refresh and reload on the server stops — for all tables, not just the two involved. Neither acquisition has a timeout (SegmentReloadSemaphore.acquire calls bare Semaphore.acquire(); the reload path calls bare Lock.lock()), so the server never recovers without a restart.

We hit this in production and it silently stopped all segment refresh on four servers for ~62 hours.

Setup

An offline job refreshes a fixed set of segments for several tables daily. Each run rewrites the segments under the same names with new CRCs, so every run triggers SegmentRefreshMessage for each segment.

Deadlock timeline

1. table_B's segments are uploaded with modified CRCs, triggering refreshes. table_B is missing its Lucene text index in the generated segments, so SegmentPreProcessor builds it at load time — ~20 minutes per segment, all of it while holding the server-wide semaphore. A backlog forms.

2. table_A's segments are uploaded with modified CRCs, triggering refreshes for table_A_00001 … table_A_00010. The refresh for table_A_00001 takes that segment's lock, then blocks waiting for the semaphore, still holding the segment lock.

3. A table reload is issued for table_A. The reload for table_A_00001 acquires the semaphore first, then reaches for the segment lock.

4. Deadlock. The refresh holds the segment lock and waits for the permit; the reload holds the permit and waits for the segment lock. With one permit, all other queued refreshes now wait forever too, and since each occupies one of the 40 threads in the Helix message-handler pool (HelixTaskExecutor.DEFAULT_PARALLEL_TASKS), no refresh or reload for any table can start.

Evidence

Thread dump

Refresh handler — holds the segment lock, waiting on the semaphore:

"HelixTaskExecutor-message_handle_thread_62" daemon prio=5 waiting on condition
   java.lang.Thread.State: WAITING (parking)
	at jdk.internal.misc.Unsafe.park(java.base@17.0.15/Native Method)
	- parking to wait for  <0x0000ffbeef659ca0> (a java.util.concurrent.Semaphore$FairSync)
	at java.util.concurrent.Semaphore.acquire(java.base@17.0.15/Semaphore.java:318)
	at org.apache.pinot.segment.local.utils.SegmentReloadSemaphore.acquire(SegmentReloadSemaphore.java:39)
	at org.apache.pinot.core.data.manager.BaseTableDataManager.doReplaceSegment(BaseTableDataManager.java:481)
	at org.apache.pinot.core.data.manager.BaseTableDataManager.replaceSegment(BaseTableDataManager.java:464)
	at org.apache.pinot.server.starter.helix.HelixInstanceDataManager.replaceSegment(HelixInstanceDataManager.java:343)
	at org.apache.pinot.server.starter.helix.SegmentMessageHandlerFactory$SegmentRefreshMessageHandler.handleMessage(SegmentMessageHandlerFactory.java:110)
	at org.apache.helix.messaging.handling.HelixTask.call(HelixTask.java:97)

   Locked ownable synchronizers:
	- <0x0000ffbe9cbfea00> (a java.util.concurrent.locks.ReentrantLock$NonfairSync)

Reload worker — holds the permit, waiting on that same lock 0x0000ffbe9cbfea00:

"segment-reload-thread-0" prio=5 waiting on condition
   java.lang.Thread.State: WAITING (parking)
	at jdk.internal.misc.Unsafe.park(java.base@17.0.15/Native Method)
	- parking to wait for  <0x0000ffbe9cbfea00> (a java.util.concurrent.locks.ReentrantLock$NonfairSync)
	at java.util.concurrent.locks.ReentrantLock.lock(java.base@17.0.15/ReentrantLock.java:322)
	at org.apache.pinot.core.data.manager.BaseTableDataManager.reloadSegment(BaseTableDataManager.java:803)
	at org.apache.pinot.core.data.manager.BaseTableDataManager.reloadSegment(BaseTableDataManager.java:790)
	at org.apache.pinot.core.data.manager.BaseTableDataManager.lambda$reloadSegments$2(BaseTableDataManager.java:747)
	at java.util.concurrent.CompletableFuture$AsyncRun.run(java.base@17.0.15/CompletableFuture.java:1804)

(Line numbers above are from the version we ran; on master they map to doReplaceSegment:556, replaceSegmentInternal:537 and reloadSegment:1108.)

Pool census — 40 idle state-transition threads, and 40 user-defined-message threads all blocked:

Count	State
37	refresh, blocked at doReplaceSegment:481 on the semaphore, each holding one segment lock
2	reloadAllSegments:569 → reloadSegments:756, blocked on CompletableFuture.get
1	refresh, blocked at replaceSegment:462 on the same segment lock

Two dumps taken 21 minutes apart show byte-identical CPU counters for every participant (thread 62: 2455564.32 ms in both) and an identical set of 37 waiters — a hang, not slow progress.

Semaphore queue growth

10:45:12  table_B_00007  Acquired lock to reload segment (lock-time=0ms, queue-length=0)
10:46:46  table_B_00009  Waiting for lock to reload, queue-length: 1
11:04:47  table_B_00001  Waiting for lock to reload, queue-length: 8
11:05:53  table_B_00002  Acquired lock to reload segment (lock-time=1205789ms, queue-length=8)
11:23:32  table_A_00001  Waiting for lock to reload, queue-length: 8     <- holds its segment lock
11:26:25  table_B_00003  Acquired lock to reload segment (lock-time=2378918ms, queue-length=8)

Note: jstack does not report this as a deadlock

jstack -l prints no Found one Java-level deadlock section. The JVM's detector only follows monitors and exclusively-owned AQS synchronizers; a Semaphore has no owning thread, so a cycle passing through one is invisible to it. This makes the bug considerably harder to attribute — the observable symptom is just "Helix completed-task counter flat, 40 active threads, restart fixes it," which is easy to misdiagnose.

Proposed fix

Make reloadSegment acquire the segment lock before the semaphore, matching doReplaceSegment. In BaseTableDataManager.java:1106-1108:

// current
_segmentReloadSemaphore.acquire(segmentName, _logger);
Lock segmentLock = getSegmentLock(segmentName);
segmentLock.lock();

// proposed
Lock segmentLock = getSegmentLock(segmentName);
segmentLock.lock();
_segmentReloadSemaphore.acquire(segmentName, _logger);

with the finally block at :1221-1224 swapped to match:

} finally {
  _segmentReloadSemaphore.release();
  segmentLock.unlock();
}

One wrinkle: acquire throws InterruptedException, so it needs to move inside the existing try (or get its own try/finally) to guarantee the lock is released if the acquire is interrupted.

Converging the other way — refresh taking the permit before the lock — also removes the cycle, but is a worse trade: both paths could then hold the single global permit while blocked on a fine-grained lock, converting a per-segment stall into a server-wide one. Holding the segment lock while throttled only delays that one segment.

Better still would be a single helper that both paths call, so the ordering cannot diverge again. The bug exists because the same two-resource protocol is implemented independently in two places.

Worth considering alongside:

• Release the semaphore after download/untar and before index construction. It exists to bound concurrent segment loads, and there is already a separate instance-level download semaphore for deep-store throttling — in the trace above the permit was held 20 minutes for 12 seconds of I/O.
• Scope the semaphore per table, or at least document that the default of 1 makes any slow refresh a server-wide stall.
• Timed tryAcquire/tryLock that fail the Helix message so it surfaces as ERROR and the controller can retry, rather than hanging indefinitely.
• A gauge for semaphore queue depth and permit hold time. Both values are already printed in the existing log line (queue-length, lock-time) but are not exposed as metrics.

[abhiramaab]

I'd like to work on this. My plan is the minimal fix: in reloadSegment, acquire the segment lock before acquire(_segmentReloadSemaphore, ...), matching doReplaceSegment, with a try/finally so the lock is released if acquire is interrupted. I'll add a unit test covering the interleaving. Any preference between converging on that order vs. a shared helper that both paths call? Happy to take the latter if you'd rather prevent divergence.