test(cli): harden output capture and signal failure diagnostics

[cristim]

Closes #2150

Summary

This PR fixes two test-harness reliability gaps. It changes test files only, with no production code changes.

1. captureAppOutput deadlock (cmd/multi_service_stats_test.go)

   • Before: the helper ran fn synchronously and read the pipe only afterwards. Any output larger than the pipe buffer blocked the writer forever.
   • Now: it drains the pipe in a goroutine while fn runs. A defer restores stdout and the logger, closes the writer, waits for the drain, and then closes the reader, so cleanup also happens when fn panics.
   • New regression test TestCaptureAppOutputDrainsLargeOutput writes 1 MiB through AppLogger. It fails after 10s instead of hanging, and it restores os.Stdout and AppLogger in t.Cleanup so a failure does not stall the rest of the package.
   • Reuse: captureRealStd uses the same drain pattern but swaps stdout and stderr, not the logger. captureAppLog uses no pipe. So a separate helper is justified.

2. GCP signal marker timeouts (cmd/configure_gcp_signal_test.go)

   • A requireMarker helper replaces require.Eventually at both marker waits. The timeout stays at 10s.
   • On timeout it reports the child process state and the child's stderr. Stderr now goes through a mutex-guarded buffer, so reading it mid-run does not race.
   • Child stdout is still never read, which preserves the deliberately blocked-stdout scenario.

Evidence

All runs are local on macOS with GOTOOLCHAIN=go1.26.9.

• Fail before / pass after:
  • With the old synchronous helper, TestCaptureAppOutputDrainsLargeOutput fails at 10.0s and the package still finishes in about 12.5s. An independent reviewer reproduced this in a scratch copy.
  • With the new helper, the test passes in about 3s.
• Sample diagnostic from a temporary wrong marker, since reverted:

  configure_gcp_signal_test.go:67: timed out after 10s waiting for creating marker
      child state: still running (pid 78038)
      child stderr: ""
  

• Checks:
  • make build, go vet ./..., go test -race ./cmd/...: pass.
  • Signal tests with -count=5: pass.
  • golangci-lint v2.10.1 (the CI pin): 0 issues.
  • scripts/test-git-secrets-allowlist.sh: 44/44.
• Review: two passes by an independent non-author reviewer (sonnet, high).
  • Pass 1 found one medium issue: a failing regression test hung the package. It is fixed.
  • Pass 2 found no actionable findings at a41104d.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 46 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 64 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 11c682c7-0c1f-4b15-9ef4-a2a60e74f65e

📥 Commits

Reviewing files that changed from the base of the PR and between a2c4c7e and a41104d.

📒 Files selected for processing: 2

• cmd/configure_gcp_signal_test.go
• cmd/multi_service_stats_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review verdict: CLEAN at a41104d

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Non-blocking notes:

• cmd/configure_gcp_signal_test.go (childState): on the timeout path, syscall.Wait4(WNOHANG) reaps the child, so the later cleanup Process.Kill targets an already-reaped pid. This runs only after t.Fatalf, so it cannot hang.
• cmd/multi_service_stats_test.go (captureAppOutput): on success, w is closed twice and the already-closed drained channel is waited on again in the defer. Both are harmless.

What was attacked and how (local macOS, GOTOOLCHAIN=go1.26.9, test-only change):

• Callers: all 27 callers of captureAppOutput (multi_service_stats, purchase_confirmation, helpers_count_override_rescale, multi_service_max_instances, multi_service_test) keep the same signature and pass.
• Defer ordering: the defer restores stdout and the logger, closes w, waits for the drain, then closes r. It also runs on panic and on t.FailNow/Goexit inside fn. The drain goroutine always sees EOF, so there is no leak or hang.
• go test -race -count=1 ./cmd/...: 1464 passed, 0 failed. go vet ./cmd/... was clean.
• go test -race -count=5 -run 'Signal|CaptureAppOutput|Interrupt' -v ./cmd/: every selected test passed 5 of 5 runs, with no SKIP.
• Fail-before: with the old synchronous helper restored in a scratch worktree, TestCaptureAppOutputDrainsLargeOutput fails at 10.00s with "captureAppOutput deadlocked on output larger than the pipe buffer". The package finished in 12s, so its cleanup prevents a package hang.
• Blocked-stdout scenario: this is preserved. requireMarker never reads child stdout, and the poll is still 10s / 10ms.
• Mutation probes, non-blocking: removing the syncBuffer mutex survives under -race. That is expected, because stderr is read concurrently only on the timeout path.

Gate:

• CI is green at this SHA.
• Mergeability is CLEAN.
• Labels are mirrored from test(cli): harden output capture and signal failure diagnostics #2150.
• CR waived: quota, adversarial review + local verification + green CI. CodeRabbit hit its review limit on this PR and was not paid for; this needs a retrospective review once quota is back.