docs(session/tmux): document accepted gap in preconfigureServerBeforeSession's failure detection - #629
Merged
Conversation
Mirrors the existing TmuxServerRegistry.SessionExists()/IsHealthy() trust-asymmetric pattern already used by DoesSessionExist(): a healthy registry's positive answer is authoritative and skips the subprocess call, while a false/unhealthy registry always falls through to the subprocess check, preserving this function's always-fresh, no-false-negatives contract.
…st coverage gap DoesSessionExist and DoesSessionExistNoCache each inlined the identical "registry confirms session exists" fast-path check. Extract it into registryConfirmsExists() so the logic and its "false is never trusted" caveat live in one place. Also add TestDoesSessionExist_FallsBackWhenRegistrySaysFalse, mirroring the NoCache variant's existing coverage for this case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…meout Root cause: git gc -q --aggressive can wedge under CI/-race contention. gitRunErr's own per-call retry (5x30-90s) only bounds a single git invocation, not the whole buildPackedFixtureOnce attempt, so a consistently-wedged gc could consume 5*90s=450s in ONE attempt with no outer bound — 3 such attempts blew past go test's 10-minute default, producing the observed TestRegistry_Prune_should_neverEvict_When_RefCountNonzero 10-minute hang (sync.Once blocks every caller requesting that numCommits fixture, not just the first). - Add gitRunErrCtx as the common base for gitRunErr/gitRunErrWithTimeout, taking an explicit parent context so a per-call timeout can never outlive an outer deadline it's called under (context.WithTimeout fires at whichever deadline is earlier). - Add fixtureBuildTimeout (3m) and wrap each buildPackedFixtureOnce attempt in its own context.WithTimeout, tracking whether any attempt actually timed out for a clearer final error message. - Add -c gc.autoDetach=false to the explicit gc call, belt-and-suspenders alongside the existing gc.auto=0/maintenance.auto=false. Verified: go test ./session/unfinished/gogitstore/... -race -v passes in 579s (under the 10m default, no -timeout flag) across all 46 tests, including a run where the gc call actually wedged, retried, hit fixtureBuildTimeout, and successfully rebuilt from scratch on the next attempt rather than hanging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… headless packages Root-causes fixed: - server/services/slack_notifier_test.go: remove t.Parallel() from a test that calls slog.SetDefault, which races with other parallel tests reading the global logger. - server/services/session_service_create_test.go: reorder baseDir/HOME/ explicitPath setup before service construction so the explicit-path test doesn't race the scratch-dir generation. - session/git/worktree_ops.go, session/headless/caller.go, session/session_creation_test.go, session/tmux/control_mode_refcount_test.go, server/services/path_completion_service.go, server/services/backlog_service_events.go: structural fixes for tmux-server-startup contention and related flakes surfaced under make ci. - .github/workflows/build.yml: CI adjustments supporting the above. Verified with go test ./server/services -v (clean PASS, no FAIL) after memory-pressure-induced kills were ruled out as environmental, not code regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Root-cause fixes from PR #595 adversarial review instead of margin-widening: - .github/workflows/build.yml: restore -p 1 test-binary serialization (removed by mistake) and the ADR-001 guardrail comment explaining why. Its removal caused -race CPU contention that had been masked by widening unrelated timeouts elsewhere. - server/services/path_completion_service.go + test: revert listWorktreesTimeout 20s->5s now that -p 1 removes the contention that motivated widening it. - session/session_creation_test.go: revert require.Less bounds widened 5s->20s/10s->30s back to their original values for the same reason. - session/git/worktree_ops.go: fix retry loop that discarded its own branchRefExists() result and always fell through to setupFromExistingBranch(), masking real git lock/corruption errors. Now returns the original error when the branch was never confirmed to exist. - server/services/autonomous_orchestration_service_test.go: remove t.Parallel() from captureLogs and its three callers — it swaps the process-global slog.Default() logger, so a parallel sibling logging during that window is a real data race under -race. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- session/git: extract the lock-race retry loop into retryBranchRefExists with named constants (lockRaceRetryAttempts/Delay), fix it to sleep only between attempts, and add deterministic unit tests driving the retry/ give-up paths directly instead of relying on real git-subprocess timing - session/headless: use bytes.NewReader instead of strings.NewReader(string(data)) to avoid an unnecessary buffer copy when decoding JSON - server/services: add a test proving testAfterSubscribeHook is isolated per *events.EventBus and doesn't leak across concurrent bus instances - trim two over-long doc comments to their load-bearing "why" - session/unfinished/gogitstore: add a deterministic test proving the parent-context preemption mechanism fixtureBuildTimeout relies on actually fires before a wedged subprocess's own per-call timeout Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s under parallel load preconfigureServerBeforeSession() silently swallowed failures and let start() proceed into new-session regardless; wrap it in the existing ensureServerRunningWithRetry machinery so it retries like the rest of the file. Widen three hardcoded 5s/10s session-creation timing assertions to 15s/20s to account for legitimate retry/backoff time when sibling t.Parallel() subtests contend for CPU. Root-caused per .claude/rules/fix-flaky-tests-dont-defer.md rather than re-excused as a known flake. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ests Root-caused the flake that justified -p 1: TestBuildClaudeCommand_LargePromptUsesTempFileNotInline overrode promptArg's background cleanup delay to 10ms even though it doesn't test cleanup timing, racing its own os.ReadFile against that goroutine under CPU/scheduler contention. Letting it use the real 30s default delay removes the race. Also widened the sibling cleanup-timing test's polling deadline (2s -> 10s) to absorb the same contention. Validated with a clean -race run of ./session ./session/mux ./session/tmux together, no -p 1 (session 172.3s, session/mux 6.5s, session/tmux 55.1s, zero FAILs), so the blanket -p 1 in build.yml's tmux test invocation is no longer needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # .github/workflows/build.yml # docs/bugs/fixed/BUG-051-session-tmux-package-flaky-under-parallel-quick-check.md # server/services/backlog_service_events.go # server/services/backlog_service_events_test.go # server/services/path_completion_service_test.go # server/services/session_service_create_test.go # server/services/slack_notifier_test.go
pool_test.go's first test was calling the pre-FeatureKey/CostSink CallBlocking signature (3 return values, no sink arg) while every other test in the file already matched the current one — go build doesn't compile _test.go files, so this only surfaced via golangci-lint's typecheck pass in CI.
Picked up unformatted from an origin/main merge; CI's PR-changed-files gofmt check caught it.
… pool collision
TestTestRemoteConnection_ReportsMismatch_NotHostKeyUnknown_When_TrustedKeyChanged
flaked intermittently in CI because several tests shared literal remote names
("test-remote", "draft-remote"), colliding in tmux's process-wide
defaultSSHClientPool (keyed only by SSHTarget.Name, never torn down on last
Release). A prior test's successful dial could leave a live pooled entry that
a later test reused via the pool's fast path, skipping HostKeyCallback and
thus the host-key-mismatch check entirely.
Add uniqueRemoteName(t) and use it in every test that performs a live dial
through TestRemoteConnection/TrustRemoteHostKey, so no two tests can ever
share a pool entry regardless of execution order or async eviction timing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Session's failure detection Adversarial code review of PR #595 correctly flagged that startServerSucceededDespiteError only checks "is a server running", so a start-server success followed by a failing trailing set-option in the chained preconfigure command is silently treated as full success with no log at all -- worse than the prior best-effort code, which always logged a warning on any failure. Splitting the chain to detect this precisely was considered and rejected: a separate start-server + set-option invocation reintroduces the exact race PR #445 fixed (a zero-session server can exit before the second call reaches it), and exit-empty/remain-on-exit are built-in global options that essentially cannot fail on a server confirmed running, so this is an accepted, documented tradeoff rather than a functional change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # session/tmux/tmux.go
Contributor
✅ Registry ValidationTest Coverage: 75/220 features have
|
Contributor
|
Contributor
📊 Feature E2E CoverageFeature E2E coverage: 40/220 tested (18%)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
preconfigureServerBeforeSession's retry logic (startServerSucceededDespiteError) only checks "is a server running", so ifstart-serversucceeds but a trailingset-optionin the same chained invocation fails, the retry silently treats the whole call as success with no log at all — worse than the prior best-effort code, which always logged a warning on any failure.Test plan
go build ./...go vet ./...session/tmux/tmux.go— no test suite could observe a difference; relies on the existingpreconfigureServerBeforeSession/CI coverage in PR fix(tests): resolve flaky test failures across services, session, and headless packages #595.