chore(lint): satisfy golangci-lint after big refactor

Address 25 issues reported by golangci-lint following the structural
refactor:

- cyclop: split common.Reassembler.Push into upsert/storeChunk/deliver
  helpers (12→5). Move seichannel option-default fill into Options.
  withDefaults so New stays under the limit.
- exhaustive: enumerate ResultPartial / ResultIgnore explicitly in
  seichannel and videochannel switches over common.Result.
- gosec G115: annotate the test-fixture int→uint16/uint32 conversions
  in common_test.go with //nolint:gosec.
- lll: break up the 130+ character one-liners in transport
  unit/integration tests and the videochannel track-ID construction.
- nolintlint: drop the stale //nolint:cyclop in mobile_test.go where
  the underlying complexity already cleared the limit.
- wrapcheck: wrap errors returned from internal/framing and
  internal/runtime in their public callers (handshake, control,
  server.setupCipher, client.setupCipher) so they carry the layer name.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
zarazaex69
2026-05-16 14:38:03 +03:00
parent 190c2b5f84
commit 80cc3bafe4
14 changed files with 122 additions and 70 deletions

View File

@@ -504,7 +504,11 @@ func (c *Client) shutdown() {
} }
func setupCipher(keyHex string) (*crypto.Cipher, error) { func setupCipher(keyHex string) (*crypto.Cipher, error) {
return runtime.SetupCipher(keyHex) cipher, err := runtime.SetupCipher(keyHex)
if err != nil {
return nil, fmt.Errorf("client: %w", err)
}
return cipher, nil
} }
func (c *Client) onData(data []byte) { func (c *Client) onData(data []byte) {

View File

@@ -309,9 +309,16 @@ func parseMessage(raw []byte) (Message, error) {
} }
func writeFrame(w io.Writer, msg Message) error { func writeFrame(w io.Writer, msg Message) error {
return framing.WriteJSON(w, msg, MaxMessageSize) if err := framing.WriteJSON(w, msg, MaxMessageSize); err != nil {
return fmt.Errorf("control: %w", err)
}
return nil
} }
func readFrame(r io.Reader) ([]byte, error) { func readFrame(r io.Reader) ([]byte, error) {
return framing.ReadBytes(r, MaxMessageSize) body, err := framing.ReadBytes(r, MaxMessageSize)
if err != nil {
return nil, fmt.Errorf("control: %w", err)
}
return body, nil
} }

View File

@@ -192,9 +192,16 @@ func Server(rw io.ReadWriter, auth AuthFunc) (Hello, string, error) {
} }
func writeFrame(w io.Writer, msg any) error { func writeFrame(w io.Writer, msg any) error {
return framing.WriteJSON(w, msg, MaxMessageSize) if err := framing.WriteJSON(w, msg, MaxMessageSize); err != nil {
return fmt.Errorf("handshake: %w", err)
}
return nil
} }
func readFrame(r io.Reader) ([]byte, error) { func readFrame(r io.Reader) ([]byte, error) {
return framing.ReadBytes(r, MaxMessageSize) body, err := framing.ReadBytes(r, MaxMessageSize)
if err != nil {
return nil, fmt.Errorf("handshake: %w", err)
}
return body, nil
} }

View File

@@ -183,7 +183,11 @@ func Run(ctx context.Context, cfg Config) error {
} }
func setupCipher(keyHex string) (*crypto.Cipher, error) { func setupCipher(keyHex string) (*crypto.Cipher, error) {
return runtime.SetupCipher(keyHex) cipher, err := runtime.SetupCipher(keyHex)
if err != nil {
return nil, fmt.Errorf("server: %w", err)
}
return cipher, nil
} }
func (s *Server) setupResolver() { func (s *Server) setupResolver() {

View File

@@ -114,31 +114,47 @@ func (r *Reassembler) Push(fragment Fragment) (Result, []byte) {
return ResultDuplicate, nil return ResultDuplicate, nil
} }
msg, ok := r.inbound[fragment.Seq] msg := r.upsert(fragment)
if !ok || msg.CRC != fragment.CRC || msg.TotalLen != fragment.TotalLen ||
len(msg.frags) != int(fragment.FragTotal) {
msg = &InboundMessage{
TotalLen: fragment.TotalLen,
CRC: fragment.CRC,
frags: make([][]byte, fragment.FragTotal),
remain: int(fragment.FragTotal),
}
r.inbound[fragment.Seq] = msg
}
if int(fragment.FragIdx) >= len(msg.frags) { if int(fragment.FragIdx) >= len(msg.frags) {
return ResultIgnore, nil return ResultIgnore, nil
} }
if msg.frags[fragment.FragIdx] == nil { r.storeChunk(msg, fragment)
chunk := make([]byte, len(fragment.Payload))
copy(chunk, fragment.Payload)
msg.frags[fragment.FragIdx] = chunk
msg.remain--
}
if msg.remain > 0 { if msg.remain > 0 {
return ResultPartial, nil return ResultPartial, nil
} }
return r.deliver(fragment.Seq, msg)
}
delete(r.inbound, fragment.Seq) // upsert returns the inbound message tracking entry for fragment.Seq,
// creating a fresh entry if no compatible one is present.
func (r *Reassembler) upsert(fragment Fragment) *InboundMessage {
msg, ok := r.inbound[fragment.Seq]
if ok && msg.CRC == fragment.CRC && msg.TotalLen == fragment.TotalLen &&
len(msg.frags) == int(fragment.FragTotal) {
return msg
}
msg = &InboundMessage{
TotalLen: fragment.TotalLen,
CRC: fragment.CRC,
frags: make([][]byte, fragment.FragTotal),
remain: int(fragment.FragTotal),
}
r.inbound[fragment.Seq] = msg
return msg
}
func (r *Reassembler) storeChunk(msg *InboundMessage, fragment Fragment) {
if msg.frags[fragment.FragIdx] != nil {
return
}
chunk := make([]byte, len(fragment.Payload))
copy(chunk, fragment.Payload)
msg.frags[fragment.FragIdx] = chunk
msg.remain--
}
func (r *Reassembler) deliver(seq uint32, msg *InboundMessage) (Result, []byte) {
delete(r.inbound, seq)
data := assemble(msg) data := assemble(msg)
if crc32.ChecksumIEEE(data) != msg.CRC { if crc32.ChecksumIEEE(data) != msg.CRC {
return ResultIgnore, nil return ResultIgnore, nil
@@ -146,7 +162,7 @@ func (r *Reassembler) Push(fragment Fragment) (Result, []byte) {
if len(r.delivered) > r.maxRecent { if len(r.delivered) > r.maxRecent {
r.delivered = make(map[uint32]uint32) r.delivered = make(map[uint32]uint32)
} }
r.delivered[fragment.Seq] = msg.CRC r.delivered[seq] = msg.CRC
return ResultDelivered, data return ResultDelivered, data
} }

View File

@@ -43,9 +43,9 @@ func TestReassemblerDeliveredAndDuplicate(t *testing.T) {
result, data := r.Push(common.Fragment{ result, data := r.Push(common.Fragment{
Seq: 1, Seq: 1,
CRC: crc, CRC: crc,
TotalLen: uint32(len(payload)), TotalLen: uint32(len(payload)), //nolint:gosec // bounded test fixture
FragIdx: uint16(i), FragIdx: uint16(i),
FragTotal: uint16(len(frags)), FragTotal: uint16(len(frags)), //nolint:gosec // bounded test fixture
Payload: frag, Payload: frag,
}) })
if i < len(frags)-1 { if i < len(frags)-1 {
@@ -63,9 +63,9 @@ func TestReassemblerDeliveredAndDuplicate(t *testing.T) {
result, _ := r.Push(common.Fragment{ result, _ := r.Push(common.Fragment{
Seq: 1, Seq: 1,
CRC: crc, CRC: crc,
TotalLen: uint32(len(payload)), TotalLen: uint32(len(payload)), //nolint:gosec // bounded test fixture
FragIdx: uint16(len(frags) - 1), FragIdx: uint16(len(frags) - 1), //nolint:gosec // bounded test fixture
FragTotal: uint16(len(frags)), FragTotal: uint16(len(frags)), //nolint:gosec // bounded test fixture
Payload: frags[len(frags)-1], Payload: frags[len(frags)-1],
}) })
if result != common.ResultDuplicate { if result != common.ResultDuplicate {
@@ -80,9 +80,9 @@ func TestReassemblerIgnoresCRCMismatch(t *testing.T) {
result, _ := r.Push(common.Fragment{ result, _ := r.Push(common.Fragment{
Seq: 1, Seq: 1,
CRC: 0xdeadbeef, // wrong CRC: 0xdeadbeef, // wrong
TotalLen: uint32(len(payload)), TotalLen: uint32(len(payload)), //nolint:gosec // bounded test fixture
FragIdx: 0, FragIdx: 0,
FragTotal: uint16(len(frags)), FragTotal: uint16(len(frags)), //nolint:gosec // bounded test fixture
Payload: frags[0], Payload: frags[0],
}) })
if result != common.ResultDelivered { if result != common.ResultDelivered {

View File

@@ -100,13 +100,15 @@ func TestNewAndFeatures(t *testing.T) {
func TestNewErrorPaths(t *testing.T) { func TestNewErrorPaths(t *testing.T) {
registerCarrier("datachannel-fail-create", nil, errDCBoom) registerCarrier("datachannel-fail-create", nil, errDCBoom)
if _, err := New(context.Background(), transport.Config{Carrier: "datachannel-fail-create"}); err == nil || err.Error() != "open engine session: boom" { _, err := New(context.Background(), transport.Config{Carrier: "datachannel-fail-create"})
if err == nil || err.Error() != "open engine session: boom" {
t.Fatalf("New() error = %v", err) t.Fatalf("New() error = %v", err)
} }
nonByteStream := &stubSession{caps: engine.Capabilities{}} nonByteStream := &stubSession{caps: engine.Capabilities{}}
registerCarrier("datachannel-no-stream", nonByteStream, nil) registerCarrier("datachannel-no-stream", nonByteStream, nil)
if _, err := New(context.Background(), transport.Config{Carrier: "datachannel-no-stream"}); !errors.Is(err, ErrByteStreamUnsupported) { _, err = New(context.Background(), transport.Config{Carrier: "datachannel-no-stream"})
if !errors.Is(err, ErrByteStreamUnsupported) {
t.Fatalf("New() error = %v, want %v", err, ErrByteStreamUnsupported) t.Fatalf("New() error = %v, want %v", err, ErrByteStreamUnsupported)
} }
} }

View File

@@ -2,6 +2,7 @@ package seichannel
import ( import (
"fmt" "fmt"
"time"
"github.com/openlibrecommunity/olcrtc/internal/transport" "github.com/openlibrecommunity/olcrtc/internal/transport"
) )
@@ -17,6 +18,23 @@ type Options struct {
// TransportOptions marks Options as belonging to the transport options family. // TransportOptions marks Options as belonging to the transport options family.
func (Options) TransportOptions() {} func (Options) TransportOptions() {}
// withDefaults fills unset Options fields with the package defaults.
func (o Options) withDefaults() Options {
if o.FPS <= 0 {
o.FPS = defaultFPS
}
if o.BatchSize <= 0 {
o.BatchSize = defaultBatchSize
}
if o.FragmentSize <= 0 {
o.FragmentSize = defaultFragmentSize
}
if o.AckTimeoutMS <= 0 {
o.AckTimeoutMS = int(defaultAckTimeout / time.Millisecond)
}
return o
}
func optionsFrom(cfg transport.Config) (Options, error) { func optionsFrom(cfg transport.Config) (Options, error) {
if cfg.Options == nil { if cfg.Options == nil {
return Options{}, nil return Options{}, nil

View File

@@ -150,23 +150,7 @@ func New(ctx context.Context, cfg transport.Config) (transport.Transport, error)
return nil, fmt.Errorf("create local video track: %w", err) return nil, fmt.Errorf("create local video track: %w", err)
} }
fps := opts.FPS opts = opts.withDefaults()
if fps <= 0 {
fps = defaultFPS
}
batchSize := opts.BatchSize
if batchSize <= 0 {
batchSize = defaultBatchSize
}
fragmentSize := opts.FragmentSize
if fragmentSize <= 0 {
fragmentSize = defaultFragmentSize
}
ackTimeout := defaultAckTimeout
if opts.AckTimeoutMS > 0 {
ackTimeout = time.Duration(opts.AckTimeoutMS) * time.Millisecond
}
tr := &streamTransport{ tr := &streamTransport{
stream: stream, stream: stream,
track: track, track: track,
@@ -177,10 +161,10 @@ func New(ctx context.Context, cfg transport.Config) (transport.Transport, error)
writerDone: make(chan struct{}), writerDone: make(chan struct{}),
acks: common.NewAckRegistry(), acks: common.NewAckRegistry(),
reassembler: common.NewReassembler(256), reassembler: common.NewReassembler(256),
fragmentSize: fragmentSize, fragmentSize: opts.FragmentSize,
ackTimeout: ackTimeout, ackTimeout: time.Duration(opts.AckTimeoutMS) * time.Millisecond,
frameInterval: time.Second / time.Duration(fps), frameInterval: time.Second / time.Duration(opts.FPS),
batchSize: batchSize, batchSize: opts.BatchSize,
} }
if err := stream.AddTrack(track); err != nil { if err := stream.AddTrack(track); err != nil {
@@ -470,8 +454,8 @@ func (p *streamTransport) handleInboundFrame(frame transportFrame) {
p.onData(data) p.onData(data)
} }
p.sendAck(frame.seq, frame.crc) p.sendAck(frame.seq, frame.crc)
default: case common.ResultPartial, common.ResultIgnore:
// Partial or Ignore: do nothing. // fragment stored or discarded; no peer response needed yet.
} }
} }

View File

@@ -148,14 +148,16 @@ func TestNewErrorPaths(t *testing.T) {
enginebuiltin.Register("seichannel-create-fails", func(context.Context, enginebuiltin.Config) (engine.Session, error) { enginebuiltin.Register("seichannel-create-fails", func(context.Context, enginebuiltin.Config) (engine.Session, error) {
return nil, errBoom return nil, errBoom
}) })
if _, err := New(context.Background(), transport.Config{Carrier: "seichannel-create-fails"}); err == nil || err.Error() != "open engine session: boom" { //nolint:lll // long test description _, err := New(context.Background(), transport.Config{Carrier: "seichannel-create-fails"})
if err == nil || err.Error() != "open engine session: boom" {
t.Fatalf("New() error = %v", err) t.Fatalf("New() error = %v", err)
} }
enginebuiltin.Register("seichannel-no-video", func(context.Context, enginebuiltin.Config) (engine.Session, error) { enginebuiltin.Register("seichannel-no-video", func(context.Context, enginebuiltin.Config) (engine.Session, error) {
return &fakeEngineSession{stream: &fakeVideoStream{}, noVideo: true}, nil return &fakeEngineSession{stream: &fakeVideoStream{}, noVideo: true}, nil
}) })
if _, err := New(context.Background(), transport.Config{Carrier: "seichannel-no-video"}); !errors.Is(err, ErrVideoTrackUnsupported) { _, err = New(context.Background(), transport.Config{Carrier: "seichannel-no-video"})
if !errors.Is(err, ErrVideoTrackUnsupported) {
t.Fatalf("New() error = %v, want %v", err, ErrVideoTrackUnsupported) t.Fatalf("New() error = %v, want %v", err, ErrVideoTrackUnsupported)
} }
} }

View File

@@ -125,7 +125,9 @@ func New(ctx context.Context, cfg transport.Config) (transport.Transport, error)
// Stream/track IDs must be unique per peer: Jitsi/Jicofo keys participant // Stream/track IDs must be unique per peer: Jitsi/Jicofo keys participant
// sources by msid (stream-id+track-id) and rejects a session-accept whose // sources by msid (stream-id+track-id) and rejects a session-accept whose
// msid collides with one already in the conference. // msid collides with one already in the conference.
track, err := webrtc.NewTrackLocalStaticSample(codec.capability, "videochannel-"+common.RandomID(), "olcrtc-"+common.RandomID()) streamID := "videochannel-" + common.RandomID()
trackID := "olcrtc-" + common.RandomID()
track, err := webrtc.NewTrackLocalStaticSample(codec.capability, streamID, trackID)
if err != nil { if err != nil {
return nil, fmt.Errorf("create local video track: %w", err) return nil, fmt.Errorf("create local video track: %w", err)
} }
@@ -580,8 +582,8 @@ func (p *streamTransport) handleInboundFrame(frame transportFrame) {
p.onData(data) p.onData(data)
} }
p.sendAck(frame.seq, frame.crc) p.sendAck(frame.seq, frame.crc)
default: case common.ResultPartial, common.ResultIgnore:
// Partial or Ignore: do nothing. // fragment stored or discarded; no peer response needed yet.
} }
} }

View File

@@ -129,17 +129,22 @@ func TestNewCallbacksFeaturesAndClose(t *testing.T) {
} }
func TestNewErrorPaths(t *testing.T) { func TestNewErrorPaths(t *testing.T) {
enginebuiltin.Register("videochannel-create-fails", func(context.Context, enginebuiltin.Config) (engine.Session, error) { enginebuiltin.Register(
return nil, errVideoUnitBoom "videochannel-create-fails",
}) func(context.Context, enginebuiltin.Config) (engine.Session, error) {
if _, err := New(context.Background(), transport.Config{Carrier: "videochannel-create-fails"}); err == nil || err.Error() != "open engine session: boom" { //nolint:lll // long test description return nil, errVideoUnitBoom
},
)
_, err := New(context.Background(), transport.Config{Carrier: "videochannel-create-fails"})
if err == nil || err.Error() != "open engine session: boom" {
t.Fatalf("New() error = %v", err) t.Fatalf("New() error = %v", err)
} }
enginebuiltin.Register("videochannel-no-video", func(context.Context, enginebuiltin.Config) (engine.Session, error) { enginebuiltin.Register("videochannel-no-video", func(context.Context, enginebuiltin.Config) (engine.Session, error) {
return &fakeEngineSession{stream: &fakeVideoStream{}, noVideo: true}, nil return &fakeEngineSession{stream: &fakeVideoStream{}, noVideo: true}, nil
}) })
if _, err := New(context.Background(), transport.Config{Carrier: "videochannel-no-video"}); !errors.Is(err, ErrVideoTrackUnsupported) { _, err = New(context.Background(), transport.Config{Carrier: "videochannel-no-video"})
if !errors.Is(err, ErrVideoTrackUnsupported) {
t.Fatalf("New() error = %v, want %v", err, ErrVideoTrackUnsupported) t.Fatalf("New() error = %v, want %v", err, ErrVideoTrackUnsupported)
} }
} }

View File

@@ -169,14 +169,16 @@ func TestNewErrorPaths(t *testing.T) {
enginebuiltin.Register("vp8channel-create-fails", func(context.Context, enginebuiltin.Config) (engine.Session, error) { enginebuiltin.Register("vp8channel-create-fails", func(context.Context, enginebuiltin.Config) (engine.Session, error) {
return nil, errVP8UnitBoom return nil, errVP8UnitBoom
}) })
if _, err := New(context.Background(), transport.Config{Carrier: "vp8channel-create-fails"}); err == nil || err.Error() != "open engine session: boom" { //nolint:lll // long test description _, err := New(context.Background(), transport.Config{Carrier: "vp8channel-create-fails"})
if err == nil || err.Error() != "open engine session: boom" {
t.Fatalf("New() error = %v", err) t.Fatalf("New() error = %v", err)
} }
enginebuiltin.Register("vp8channel-no-video", func(context.Context, enginebuiltin.Config) (engine.Session, error) { enginebuiltin.Register("vp8channel-no-video", func(context.Context, enginebuiltin.Config) (engine.Session, error) {
return &fakeEngineSession{stream: &fakeVideoStream{}, noVideo: true}, nil return &fakeEngineSession{stream: &fakeVideoStream{}, noVideo: true}, nil
}) })
if _, err := New(context.Background(), transport.Config{Carrier: "vp8channel-no-video"}); !errors.Is(err, ErrVideoTrackUnsupported) { _, err = New(context.Background(), transport.Config{Carrier: "vp8channel-no-video"})
if !errors.Is(err, ErrVideoTrackUnsupported) {
t.Fatalf("New() error = %v, want %v", err, ErrVideoTrackUnsupported) t.Fatalf("New() error = %v, want %v", err, ErrVideoTrackUnsupported)
} }
} }

View File

@@ -78,7 +78,6 @@ func TestProtectorAndLogging(t *testing.T) {
} }
} }
//nolint:cyclop // compact setter smoke test verifies several related defaults together
func TestDefaultsAndSetters(t *testing.T) { func TestDefaultsAndSetters(t *testing.T) {
resetMobileGlobals(t) resetMobileGlobals(t)