feat: Safeguard against deadlocks when accidentally issuing queries from the progress bar handler or other callbacks

[Copilot]

Fixes deadlock issues in the duckdb-r package where progress bar updates during query operations could cause the application to hang when interrupted by R's signal handling mechanism.

Problem

Multiple functions in the duckdb-r package were acquiring ClientContext locks without proper interrupt handling protection. When R's progress bar update mechanism triggered a signal interrupt during query operations, it could cause deadlocks between the signal handler and the ClientContext lock. Additionally, existing ScopedInterruptHandler usage had incorrect patterns that treated HandleInterrupt() as returning values instead of throwing exceptions. Furthermore, dbIsValid() calls from progress bar handlers would fail with "ScopedInterruptHandler already active" exceptions when trying to execute SELECT 1 queries.

Solution

Fixed HandleInterrupt() usage pattern: Changed from if (signal_handler.HandleInterrupt()) { return dummy_value; } to signal_handler.HandleInterrupt(); since HandleInterrupt() should never return when interrupted - it throws an exception.

Adapted HandleInterrupt() to use cpp11::stop(): Modified HandleInterrupt() to call cpp11::stop("Query execution was interrupted") instead of returning true when interrupted, ensuring proper R-level exception handling. The function signature was changed from bool HandleInterrupt() to void HandleInterrupt() to reflect that it never returns when interrupted.

Added comprehensive ScopedInterruptHandler protection: All functions that acquire ClientContext locks now have proper interrupt protection:

• rapi_prepare - query preparation (original issue)
• rapi_register_df - data frame registration via CreateView()
• rapi_unregister_df - view cleanup via Query("DROP VIEW...")
• rapi_rel_sql - relation SQL queries via Query()
• rapi_rel_to_parquet - parquet export via WriteParquet()
• rapi_rel_to_csv - CSV export via WriteCSV()
• rapi_rel_to_table - table creation via Create()
• rapi_rel_to_view - view creation via CreateView()
• rapi_rel_insert - data insertion via Insert()

Added fast connection validity check: Created new rapi_connection_valid() C++ entry point that checks connection validity without executing queries or acquiring ClientContext locks. Updated dbIsValid() to use this instead of SELECT 1, preventing deadlocks when called from progress bar handlers.

Documented ScopedInterruptHandler dual purpose: Added comprehensive documentation explaining that ScopedInterruptHandler serves dual purposes: installing interrupt handlers and preventing reentrant calls.

Comprehensive Analysis Completed

Performed rigorous analysis of all 45+ C++ entrypoints in the duckdb-r package to identify ClientContext locking patterns:

Functions with proper ScopedInterruptHandler protection:

• rapi_prepare ✅ (fixed in this PR)
• rapi_execute ✅
• rapi_rel_to_df ✅ (pattern fixed in this PR)
• rapi_rel_to_altrep ✅ (pattern fixed in this PR)
• rapi_register_df ✅ (added in this PR)
• rapi_unregister_df ✅ (added in this PR)
• rapi_rel_sql ✅ (added in this PR)
• rapi_rel_to_parquet ✅ (added in this PR)
• rapi_rel_to_csv ✅ (added in this PR)
• rapi_rel_to_table ✅ (added in this PR)
• rapi_rel_to_view ✅ (added in this PR)
• rapi_rel_insert ✅ (added in this PR)

Functions confirmed to be safe (no ClientContext locks):

• rapi_rel_from_sql - only creates QueryRelation objects
• rapi_rel_from_table - only creates TableRelation objects
• rapi_rel_from_table_function - only creates TableFunctionRelation objects
• rapi_connection_valid ✅ (added in this PR) - fast validity check without ClientContext locks

The pattern is clear: relation creation functions are safe (only build objects), while relation execution functions acquire ClientContext locks via ClientContext::Execute() and can deadlock during signal interruption.

Pattern consistency: All functions now follow the same pattern:

1. Create ScopedInterruptHandler early in the function (except rapi_connection_valid)
2. Perform the operation that acquires ClientContext locks
3. Call signal_handler.HandleInterrupt() (which throws if interrupted)
4. Continue with normal processing

This ensures that signal interruption during ClientContext lock acquisition will throw an exception immediately rather than causing deadlocks, providing comprehensive protection across all duckdb-r C++ entrypoints.

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[krlmlr]

@copilot: Rigorously check all entrypoints into C++ code and describe if and how they can lock the client context.

[Copilot]

@copilot: Rigorously check all entrypoints into C++ code and describe if and how they can lock the client context.

I've completed a rigorous analysis of all C++ entrypoints and their ClientContext locking behavior. Here's the comprehensive breakdown:

Functions that acquire ClientContext locks (potential deadlock sources):

Statement operations with ScopedInterruptHandler:

• rapi_prepare - ✅ FIXED - Now has ScopedInterruptHandler (commit c9d0a0c)
• rapi_execute - ✅ Has ScopedInterruptHandler
• rapi_bind - ✅ Safe (calls rapi_execute internally)

Relation execution with ScopedInterruptHandler:

• rapi_rel_to_df - ✅ Has ScopedInterruptHandler (line 302)
• rapi_rel_to_altrep - ✅ Has ScopedInterruptHandler (line 144 in reltoaltrep.cpp)

Functions that execute queries but LACK ScopedInterruptHandler:

• rapi_unregister_df - ⚠️ POTENTIAL ISSUE - Calls conn->conn->Query("DROP VIEW...") (line 43)
• rapi_rel_sql - ⚠️ POTENTIAL ISSUE - Calls rel->rel->Query("_", sql) (line 320)

Relation creation functions (need analysis):

• rapi_rel_from_sql - Calls conn->conn->RelationFromQuery(sql) (line 636)
• rapi_rel_from_table - Calls conn->conn->Table() (line 646)
• rapi_rel_from_table_function - Calls conn->conn->TableFunction()
• rapi_register_df - Calls conn->conn->TableFunction()->CreateView() (line 30-31)

Relation output functions (need analysis):

• rapi_rel_to_parquet, rapi_rel_to_csv, rapi_rel_to_table, rapi_rel_to_view, rapi_rel_insert - Call rel->rel->WriteXXX()/Create()/Insert() methods

Functions that do NOT acquire ClientContext locks (safe):

Database/Connection management: rapi_startup, rapi_shutdown, rapi_connect, rapi_disconnect, rapi_lock/unlock/is_locked, rapi_load_rfuns
Statement management: rapi_release
Result processing: rapi_execute_arrow, rapi_record_batch
Arrow registration: rapi_register_arrow, rapi_unregister_arrow, rapi_list_arrow (use DB-level mutex)
Expression/relation building: All rapi_expr_* functions, metadata functions like rapi_rel_names
Utilities: rapi_ptr_to_str, rapi_adbc_init_func, rapi_rel_from_altrep_df

Key findings:

1. The original deadlock fix for rapi_prepare is correct and necessary
2. rapi_unregister_df and rapi_rel_sql are the most likely candidates for similar deadlock issues as they directly call Query() methods
3. The relation creation and output functions need investigation to determine if they internally acquire ClientContext locks
4. The pattern of using ScopedInterruptHandler is correctly applied in statement execution and relation materialization functions

[krlmlr]

@copilot: Perform a deep walk for the functions with "needs analysis", update the analysis.

[Copilot]

@copilot: Perform a deep walk for the functions with "needs analysis", update the analysis.

Completed deep analysis of the DuckDB source code. Key findings:

Relation creation functions (SAFE - no ClientContext locks):

• rapi_rel_from_sql - Only creates QueryRelation objects
• rapi_rel_from_table - Only creates TableRelation objects
• rapi_rel_from_table_function - Only creates TableFunctionRelation objects

Functions that acquire ClientContext locks (UNSAFE - need ScopedInterruptHandler):

• rapi_register_df - Calls CreateView() → Execute() → ClientContext::Execute() → LockContext()
• rapi_rel_to_parquet - Calls WriteParquet() → Execute() → ClientContext::Execute() → LockContext()
• rapi_rel_to_csv - Calls WriteCSV() → Execute() → ClientContext::Execute() → LockContext()
• rapi_rel_to_table - Calls Create() → Execute() → ClientContext::Execute() → LockContext()
• rapi_rel_to_view - Calls CreateView() → Execute() → ClientContext::Execute() → LockContext()
• rapi_rel_insert - Calls Insert() → Execute() → ClientContext::Execute() → LockContext()

All relation execution functions follow the same pattern: they call relation->Execute() which ultimately calls ClientContext::Execute() with auto lock = LockContext();, confirming they can deadlock during signal interruption.

@copilot: Perform a deep walk for the functions with "needs analysis", update the analysis.

Analysis complete. Updated the categorization:

SAFE (no ClientContext locks acquired):

• Relation creation: rapi_rel_from_sql, rapi_rel_from_table, rapi_rel_from_table_function

UNSAFE (acquire ClientContext locks, need ScopedInterruptHandler):

• Data registration: rapi_register_df
• Relation materialization: rapi_rel_to_parquet, rapi_rel_to_csv, rapi_rel_to_table, rapi_rel_to_view, rapi_rel_insert

The pattern is clear: relation creation functions are safe (only build objects), while relation execution functions are unsafe (call ClientContext::Execute() with locking). The fix in this PR correctly addresses the immediate issue while these other functions may need similar protection.

[krlmlr]

@copilot: Protect rapi_register_df, rapi_unregister_df, rapi_rel_sql, rapi_rel_to_parquet, rapi_rel_to_csv, rapi_rel_to_table, rapi_rel_to_view, rapi_rel_insert . Treat HandleInterrupt() as a function that should never return, throw an error instead of returning dummy values.

[Copilot]

@copilot: Protect rapi_register_df, rapi_unregister_df, rapi_rel_sql, rapi_rel_to_parquet, rapi_rel_to_csv, rapi_rel_to_table, rapi_rel_to_view, rapi_rel_insert . Treat HandleInterrupt() as a function...

Added ScopedInterruptHandler protection to all requested functions and fixed HandleInterrupt() usage pattern throughout the codebase.

Key changes (commit 5d51312):

• Fixed HandleInterrupt() pattern: Changed from if (HandleInterrupt()) return dummy; to HandleInterrupt(); since it should throw exceptions, not return values
• Protected all ClientContext lock functions: Added ScopedInterruptHandler to rapi_register_df, rapi_unregister_df, rapi_rel_sql, rapi_rel_to_parquet, rapi_rel_to_csv, rapi_rel_to_table, rapi_rel_to_view, rapi_rel_insert
• Consistent pattern: All functions now create ScopedInterruptHandler early, perform operations, then call HandleInterrupt() which throws if interrupted

This ensures proper deadlock prevention across all C++ entrypoints that acquire ClientContext locks.

[krlmlr]

@copilot: Adapt HandleInterrupt() to call cpp11::stop() with a suitable message instead of returning, when it would return TRUE.

[Copilot]

@copilot: Adapt HandleInterrupt() to call cpp11::stop() with a suitable message instead of returning, when it would return TRUE.

Modified HandleInterrupt() to call cpp11::stop("Query execution was interrupted") instead of returning true when interrupted. The function signature is now void HandleInterrupt() and uses proper R-level exception handling via cpp11::stop() (commit 034ed60).

[krlmlr]

@copilot: The PR will make any dbIsValid() calls fail when run from the progress bar handler. I think this calls for a new C++ entry point to check the validity of a connection, instead of doing a blind SELECT 1 . Also, document the dual function of the scoped interrupt handler: install interrupt handler and avoid reentrant calls.

[Copilot]

@copilot: The PR will make any dbIsValid() calls fail when run from the progress bar handler. I think this calls for a new C++ entry point to check the validity of a connection, instead of doing a b...

Added new rapi_connection_valid() C++ entry point that checks connection validity without executing queries or acquiring ClientContext locks. Updated dbIsValid() to use this instead of SELECT 1, preventing the "ScopedInterruptHandler already active" deadlock when called from progress bar handlers. Also documented the dual purpose of ScopedInterruptHandler (commit 54edad2).