WASM: recover panics in compile goroutine to avoid unresolved browser promises#47854
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Adds panic containment to the WASM compiler so browser promises reject instead of hanging.
Changes:
- Recovers panics from the compile goroutine.
- Converts panic values into JavaScript
Errorrejections.
Show a summary per file
| File | Description |
|---|---|
cmd/gh-aw-wasm/main.go |
Adds panic-to-Promise rejection handling. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Medium
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47854 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The panic recovery is correctly placed and wired. LIFO defer ordering ensures recover() runs before handler.Release(). Every goroutine exit path now settles the Promise. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 8.55 AIC · ⌖ 4.45 AIC · ⊞ 5K
There was a problem hiding this comment.
Review: Panic recovery in WASM compile goroutine
The fix is correct. Defer order is right (recover fires first, then handler.Release()), and every normal execution path still settles the Promise. One non-blocking edge case is worth documenting.
Finding: silent swallow when resolve.Invoke itself panics
If resolve.Invoke(result) panics after doCompile succeeds (e.g., the JS runtime tears down mid-call), recover() catches the panic and calls reject. The browser ignores the second settlement — the Promise remains resolved and the caller sees the correct outcome. However, the panic is silently swallowed with no observable signal, making it invisible during debugging.
Consider a settled flag to skip the redundant reject:
var settled bool
defer func() {
if r := recover(); r != nil && !settled {
reject.Invoke(js.Global().Get("Error").New(fmt.Sprintf("compileWorkflow panic: %v", r)))
}
}()
result, err := doCompile(markdown, files, filename)
if err != nil {
reject.Invoke(js.Global().Get("Error").New(err.Error()))
return
}
settled = true
resolve.Invoke(result)Non-blocking; does not need to hold up merge.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 27.3 AIC · ⌖ 4.58 AIC · ⊞ 5.7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is correct and well-scoped; two follow-up improvements are flagged.
📋 Key Themes & Highlights
Key Themes
- Missing regression test: The panic-recovery path is untested; a future refactor could silently drop the
recover()and re-introduce the browser hang. - Debuggability: Panic messages omit a stack trace, which would be valuable when diagnosing browser-side failures.
Positive Highlights
- ✅ Minimal, targeted change — exactly the right scope for a correctness fix.
- ✅
deferordering is correct:handler.Release()fires after recovery, preventing a use-after-free on the JS func. - ✅ All Promise execution paths now settle (
resolve,rejecton error,rejecton panic) — no more browser hangs. - ✅ Clear PR description with a before/after code sample.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.4 AIC · ⌖ 4.72 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| go func() { | ||
| defer handler.Release() | ||
| defer func() { |
There was a problem hiding this comment.
[/tdd] No regression test for the panic-recovery path — without one, a future refactor could silently drop the recover() and re-introduce the browser hang.
💡 Suggested approach
Add a test that injects a panic into doCompile and asserts the returned Promise rejects with an Error whose message matches compileWorkflow panic:. Even a Go-level unit test for the inner goroutine body via a thin wrapper would document the intent and catch regressions.
@copilot please address this.
There was a problem hiding this comment.
Implemented in fa765f0: I extracted the goroutine settlement path into runCompileWithRecovery and added cmd/gh-aw-wasm/compile_recovery_test.go with explicit success/error/panic regression tests so this path is now covered.
| defer handler.Release() | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| reject.Invoke(js.Global().Get("Error").New(fmt.Sprintf("compileWorkflow panic: %v", r))) |
There was a problem hiding this comment.
[/diagnosing-bugs] The panic message prefix "compileWorkflow panic: " is helpful, but consider also including a stack trace to make post-mortem debugging easier. Go's runtime/debug.Stack() can be captured at recover-time before the goroutine unwinds.
💡 Suggested addition
import "runtime/debug"
if r := recover(); r != nil {
stack := debug.Stack()
reject.Invoke(js.Global().Get("Error").New(
fmt.Sprintf("compileWorkflow panic: %v\n%s", r, stack),
))
}Browsers surface the full Error.message in DevTools, so the stack trace will be visible to developers without any additional tooling.
@copilot please address this.
There was a problem hiding this comment.
Implemented in fa765f0: panic rejection now includes a stack trace via runtime/debug.Stack() (compileWorkflowPanicError), and the panic-path test asserts the rejection includes both the panic prefix and stack context.
|
@copilot please run the Failed checks:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the Unresolved review feedback:
Run: https://github.com/github/gh-aw/actions/runs/30135795800
|
|
Triage | Category: Recommended action: Score breakdown: impact=30 (browser/WASM stability), urgency=23 (CI green, active area), quality=15 (CI pass, test included)
|
|
🎉 This pull request is included in a new release. Release: |
compileWorkflowin the WASM entrypoint spawneddoCompilein a goroutine without panic recovery. A panic could terminate the WASM runtime path and leave the JSPromiseunresolved (browser hang) instead of returning an actionable error.Goroutine panic containment
defer-basedrecover()inside the compile goroutine incmd/gh-aw-wasm/main.go.Errorobjects and routed throughreject(...).Promise lifecycle behavior
resolveon success,rejecton normal compile error).