Skip to content

fix(client): name the cause inside an exception group in every log (see #224) - #225

Merged
ViperJuice merged 3 commits into
mainfrom
fix/224-unwrap-exception-groups
Sep 3, 2026
Merged

ViperJuice merged 3 commits into
mainfrom
fix/224-unwrap-exception-groups

Conversation

@ViperJuice

@ViperJuice ViperJuice commented Sep 3, 2026 •

Copy link
Copy Markdown
Collaborator

See #224. First half of stage 2: make the failures legible. No behavioural change to the transport itself.

The defect. Every remote-transport path in ClientManager runs inside an anyio task group, and str(ExceptionGroup) is "unhandled errors in a TaskGroup (1 sub-exception)" — it names neither the type nor the message of what actually failed. That string was the entire content of 20 log sites, and it is what a week of CI hangs (#200) produced instead of a cause:

[fr-emit-remove] remote transport owner exited unexpectedly: unhandled errors in a TaskGroup (1 sub-exception)
[fr-emit-remove] Catalog reconciliation failed: Server fr-emit-remove disconnected
Connection to fr-emit-remove failed (attempt 1/3), retrying in 1.0s: unhandled errors in a TaskGroup (1 sub-exception)

The fix. describe_exception() flattens a group to its leaves and renders ConnectionResetError: peer went away.

  • Duck-types .exceptions rather than isinstance: 3.11+ raises the builtin BaseExceptionGroup, 3.10 raises exceptiongroup.ExceptionGroup from the backport (verified in this repo's 3.10 venv), and neither name imports on the other interpreter. Narrowed by requiring every element to be a BaseException, and depth-capped so a self-referential group cannot hang the thing doing the diagnosing.
  • Goes through sanitize_auth_diagnostic. Not optional: flattening increases how much exception text reaches the log, and these come from an HTTP transport whose messages can carry a URL or an Authorization header. Most of these sites logged the raw exception before, so they are newly redacted as well as newly useful.
  • Applied at all 20 sites, and to the error string disconnect_server returns to its caller — that reached the public API as the useless string too.

Scope note. #224 named 5 sites, from the CI log. The AST guard below found 14 more, including SSE read error in _read_sse and Catalog reconciliation failed — both directly on the path #224 is chasing. Fixing the class rather than the instances is deliberate.

Tests (tests/test_exception_group_diagnostics.py, 13):

  • The defect itself is pinned — if str(group) ever becomes useful, the helper should go.
  • Groups are built through a real anyio task group, so they exercise whichever type this interpreter produces rather than a hand-built one.
  • Nested groups flatten; a class with a non-exception .exceptions is not treated as a group; a self-referential group terminates; many leaves are capped and the remainder counted.
  • Redaction holds on both the group path and the single-exception early return.
  • Wiring, not just the helper: a failing remote disconnect must log ConnectionResetError: peer went away and return it. Every other test would pass with a call site reverted — which is how this defect survived.
  • AST structural guard: fails if any logger.* call interpolates a caught exception directly.

Verification (each run in the step it is reported): new file 13 passed; full suite 3195 passed vs main's 3182 (+13, exactly the new tests) with failures/errors/skips/deselects identical — the 107/106 are the documented /tmp/package.json host quirk; ruff check, ruff format --check, mypy src/ all exit 0.

Also, unrelated to this diff but recorded on #224: the #200 flake now reproduces locally, 1 run in 25 of tests/runtime/test_emitter_harness.py alone — same test, same message as CI. Stage 2 no longer needs to wait on CI.

🤖 Generated with Claude Code

https://claude.ai/code/session_018NR2LKqZgn7CXVH2816gaa


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

#224)

str(ExceptionGroup) is 'unhandled errors in a TaskGroup (1 sub-exception)' --
it names neither the type nor the message of what failed. Every remote
transport path in ClientManager runs inside an anyio task group, so that was
the entire content of 20 log sites, and it is what a week of CI hangs (#200)
produced instead of a cause.

describe_exception() flattens a group to its leaves and renders
'ConnectionResetError: peer went away'. Duck-types .exceptions rather than
isinstance: 3.11+ raises builtin BaseExceptionGroup, 3.10 raises
exceptiongroup.ExceptionGroup from the backport, and neither name imports on
the other. Output goes through sanitize_auth_diagnostic -- flattening widens
how much exception text reaches the log, and these come from an HTTP
transport, so most sites are newly redacted as well as newly useful.

Applied at all 20 sites, not just the 6 seen in the CI log, and to the error
string disconnect_server returns to its caller. An AST guard fails if a future
edit logs a caught exception directly -- that guard is what found the other 14.
…st in logs (see #224)

Board review found the first cut was a narrower fix than it claimed, and its
AST guard was a false green: it only looked inside logger.* calls, so it passed
while seven sites still rendered a group as 'unhandled errors in a TaskGroup':

  - read_failure_reason = f'stdout read error: {e}' -- logged twice AND stored
    as last_error, via an intermediate the guard could not see
  - last_error = str(e) at four sites -- these reach pmcp status, pmcp doctor
    and health output, further than any log line
  - the error list connect_server returns to its caller, and thence to
    provision output

The guard is now the broad rule: inside 'except ... as e', neither f'{e}' nor
str(e) may appear anywhere; describe_exception(e) is a Call and passes. A test
feeds it the indirect shape that defeated version one and requires a finding.
New wiring test asserts last_error names the leaf.
…aw exception (see #224)

Board finding: exc_info= passes the raw exception to the logging machinery,
which appends the unredacted exception tree AFTER the sanitised message -- so
a bearer token in a transport error reached the log in full while the line
above it looked clean. The tests could not see it because they inspect
record.getMessage(), which excludes exc_info output.

Formatting the traceback in-process and passing it through
sanitize_auth_diagnostic(max_length=None) keeps every frame and closes the
hole. Two guards: no logger call may pass exc_info, and a formatted traceback
carrying a secret must come back redacted with its frames intact.
@ViperJuice
ViperJuice merged commit fed8716 into main Sep 3, 2026
21 of 23 checks passed
@ViperJuice
ViperJuice deleted the fix/224-unwrap-exception-groups branch September 3, 2026 08:08
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