Yes—several things are worth modernising

The project is generally well-structured and actively maintained. I would avoid a wholesale rewrite, but there are a few concrete correctness, security, and maintenance improvements.

Highest priority

1. Harden the C extension

   • stat_profile.c indexes the first tuple item without checking whether the tuple is empty. That is undefined behavior in C and could crash Python.
   • ProfilerState_GetTime leaks a reference when a custom timer returns a non-float.
   • The callback path around code_from_frame dereferences API results without checking for failure.
   • These are small, worthwhile fixes with regression tests.

2. Update the frontend dependency stack

   • package.json is on Svelte 4, Vite 5, and Vitest 2.
   • A live npm audit reports 15 vulnerabilities: 1 critical, 8 high, 6 moderate. Most concern development tooling rather than the distributed Python package, but the Vitest and Vite development servers have meaningful exposure.
   • Update safe transitive dependencies first, then handle the Svelte/Vite/Vitest major upgrades as a separate tested change. Do not use npm audit fix --force blindly.

3. Correct and update the Python CI matrix

   • Tests currently stop at Python 3.13 in test.yml, while prerelease wheels target 3.15.
   • Add stable Python 3.14 to CI and Nox.
   • Decide whether Python 3.8 and 3.9 should remain supported. Both are now end-of-life; moving to Python 3.10+ or 3.11+ in the next major release would materially reduce compatibility code and dependency constraints.

4. Fix the wheel workflow’s dead matrix condition

   • wheels.yml references matrix.archs, but the matrix only defines os.
   • Consequently the QEMU condition never runs and the job name contains a blank architecture.
   • Since ubuntu-24.04-arm is already a native ARM runner, the likely fix is simply to remove the obsolete QEMU steps and matrix.archs references.

Next worthwhile batch

• Add frontend CI. Run npm ci, npm run check, vitest run, and npm run build. The current npm test invokes watch mode, which is unsuitable for CI.
• Expand Dependabot. dependabot.yml ignores all official Actions and does not monitor html_renderer. Add npm monitoring and remove the broad actions/* exclusion.
• Harden GitHub Actions. Pin third-party/publishing actions to commit SHAs and set workflow-level permissions: contents: read, elevating only the PyPI job.
• Gate releases more strongly. The tag publishing workflow builds artifacts but does not require tests for the tagged commit. Add test gating, twine check, and an installation smoke test before publishing.
• Make stack-sampler subscription transactional. subscribe() mutates the context variable and subscriber list before _update() can fail, potentially leaving stale profiler state.
• Clean up the IPython event loop. run_cell_async() starts a thread but never joins it and creates a loop without closing it.
• Fix the empty-stack edge case. Session._trim_stem() calls popleft() without checking whether start_call_stack is empty.

Packaging/tooling modernization

These are useful but less urgent:

• Move static metadata from setup.py into PEP 621 [project] metadata in pyproject.toml. Keep a minimal setup.py for the C extension if that remains simplest.
• Add Python-version classifiers.
• Remove or rename the misleading bin extra: Click is used by the maintainer release script, not the installed pyinstrument command.
• Install only .[docs] on Read the Docs instead of the complete development dependency set.
• Align the manual Pyupgrade hook from --py37-plus to the declared minimum.
• Consider adding a conservative Ruff ruleset alongside Black, isort, and Pyright.
• Use npm ci rather than npm install in the frontend Nox session.
• Declare a supported Node LTS version in package.json and CI.

What I would not modernise just for fashion

• Don’t replace setuptools solely because it is old.
• Don’t force deletion of setup.py while it remains a simple way to define the extension.
• Don’t migrate Svelte and the Python packaging configuration simultaneously.
• Don’t add a new dependency manager or lock every Python dependency without a specific reproducibility need.
• Don’t remove Python 3.8/3.9 in a patch release; make that an explicit major-version support decision.

No files were changed. The working tree remains clean. My suggested first implementation batch would be the three C safety fixes, the wheel matrix correction, Python 3.14 CI, and safe frontend lockfile updates.

First C safety pass complete

Updated stat_profile.c to:

• Release invalid custom-timer results rather than leaking references.
• Abort profiler initialization cleanly when the timer fails.
• Check for empty co_varnames before accessing its first item.
• Guard frame/code API results and propagate allocation failures safely.
• Avoid leaving a profile callback installed after initialization errors.

Added regression coverage for:

• Frames with no local variables.
• Invalid timer result lifetime and profiler cleanup.

Verification

• Python 3.10 focused native tests: 15 passed
• Python 3.12 low-level suite: 30 passed
• Python 3.12 main suite: 130 passed, 5 skipped
• IPython suite: 5 passed
• C extension compiled without warnings from these changes.
• git diff --check: clean

Next up should be making StackSampler.subscribe() transactional so a failed subscription cannot leave stale context or subscriber state.

You were right—the NULL check around code_from_frame() was unnecessary and implied a failure mode that does not exist.

PyFrame_GetCode():

• Guarantees a non-NULL code object for a valid frame.
• Does not allocate or set exceptions.
• Works for comprehensions, generators, coroutines, and async generators.
• Returns a new reference that must be decremented.

I removed the checks from both _get_frame_info() and the profile callback. The frame_identifier == NULL check remains because constructing that string involves allocations and genuinely fallible APIs.

Rebuilt successfully on Python 3.12; all 30 low-level tests pass.

Confirmed with the C patch stashed:

• Custom timer test failed against old code:

  • Expected RuntimeError
  • Received SystemError because setstatprofile() returned with an exception pending
  • Once that is fixed, the same test also checks the leaked reference.

• Empty-locals frame test passed against old code. The out-of-bounds tuple read happens to return harmless data in this build, so this is not a deterministic red/green test. It remains useful edge-case coverage, but the C bounds fix is justified by the API contract rather than a reproducible failure.

I attempted AddressSanitizer, but macOS rejected loading Xcode’s ASan runtime due to platform signing policy.

The C patch has been restored, a normal extension rebuilt, and both tests now pass. Pre-existing stashes were left untouched.

Created and committed on branch:

• Branch: modernize-c-extension-safety
• Commit: c38e33d Harden native profiler error handling

All commit hooks passed, including Pyright, Black, isort, and codespell. The working tree is clean.

Renamed the branch to modernise. Working tree remains clean.

Implemented and committed the next Python safety fix:

• Commit: 8cf5369 Roll back failed sampler subscriptions
• Failed subscriptions now restore:
  • Subscriber list
  • Async context variable
  • Previous sampling configuration
  • Active profile callback
• Duplicate/error behavior remains unchanged.

Verification:

• 26 relevant tests passed
• Black, isort, Pyright, and all commit hooks passed
• Working tree is clean

Next smallest Python fix is guarding Session._trim_stem() against an empty start_call_stack; after that, the IPython event-loop cleanup.

I had not checked it before committing—that was an omission.

I have now verified the red/green behavior:

• Old implementation: regression test fails because the rejected second subscriber remains in sampler.subscribers.
• New implementation: regression test passes.
• The committed source was restored afterward.
• Working tree is clean.

Understood. From now on I’ll follow strict red–green engineering:

1. Write the smallest behavioral regression test first.
2. Run it against the unchanged implementation.
3. Show the exact expected failure.
4. Only then design and apply the fix.
5. Run the targeted test green, followed by relevant broader tests.
6. Commit only after hooks and verification pass.

If a defect cannot be demonstrated deterministically—such as undefined C behavior—I’ll say so upfront and propose sanitizer/static-analysis coverage rather than presenting a passing edge-case test as a regression test.

Completed the next fix using strict red–green:

Red

The new regression test failed against unchanged code with:

IndexError: pop from an empty deque
pyinstrument/session.py:171

Green

Added the minimal empty-stack guard. Verification:

• Regression test: 1 passed
• Full profiler module: 14 passed
• Black, isort, Pyright, and all hooks passed

Committed as:

4b4c047 Handle sessions without a start call stack

Working tree is clean. Next is the IPython async event-loop lifecycle and its observed Python 3.12 deprecation warning.

In normal profiling, it should never be None or empty.

Profiler.start() populates it using build_call_stack(), which always appends a synthetic thread frame—even when the caller frame is unavailable. Its type is list[str], so None is invalid and the new guard does not support it.

An empty list is only realistically possible when:

• Constructing Session manually.
• Loading externally generated or edited session JSON containing "start_call_stack": [].
• Using the test helper dummy_session().

So this is defensive handling for a representable external state, not a bug produced by the normal profiler lifecycle. Given the project’s preference for small, meaningful changes, I’d lean toward dropping commit 4b4c047 unless manually constructed/third-party sessions are intended to support an empty start stack.

Completed with strict red–green.

Red

The unchanged implementation failed both new tests:

• Temporary event loop remained running and open.
• Calling without a configured current loop raised RuntimeError.

Green

The implementation now:

• Does not modify the caller thread’s configured event loop.
• Retains and joins the worker thread.
• Stops and closes the temporary loop in finally.

Verification:

• Regression tests: 2 passed
• Complete IPython suite: 7 passed
• All commit hooks passed
• Working tree clean

Committed as:

42cd892 Clean up IPython async event loops

One separate IPython warning remains about future run_cell_async() transformation behavior; the event-loop warning is gone.

👍

Completed the next modernization with red–green verification.

Red

The existing async-cell test failed when the IPython deprecation was treated as an error:

DeprecationWarning: `run_cell_async` will not call `transform_cell`
automatically in the future

Green

The magic now mirrors IPython’s supported preprocessing contract:

• Transforms the raw cell explicitly.
• Captures preprocessing exceptions.
• Passes transformed_cell and preprocessing_exc_tuple.
• Retains compatibility with IPython versions older than 7.17.

Verification:

• Complete IPython suite: 7 passed, no warnings
• All commit hooks passed
• Working tree clean

Committed as:

2c01a4f Preprocess asynchronous IPython cells

Great—real IPython validation confirms the behavior beyond the automated suite.

CI modernization committed:

8a8b4ca Add Python 3.14 CI coverage

Changes:

• Added Python 3.14 to the cross-platform test matrix and Nox.
• Moved the ARM test to Python 3.14.
• Removed dead QEMU steps and undefined matrix.archs references.
• Simplified wheel job naming.

Validation:

• Baseline actionlint: 3 errors
• Updated workflow: clean
• All commit hooks passed
• Working tree clean

Python 3.14 execution will be validated by GitHub Actions since there is no local 3.14 environment.

Draft PR created: Modernise profiler internals and CI

• Branch pushed: modernise
• Base: main
• Draft status confirmed
• Description includes a brief AI-assistance disclosure
• CI checks have not populated yet.

Kept this pass strictly Python/C.

Red

An unset ContextVar produced:

• Native implementation: SIGSEGV, subprocess return code -11
• Python reference implementation: LookupError

Green

Both implementations now treat an unset context as None.

Verification:

• Exact regression tests: 2 passed
• Python 3.12 low-level suite: 32 passed
• Python 3.10 context suite: 6 passed
• All commit hooks passed

Committed as:

059ac11 Handle unset profiler context variables

The branch is one commit ahead of the draft PR; this commit has not been pushed yet.

It occurs when the low-level profiler is given a valid ContextVar that:

1. Was created without a default, and
2. Has no value in the current context.

For example:

import contextvars
from pyinstrument.low_level.stat_profile import setstatprofile

context_var = contextvars.ContextVar("profiler")  # no default and currently unset

setstatprofile(
    lambda frame, event, arg: None,
    context_var=context_var,
)

Previously, the C path did this:

PyObject *new = NULL;
int status = PyContextVar_Get(context_var, NULL, &new);

For an unset variable without a default, PyContextVar_Get() returns 0—not -1—and leaves new as NULL. The code treated that as success. During initial setup, both old and new were NULL, so it entered this branch:

if (old == new) {
    Py_DECREF(new);  // Py_DECREF(NULL) → segmentation fault
}

It could also happen later if the variable was initially set but execution switched into a copied async context where it was unset.

The normal high-level Profiler was protected because its internal context variable is created with default=None. This affected direct low-level API users or alternative integrations supplying their own no-default ContextVar.

The fix passes Py_None as the fallback, ensuring the result is always a valid owned Python object:

PyContextVar_Get(context_var, Py_None, &new);

The native test would run setstatprofile() inside the pytest process and trigger the same Py_DECREF(NULL) segmentation fault.

Consequences:

• The entire pytest process terminates immediately.
• Remaining tests do not run.
• Fixtures and finally cleanup do not execute.
• Pytest usually reports something like Fatal Python error: Segmentation fault, rather than a normal assertion failure.
• CI sees the test worker exit from a signal, often with exit code 139 on Linux or -11 when observed by a parent process.
• Depending on OS configuration, it may produce a core dump.

The subprocess wrapper contains that native crash:

┌───────────────┐        ┌──────────────────┐
│ pytest parent │───────▶│ child Python     │
│ remains alive │        │ crashes with     │
│               │◀───────│ SIGSEGV / -11    │
└───────────────┘        └──────────────────┘

The parent can then turn the crash into an ordinary, useful assertion:

assert result.returncode == 0

The Python-reference test does not need isolation because it raises a normal LookupError; pytest can catch and report that safely.

Without the subprocess wrapper, pytest terminated exactly as expected:

Fatal Python error: Segmentation fault
...
test_unset_context_var_directly
...
Segmentation fault: 11
PYTEST_PROCESS_EXIT=139

Pytest produced no normal pass/fail result, and no cleanup could run.

The temporary test has been deleted, the fixed C extension rebuilt, and the isolated regression test passes again. The working tree is clean.

Done. The regression test now runs both implementations directly via the existing parameterization:

• Native C implementation
• Python reference implementation

No subprocess wrapper remains. The context suite passes all 6 tests, and all commit hooks passed.

Committed as:

dde92f2 Run context regression tests in process

The branch is now two commits ahead of the draft PR.

The committed parity test is now red: 1 failed, 9 passed, with the native result reporting InnerClass instead of OuterClass.InnerClass.

The correct replacement should use the public __qualname__ attribute and return a managed Python string, rather than swapping _PyType_Name for another short-name API. I’m checking supported-version API availability and existing helper ownership before editing the C return contract.

Yes—there is a measurable regression in the frame-info microbenchmark.

On Python 3.12 / Apple Silicon, comparing the exact commits:

The slowdown comes from replacing the private _PyType_Name() with a public PyObject_GetAttrString(type, "__qualname__") lookup. It only meaningfully affects method/classmethod frames.

In practical profiling terms, at the default 1 ms sampling interval:

• 1,000 samples/sec
• 20 method frames per sample
• $20 \times 60\text{ ns} \times 1000 \approx 1.2\text{ ms/sec}$

So roughly 0.12% of one CPU core for that representative stack. Small, but it is a real hot-path regression.

Python 3.11+ provides the public PyType_GetQualName() API, which should avoid the generic attribute lookup. Since Python 3.10 lacks it, we can use:

• PyType_GetQualName() on Python 3.11+
• the current attribute lookup fallback on Python 3.10

I think we should optimize that before moving on. The benchmark required no workspace changes; the tree remains clean.

Implemented and committed as 868b414.

Performance on Python 3.12

So it removes the 14% regression and is approximately 3–5% faster than the original private API. Plain frames remain unchanged.

Compatibility and verification

• Python 3.11+: PyType_GetQualName()
• Python 3.10: existing attribute-lookup fallback
• Python 3.10 low-level suite: 34 passed
• Python 3.11 frame-info tests: 14 passed
• Python 3.12 low-level tests: passed
• Commit hooks: passed

One obscure semantic difference: a custom metaclass overriding __getattribute__ specifically for __qualname__ can make the Python fallback report the synthetic name while PyType_GetQualName() reports the class’s actual qualified name. I think using the canonical C API is preferable for profiler identity.

The branch is now four commits ahead of the draft PR.

Already committed:

868b414 Use public type qualname API on Python 3.11+

Working tree is clean. Branch is four commits ahead of the remote.

Next I’d bump the minimum supported Python version from 3.8 to 3.10:

• Update package metadata and README
• Remove 3.8/3.9 from CI and Nox
• Set Pyright to Python 3.10
• Set Pyupgrade to --py310-plus

I’d keep obsolete C compatibility-shim removal as a separate follow-up commit so the support-policy change stays clear.

Shall I do the Python 3.10 minimum-version bump first?

Correct—we don’t need to bump the minimum version for the qualname fix.

The implementation is already compatible:

• Python 3.11+: PyType_GetQualName()
• Python 3.8–3.10: PyObject_GetAttrString(..., "__qualname__")

The minimum-version bump would only be a support-policy decision, not a technical requirement. We can leave Python 3.8/3.9 support intact.

There are no remaining private CPython API usages that urgently need replacing.

The next meaningful C candidate is the timing-thread lifecycle in pyi_timing_thread.c. When the last subscriber unsubscribes and another subscribes immediately:

1. The old timing thread is told to exit.
2. The code marks it dead without waiting for it to finish.
3. A new subscription clears the exit flag and starts another thread.
4. The old thread may observe the cleared flag and continue, leaving two timing threads running.

Several shared variables are also accessed without consistent locking. That is more substantive than cosmetic modernization, but we’d need a deterministic failing regression test before touching it.

A smaller mechanical option is converting the positional PyTypeObject initializer to designated fields, avoiding stale slots such as the comment naming tp_print. It offers little immediate user benefit.

My recommendation is to investigate and test the timing-thread race first, without applying a fix unless we can demonstrate the failure. Shall I do that?

Noted—I won’t treat private CPython API usage as something that needs changing by itself.

The qualname change still earns its place because it fixes nested class names and is now faster, not merely because _PyType_Name was private.

The timing-thread race remains an independent correctness concern. Shall I investigate whether we can reproduce it with a failing test?

I can reproduce the race, but not with a suitable regression test.

Rapidly subscribing/unsubscribing produced up to 12 native threads temporarily instead of one, then returned to one after settling. However:

• The failure is scheduler-dependent.
• The standard library cannot count native C threads portably.
• Timing-based behavioral tests were intermittent.
• Fixing it cleanly would add synchronization to the timing thread’s hot loop, so it is not strictly trade-off-free.

Adding test-only C instrumentation or a new psutil dependency would cost more than this issue warrants. Given your criteria—and the red/green requirement—I recommend leaving it alone. No project files were changed.

Yes, I’d recommend it. I audited the current tree with Ruff 0.16.0 using the existing 100-character line length and Python 3.8 target.

Migration impact

• Ruff formatter: only 3 files would change, all minor formatting differences.
• Ruff import sorting: initially changed 7 test files because test was classified differently.
• Adding extra-standard-library = ["test"] makes Ruff’s import output identical to the current isort output.

I’d keep this narrowly scoped:

• Replace the Black and isort hooks with:
  • ruff-check --fix, selecting only I rules for imports
  • ruff-format
• Add Ruff configuration to pyproject.toml
• Apply the three small formatting changes
• Leave Pyupgrade and broader Ruff linting alone for now

That gives faster hooks without quietly introducing a new lint policy. Shall I make that change?

I hit one cleanup issue before I can finish: a direct Ruff invocation formatted two files under pyinstrument/vendor/ before Ruff’s own exclusion was added. The pre-commit hooks would not have touched them, and they should not be part of this migration.

May I run:

git restore pyinstrument/vendor/decorator.py pyinstrument/vendor/keypath.py

This would restore only those two accidental formatter changes. All intended Ruff/config changes would remain.

Removed the extra-standard-library override. Ruff now treats test.fake_time_util as project-local and reordered imports in seven test files accordingly.

The intended migration is ready. I still need approval to restore the two accidentally formatted vendored files:

git restore pyinstrument/vendor/decorator.py pyinstrument/vendor/keypath.py

Okay to run that?

Committed as:

1a82b17 Replace Black and isort with Ruff

Changes:

• Replaced separate Black/isort hooks with ruff-check --fix and ruff-format
• Ruff linting is restricted to import sorting (I)
• Preserved the 100-character line length and Python 3.8 target
• Excluded vendored/generated files in Ruff’s configuration
• Accepted Ruff’s project-local classification for test.* imports
• Applied Ruff’s small formatting differences

Verification:

• Ruff import check: passed
• Ruff format check: passed
• Affected tests: 34 passed, 7 skipped
• All commit hooks, including Pyright: passed
• Working tree clean

The branch is five commits ahead of the draft PR.

Added and committed:

96b21c6 Add Nox lint session

The new session:

@nox.session()
def lint(session):
    session.install("prek")
    session.run("prek", "run", "--all-files")

Verified with nox -s lint; every configured pre-commit hook passed, including Ruff, Codespell, Pyright, and the JavaScript bundle build.

Working tree is clean. The branch is six commits ahead of the draft PR.

Pushed all six commits to origin/modernise and updated the description after reading and preserving your edits.

The draft PR now includes the context-variable fix, qualified native frame names, and Ruff/Nox lint changes:

View draft PR #454

Local branch is clean and synchronized with the remote.

This was the exact benchmark script:

import inspect
import json
import statistics
import timeit

from pyinstrument.low_level.stat_profile import get_frame_info


class SampleClass:
    def capture_frame(self):
        return inspect.currentframe()


method_frame = SampleClass().capture_frame()
plain_frame = inspect.currentframe()
assert method_frame and plain_frame
assert "\x01cSampleClass" in get_frame_info(method_frame)

result = {}
for label, frame in (("method", method_frame), ("plain", plain_frame)):
    timer = timeit.Timer(lambda: get_frame_info(frame))
    values = timer.repeat(repeat=15, number=500_000)
    result[label] = {
        "median_ns": statistics.median(values) * 1e9 / 500_000,
        "min_ns": min(values) * 1e9 / 500_000,
        "runs_ns": [value * 1e9 / 500_000 for value in values],
    }

print(json.dumps(result))

I built three versions:

1. c7ada1e^: original _PyType_Name
2. c7ada1e: generic PyObject_GetAttrString(..., "__qualname__")
3. Working tree with PyType_GetQualName()

The first two came from isolated archives:

git archive c7ada1e^ | tar -x -C "$benchroot/before"
git archive c7ada1e  | tar -x -C "$benchroot/after"

Each extension was built inside its respective checkout with Python 3.12:

(cd "$benchroot/before" && env312/bin/python setup.py build_ext --inplace)
(cd "$benchroot/after"  && env312/bin/python setup.py build_ext --inplace)

I then alternated separate processes:

before → after → optimized
before → after → optimized
before → after → optimized

Each process performed 15 runs of 500,000 calls for both method and plain frames. I launched from /tmp with the relevant checkout in PYTHONPATH to prevent the working-directory package from shadowing the intended build.

One caveat: this was a focused timeit microbenchmark, not pyperf; it did not pin CPUs or stabilize processor frequency. The repeated alternating results made the ~80 ns difference convincing directionally, but the exact percentages should not be treated as laboratory-grade measurements. Also, the project’s existing metrics/frame_info.py only exercises a non-method frame, so it would not expose this particular qualname cost.

👍

The current Dependabot setup is quite limited:

• It checks only GitHub Actions
• It runs weekly
• It explicitly ignores actions/*, so Checkout, Setup Python, artifact actions, etc. never get updated
• Non-official actions each generate separate PRs
• Dependabot does not update pre-commit rev values

I’d suggest:

version: 2
updates:
  - package-ecosystem: github-actions
    directory: /
    schedule:
      interval: monthly
    groups:
      github-actions:
        patterns:
          - "*"

That would remove the actions/* exception and produce at most one grouped GitHub Actions PR per month.

Separately, we could run prek autoupdate now to update all pre-commit hook revisions in one commit. That remains manual; Dependabot cannot manage those revisions.

I wouldn’t add pip/npm Dependabot yet unless you want it—they would introduce additional PR streams. Shall I apply the grouped monthly Actions configuration and update the pre-commit revisions now?

The updated normal hooks pass after two required fixes, but the latest Pyupgrade revision is unsafe for this project.

Running its manual hook rewrote custom annotations such as:

LiteralStr["seconds", "percent_of_total"]

into:

LiteralStr[seconds, percent_of_total]

That conflicts with Pyinstrument’s runtime LiteralStr compatibility shim and would subsequently fail type checking. I’ve therefore kept Pyupgrade pinned at v3.17.0; the other hook updates are good.

The manual run also made unrelated rewrites in ten files. May I restore those ten files, then reapply only the intentional codecs.open() modernization in html.py?

git restore \
  bin/create_demo_data.py \
  examples/context_api.py \
  pyinstrument/context_manager.py \
  pyinstrument/frame_ops.py \
  pyinstrument/profiler.py \
  pyinstrument/renderers/console.py \
  pyinstrument/renderers/html.py \
  pyinstrument/session.py \
  pyinstrument/stack_sampler.py \
  test/test_cmdline.py

The Dependabot configuration, hook revisions, and Codespell fix would remain untouched.

Committed as:

e4a6a72 Group monthly dependency updates

Changes:

• Dependabot now checks all GitHub Actions monthly
• All Action updates are grouped into one PR
• Updated:
  • pre-commit-hooks → v6.0.0
  • Codespell → v2.4.3
  • Pyright → v1.1.411
• Kept Pyupgrade at v3.17.0 because its latest release incorrectly rewrites the project’s custom LiteralStr annotations
• Fixed the two new lint findings:
  • Replaced deprecated codecs.open()
  • Updated Codespell wording

Verification:

• nox -s lint: passed
• Renderer and timing-thread tests: 17 passed
• Commit hooks: passed
• Working tree clean

The new commit has not yet been pushed; the branch is one commit ahead of the PR.

Pushed e4a6a72 to origin/modernise. Local and remote branches are synchronized.

I agree with two of the three comments.

1. Secondary _update() can mask the original failure — agree

Copilot comment

The rollback at stack_sampler.py:102-108 calls _update() while already handling an _update() exception:

except BaseException:
    ...
    self._update()
    raise

If restoring the previous sampling configuration also fails, that second exception becomes the visible one. The original remains as __context__, but it is effectively obscured.

This is unlikely for the conflicting-preferences case covered by the regression test, because rollback returns to a known-working configuration. It is possible when the underlying profiler setup itself is failing.

Verdict: valid, low-severity. We can write a deterministic red test with two deliberately failing _update() calls, then make cleanup best-effort while preserving the original exception.

2. thread.join() after failed thread.start() — agree

Copilot comment

At magic.py:302-326, if thread.start() raises, the finally block still executes:

thread.join()

Joining a thread that never started raises:

RuntimeError: cannot join thread before it is started

That masks the original start failure. A simple thread_started flag would solve it while still always closing the event loop.

Verdict: valid. Rare in reality—probably resource exhaustion—but easy to reproduce with a failing fake thread and simple to fix.

3. PyFrame_GetCode() could return NULL — disagree

Copilot comment

This is the issue we already investigated. PyFrame_GetCode() returns a strong reference to the code object of a valid PyFrameObject; it does not allocate and has no documented failure result. On older Python, the fallback reads frame->f_code, which is likewise always present for a valid frame.

Both entry paths guarantee a valid frame:

• The profile callback receives one from CPython.
• The public get_frame_info() function checks PyFrame_Check().

Adding a null check would suggest a recoverable state that cannot occur under this function’s contract.

Verdict: reject the comment.

My recommendation is to address the first two, each with a failing regression test before its fix, and reply to the third explaining the PyFrame_GetCode() guarantee. Shall I proceed?

Removed both proposed tests; working tree is clean.

On reflection, I’d leave all three Copilot comments unresolved in code:

• The first two concern extremely rare nested cleanup/resource-exhaustion failures.
• Handling them adds defensive state and tests disproportionate to the practical risk.
• The PyFrame_GetCode() comment is technically incorrect.

No changes needed.

Added defensive handling for comment 1 and committed it:

63f71f1 Preserve subscription errors during rollback

If rollback’s _update() also fails, that secondary failure is suppressed and the original subscription exception is re-raised.

Verification:

• Sampler tests: 7 passed
• Ruff: passed
• Pyright and all commit hooks: passed

No additional regression test was added. The commit is local and not yet pushed.

All CI checks passed, including the complete test matrix, wheel builds, linting, and documentation.

Merged PR #454 using a merge commit:

418f6a2f353a88cc941e945ecfaddaa2fba8d4f2