Skip to content

Fix potential panic bugs in string slicing and server shutdown#410

Closed
hobostay wants to merge 1 commit intoRightNow-AI:mainfrom
hobostay:fix/string-slicing-panic-bug
Closed

Fix potential panic bugs in string slicing and server shutdown#410
hobostay wants to merge 1 commit intoRightNow-AI:mainfrom
hobostay:fix/string-slicing-panic-bug

Conversation

@hobostay
Copy link
Copy Markdown

@hobostay hobostay commented Mar 7, 2026

Summary

This PR fixes several potential panic bugs and race conditions found in the codebase:

1. Race Condition in Desktop Server Shutdown (crates/openfang-desktop/src/server.rs)

Problem: Both shutdown() and Drop implementation tried to send shutdown signals and join threads, which could lead to race conditions when both try to manipulate the same resources simultaneously.

Fix:

  • Added AtomicBool flag (shutdown_initiated) to track shutdown state
  • Both shutdown() and Drop now use compare_exchange to ensure shutdown only happens once
  • This prevents duplicate shutdown attempts and potential race conditions

2. String Slicing Panic (crates/openfang-api/src/channel_bridge.rs)

Problem: Multiple locations used &str[..8] to truncate UUID strings for display without checking if the string was at least 8 characters long. While UUIDs are always 36 characters, this pattern is unsafe and could cause panics if the data changes.

Fix:

  • Added truncate_str() helper function that safely returns the full string if it's shorter than the requested length
  • Fixed 9 occurrences at lines: 354, 393, 408, 431, 491, 513, 545, 565, 609

3. String Slicing Panic (crates/openfang-runtime/src/tool_runner.rs)

Problem: Canvas filename generation used &canvas_id[..8] without checking length.

Fix:

  • Added length check before slicing
  • Falls back to full string if shorter than 8 characters

Test Plan

  • Build passes: cargo build --workspace
  • Tests pass: cargo test --workspace
  • No new clippy warnings introduced
  • Manual code review of all changes

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

🤖 Generated with Claude Code

This commit fixes several potential panic bugs and race conditions:

1. **Race condition in desktop server shutdown** (crates/openfang-desktop/src/server.rs):
   - Added `AtomicBool` flag to track shutdown state
   - Both `shutdown()` and `Drop` now use `compare_exchange` to ensure
     shutdown only happens once, preventing race conditions where both
     try to manipulate the same resources simultaneously

2. **String slicing panic** (crates/openfang-api/src/channel_bridge.rs):
   - Added `truncate_str()` helper function to safely truncate strings
   - Fixed 9 occurrences where `&str[..8]` could panic if string < 8 chars
   - Lines affected: 354, 393, 408, 431, 491, 513, 545, 565, 609

3. **String slicing panic** (crates/openfang-runtime/src/tool_runner.rs):
   - Fixed canvas filename generation where `&canvas_id[..8]` could panic
   - Now safely handles strings shorter than 8 characters

These fixes prevent potential panics in production code when handling
UUIDs or other identifiers that might be shorter than expected.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jaberjaber23 jaberjaber23 added the under-review PR is under review label Mar 10, 2026
@jaberjaber23
Copy link
Copy Markdown
Member

thanks for this! we're reviewing and may implement this differently to fit our architecture. will keep you posted

@jaberjaber23
Copy link
Copy Markdown
Member

implemented in v0.3.46 — fixed 9 string slicing instances in channel_bridge using our safe_truncate_str + desktop shutdown race. some overlapped with our v0.3.43 fix. thanks @hobostay

@hobostay hobostay closed this by deleting the head repository Mar 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

under-review PR is under review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants