e2e: de-globalize BehaviorOnFatal and slog default handler sinks#703
Conversation
cmdutil.BehaviorOnFatal was reinstalled by every RootCmd().Execute() call, overwriting a shared closure -- two concurrent invocations (e.g. parallel e2e subtests) could capture each other's error instead of their own. Replace the BehaviorOnFatal+CheckErr dance with a checkErr() helper that still routes through cmdutil.CheckErr for its message formatting (some e2e assertions pin on the exact "error: ..." text), but scopes the lock tightly around install-and-consume rather than around the render itself, so it doesn't re-serialize plugin.Run for parallel subtests. setupDeprecationFilter rebound the global slog default handler on every render; sprigin reads slog.Default() fresh on every template function call (not just once when the funcMap is built), so two renders in flight at once would race that rebind. Since every record this handler is known to emit carries the deprecated-notice tag it drops, installing it once per process (sync.Once) and forwarding the unreachable non-deprecated case to os.Stderr is behaviorally identical to before, without needing to serialize renders. Neither sink can be "threaded through the call path" as literally proposed -- both are process-global by the design of the vendored libraries that own them (kubectl's cmdutil, go-sprout's sprigin) -- so this scopes/serializes access narrowly instead. Also re-audited for other shared cluster-scoped resource names beyond createBadNode and vap-binding.yaml (both already handled): no other e2e subtest hardcodes a cluster-scoped name another subtest could collide with. Fixes #701 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request addresses concurrency issues related to process-global state. It introduces a mutex (fatalMu) to serialize the installation and consumption of cmdutil's global fatal handler in cmd/main.go, and refactors setupDeprecationFilter in pkg/plugin/render_engine.go to run only once per process using sync.Once. However, the global fatal handler registered in checkErr is never restored to its default state, which can lead to stale closures suppressing errors in subsequent executions. It is recommended to restore the default handler using a deferred call.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Addresses gemini-code-assist review on #703: BehaviorOnFatal registers a process-global handler; without restoring it, a later CheckErr call could invoke a stale closure from a prior checkErr instead of the default. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses concurrency issues and race conditions by serializing the process-global fatal handler with a mutex in cmd/main.go and using sync.Once to install the global slog deprecation filter in pkg/plugin/render_engine.go. Feedback was provided to defensively return the original error in checkErr if the fatal handler is not triggered, preventing potential silent error swallowing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Summary
cmdutil.BehaviorOnFatalwas reinstalled by everyRootCmd().Execute()call, overwriting a shared closure — concurrent invocations could capture each other's error. Replaced with acheckErr()helper that still usescmdutil.CheckErrfor its message formatting, but scopes the lock (fatalMu) tightly around install-and-consume rather than around the render itself, so it doesn't re-serializeplugin.Runfor parallel subtests.setupDeprecationFilterrebound the global slog default handler on every render; go-sprout's sprigin readsslog.Default()fresh on every template function call, so concurrent renders would race that rebind. Installed once per process (sync.Once) instead — every record this handler is known to emit carries the deprecated-notice tag it drops, so this is behaviorally identical to before.cmdutil, go-sprout'ssprigin), so this scopes/serializes access narrowly instead of eliminating the global.createBadNodeandvap-binding.yaml(both already handled) — no other e2e subtest hardcodes a cluster-scoped name another subtest could collide with.Fixes #701.
Test plan
go build ./...,go vet ./...,gofmt -l .go test ./...(457 tests, non-e2e suite) — includesTestRootCmdWithoutACluster, which exercises the exactcheckErr"error: ..." formatting pathcheckErrcode path, so expected to behave the same🤖 Generated with Claude Code