Skip to content

fix: dispatch timeout metrics for all priorities with staggered delay#4196

Draft
hengyuss wants to merge 9 commits into
apache:masterfrom
hengyuss:fix/continue-metrics-on-availability-failure-with-staggered-dispatch
Draft

fix: dispatch timeout metrics for all priorities with staggered delay#4196
hengyuss wants to merge 9 commits into
apache:masterfrom
hengyuss:fix/continue-metrics-on-availability-failure-with-staggered-dispatch

Conversation

@hengyuss

Copy link
Copy Markdown
Contributor

What's changed?

fix #4194

Allow all priority timeout metrics to execute dispatchCollectData and add staggered delay to avoid thread pool overload

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Add or update API

  • I have added the necessary e2e tests and all cases have passed.

@hengyuss

Copy link
Copy Markdown
Contributor Author

i will remove the delay handle and open a new issue to resolve worker pool accumulate usage

@tomsun28

Copy link
Copy Markdown
Member

If we make this change, could it result in an unmanageable number of tasks? When a task is deemed unavailable, is there no longer a need to retry its other subtasks?

@hengyuss

hengyuss commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

1、the judgement of priority == 0 introduce by #66 at first it's same as this changed. and alert already is different now. but i can‘t estimate the number of tasks indeed currently.

2、it's true that no longer a need to retry its other substasks if a task is deemed unabailable. But the function monitorCollectTaskTimeout is guarantee the bottom line for task. Doesn't that mean the availability task with priority 0 has already been passed?if the program has run up to here.

@Duansg

Duansg commented Jul 19, 2026

Copy link
Copy Markdown
Member

If we make this change, could it result in an unmanageable number of tasks? When a task is deemed unavailable, is there no longer a need to retry its other subtasks?

@tomsun28 @hengyuss That's right; simply removing the priority == 0 check isn't safe in and of itself, but I still think we need to remove this validation. Regarding the background of this PR, here's my take:

  1. Would tasks grow unmanageably?
  • No task repetition: The timeout dispatch only advances the task’s priorMetrics state machine; the next-level metric it triggers is precisely what should be executed in that round. cyclicJob only restarts after the current round is complete, so rounds are never repeated. Additionally, since [bugfix] Fix duplicate collector job timeouts #4169/[fix] cancel the overwritten timeout in cyclicJob to fix HashedWheelTimeout memory leak #4201, the timer round mechanism has been fully safeguarded.

  • Issue 1: Currently, there is no mechanism to cancel a worker that remains suspended (e.g., due to I/O blocking). Once this check (for priority == 0) is removed, the catch-all thread will continue to reschedule the worker in every round; therefore, a metric that never returns will leak a permanently stuck worker task in every round until the thread pool is exhausted;

  • Issue 2: This change will exacerbate an existing race condition. For example, a task that eventually completes after timing out (>4 min) will still trigger a second call to dispatchCollectData

  1. When a task is deemed unavailable, do we still skip its other metrics?
  • The current logic is that if an availability collection with priority 0 fails or times out, isAvailableCollectFailed will still cause the entire round to terminate early, and none of the tasks below it will be scheduled. The only change in behavior occurs when the availability collection succeeds but a non-availability metric is suspended: currently, this causes the monitor to become permanently stalled, because cyclicJob is only restarted within dispatchCollectData, and the system does not schedule tasks for suspended metrics.

  • Assumption: Suppose the task does indeed resume after 30 minutes. During those 30 minutes, there is no data, no TIMEOUT events, and no alerts; the page displays what appears to be normal, but outdated data. “Waiting for it to recover on its own” effectively leaves the decision on when to detect the delay up to the party responsible for the failure. While this may not matter to the data collector, it is disastrous for those using the monitoring system.

@Duansg

Duansg commented Jul 19, 2026

Copy link
Copy Markdown
Member

@tomsun28 But I think we should adjust our approach here, rather than simply removing this validation.

For example:

  1. The worker thread and the timeout monitor thread compete for exclusive access via CAS, ensuring that the result is distributed only once; the losing party silently backs off.

  2. Adjust scheduling priority and interrupt the running worker thread. Note: In the default virtual thread pool, blocking socket I/O is interruptible (JEP 444), so stuck HTTP/JDBC/Redis read operations will be physically released at the timeout, rather than causing a resource leak.

  3. The work thread must be mutually exclusive with the timeout monitoring thread to ensure that late interrupts never affect threads that have already switched to processing other tasks;

  4. A mechanism for rolling back after consecutive timeouts (for example, skipping N rounds for a metric after M consecutive timeouts) will further reduce the number of repeated 4-minute waits caused by a single persistently suspended metric;

Under this logic, in an extreme scenario, the worst-case scenario would be:

→ p0 Success
→ p1 Start (Response Blocked) ── 4 min ──→ Fallback: Discard p1, resend TIMEOUT, start p2
→ p2 (response blocked) ── 4 min ──→ Discard p2, resend TIMEOUT, start p3
→ p3 (response blocked) ── 4 min ──→ Discard p3, resend TIMEOUT, start p4
→ p4 (response blocked) ── 4 min ──→ Discard p4, resend TIMEOUT, end this round → cyclicJob → Start over from the beginning in the next round

Proceed sequentially, advancing one layer every 4 minutes; at any given time, only one layer is active, provided that p0 succeeds. If p0 fails first, maintain the existing design—the top layer will short-circuit—and simply proceed to the next round.

To summarize, the design goals are as follows: The fallback thread can advance the state machine without relying on the response status of a deadlocked task. Interrupts are used to free up resources as much as possible, while fallback dispatch ensures bounded recovery.

@Duansg
Duansg marked this pull request as draft July 19, 2026 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Availability (priority=0) failure blocks all lower priority metrics collection, causing monitoring pipeline stall

3 participants