Repository navigation
fix: resolve containerd pid reliably before sending SIGHUP - #2127
Open
jpschmitt02 wants to merge 1 commit into
Open
jpschmitt02 wants to merge 1 commit into
jpschmitt02 wants to merge 1 commit into
Conversation
jpschmitt02
requested review from
cdesiniotis,
henry118 and
tariq1890
as code owners
October 7, 2026 17:08
jpschmitt02
force-pushed
the
fix/signal-containerd-passcred-race
branch
2 times, most recently
from
October 7, 2026 17:19
7716d8c to
c2617c5
Compare
cdesiniotis
reviewed
Oct 8, 2026
cdesiniotis
left a comment
Contributor
There was a problem hiding this comment.
@jpschmitt02 thank you for the contribution! I left some minor review comments.
SignalContainerd enabled SO_PASSCRED after connect(). containerd's gRPC server writes its first frames immediately on accept, and the kernel only records peer credentials on data queued while SO_PASSCRED is set, so the credentials could parse as pid 0. The pid was passed to kill(2) unvalidated, and kill(0, SIGHUP) signals the calling process's own process group: the toolkit killed itself, logged "Successfully signaled containerd", and crash-looped until a retry resolved the real pid, restarting containerd well after node startup had completed. Enable SO_PASSCRED before connect() via net.Dialer.Control so every message carries credentials, and reject non-positive peer pids so a failed resolution feeds the existing retry loop instead of a signal. Fixes NVIDIA#2126 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Justin Schmitt <justin.schmitt@baseten.co>
jpschmitt02
force-pushed
the
fix/signal-containerd-passcred-race
branch
from
October 8, 2026 21:29
c2617c5 to
e9fd70f
Compare
cdesiniotis
approved these changes
Oct 8, 2026
Contributor
|
/ok to test e9fd70f |
Coverage Report for CI Build 37847168109Coverage increased (+0.2%) to 44.795%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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
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.
Description
Fixes #2126.
SignalContainerdresolves containerd's PID from socket peer credentials, but enablesSO_PASSCREDafterconnect(). containerd's gRPC server writes its first frames immediately on accept, and the kernel only records peer credentials on data queued whileSO_PASSCREDis set — so the credentials can parse aspid=0(with overflow uid/gid 65534). The PID was passed tokill(2)unvalidated, andkill(0, SIGHUP)signals the calling process's own process group: the toolkit killed itself, logged "Successfully signaled containerd", and crash-looped, with the real containerd kill landing on a later retry — well after node startup had completed. Details, production evidence, and a standalone reproducer are in #2126.This change:
containerdPidhelper and enablesSO_PASSCREDbeforeconnect()vianet.Dialer.Control, so every message from the server carries credentials;Checklist
make test)make lint)Testing
TestContainerdPidresolves the PID of a mock unix server that writes immediately on accept (the containerd behavior that triggers the race), andTestContainerdPidNeverZeroruns 200 iterations as a regression test — on the previous code this resolvespid=0for a fraction of attempts (1–3/200 on an idle machine; 200/200 with a 50ms delay beforesetsockopt), with the fix it is deterministic.go build,go vet,golangci-lint run(v2.6.1), and the package test suite pass on go 1.26 (linux/arm64 container).🤖 Generated with Claude Code