diff --git a/.changeset/recreate-settle-retry.md b/.changeset/recreate-settle-retry.md new file mode 100644 index 00000000..80f4e5fa --- /dev/null +++ b/.changeset/recreate-settle-retry.md @@ -0,0 +1,5 @@ +--- +"@kurrent/gaffer": patch +--- + +`gaffer recreate` (and the `deploy_recreate` MCP tool) no longer strands a projection when the server is slow to settle the delete. KurrentDB deletes projections asynchronously, so the rebuild's create could bounce off the still-registered name with a Conflict and leave the projection deleted but not recreated. The create now retries over a ten-second settle window before giving up with the recovery instructions. diff --git a/.changeset/runner-teardown-race.md b/.changeset/runner-teardown-race.md new file mode 100644 index 00000000..7a316297 --- /dev/null +++ b/.changeset/runner-teardown-race.md @@ -0,0 +1,5 @@ +--- +"@kurrent/gaffer": patch +--- + +Session teardown in the debug surfaces no longer races in-flight debug commands. Stopping an MCP run or ending/restarting a DAP debug session could free the native projection session while a step or resume from another goroutine was still executing inside it. That use-after-free could crash the process. The engine runner now refuses new session calls once teardown begins and waits out the in-flight ones before freeing the session. diff --git a/cli/cmd/dev.go b/cli/cmd/dev.go index 302113ca..5b30841d 100644 --- a/cli/cmd/dev.go +++ b/cli/cmd/dev.go @@ -637,7 +637,11 @@ func runDevDebug( select { case <-adapter.Ready(): case <-adapter.RestartRequested(): - session.Destroy() + // Destroy through the runner (here and on every in-loop teardown + // below): the adapter's handlers issue session calls on their own + // goroutines, and Runner.Destroy waits those out where a raw + // session.Destroy would free the session under them. + r.Destroy() session, info, err = engine.CreateSession(proj, true, includeShape) if err != nil { adapter.AckRestart() @@ -654,7 +658,7 @@ func runDevDebug( adapter.AckRestart() continue case <-ctx.Done(): - session.Destroy() + r.Destroy() return nil } @@ -689,7 +693,7 @@ func runDevDebug( if err != nil { innerCancel() close(iterDone) - session.Destroy() + r.Destroy() return err } @@ -722,7 +726,7 @@ func runDevDebug( // events, and a restart won't fix it - surface the signal and stop. if authErr := asAuthRequired(srcErr); authErr != nil { writer.WriteAuthRequired(authErr.Env) - session.Destroy() + r.Destroy() return silent(srcErr) } @@ -758,7 +762,7 @@ func runDevDebug( // projection_errors_seen. The teardown-path branch below // has its own r.Faulted() check; this one matches it. recordProjectionFault(r, obs.onProjectionError) - session.Destroy() + r.Destroy() session, info, err = engine.CreateSession(proj, true, includeShape) if err != nil { adapter.AckRestart() @@ -787,7 +791,7 @@ func runDevDebug( adapter.SendTerminated() if runErr := finalizeRun(ctx, caughtUp, srcErr, r, os.Stderr); runErr != nil { - session.Destroy() + r.Destroy() if code, ok := runErrorCode(runErr); ok { writer.WriteRunError(code, runErr.Error()) return silent(runErr) @@ -800,7 +804,7 @@ func runDevDebug( fmt.Fprintf(os.Stderr, "warning: reading projection state: %v\n", stateErr) } writer.WriteSummary(r.Stats(), summary) - session.Destroy() + r.Destroy() if r.Faulted() { lastErr := r.LastError() if lastErr != nil { diff --git a/cli/cmd/recreate.go b/cli/cmd/recreate.go index 869ff4eb..ab09efda 100644 --- a/cli/cmd/recreate.go +++ b/cli/cmd/recreate.go @@ -135,7 +135,8 @@ func runRecreate(cmd *cobra.Command, name string, opts recreateOpts) error { // recovery messages, live in internal/deploy (shared in shape with deploy's // rebuild); here we only bind each step to the client and its option mapping. if err := deploy.Recreate(ctx, name, deploy.RecreateSteps{ - Disable: func(ctx context.Context) error { return r.Disable(ctx, name) }, + RetryCreate: remote.IsCreateConflict, + Disable: func(ctx context.Context) error { return r.Disable(ctx, name) }, Delete: func(ctx context.Context) error { return r.Delete(ctx, name, remote.DeleteOptions{ DeleteStateStream: true, diff --git a/cli/cmd/status_integration_test.go b/cli/cmd/status_integration_test.go index 70988065..38023cb4 100644 --- a/cli/cmd/status_integration_test.go +++ b/cli/cmd/status_integration_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/kurrent-io/gaffer/cli/internal/cliout" "github.com/kurrent-io/gaffer/cli/internal/remote" @@ -133,7 +134,21 @@ func TestStatus_Integration(t *testing.T) { t.Fatal(err) } - report := runStatusReportJSON(t) + // The drift check degrades to "no drift" by design when the + // /info/options read misses its 3s budget (it's advisory - see + // nodeOptionsHTTPTimeout), and a race-instrumented CI runner under + // full parallel-package load can miss that window. Retry the whole + // command a few times so the assertion tests the envelope plumbing, + // not one 3-second slot on a loaded runner. + var report cliout.StatusReportJSON + for attempt := 1; ; attempt++ { + report = runStatusReportJSON(t) + if len(report.ConfigDrift) > 0 || attempt == 3 { + break + } + t.Logf("attempt %d: configDrift empty (advisory check likely timed out), retrying", attempt) + time.Sleep(2 * time.Second) + } if len(report.ConfigDrift) != 1 || report.ConfigDrift[0].Knob != "max_state_size" || report.ConfigDrift[0].Local != 1 { t.Fatalf("configDrift = %+v, want the max_state_size divergence", report.ConfigDrift) } diff --git a/cli/internal/dap/adapter_test.go b/cli/internal/dap/adapter_test.go index c652d6d3..250ac09b 100644 --- a/cli/internal/dap/adapter_test.go +++ b/cli/internal/dap/adapter_test.go @@ -55,6 +55,13 @@ func mustSetupDebugSessionUnbound(t *testing.T, source string) (*DebugAdapter, * OnBreak: adapter.HandleBreak, }, }) + // Tear down through the Runner, which waits out in-flight session calls - + // the adapter's handlers issue step verbs on their own goroutines, and a + // raw session.Destroy here raced them (caught by the race detector). The + // session.Destroy cleanup above stays as the failure-path backstop for a + // t.Fatal between session and runner creation; it no-ops once the runner + // has destroyed the session. Cleanups run LIFO, so the runner goes first. + t.Cleanup(runner.Destroy) handler := adapter.Handler() srv, err := NewServer("127.0.0.1:0", handler) @@ -335,6 +342,7 @@ func mustSetupDebugSessionWithHistory(t *testing.T) (*DebugAdapter, *engine.Runn if err != nil { t.Fatal(err) } + t.Cleanup(func() { _ = store.Close() }) adapter := NewDebugAdapter(session, "/tmp/test/projection.js", "/tmp/test") runner := engine.NewRunner(engine.RunnerConfig{ @@ -349,6 +357,9 @@ func mustSetupDebugSessionWithHistory(t *testing.T) (*DebugAdapter, *engine.Runn OnBreak: adapter.HandleBreak, }, }) + // See mustSetupDebugSessionUnbound: teardown goes through the Runner so + // in-flight step verbs from the adapter's goroutines are waited out. + t.Cleanup(runner.Destroy) adapter.SetRunner(runner) handler := adapter.Handler() diff --git a/cli/internal/deploy/apply.go b/cli/internal/deploy/apply.go index 84a04149..f8903b41 100644 --- a/cli/internal/deploy/apply.go +++ b/cli/internal/deploy/apply.go @@ -102,6 +102,13 @@ type RecreateSteps struct { Disable Step Delete Step Create Step + // RetryCreate, when set, marks a Create error as worth retrying over the + // settle budget. The server deletes asynchronously - the Delete RPC + // returns while the name can still be registered - so an immediate + // Create can bounce off the lingering registration (the projections + // subsystem replies Conflict; see remote.IsCreateConflict). Optional: + // nil means one attempt. + RetryCreate func(error) bool } func (s RecreateSteps) validate() error { @@ -112,11 +119,18 @@ func (s RecreateSteps) validate() error { ) } +// createSettleBudget bounds how long Recreate keeps retrying a Create that +// bounces off the deleted projection's lingering registration, on top of the +// per-attempt RPCTimeout. +const createSettleBudget = 10 * time.Second + // Recreate destroys a projection and rebuilds it from local config: stop it, // delete it (with its state and checkpoint streams), then create it fresh, // reprocessing from zero. The destroy precedes the create, so a failure after // Delete leaves the projection gone: each step names the recovery rather than a -// bare error. There's no auto-rollback. +// bare error. There's no auto-rollback. A Create refused while the server is +// still settling the delete retries over createSettleBudget (see +// RecreateSteps.RetryCreate) before giving up with the recovery message. func Recreate(ctx context.Context, name string, s RecreateSteps) error { if err := s.validate(); err != nil { return fmt.Errorf("recreate %s: %w", name, err) @@ -127,7 +141,19 @@ func Recreate(ctx context.Context, name string, s RecreateSteps) error { if err := bound(ctx, s.Delete); err != nil { return fmt.Errorf("could not delete %s before recreating: %w", name, err) } - if err := bound(ctx, s.Create); err != nil { + err := bound(ctx, s.Create) + if s.RetryCreate != nil { + deadline := time.Now().Add(createSettleBudget) + for delay := 250 * time.Millisecond; err != nil && s.RetryCreate(err) && time.Now().Before(deadline); delay = min(delay*2, 2*time.Second) { + select { + case <-ctx.Done(): + return fmt.Errorf("%s was deleted but recreating it failed - re-run gaffer recreate %s, or gaffer deploy %s: %w", name, name, name, ctx.Err()) + case <-time.After(delay): + } + err = bound(ctx, s.Create) + } + } + if err != nil { return fmt.Errorf("%s was deleted but recreating it failed - re-run gaffer recreate %s, or gaffer deploy %s: %w", name, name, name, err) } return nil diff --git a/cli/internal/deploy/apply_test.go b/cli/internal/deploy/apply_test.go index 5ffd1947..cae4dd3f 100644 --- a/cli/internal/deploy/apply_test.go +++ b/cli/internal/deploy/apply_test.go @@ -2,6 +2,7 @@ package deploy import ( "context" + "errors" "fmt" "strings" "testing" @@ -129,6 +130,65 @@ func TestRecreateCreateFailureNamesBothRecoveries(t *testing.T) { } } +// TestRecreateRetriesCreateWhileSettling pins the delete-settle retry: the +// server deletes asynchronously, so a create can bounce off the lingering +// registration (the envelope's Conflict reply) and must be retried rather +// than stranding the projection deleted-but-not-recreated. +func TestRecreateRetriesCreateWhileSettling(t *testing.T) { + r := &recorder{} + attempts := 0 + steps := r.recreateSteps() + create := steps.Create + steps.Create = func(ctx context.Context) error { + attempts++ + if attempts < 3 { + return errors.New("rpc error: code = Unknown desc = Envelope callback expected Updated, received Conflict instead") + } + return create(ctx) + } + steps.RetryCreate = func(err error) bool { return strings.Contains(err.Error(), "received Conflict") } + + if err := Recreate(context.Background(), "orders", steps); err != nil { + t.Fatalf("Recreate should have retried through the settle window: %v", err) + } + if attempts != 3 { + t.Errorf("create attempts = %d, want 3 (two conflicts, then success)", attempts) + } +} + +// A non-retryable create failure keeps the single-attempt behaviour even with +// the predicate set. +func TestRecreateDoesNotRetryOtherCreateFailures(t *testing.T) { + r := &recorder{failOn: "create"} + steps := r.recreateSteps() + steps.RetryCreate = func(err error) bool { return strings.Contains(err.Error(), "received Conflict") } + + if err := Recreate(context.Background(), "orders", steps); err == nil { + t.Fatal("expected the create failure to surface") + } + if got := strings.Join(r.calls, ","); got != "disable,delete,create" { + t.Errorf("calls = %q, want a single create attempt", got) + } +} + +// A cancelled context ends the retry loop with the recovery message instead +// of sleeping out the settle budget. +func TestRecreateRetryStopsOnCancel(t *testing.T) { + r := &recorder{} + ctx, cancel := context.WithCancel(context.Background()) + steps := r.recreateSteps() + steps.Create = func(context.Context) error { + cancel() + return errors.New("received Conflict") + } + steps.RetryCreate = func(err error) bool { return strings.Contains(err.Error(), "received Conflict") } + + err := Recreate(ctx, "orders", steps) + if err == nil || !strings.Contains(err.Error(), "orders was deleted but recreating it failed") { + t.Fatalf("got %v, want the deleted-but-not-recreated recovery", err) + } +} + func TestNilStepRejectedBeforeAnyCall(t *testing.T) { // A wiring gap (a nil step) must be refused before any step runs: these // sequences are destructive, so a nil caught mid-flight could strand a diff --git a/cli/internal/engine/runner.go b/cli/internal/engine/runner.go index e1b4af17..4559b46e 100644 --- a/cli/internal/engine/runner.go +++ b/cli/internal/engine/runner.go @@ -63,10 +63,25 @@ type RunnerConfig struct { // only to snapshot Runner fields the FFI call needs (e.g. the partition // set), then released before crossing into the session. // +// Teardown adds a third guarantee: every session-crossing method - including +// ProcessOne - registers with r.ops (via beginSessionOp, or directly under +// r.mu), and Destroy sets closed (refusing new ops), drains, then waits for +// r.ops before freeing the session. Without it, Destroy races the in-flight +// FFI call of a step verb issued from another goroutine - and two of those +// goroutines are the Runner's own: the drain-resume and break_at auto-step +// goroutines OnBreak spawns are, by construction, still alive at the moment +// the feed goroutine they unpark exits, which is exactly when front-end +// teardowns proceed to Destroy. With ProcessOne registered too, Destroy is +// safe even against a feed still in flight: the drain unparks it, ops.Wait +// sees it out, and the source loop's next ProcessOne returns stop. (The +// OnBreak-spawned goroutines register unconditionally rather than through +// the closed gate - see the comment there for why that cannot deadlock.) +// // The mutable state is split into two concerns, both guarded by r.mu: run // holds what processing produces (stats, partitions, fault, status), control // holds the debug pause/break wiring. step is an atomic.Int64 read lock-free -// (see Step), deliberately outside the mutex. +// (see Step), deliberately outside the mutex. closed is lifecycle, guarded +// by r.mu alongside them. type Runner struct { feed FeedFn session *gafferruntime.Session @@ -78,10 +93,26 @@ type Runner struct { onDiagnostic func(code string) step atomic.Int64 + ops sync.WaitGroup mu sync.Mutex run runState control debugControl + closed bool +} + +// beginSessionOp registers an in-flight session-crossing call so Destroy can +// wait for it, pairing with r.ops.Done() at the call's end. Returns false once +// Destroy has begun: the session may already be freed, so the caller must skip +// the session call entirely. +func (r *Runner) beginSessionOp() bool { + r.mu.Lock() + defer r.mu.Unlock() + if r.closed { + return false + } + r.ops.Add(1) + return true } // runState is the Runner's per-run progress, mutated as events are processed @@ -147,10 +178,26 @@ func NewRunner(cfg RunnerConfig) *Runner { // step the break_at pause converts into - so the blocked // Feed returns and the feed goroutine can exit. The resume // error is irrelevant: the session is going away. + // The two goroutines spawned below register with r.ops directly, + // NOT via beginSessionOp: they must run even mid-Destroy (closed + // set), because they are what unparks the blocked Feed that + // Destroy's ops.Wait is waiting out - gating them on closed would + // deadlock the teardown. The unconditional Add is safe: this + // callback runs inside the feed goroutine's own registered op + // (ProcessOne), so the counter is non-zero and the session cannot + // be freed before these goroutines finish. Registration happens + // here, before the spawn, so ops.Wait can never pass between the + // spawn and a goroutine registering itself. That safety argument + // rests on Feed only ever being driven through ProcessOne - a + // caller feeding the session directly with debug wired would let + // these Adds race ops.Wait at counter zero. (WaitGroup.Go adds + // synchronously before spawning, preserving exactly that order.) if r.control.draining { r.control.paused = false r.mu.Unlock() - go func() { _ = r.debug.Session.Continue() }() + r.ops.Go(func() { + _ = r.debug.Session.Continue() + }) return } if info.Reason == "pause" && r.control.breakAtStep > 0 { @@ -160,11 +207,11 @@ func NewRunner(cfg RunnerConfig) *Runner { // StepInto blocks on the engine thread that's currently running // this callback. A returned error has no caller here, so route // it through OnError (e.g. MCP's error channel). - go func() { + r.ops.Go(func() { if err := r.debug.Session.StepInto(); err != nil && r.debug.OnError != nil { r.debug.OnError(err) } - }() + }) return } r.control.paused = true @@ -177,6 +224,14 @@ func NewRunner(cfg RunnerConfig) *Runner { func (r *Runner) ProcessOne(eventJSON string) (stop bool) { r.mu.Lock() + if r.closed { + // Teardown has begun and the session may already be freed: stop the + // source loop instead of feeding into it. + r.mu.Unlock() + return true + } + r.ops.Add(1) + defer r.ops.Done() step := r.step.Add(1) var pauseErr error if r.debug != nil { diff --git a/cli/internal/engine/runner_debug.go b/cli/internal/engine/runner_debug.go index a95d2991..f089ce0c 100644 --- a/cli/internal/engine/runner_debug.go +++ b/cli/internal/engine/runner_debug.go @@ -31,6 +31,10 @@ func (r *Runner) SetBreakpoints(breakpoints []Breakpoint) ([]*gafferruntime.Snap if r.debug == nil { return nil, errors.New("debug not enabled") } + if !r.beginSessionOp() { + return nil, gafferruntime.ErrSessionDestroyed + } + defer r.ops.Done() if err := r.debug.Session.ClearBreakpoints(); err != nil { return nil, fmt.Errorf("clearing breakpoints: %w", err) } @@ -53,6 +57,10 @@ func (r *Runner) ClearBreakpoints() error { if r.debug == nil { return nil } + if !r.beginSessionOp() { + return gafferruntime.ErrSessionDestroyed + } + defer r.ops.Done() return r.debug.Session.ClearBreakpoints() } @@ -66,8 +74,17 @@ func (r *Runner) doStep(fn func() error) error { // false on error also lets the caller's next Paused() guard reject cleanly // rather than re-entering here and failing again. r.mu.Lock() + if r.closed { + // Teardown has begun: the session may already be freed. Refuse + // loudly rather than report a step that never happened - the + // inspection methods do the same. + r.mu.Unlock() + return gafferruntime.ErrSessionDestroyed + } + r.ops.Add(1) r.control.paused = false r.mu.Unlock() + defer r.ops.Done() return fn() } @@ -98,6 +115,10 @@ func (r *Runner) Pause() error { if r.debug == nil { return nil } + if !r.beginSessionOp() { + return gafferruntime.ErrSessionDestroyed + } + defer r.ops.Done() return r.debug.Session.Pause() } @@ -113,6 +134,18 @@ func (r *Runner) Pause() error { // // Safe to call when debug is disabled (no-op) or no breakpoint is set. func (r *Runner) Drain() { + if !r.beginSessionOp() { + // Destroy has begun, and its own drain covers the teardown. + return + } + defer r.ops.Done() + r.drain() +} + +// drain is Drain without the session-op registration: Destroy calls it after +// setting closed (which makes beginSessionOp refuse), sequenced on its own +// goroutine before the session is freed. +func (r *Runner) drain() { if r.debug == nil { return } @@ -137,14 +170,37 @@ func (r *Runner) Drain() { } } +// Destroy frees the session after draining and waiting out every in-flight +// session-crossing call, an in-flight ProcessOne included (see the Runner +// doc comment's teardown guarantee). Idempotent; a concurrent second call +// no-ops immediately, returning before the first call's teardown completes. +// Front-ends should still stop their source loop first as a matter of +// hygiene, but a straggling feed no longer races the teardown. +// +// The wait trusts every registered op to finish. The runtime's debug-command +// queue can strand a command (an unsynchronized paused check-then-add racing +// the command loop's exit leaves the command's Done unsignalled), and a +// stranded verb now blocks Destroy where it previously leaked a goroutine. +// The window needs concurrent debug verbs on a paused engine; the fix is +// runtime-side, in the command loop. +// +// The history store is NOT closed here: the Runner only borrows it, and in +// the dev debug loop one store serves every restart iteration's runner. +// Whoever created the store closes it. func (r *Runner) Destroy() { - r.Drain() + r.mu.Lock() + if r.closed { + r.mu.Unlock() + return + } + r.closed = true + r.mu.Unlock() + + r.drain() + r.ops.Wait() if r.session != nil { r.session.Destroy() } - if r.history != nil { - _ = r.history.Close() - } } // Inspection methods. These cross into the runtime session, which is not @@ -157,6 +213,10 @@ func (r *Runner) Evaluate(expression string) (*gafferruntime.DebugVariable, erro if r.debug == nil { return nil, errors.New("debug not enabled") } + if !r.beginSessionOp() { + return nil, gafferruntime.ErrSessionDestroyed + } + defer r.ops.Done() return r.debug.Session.Evaluate(expression) } @@ -164,6 +224,10 @@ func (r *Runner) GetCallStack() ([]gafferruntime.DebugCallFrame, error) { if r.debug == nil { return nil, errors.New("debug not enabled") } + if !r.beginSessionOp() { + return nil, gafferruntime.ErrSessionDestroyed + } + defer r.ops.Done() return r.debug.Session.GetCallStack() } @@ -171,6 +235,10 @@ func (r *Runner) GetScopes(frameID int) ([]gafferruntime.DebugScopeInfo, error) if r.debug == nil { return nil, errors.New("debug not enabled") } + if !r.beginSessionOp() { + return nil, gafferruntime.ErrSessionDestroyed + } + defer r.ops.Done() return r.debug.Session.GetScopes(frameID) } @@ -178,6 +246,10 @@ func (r *Runner) GetVariables(variablesReference int) ([]gafferruntime.DebugVari if r.debug == nil { return nil, errors.New("debug not enabled") } + if !r.beginSessionOp() { + return nil, gafferruntime.ErrSessionDestroyed + } + defer r.ops.Done() return r.debug.Session.GetVariables(variablesReference) } @@ -191,8 +263,16 @@ func (r *Runner) CollectState() (StateSummary, error) { return StateSummary{}, nil } r.mu.Lock() + if r.closed { + // Mirrors the nil-session return: after teardown there is no state + // left to read. + r.mu.Unlock() + return StateSummary{}, nil + } + r.ops.Add(1) partitions := maps.Clone(r.run.partitions) r.mu.Unlock() + defer r.ops.Done() return CollectState(r.session, r.info, partitions) } @@ -204,6 +284,11 @@ func (r *Runner) GetPartitionState(partition string) (state *string, result *str if r.session == nil { return nil, nil, nil } + if !r.beginSessionOp() { + // Mirrors the nil-session return, like CollectState. + return nil, nil, nil + } + defer r.ops.Done() state, err = r.session.GetState(&partition) if err != nil { return nil, nil, fmt.Errorf("reading state for partition %q: %w", partition, err) diff --git a/cli/internal/engine/runner_test.go b/cli/internal/engine/runner_test.go index ece9e7f8..88619f95 100644 --- a/cli/internal/engine/runner_test.go +++ b/cli/internal/engine/runner_test.go @@ -345,6 +345,68 @@ func TestRunner_Integration_FaultedMidStream(t *testing.T) { // --- nil-guard tests: debug and history methods on a minimal runner --- +// TestRunner_Destroy_WaitsForInflightStep pins the teardown guarantee (see +// the Runner doc comment): a step verb issued from another goroutine - the +// DAP continue handler, the MCP stop path, the Runner's own drain-resume - +// can still be inside its FFI call at the moment the feed goroutine exits, +// which is exactly when callers proceed to Destroy. Destroy must wait it out +// rather than free the session under it; the race detector enforces the +// "rather than". +func TestRunner_Destroy_WaitsForInflightStep(t *testing.T) { + opts := `{"engineVersion":2,"debug":true}` + session, err := gafferruntime.NewSession(`fromAll().when({ + $init() { return { count: 0 } }, + ItemAdded(s, e) { + s.count++; + return s; + } + })`, &opts) + if err != nil { + t.Fatal(err) + } + t.Cleanup(session.Destroy) + + broke := make(chan struct{}, 1) + r := NewRunner(RunnerConfig{ + Feed: session.Feed, + Session: session, + Info: session.GetSources(), + Debug: &DebugConfig{ + Session: session, + Info: session.GetSources(), + OnBreak: func(gafferruntime.BreakInfo) { broke <- struct{}{} }, + }, + }) + if _, err := r.SetBreakpoints([]Breakpoint{{Line: 4}}); err != nil { + t.Fatal(err) + } + + feedDone := make(chan struct{}) + go func() { + defer close(feedDone) + r.ProcessOne(testutil.Event("ItemAdded", "s-1", 0)) + }() + <-broke + + // Resume from a second goroutine and tear down as soon as the feed + // goroutine exits, without joining the resumer - the shape every + // front-end teardown has. + go func() { _ = r.Continue() }() + <-feedDone + r.Destroy() + + // After Destroy the whole control surface refuses with + // ErrSessionDestroyed instead of touching (or panicking on) the freed + // session - or worse, reporting a step that never happened. + if err := r.Continue(); !errors.Is(err, gafferruntime.ErrSessionDestroyed) { + t.Errorf("Continue after Destroy: got %v, want ErrSessionDestroyed", err) + } + if _, err := r.Evaluate("1"); !errors.Is(err, gafferruntime.ErrSessionDestroyed) { + t.Errorf("Evaluate after Destroy: got %v, want ErrSessionDestroyed", err) + } + r.Destroy() // idempotent +} + func TestRunner_DebugNilGuards(t *testing.T) { r := NewRunner(RunnerConfig{Feed: func(string) (*gafferruntime.FeedResult, error) { return nil, nil }}) diff --git a/cli/internal/lsp/handlers.go b/cli/internal/lsp/handlers.go index e5e41520..7681673f 100644 --- a/cli/internal/lsp/handlers.go +++ b/cli/internal/lsp/handlers.go @@ -12,6 +12,17 @@ import ( // handle dispatches a single JSON-RPC message to the right method. // jsonrpc2.HandlerWithError takes care of error/result wrapping. func (s *Server) handle(ctx context.Context, _ *jsonrpc2.Conn, req *jsonrpc2.Request) (any, error) { + // Wait for Run to finish storing the run state (see Server.ready): + // dispatch can start before Run's conn assignment, and a handler + // observing the half-initialized state loses work silently. Closed + // microseconds after the conn exists; nil only when no Run is + // active, which a dispatched handler can't observe. + s.mu.Lock() + ready := s.ready + s.mu.Unlock() + if ready != nil { + <-ready + } switch req.Method { case MethodInitialize: return s.handleInitialize(ctx, req) diff --git a/cli/internal/lsp/lifecycle.go b/cli/internal/lsp/lifecycle.go index fec0db54..0241be70 100644 --- a/cli/internal/lsp/lifecycle.go +++ b/cli/internal/lsp/lifecycle.go @@ -20,21 +20,31 @@ import ( func (s *Server) Run(ctx context.Context, stream io.ReadWriteCloser) error { runCtx, cancel := context.WithCancel(ctx) defer cancel() + // The run state must be visible to handlers before any handler can + // run, and NewConn starts dispatching the moment it returns. runCtx + // exists already, so it's stored first; the conn can only be stored + // after NewConn, so handler dispatch blocks on `ready` until then + // (see Server.ready and the gate at the top of handle). Without + // this, a fast client's `initialized` could find runCtxFn nil and + // silently lose the workspace walk. + ready := make(chan struct{}) + s.mu.Lock() + s.ready = ready + s.runCtxFn = func() context.Context { return runCtx } + s.cancelRun = cancel + s.mu.Unlock() conn := jsonrpc2.NewConn( runCtx, jsonrpc2.NewBufferedStream(stream, jsonrpc2.VSCodeObjectCodec{}), jsonrpc2.HandlerWithError(s.handle), ) - // Capture the conn + runCtx for server-pushed notifications - // (publishDiagnostics, registerCapability) and for spawned work - // (workspace walk) that needs a shutdown signal. Cleared on - // disconnect so handlers that reach for them post-shutdown bail - // cleanly. + // Capture the conn for server-pushed notifications + // (publishDiagnostics, registerCapability). Cleared on disconnect + // so handlers that reach for it post-shutdown bail cleanly. s.mu.Lock() s.conn = conn - s.runCtxFn = func() context.Context { return runCtx } - s.cancelRun = cancel s.mu.Unlock() + close(ready) defer func() { // Teardown order: // 1. Cancel runCtx so in-flight parseAndPublish notices @@ -58,6 +68,7 @@ func (s *Server) Run(ctx context.Context, stream io.ReadWriteCloser) error { s.conn = nil s.runCtxFn = nil s.cancelRun = nil + s.ready = nil s.mu.Unlock() }() // Three ways out: peer disconnects, client sent exit (we drive diff --git a/cli/internal/lsp/server.go b/cli/internal/lsp/server.go index 56b2c6cc..60a1daa7 100644 --- a/cli/internal/lsp/server.go +++ b/cli/internal/lsp/server.go @@ -84,6 +84,14 @@ type Server struct { // goroutines blocked on I/O after the connection is gone. runCtxFn func() context.Context cancelRun context.CancelFunc + // ready gates handler dispatch on Run having stored conn/runCtxFn. + // jsonrpc2.NewConn starts dispatching the moment it's constructed, + // so without the gate a fast client's `initialized` can reach + // spawnWithCtx while runCtxFn is still nil - silently dropping the + // workspace walk, which nothing retries. Created before the conn, + // closed once the run state is stored; nil when Run isn't active + // (handlers can't be dispatched then anyway - no conn exists). + ready chan struct{} // roots holds workspace folder paths captured from initialize. // Used by the initialized handler to walk for gaffer.toml files. // Stored as filesystem paths (URIs converted at capture time) diff --git a/cli/internal/lsp/server_test.go b/cli/internal/lsp/server_test.go index b4f1427f..e029ebfe 100644 --- a/cli/internal/lsp/server_test.go +++ b/cli/internal/lsp/server_test.go @@ -456,10 +456,13 @@ func TestServer_DidSaveIsAccepted(t *testing.T) { // waitForTimeout is the canonical "wait for an async server effect" // budget. The happy-path latency for these helpers is milliseconds // (one pipe roundtrip plus a goroutine schedule); the slack is for -// GitHub-runner CPU contention. Tests that intentionally bound a +// GitHub-runner CPU contention, which the race-instrumented run +// multiplies - 5s was observed starved out with every package's test +// binary in flight. Passing tests never spend the slack (waitFor +// returns on the first true poll). Tests that intentionally bound a // shorter window (e.g. "no second publish within 500ms") pass the // value to waitFor explicitly. -const waitForTimeout = 5 * time.Second +const waitForTimeout = 30 * time.Second // ctxTimeout backstops a test's server connection. It must exceed // waitForTimeout: a waitFor poll outliving its connection context diff --git a/cli/internal/mcpserver/deploy_recreate.go b/cli/internal/mcpserver/deploy_recreate.go index 8f051725..fa2d08a5 100644 --- a/cli/internal/mcpserver/deploy_recreate.go +++ b/cli/internal/mcpserver/deploy_recreate.go @@ -108,7 +108,8 @@ func (s *Server) handleDeployRecreate(ctx context.Context, req *mcp.CallToolRequ // per-step RPC bounds, and recovery messages live in internal/deploy // (shared with the CLI); here we only bind each step to the client. err = deploy.Recreate(ctx, in.Name, deploy.RecreateSteps{ - Disable: func(sctx context.Context) error { return conn.client.Disable(sctx, in.Name) }, + RetryCreate: remote.IsCreateConflict, + Disable: func(sctx context.Context) error { return conn.client.Disable(sctx, in.Name) }, Delete: func(sctx context.Context) error { return conn.client.Delete(sctx, in.Name, remote.DeleteOptions{ DeleteStateStream: true, diff --git a/cli/internal/mcpserver/server.go b/cli/internal/mcpserver/server.go index c009c70a..79c22a76 100644 --- a/cli/internal/mcpserver/server.go +++ b/cli/internal/mcpserver/server.go @@ -142,6 +142,9 @@ func (s *Server) recordProjectionError(err error) { type activeSession struct { runner *engine.Runner cancel context.CancelFunc + // store backs the runner's step history. The runner only borrows it, so + // closeSession closes it here after the runner is destroyed. + store *history.Store // debug is true when the run requested a breakpoint or break_at, so a // timeout can name a breakpoint; live is true for a live subscription, @@ -461,6 +464,9 @@ func (s *Server) closeSession() { <-sess.done } sess.runner.Destroy() + if sess.store != nil { + _ = sess.store.Close() + } } func (s *Server) createSession(cfg *config.Config, root, name string, debug bool) (*activeSession, error) { @@ -479,7 +485,7 @@ func (s *Server) createSession(cfg *config.Config, root, name string, debug bool return nil, fmt.Errorf("creating history store: %w", err) } - sess := &activeSession{debug: debug} + sess := &activeSession{debug: debug, store: store} runnerCfg := engine.RunnerConfig{ Feed: engine.FeedFn(runtime.Feed), diff --git a/cli/internal/remote/classify_test.go b/cli/internal/remote/classify_test.go index ce88972c..cdf4dc79 100644 --- a/cli/internal/remote/classify_test.go +++ b/cli/internal/remote/classify_test.go @@ -60,3 +60,23 @@ func TestClassifyVersion(t *testing.T) { }) } } + +func TestIsCreateConflict(t *testing.T) { + for _, tc := range []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"envelope conflict reply", errors.New("rpc error: code = Unknown desc = Envelope callback expected Updated, received Conflict instead"), true}, + {"typed sentinel", ErrAlreadyExists, true}, + {"wrapped sentinel", errors.Join(errors.New("create orders"), ErrAlreadyExists), true}, + {"unrelated failure", errors.New("rpc error: code = Unavailable desc = leader down"), false}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := IsCreateConflict(tc.err); got != tc.want { + t.Errorf("IsCreateConflict(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} diff --git a/cli/internal/remote/errors.go b/cli/internal/remote/errors.go index 591ebff0..62134be4 100644 --- a/cli/internal/remote/errors.go +++ b/cli/internal/remote/errors.go @@ -3,6 +3,7 @@ package remote import ( "errors" "fmt" + "strings" "github.com/kurrent-io/KurrentDB-Client-Go/kurrentdb" "google.golang.org/grpc/codes" @@ -42,6 +43,21 @@ var ( ErrMalformedLedger = errors.New("malformed tool metadata") ) +// IsCreateConflict reports whether err is the server refusing a projection +// create because the name is still registered. The projections subsystem +// replies Conflict through its command envelope - "Envelope callback expected +// Updated, received Conflict instead" inside an unclassified gRPC Unknown - +// never AlreadyExists (see that sentinel), so this has to match the envelope +// text; the sentinel check covers a server version that someday returns the +// code. Recreate retries on it: a delete settles asynchronously, and a create +// racing the lingering registration bounces with exactly this reply. +func IsCreateConflict(err error) bool { + if err == nil { + return false + } + return errors.Is(err, ErrAlreadyExists) || strings.Contains(err.Error(), "received Conflict") +} + // classify maps a projection-operation error to a typed sentinel where it // recognises one, leaving unrecognised errors wrapped unchanged. // diff --git a/cli/justfile b/cli/justfile index 59525eac..acfc8884 100644 --- a/cli/justfile +++ b/cli/justfile @@ -34,13 +34,15 @@ build: _resources build-release version: _resources CGO_ENABLED=1 go build -ldflags '-X github.com/kurrent-io/gaffer/cli/cmd.Version={{version}} -X github.com/kurrent-io/gaffer/cli/cmd.devBuild=false -X github.com/kurrent-io/gaffer/cli/internal/telemetry.DefaultWorkerURL=https://telemetry.gaffer.kurrent.io/v1/ingest' -o gaffer{{exe-suffix}} . -# Run tests +# Run tests. -race doubles as a checkptr gate on the FFI boundary and +# guards the session-teardown paths (an in-flight debug command racing +# Destroy fatals here instead of corrupting a user's process). test: _resources - go test -tags dev ./... + go test -tags dev -race ./... # Run integration tests (requires KurrentDB) test-integration: _resources - go test -tags 'dev integration' ./... + go test -tags 'dev integration' -race ./... # Run the CLI run *args: _resources