fix(sqlalchemy): safely handle garbage-collected target during event listener removal

[aditya-cyberverse]

Fixes #4817

Description

During test teardown or uninstrument(), event listener cleanup in EngineTracer.remove_all_event_listeners can fail if the weakref target resolves to None or raises InvalidRequestError when SQLAlchemy tries to unregister the event.

Changes

• Safely resolves the weakref target and checks for None before removal.
• Catches InvalidRequestError during event.remove() to prevent flaky teardown crashes.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: aditya-cyberverse / name: aditya-cyberverse (9f763de)

[chuenchen309]

This has been sitting without review for ten days, and the test workflows here are still waiting on first-contributor approval — so nothing automated has actually exercised the change yet. I ran it locally, and I think there's a blocker worth catching before a maintainer looks at it.

event.remove(target, identifier, fn) raises NameError: the name event is never bound in this module. The existing import brings in the functions directly:

from sqlalchemy.event import (  # pylint: disable=no-name-in-module
    listen,
    remove,
)

Instrumenting, opening one connection, then calling uninstrument() on this branch (dbef557, SQLAlchemy 2.0.51):

listeners recorded: 7
--- uninstrument() ---
  File ".../opentelemetry/instrumentation/sqlalchemy/__init__.py", line 304, in _uninstrument
    EngineTracer.remove_all_event_listeners()
  File ".../opentelemetry/instrumentation/sqlalchemy/engine.py", line 317, in remove_all_event_listeners
    event.remove(target, identifier, fn)
    ^^^^^
NameError: name 'event' is not defined

Same script, same venv, with only engine.py swapped for the one on main:

listeners recorded: 7
--- uninstrument() ---
✅ no exception

So it fires on every uninstrument() where any weakref is still alive, which turns the intermittent teardown failure in #4817 into a deterministic one. Two ways out: call remove(...), which is already imported, or add from sqlalchemy import event — with the second, the imported remove becomes unused (grep -nE "(^|[^.\w])remove\(" has no hits on the branch), and #4926 is currently rolling ruff's default rules across the repo, so that would get flagged.

Two smaller things while you're in there:

• from sqlalchemy.exc import InvalidRequestError landed between import weakref and import sqlalchemy, i.e. in the stdlib block; the other third-party imports are in the group below. Same ruff/isort caveat as above.
• Swallowing InvalidRequestError is a reasonable call in teardown, but it also covers cases other than "this listener was never registered". A one-line comment saying why it's safe here would save the next reader from having to work out whether it was deliberate.

The other half of the change looks right to me — target = weak_ref_target() with an early continue is equivalent to the old if weak_ref_target() is not None, minus a second weakref call, so the only behavioural change is the try/except.

AI-assisted; the traceback and the control run above are output from actually running it, not inferred.