stub: bound Configure() wait in Start() and break connClosed() deadlock - #298
wevans-ant wants to merge 1 commit into
Conversation
|
@klihub, this picks up where #238 left off. It keeps the Would be interested to know why you dropped the PR, let me know if there's something I am missing. |
3899911 to
147e2be
Compare
@wevans-ant Thanks for picking this up and working on it. About #238, IIRC it triggered a data read/write race in the tests. I don't recall the details, but it might have been on stub.registrationTimeout. If it was then your PR now avoids it, IIUC by sampling registrationTimeout before it could get written in Configure(). Although I think this has the side-effect that if the registration timeout has been altered by configuration on the runtime/NRI server side then the stub/client will not use this timeout but the previous locally known one instead (which should be the default one in the beginning). |
c574df4 to
78ea274
Compare
|
#238's failing run's logs have expired, but I reproduced its 1 ns registration-timeout test under
with the artificially lowered timeout, the connection dies before I've moved the
The timer bounds the wait for the |
|
Friendly ping. |
@wevans-ant Sorry, I've been AFK for a while that's why I haven't responded. I'll try to pick this up and take another look... |
Start() holds stub.Lock() for its entire body and blocks on <-cfgErrC waiting for the runtime's Configure() callback. If the runtime accepts RegisterPlugin but never sends Configure() (observed when a plugin and containerd race during node boot), Start() blocks forever holding the lock. When the connection later closes, the ttrpc OnClose handler connClosed() tries to take the same lock and deadlocks, so the plugin never observes the disconnect and cannot recover. Fix by: - replacing the naked <-cfgErrC receive with a select that also honours ctx cancellation and a registrationTimeout-bounded timer; - having connClosed() do a non-blocking send of a ttrpc.ErrClosed-wrapped error on cfgErrC before taking the lock, so a blocked Start() is woken and releases the lock; - making Configure()'s deferred cfgErrC send non-blocking so it cannot wedge if Start() has already given up or connClosed() has already filled the buffer. To keep this race-clean under -race: - snapshot stub.registrationTimeout into a local before any goroutines are running, since Configure() rewrites that field without the lock; - create srvErrC/cfgErrC before ttrpc.NewClient spawns the goroutine whose OnClose reads cfgErrC without the lock (this is the race that containerd#238's test surfaced). Add two adaptation-suite regression tests: one that lets Register succeed and blocks the plugin's Configure handler so cfgErrC is never written, then has the runtime time out and close the connection (deterministic deadlock repro); and one that has the runtime drop the connection during registration (surfaces the cfgErrC field race under -race). Supersedes containerd#238. Signed-off-by: Chris Evans <wevans@anthropic.com>
78ea274 to
c0686a4
Compare
|
Rebased onto main and ported the two regression tests to the testify style from #309; the stub.go change is unchanged. @klihub whenever you're back, this is ready for another look. On your earlier cc @mikebrow @chrishenzie, who reviewed #238, which this supersedes. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved retry-safety and stale-connection issues remain in stub.go.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR bounds stub.Start() while waiting for runtime configuration and adds regression coverage for connection-loss deadlocks.
Changes:
- Adds cancellation and timeout handling for
Configure(). - Makes configuration and connection-close signaling non-blocking.
- Adds adaptation-suite coverage for startup and registration disconnects.
Outstanding findings:
- Critical: Mutable
cfgErrCcan race with retries or deliver a stale connection error. - Moderate: Pre-start connection failure leaves
stub.connset, preventing successful retries.
File summaries
| File | Description |
|---|---|
pkg/stub/stub.go |
Updates startup and connection-loss handling. |
pkg/adaptation/suite_test.go |
Adds configurable mock plugin behavior. |
pkg/adaptation/adaptation_suite_test.go |
Adds connection-loss regression tests. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| select { | ||
| case stub.cfgErrC <- fmt.Errorf("connection closed before Configure(): %w", ttrpc.ErrClosed): | ||
| default: |
| stub.Lock() | ||
| stub.close() | ||
| stub.Unlock() |
|
@wevans-ant I'll take a look at it later today or tomorrow morning. |
stub.Start()holdsstub.Lock()for its whole body and then does an unbounded<-stub.cfgErrCwaiting for the runtime'sConfigure()callback. If the runtime acceptsRegisterPluginbut never sendsConfigure()— which can happen when a plugin and containerd race during node boot —Start()blocks forever with the lock held. When the connection later closes, the ttrpcOnClosehandlerconnClosed()tries to take the same lock and deadlocks, so the plugin never observes the disconnect,WithOnClosenever fires, and the plugin cannot recover.This change:
<-stub.cfgErrCreceive with aselectthat also honoursctxcancellation and aregistrationTimeout-bounded timer, soStart()cannot block indefinitely;connClosed()do a non-blocking send of attrpc.ErrClosed-wrapped error ontocfgErrCbefore taking the lock, so a blockedStart()is woken and releases the lock (this is the mechanism from stub: cancel Start() for early connection loss. #238);Configure()'s deferredcfgErrCsend non-blocking so it cannot wedge ifStart()has already given up orconnClosed()has already filled the buffer.The new adaptation-suite spec reproduces the deadlock deterministically: it lets
Registersucceed, blocks the plugin'sConfigurehandler socfgErrCis never written, then has the runtime time out theConfigureRPC and close the connection. Without this changeStart()never returns and the spec fails at the 3 s guard; with it,Start()returns a wrappedttrpc.ErrClosedin ~200 ms.Supersedes #238.