fix(nvkit): give shutdown signals a typed error consumers can test - #1450
Open
kristinapathak wants to merge 2 commits into
Open
fix(nvkit): give shutdown signals a typed error consumers can test#1450kristinapathak wants to merge 2 commits into
kristinapathak wants to merge 2 commits into
Conversation
The servers run group reports a shutdown signal by returning
fmt.Errorf("received signal %s", sig), so a graceful stop and a genuine
failure reach callers through the same channel with nothing but message
text to tell them apart. Every consumer therefore wrote its own check,
and each one compared against the SIGINT wording. SIGTERM stringifies as
"terminated" rather than "interrupt", so those checks classify every
SIGTERM as a crash -- and SIGTERM is how Kubernetes asks a container to
stop, so the checks excuse the signal that never arrives in production
and panic on the one that always does.
#1319 fixed this in worker-utils by matching on signal names instead of
one hard-coded string, but left the underlying design issue in place:
there is no sentinel to test against, so every other consumer still
string-matches, and four of them still get it wrong.
Add pkg/nvkit/shutdown as the single place that decision lives:
- SignalError plus the ErrSignal sentinel, so a stop is recognizable
with errors.Is rather than by inspecting a message. Error() keeps the
historical wording, which consumers pinned to an older lib revision,
and consumers whose servers package comes from the separate nvcf-go
module, still depend on.
- IsSignalError, which prefers errors.Is and falls back to matching on
the signal names. The fallback is what lets a caller adopt this before
its own producer is on the typed error.
- Signals(), so the set a server installs a handler for and the set
callers forgive cannot drift apart.
servers now returns the typed error and takes its signal list from the
same place. The run group's signal actor moves into a named function so
the behavior is testable without signalling the test process.
This lands on its own so the pseudo-version exists before consumers
depend on it. Converting the remaining call sites, along with the lib
pin bumps those modules need, follows in a second change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
The predicate each consumer actually needs is "should I panic on this?", which is err != nil && !IsSignalError(err). Spelled out by hand that has a footgun: IsSignalError(nil) is false, so dropping the nil guard turns a clean exit into a panic. Five call sites are about to be converted; giving them one call rather than one expression is the difference between consolidating the logic and copying it again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change adds a shared Go shutdown package with typed signal errors and fatal-error classification. The gRPC server uses the package to handle SIGINT, SIGTERM, and run-group cancellation through a common helper. Bazel targets and tests cover the new behavior. ChangesShutdown handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change adds typed shutdown-signal errors while preserving existing signal behavior, with no actionable merge-blocking risk remaining after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Title checkExplanation The title follows Conventional Commits syntax and accurately describes the shutdown error change, but
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
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.
TL;DR
The
serversrun group reports a shutdown signal asfmt.Errorf("received signal %s", sig), with no sentinel to test against. That forces every consumer to string-match, and four of them still match on the SIGINT wording alone and therefore panic on SIGTERM. This addspkg/nvkit/shutdown— a typedSignalError, anErrSignalsentinel, and anIsSignalErrorpredicate — and wiresserversto return it.This is step 1 of 2 and changes no service behavior on its own. It lands separately so the pseudo-version exists before consumers depend on it; see For the Reviewer for why the split is load-bearing.
Additional Details
service.Run()and friends have to distinguish a graceful stop from a crash, but the run group hands both to the caller through the same channel: a non-nil error whose only distinguishing feature is its text. Each consumer wrote its own check, and each compared against"received signal interrupt".syscall.SIGTERMstringifies asterminated, so the error readsreceived signal terminated, does not match, and falls through tozap.S().Panic. SIGTERM is how Kubernetes asks a container to stop and SIGINT essentially never arrives there, so the checks excuse the signal that does not happen in production and panic on the one that always does.#1319 fixed
worker-utilsby deriving the match from signal names instead of one hard-coded string, but deliberately left the design issue in place. This is that follow-up.pkg/nvkit/shutdownSignalError+ErrSignal— a stop is now recognizable witherrors.Is, not by inspecting a message.Error()deliberately preserves the historical wording; it is part of the contract, not an implementation detail.IsSignalError— preferserrors.Is, falls back to matching on the signal names. The fallback is what lets a consumer adopt this immediately, before its own producer is on the typed error.Signals()— the set a server installs a handler for and the set callers forgive now come from one list and cannot drift.serversgrpc.goreturnsshutdown.NewSignalError(sig)and takes itssignal.Notifylist fromshutdown.Signals(). The run group's signal actor moves into a namedawaitShutdownSignalso the behavior is testable without signalling the test process; the extraction is mechanical and theselectis unchanged.For the Reviewer
Why the fallback is permanent, not a shim.
vanity-gatewaytakes itsserverspackage fromgithub.com/NVIDIA/nvcf-go, a separate module, so it can never receive this sentinel. It can still import the predicate. Consumers pinned to olderlibrevisions are in the same position until their pins move.Why this is split into two PRs. All five affected services are registered in
tools/ci/github-release-subprojects.json, so afix:commit touching them auto-cuts a release tag on merge. Converting the call sites in this PR would tag modules whosego.modstill pins alibrevision withoutpkg/nvkit/shutdown— green here, because Bazel resolvesliblocally throughgo.work.bazel, but broken for external consumers like the GitLabgo-nvcf-workerrepo, which resolves the pin.src/libraries/go/libis not a released subproject, so this PR cuts no tags. The follow-up carries thelibpin bumps and the call-site conversions together, keeping every tag it cuts self-consistent.Worth a close look:
SignalError.Error()(wording is contract), andIsSignalError's fallback, which is text matching and so cannot be exact — an unrelated error embeddingreceived signal terminatedreads as a shutdown. The doc comment says so and points producers atNewSignalError.BUILD.bazelsrcs/depsentries were added by hand because Bazel was not available in the environment used to prepare this change. Please confirmbazel run //:gazelleproduces no diff.For QA
No QA needed — no service behavior changes until the follow-up.
Verified in
src/libraries/go/libwith the flags CI uses (GOWORK=off GOFLAGS=-mod=vendor):go build ./...,go vet ./pkg/nvkit/servers/... ./pkg/nvkit/shutdown/..., andgofmt -lare clean.go test -race ./pkg/nvkit/shutdown/... ./pkg/nvkit/servers/...passes, as does the full./pkg/nvkit/...sweep.go.mod/go.sumand the vendor tree are untouched; the new package imports only the standard library.Tests were written before the implementation and confirmed failing first.
pkg/nvkit/shutdown/shutdown_test.go—IsSignalErroracross typed SIGTERM/SIGINT, wrapped, untyped historical wording (both signals, and wrapped), a signal-shaped message for a signal we do not handle, an unrelated failure, and nil; sentinel matching undererrors.Is; the wording contract;errors.Asrecovering the signal; a zero value not panicking; andSignals()returning the right list as a copy.pkg/nvkit/servers/shutdown_test.go—awaitShutdownSignalreports both signals aserrors.Is-matchable, returns nil when the run group interrupts it instead, and blocks until something actually happens.Issues
Relates to #1449
Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes