Drop _VeraExit(130) Ctrl-C workaround; centralize SIGINT handling on wasmtime 45 (#599)#709
Merged
Merged
Conversation
Closes #599 and #595. wasmtime-py 45.0.0 (released 2026-05-26) is the first PyPI release whose host-import trampoline catches BaseException rather than Exception (bytecodealliance/wasmtime-py#337, the fix Vera filed via #336). That makes a raw KeyboardInterrupt raised inside a host import unwind the wasm call safely and re-raise in Python at the call site, instead of escaping into Rust with an undefined ABI return value and aborting with a libmalloc SIGABRT (#595). Empirically determined the post-upgrade behavior before changing anything: the upstream fix prevents the crash but does NOT by itself produce the clean exit-130 UX — it re-raises a bare KeyboardInterrupt, which would otherwise escape execute() (whose except clauses only catch Exception, not BaseException). So this is a *relocation*, not a deletion: - Bump pyproject.toml wasmtime floor 44.0.0 -> 45.0.0 (+ uv.lock). - Add ONE `except KeyboardInterrupt` handler at the func(store, ...) call site in execute(), mapping a Ctrl-C in any host import to exit_code=130 with captured stdout/stderr/state preserved (modeled on the existing IO.exit / _VeraExit handler). - Remove the four per-host-import launder guards: one in host_sleep, three across host_read_char (Unix non-TTY, Unix TTY cbreak, Windows getwch). The Unix-TTY terminal restore stays correct — it lives in a `finally`, so the terminal is restored before the interrupt propagates. One source of truth replaces four duplicated workarounds. Verification: - Real SIGINT to a running `vera run` of an IO.sleep program: exit code 130, pre-interrupt stdout preserved, no SIGABRT (the exact #595 repro). - pytest tests/ -m "not stress": 3930 passed, 15 skipped (full suite green on wasmtime 45.0.0 — the broader-surface upgrade the issue flagged). - mypy vera/ clean; doc counts + site assets consistent. Tests (tests/test_runtime_traps.py::TestHostSleepKeyboardInterrupt): - Replaced the structural "host_sleep has the guard" assertion with a structural assertion that the four guards are gone AND execute() carries the centralized exit-130 handler. - Kept the IO.sleep end-to-end test (passes identically before and after the relocation — it is the behavioral contract that justified removal). - Added an IO.read_char end-to-end test (non-TTY branch) proving the removed read_char guards still exit cleanly via the central handler. KNOWN_ISSUES.md: removed the host_sleep runtime-workaround row (table now empty -> "No runtime workarounds currently in place"). Co-Authored-By: Claude <noreply@anthropic.invalid>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRelease v0.0.160 centralises Ctrl-C handling into a single ChangesKeyboardInterrupt centralisation refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #709 +/- ##
==========================================
+ Coverage 90.80% 90.84% +0.03%
==========================================
Files 60 60
Lines 24091 24086 -5
Branches 292 292
==========================================
+ Hits 21875 21880 +5
+ Misses 2209 2199 -10
Partials 7 7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@HISTORY.md`:
- Line 348: The file contains inconsistent counts for active-development days:
update the earlier summary occurrence that currently reads "59 active
development days" so it matches the later line which reads "Total: **810+
commits, 160 tagged releases, 60 active development days.**"; search for the
text "59 active development days" (or the summary paragraph near the top) and
change the numeric value to "60" to ensure both summaries are identical.
In `@tests/test_runtime_traps.py`:
- Around line 2195-2198: The test reads the API source using
Path(...).read_text() without specifying encoding, which can cause
cross-platform differences; update the read of api.py by calling read_text with
encoding="utf-8" (the variable is api_src in tests/test_runtime_traps.py) so the
file is opened using explicit UTF-8 encoding for portable test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 597d5dbe-9b17-4691-9111-b8e46dce146f
⛔ Files ignored due to path filters (6)
docs/index.htmlis excluded by!docs/**docs/index.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**docs/sitemap.xmlis excluded by!docs/**uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (10)
CHANGELOG.mdHISTORY.mdKNOWN_ISSUES.mdREADME.mdROADMAP.mdTESTING.mdpyproject.tomltests/test_runtime_traps.pyvera/__init__.pyvera/codegen/api.py
- HISTORY.md: the top-of-file summary still read '59 active development days' (my earlier replace_all only matched the 'Total:' footer phrasing, not the 'across N active development days' intro). Now 60, consistent with the footer. - tests/test_runtime_traps.py: the structural test's api.py read_text() now passes encoding='utf-8', matching the adjacent test at line 2043 and the project's cross-platform fixture convention (CLAUDE.md). The other read_text() at line 1826 is pre-existing and outside this PR's diff; left untouched to keep the change minimal. Co-Authored-By: Claude <noreply@anthropic.invalid>
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
1 participant
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.
Summary
Removes the
_VeraExit(130)Ctrl-C workaround that #599 bookmarked, now that the upstream fix has shipped on PyPI.Key finding (the issue's premise was incomplete). #599 framed step 2 as "remove the guard and let
KeyboardInterruptpropagate naturally." I verified the actual post-upgrade behavior first, and it turns out the upstream fix (except Exception→except BaseException, [bytecodealliance/wasmtime-py#337]) prevents the SIGABRT but does not by itself produce the clean exit-130 UX — it re-raises a bareKeyboardInterrupt, which would escapeexecute()entirely (itsexceptclauses catchException, andKeyboardInterruptis aBaseException). So this is a relocation, not a deletion.What changed
wasmtime>=44.0.0→>=45.0.0(+uv.lock). 45.0.0 (2026-05-26) is the first PyPI release containing the trampoline fix; 44.0.0 was tagged 2026-04-20, before the 2026-05-07 merge, so it never had it.except KeyboardInterruptat thefunc(store, ...)call site inexecute(), mapping a Ctrl-C in any host import toexit_code=130with stdout/stderr/state preserved (modeled on the existingIO.exit/_VeraExithandler).host_sleep, three acrosshost_read_char(Unix non-TTY, Unix TTY cbreak, Windows getwch). Terminal restore on the Unix-TTY path stays correct: it lives in afinally, so it runs before the interrupt propagates.One source of truth replaces four duplicated workarounds.
Why 45.0.0, not 44.0.0
The bookmark text said "current pin 43.0.0" — but the pin had already drifted to 44.0.0. That was a false floor: 44.0.0 predates the fix. Confirmed three ways (git ancestry
ahead_by, release dates, and the installed 44.0.0 still carryingexcept Exception as e:). 45.0.0 has wheels for every supported platform (macosx_11_0_arm64,macosx_10_13_x86_64,manylinux*_x86_64,win_amd64; allpy3-none,requires_python>=3.9).Verification
vera runof anIO.sleepprogram → exit code 130, pre-interrupt stdout preserved, no SIGABRT. This is the exact macOS malloc abort during wasmtime cleanup after Ctrl-C-in-host-import #595 repro (Conway's Life Ctrl-C), now clean.pytest tests/ -m "not stress"→ 3930 passed, 15 skipped on wasmtime 45.0.0. The upgrade touches all execution (the broader-surface change the issue flagged); zero regressions.mypy vera/clean; all 25 pre-commit hooks green; doc counts + site assets consistent.Tests (
TestHostSleepKeyboardInterrupt)raise _VeraExit(130)guards are gone andexecute()carries the centralized exit-130 handler.Release prep (in this PR, per workflow)
Version bump to v0.0.160 across all tracked sites, CHANGELOG cut, HISTORY row, doc counts,
KNOWN_ISSUES.mdruntime-workaround row removed (table now empty → "No runtime workarounds currently in place").Closes #599
Closes #595
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Bug Fixes
Documentation