stub: bound Configure() wait in Start() and break connClosed() deadlock

[wevans-ant]

stub.Start() holds stub.Lock() for its whole body and then does an unbounded <-stub.cfgErrC waiting for the runtime's Configure() callback. If the runtime accepts RegisterPlugin but never sends Configure() — 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 ttrpc OnClose handler connClosed() tries to take the same lock and deadlocks, so the plugin never observes the disconnect, WithOnClose never fires, and the plugin cannot recover.

This change:

• replaces the naked <-stub.cfgErrC receive with a select that also honours ctx cancellation and a registrationTimeout-bounded timer, so Start() cannot block indefinitely;
• has connClosed() do a non-blocking send of a ttrpc.ErrClosed-wrapped error onto cfgErrC before taking the lock, so a blocked Start() is woken and releases the lock (this is the mechanism from stub: cancel Start() for early connection loss. #238);
• makes Configure()'s deferred cfgErrC send non-blocking so it cannot wedge if Start() has already given up or connClosed() has already filled the buffer.

The new adaptation-suite spec reproduces the deadlock deterministically: it lets Register succeed, blocks the plugin's Configure handler so cfgErrC is never written, then has the runtime time out the Configure RPC and close the connection. Without this change Start() never returns and the spec fails at the 3 s guard; with it, Start() returns a wrapped ttrpc.ErrClosed in ~200 ms.

Supersedes #238.

[wevans-ant]

@klihub, this picks up where #238 left off. It keeps the connClosed() -> cfgErrC non-blocking send from your PR, and adds a bounded select in Start() so the wait is also broken by ctx cancellation or a registrationTimeout timer — in case the runtime accepts Register but never sends Configure while the connection stays open.

Would be interested to know why you dropped the PR, let me know if there's something I am missing.

[klihub]

@klihub, this picks up where #238 left off. It keeps the connClosed() -> cfgErrC non-blocking send from your PR, and adds a bounded select in Start() so the wait is also broken by ctx cancellation or a registrationTimeout timer — in case the runtime accepts Register but never sends Configure while the connection stays open.

Would be interested to know why you dropped the PR, let me know if there's something I am missing.

@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).

[wevans-ant]

#238's failing run's logs have expired, but I reproduced its 1 ns registration-timeout test under -race and the race is on the stub.cfgErrC field, not registrationTimeout:

1. connClosed() reads the field before taking the lock (the non-blocking send)
2. Start() writes it under the lock (stub.cfgErrC = make(...))

with the artificially lowered timeout, the connection dies before register() has sent anything through the ttrpc client, so there's no synchronization between the two accesses and they race.

I've moved the srvErrC/cfgErrC make()s to before ttrpc.NewClient so the field is assigned before the OnClose goroutine is spawned, and added the 1 ns test as a second spec so -race keeps it covered.

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).

The timer bounds the wait for the Configure() message that carries the runtime's value, so at the point we arm it that value can't be known yet. Once Configure() does arrive, cfgErrC fires and the timer is moot. stub.registrationTimeout is still updated by Configure() for register() on reconnect and the RegistrationTimeout() accessor.