Skip to content

fix(engine): handle semaphore and init failures gracefully in async_utils#48

Closed
echobt wants to merge 3 commits intomainfrom
fix/async-utils-panic
Closed

fix(engine): handle semaphore and init failures gracefully in async_utils#48
echobt wants to merge 3 commits intomainfrom
fix/async-utils-panic

Conversation

@echobt
Copy link
Contributor

@echobt echobt commented Feb 4, 2026

Summary

Fixes #5205 and #5201 - Potential panics in async utilities.

Problem

Semaphore acquire and AsyncOnce init use unwrap() that can panic:

  • ConcurrencyLimiter::execute in async_utils.rs panics if semaphore is closed
  • AsyncOnce::get_or_init panics if value is missing after init flag is set
  • concurrent() function panics if semaphore is closed
  • ConcurrencyLimiter::acquire in ratelimit.rs panics if semaphore is closed

Solution

Added proper error handling with Result return types:

  • ConcurrencyLimiter::execute now returns Result<T> instead of T
  • AsyncOnce::get_or_init now returns Result<T> with descriptive error messages
  • concurrent() function now returns Result<Vec<T>>
  • ConcurrencyLimiter::acquire in ratelimit.rs now returns Result<ConcurrencyPermit>

All semaphore acquire calls now use map_err instead of unwrap() to provide descriptive error messages when semaphores are closed unexpectedly.

Testing

  • All existing tests updated to handle new Result types
  • All tests pass: cargo test -p cortex-engine async_utils and cargo test -p cortex-engine ratelimit

…tils

Fixes potential panics in async utilities:

- ConcurrencyLimiter::execute now returns Result<T> instead of T
- AsyncOnce::get_or_init now returns Result<T> with proper error handling
- concurrent() function now returns Result<Vec<T>>
- ConcurrencyLimiter::acquire in ratelimit.rs now returns Result

All semaphore acquire calls now use map_err instead of unwrap() to
provide descriptive error messages when semaphores are closed.

Fixes #5205, #5201
@greptile-apps
Copy link

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

This PR eliminates potential panic points in async utilities by replacing unwrap() calls with proper error handling.

Key Changes:

  • async_utils.rs: Converted ConcurrencyLimiter::execute(), AsyncOnce::get_or_init(), and concurrent() to return Result<T> instead of panicking on semaphore closure or inconsistent state
  • ratelimit.rs: Updated ConcurrencyLimiter::acquire() to return Result<ConcurrencyPermit> with proper error handling
  • mention.rs: Added UTF-8 boundary validation helpers (safe_slice_up_to, safe_slice_from) to prevent panics when slicing strings at invalid byte positions
  • TUI files: Fixed potential overflow issues in desired_height() calculations by using try_from() and saturating_add() instead of unsafe as casts

All error cases now return descriptive CortexError::Internal variants instead of panicking. Tests have been updated to handle the new Result types appropriately.

Confidence Score: 5/5

  • This PR is safe to merge - it improves robustness without breaking existing functionality
  • All changes replace panic-inducing code with proper error handling, tests are updated correctly, and the changes are well-documented with clear error messages
  • No files require special attention

Important Files Changed

Filename Overview
src/cortex-engine/src/async_utils.rs Replaced unwrap() calls with proper error handling using Result types for semaphore operations
src/cortex-engine/src/ratelimit.rs Added error handling for semaphore acquire operation in ConcurrencyLimiter
src/cortex-agents/src/mention.rs Added UTF-8 boundary validation for string slicing operations to prevent panics

Sequence Diagram

sequenceDiagram
    participant Caller
    participant ConcurrencyLimiter
    participant Semaphore
    participant ErrorHandler

    Note over Caller,ErrorHandler: Before: Panics on semaphore closure
    Caller->>ConcurrencyLimiter: execute(fn)
    ConcurrencyLimiter->>Semaphore: acquire().await
    alt Semaphore closed (old behavior)
        Semaphore-->>ConcurrencyLimiter: Err
        ConcurrencyLimiter->>ConcurrencyLimiter: unwrap() → PANIC!
    else Semaphore open (old behavior)
        Semaphore-->>ConcurrencyLimiter: Ok(permit)
        ConcurrencyLimiter->>Caller: fn().await result
    end

    Note over Caller,ErrorHandler: After: Graceful error handling
    Caller->>ConcurrencyLimiter: execute(fn)
    ConcurrencyLimiter->>Semaphore: acquire().await
    alt Semaphore closed (new behavior)
        Semaphore-->>ConcurrencyLimiter: Err
        ConcurrencyLimiter->>ErrorHandler: map_err → CortexError::Internal
        ErrorHandler-->>Caller: Result::Err
    else Semaphore open (new behavior)
        Semaphore-->>ConcurrencyLimiter: Ok(permit)
        ConcurrencyLimiter->>Caller: Result::Ok(fn().await)
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

echobt added a commit that referenced this pull request Feb 4, 2026
This PR consolidates the following error handling fixes:
- #48: Handle semaphore and init failures gracefully in async_utils
- #54: Improve error handling in session storage operations (includes TOCTOU race fixes)
- #55: Add validation for threshold, ratio, and token count fields
- #56: Replace unwrap with proper error handling for client access
- #57: Use unwrap_or_default for SystemTime operations
- #61: Handle invalid request-id header values gracefully
- #65: Improve error handling for timestamp and JSON operations in streaming

Key changes:
- Added graceful handling for semaphore and init failures
- Bound ToolResponseStore size and cleanup consumed entries
- Eliminated TOCTOU races in MCP server and plugin registry
- Replaced unwrap() with proper error handling throughout
- Added validation for config fields
- Improved error propagation in middleware
@echobt
Copy link
Contributor Author

echobt commented Feb 4, 2026

Consolidated into #73 - fix: consolidated error handling improvements

@echobt echobt closed this Feb 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant