fix: make event queue sink removal idempotent

[dorlugasigal]

Description

Thank you for opening a Pull Request!

This pull request changed EventQueueSource.remove_sink() to use
set.discard(), so removing an already-absent sink completes without raising
KeyError. EventQueueSink.close() now delegates directly to that idempotent
operation instead of suppressing the exception after telemetry has recorded it.
A regression test closes the same sink twice and verifies that the mocked
OpenTelemetry span records neither an exception nor an error description.

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Follow the CONTRIBUTING Guide.
• Make your Pull Request title in the https://www.conventionalcommits.org/ specification.
  • Important Prefixes for release-please:
    • fix: which represents bug fixes, and correlates to a SemVer patch.
    • feat: represents a new feature, and correlates to a SemVer minor.
    • feat!:, or fix!:, refactor!:, etc., which represent a breaking change (indicated by the !) and will result in a SemVer major.
• Ensure the tests and linter pass (Run bash scripts/format.sh from the repository root to format)
• Appropriate docs were updated (if necessary)

Fixes #1133 🦕

[gemini-code-assist]

Code Review

This pull request refactors the remove_sink method in EventQueueSource to use discard instead of remove, making the operation idempotent and allowing the removal of a contextlib.suppress(KeyError) block in the close method. A new test is added to verify that closing a sink multiple times does not record exceptions on the telemetry span. The review feedback suggests asserting that the tracer mock's start_as_current_span method was actually called to prevent a vacuous test pass if the mock is not active during execution.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[github-actions]

🧪 Code Coverage (vs main)

⬇️ Download Full Report

	Base	PR	Delta
src/a2a/server/events/event_queue_v2.py	91.79%	91.24%	🔴 -0.56%
src/a2a/utils/telemetry.py	91.47%	90.70%	🔴 -0.78%
Total	93.00%	92.97%	🔴 -0.02%

Generated by coverage-comment.yml

[astrogilda]

Outside contributor, not a maintainer, and I should declare an interest up front: I have an open PR against this same file (#1137) and a merged one next door (#1105), so I read this one carefully for my own reasons before deciding it was worth saying something. Short version: I think this is correct, minimal, and fixing the right layer, and I could not find anything wrong with it. Here is what I actually ran, so the confirmation is worth something rather than being another opinion on the pile.

It still applies cleanly to current main. The PR shows as behind from 3e6fa6a, but git merge-tree --write-tree against cff6727 exits 0 with no conflict stanzas, and applying the diff to a cff6727 worktree succeeds. On that rebased tree the full suite is 1769 passed, 90 skipped, 3 xfailed, 1 xpassed, versus 1768 passed on cff6727 alone, so the new test is the entire delta and nothing else moved. ruff check is clean on both changed files. I ran with --ignore=tests/integration/cross_version since that directory needs network I do not have.

The new test is not vacuous, which was @gemini-code-assist's concern in the earlier round and is the thing I most wanted to check. I reverted event_queue_v2.py to main while keeping the test, and it fails with AssertionError: Expected 'record_exception' to not have been called. Called 1 times. So it genuinely pins the reported symptom, and the tracer.start_as_current_span.assert_called() line you added does its job of ruling out a mock that was never active.

On the change itself, discard is the right place to fix this rather than widening the suppression at the call site. EventQueueSource carries @trace_class(kind=SpanKind.SERVER), so remove_sink gets its own span and the KeyError was recorded there before contextlib.suppress ever saw it in the caller. Suppressing at the caller could never have cleared the span; only not raising can. I also checked the blast radius of loosening remove to discard: remove_sink has exactly one caller in the tree, EventQueueSink.close at line 369, so there is no other path that was relying on the KeyError as a signal. And contextlib is still used at three other places in the file, so dropping that one block does not leave an unused import behind.

One small thought, take it or leave it. Since remove_sink is public and its contract is now genuinely idempotent, the docstring change to "if present" is doing real work and it might be worth saying "no-op if the sink is not registered" so a caller cannot read silence as success. Not a blocker.

Finally, for whoever picks this up: this does not collide with my #1137. git merge-tree --write-tree between #1137's head 83fdd8d and this branch exits 0 with a clean tree, and the two diffs touch disjoint regions, since #1137 changes tap and the dispatch path and leaves remove_sink and close untouched. If anything the two are complementary, because #1137's eviction path force-closes a sink that a subscriber's own finally may close concurrently, which is exactly the double-remove this PR stops recording as an error. Merge order between them should not matter, and if it turns out to I will rebase mine rather than the other way round.