## Summary `handleConn` keeps the context that holds a session's log data in one variable per connection ([`internal/sshd/sshd.go#L240`](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/16e5a730a9609ccdc2fdee955fd9c77961c69e68/internal/sshd/sshd.go#L240)), and the channel handler of every session assigns to it ([`#L261-262`](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/16e5a730a9609ccdc2fdee955fd9c77961c69e68/internal/sshd/sshd.go#L261-262)). `handleRequests` runs that handler in a goroutine of its own for each session channel ([`internal/sshd/connection.go#L207-224`](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/16e5a730a9609ccdc2fdee955fd9c77961c69e68/internal/sshd/connection.go#L207-224)), and up to `concurrent_sessions_limit` sessions, 10 by default ([`internal/config/config.go#L131`](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/16e5a730a9609ccdc2fdee955fd9c77961c69e68/internal/config/config.go#L131)), run at once on one connection. Two sessions that run at the same time write the variable with nothing ordering the writes. `handleConn` then reads the variable for the `access: finish` line ([`sshd.go#L267`](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/16e5a730a9609ccdc2fdee955fd9c77961c69e68/internal/sshd/sshd.go#L267)). Only the semaphore `handleRequests` waits on orders that read after the sessions' writes ([`connection.go#L235-237`](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/16e5a730a9609ccdc2fdee955fd9c77961c69e68/internal/sshd/connection.go#L235-237)), and the wait gives up after `EOFTimeout` (10 seconds), or at once when the server's context is cancelled at the end of a graceful shutdown ([`cmd/gitlab-sshd/main.go#L134`](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/16e5a730a9609ccdc2fdee955fd9c77961c69e68/cmd/gitlab-sshd/main.go#L134)). A session still running at that point writes the variable unordered with the read. A client runs several sessions on one connection when it multiplexes them, as OpenSSH does with `ControlMaster`. Git LFS turns that on by itself for its SSH transfer protocol (`lfs.ssh.automultiplex`, true by default except on Windows) and runs up to `lfs.concurrenttransfers` (8 by default) transfers at once, so with gitlab-shell's `lfs.pure_ssh_protocol` enabled an LFS push or pull is enough. The impact is low. In practice the line reports the session that ended last, which is what the code intends, but under the Go memory model a data race has no defined outcome. ## Steps to reproduce Save this test as `internal/sshd/concurrent_sessions_repro_test.go` on `main`. It opens ten sessions over one connection, holds each access check until all ten have asked for one, and sends them to a Gitaly address that does not exist, so each session ends right after its access check: ```go package sshd import ( "bytes" "context" "fmt" "log/slog" "net/http" "path" "sync" "sync/atomic" "testing" "time" "github.com/stretchr/testify/require" "golang.org/x/crypto/ssh" "gitlab.com/gitlab-org/gitlab-shell/v14/client/testserver" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/config" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/testhelper" ) func TestConcurrentSessionsRepro(t *testing.T) { const sessions = 10 // Discard the logs: a handler that writes them one at a time orders the // sessions, and hides the race in some runs. originalDefault := slog.Default() slog.SetDefault(slog.New(slog.DiscardHandler)) t.Cleanup(func() { slog.SetDefault(originalDefault) }) var arrived atomic.Int32 allArrived := make(chan struct{}) requests := []testserver.TestRequestHandler{ { Path: authorizedKeysAPIPath, Handler: func(w http.ResponseWriter, _ *http.Request) { fmt.Fprint(w, `{"id": 1000, "key": "key"}`) }, }, { // Hold every access check until all the sessions have asked for // one, then send them to a Gitaly address that does not exist. Path: "/api/v4/internal/allowed", Handler: func(w http.ResponseWriter, _ *http.Request) { if arrived.Add(1) == sessions { close(allArrived) } select { case <-allArrived: case <-time.After(5 * time.Second): } fmt.Fprint(w, `{"status": true, "gl_id": "user-1", "gl_username": "alex-doe", "gitaly": {"repository": {"storage_name": "default", "relative_path": "repo.git", "gl_repository": "project-1", "gl_project_path": "flightjs/Flight"}, "address": "unix:/nonexistent/gitaly.socket", "token": "token"}}`) }, }, } testRoot := testhelper.PrepareTestRootDir(t) cfg := &config.Config{GitlabURL: testserver.StartSocketHTTPServer(t, requests), RootDir: "/tmp", User: user} cfg.Server.Listen = "127.0.0.1:0" cfg.Server.ConcurrentSessionsLimit = sessions cfg.Server.HostKeyFiles = []string{path.Join(testRoot, "certs/valid/server.key")} cfg.GitalyClient.InitSidechannelRegistry(context.Background()) s, err := NewServer(cfg) require.NoError(t, err) go func() { _ = s.ListenAndServe(context.Background()) }() t.Cleanup(func() { _ = s.Shutdown() }) verifyStatus(t, s, StatusReady) client, err := ssh.Dial("tcp", s.Addr(), clientConfig(t, testRoot)) require.NoError(t, err) var wg sync.WaitGroup for range sessions { wg.Go(func() { session, sessionErr := client.NewSession() if sessionErr != nil { t.Error(sessionErr) return } defer session.Close() session.Stdin = &bytes.Buffer{} _ = session.Run("git-receive-pack flightjs/Flight") }) } wg.Wait() require.NoError(t, client.Close()) // Give handleConn time to log the "access: finish" line. time.Sleep(100 * time.Millisecond) } ``` Then run it under the race detector: ```shell go test -race -count=1 -run TestConcurrentSessionsRepro ./internal/sshd/ ``` ## What is the current bug behavior? The race detector reports the two writes. With Go 1.25.14 it did so in 20 runs out of 20, each in a process of its own: ```plaintext ================== WARNING: DATA RACE Write at 0x00c000416390 by goroutine 84: gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*Server).handleConn.func3() internal/sshd/sshd.go:262 +0x724 gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*connection).handleRequests.func1() internal/sshd/connection.go:224 +0x302 Previous write at 0x00c000416390 by goroutine 79: gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*Server).handleConn.func3() internal/sshd/sshd.go:262 +0x724 gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*connection).handleRequests.func1() internal/sshd/connection.go:224 +0x302 Goroutine 84 (running) created at: gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*connection).handleRequests() internal/sshd/connection.go:207 +0x6dc gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*connection).handle() internal/sshd/connection.go:150 +0x3b1 gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*Server).handleConn() internal/sshd/sshd.go:242 +0x607 gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*Server).serve.gowrap1() internal/sshd/sshd.go:173 +0x6b Goroutine 79 (finished) created at: gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*connection).handleRequests() internal/sshd/connection.go:207 +0x6dc gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*connection).handle() internal/sshd/connection.go:150 +0x3b1 gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*Server).handleConn() internal/sshd/sshd.go:242 +0x607 gitlab.com/gitlab-org/gitlab-shell/v14/internal/sshd.(*Server).serve.gowrap1() internal/sshd/sshd.go:173 +0x6b ================== --- FAIL: TestConcurrentSessionsRepro (0.15s) testing.go:1617: race detected during execution of test FAIL ``` ## What is the expected correct behavior? No race. The `access: finish` line goes on logging the user, the project and the written bytes of the session that ended last, as it does today. ## Possible fixes Have each session store its `command.LogData` in an `atomic.Value` when it ends, and read the line's data from there, the way `connOutcome.serverError` is already atomic because sessions write it from their own goroutines. A mutex around the variable would work as well. I have a merge request ready for it.