crueber/walhub

Watch

Fork 0

TestMaxSessionsRefuses hangs for the full 10-minute test timeout in CI (self-blocking transport + test-ordering race) #409

New issue

Closed

opened 2026-09-12 20:19:17 +00:00 by crueber · 3 comments

crueber commented

2026-09-12 20:19:17 +00:00

Owner

TestMaxSessionsRefuses hangs for the full 10-minute test timeout in CI (self-blocking transport + test-ordering race)

What's requested

Fix TestMaxSessionsRefuses in internal/sshd/coverage_test.go so it cannot deadlock: today the test's own transport can capture the single session slot it meant to prove is refused, blocking the test until Go's 10-minute package timeout panics the CI job.

Evidence

GitHub CI run 34714217427, job "test", step "vet + fast Go tests" (go test -short -count=1 ./...), failed after the full 10-minute per-package timeout. The goroutine dump shows a three-way stalemate:

Client test goroutine blocked in ssh.Session.Run at coverage_test.go:179 — sess2.Run(...), waiting for an exec reply that never terminates.
Server session goroutine inside the capture transport at coverage_test.go:48 — SSHUploadPack parked on <-c.block.
ListenAndServe accept loop alive and idle (sshd.go:160/:203) — nothing is wedged at the server level; the deadlock is between the test binary and its own fake transport.

Mechanism (static reading)

The test assumes an ordering the scheduler does not guarantee (coverage_test.go:164-183):

sess1.Run is launched in a bare goroutine at :170 and the test immediately proceeds to sess2.Run at :179. There is no synchronization proving sess1's exec reached the server first — captureTransport.entered exists for exactly this (closed in SSHUploadPack at :45-46) but this test never sets it.
Server-side, whichever exec reaches the semaphore first wins it. The limiter in exec (sshd.go:327-334) is a non-blocking select/default, so it always refuses someone immediately — the refusal logic itself is correct and fires before any transport work. But which session gets refused is a race.
In the intended ordering (sess1 wins the slot), sess1 parks at coverage_test.go:48, sess2's exec takes the default branch, writes "too many concurrent sessions", exits 1, and Run returns — the test passes and close(tr.block) at :183 releases sess1.
In the inverted ordering (the CI hang), sess2's exec reaches the semaphore first and wins the only slot. sess2's SSHUploadPack blocks on <-c.block — a channel that is only closed at :183, i.e. after sess2.Run returns. sess2.Run returns only when its transport returns. Self-deadlock: the test goroutine waits on Run (:179), Run waits on the transport, the transport waits on the test goroutine. sess1's exec is then the one refused, but its Run runs in a goroutine whose error is discarded (_ = at :170) and its stderr is never captured, so the assertion at :180 can never see it either. Nothing breaks the cycle until the 10-minute go test timeout panics.

Secondary fragility: even in the intended ordering, the slot-holder (sess1) sits inside the transport with stdin/stdout wired to the raw SSH channel; the test relies on close(tr.block) rather than on session teardown/stdio close to end the upload, so any future change to transport or channel close semantics turns a slow close into the same hang.

Architecture notes

exec's session limiter is already correct for the property this test pins: the select { case s.sessions <- ...: default: refuse } at sshd.go:327-334 is non-blocking, fires before any transport dispatch, and never burns a slot on refusal. No production-code change is required for the cap itself.
The file already contains the right tool: captureTransport.entered (coverage_test.go:32, 44-46) and the releaseQueue per-exec hold pattern (:187-216, used by the sibling TestSessionCapIsPerConnection, which polls with a deadline instead of assuming ordering at :280-292).

Prescribed fix

All changes in internal/sshd/coverage_test.go:

Make the slot-holder deterministic: construct the transport with entered: make(chan struct{}) and wait <-tr.entered after launching sess1.Run and before calling sess2.Run. This guarantees sess1 owns the slot when sess2 is offered, so the refusal at :180 is asserted against the intended session — the race at step 1 disappears.
Terminate the held session so the transport can never self-deadlock: when the refusal assertion completes, tear the held session down rather than only closing tr.block — close tr.block AND sess1.Close()/cancel the test context so SSHUploadPack is released via channel close and the client-side session is closed, so the upload path sees EOF/channel-close regardless of ordering. (Optionally assert that the goroutine-launched sess1.Run returned.)
Give the test its own deadline: derive a short deadline for the whole test (e.g. check t.Deadline() and skip-or-budget, or run sess2.Run against a client config/timer that fails the test with a clear message after a few seconds) so a regression degrades to a fast, named failure instead of consuming the package's 10-minute timeout in CI.
Match the sibling pattern where useful: TestSessionCapIsPerConnection already demonstrates the poll-with-deadline idiom — reuse that shape rather than fixed sleeps.

Acceptance criteria

TestMaxSessionsRefuses no longer depends on goroutine scheduling: the slot-holder is synchronized via captureTransport.entered before the second exec is offered.
The held session is torn down (channel close + session close) so the blocked transport cannot wait on the test goroutine; no path exists where the transport blocks on a channel only the test's own continuation can close.
The test enforces its own short deadline; a future regression fails in seconds with a message naming the stuck wait, not a 10-minute package panic.
The limiter's production behavior is unchanged (select/default refusal before transport dispatch); no server-code changes are required by this fix.
go test -short -count=1 ./internal/sshd/ and go test -race -short -count=1 ./internal/sshd/ pass repeatedly (e.g. -count=10) without hangs.

# TestMaxSessionsRefuses hangs for the full 10-minute test timeout in CI (self-blocking transport + test-ordering race) ## What's requested Fix `TestMaxSessionsRefuses` in `internal/sshd/coverage_test.go` so it cannot deadlock: today the test's own transport can capture the single session slot it meant to prove is refused, blocking the test until Go's 10-minute package timeout panics the CI job. ## Evidence GitHub CI run 34714217427, job "test", step "vet + fast Go tests" (`go test -short -count=1 ./...`), failed after the full 10-minute per-package timeout. The goroutine dump shows a three-way stalemate: - Client test goroutine blocked in `ssh.Session.Run` at `coverage_test.go:179` — `sess2.Run(...)`, waiting for an exec reply that never terminates. - Server session goroutine inside the capture transport at `coverage_test.go:48` — `SSHUploadPack` parked on `<-c.block`. - `ListenAndServe` accept loop alive and idle (`sshd.go:160/:203`) — nothing is wedged at the server level; the deadlock is between the test binary and its own fake transport. ## Mechanism (static reading) The test assumes an ordering the scheduler does not guarantee (`coverage_test.go:164-183`): 1. `sess1.Run` is launched in a bare goroutine at `:170` and the test immediately proceeds to `sess2.Run` at `:179`. There is **no synchronization** proving sess1's exec reached the server first — `captureTransport.entered` exists for exactly this (closed in `SSHUploadPack` at `:45-46`) but this test never sets it. 2. Server-side, whichever exec reaches the semaphore first wins it. The limiter in `exec` (`sshd.go:327-334`) is a non-blocking `select`/`default`, so it always refuses *someone* immediately — the refusal logic itself is correct and fires before any transport work. But which session gets refused is a race. 3. In the intended ordering (sess1 wins the slot), sess1 parks at `coverage_test.go:48`, sess2's exec takes the `default` branch, writes "too many concurrent sessions", exits 1, and `Run` returns — the test passes and `close(tr.block)` at `:183` releases sess1. 4. In the inverted ordering (**the CI hang**), sess2's exec reaches the semaphore first and wins the only slot. sess2's `SSHUploadPack` blocks on `<-c.block` — a channel that is only closed at `:183`, i.e. *after* `sess2.Run` returns. `sess2.Run` returns only when its transport returns. Self-deadlock: the test goroutine waits on `Run` (`:179`), `Run` waits on the transport, the transport waits on the test goroutine. sess1's exec is then the one refused, but its `Run` runs in a goroutine whose error is discarded (`_ =` at `:170`) and its stderr is never captured, so the assertion at `:180` can never see it either. Nothing breaks the cycle until the 10-minute `go test` timeout panics. Secondary fragility: even in the intended ordering, the slot-holder (sess1) sits inside the transport with stdin/stdout wired to the raw SSH channel; the test relies on `close(tr.block)` rather than on session teardown/stdio close to end the upload, so any future change to transport or channel close semantics turns a slow close into the same hang. ## Architecture notes - `exec`'s session limiter is already correct for the property this test pins: the `select { case s.sessions <- ...: default: refuse }` at `sshd.go:327-334` is non-blocking, fires before any transport dispatch, and never burns a slot on refusal. **No production-code change is required for the cap itself.** - The file already contains the right tool: `captureTransport.entered` (`coverage_test.go:32, 44-46`) and the `releaseQueue` per-exec hold pattern (`:187-216`, used by the sibling `TestSessionCapIsPerConnection`, which polls with a deadline instead of assuming ordering at `:280-292`). ## Prescribed fix All changes in `internal/sshd/coverage_test.go`: 1. **Make the slot-holder deterministic**: construct the transport with `entered: make(chan struct{})` and wait `<-tr.entered` after launching `sess1.Run` and before calling `sess2.Run`. This guarantees sess1 owns the slot when sess2 is offered, so the refusal at `:180` is asserted against the intended session — the race at step 1 disappears. 2. **Terminate the held session so the transport can never self-deadlock**: when the refusal assertion completes, tear the held session down rather than only closing `tr.block` — close `tr.block` AND `sess1.Close()`/cancel the test context so `SSHUploadPack` is released via channel close *and* the client-side session is closed, so the upload path sees EOF/channel-close regardless of ordering. (Optionally assert that the goroutine-launched `sess1.Run` returned.) 3. **Give the test its own deadline**: derive a short deadline for the whole test (e.g. check `t.Deadline()` and skip-or-budget, or run `sess2.Run` against a client config/timer that fails the test with a clear message after a few seconds) so a regression degrades to a fast, named failure instead of consuming the package's 10-minute timeout in CI. 4. Match the sibling pattern where useful: `TestSessionCapIsPerConnection` already demonstrates the poll-with-deadline idiom — reuse that shape rather than fixed sleeps. ## Acceptance criteria - [ ] `TestMaxSessionsRefuses` no longer depends on goroutine scheduling: the slot-holder is synchronized via `captureTransport.entered` before the second exec is offered. - [ ] The held session is torn down (channel close + session close) so the blocked transport cannot wait on the test goroutine; no path exists where the transport blocks on a channel only the test's own continuation can close. - [ ] The test enforces its own short deadline; a future regression fails in seconds with a message naming the stuck wait, not a 10-minute package panic. - [ ] The limiter's production behavior is unchanged (`select`/`default` refusal before transport dispatch); no server-code changes are required by this fix. - [ ] `go test -short -count=1 ./internal/sshd/` and `go test -race -short -count=1 ./internal/sshd/` pass repeatedly (e.g. `-count=10`) without hangs.

crueber added the

bug

git storage

labels

2026-09-12 20:20:10 +00:00

crueber added this to the v1 milestone

2026-09-12 20:20:19 +00:00

crueber referenced this issue from a commit

2026-09-12 20:31:11 +00:00

Fix #409: TestMaxSessionsRefuses deadlock (deterministic slot-holder + teardown + own deadlines)

crueber referenced this issue from a pull request that will close it,

2026-09-12 20:31:15 +00:00

Fix #409: TestMaxSessionsRefuses deadlock #411

crueber commented

2026-09-12 20:31:20 +00:00

Author

Owner

Fixed by PR #411 (#411): deterministic slot-holder via entered, held-session teardown (block close + sess1.Close), 5s own-deadline budgets with named failures; test-only, limiter unchanged. All acceptance runs pass (short, race, -count=10, no hangs).

Fixed by PR #411 (https://git.packden.us/crueber/walhub/pulls/411): deterministic slot-holder via entered, held-session teardown (block close + sess1.Close), 5s own-deadline budgets with named failures; test-only, limiter unchanged. All acceptance runs pass (short, race, -count=10, no hangs).

crueber commented

2026-09-12 20:32:47 +00:00

Author

Owner

Review of PR #411 (413da51) — verified in scratch worktree, main untouched.

ACCEPTANCE CRITERIA (all 5 pass):

Deterministic slot-holder — PASS. entered wait (coverage_test.go:187-191) gates sess2 before it is offered, and the limiter acquire (sshd.go:327-334) strictly precedes transport dispatch (sshd.go:341), so observing entered-closed proves sess1 owns the slot. The CI inversion is structurally impossible now — no scheduling dependence.
Teardown — PASS. Explicit release() + sess1.Close() + join on sess1Done (218-226), plus Once-guarded Cleanup pair (177-180; LIFO order correct, comment accurate). Buffered done-channels (1-cap) mean no goroutine can block on test continuation in any ordering; hypothetic inversion would hit the 5s named failure, never the 10-min hang.
Own deadline — PASS. All three waits budgeted 5s with named stuck-wait messages (entered / limiter refusal / transport block); worst case ~15s, no path consumes the package timeout.
No production change — PASS. git diff main...branch --name-only under internal/sshd shows coverage_test.go only; sshd.go limiter untouched (verified select/default still fires pre-dispatch).
Sibling idiom / no sleeps — PASS. Deadline-bounded channel waits only; no time.Sleep added. Coverage unaffected (test-only; new arms are defensive).

STRESS RESULTS (scratch worktree @413da51):

-race -short -run TestMaxSessionsRefuses -count=20: PASS (each ~0.03-0.04s)
GOMAXPROCS=1 -race -short -count=10: PASS
GOMAXPROCS=2 -short -count=50: PASS
full package -short -count=1: PASS; -race -short -count=1: PASS
gofmt -l clean; go vet clean.
docs/go/15_testing.md note accurate (law-12 rule restatement, test-only framing correct).

NITS (non-blocking, left unpushed to avoid churn):

Comment at :183-186 says poll-with-deadline but the code is a single select-with-timeout — the correct shape for one close-event (sibling polls for N execs). Wording only.
close(c.entered) (coverage_test.go:44-46) is unguarded: a future limiter regression letting two execs reach the transport would panic (double close) instead of hitting the named 5s failure. Still fail-fast (panic, not hang), so optional hardening only.

MERGE RECOMMENDATION: ready to merge.

Review of PR #411 (413da51) — verified in scratch worktree, main untouched. ACCEPTANCE CRITERIA (all 5 pass): 1. Deterministic slot-holder — PASS. entered wait (coverage_test.go:187-191) gates sess2 before it is offered, and the limiter acquire (sshd.go:327-334) strictly precedes transport dispatch (sshd.go:341), so observing entered-closed proves sess1 owns the slot. The CI inversion is structurally impossible now — no scheduling dependence. 2. Teardown — PASS. Explicit release() + sess1.Close() + join on sess1Done (218-226), plus Once-guarded Cleanup pair (177-180; LIFO order correct, comment accurate). Buffered done-channels (1-cap) mean no goroutine can block on test continuation in any ordering; hypothetic inversion would hit the 5s named failure, never the 10-min hang. 3. Own deadline — PASS. All three waits budgeted 5s with named stuck-wait messages (entered / limiter refusal / transport block); worst case ~15s, no path consumes the package timeout. 4. No production change — PASS. git diff main...branch --name-only under internal/sshd shows coverage_test.go only; sshd.go limiter untouched (verified select/default still fires pre-dispatch). 5. Sibling idiom / no sleeps — PASS. Deadline-bounded channel waits only; no time.Sleep added. Coverage unaffected (test-only; new arms are defensive). STRESS RESULTS (scratch worktree @413da51): - -race -short -run TestMaxSessionsRefuses -count=20: PASS (each ~0.03-0.04s) - GOMAXPROCS=1 -race -short -count=10: PASS - GOMAXPROCS=2 -short -count=50: PASS - full package -short -count=1: PASS; -race -short -count=1: PASS - gofmt -l clean; go vet clean. - docs/go/15_testing.md note accurate (law-12 rule restatement, test-only framing correct). NITS (non-blocking, left unpushed to avoid churn): - Comment at :183-186 says poll-with-deadline but the code is a single select-with-timeout — the correct shape for one close-event (sibling polls for N execs). Wording only. - close(c.entered) (coverage_test.go:44-46) is unguarded: a future limiter regression letting two execs reach the transport would panic (double close) instead of hitting the named 5s failure. Still fail-fast (panic, not hang), so optional hardening only. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-12 20:33:02 +00:00

crueber commented

2026-09-12 20:33:02 +00:00

Author

Owner

Fixed by PR #411 (review clean — deterministic slot-holder proven, teardown + deadlines verified, stress incl. GOMAXPROCS runs green), merged. Closing.

crueber referenced this issue from a commit

2026-09-12 20:33:02 +00:00

Merge pull request 'Fix #409: TestMaxSessionsRefuses deadlock' (#411) from fix/issue-409 into main