Skip to content

fix: catch BaseException in cleanup loops to survive asyncio.CancelledError #11

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

Open
ramparte wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: catch BaseException in cleanup loops to survive asyncio.CancelledError #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions amplifier_core/coordinator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ async def collect_contributions(self, channel: str) -> list[Any]:

if result is not None:
contributions.append(result)
except Exception as e:
except BaseException as e:
logger.warning(
f"Contributor '{contributor['name']}' on channel '{channel}' failed: {e}"
)
Comment on lines +338 to 341
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

Catching BaseException without re-raising critical exceptions like KeyboardInterrupt and SystemExit can prevent proper program termination. While this allows the contribution collection to survive asyncio.CancelledError, it also suppresses other critical exceptions.

Consider explicitly re-raising these critical exceptions:

except BaseException as e:
    if isinstance(e, (KeyboardInterrupt, SystemExit)):
        raise
    logger.warning(f"Contributor '{contributor['name']}' on channel '{channel}' failed: {e}")

Copilot uses AI. Check for mistakes.
Expand All @@ -353,7 +353,7 @@ async def cleanup(self):
result = cleanup_fn()
if inspect.iscoroutine(result):
await result
except Exception as e:
except BaseException as e:
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

Catching BaseException without re-raising critical exceptions like KeyboardInterrupt and SystemExit can prevent proper program termination. While this allows cleanup to survive asyncio.CancelledError, it also suppresses other critical exceptions that should propagate immediately.

Consider explicitly re-raising these critical exceptions:

except BaseException as e:
    if isinstance(e, (KeyboardInterrupt, SystemExit)):
        raise
    logger.error(f"Error during cleanup: {e}")

This ensures cleanup survives CancelledError while still allowing the user to forcefully terminate the program with Ctrl+C.

Suggested change
except BaseException as e:
except BaseException as e:
if isinstance(e, (KeyboardInterrupt, SystemExit)):
raise

Copilot uses AI. Check for mistakes.
logger.error(f"Error during cleanup: {e}")

def reset_turn(self):
Expand Down
11 changes: 7 additions & 4 deletions amplifier_core/session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
logger = logging.getLogger(__name__)


def _safe_exception_str(e: Exception) -> str:
def _safe_exception_str(e: BaseException) -> str:
"""
CRITICAL: Explicitly handle exception string conversion for Windows cp1252 compatibility.
Default encoding can fail on non-cp1252 characters, causing a crash during error handling.
Expand Down Expand Up @@ -432,7 +432,7 @@ async def execute(self, prompt: str) -> str:
self.status.status = "completed"
return result

except Exception as e:
except BaseException as e:
# Check if this was a cancellation-related exception
if self.coordinator.cancellation.is_cancelled:
self.status.status = "cancelled"
Expand All @@ -455,8 +455,11 @@ async def execute(self, prompt: str) -> str:

async def cleanup(self: "AmplifierSession") -> None:
"""Clean up session resources."""
await self.coordinator.cleanup()
# Clean up sys.path modifications
try:
await self.coordinator.cleanup()
except BaseException as e:
logger.error(f"Error during coordinator cleanup: {e}")
# Clean up sys.path modifications - must always run
Comment on lines +461 to +462
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

Catching BaseException and logging without re-raising means critical exceptions like KeyboardInterrupt and SystemExit will be suppressed during coordinator cleanup. This prevents proper program termination and means loader.cleanup() might run when it shouldn't.

Consider explicitly re-raising these critical exceptions:

try:
    await self.coordinator.cleanup()
except BaseException as e:
    if isinstance(e, (KeyboardInterrupt, SystemExit)):
        raise
    logger.error(f"Error during coordinator cleanup: {e}")
Suggested change
logger.error(f"Error during coordinator cleanup: {e}")
# Clean up sys.path modifications - must always run
# Re-raise critical exceptions to allow proper termination
if isinstance(e, (KeyboardInterrupt, SystemExit)):
raise
logger.error(f"Error during coordinator cleanup: {_safe_exception_str(e)}")
# Clean up sys.path modifications - must always run for non-critical errors

Copilot uses AI. Check for mistakes.
if self.loader:
self.loader.cleanup()

Expand Down
Loading