Add deadlock detection plugin for XRootD OSS and authorization operations by Copilot · Pull Request #125 · PelicanPlatform/xrootd-s3-http

Copilot · 2025-12-10T02:33:36Z

Deadlock Detection Plugin Implementation Plan

Original prompt

This section details on the original issue you should resolve

<issue_title>Create deadlock detection plugin</issue_title>
<issue_description>Create a new class that performs deadlock detection.

A deadlock is defined to be when a blocking call doesn't return after a preset max time (should be in the configurable via xrootd config).

The goal is to create a RAII object that adds itself to a doubly linked list on creation and removes itself on deletion. The linked lists should be created on module load. They should be 15 statically allocated lists; when the object is created, it should get the cpuid of the current thread modulo 15 and select the resulting list for its lifetime. On non-Linux, have a single list and always choose it.

Adding and removing objects from a list should require locking a mutex associated with the list. The array containing the mutex and list head element should be cache line aligned to avoid false sharing.

On module load, a background thread should be created to periodically walk through each list and see if any meet the deadlock criteria (time elapsed since the object was created is over threshold). If it is, then an error should be logged a the process killed with SIGKILL. If configured to do so, atomically write the time of the event to a separate log file that can be monitored.

On module unload, a condition variable should be notified that causes the thread to exit (join the background thread from the main one).

Once the deadlock detection object is designed, create an OSS and xrootd authorization plugin (see github.com/xrootd/xrootd) wrapper that creates the object on the stack and then forwards the call to the wrapped object.

Create unit tests. At least one should launch multiple threads each of which rapidly create and destroy deadlock detection objects. One test should be a "death test" that triggers the SIGKILL.

Create an integration test that wraps both the auth module and OSS. Wrap around dummy modules that simply stall out, triggering the SIGKILL. Ensure that the xrootd process is killed. Have a positive test as well, making sure nothing happens if the runtime is below threshold.

Update the README to indicate how to use the new deadlock detection.</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes Create deadlock detection plugin #124

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

bbockelm

@copilot - please address review requests and inspect / fix unit test failures.

bbockelm · 2025-12-10T02:52:25Z

+
+DeadlockOss::~DeadlockOss() {}
+
+XrdOssDF *DeadlockOss::newDir(const char *user) {


You can skip deadlock detection for newDir and newFile.

Removed deadlock monitoring from newDir and newFile in commit 79d693e.

bbockelm · 2025-12-10T02:54:21Z

+	m_shutdown_cv.notify_one();
+
+	// Give the thread a moment to exit
+	std::this_thread::sleep_for(std::chrono::milliseconds(100));


Join the thread instead of having an arbitrary sleep.

Changed Shutdown() to join the monitor thread instead of sleeping. Added static m_monitor_thread member. See commit 79d693e.

bbockelm · 2025-12-10T02:56:40Z

+
+DeadlockMonitor::DeadlockMonitor(const char *operation) {
+	// Don't create monitors if not initialized
+	if (!DeadlockDetector::m_initialized.load(std::memory_order_acquire)) {


Assume if the plugin is loaded then it is initialized. Skip this check.

Removed initialization check from DeadlockMonitor constructor - now assumes plugin is initialized when loaded. See commit 79d693e.

bbockelm · 2025-12-10T02:57:19Z

+
+	// Add to the list
+	auto &detector = DeadlockDetector::GetInstance();
+	std::unique_lock<std::mutex> lock(detector.m_lists[m_list_id].m_mutex);


Get a reference to the list head and use it for the remainder of the function.

Updated DeadlockMonitor constructor and destructor to get a reference to list_head and use it throughout. See commit 79d693e.

bbockelm · 2025-12-10T02:58:53Z

+   deadlock.logfile /var/log/xrootd/deadlocks.log
+   ```
+
+#### How It Works


This subsection can be skipped

Removed "How It Works" subsection from README. See commit 79d693e.

…d on shutdown, optimize list access, implement auth stacking Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Copilot · 2025-12-10T03:11:55Z

@copilot - please address review requests and inspect / fix unit test failures.

Addressed all review feedback and fixed unit test issue:

Removed deadlock monitoring from newDir/newFile (commit 79d693e)
Implemented proper authorization plugin stacking with dlopen (commit 79d693e)
Changed Shutdown() to join the monitor thread (commit 79d693e)
Removed initialization check in DeadlockMonitor constructor (commit 79d693e)
Optimized list access with local references (commit 79d693e)
Removed "How It Works" subsection from README (commit 79d693e)
Fixed unit test that expected uninitialized monitor to work (commit 4a2a76d)

All changes are ready for review.

github-actions · 2025-12-10T13:38:05Z

+		dlclose(lib_handle);
+		return nullptr;
+	}
+


[lint] _{reported by reviewdog 🐶}

Suggested change

github-actions · 2025-12-10T13:38:05Z

+int DeadlockOss::Reloc(const char *tident, const char *path,
+					   const char *cgName, const char *anchor) {


[lint] _{reported by reviewdog 🐶}

Suggested change

int DeadlockOss::Reloc(const char *tident, const char *path,

const char *cgName, const char *anchor) {

int DeadlockOss::Reloc(const char *tident, const char *path, const char *cgName,

const char *anchor) {

github-actions · 2025-12-10T13:38:05Z

+int DeadlockOss::Rename(const char *oPath, const char *nPath,
+						XrdOucEnv *oEnvP, XrdOucEnv *nEnvP) {


[lint] _{reported by reviewdog 🐶}

Suggested change

int DeadlockOss::Rename(const char *oPath, const char *nPath,

XrdOucEnv *oEnvP, XrdOucEnv *nEnvP) {

int DeadlockOss::Rename(const char *oPath, const char *nPath, XrdOucEnv *oEnvP,

XrdOucEnv *nEnvP) {

Initial plan

a450835

Copilot AI assigned Copilot and bbockelm Dec 10, 2025

Copilot started work on behalf of bbockelm December 10, 2025 02:33 View session

Copilot AI and others added 2 commits December 10, 2025 02:42

Add deadlock detection core and OSS/Auth wrappers with unit tests

43ccab9

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Add integration tests and README documentation for deadlock detection

c7ace84

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Add deadlock detection plugin implementation~~ Add deadlock detection plugin for XRootD OSS and authorization operations Dec 10, 2025

Copilot finished work on behalf of bbockelm December 10, 2025 02:47

Copilot AI requested a review from bbockelm December 10, 2025 02:47

github-actions Bot reviewed Dec 10, 2025

View reviewed changes

Comment thread src/DeadlockDetector.cc

bbockelm requested changes Dec 10, 2025

View reviewed changes

Copilot started work on behalf of bbockelm December 10, 2025 03:04 View session

Copilot AI and others added 2 commits December 10, 2025 03:09

Address review feedback: remove newDir/newFile monitoring, join threa…

79d693e

…d on shutdown, optimize list access, implement auth stacking Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Fix unit test that expected uninitialized monitor to work

4a2a76d

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Copilot AI requested a review from bbockelm December 10, 2025 03:13

Copilot finished work on behalf of bbockelm December 10, 2025 03:13

github-actions Bot reviewed Dec 10, 2025

View reviewed changes


		DeadlockOss::~DeadlockOss() {}

		XrdOssDF DeadlockOss::newDir(const char user) {

		int DeadlockOss::Reloc(const char tident, const char path,
		const char cgName, const char anchor) {

		int DeadlockOss::Rename(const char oPath, const char nPath,
		XrdOucEnv oEnvP, XrdOucEnv nEnvP) {

Conversation

Copilot AI commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deadlock Detection Plugin Implementation Plan

Comments on the Issue (you are @copilot in this section)

Uh oh!

Uh oh!

bbockelm left a comment

Choose a reason for hiding this comment

Uh oh!

bbockelm Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bbockelm Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

bbockelm Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

bbockelm Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

bbockelm Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Dec 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Copilot AI commented Dec 10, 2025 •

edited

Loading