fix: always capture service logs during startup and show on failure#202
fix: always capture service logs during startup and show on failure#202sohil-kshirsagar merged 10 commits intomainfrom
Conversation
There was a problem hiding this comment.
4 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/runner/environment_replay.go">
<violation number="1" location="internal/runner/environment_replay.go:54">
P1: Startup failure log lines are not shown in non-debug print mode because they are sent only to `ServiceLog`.</violation>
</file>
<file name="internal/tui/test_executor.go">
<violation number="1" location="internal/tui/test_executor.go:297">
P2: The stderr pipe reader assumes each `Read` returns whole lines, so log lines split across chunks are broken into incorrect entries. Buffer incomplete trailing data between reads before splitting on `\n`.</violation>
</file>
<file name="internal/runner/service.go">
<violation number="1" location="internal/runner/service.go:177">
P2: The goroutine captures `e.processExitCh` by reference through `e`. During sandbox retry, if the old process hasn't fully exited when the new `StartService()` runs and creates a new channel (`e.processExitCh = make(chan error, 1)`), the old goroutine's deferred send will target the **new** channel, causing a spurious early-exit detection for the new service. Capture the channel in a local variable before launching the goroutine to eliminate this race.</violation>
</file>
<file name="internal/runner/environment.go">
<violation number="1" location="internal/runner/environment.go:29">
P2: `savedLogFile` will be `nil` when the first `StartService()` fails at readiness, because `StartService()` internally calls `StopService()` (which nils `e.serviceLogFile`) before returning the error. The subsequent `e.serviceLogFile = nil` and restore are no-ops. Unlike the in-memory buffer path (which has a `e.startupLogBuffer == nil` guard to preserve across retries), the file path has no equivalent guard — `setupServiceLogging` unconditionally creates a new log file, losing the first attempt's logs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Generated 25 tests - 25 passedTip New to Tusk Unit Tests? Learn more here. You are using "Only surface failing tests" mode, so Tusk will only surface failing tests in the PR comment but you can still see any passing tests in the Tusk UI. Considered 25 test scenarios and 11 symbols in 4 files but found no failing tests. Avg +47% line coverage gain across 4 files
Coverage is calculated by running tests (including passing) directly associated with each source file, learn more here. View check history
Was Tusk helpful? Give feedback by reacting with 👍 or 👎 |
There was a problem hiding this comment.
4 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/runner/service_test_helpers_windows.go">
<violation number="1" location="internal/runner/service_test_helpers_windows.go:15">
P2: The ping-based sleep uses `duration` instead of `duration+1`, so this helper under-sleeps by about one second and can make timing-dependent tests flaky.</violation>
</file>
<file name="internal/runner/environment.go">
<violation number="1" location="internal/runner/environment.go:29">
P2: The sandbox-retry path does not actually preserve file-based startup logs: `StartService()` recreates `serviceLogFile` on retry, so first-attempt logs/separator can be lost instead of accumulated.</violation>
</file>
<file name="internal/tui/test_executor.go">
<violation number="1" location="internal/tui/test_executor.go:301">
P2: Pipe reads are chunk-based, so splitting each read directly by `\n` can emit partial log lines. Preserve incomplete trailing data between reads (or use a line scanner) before appending to service logs.</violation>
</file>
<file name="internal/runner/service.go">
<violation number="1" location="internal/runner/service.go:178">
P1: Capture the started command in a local variable before launching the monitor goroutine; using `e.serviceCmd` inside the closure can race with retries and wait on the wrong process.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.



Summary
Service logs are now always captured during startup and automatically shown when the service fails to start. Previously, users had no visibility into startup failures unless they passed
--enable-service-logs. This was especially painful in CI where iteration cycles are slow.Also adds early process exit detection, print mode duration logging, TUI debug log cleanup, and fixes a pre-existing crash recovery bug.
Changes
Always capture service logs during startup
--enable-service-logs)io.Discard) to avoid unbounded memory growthsyncBufferwith mutex) since both stdout and stderr write concurrently⚠️ Retrying without sandbox...separatorEarly process exit detection
serviceCmd.Start(), a goroutine monitorsserviceCmd.Wait()viaprocessExitChwaitForReadiness()usesselectonprocessExitChvstime.After— fails immediately instead of waiting for the full readiness timeout"service process exited during startup: exit status 1"Print mode duration logging
➤ Loaded N tests (Xs)— test loading time↳ Loaded N suite spans (...) (Xs)— span preparation time✓ Environment ready (Xs)— server + service + SDK ack timeTotal elapsed: Xs— overall timeTUI improvements
tuiSlogHandler): Formats debug slog output as cleanmessage key=valueinstead of rawtime=... level=DEBUG msg=...redirectStderrToTUI): Captures fence library's directfmt.Fprintf(os.Stderr, ...)calls and routes them to the service log panel, preventing alt screen corruptionlog.Info(always shown with full stack trace in service logs) tolog.TestDebug(debug-only, routed to test panel)hasMoreTestsdidn't account for tests queued for retry. Fixed by checkinglen(m.testsToRetry) > 0.Print mode fallback for TestLog/ServiceLog
handleLogMessageroutes throughslog.Debuginstead of silently dropping--debugprint mode now shows test-level log messages alongside other debug outputNew
log.TestDebugfunctionTestLog— only emits when--debugis enabledHelp message simplified
--enable-service-logs" tip (logs are now always captured)--enable-service-logsis activeSetup agent prompt updates
debug: trueto see themdebug: trueguidance preserved for runtime/replay failureslogLevel: "debug"guidance added for SDK-level diagnosticsTUI testing doc
Future work
--debugTUI mode,log.Debug()calls withtraceIDattrs still route to service logs instead of the corresponding test panel. Fix: convert ~20 callsites incomparison.go,mock_matcher.go,dynamic_fields.go,server.gofromlog.Debug("...", "traceID", id)tolog.TestDebug(id, "...")--debugis used, propagate debug log level to SDKs via env var so users don't need to manually setlogLevel: "debug"