Skip to content

revert code with lock #1217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Nov 14, 2025
Merged

revert code with lock #1217

merged 2 commits into from
Nov 14, 2025

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Nov 14, 2025 •
edited by qodo-merge-pro bot
Loading

PR Type

Bug fix, Enhancement

Description

  • Add exception handling in Run method for lock-based execution

  • Simplify lock acquisition logic by removing lockAcquired flag

  • Always release semaphore in finally block without conditional check

  • Add thread sleep delay before GIL acquisition for debugging/observation

Diagram Walkthrough

flowchart LR
  A["Run method"] -- "UseLock == true" --> B["Try InnerRunWithLock"]
  B -- "Exception caught" --> C["Log error and return ErrorMsg"]
  B -- "Success" --> D["Return response"]
  E["InnerRunWithLock"] -- "Acquire semaphore" --> F["Execute code"]
  F -- "Finally block" --> G["Always release semaphore"]
Loading

File Walkthrough
Relevant files
Bug fix, error handling, enhancement
PyCodeInterpreter.cs
Enhanced error handling and simplified lock management     

src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs

  • Added try-catch block in Run method to handle exceptions from
    InnerRunWithLock and return error message
  • Removed lockAcquired flag variable and simplified lock release logic
    in InnerRunWithLock
  • Changed semaphore acquisition to occur before try block, ensuring
    unconditional release in finally
  • Added Thread.Sleep(100) in CoreRunScript before GIL acquisition for
    debugging purposes
 +14/-8   

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Nov 14, 2025 •
edited
Loading

PR Compliance Guide 🔍

(Compliance updated until commit cf9b7fa)

Below is a summary of compliance checks for this PR:

Security Compliance
⚪
Sensitive information exposure

Description: Logging the caught exception with string interpolation that includes the dynamic provider
name ({Provider}) may expose sensitive runtime details and internal identifiers in logs,
increasing the risk of information disclosure if logs are accessible to untrusted parties.

PyCodeInterpreter.cs [101-109]

Referred Code 
    _semLock.Wait(cancellationToken);
    lockAcquired = true;
    return InnerRunCode(codeScript, options, cancellationToken);
}
catch (Exception ex)
{
    _logger.LogError(ex, $"Error in {nameof(InnerRunWithLock)} in {Provider}");
    return new() { ErrorMsg = ex.Message };
}
Denial of service

Description: Introducing a fixed Thread.Sleep(100) before acquiring the Python GIL can enable trivial
thread blocking and resource exhaustion under load, creating a denial-of-service
amplification vector if attackers can trigger many executions concurrently.
PyCodeInterpreter.cs [161-163]

Referred Code 
Thread.Sleep(100);

// For observation purpose
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:
Insufficient Auditing: Newly added error handling and lock execution paths add minimal logging without
user/context identifiers, which may be insufficient to reconstruct critical events.

Referred Code 
try
{
    _semLock.Wait(cancellationToken);
    lockAcquired = true;
    return InnerRunCode(codeScript, options, cancellationToken);
}
catch (Exception ex)
{
    _logger.LogError(ex, $"Error in {nameof(InnerRunWithLock)} in {Provider}");
    return new() { ErrorMsg = ex.Message };

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 Return: The catch block returns only ex.Message without structured context (e.g., operation,
requestId), which may limit actionable debugging while possibly exposing internal details.

Referred Code 
catch (Exception ex)
{
    _logger.LogError(ex, $"Error in {nameof(InnerRunWithLock)} in {Provider}");
    return new() { ErrorMsg = ex.Message };
}

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 Message Exposure: Returning ex.Message in the response may expose internal details to users if this value is
user-facing.

Referred Code 
    _logger.LogError(ex, $"Error in {nameof(InnerRunWithLock)} in {Provider}");
    return new() { ErrorMsg = ex.Message };
}

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:
Unstructured Logging: Added logs use free-form strings (e.g., requestId and Provider) rather than structured
fields, which may hinder auditing and risk accidental sensitive data inclusion.

Referred Code 
// For observation purpose
var requestId = Guid.NewGuid();
_logger.LogWarning($"Before acquiring Py.GIL for request {requestId}");

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:
Input Validation Omission: The new paths execute codeScript with added logging and delays but do not show validation
or sanitization of the script input in the changed code.

Referred Code 
{
    var lockAcquired = false;

    try
    {
        _semLock.Wait(cancellationToken);
        lockAcquired = true;
        return InnerRunCode(codeScript, options, cancellationToken);
    }

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 99c3469
Security Compliance
🔴
Concurrency misuse

Description: Potential semaphore over-release: calling _semLock.Release() unconditionally in finally
after Wait(cancellationToken) may release without a successful wait if Wait throws (e.g.,
cancellation), leading to semaphore count corruption or runtime exceptions.
PyCodeInterpreter.cs [105-119]

Referred Code 
_semLock.Wait(cancellationToken);

try
{
    return InnerRunCode(codeScript, options, cancellationToken);
}
catch (Exception ex)
{
    _logger.LogError(ex, $"Error in {nameof(InnerRunWithLock)}");
    return new() { ErrorMsg = ex.Message };
}
finally
{
    _semLock.Release();
}
⚪
Sensitive info exposure

Description: Error message propagation may leak internal exception details via new() { ErrorMsg =
ex.Message }, exposing potentially sensitive environment or implementation information to
callers.
PyCodeInterpreter.cs [34-42]

Referred Code 
try
{
    return InnerRunWithLock(codeScript, options, cancellationToken);
}
catch (Exception ex)
{
    _logger.LogError(ex, $"Error when running code script with lock in {Provider}.");
    return new() { ErrorMsg = ex.Message };
}
DoS risk via blocking

Description: Inserting Thread.Sleep(100) before acquiring the GIL can create timing windows that
degrade responsiveness and encourage denial-of-service scenarios under high concurrency by
blocking threads unnecessarily.
PyCodeInterpreter.cs [164-166]

Referred Code 
Thread.Sleep(100);

// For observation purpose
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: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Error Leakage: The code returns exception messages to the caller via new() { ErrorMsg = ex.Message },
risking exposure of internal details to end users.

Referred Code 
_logger.LogError(ex, $"Error when running code script with lock in {Provider}.");
return new() { ErrorMsg = ex.Message };

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:
Limited Auditing: New error paths and lock acquisition attempts are logged, but successful critical actions
(e.g., code execution start/finish, outcome) are not comprehensively logged with user
identity and outcome details.

Referred Code 
        try
        {
            return InnerRunWithLock(codeScript, options, cancellationToken);
        }
        catch (Exception ex)
        {
            _logger.LogError(ex, $"Error when running code script with lock in {Provider}.");
            return new() { ErrorMsg = ex.Message };
        }
    }

    return InnerRunCode(codeScript, options, cancellationToken);
}

public async Task<CodeGenerationResult> GenerateCodeScriptAsync(string text, CodeGenerationOptions? options = null)
{
    Agent? agent = null;

    var agentId = options?.AgentId;
    var templateName = options?.TemplateName;



 ... (clipped 118 lines)

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 Errors: Exceptions are caught and logged but returned to callers as plain messages without
structured context or categorization, and cancellation handling around semaphore wait may
throw OperationCanceledException without tailored handling.

Referred Code 
        try
        {
            return InnerRunWithLock(codeScript, options, cancellationToken);
        }
        catch (Exception ex)
        {
            _logger.LogError(ex, $"Error when running code script with lock in {Provider}.");
            return new() { ErrorMsg = ex.Message };
        }
    }

    return InnerRunCode(codeScript, options, cancellationToken);
}

public async Task<CodeGenerationResult> GenerateCodeScriptAsync(string text, CodeGenerationOptions? options = null)
{
    Agent? agent = null;

    var agentId = options?.AgentId;
    var templateName = options?.TemplateName;



 ... (clipped 65 lines)

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:
Unstructured Logs: Logging uses free-form strings (warnings/errors) rather than structured entries, and
inclusion of exception details may inadvertently log sensitive info depending on upstream
data.

Referred Code 
        catch (Exception ex)
        {
            _logger.LogError(ex, $"Error when running code script with lock in {Provider}.");
            return new() { ErrorMsg = ex.Message };
        }
    }

    return InnerRunCode(codeScript, options, cancellationToken);
}

public async Task<CodeGenerationResult> GenerateCodeScriptAsync(string text, CodeGenerationOptions? options = null)
{
    Agent? agent = null;

    var agentId = options?.AgentId;
    var templateName = options?.TemplateName;

    if (!string.IsNullOrEmpty(agentId))
    {
        var agentService = _services.GetRequiredService<IAgentService>();
        agent = await agentService.GetAgent(agentId);


 ... (clipped 113 lines)

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:
Input Validation: Executing codeScript lacks visible validation or sandboxing in the new paths, which may
pose security risks if input originates externally.

Referred Code 
        try
        {
            return InnerRunWithLock(codeScript, options, cancellationToken);
        }
        catch (Exception ex)
        {
            _logger.LogError(ex, $"Error when running code script with lock in {Provider}.");
            return new() { ErrorMsg = ex.Message };
        }
    }

    return InnerRunCode(codeScript, options, cancellationToken);
}

public async Task<CodeGenerationResult> GenerateCodeScriptAsync(string text, CodeGenerationOptions? options = null)
{
    Agent? agent = null;

    var agentId = options?.AgentId;
    var templateName = options?.TemplateName;



 ... (clipped 118 lines)

Learn more about managing compliance generic rules or creating your own custom rules

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Nov 14, 2025 •
edited
Loading

PR Code Suggestions ✨

Latest suggestions up to cf9b7fa

CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Use cancelable, safe lock handling

Improve lock handling by replacing the blocking Wait with WaitAsync, handling
OperationCanceledException separately, and adding a try-catch block around
_semLock.Release() to prevent crashes.

src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [99-116]

 try
 {
     _semLock.Wait(cancellationToken);
     lockAcquired = true;
     return InnerRunCode(codeScript, options, cancellationToken);
+}
+catch (OperationCanceledException oce)
+{
+    _logger.LogWarning(oce, $"Cancelled in {nameof(InnerRunWithLock)} for {Provider}");
+    return new() { ErrorMsg = "Operation cancelled." };
 }
 catch (Exception ex)
 {
     _logger.LogError(ex, $"Error in {nameof(InnerRunWithLock)} in {Provider}");
     return new() { ErrorMsg = ex.Message };
 }
 finally
 {
     if (lockAcquired)
     {
-        _semLock.Release();
+        try { _semLock.Release(); }
+        catch (SemaphoreFullException sfe)
+        {
+            _logger.LogError(sfe, $"Semaphore release mismatch in {nameof(InnerRunWithLock)} for {Provider}");
+        }
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion proposes several valid improvements to error handling and robustness, such as specifically catching OperationCanceledException and safely releasing the semaphore, which enhances the reliability of the locking mechanism.

Medium
Avoid indefinite lock waits

To prevent potential deadlocks, consider using a timeout with _semLock.Wait
instead of waiting indefinitely. This would make the lock acquisition more
robust against unrecoverable errors.

src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [16]

+private static readonly SemaphoreSlim _semLock = new(initialCount: 1, maxCount: 1);
 
-
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential deadlock scenario due to the blocking _semLock.Wait without a timeout and recommends a more robust approach, which is a valuable improvement for reliability.

Medium
Possible issue
Use dedicated long-running task

Specify TaskCreationOptions.LongRunning when creating the task with
Task.Factory.StartNew to prevent potential thread pool starvation caused by
long-running Python code execution.

src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [159-165]

 var execTask = Task.Factory.StartNew(() =>
 {
-    Thread.Sleep(100);
-
     // For observation purpose
     var requestId = Guid.NewGuid();
     _logger.LogWarning($"Before acquiring Py.GIL for request {requestId}");
+}, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: This is a valuable performance and stability suggestion. Using TaskCreationOptions.LongRunning for a task that may block for a long time is a best practice to prevent thread pool starvation, which can degrade application responsiveness.

Medium
Ensure single semaphore initialization

To guarantee single initialization of the SemaphoreSlim across application
reloads, move its instantiation from a static readonly field initializer into a
static constructor.

src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [16]

-private static readonly SemaphoreSlim _semLock = new(initialCount: 1, maxCount: 1);
+private static readonly SemaphoreSlim _semLock;
 
+static PyCodeInterpreter()
+{
+    _semLock = new SemaphoreSlim(initialCount: 1, maxCount: 1);
+}
+
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion to use a static constructor is functionally equivalent to the existing static readonly field initializer in most scenarios, but it can offer slightly more robustness in complex application lifecycle events like hot-reloading.

Low
  • More

Previous suggestions

✅ Suggestions up to commit 99c3469
CategorySuggestion                                                                                                                                    Impact
Possible issue
✅ Prevent potential deadlock in semaphore usage
Suggestion Impact:The commit changed InnerRunWithLock to acquire the semaphore inside the try block, set a lockAcquired flag, and conditionally release the semaphore in finally, matching the suggested deadlock-prevention pattern.

code diff:

     #region Private methods
     private CodeInterpretResponse InnerRunWithLock(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default)
     {
-        _semLock.Wait(cancellationToken);
+        var lockAcquired = false;
 
         try
         {
+            _semLock.Wait(cancellationToken);
+            lockAcquired = true;
             return InnerRunCode(codeScript, options, cancellationToken);
         }
         catch (Exception ex)
         {
-            _logger.LogError(ex, $"Error in {nameof(InnerRunWithLock)}");
+            _logger.LogError(ex, $"Error in {nameof(InnerRunWithLock)} in {Provider}");
             return new() { ErrorMsg = ex.Message };
         }
         finally
         {
-            _semLock.Release();
+            if (lockAcquired)
+            {
+                _semLock.Release();
+            }
         }

To prevent potential deadlocks, revert the InnerRunWithLock method to use a
boolean flag to ensure the semaphore is always released correctly, even in the
case of asynchronous exceptions.

src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [103-120]

 private CodeInterpretResponse InnerRunWithLock(string codeScript, CodeInterpretOptions? options = null, CancellationToken cancellationToken = default)
 {
-    _semLock.Wait(cancellationToken);
-
+    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
     {
-        _semLock.Release();
+        if (lockAcquired)
+        {
+            _semLock.Release();
+        }
     }
 }

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a subtle but critical reliability issue where an asynchronous exception could cause a deadlock, and proposes reverting to a more robust, exception-safe pattern that was present before the PR.

High
General
Remove unnecessary thread sleep call

Remove the Thread.Sleep(100) call from the execTask as it was likely added for
debugging and negatively impacts performance.

src/Plugins/BotSharp.Plugin.PythonInterpreter/Services/PyCodeInterpreter.cs [162-167]

 var execTask = Task.Factory.StartNew(() =>
 {
-    Thread.Sleep(100);
-
     // For observation purpose
     var requestId = Guid.NewGuid();
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the added Thread.Sleep(100) is likely for debugging and unnecessarily degrades performance, and its removal would improve code quality.

Low

@iceljc iceljc merged commit dd6697a into SciSharp:master Nov 14, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@iceljc