mirror of
https://github.com/mudler/LocalAI.git
synced 2026-09-21 21:54:52 -04:00
Ettore Di Giacinto
d7002b89c5
Re-review round 2. One blocking item, and it was a spec I wrote: eight goroutines raced at the probe cache and nothing made them coalesce, so a straggler that missed the flight re-entered the probe and double-closed a channel. It panicked about one run in three and took the four-suite race block down. The green verification I reported was not reproducible, which means one green run was never evidence for a spec that coordinates goroutines. Its comment claimed the probe blocked until every goroutine was inside flight.Do, and that gap was exactly the panic: the comment described the design intended rather than the one written. It is deterministic now rather than tolerant. singleflight.DoChan registers its channel on an in-flight call under the group's own mutex and returns without running its function, so calling it while the leader is provably parked inside the probe joins that exact flight with no window and no dependence on the scheduler. The spec asserts the join really happened, that the joiner got the reason and not only the answer, and that the probe ran once; the entered channel is sent on rather than closed so a second probe fails an assertion instead of panicking. Twenty runs green under race against the committed code, five out of five red on the mutation back to a closed-over variable. The future-decorator gap is closed in the lint gate, but not the way the review suggested, and the reason is worth recording. HasMethod rejects inline signatures outright, its method-reference form needs a package ruleguard's own typechecker can import and that typechecker cannot import this module, and Implements tests the value method set while every Unwrap is on a pointer receiver, so it fired on all three wrappers that already had one. So the safe shape is structural instead. grpc.WrappedBackend gives the same pass-through method set plus Unwrap on a value receiver, and a decorator that embeds it is transparent by construction; forgetting stops being expressible rather than merely discouraged, which is the move loopbackService already makes in the worker. FileStagingClient and ConnectionEvictingClient embed it and their hand-written Unwrap methods are gone. The ruleguard rule then only has to catch the raw embedding, needs no type filter, and cannot misfire. It was verified to fire on a throwaway wrapper and stay silent on a correct one, and reports nothing across core and pkg with the baseline disabled. InFlightTrackingClient is the one exception and says why in a nolint: it embeds ControlBackend deliberately so that leaving an inference method unwrapped breaks the build, and WrappedBackend embeds the full interface, so adopting it would silently restore pass-through for every inference method and delete that guarantee. Assisted-by: Claude Opus 5 [claude-code] Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
47 lines
2.6 KiB
Go
47 lines
2.6 KiB
Go
//go:build ruleguard
|
|
|
|
// Package gorules holds the go-ruleguard rules gocritic runs inside
|
|
// `make lint`. It is never compiled into the binary: the build tag keeps it out
|
|
// of every normal build, and golangci-lint loads the file as data.
|
|
package gorules
|
|
|
|
import "github.com/quasilyte/go-ruleguard/dsl"
|
|
|
|
// backendWrapperMustBeUnwrappable fires on a struct that decorates a gRPC
|
|
// backend by embedding the raw interface.
|
|
//
|
|
// This exists because the same defect shipped twice. A wrapper that embeds
|
|
// grpc.Backend inherits exactly the methods Backend declares and nothing else.
|
|
// grpc.DialErrorReporter is deliberately NOT on Backend, so a wrapped client
|
|
// silently stops answering "did the transport fail, or did the backend die",
|
|
// and every guard built on that answer reads nil. The consequence is not
|
|
// subtle: core/services/nodes and pkg/model delete replica rows and stop
|
|
// backends on that answer, so a wrapper that swallows it turns a momentary loss
|
|
// of route into fleet-wide model reclamation.
|
|
//
|
|
// The rule is SYNTACTIC, and deliberately so. The obvious formulation, "embeds
|
|
// a backend and has no Unwrap", cannot be written: HasMethod rejects inline
|
|
// signatures outright ("inline func signatures are not supported yet"), its
|
|
// method-reference form needs a package ruleguard's own typechecker can import
|
|
// and it cannot import this module, and Implements tests the VALUE method set
|
|
// while every Unwrap here would be on a pointer receiver. So instead of
|
|
// checking for the method, this checks for the shape that CANNOT lack it:
|
|
// grpc.WrappedBackend provides the same pass-through method set plus Unwrap,
|
|
// with a value receiver, so anything embedding it is transparent by
|
|
// construction. Forgetting is then not expressible rather than merely
|
|
// discouraged, which is the same move loopbackService makes in the worker.
|
|
//
|
|
// Test doubles are excluded by path in .golangci.yml: they embed a NIL backend
|
|
// to inherit the interface's method set, decorate nothing, and have no
|
|
// transport answer to forward.
|
|
func backendWrapperMustBeUnwrappable(m dsl.Matcher) {
|
|
m.Import("github.com/mudler/LocalAI/pkg/grpc")
|
|
|
|
m.Match(
|
|
`type $w struct { $*_; grpc.Backend; $*_ }`,
|
|
`type $w struct { $*_; grpc.ControlBackend; $*_ }`,
|
|
`type $w struct { $*_; grpc.InferenceBackend; $*_ }`,
|
|
).
|
|
Report(`$w decorates a gRPC backend by embedding the raw interface, so grpc.LastDialErrorOf cannot see through it and every transport-failure guard behind it reads nil, which deletes replica rows for workers that are merely unroutable. Embed grpc.WrappedBackend instead: it gives the same pass-through plus Unwrap. If $w decorates nothing, silence this with //nolint:gocritic and say so.`)
|
|
}
|