chore(crashtracking): harden multi thread ptrace collection

libdd-crashtracker/src/receiver/ptrace_collector.rs

@@ -173,15 +173,27 @@ fn attach_thread(tid: libc::pid_t, stop_deadline: Instant) -> Result<(), PtraceE
     // On older kernels, the register state may not be
     // immediately readable after waitpid reports the stop. Spin briefly
     // until PEEKUSER returns a non-zero IP, proving registers are committed.
-    wait_for_registers(tid, stop_deadline);
+    //
+    // If the deadline expires before we see a non-zero IP, proceed anyway:
+    // libunwind uses PTRACE_GETREGSET which may succeed even when PEEKUSER
+    // returns zero. If registers are truly uncommitted, unwind_remote_thread
+    // will return 0 frames and capture_with_retry will retry without needing
+    // a costly detach/re-attach cycle (which can fail with EPERM under CPU
+    // pressure because the kernel hasn't fully released the prior ptrace
+    // state).
+    let _ = wait_for_registers(tid, stop_deadline);
 
     Ok(())
 }
 
 /// Poll the thread's instruction pointer using PTRACE_PEEKUSER until it is
 /// non-zero or the deadline expires. On modern kernels this should return on the
 /// first iteration; on older ones, it may take a few microseconds.
-fn wait_for_registers(tid: libc::pid_t, deadline: Instant) {
+///
+/// Returns `true` if a non-zero IP was observed or the check is not applicable
+/// (PTRACE_PEEKUSER unsupported on the architecture), `false` if the
+/// deadline expired without reading a valid IP on a platform that supports it.
+fn wait_for_registers(tid: libc::pid_t, deadline: Instant) -> bool {
     #[cfg(target_arch = "x86_64")]
     const IP_OFFSET: libc::c_long = 16 * std::mem::size_of::<libc::c_long>() as libc::c_long; // RIP
 
@@ -190,17 +202,33 @@ fn wait_for_registers(tid: libc::pid_t, deadline: Instant) {
 
     const SPIN_SLEEP: Duration = Duration::from_micros(100);
 
+    // First probe: if PTRACE_PEEKUSER returns EIO, the kernel doesn't support it
+    // In that case, skip the check. libunwind uses PTRACE_GETREGSET which works
+    // regardless, and modern kernels commit register state synchronously on ptrace-stop.
+    unsafe { *libc::__errno_location() = 0 };
+    let ip = unsafe { libc::ptrace(libc::PTRACE_PEEKUSER, tid as libc::c_long, IP_OFFSET, 0) };
+    let errno = unsafe { *libc::__errno_location() };
+    if errno == libc::EIO {
+        return true;
+    }
+    if ip != 0 && errno == 0 {
+        return true;
+    }
+
     loop {
+        if Instant::now() >= deadline {
+            return false;
+        }
+        std::thread::sleep(SPIN_SLEEP);
         unsafe { *libc::__errno_location() = 0 };
         let ip = unsafe { libc::ptrace(libc::PTRACE_PEEKUSER, tid as libc::c_long, IP_OFFSET, 0) };
-        // PEEKUSER returns the register value; errno==0 means success.
-        if ip != 0 && unsafe { *libc::__errno_location() } == 0 {
-            return;
+        let errno = unsafe { *libc::__errno_location() };
+        if errno == libc::EIO {
+            return true;
         }
-        if Instant::now() >= deadline {
-            return;
+        if ip != 0 && errno == 0 {
+            return true;
         }
-        std::thread::sleep(SPIN_SLEEP);
     }
 }
 
@@ -222,6 +250,14 @@ fn detach_thread(tid: libc::pid_t) -> Result<(), PtraceError> {
             return Err(PtraceError::Detach(tid, errno));
         }
     }
+
+    // Drain any pending waitpid event so the kernel fully releases the thread.
+    // Without this, a rapid re-attach (PTRACE_SEIZE) can fail with EPERM under
+    // CPU pressure because the kernel hasn't finished processing the detach.
+    unsafe {
+        libc::waitpid(tid, ptr::null_mut(), libc::__WALL | libc::WNOHANG);
+    }
+
     Ok(())
 }
 
@@ -367,7 +403,66 @@ pub fn capture_thread_context(
 }
 
 /// Maximum time to wait for a single thread to enter ptrace-stop.
-const STOP_TIMEOUT_PER_THREAD: Duration = Duration::from_millis(50);
+const STOP_TIMEOUT_PER_THREAD: Duration = Duration::from_millis(200);
+
+/// Delay between retry attempts that is used as a base for an exponential back-off starting from
+/// this value (10ms, 20ms, 40ms).
+const RETRY_BASE_DELAY: Duration = Duration::from_millis(10);
+
+/// Maximum number of retry attempts per thread.
+const MAX_RETRIES: u32 = 3;
+
+/// Returns true if err is worth retrying.
+///
+/// EPERM (Yama denial / missing PR_SET_PTRACER) and ESRCH (thread exited)
+/// are permanent for the receiver's lifetime and are not retried.
+fn is_transient_ptrace_error(err: &PtraceError) -> bool {
+    matches!(err, PtraceError::Attach(_, libc::ETIMEDOUT))
+}
+
+/// Attempt to capture a thread context, retrying on transient failures.
+///
+/// Each attempt gets its own `STOP_TIMEOUT_PER_THREAD` budget (capped at the
+/// overall deadline) so that a retry after a timeout-induced failure actually
+/// has enough time to succeed. On older kernels (CentOS 7 / kernel 3.10)
+/// the first attempt can consume its entire budget waiting for registers to
+/// become readable; reusing that exhausted deadline would make the retry a
+/// no-op.
+///
+/// A capture that succeeds but produces zero frames is also retried: on a
+/// running thread with a confirmed non-zero IP, empty frames indicates a
+/// transient issue.
+fn capture_with_retry(
+    tid: libc::pid_t,
+    resolve_frames: crate::StacktraceCollection,
+    addr_space: UnwAddrSpaceT,
+    overall_deadline: Instant,
+) -> Option<CapturedThreadContext> {
+    for attempt in 0..=MAX_RETRIES {
+        let thread_deadline = (Instant::now() + STOP_TIMEOUT_PER_THREAD).min(overall_deadline);
+
+        match capture_thread_context(tid, resolve_frames, addr_space, thread_deadline) {
+            Ok(ctx) if !ctx.stack_trace.frames.is_empty() => return Some(ctx),
+            Ok(_) => {}                                      // 0 frames -- retry
+            Err(ref e) if is_transient_ptrace_error(e) => {} // ETIMEDOUT -- retry
+            Err(_) => return None,                           // permanent error
+        }
+
+        if attempt == MAX_RETRIES {
+            break;
+        }
+
+        let delay = RETRY_BASE_DELAY * 2u32.saturating_pow(attempt);
+        if Instant::now() + delay >= overall_deadline {
+            break;
+        }
+        std::thread::sleep(delay);
+    }
+
+    // All attempts produced 0 frames or timed out; return None so the caller
+    // records the thread with an incomplete stack rather than frames: [].
+    None
+}
 
 /// Stream thread contexts to a callback one at a time.
 ///
@@ -412,9 +507,8 @@ where
 
     // Process the crashing thread first so it is never dropped by the cap.
     if crashing_tid != 0 && tids.contains(&crashing_tid) {
-        let stop_deadline = (Instant::now() + STOP_TIMEOUT_PER_THREAD).min(overall_deadline);
         let context =
-            capture_thread_context(crashing_tid, resolve_frames, addr_space, stop_deadline).ok();
+            capture_with_retry(crashing_tid, resolve_frames, addr_space, overall_deadline);
         callback(crashing_tid, context.as_ref());
         processed += 1;
     }
@@ -423,16 +517,11 @@ where
         if tid == crashing_tid {
             continue;
         }
-        let now = Instant::now();
-        if now >= overall_deadline || processed >= max_threads {
+        if Instant::now() >= overall_deadline || processed >= max_threads {
             break;
         }
 
-        // Cap the per-thread stop wait at STOP_TIMEOUT_PER_THREAD but never
-        // past the overall deadline, so one thread can't consume the budget.
-        let stop_deadline = (now + STOP_TIMEOUT_PER_THREAD).min(overall_deadline);
-
-        let context = capture_thread_context(tid, resolve_frames, addr_space, stop_deadline).ok();
+        let context = capture_with_retry(tid, resolve_frames, addr_space, overall_deadline);
         callback(tid, context.as_ref());
         processed += 1;
     }