Skip to content

Commit b7e9ba9

Browse files
committed
fix: address Copilot review feedback on #937
Two comments from Copilot on PR #937, both addressed: 1. safeTick now includes the recovered panic value's type and a runtime/debug.Stack() dump. Since safeTick intentionally swallows all panics — including latent invariant bugs — the log must carry enough context to diagnose the bug from the log alone; repeated occurrences surface the problem via the stack, not just the message. 2. cr.tick() now ends its trace cycle via defer with a completion sentinel (matching the startup block). Previously, a panic inside tick() would be recovered by safeTick but the trace cycle would never call end(), orphaning the record. Now the trace closes as Aborted on panic or Completed on normal exit. Unit test updated to assert the type and stack trace appear in the log.
1 parent cd16719 commit b7e9ba9

2 files changed

Lines changed: 23 additions & 5 deletions

File tree

cmd/gc/city_runtime.go

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"io"
77
"os"
88
"path/filepath"
9+
"runtime/debug"
910
"strings"
1011
"sync"
1112
"sync/atomic"
@@ -446,8 +447,12 @@ func (cr *CityRuntime) run(ctx context.Context) {
446447
func (cr *CityRuntime) safeTick(fn func(), trigger string) {
447448
defer func() {
448449
if r := recover(); r != nil {
449-
fmt.Fprintf(cr.stderr, "%s: reconciler tick panicked (trigger=%s): %v\n", //nolint:errcheck // best-effort stderr
450-
cr.logPrefix, trigger, r)
450+
// Include the recovered type and a stack trace so a latent
451+
// invariant bug (e.g. nil deref) is diagnosable from the log
452+
// alone — crucial because safeTick intentionally swallows
453+
// the panic and the bug may only surface via repetition.
454+
fmt.Fprintf(cr.stderr, "%s: reconciler tick panicked (trigger=%s): %v (type=%T)\n%s\n", //nolint:errcheck // best-effort stderr
455+
cr.logPrefix, trigger, r, r, debug.Stack())
451456
}
452457
}()
453458
fn()
@@ -466,6 +471,15 @@ func (cr *CityRuntime) tick(
466471
) {
467472
sessionBeads := cr.loadSessionBeadSnapshot()
468473
trace := cr.beginTraceCycle(trigger, "controller_tick", sessionBeads)
474+
// End the trace via defer so a panic recovered by safeTick still
475+
// closes the cycle (aborted). completion flips to Completed at the
476+
// normal end of the tick body below.
477+
completion := TraceCompletionAborted
478+
defer func() {
479+
if trace != nil {
480+
trace.end(completion, traceRecordPayload{"phase": "tick", "trigger": trigger})
481+
}
482+
}()
469483
// Detect pool instance deaths since last tick.
470484
if len(cr.poolDeathHandlers) > 0 {
471485
currentRunning, _ := cr.sp.ListRunning("")
@@ -551,9 +565,7 @@ func (cr *CityRuntime) tick(
551565

552566
// Convergence tick: process active convergence loops.
553567
cr.convergenceTick(ctx)
554-
if trace != nil {
555-
trace.end(TraceCompletionCompleted, traceRecordPayload{"phase": "tick", "trigger": trigger})
556-
}
568+
completion = TraceCompletionCompleted
557569
}
558570

559571
// reloadConfig attempts to reload city.toml and update all internal

cmd/gc/city_runtime_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1404,6 +1404,12 @@ func TestCityRuntimeSafeTick_RecoversFromPanicAndLogsTrigger(t *testing.T) {
14041404
if !strings.Contains(got, "simulated dolt eof cascade") {
14051405
t.Errorf("stderr = %q, want to contain panic payload", got)
14061406
}
1407+
if !strings.Contains(got, "type=string") {
1408+
t.Errorf("stderr = %q, want to contain panic value type (helps distinguish errors from strings)", got)
1409+
}
1410+
if !strings.Contains(got, "goroutine ") {
1411+
t.Errorf("stderr = %q, want to contain a stack trace so latent bugs stay diagnosable", got)
1412+
}
14071413
}
14081414

14091415
// safeTick must forward normal (non-panicking) returns unchanged so the

0 commit comments

Comments
 (0)