Skip to content

feat: list command use parallel execution #308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
joshmedeski merged 1 commit into from
Nov 21, 2025
Merged

feat: list command use parallel execution #308

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 33 additions & 4 deletions lister/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package lister

import (
"github.com/joshmedeski/sesh/v2/model"
"slices"
"sync"

"github.com/joshmedeski/sesh/v2/model"
)

type (
Expand All @@ -19,6 +21,12 @@ type (
srcStrategy func(*RealLister) (model.SeshSessions, error)
)

type strategyResult struct {
source string
sessions model.SeshSessions
err error
}

var srcStrategies = map[string]srcStrategy{
"tmux": listTmux,
"config": listConfig,
Expand All @@ -33,11 +41,32 @@ func (l *RealLister) List(opts ListOptions) (model.SeshSessions, error) {
srcsOrderedIndex := srcs(opts)
srcsOrderedIndex = sortSources(srcsOrderedIndex, l.config.SortOrder)

resultsChan := make(chan strategyResult, len(srcsOrderedIndex))
var wg sync.WaitGroup

for _, src := range srcsOrderedIndex {
sessions, err := srcStrategies[src](l)
if err != nil {
return model.SeshSessions{}, err
wg.Add(1)
go func(s string) {
defer wg.Done()
sessions, err := srcStrategies[s](l)
resultsChan <- strategyResult{source: s, sessions: sessions, err: err}
}(src)
}
Comment on lines 47 to +54
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The concurrent execution of strategies may cause race conditions in tests that use testify/mock objects. The testify/mock library is not thread-safe by default, as it maintains internal state for tracking method calls and expectations. When multiple goroutines call methods on the same mock object concurrently (as they will with l.tmux, l.zoxide, l.home, and l.tmuxinator), this can lead to data races.

Consider one of the following solutions:

  1. Protect mock method calls with a mutex in the test mocks
  2. Use a thread-safe mocking library
  3. Document that tests should be run without the -race flag (not recommended)
  4. Ensure real implementations are thread-safe and add integration tests

The production code itself appears safe since the real implementations only read configuration or call external commands, but the test suite may exhibit race conditions.

Copilot uses AI. Check for mistakes.
Copy link
Owner

@joshmedeski joshmedeski Nov 21, 2025

Choose a reason for hiding this comment

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

Manually testing it worked well! But I'd like to make sure we acknowledge any potential issues with testing. Can you clarify this then I'm happy to merge this PR.

Copy link
Contributor Author

@zh1C zh1C Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @joshmedeski , thanks for catching that. I initially didn't fully consider the impact of concurrent execution on the testify/mock objects.

I want to share some findings:
Current test status:

  • The tests are currently running with the -race flag enabled and all tests pass without any race condition warnings.
  • This suggests that the concurrent execution is working correctly with the current mockery-generated mock_Home.go.

About testify/mock thread safety:
I'm not deeply experienced with mockery, so I wasn't initially certain about its thread-safety guarantees. However, after checking the source code, I found that testify/mock's Mock struct does include a sync.Mutex field.
I looked into the testify repository and found several related issues about concurrency (#1340, #1020, #1128). After reviewing them and our code, I believe our test doesn't have these issues:

  • Issue #1340 warns against calling assert/require from multiple goroutines. In our test, all assertions are called only in the main test goroutine after List() completes.
  • Issue #1020 is about mutable arguments affecting mock recordings. We only use immutable string parameters.
  • Issue #1128 discusses unsafe direct access to mock.Calls field. We only use the safe methods like AssertExpectations(), which are protected by testify's internal mutex.

That said:
If you have evidence or experience showing that testify/mock is not thread-safe in concurrent scenarios, or if you've seen race conditions that the race detector might have missed, I'd be happy to implement a thread-safe FakeHome wrapper:

type FakeHome struct {
    mu          sync.Mutex
    shortenHome map[string]string
    expandHome  map[string]string
}

func (h *FakeHome) ShortenHome(path string) (string, error) {
    h.mu.Lock()
    defer h.mu.Unlock()
    if shortened, ok := h.shortenHome[path]; ok {
        return shortened, nil
    }
    return path, nil
}

// ... similar for ExpandHome

This would provide explicit concurrency protection and remove any dependency on the framework's internal implementation.

Let me know what you think – I'm open to either approach!

Copy link
Owner

@joshmedeski joshmedeski Nov 21, 2025

Choose a reason for hiding this comment

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

I don't that kind of work is necessary right now, I think I'm okay with merging as-is. But thanks for the research and suggestions, we can revisit this if it starts becoming an issue.


wg.Wait()
close(resultsChan)

// Collect results into a map for easy lookup
resultsMap := make(map[string]model.SeshSessions)
for res := range resultsChan {
if res.err != nil {
return model.SeshSessions{}, res.err
}
resultsMap[res.source] = res.sessions
}

for _, src := range srcsOrderedIndex {
sessions := resultsMap[src]
fullOrderedIndex = append(fullOrderedIndex, sessions.OrderedIndex...)
for _, i := range sessions.OrderedIndex {
fullDirectory[i] = sessions.Directory[i]
Expand Down
Loading