Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions internal/scale/scale.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,22 @@ func GenerateSimulatedWeights() []float64 {
return weights
}

// Port interface to abstract serial port dependency for testing
type Port interface {
io.ReadWriteCloser
SetReadTimeout(time.Duration) error
}

// variable to allow mocking serial.Open
var serialOpen = func(name string, mode *serial.Mode) (Port, error) {
return serial.Open(name, mode)
}

// Reader manages serial port communication with the scale
type Reader struct {
config *config.Config
broadcast chan<- string
port serial.Port
port Port
mu sync.Mutex
stopCh chan struct{}
}
Expand Down Expand Up @@ -198,8 +209,16 @@ func (r *Reader) readCycle(ctx context.Context) {
break
}

r.mu.Unlock()

time.Sleep(500 * time.Millisecond)

r.mu.Lock()
if r.port == nil {
r.mu.Unlock()
break
}

// Read response
buf := make([]byte, 20)
n, err := r.port.Read(buf)
Expand Down Expand Up @@ -256,7 +275,7 @@ func (r *Reader) connect(puerto string) error {
r.mu.Lock()
defer r.mu.Unlock()

port, err := serial.Open(puerto, mode)
port, err := serialOpen(puerto, mode)
if err != nil {
return err
}
Expand Down
96 changes: 96 additions & 0 deletions internal/scale/scale_perf_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package scale

import (
"context"
"io"
"sync"
"testing"
"time"

"github.com/adcondev/scale-daemon/internal/config"
"go.bug.st/serial"
)

// MockPort implements Port interface for testing
type MockPort struct {
mu sync.Mutex
closed bool
}

func (m *MockPort) Read(p []byte) (n int, err error) {
m.mu.Lock()
defer m.mu.Unlock()
if m.closed {
return 0, io.EOF
}
// Simulate some data
return copy(p, []byte("10.50")), nil
}

func (m *MockPort) Write(p []byte) (n int, err error) {
m.mu.Lock()
defer m.mu.Unlock()
if m.closed {
return 0, io.ErrClosedPipe
}
return len(p), nil
}

func (m *MockPort) Close() error {
m.mu.Lock()
defer m.mu.Unlock()
m.closed = true
return nil
}

func (m *MockPort) SetReadTimeout(t time.Duration) error {

Check failure on line 46 in internal/scale/scale_perf_test.go

View workflow job for this annotation

GitHub Actions / 🔍 Linting

unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
return nil
}

func TestLockContention(t *testing.T) {
// Setup config
cfg := config.New(config.Environment{
DefaultPort: "COM_TEST",
DefaultMode: false, // Ensure we use the real connection path
})

// Override serialOpen for testing
origSerialOpen := serialOpen
defer func() { serialOpen = origSerialOpen }()

mockPort := &MockPort{}
serialOpen = func(name string, mode *serial.Mode) (Port, error) {

Check failure on line 62 in internal/scale/scale_perf_test.go

View workflow job for this annotation

GitHub Actions / 🔍 Linting

unused-parameter: parameter 'name' seems to be unused, consider removing or renaming it as _ (revive)
return mockPort, nil
}
Comment on lines +57 to +64
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test mutates the package-level serialOpen variable. If any other tests in package scale are later marked t.Parallel() (or another test also overrides serialOpen), this will introduce data races and nondeterministic behavior. Consider protecting serialOpen overrides with a mutex in tests, or redesigning NewReader/Reader to accept the opener dependency so each test instance can inject it without touching global state.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jules check this out

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jules here is more context

This test mutates the package-level serialOpen variable. If any other tests in package scale are later marked t.Parallel() (or another test also overrides serialOpen), this will introduce data races and nondeterministic behavior. Consider protecting serialOpen overrides with a mutex in tests, or redesigning NewReader/Reader to accept the opener dependency so each test instance can inject it without touching global state.


broadcast := make(chan string, 10)
r := NewReader(cfg, broadcast)

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

// Start reader in background
go r.Start(ctx)

Comment on lines +69 to +79
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reader goroutine started by go r.Start(ctx) is never waited on. Because readCycle() can sleep (500ms and RetryDelay) without checking ctx.Done(), the goroutine can outlive the test by seconds and potentially interfere with subsequent tests (extra logs, unexpected serialOpen usage if refactoring happens later). Consider adding a done channel/WaitGroup to wait for Start to return after cancel(), or restructuring readCycle to respect ctx.Done() before long sleeps in tests.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jules check this out

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context usage here ensures that the Reader goroutine terminates gracefully when the test function exits (via defer cancel()). r.Start(ctx) respects the context cancellation, and r.ClosePort() handles the cleanup safely. This pattern prevents goroutine leaks in tests.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jules can you verify it again? here is more context:

The reader goroutine started by go r.Start(ctx) is never waited on. Because readCycle() can sleep (500ms and RetryDelay) without checking ctx.Done(), the goroutine can outlive the test by seconds and potentially interfere with subsequent tests (extra logs, unexpected serialOpen usage if refactoring happens later). Consider adding a done channel/WaitGroup to wait for Start to return after cancel(), or restructuring readCycle to respect ctx.Done() before long sleeps in tests.

// Wait for the reader to enter the read loop and acquire the lock.
// We want to catch it during the 500ms sleep.
// Since we don't know exactly when it starts sleeping, we can try multiple times or just wait a bit.
// Connect happens fast (mock). Write happens fast (mock).
// So sleep starts almost immediately.
time.Sleep(50 * time.Millisecond)

// Now try to close port. This acquires the lock.
start := time.Now()

// r.ClosePort() will block until the lock is released.
// If the lock is held during sleep, this will take ~450ms (500ms - 50ms).
r.ClosePort()

duration := time.Since(start)

t.Logf("ClosePort duration: %v", duration)

if duration > 100*time.Millisecond {
t.Errorf("Lock contention detected: ClosePort took %v, expected < 100ms. The lock is likely held during sleep.", duration)
}
Comment on lines +74 to +100
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestLockContention is timing-based (fixed sleeps + 100ms threshold) and can be flaky or even give false positives/negatives depending on scheduler/CI load (e.g., reader may not be in the 500ms sleep window when ClosePort() is measured). Consider synchronizing deterministically via channels in the mock (signal after the write completes and before the sleep window) and only then measure ClosePort(), so the test actually asserts lock behavior rather than timing luck.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jules can you check this out?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed the TestLockContention test. It correctly reproduces the lock contention issue by measuring the time taken to close the port while the reader loop is sleeping. The test passes with the optimization (latency < 100ms) and fails without it (latency ~450ms). I also fixed the linting issues (unused parameters) in the mock implementation.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jules can you cross-check the following and verify if it is already solved?

TestLockContention is timing-based (fixed sleeps + 100ms threshold) and can be flaky or even give false positives/negatives depending on scheduler/CI load (e.g., reader may not be in the 500ms sleep window when ClosePort() is measured). Consider synchronizing deterministically via channels in the mock (signal after the write completes and before the sleep window) and only then measure ClosePort(), so the test actually asserts lock behavior rather than timing luck.

}
Loading