diff --git a/hscontrol/mapper/batcher.go b/hscontrol/mapper/batcher.go index 0a1e30d0..c1349f75 100644 --- a/hscontrol/mapper/batcher.go +++ b/hscontrol/mapper/batcher.go @@ -15,6 +15,13 @@ import ( "tailscale.com/tailcfg" ) +// Sentinel errors for batcher operations. +var ( + ErrInvalidNodeID = errors.New("invalid nodeID") + ErrMapperNil = errors.New("mapper is nil") + ErrNodeConnectionNil = errors.New("nodeConnection is nil") +) + var mapResponseGenerated = promauto.NewCounterVec(prometheus.CounterOpts{ Namespace: "headscale", Name: "mapresponse_generated_total", @@ -80,11 +87,11 @@ func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*t } if nodeID == 0 { - return nil, fmt.Errorf("invalid nodeID: %d", nodeID) + return nil, fmt.Errorf("%w: %d", ErrInvalidNodeID, nodeID) } if mapper == nil { - return nil, fmt.Errorf("mapper is nil for nodeID %d", nodeID) + return nil, fmt.Errorf("%w for nodeID %d", ErrMapperNil, nodeID) } // Handle self-only responses @@ -135,7 +142,7 @@ func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*t // handleNodeChange generates and sends a [tailcfg.MapResponse] for a given node and [change.Change]. func handleNodeChange(nc nodeConnection, mapper *mapper, r change.Change) error { if nc == nil { - return errors.New("nodeConnection is nil") + return ErrNodeConnectionNil } nodeID := nc.nodeID() diff --git a/hscontrol/mapper/batcher_lockfree.go b/hscontrol/mapper/batcher_lockfree.go index 3ff3406b..988f0b35 100644 --- a/hscontrol/mapper/batcher_lockfree.go +++ b/hscontrol/mapper/batcher_lockfree.go @@ -16,7 +16,14 @@ import ( "tailscale.com/tailcfg" ) -var errConnectionClosed = errors.New("connection channel already closed") +// Sentinel errors for lock-free batcher operations. +var ( + errConnectionClosed = errors.New("connection channel already closed") + ErrInitialMapTimeout = errors.New("failed to send initial map: timeout") + ErrNodeNotFound = errors.New("node not found") + ErrBatcherShutdown = errors.New("batcher shutting down") + ErrConnectionTimeout = errors.New("connection timeout sending to channel (likely stale connection)") +) // LockFreeBatcher uses atomic operations and concurrent maps to eliminate mutex contention. type LockFreeBatcher struct { @@ -88,12 +95,12 @@ func (b *LockFreeBatcher) AddNode(id types.NodeID, c chan<- *tailcfg.MapResponse case c <- initialMap: // Success case <-time.After(5 * time.Second): - log.Error().Uint64("node.id", id.Uint64()).Err(errors.New("timeout")).Msg("Initial map send timeout") + log.Error().Uint64("node.id", id.Uint64()).Err(ErrInitialMapTimeout).Msg("Initial map send timeout") log.Debug().Caller().Uint64("node.id", id.Uint64()).Dur("timeout.duration", 5*time.Second). Msg("Initial map send timed out because channel was blocked or receiver not ready") nodeConn.removeConnectionByChannel(c) - return fmt.Errorf("failed to send initial map to node %d: timeout", id) + return fmt.Errorf("%w for node %d", ErrInitialMapTimeout, id) } // Update connection status @@ -234,7 +241,7 @@ func (b *LockFreeBatcher) worker(workerID int) { nc.updateSentPeers(result.mapResponse) } } else { - result.err = fmt.Errorf("node %d not found", w.nodeID) + result.err = fmt.Errorf("%w: %d", ErrNodeNotFound, w.nodeID) b.workErrors.Add(1) log.Error().Err(result.err). @@ -492,7 +499,7 @@ func (b *LockFreeBatcher) MapResponseFromChange(id types.NodeID, ch change.Chang case result := <-resultCh: return result.mapResponse, result.err case <-b.done: - return nil, fmt.Errorf("batcher shutting down while generating map response for node %d", id) + return nil, fmt.Errorf("%w: generating map response for node %d", ErrBatcherShutdown, id) } } @@ -707,7 +714,7 @@ func (entry *connectionEntry) send(data *tailcfg.MapResponse) error { case <-time.After(50 * time.Millisecond): // Connection is likely stale - client isn't reading from channel // This catches the case where Docker containers are killed but channels remain open - return fmt.Errorf("connection %s: timeout sending to channel (likely stale connection)", entry.id) + return fmt.Errorf("%w: connection %s", ErrConnectionTimeout, entry.id) } } diff --git a/hscontrol/mapper/builder.go b/hscontrol/mapper/builder.go index df0693e3..cd1d9a9d 100644 --- a/hscontrol/mapper/builder.go +++ b/hscontrol/mapper/builder.go @@ -2,7 +2,6 @@ package mapper import ( "cmp" - "errors" "net/netip" "slices" "time" @@ -71,7 +70,7 @@ func (b *MapResponseBuilder) WithCapabilityVersion(capVer tailcfg.CapabilityVers func (b *MapResponseBuilder) WithSelfNode() *MapResponseBuilder { nv, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - b.addError(errors.New("node not found")) + b.addError(ErrNodeNotFound) return b } @@ -133,7 +132,7 @@ func (b *MapResponseBuilder) WithDebugConfig() *MapResponseBuilder { func (b *MapResponseBuilder) WithSSHPolicy() *MapResponseBuilder { node, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - b.addError(errors.New("node not found")) + b.addError(ErrNodeNotFound) return b } @@ -152,7 +151,7 @@ func (b *MapResponseBuilder) WithSSHPolicy() *MapResponseBuilder { func (b *MapResponseBuilder) WithDNSConfig() *MapResponseBuilder { node, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - b.addError(errors.New("node not found")) + b.addError(ErrNodeNotFound) return b } @@ -165,7 +164,7 @@ func (b *MapResponseBuilder) WithDNSConfig() *MapResponseBuilder { func (b *MapResponseBuilder) WithUserProfiles(peers views.Slice[types.NodeView]) *MapResponseBuilder { node, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - b.addError(errors.New("node not found")) + b.addError(ErrNodeNotFound) return b } @@ -178,7 +177,7 @@ func (b *MapResponseBuilder) WithUserProfiles(peers views.Slice[types.NodeView]) func (b *MapResponseBuilder) WithPacketFilters() *MapResponseBuilder { node, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - b.addError(errors.New("node not found")) + b.addError(ErrNodeNotFound) return b } @@ -232,7 +231,7 @@ func (b *MapResponseBuilder) WithPeerChanges(peers views.Slice[types.NodeView]) func (b *MapResponseBuilder) buildTailPeers(peers views.Slice[types.NodeView]) ([]*tailcfg.Node, error) { node, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - return nil, errors.New("node not found") + return nil, ErrNodeNotFound } // Get unreduced matchers for peer relationship determination.