Fix non-terminating and exponential traversal of exception graphs - #10
Closed
Heknon wants to merge 18 commits into
Closed
Fix non-terminating and exponential traversal of exception graphs#10Heknon wants to merge 18 commits into
Heknon wants to merge 18 commits into
Conversation
`__cause__`/`__context__` form an arbitrary directed graph, not a tree, but both the save and load paths walked them recursively as if they were one. That produced two distinct failures: - Cycles never terminated. `raise e from e` makes an exception its own cause, and longer cycles are equally reachable (mutual `__cause__` links, a manually assigned `__context__`, a group whose cause is itself). Every one of these died with a RecursionError. - Shared nodes were re-expanded exponentially. `raise X from Y` sets *both* `X.__cause__` and `X.__context__` to `Y`, so the ordinary chaining idiom produces a diamond. Walking it as a tree revisits the shared node down both edges, making a chain of n chained exceptions cost 2**n: measured at 2**(n+1)-1 nodes, with a 16-link chain producing 131071 nodes and a 122 MB dump. This terminated, which is why it surfaced as a load that ran for hours rather than as an error. Both paths now memoize on identity and walk iteratively: - `_serialize_exc_data` builds each node once and links them with a worklist, so the dump mirrors the original graph — a cycle stays a cycle and a node reachable twice stays one shared node. pickle reproduces that sharing. - `_reconstruct_exc_data` builds every exception first, then wires up `__cause__`/`__context__` from the identity map. Deferring the links is what lets a cycle be restored as a genuine cycle instead of an endless chain of copies. Group members are ordered before their group, since a group can only be built once its members exist. Being iterative also removes the recursion-depth ceiling on legitimately deep cause chains, which previously failed well before any cycle was involved. Tests cover both shapes; all nine fail on the previous implementation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJBKuuFRsXohzgomc7sLiy
Recording the exception graph faithfully means a dump can now contain cycles and shared nodes, which is correct but hostile to consumers that assumed a tree. A naive recursive walk of `cause`/`context` no longer terminates, and both `dataclasses.asdict()` and `json.dumps()` fail outright on a cycle — so anything projecting a dump over an API boundary needs a traversal that stops at nodes it has already seen. `walk_exception_data` yields every reachable node exactly once, covering causes, contexts and exception group members. It is iterative, so a long cause chain cannot exhaust the stack. The `id()` of the yielded nodes is the stable key callers need to emit references instead of nesting, which is what makes a cyclic graph representable in JSON at all. Documented in the README with a JSON projection example. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJBKuuFRsXohzgomc7sLiy
Formatting a loaded exception failed with `RuntimeError: generator raised StopIteration` for every dump, cycles or not — so an exception restored by `load_traceback` could not be printed, logged, or rendered by anything built on `traceback`. That defeats the point of reconstructing real frames. Reconstructed frames deliberately carry a synthetic (empty) code object, because accessing `f_locals` on a frame built by `PyFrame_New` over real optimized bytecode segfaults. The traceback objects were still built with the *original* `tb_lasti`, which indexes into the *original* bytecode. Pairing the two made `traceback._get_code_position` search a two-entry `co_positions()` for instruction `lasti // 2` — measured as index 33 against 2 available entries — and `next()` raised StopIteration inside a generator. A negative `tb_lasti` is the stdlib's own signal for "no instruction position available"; it then falls back to `tb_lineno`, which is restored accurately. Since the code object is always synthetic, the original offset never applies, so this is unconditional. `FrameData.lasti` stays in the dump as original metadata, and the on-disk format is unchanged. Loaded exceptions now print with correct files, lines, functions and source lines, and chaining, self-caused cycles and exception groups all render in the standard layout. The `^^^^` column markers remain unavailable for reconstructed frames, as column spans cannot be recovered without the original code object; this is now documented. All six tests fail on the previous implementation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJBKuuFRsXohzgomc7sLiy
New dumps are not universally readable by older releases, which is more than a patch-level change: where an exception graph genuinely contains a cycle (`raise e from e`), a pre-0.4.0 reader follows `__cause__` forever and dies with a RecursionError. An ordinary new dump still reads on an older release. The reverse direction is safe and was verified rather than assumed. The on-disk layout is unchanged — no field added, removed or renamed — so a dump written by an earlier version parses, loads and formats under this one; reconstructing the exact shape the previous `save_traceback` wrote (cause and context as separate duplicated nodes) round-trips at every depth tried. Such dumps also gain the traceback-formatting fix, having previously failed to format at all. What they do not gain is the deduplication: the duplication lives in the file, so an old five-link dump still expands to 63 nodes where a freshly saved one holds 6. Re-saving is what shrinks it. Documented in the README so the distinction is not discovered as a surprise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJBKuuFRsXohzgomc7sLiy
`_build_exception` raises ValueError when a group's members cannot be built before the group itself, but nothing exercised it, so coverage came in at 99.14% and the `--cov-fail-under=100` gate failed every test job. The branch is reachable rather than defensive: `save_traceback` cannot emit this shape, but a hand-crafted or corrupted dump can, and loading one hits it cleanly. Two tests now pin it -- a group listing itself as a member, and two groups listing each other. The second case is why the message changed. It also has no valid build order and reports the same error, so "contains itself" was inaccurate for it; the code comment already said "transitively", and the message now agrees. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161wStm2zaX9LCqsMeKJaWQ
`raise X from None` sets `__suppress_context__` to keep an internal failure out of a user-facing traceback. That flag was never serialized, so a loaded exception printed the context its author had explicitly suppressed. This was invisible until now: formatting a loaded exception raised `RuntimeError: generator raised StopIteration`, so nobody could see the wrong output. Fixing formatting is what surfaced it, which is why it belongs here. `__suppress_context__` is a C-level slot rather than `__dict__` state, so pickling the exception does not carry it and it has to be recorded explicitly. Restoring it has to happen after the `__cause__`/`__context__` links, because assigning `__cause__` sets the flag as a side effect -- and unconditionally, since `raise X from None` outside an `except` block suppresses a context that is itself None and so offers no assignment to piggyback on. The field is defaulted, so a dump written before it existed still loads: dataclass defaults live on the class, and the missing key resolves there. Verified in both directions -- an old dump reads as False under this code, and an older release ignores the added field in a new dump. The README's compatibility section is corrected to describe the field rather than claim the layout is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161wStm2zaX9LCqsMeKJaWQ
`ruff check` failed with 26 errors on a clean checkout -- every one CPY001, missing-copyright-notice -- so the pre-commit ruff hook and the CI lint job were both red without a single source change causing it. The rule left preview and so joined `select = ["ALL"]`, which is picking up new rules as ruff floats on `>=0.15.8`. It does not apply here: licensing lives in the root LICENSE file rather than a per-file header, so it is ignored alongside the four rules already listed. This clears the symptom, not the cause. `select = ["ALL"]` with an unpinned ruff and no committed lockfile will do the same thing the next time a rule graduates. Bounding the dev toolchain, or committing uv.lock (currently gitignored), is the durable fix and is left as a separate decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161wStm2zaX9LCqsMeKJaWQ
The CPY001 ignore in the previous commit fixed the symptom. The cause is that every CI step and pre-commit hook ran a bare `uv run`, which re-resolves the dev dependencies to whatever is newest: `ruff>=0.15.8` was silently supplying 0.16.4. Paired with `select = ["ALL"]`, which adopts every rule a new ruff ships, a routine upstream release turns into a red build on an unchanged commit -- which is exactly what happened. Two changes, because either alone is insufficient: - uv.lock is now tracked. It was excluded by the stock Python .gitignore template rather than by a decision here; the template's own comment calls tracking it "generally recommended". Verified that it genuinely pins: with the lock naming ruff 0.15.8 and pyproject allowing >=0.15.8, `uv sync` installs 0.15.8 rather than the newer 0.16.4. - CI passes `--locked`. Without it a bare `uv run` silently re-locks when pyproject drifts from the lockfile -- measured upgrading ruff 0.15.8 to 0.16.4 mid-run -- so tracking the file alone would not have held. With it, a stale lockfile fails loudly: "The lockfile at `uv.lock` needs to be updated, but `--locked` was provided." The pre-commit hooks deliberately keep the bare `uv run` so that changing a dependency locally does not block the commit that changes it; CI is the enforcement point. Toolchain upgrades are now an explicit `uv lock --upgrade-package ruff`, reviewable in the diff, instead of arriving unannounced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161wStm2zaX9LCqsMeKJaWQ
`test_get_f_back_offset_wrong_offset_restoration` passed in a full run and failed 10 times out of 10 on its own, with `ctypes.ArgumentError: argument 2`. It called `ctypes.pythonapi.PyFrame_New` directly. That function pointer is cached and process-global, and it only carries argtypes/restype once `_get_py_frame_new` has configured it -- so the test silently depended on some earlier test in the same process having created a frame first. Running it alone, under `-k`, or under `--lf` had nothing to configure it. It was the only one of the 65 tests with such a dependency. Building the frame through the package's own `create_frame` removes the dependency instead of papering over it with an explicit warm-up call. The probe also writes eight raw bytes at a fixed offset 80 into the frame, which happens to be in bounds today (the object is 136 bytes) but is not guaranteed across versions or platforms. It now asserts the object is large enough first, so a layout change fails the assertion rather than corrupting adjacent heap memory. Verified: all 65 tests pass individually in a fresh process, in reverse order, per-file, and as a full run, on 3.12 and 3.13. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161wStm2zaX9LCqsMeKJaWQ
Three test modules carried byte-identical copies of the save-to-buffer-and-load helper, and the exception-cycle tests kept a private copy of the node traversal that the same branch had just made public as walk_exception_data. A change to the round-trip path had to be made in three places, and a regression in the public traversal would have gone unnoticed by the test that counts nodes, because it never used it. Move the helper into tests/helpers.py and count nodes with the public traversal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0183gC2Cb37XREy7HSim1TQr
The loader ordered nodes by a depth-first post-order over cause and context edges as well as member edges, but only member edges constrain build order: a group is rebuilt around already-built members, whereas the links are assigned afterwards. Whenever a group was first reached from one of its own members the group was emitted before that member, and loading failed with the misleading "transitively contains itself" error. The common unwrap idiom, `except ExceptionGroup as g: raise g.exceptions[0]` (and its except* form), saves exactly that shape, so dumps save_traceback itself wrote could not be loaded. Walk member edges alone, and queue each cause/context target as a new starting point that is taken up only once the current search has finished. The "contains itself" error is now truly reachable only from a hand-crafted dump. While there, unpickle each node once on discovery and skip the members of a group whose pickle fell back to the placeholder: the placeholder references none of them, so rebuilding their frames was pure waste. A member something else links to is still built. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0183gC2Cb37XREy7HSim1TQr
ExceptionData kept the dataclass-generated structural __eq__ while the saved graph became cyclic, so comparing two parses of a `raise e from e` dump with ==, !=, or `in` recursed forever. Such graphs could not be saved at all before, so any consumer check like "re-saving is idempotent" now blew up instead of answering. Declare eq=False on both node classes: they compare and hash by identity, which matches the id()-based contract walk_exception_data already documents, and makes nodes usable directly as dict keys and set members. Say so in the README next to the asdict/json warning. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0183gC2Cb37XREy7HSim1TQr
Reconstructed frames carried tb_lasti = -1 so that the traceback module would fall back to tb_lineno instead of indexing the synthetic code object's positions. Python 3.12 prints unhandled exceptions from C, though, and that printer treats a negative offset as "columns 0 to 0", drawing a zero-width caret line, an empty line, under every reconstructed frame. The tests only went through the traceback module, which hid it. Give the synthetic code object a location table of its own instead: every instruction maps to the restored line with no column information, and the traceback entry points at a real offset. Both printers then show the right line and have no columns to draw markers from, and frame.f_lineno now reports the restored line as well. The encoder is checked against CPython's own decoder, including negative deltas and multi-byte varints. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0183gC2Cb37XREy7HSim1TQr
The wrong-offset test zeroed the slot at offset 80 of a live frame and left it zeroed. That slot is f_builtins on 3.12 and the strong f_funcobj reference on 3.13, so the test only passed because those fields happen to be NULL-safe at deallocation, and on 3.13 it leaked the function object. Its comment called the slot "a slot that is 0", and its size guard compared against sys.getsizeof, which includes the GC header that precedes the object. Find a slot that actually holds 0 and is not f_back, bounded by the type's __basicsize__, so the scan writes into a NULL pointer such as f_trace and restores it, leaving the frame exactly as it was. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0183gC2Cb37XREy7HSim1TQr
The pickle stored for a group carried its entire member tree, although the loader discards those embedded members and rebuilds the group around the nodes saved for each of them. That duplicated every member in the dump, once per level for nested groups, and it let a single member that fails only on load replace the whole group with the placeholder RuntimeError, dropping every member that would have loaded fine. Pickle the group around a single stand-in member instead, so the dump holds the group's class, message and state and nothing else. A BaseExceptionGroup cannot be empty, and its constructor decides the resulting class from the members it is given, so the stand-in is a BaseException whenever the group is one, keeping a BaseExceptionGroup of KeyboardInterrupts from being recorded as an ExceptionGroup. The loader is unchanged, and so is the on-disk format: a group written this way is read by every earlier release, and their dumps by this one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ArfHWPuzeVqEMipVKY1r6X
The traversal that saves and loads an exception graph is iterative, but the dump still nests every node inside the node that links to it, and pickling that nesting recurses at the C level. A cause chain of roughly 3,300 links raises RecursionError from save_traceback regardless of sys.setrecursionlimit, while the docstrings, the README and the test named for it suggested deep chains were unbounded. State the ceiling where a user would look for it, in save_traceback's docstring and the README, scope the traversal docstring to the walk, and exercise the deep-chain round trip at 1500 links: above the interpreter's default recursion limit, where the pre-0.4.0 recursive walk gave out, and well below pickle's. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ArfHWPuzeVqEMipVKY1r6X
Raising the deep-chain tests to 1500 links turned both Windows CI jobs red: save_traceback raised RecursionError from inside pickle on 3.12 and 3.13, while every Linux job passed. The ceiling the nested dump puts on pickle is set by CPython's C recursion limit, which is lower on Windows than on Linux, so a depth that is comfortably inside the Linux ceiling of roughly 3,300 links is already past the Windows one. Run the deep-chain round trip at 400 links again, under a recursion limit of 200 so that anything recursive on the path still fails long before the chain ends, and name that limit instead of comparing against the interpreter's default. State the platform-dependent ceiling in the save_traceback docstring and the README, with the Windows figure the CI run established. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ArfHWPuzeVqEMipVKY1r6X
Reconstructed frames run a synthetic code object, and its location table mapped every instruction to the restored line with no columns, so no printer had anything to draw the ^^^^ markers from. The columns were never lost, though: the dump keeps the original tb_lasti on every FrameData and marshals the original code object with its location table intact, so the failing instruction's position is one co_positions() lookup away. Look that position up on load and emit it as a long-form location entry (line delta, end line delta, and the two columns stored plus one) covering every synthetic instruction. Both the traceback module and the C printer behind the default excepthook now render a loaded exception, carets and tilde anchors included, byte for byte as they render the original; the new tests assert exactly that on both printers. Old dumps gain the carets too. Fall back to the line-only entry when the dump gives nothing usable: an offset the original bytecode cannot resolve, an instruction without column information (-X no_debug_ranges), or a position on another line than the one recorded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018UU24YJpF4m9snvpdP5NRj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
__cause__/__context__form an arbitrary directed graph, not a tree, but both the save and load paths walked them recursively as if they were one. That produced three distinct failures, all fixed here.1. Cycles never terminated
raise e from emakes an exception its own cause. Saving one died withRecursionError, as did every other cycle shape reachable in Python: mutual__cause__links, a manually assigned__context__, and a group whose cause is itself.2. Shared nodes were re-expanded exponentially
This is the one that hurts in practice.
raise X from Ysets bothX.__cause__andX.__context__toY, so the ordinary chaining idiom produces a diamond. Walking it as a tree revisits the shared node down both edges, so a chain ofnexceptions costs2**n. This one terminates, which is why it surfaced as a load that ran for hours rather than as an error.All figures below use minimal frames, so they measure structure rather than payload.
Before —
2**(n+1)-1nodes, everything doubling per link:After —
n+1nodes, flat at ~930 bytes per link:At the last depth both versions can do, 16 links, load goes from 7.6 s to 0.55 ms. Scaling is linear: 256× the links gives 205× the nodes, 199× the bytes and 185× the load time. The old code could not have reached 1024 links at all — that would have been
2**1025nodes.Nothing about the dump format changed to achieve this.
picklecould always represent a shared node; the old code simply never handed it one, having already expanded the graph into 131,071 distinct objects in memory beforepicklesaw it. A cycle now costs a two-byteBINGETback-reference.At the depth this actually bites
The table stops at 16 links because that is the deepest both versions can do. The failures that prompted this were deeper. Working back from the measured curve, a load that ran for 12 hours is a chain of roughly 27–29 links — 28.5 with minimal frames, nearer 27 with app-sized ones, where the per-node cost is around six times higher.
That same dump, after the fix:
43,200 s → 0.72 ms, about a 60-million-fold difference. It is that large because the change is
2**n → n, not a constant factor: at 29 links the old dump held roughly 537 million exception nodes (2**29-1) where the new one holds 29. Almost all of the work stopped existing rather than getting faster.Two honest caveats. The 12-hour figure is extrapolated, not measured — but from measured data: across the nine pre-fix points timed here (k=8→16) the load ratio per added link averaged 2.26 (range 1.88–2.86), and the extrapolation runs from the measured 7.578 s at k=16. And the depth is a range rather than a number because frame size moves it: heavier frames cost more per node, so fewer links reach 12 hours.
Cases that previously produced an error rather than a number
raise e from eRecursionErrora↔bcycleRecursionErrorRecursionErrorThe last row is worth noting separately: it contains no cycle at all. Deep-but-ordinary cause chains failed purely on recursion depth, and the iterative walk removes that ceiling too.
3. Loaded exceptions could not be formatted
Separately,
traceback.format_exceptionraisedRuntimeError: generator raised StopIterationfor every loaded dump, cycles or not — so a restored exception could not be printed, logged, or rendered by anything built ontraceback. That defeats the point of reconstructing real frames.Reconstructed frames deliberately carry a synthetic (empty) code object, because accessing
f_localson a frame built byPyFrame_Newover real optimized bytecode segfaults. The traceback objects were still built with the originaltb_lasti, which indexes into the original bytecode. Pairing the two madetraceback._get_code_positionsearch a two-entryco_positions()for instructionlasti // 2— measured as index 33 against 2 available entries. A negativetb_lastiis the stdlib's own signal for "no instruction position available"; it then falls back totb_lineno, which is restored accurately.Approach
Both paths now memoize on identity and walk iteratively:
_serialize_exc_databuilds each node once and links them with a worklist, so the dump mirrors the original graph — a cycle stays a cycle, a node reachable twice stays one shared node.picklereproduces that sharing via its memo, so a cycle costs a 2-byte back-reference rather than a copy._reconstruct_exc_databuilds every exception first, then wires__cause__/__context__from the identity map. Deferring the links is what restores a cycle as a genuine cycle instead of an endless chain of copies. Group members are ordered before their group, since a group can only be built once its members exist.Being iterative also removes the recursion-depth ceiling on legitimately deep cause chains, which previously failed well before any cycle was involved.
walk_exception_datais added as public API: consumers ofparse_tracebacknow need a traversal that stops at nodes it has already seen, since a naive recursive walk no longer terminates and bothdataclasses.asdict()andjson.dumps()fail outright on a cycle. Itsid()s are the stable key for emitting references instead of nesting, which is what makes a cyclic graph representable in JSON at all.Compatibility
Verified rather than assumed, by reconstructing the exact shape the previous
save_tracebackwrote (cause and context as separate duplicated nodes):__cause__forever. An ordinary new dump still reads fine on an older version. This is why the version goes to 0.4.0 rather than a patch bump.Documented in the README so the "loads fine but stays bloated" distinction is not discovered as a surprise.
Testing
15 new tests across
tests/test_exception_cycles.pyandtests/test_traceback_formatting.py; all of them fail on the currentmain. 58 pass in total.ruff format,ruff checkandty checkare clean, and every commit in the branch is independently green (48 → 52 → 58 → 58), so the history stays bisectable.One limitation worth stating: the
^^^^column markers are unavailable on reconstructed frames. Column spans live in the real code object'sco_positions(), and that is exactly the object that cannot be kept without segfaulting onf_locals. Line-level fidelity is recovered; column-level is not, short of storing position tables separately. This is noted in the README rather than left as a surprise.🤖 Generated with Claude Code
https://claude.ai/code/session_01CJBKuuFRsXohzgomc7sLiy