Bugfix/refine code lock

[iceljc]

PR Type

Bug fix, Enhancement

---

Description

• Convert code processor from async to synchronous execution model

• Remove centralized CodeScriptExecutor and integrate locking directly into PyCodeInterpreter

• Replace async/await patterns with synchronous SemaphoreSlim.Wait() for code execution

• Simplify concurrency control by removing MaxConcurrency configuration setting

• Remove Python thread interruption logic from cancellation handling

---

Diagram Walkthrough

flowchart LR
  A["ICodeProcessor<br/>RunAsync"] -->|"Convert to"| B["ICodeProcessor<br/>Run"]
  C["CodeScriptExecutor<br/>Removed"] -->|"Integrate into"| D["PyCodeInterpreter<br/>SemaphoreSlim"]
  E["Async Execution"] -->|"Simplify to"| F["Sync Execution"]
  G["MaxConcurrency<br/>Config"] -->|"Remove"| H["Fixed Concurrency"]

Loading

File Walkthrough

	Relevant files
Enhancement	4 files ICodeProcessor.cs Convert RunAsync method to synchronous Run                              +1/-1      RuleEngine.cs Update code processor call from async to sync                        +1/-1      InstructService.Execute.cs Update code processor call from async to sync                        +1/-1      PyProgrammerFn.cs Convert InnerRunCode from async to synchronous                      +3/-3
Configuration changes	4 files CodingSettings.cs Remove MaxConcurrency configuration property                          +0/-1      CodingPlugin.cs Remove CodeScriptExecutor dependency injection                      +0/-2      ConversationPlugin.cs Remove CodeScriptExecutor singleton registration                  +0/-1      appsettings.json Remove MaxConcurrency from coding configuration                    +1/-2
Bug fix	2 files CodeScriptExecutor.cs Delete centralized code script executor class                        +0/-39    PyCodeInterpreter.cs Integrate locking logic and convert to synchronous execution +34/-35

---

[qodo-merge-pro]

PR Compliance Guide 🔍

(Compliance updated until commit cd6eb1c)

Below is a summary of compliance checks for this PR:

Security Compliance
🔴	Unstoppable execution Description: Python in-process execution now ignores cancellation mid-execution (removed PythonEngine.Interrupt and ThrowIfCancellationRequested checks) so long-running or malicious scripts cannot be aborted, allowing unbounded CPU time and resource consumption until the external timeout, facilitating DoS. PyCodeInterpreter.cs [154-235] Referred Code private CodeInterpretResponse CoreRunScript(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); var execTask = Task.Factory.StartNew(() => { // For observation purpose var requestId = Guid.NewGuid(); _logger.LogWarning($"Before acquiring Py.GIL for request {requestId}"); using (Py.GIL()) { _logger.LogWarning($"After acquiring Py.GIL for request {requestId}"); // Import necessary Python modules dynamic sys = Py.Import("sys"); dynamic io = Py.Import("io"); try { // Redirect standard output/error to capture it ... (clipped 61 lines)
⚪	Deadlock/DoS risk Description: Replacing the async lock executor with a static global SemaphoreSlim and synchronous Wait introduces potential thread starvation and deadlock under ASP.NET synchronization contexts, enabling a trivial denial-of-service if code execution threads exhaust the thread pool while holding the lock. PyCodeInterpreter.cs [116-115] Referred Code }
	Sync-over-async deadlock Description: Synchronous blocking of async operations via ConfigureAwait(false).GetAwaiter().GetResult() on CoreRunProcess can deadlock under certain hosting contexts and ties up thread-pool threads, increasing DoS surface. PyCodeInterpreter.cs [121-134] Referred Code var scriptName = options?.ScriptName ?? codeScript.SubstringMax(30); try { _logger.LogWarning($"Begin running python code script in {Provider}: {scriptName}"); if (options?.UseProcess == true) { response = CoreRunProcess(codeScript, options, cancellationToken).ConfigureAwait(false).GetAwaiter().GetResult(); } else { response = CoreRunScript(codeScript, options, cancellationToken); }
	Information disclosure Description: Standard output and error from executed Python scripts are fully captured and returned without sanitization, which can leak sensitive runtime data or system information to callers if scripts print environment or error details. PyCodeInterpreter.cs [154-218] Referred Code private CodeInterpretResponse CoreRunScript(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); var execTask = Task.Factory.StartNew(() => { // For observation purpose var requestId = Guid.NewGuid(); _logger.LogWarning($"Before acquiring Py.GIL for request {requestId}"); using (Py.GIL()) { _logger.LogWarning($"After acquiring Py.GIL for request {requestId}"); // Import necessary Python modules dynamic sys = Py.Import("sys"); dynamic io = Py.Import("io"); try { // Redirect standard output/error to capture it ... (clipped 44 lines)
	Cross-tenant interference Description: Removing centralized executor-based isolation consolidates locking into a single static semaphore across the app domain without per-tenant or per-agent scoping, enabling cross-tenant execution interference and potential priority inversion. PyCodeInterpreter.cs [30-38] Referred Code public CodeInterpretResponse Run(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) { if (options?.UseLock == true) { return InnerRunWithLock(codeScript, options, cancellationToken); } return InnerRunCode(codeScript, options, cancellationToken); }
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: Critical code execution actions run and complete are logged at warning level but lack user/context identifiers, making audit trails potentially insufficient. Referred Code var scriptName = options?.ScriptName ?? codeScript.SubstringMax(30); try { _logger.LogWarning($"Begin running python code script in {Provider}: {scriptName}"); if (options?.UseProcess == true) { response = CoreRunProcess(codeScript, options, cancellationToken).ConfigureAwait(false).GetAwaiter().GetResult(); } else { response = CoreRunScript(codeScript, options, cancellationToken); } _logger.LogWarning($"End running python code script in {Provider}: {scriptName}"); return response; Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Generic error context: Errors caught during code execution return only exception messages without richer context (e.g., script name, options, args), limiting diagnosability. Referred Code catch (Exception ex) { _logger.LogError(ex, $"Error when executing code script ({scriptName}) in {nameof(InnerRunCode)} in {Provider}."); response.ErrorMsg = ex.Message; return response; } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Error exposure risk: Returning raw exception messages in CodeInterpretResponse.ErrorMsg may expose internal details to callers that could be user-facing. Referred Code catch (Exception ex) { _logger.LogError(ex, $"Error when executing code script ({scriptName}) in {nameof(InnerRunCode)} in {Provider}."); response.ErrorMsg = ex.Message; return response; } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Log content risk: Logging full script names and generic warnings without explicit redaction policies risks logging sensitive script content or arguments depending on upstream usage. Referred Code var scriptName = options?.ScriptName ?? codeScript.SubstringMax(30); try { _logger.LogWarning($"Begin running python code script in {Provider}: {scriptName}"); if (options?.UseProcess == true) { response = CoreRunProcess(codeScript, options, cancellationToken).ConfigureAwait(false).GetAwaiter().GetResult(); } else { response = CoreRunScript(codeScript, options, cancellationToken); } _logger.LogWarning($"End running python code script in {Provider}: {scriptName}"); return response; Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Unvalidated input: Arbitrary codeScript is executed via Python without visible validation or sandboxing, and process execution path may allow command injection via arguments. Referred Code private CodeInterpretResponse CoreRunScript(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); var execTask = Task.Factory.StartNew(() => { // For observation purpose var requestId = Guid.NewGuid(); _logger.LogWarning($"Before acquiring Py.GIL for request {requestId}"); using (Py.GIL()) { _logger.LogWarning($"After acquiring Py.GIL for request {requestId}"); // Import necessary Python modules dynamic sys = Py.Import("sys"); dynamic io = Py.Import("io"); try { // Redirect standard output/error to capture it ... (clipped 61 lines) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

---

Previous compliance checks

Compliance check up to commit 558f01d

Security Compliance
⚪	Blocking async deadlock Description: Potential deadlock and thread-pool starvation by synchronously blocking on Task and using .GetAwaiter().GetResult()/WaitAsync for Python execution, which can block the calling thread (including ASP.NET request threads), ignore cooperative cancellation inside the Python code path, and lead to denial-of-service under load. PyCodeInterpreter.cs [162-243] Referred Code private CodeInterpretResponse CoreRunScript(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); var execTask = Task.Factory.StartNew(() => { // For observation purpose var requestId = Guid.NewGuid(); _logger.LogWarning($"Before acquiring Py.GIL for request {requestId}"); using (Py.GIL()) { _logger.LogWarning($"After acquiring Py.GIL for request {requestId}"); // Import necessary Python modules dynamic sys = Py.Import("sys"); dynamic io = Py.Import("io"); try { // Redirect standard output/error to capture it ... (clipped 61 lines)
	Ineffective cancellation Description: Switching from asynchronous RunAsync to synchronous Run removes awaited cancellation propagation, increasing the risk that long-running or malicious Python scripts cannot be cancelled in a timely manner and may exhaust resources. PyCodeInterpreter.cs [30-46] Referred Code public CodeInterpretResponse Run(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) { if (options?.UseLock == true) { try { return InnerRunWithLock(codeScript, options, cancellationToken); } catch (Exception ex) { _logger.LogError(ex, $"Error when using code script with {nameof(SemaphoreSlim)}."); return new() { ErrorMsg = ex.Message }; } } return InnerRunCode(codeScript, options, cancellationToken); }
	Global lock DoS risk Description: The global static SemaphoreSlim used for "UseLock" introduces a cross-tenant/process-wide bottleneck without a timeout or fairness, allowing any caller to monopolize the interpreter lock and potentially cause denial-of-service if Release is not reached due to unforeseen exceptions. PyCodeInterpreter.cs [101-126] Referred Code #region Private methods private CodeInterpretResponse InnerRunWithLock(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) { var lockAcquired = false; try { _semLock.Wait(cancellationToken); lockAcquired = true; return InnerRunCode(codeScript, options, cancellationToken); } catch (Exception ex) { _logger.LogError(ex, $"Error in {nameof(InnerRunWithLock)}"); return new() { ErrorMsg = ex.Message }; } finally { if (lockAcquired) { _semLock.Release(); ... (clipped 5 lines)
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Cancellation removed: The change removes Python execution cancellation/interrupt handling and multiple cancellation checks, weakening robustness against timeouts and preventing graceful termination of long-running scripts. Referred Code _logger.LogError(ex, $"Error when executing code script ({scriptName}) in {nameof(InnerRunCode)} in {Provider}."); response.ErrorMsg = ex.Message; return response; } } private CodeInterpretResponse CoreRunScript(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); var execTask = Task.Factory.StartNew(() => { // For observation purpose var requestId = Guid.NewGuid(); _logger.LogWarning($"Before acquiring Py.GIL for request {requestId}"); using (Py.GIL()) { _logger.LogWarning($"After acquiring Py.GIL for request {requestId}"); // Import necessary Python modules ... (clipped 28 lines) Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing logging: Execution of potentially sensitive code scripts now occurs via synchronous methods without added audit logs for user identity, action description, and outcome beyond generic start/end warnings, which may be insufficient for comprehensive audit trails. Referred Code var scriptName = options?.ScriptName ?? codeScript.SubstringMax(30); try { _logger.LogWarning($"Begin running python code script in {Provider}: {scriptName}"); if (options?.UseProcess == true) { response = CoreRunProcess(codeScript, options, cancellationToken).ConfigureAwait(false).GetAwaiter().GetResult(); } else { response = CoreRunScript(codeScript, options, cancellationToken); } _logger.LogWarning($"End running python code script in {Provider}: {scriptName}"); return response; Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Vague variable: The variable name ret provides little semantic meaning for the deserialized LLM response context and could be more descriptive. Referred Code { Id = agent?.Id ?? BuiltInAgentId.AIProgrammer, Name = agent?.Name ?? "AI Programmer", Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Error detail risk: Returning ex.Message in responses (e.g., when lock acquisition fails) may expose internal details to callers depending on how CodeInterpretResponse.ErrorMsg is surfaced. Referred Code _logger.LogError(ex, $"Error when using code script with {nameof(SemaphoreSlim)}."); return new() { ErrorMsg = ex.Message }; } } return InnerRunCode(codeScript, options, cancellationToken); } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Log content risk: Logging script names and generic messages without structured context controls could inadvertently include sensitive data if ScriptName or arguments contain PII, and logs are unstructured warnings. Referred Code _logger.LogWarning($"Begin running python code script in {Provider}: {scriptName}"); if (options?.UseProcess == true) { response = CoreRunProcess(codeScript, options, cancellationToken).ConfigureAwait(false).GetAwaiter().GetResult(); } else { response = CoreRunScript(codeScript, options, cancellationToken); } _logger.LogWarning($"End running python code script in {Provider}: {scriptName}"); return response; Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Exec without validation: The interpreter executes provided Python code and forwards arguments to sys.argv without added validation or sanitization in the new code, which could permit unsafe inputs depending on upstream controls not shown in the diff. Referred Code { list.Append(new PyString($"--{arg.Key}")); list.Append(new PyString($"{arg.Value}")); } } } sys.argv = list; // Execute Python script PythonEngine.Exec(codeScript, globals); _logger.LogWarning($"Complete {nameof(CoreRunScript)} in {Provider}: ${options?.ScriptName}"); // Get result var stdout = outIO.getvalue()?.ToString() as string; var stderr = errIO.getvalue()?.ToString() as string; return new CodeInterpretResponse Learn more about managing compliance generic rules or creating your own custom rules

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Restore script cancellation support Restore the removed cancellation logic that uses PythonEngine.Interrupt to allow for the termination of long-running or stuck Python scripts when a cancellation is requested. src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [162-243] -private CodeInterpretResponse CoreRunScript(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) +private async Task<CodeInterpretResponse> CoreRunScript(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); - var execTask = Task.Factory.StartNew(() => + return await Task.Run(() => { // For observation purpose var requestId = Guid.NewGuid(); _logger.LogWarning($"Before acquiring Py.GIL for request {requestId}"); using (Py.GIL()) { _logger.LogWarning($"After acquiring Py.GIL for request {requestId}"); // Import necessary Python modules dynamic sys = Py.Import("sys"); dynamic io = Py.Import("io"); try { + // Capture the Python thread ID for the current thread executing under the GIL + var pythonThreadId = PythonEngine.GetPythonThreadID(); + using var reg = cancellationToken.Register(() => + { + try + { + PythonEngine.Interrupt(pythonThreadId); + _logger.LogWarning($"Cancellation requested: issued PythonEngine.Interrupt for thread {pythonThreadId} (request {requestId})"); + } + catch (Exception ex) + { + _logger.LogError(ex, "Failed to interrupt Python execution on cancellation."); + } + }); + // Redirect standard output/error to capture it dynamic outIO = io.StringIO(); dynamic errIO = io.StringIO(); sys.stdout = outIO; sys.stderr = errIO; // Set arguments var list = new PyList(); if (options?.Arguments?.Any() == true) { foreach (var arg in options.Arguments) { list.Append(new PyString(arg.Value)); } } sys.argv = list; + cancellationToken.ThrowIfCancellationRequested(); + // Execute Python script PythonEngine.Exec(codeScript, globals); _logger.LogWarning($"Complete {nameof(CoreRunScript)} in {Provider}: ${options?.ScriptName}"); // Get result var stdout = outIO.getvalue()?.ToString() as string; var stderr = errIO.getvalue()?.ToString() as string; + + cancellationToken.ThrowIfCancellationRequested(); return new CodeInterpretResponse { Result = stdout?.TrimEnd('\r', '\n') ?? string.Empty, Success = true }; } catch (Exception ex) { _logger.LogError(ex, $"Error in {nameof(CoreRunScript)} in {Provider}."); return new() { ErrorMsg = ex.Message }; } finally { // Restore the original stdout/stderr/argv sys.stdout = sys.__stdout__; sys.stderr = sys.__stderr__; sys.argv = new PyList(); } } }, cancellationToken); - - return execTask.WaitAsync(cancellationToken).ConfigureAwait(false).GetAwaiter().GetResult(); } Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly points out the removal of a critical feature for handling long-running Python scripts. Restoring the cancellation logic with PythonEngine.Interrupt is crucial for preventing resource exhaustion and ensuring the application remains responsive.	High
	Revert to asynchronous method signature Revert the ICodeProcessor.Run method and its implementation back to an asynchronous signature (async Task<...>) to avoid blocking calls and prevent performance issues like thread pool starvation in an async environment. src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [30-46] -public CodeInterpretResponse Run(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) +public async Task<CodeInterpretResponse> RunAsync(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) { if (options?.UseLock == true) { try { - return InnerRunWithLock(codeScript, options, cancellationToken); + return await InnerRunWithLock(codeScript, options, cancellationToken); } catch (Exception ex) { _logger.LogError(ex, $"Error when using code script with {nameof(SemaphoreSlim)}."); return new() { ErrorMsg = ex.Message }; } } - return InnerRunCode(codeScript, options, cancellationToken); + return await InnerRunCode(codeScript, options, cancellationToken); } Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that changing the ICodeProcessor.Run method to be synchronous is a fundamental design flaw in an asynchronous environment, leading to blocking calls and potential thread-pool starvation. Reverting to an async pattern is critical for application performance and stability.	High
	Use async lock to prevent blocking Convert the InnerRunWithLock method to be asynchronous by using _semLock.WaitAsync() instead of the blocking _semLock.Wait() to prevent potential thread pool starvation. src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [103-124] -private CodeInterpretResponse InnerRunWithLock(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) +private async Task<CodeInterpretResponse> InnerRunWithLock(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default) { - var lockAcquired = false; + await _semLock.WaitAsync(cancellationToken); try { - _semLock.Wait(cancellationToken); - lockAcquired = true; - return InnerRunCode(codeScript, options, cancellationToken); + return await InnerRunCode(codeScript, options, cancellationToken); } catch (Exception ex) { _logger.LogError(ex, $"Error in {nameof(InnerRunWithLock)}"); return new() { ErrorMsg = ex.Message }; } finally { - if (lockAcquired) - { - _semLock.Release(); - } + _semLock.Release(); } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that using the blocking _semLock.Wait() in an async-over-sync pattern can lead to thread pool starvation, which is a significant performance and scalability issue in a web application.	Medium
Update