wazero: fix guard shutdown leak, logging namespace, and typed trap detection#3790
wazero: fix guard shutdown leak, logging namespace, and typed trap detection#3790
Conversation
…p detection
- Add Registry.Close(ctx) method to close all registered guards that
implement io.Closer; call it from UnifiedServer.Close() and
InitiateShutdown() to release WASM runtime resources on shutdown
- Fix logging namespace confusion in registry.go: replace log.Printf
(which was using the guard:context namespace logger) with
logger.LogInfo("guard", ...) for operational events
- Use typed sys.ExitError check in isWasmTrap: check exit code 0 as a
normal process exit (not a trap) before falling back to string matching
- Add tests for Registry.Close and sys.ExitError handling in isWasmTrap
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/842dba68-e351-47f7-84e1-cf101f122182
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the wazero-based WASM guard integration by ensuring guard runtimes are released on shutdown, fixing guard registry logging to use the intended logging mechanism, and making WASM trap detection more robust by using typed sys.ExitError checks instead of only string matching.
Changes:
- Add
Registry.Close(ctx)and wire it intoUnifiedServer.Close()andUnifiedServer.InitiateShutdown()to release guard-held WASM runtime resources. - Fix
internal/guard/registry.gooperational logs to uselogger.LogInfo("guard", ...)and remove a duplicate log line. - Update
isWasmTrapto treatsys.ExitErrorwith exit code 0 as non-trap; add focused unit tests for exit-code behavior.
Show a summary per file
| File | Description |
|---|---|
| internal/server/unified.go | Calls into the guard registry during shutdown/close to release guard runtime resources. |
| internal/guard/registry.go | Adds Registry.Close(ctx) and adjusts logging away from the wrong debug namespace. |
| internal/guard/wasm.go | Improves trap detection by checking typed WASI exit errors before falling back to string matching. |
| internal/guard/wasm_test.go | Adds test coverage for isWasmTrap behavior with sys.ExitError (including wrapped errors). |
| internal/guard/guard_test.go | Adds unit tests validating registry Close behavior across closable/non-closable guards and error cases. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 4
| // Release WASM runtime resources held by guards | ||
| if us.guardRegistry != nil { | ||
| us.guardRegistry.Close(context.Background()) | ||
| } |
There was a problem hiding this comment.
InitiateShutdown closes guards immediately while the HTTP server may still be draining in-flight requests. If any in-flight request is currently executing a guard call, closing the underlying WASM module/runtime concurrently can race (WasmGuard serializes calls with a mutex, but Close does not take that mutex). Consider either deferring guardRegistry.Close until after HTTP shutdown/drain completes, or ensuring guard Close implementations synchronize with in-flight calls (e.g. WasmGuard.Close acquiring g.mu).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Review FeedbackThree items — I'll push commits for each. 1.
|
- Use RLock/RUnlock instead of Lock/Unlock in Registry.Close() since the guards map is only read (not modified) during the closers scan - Add summary LogInfo after closing guards for shutdown visibility - Add double-close idempotency test with closeCount tracking on mock Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three actionable improvements to the wazero WASM guard integration identified in a Go module review: WASM runtimes were never closed on shutdown,
registry.gowas emitting logs under the wrong namespace (guard:contextinstead ofguard:registry), and trap detection relied on fragile string matching.Changes
Registry.Close()+ shutdown wiringClose(ctx context.Context)toRegistry— iterates guards and callsClose(ctx)on those that implement itUnifiedServer.Close()andInitiateShutdown()(nil-safe) to release WASM JIT runtime resources on shutdownWithCloseOnContextDone(true)was effectively inert since guards were created withcontext.Background()Fix logging namespace in
registry.goregistry.gowas callinglog.Printf(...)which resolved to the package-levellogvar declared incontext.gowith namespace"guard:context"— meaningDEBUG=guard:registrysilently dropped those messageslogger.LogInfo("guard", ...)for operational events; retaineddebugLog.Printffor debug-only pathsTyped
sys.ExitErrorcheck inisWasmTrapstrings.Contains(err.Error(), "wasm error:")— fragile against wazero message format changes, and would incorrectly poison a guard on a cleanexit(0)(e.g. TinyGo init)*sys.ExitErrorviaerrors.Asfirst; only non-zero exit codes are treated as traps:Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build900194802/b514/launcher.test /tmp/go-build900194802/b514/launcher.test -test.testlogfile=/tmp/go-build900194802/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -w o4Cr-wAQW cfg x_amd64/vet -c -I /tmp/go-build185-bool x_amd64/vet 1085�� elemetry.io/otel-errorsas cfg x_amd64/vet --gdwarf-5 ions =0 x_amd64/vet(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build900194802/b496/config.test /tmp/go-build900194802/b496/config.test -test.testlogfile=/tmp/go-build900194802/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build900194802/b389/vet.cfg g_.a ache/go/1.25.8/x64/src/bufio/bufio.go x_amd64/vet --gdwarf-5 1085977/b140/_cg-atomic -o x_amd64/vet -W _.a /tmp/go-build185-ifaceassert x_amd64/vet . .io/otel/attribu-atomic --64 x_amd64/vet(dns block)nonexistent.local/tmp/go-build900194802/b514/launcher.test /tmp/go-build900194802/b514/launcher.test -test.testlogfile=/tmp/go-build900194802/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -w o4Cr-wAQW cfg x_amd64/vet -c -I /tmp/go-build185-bool x_amd64/vet 1085�� elemetry.io/otel-errorsas cfg x_amd64/vet --gdwarf-5 ions =0 x_amd64/vet(dns block)slow.example.com/tmp/go-build900194802/b514/launcher.test /tmp/go-build900194802/b514/launcher.test -test.testlogfile=/tmp/go-build900194802/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -w o4Cr-wAQW cfg x_amd64/vet -c -I /tmp/go-build185-bool x_amd64/vet 1085�� elemetry.io/otel-errorsas cfg x_amd64/vet --gdwarf-5 ions =0 x_amd64/vet(dns block)this-host-does-not-exist-12345.com/tmp/go-build900194802/b523/mcp.test /tmp/go-build900194802/b523/mcp.test -test.testlogfile=/tmp/go-build900194802/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o 1085977/b432/_pkg_.a -trimpath x_amd64/vet us.pb.go t/unicode/bidi -lang=go1.25 x_amd64/vet(dns block)If you need me to access, download, or install something from one of these locations, you can either: