feat(async): concurrent async(Http.get/post) on host worker threads — release v0.0.192#842
Conversation
Nine spike checks executed against wasmtime-py 45.0.0 + wasmtime CLI 46.0.1 -- all pass (one PARTIAL: structured trap frames degrade to message text through a component boundary). No sprint stage invalidated. WASI.md records the results, the invariants (the set_wasi-or-SIGABRT rule, canonical-ABI text landmines, the 4096-byte write cap, the sync-only #406 gate, the wasm-tools supply-chain trap), and where each loose end lands. Referenced from TOOLCHAIN.md. Refs #841 #305 #237 #406 Skip-changelog: design doc only; the sprint stages carry their own CHANGELOG entries Co-Authored-By: Claude <[email protected]>
First slice of host-threaded concurrent <Async>: the checker rule that
keeps the verifier's sequential-semantics claims honest. async(e) will
only be made concurrent when e's effect row is within the commutative
whitelist ({Http} v1; the Async marker has no operations); any other
row now gets W002 "evaluates eagerly" with rationale/fix/spec_ref --
never a silent implication of concurrency.
The effect collector resolves effect-op calls to their parent effect
and fn calls to declared rows, recursively; unresolvable callees are
conservatively non-commutative. Registered W002 in ERROR_CODES;
diagnostic-fields gate green (spec_ref resolves to §9.5.4 "Async").
Test-first: TestAsyncConcurrencyWhitelist841 was RED (expected W002,
got []) before the rule; mutation-validated four ways -- hook deleted,
whitelist over-permissive (+IO), whitelist over-firing (-Http), and
fn-call rows ignored each flip exactly their discriminating tests.
Refs #841 (the lowering/runtime half follows in this PR)
Co-Authored-By: Claude <[email protected]>
…841) The concurrency half deferred when #59 shipped the Async surface. async(Http.get(url)) / async(Http.post(url, body)) -- with call-free argument expressions -- fuse into a single vera.async_http_* host import that submits the request to a host ThreadPoolExecutor at the async(...) point and returns the Future as a #578 bit-31-tagged handle wrapper (new wrap kind 4). await probes the value's first word for the wrapper tag (0xFEEDC004): fused handles unwrap and block on vera.async_await; eager values pass through unchanged. Two overlapping gets are pinned by a server-side request-ordering test (ThreadingHTTPServer, no wall-clock). Design invariants: - The fusion predicate lives in ONE module (vera/wasm/async_fusion.py) consumed by both import-emission passes (_scan_io_ops pre-scan and the WasmContext translation), so they cannot desync. It is strictly narrower than W002's whitelist, so an "evaluates eagerly" warning can never coexist with concurrent execution. - The await handle-check keys on the literal type Future<Result<String, String>> -- the only type fused futures inhabit, always a heap pointer, making the tag probe memory-safe. Value-typed futures (Future<Int> is an i64) keep the identity lowering; alias-typed slots fall back loudly (wrapper tag matches no constructor -> trap), never silently. - Workers run only the pure fetch halves (fetch_get/fetch_post, split out of the Http host callbacks in vera/runtime/http.py) and never touch guest memory; the Result ADT is built at await time on the guest thread under the usual rooting discipline. - GC follows the full opaque-handle contract: wrapper shadow-rooted at the async site (operand-stack window), Phase 2c fires host_decref_handle(4, handle) for unawaited futures (cancel + evict, observable via ExecuteResult.host_store_sizes["future"]). - Ctrl-C during a blocked await rides the wasmtime>=45 BaseException trampoline to the exit-130 handler; the executor is torn down (shutdown(cancel_futures=True)) on every exit path via the invocation finally. - Browser runtime stays eager (spec-conformant MAY): the request fires synchronously at the async point, the (isOk, payload) strings are buffered host-side (a guest ptr in a JS map would be GC-invisible), and the ADT is built at await. - Functions may now declare a Future<T> return type (previously E605-skipped; Future is transparent in the type mapper). Spec: 9.5.4 rewritten (implementation MAY evaluate concurrently when the row is commutative; reference-implementation shape documented), 7.4 effect-ordering note, 12.9.3 browser divergence row. SKILL.md async section rewritten with a fan-out example. Tests (all RED-first; 8 mutations each kill their discriminating tests): TestConcurrentAsync841 (5), TestFutureHandleGCRooting841 (4), Ctrl-C-during-await e2e, browser fused-shape e2e, updated #591 fetch-half anchors. Release v0.0.192; the queued #420/#419/#839/#835 [Unreleased] split entries ride this release. HISTORY opens Stage 16 (the server-effects sprint). Closes #841 Co-Authored-By: Claude <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #842 +/- ##
==========================================
- Coverage 92.06% 91.94% -0.13%
==========================================
Files 90 92 +2
Lines 27321 27626 +305
Branches 324 332 +8
==========================================
+ Hits 25154 25400 +246
- Misses 2159 2218 +59
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds host-threaded concurrent execution for whitelisted ChangesConcurrent Async feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GuestCode
participant CallsMarkup
participant AsyncHTTP
participant ThreadPoolExecutor
participant FutureStore
GuestCode->>CallsMarkup: async(Http.get(url))
CallsMarkup->>AsyncHTTP: vera.async_http_get(ptr,len)
AsyncHTTP->>ThreadPoolExecutor: submit(fetch_get(url))
ThreadPoolExecutor-->>FutureStore: store Future under handle
AsyncHTTP-->>GuestCode: return wrapped Future handle
GuestCode->>CallsMarkup: await(future)
CallsMarkup->>AsyncHTTP: vera.async_await(handle)
AsyncHTTP->>FutureStore: lookup(handle)
FutureStore-->>AsyncHTTP: Future
AsyncHTTP->>ThreadPoolExecutor: Future.result()
AsyncHTTP-->>GuestCode: Result<String,String>
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…ions (#841) Review round (pr-review-toolkit code-reviewer on PR #842) found one confirmed critical bug and two documentation-accuracy issues: - CRITICAL: await of a cross-module call returning Future<Result<String, String>> lowered to identity -- neither the unqualified imported-call shape (compute_future_ret_fns scanned only local declarations) nor the qualified m::f shape (no ModuleCall arm in await_needs_check) classified, so the kind-4 wrapper flowed into the match's unconditional last arm and the future was silently never awaited. Fixed RED-first (TestCrossModuleFusedAwait841, both shapes): compute_future_ret_fns now derives from the codegen return-type registry (_fn_ret_type_exprs -- local Pass-1 registrations + the Pass-0 #628 cross-module harvest) and await_needs_check gains a ModuleCall arm. - The "a smuggled wrapper traps loudly on match" claim was false: the match lowering's final arm is an unconditional catch-all, so a wrapper reaching an identity await is read as the ADT (zero-length Err string), not trapped. Corrected in the async_fusion docstring, spec 9.5.4, and the CHANGELOG entry. The residual unclassifiable shape (indirectly-called closure returning this future type) is now a documented limitation: KNOWN_ISSUES.md row + #843. - Alias-typed futures: the accurate story is that an alias-typed let has no WASM representation today, so the function is E602-skipped before any await could mis-lower -- reworded in spec/CHANGELOG. Also from review: aligned the CompileResult async_ops_used indentation at the two early-return sites, and updated the _emit_wrap_handle inner bucket comment that still read Decimal-only. Doc counts follow (5,579 tests). Refs #841 Co-Authored-By: Claude <[email protected]>
|
Review round 1 (pr-review-toolkit code-reviewer) — recorded. Findings and dispositions, all addressed in 7b304f1:
Full gates re-run green after the fix (suite, mypy, ruff, doc-counts, site assets, limitations sync, spec/SKILL example checkers). |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@TESTING.md`:
- Line 9: The test-count summary is inconsistent because the total in the Tests
row does not match the passed, stress, and skipped breakdown. Update the
TESTING.md counts so the aggregate and sub-counts reconcile, using the Tests
table entry and the reported passed/stress/skipped figures as the source of
truth.
In `@vera/checker/calls.py`:
- Around line 211-283: W002 currently only inspects effect rows in
_warn_non_commutative_async and _collect_expr_effects, so it misses async
arguments that are non-fusable by shape even when they look whitelisted by
effects. Update the checker to also validate the async argument’s call shape
(for example, only direct Http.get/Http.post QualifiedCall with call-free
arguments), and warn when the expression contains a helper FnCall or other
non-fusable form. Use _warn_non_commutative_async, _collect_expr_effects, and
the async fusion shape rules from fused_async_arg_target / _expr_is_call_free as
the reference points when tightening the condition.
In `@vera/codegen/api.py`:
- Around line 1069-1081: Add a test that exercises Ctrl-C while a real async
HTTP request is in flight, since the current coverage only mocks Future.result()
and uses ftp:// and never hits the shutdown wait path. Update or extend
test_async_await_keyboard_interrupt_end_to_end to use a live HTTP fetch and
trigger KeyboardInterrupt during the request, so the
_async_executor.shutdown(wait=True, cancel_futures=True) behavior is covered
when an already-started urlopen is still running. Reference the async execution
path in vera.codegen.api around func(store, *call_args) and the _async_executor
shutdown handling to keep the test aligned with the real exit-130 flow.
In `@vera/codegen/compilability.py`:
- Around line 312-319: The narrowing assert in compilability.py should be marked
to satisfy Ruff’s S101 check. Update the `assert isinstance(inner,
ast.QualifiedCall)` inside the `fused_async_target` handling in
`CompilabilityAnalyzer._scan_io_ops` to include `# noqa: S101`, matching the
existing mypy-only narrowing asserts so the lint gate stays green.
🪄 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 Plus
Run ID: 42848729-e368-4018-ad6b-b46694dd88cf
⛔ Files ignored due to path filters (6)
docs/SKILL.mdis excluded by!docs/**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/**uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (36)
CHANGELOG.mdHISTORY.mdREADME.mdROADMAP.mdSKILL.mdTESTING.mdTOOLCHAIN.mdWASI.mdpyproject.tomlscripts/check_skill_examples.pyscripts/check_spec_examples.pyspec/07-effects.mdspec/09-standard-library.mdspec/12-runtime.mdtests/test_browser.pytests/test_checker_effects.pytests/test_codegen_effects.pytests/test_codegen_gc_rooting.pytests/test_runtime_traps.pyvera/README.mdvera/__init__.pyvera/browser/runtime.mjsvera/checker/calls.pyvera/codegen/api.pyvera/codegen/assembly.pyvera/codegen/closures.pyvera/codegen/compilability.pyvera/codegen/core.pyvera/codegen/functions.pyvera/errors.pyvera/runtime/async_http.pyvera/runtime/http.pyvera/wasm/async_fusion.pyvera/wasm/calls_containers.pyvera/wasm/calls_markup.pyvera/wasm/context.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
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 `@vera/codegen/core.py`:
- Around line 453-462: _Recompute the future-return function registry after
monomorphized registrations._ `_future_ret_fns` is currently built too early in
`Core.compile`, before the `mono_decls` loop registers clone names like
`grab$Int`, so `await_needs_check` can miss rewritten awaited calls. Move or
repeat the `compute_future_ret_fns(self._fn_ret_type_exprs)` step after `for
mdecl in mono_decls: self._register_fn(mdecl)` and before any `_compile_fn`
calls, so monomorphized names are included in the await-check classification.
In `@vera/wasm/async_fusion.py`:
- Around line 117-143: Module-qualified future detection is using only the bare
function name, so `ModuleCall` can apply the fused await path to the wrong
target when different modules share a name. Update the resolution in
`compute_future_ret_fns`/the `ModuleCall` await-lowering path to key off the
same resolved `(path, name)` WAT target used for module-qualified calls, and
compare that resolved target against the future-return registry instead of just
`"grab"`. Make sure both the pre-scan and `WasmContext` await handling use the
same resolved target lookup so imported and local functions are classified
consistently.
🪄 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 Plus
Run ID: 385b8183-1378-4ec5-b1ce-d31f4e77fc82
📒 Files selected for processing (10)
CHANGELOG.mdKNOWN_ISSUES.mdREADME.mdROADMAP.mdTESTING.mdspec/09-standard-library.mdtests/test_codegen_modules.pyvera/codegen/core.pyvera/wasm/async_fusion.pyvera/wasm/calls_containers.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
… 2 (#841) CodeRabbit round on PR #842 (6 findings, each verified empirically): - CONFIRMED (repro: name collision, value 0, async_await import missing): ModuleCall awaits classified by BARE name, so a local `grab` returning a different Future shape misclassified await(fetcher::grab(...)) in both directions (identity -> silent wrapper smuggle; or a probe on an i64 future). Fixed RED-first: the modules.py #628 harvest now also keys (module path, name) -> return-type, compute_future_ret_module_fns derives a qualified set, and the ModuleCall arm of await_needs_check resolves by (path, name). New collision regression test. - NOT REPRODUCIBLE (mono recompute claim): a generic fn with a concrete Future<Result<String, String>> return classifies via its template name -- classification runs on the pre-monomorphization AST; the call-site mangling to wrap$Int happens later, during translation. Pinned with a new test so a future move to AST-level mono rewriting fails loudly here. - Ctrl-C coverage gap: added an event-gated live-request test -- interrupt_main() fires only after the server confirms the request is in flight, the handler is then released so the invocation finally's shutdown(wait=True) has a real worker to wait out; pins exit-130 + no-hang on the un-mocked path. - Two S101 narrowing asserts now carry noqa (the CI lint job runs ruff --select S on vera/); TESTING.md summary breakdown reconciled (5,534 passed + 26 stress + 22 skipped = 5,582). - The effect-eligible-but-shape-unfusable advisory (W003 proposal) is a design extension beyond the #841 scope (W002's contract is commutativity, and the no-warning helper-wrap shape is deliberately pinned) -- filed as #844 with a ROADMAP row. Refs #841 Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@TESTING.md`:
- Line 90: Update the TESTING.md coverage row for test_codegen_effects.py to
include the newly added generic-fn future-return case. In the existing summary
near the other pinned effect tests, add a reference to
test_await_of_generic_fn_with_concrete_future_return and describe it as the
concrete Future<Result<String, String>> await-site classification check so the
documented coverage matches the current test suite.
- Line 145: The TestHostSleepKeyboardInterrupt summary undercounts the
fused-await coverage: it now includes four end-to-end tests, not three. Update
the `TESTING.md` description around `TestHostSleepKeyboardInterrupt` to mention
the additional live, unmocked interrupted fetch test
(`test_async_await_keyboard_interrupt_live_request`) alongside the existing
`IO.sleep`, `IO.read_char`, and mocked `Future.result()` fused-await coverage.
Keep the wording aligned with the test names and the `execute()`
keyboard-interrupt handling described in this section.
In `@vera/wasm/async_fusion.py`:
- Around line 147-166: The docstring in compute_future_ret_module_fns uses
inconsistent triple-single-quote style compared with the rest of the file.
Update that function’s docstring to the standard triple-double-quote form used
elsewhere so it matches Black’s formatting conventions and the surrounding
documentation style.
🪄 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 Plus
Run ID: db9cfca7-1e82-4a9f-a298-e38f391653e3
📒 Files selected for processing (15)
CHANGELOG.mdREADME.mdROADMAP.mdTESTING.mdtests/test_codegen_effects.pytests/test_codegen_modules.pytests/test_runtime_traps.pyvera/codegen/closures.pyvera/codegen/compilability.pyvera/codegen/core.pyvera/codegen/functions.pyvera/codegen/modules.pyvera/wasm/async_fusion.pyvera/wasm/calls_markup.pyvera/wasm/context.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
…uotes (#841) Two TESTING.md description rows catch up with the round-2 tests (the generic-fn future-return pin; three -> four Ctrl-C e2e tests incl. the live in-flight variant), and compute_future_ret_module_fns's docstring quote style joins the rest of the file. Refs #841 Co-Authored-By: Claude <[email protected]>
|
@coderabbitai review |
✅ Action performedReview finished.
|
What
Delivers the concurrency half deferred when #59 shipped the
<Async>language surface, and cuts release v0.0.192 — Stage A of the server-effects sprint (WASI.mdrecords the Stage-0 groundwork this builds on).async(Http.get(url))/async(Http.post(url, body))— with call-free argument expressions — now fuse into a singlevera.async_http_get/vera.async_http_posthost import that issues the request on a hostThreadPoolExecutorat theasync(...)point (request issuance keeps program order) and returns theFutureas a #578 bit-31-tagged handle wrapper (new wrap kind 4).awaitprobes the value's first word for the wrapper tag: fused handles unwrap and block onvera.async_await; eager values pass through unchanged. Every otherasyncshape keeps the eager identity lowering.Two overlapping
async(Http.get)s are pinned by a server-side request-ordering test (localThreadingHTTPServer, the/ahandler holds until/barrives — no wall-clock assertions).Design invariants
_scan_io_opspre-scan and theWasmContexttranslation; if they disagree, a fused call site references an import the pre-scan suppressed (WAT compile error) or a sync import is declared that translation never calls. The predicates live in one new module —vera/wasm/async_fusion.py— imported by both, and mutation M5 (pre-scan intercept removed) proves the pin.Http.get/postcall, call-free args) is strictly narrower than the checker'sW002commutative whitelist — a warnedasynccan never execute concurrently, and an unwarned-but-eager one is spec-conformant (§9.5.4's MAY).Future<Result<String, String>>— the only type fused futures inhabit, always a heap pointer, so the tag probe (0xFEEDC004at offset 0) is memory-safe. Value-typed futures (Future<Int>is an i64) keep the identity lowering; a pure program emits zero task imports. Alias-typed slots fall back loudly (the wrapper tag matches no constructor → trap), never silently.fetch_get/fetch_post, split out of theHttphost callbacks invera/runtime/http.py) run on workers and return Python strings; theResultADT is built at await time on the guest thread. The GC shadow-stack overflow in array_map (and sibling iterative builders) at ~4000 heap-allocating elements #570/html_parse and json_parse trap with Out-of-bounds memory access on inputs that pressure GC during host-side tree walk #692 UAF class cannot reach a worker.both(async(A), async(B)), get/post-distinguished Err text making an aliased wrapper observable) — and Phase 2c fireshost_decref_handle(4, handle)for unawaited futures (cancel + evict, observable viaExecuteResult.host_store_sizes["future"]).KeyboardInterruptduring a blockedawaitrides thewasmtime>=45BaseException trampoline to the centralized exit-130 handler (new e2e sibling of theIO.sleep/IO.read_charpins); the executor is torn down (shutdown(cancel_futures=True)) on every exit path via the invocationfinally.asyncpoint; the(isOk, payload)strings are buffered host-side (a guest pointer held only in a JS map would be invisible to the conservative GC scan) and the ADT is built at await. New Node e2e test pins the fused-shape Err path.Future<T>return types now compile. Previously a function declaring aFuture<T>return was silently[E605]-skipped;Futureis now transparent in the type mapper (RED-first via the GC-registration test).Mutation validation (8/8 kill)
i32.anddropped at awaitregister_wrapperpin(M7's first fixture survived the mutation — let-bound wrappers are rooted elsewhere — so the operand-stack fixture was added and confirmed RED under the mutation before keeping both.)
Docs
Spec §9.5.4 rewritten (implementation MAY evaluate concurrently when the effect row is commutative; the reference implementation's exact shape documented, alias limitation noted), §7.4 effect-ordering note, §12.9.3 browser-divergence row; SKILL.md async section rewritten with a fan-out example;
vera/README.mdmodule map (newasync_fusion.py+async_http.pyrows); TESTING.md tables + battery descriptions; #591 test anchors moved to the fetch halves.Release v0.0.192
Version sync across 6 files; CHANGELOG section (the queued #420/#419/#839/#835
[Unreleased]split entries ride this release); HISTORY opens Stage 16: the server-effects sprint; doc counts 5,577.Verification
pytest tests/— 5,529 passed, 22 skipped (incl. browser parity 108)pytest -m stress— 26 passedmypy vera/clean;ruff checkclean without--fixscripts/check_*.pygreen (conformance 103, examples 35, doc-counts, version-sync, site-assets, encoding, diagnostic-fields, skill/spec/readme/faq/html examples, licenses, wheel gate, limitations sync, changelog gate)Closes #841
🤖 Generated with Claude Code
Summary by CodeRabbit
async(Http.get/post(...))now lowers to a fused concurrent path on the native runtime;awaitblocks for results, including browser support.W002warning whenasync(...)must evaluate eagerly/serially outside the commutativity whitelist.awaitcoverage (including new browser parity).