diff --git a/pkg/lifecycle/reconfigure.go b/pkg/lifecycle/reconfigure.go index ab82a9b1b..bce95b3b4 100644 --- a/pkg/lifecycle/reconfigure.go +++ b/pkg/lifecycle/reconfigure.go @@ -73,7 +73,7 @@ func (s *Service) ReconfigureProcessor(ctx context.Context, pipelineID, processo if err != nil { return cerrors.Errorf("could not fetch processor %q for live reconfigure: %w", processorID, err) } - runnableProc, err := s.processors.MakeRunnableProcessor(ctx, instance) + runnableProc, err := s.processors.MakeRunnableProcessorForReconfigure(ctx, instance) if err != nil { return cerrors.Errorf("could not build runnable processor %q for live reconfigure: %w", processorID, err) } diff --git a/pkg/lifecycle/service.go b/pkg/lifecycle/service.go index af9e617ad..be1e7ea6b 100644 --- a/pkg/lifecycle/service.go +++ b/pkg/lifecycle/service.go @@ -133,6 +133,11 @@ type ConnectorService interface { type ProcessorService interface { Get(ctx context.Context, id string) (*processor.Instance, error) MakeRunnableProcessor(ctx context.Context, i *processor.Instance) (*processor.RunnableProcessor, error) + // MakeRunnableProcessorForReconfigure builds a runnable for an + // already-running instance without the running guard — for the live in-place + // reconfigure swap (ReconfigureProcessor). See + // processor.Service.MakeRunnableProcessorForReconfigure. + MakeRunnableProcessorForReconfigure(ctx context.Context, i *processor.Instance) (*processor.RunnableProcessor, error) } // ConnectorPluginService can create a connector plugin dispenser. diff --git a/pkg/lifecycle/service_test.go b/pkg/lifecycle/service_test.go index 8748e2f2a..8a6c3f261 100644 --- a/pkg/lifecycle/service_test.go +++ b/pkg/lifecycle/service_test.go @@ -1107,6 +1107,10 @@ func (s testProcessorService) MakeRunnableProcessor(context.Context, *processor. return nil, cerrors.New("not implemented") } +func (s testProcessorService) MakeRunnableProcessorForReconfigure(context.Context, *processor.Instance) (*processor.RunnableProcessor, error) { + return nil, cerrors.New("not implemented") +} + func (s testProcessorService) Get(_ context.Context, id string) (*processor.Instance, error) { proc, ok := s[id] if !ok { diff --git a/pkg/lifecycle/stream/processor.go b/pkg/lifecycle/stream/processor.go index d16330255..8b1f99eb1 100644 --- a/pkg/lifecycle/stream/processor.go +++ b/pkg/lifecycle/stream/processor.go @@ -282,7 +282,11 @@ func (n *ProcessorNode) applyPendingSwap(ctx context.Context) { // Keep the current processor running; report the failure. Best-effort // teardown of the failed new processor (Open may have partially // initialized it, e.g. started a WASM module), mirroring Run's defer. - if tdErr := p.newProcessor.Teardown(ctx); tdErr != nil { + // reconfigureTeardown, not Teardown: the current (old) processor and this + // failed new one share the same processor.Instance, so clearing the + // instance's running flag here would mark the STILL-running old processor + // stopped. See teardownForReconfigure. + if tdErr := teardownForReconfigure(ctx, p.newProcessor); tdErr != nil { n.logger.Warn(ctx).Err(tdErr).Msg("could not tear down new processor after failed live-reconfigure open") } p.done <- cerrors.Errorf("could not open new processor for live reconfigure, keeping current processor: %w", err) @@ -291,7 +295,13 @@ func (n *ProcessorNode) applyPendingSwap(ctx context.Context) { old := n.Processor n.Processor = p.newProcessor - if tdErr := old.Teardown(ctx); tdErr != nil { + // reconfigureTeardown, not Teardown: old and the new (now-live) processor + // share the same processor.Instance, which stays running via the new one. + // A plain Teardown here would clear that shared instance's running flag, + // wrongly marking the live processor stopped and disarming the Update/Delete + // guards. Only a real pipeline stop clears running (via the node's normal + // Teardown of its current processor). + if tdErr := teardownForReconfigure(ctx, old); tdErr != nil { // The swap already succeeded; a teardown error on the old processor is // logged, not surfaced as a swap failure. n.logger.Warn(ctx).Err(tdErr).Msg("could not tear down previous processor after live reconfigure") @@ -299,6 +309,23 @@ func (n *ProcessorNode) applyPendingSwap(ctx context.Context) { p.done <- nil } +// teardownForReconfigure tears down proc during a live swap without clearing the +// running state of a shared processor Instance. The built-in RunnableProcessor +// embeds a single *Instance shared across the runnables of a swap, so tearing one +// down with the plain Teardown would clear the instance's running flag even though +// the instance stays running via the swap's other runnable. A Processor that needs +// this distinction implements TeardownForReconfigure; any other implementation +// (e.g. a test mock) falls back to the plain Teardown, which is correct for +// processors that don't share instance state across a swap. +func teardownForReconfigure(ctx context.Context, proc Processor) error { + if rp, ok := proc.(interface { + TeardownForReconfigure(context.Context) error + }); ok { + return rp.TeardownForReconfigure(ctx) + } + return proc.Teardown(ctx) +} + // handleSingleRecord handles a sdk.SingleRecord by checking the position, // setting the new record on the message and sending it downstream. // If there are any errors, the method nacks the message and returns diff --git a/pkg/lifecycle/stream/processor_reconfigure_test.go b/pkg/lifecycle/stream/processor_reconfigure_test.go index 4430e3cc3..f9c2b4248 100644 --- a/pkg/lifecycle/stream/processor_reconfigure_test.go +++ b/pkg/lifecycle/stream/processor_reconfigure_test.go @@ -198,3 +198,130 @@ func TestProcessorNode_Reconfigure_ContextCancelled(t *testing.T) { err := n.Reconfigure(ctx, mock.NewProcessor(ctrl)) is.True(cerrors.Is(err, context.Canceled)) } + +// teardownSpy is a Processor that records whether it was torn down via the plain +// Teardown (which, for the real RunnableProcessor, clears the shared Instance's +// running flag) or via TeardownForReconfigure (which does not). It lets the swap +// tests below assert that applyPendingSwap never uses the running-clearing +// Teardown on a processor that is being swapped while its instance stays running. +type teardownSpy struct { + openErr error + + mu sync.Mutex + teardownCalls int + reconfigCalls int +} + +func (s *teardownSpy) Open(context.Context) error { return s.openErr } + +func (s *teardownSpy) Process(_ context.Context, recs []opencdc.Record) []sdk.ProcessedRecord { + out := make([]sdk.ProcessedRecord, len(recs)) + for i, r := range recs { + out[i] = sdk.SingleRecord(r) + } + return out +} + +func (s *teardownSpy) Teardown(context.Context) error { + s.mu.Lock() + defer s.mu.Unlock() + s.teardownCalls++ + return nil +} + +func (s *teardownSpy) TeardownForReconfigure(context.Context) error { + s.mu.Lock() + defer s.mu.Unlock() + s.reconfigCalls++ + return nil +} + +func (s *teardownSpy) counts() (teardown, reconfig int) { + s.mu.Lock() + defer s.mu.Unlock() + return s.teardownCalls, s.reconfigCalls +} + +// TestProcessorNode_Reconfigure_SuccessfulSwap_UsesReconfigureTeardown proves the +// swap tears the OLD processor down via TeardownForReconfigure (not the running +// -clearing Teardown), and that a real pipeline STOP still uses the plain Teardown +// on the node's current processor. This is the wiring half of the shared-instance +// running-flag regression (the processor-package half lives in +// TestReconfigureSwap_KeepsInstanceRunning_GuardsStayArmed). +func TestProcessorNode_Reconfigure_SuccessfulSwap_UsesReconfigureTeardown(t *testing.T) { + is := is.New(t) + ctx := context.Background() + + old := &teardownSpy{} + newp := &teardownSpy{} + n := &ProcessorNode{Name: "test", Processor: old, ProcessorTimer: noop.Timer{}} + in := make(chan *Message) + n.Sub(in) + out := n.Pub() + + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + is.NoErr(n.Run(ctx)) + }() + + is.NoErr(n.Reconfigure(ctx, newp)) // blocks until the swap is applied + + oldTd, oldRc := old.counts() + is.Equal(oldTd, 0) // the running-clearing Teardown must NOT run on a swap + is.Equal(oldRc, 1) // the old processor was torn down via TeardownForReconfigure + + close(in) + wg.Wait() + + newTd, newRc := newp.counts() + is.Equal(newTd, 1) // a real stop uses the plain, running-clearing Teardown + is.Equal(newRc, 0) + + _, ok := <-out + is.Equal(false, ok) +} + +// TestProcessorNode_Reconfigure_OpenFailure_UsesReconfigureTeardown proves that +// when the NEW processor fails to open, its best-effort cleanup also uses +// TeardownForReconfigure — the old processor (sharing the instance) keeps running, +// so its running flag must not be cleared by tearing down the failed new one. +func TestProcessorNode_Reconfigure_OpenFailure_UsesReconfigureTeardown(t *testing.T) { + is := is.New(t) + ctx := context.Background() + + old := &teardownSpy{} + newp := &teardownSpy{openErr: cerrors.New("bad new config")} + n := &ProcessorNode{Name: "test", Processor: old, ProcessorTimer: noop.Timer{}} + in := make(chan *Message) + n.Sub(in) + out := n.Pub() + + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + is.NoErr(n.Run(ctx)) + }() + + err := n.Reconfigure(ctx, newp) + is.True(err != nil) // open failed, swap rejected + + newTd, newRc := newp.counts() + is.Equal(newTd, 0) // failed-new cleanup must not use the running-clearing Teardown + is.Equal(newRc, 1) // it used TeardownForReconfigure + + oldTd, oldRc := old.counts() + is.Equal(oldTd, 0) // old kept running, untouched by the failed swap + is.Equal(oldRc, 0) + + close(in) + wg.Wait() + + oldTd2, _ := old.counts() + is.Equal(oldTd2, 1) // stop tears the still-current old processor down fully + + _, ok := <-out + is.Equal(false, ok) +} diff --git a/pkg/processor/instance.go b/pkg/processor/instance.go index cce408b2b..205e13ba8 100644 --- a/pkg/processor/instance.go +++ b/pkg/processor/instance.go @@ -18,6 +18,7 @@ package processor import ( "context" + "sync/atomic" "time" "github.com/conduitio/conduit/pkg/foundation/log" @@ -62,7 +63,13 @@ type Instance struct { // before the processor is actually running. inInsp *inspector.Inspector outInsp *inspector.Inspector - running bool + // running guards the instance against a concurrent Update/Delete while it is + // live in a pipeline. It is read on the API goroutine (Service.Update/Delete) + // and written on the pipeline's Run goroutine (RunnableProcessor.Teardown at + // stop), so it must be accessed atomically. MakeRunnableProcessor uses + // CompareAndSwap to reserve it, which also makes the "refuse if already + // running" guard a single atomic test-and-set rather than a racy read+write. + running atomic.Bool } func (i *Instance) init(logger log.CtxLogger) { diff --git a/pkg/processor/runnable_processor.go b/pkg/processor/runnable_processor.go index 1d23f5f67..e0c51b454 100644 --- a/pkg/processor/runnable_processor.go +++ b/pkg/processor/runnable_processor.go @@ -142,6 +142,20 @@ func (p *RunnableProcessor) Process(ctx context.Context, records []opencdc.Recor func (p *RunnableProcessor) Teardown(ctx context.Context) error { err := p.proc.Teardown(ctx) - p.running = false + p.running.Store(false) return err } + +// TeardownForReconfigure tears down the processor plugin WITHOUT clearing the +// shared Instance.running flag. It exists for the live in-place reconfigure swap +// (ProcessorNode.applyPendingSwap), where a RunnableProcessor is torn down but +// the underlying Instance stays running via the other runnable in the swap: on +// success the new runnable takes over; on a failed open the old runnable keeps +// running. Because both runnables embed the SAME *Instance, the plain Teardown's +// `running = false` would wrongly mark the still-running instance stopped — +// disarming the Update/Delete running-guards (service.go) for the rest of the +// pipeline's life. Only a real pipeline stop, which tears down the node's +// current processor via the plain Teardown, may clear running. +func (p *RunnableProcessor) TeardownForReconfigure(ctx context.Context) error { + return p.proc.Teardown(ctx) +} diff --git a/pkg/processor/service.go b/pkg/processor/service.go index 71d749101..a88859223 100644 --- a/pkg/processor/service.go +++ b/pkg/processor/service.go @@ -102,20 +102,46 @@ func (s *Service) Get(_ context.Context, id string) (*Instance, error) { } func (s *Service) MakeRunnableProcessor(ctx context.Context, i *Instance) (*RunnableProcessor, error) { - if i.running { + // Reserve the instance as running with a single atomic test-and-set: this both + // enforces the guard (refuse an already-running instance) and closes the + // read-then-write race two concurrent callers would otherwise have. On any + // subsequent build failure we release the reservation so the instance is not + // left wrongly marked running. + if !i.running.CompareAndSwap(false, true) { return nil, ErrProcessorRunning } p, err := s.registry.NewProcessor(ctx, i.Plugin, i.ID) if err != nil { + i.running.Store(false) return nil, err } cond, err := newProcessorCondition(i.Condition) if err != nil { + i.running.Store(false) return nil, cerrors.Errorf("invalid condition: %w", err) } - i.running = true + return newRunnableProcessor(p, cond, i), nil +} + +// MakeRunnableProcessorForReconfigure builds a fresh RunnableProcessor for an +// ALREADY-RUNNING processor instance, for the live in-place reconfigure swap +// (lifecycle.ReconfigureProcessor). Unlike MakeRunnableProcessor it does not +// refuse a running instance and does not flip the running flag: the instance +// legitimately stays running across the swap, and the caller opens this new +// runnable before tearing down the old one (open-before-teardown), so no record +// is dropped. It dispenses a new plugin from the instance's current (already +// updated) config, exactly as MakeRunnableProcessor does for a fresh start. +func (s *Service) MakeRunnableProcessorForReconfigure(ctx context.Context, i *Instance) (*RunnableProcessor, error) { + p, err := s.registry.NewProcessor(ctx, i.Plugin, i.ID) + if err != nil { + return nil, err + } + cond, err := newProcessorCondition(i.Condition) + if err != nil { + return nil, cerrors.Errorf("invalid condition: %w", err) + } return newRunnableProcessor(p, cond, i), nil } @@ -183,7 +209,7 @@ func (s *Service) Update(ctx context.Context, id string, plugin string, cfg Conf // callers (the orchestrator / HTTP+gRPC API) are refused here; the sole // exception is provisioning's live in-place reconfigure, which uses // UpdateWhileRunning and immediately swaps the node to match. - if instance.running { + if instance.running.Load() { return nil, cerrors.Errorf("could not update processor instance (ID: %s): %w", id, ErrProcessorRunning) } @@ -235,7 +261,7 @@ func (s *Service) Delete(ctx context.Context, id string) error { return err } - if instance.running { + if instance.running.Load() { return cerrors.Errorf("could not delete processor instance (ID: %s): %w", id, ErrProcessorRunning) } diff --git a/pkg/processor/service_test.go b/pkg/processor/service_test.go index 38e1c66a0..f35caf9c7 100644 --- a/pkg/processor/service_test.go +++ b/pkg/processor/service_test.go @@ -17,6 +17,7 @@ package processor import ( "context" "strings" + "sync" "testing" "github.com/conduitio/conduit-commons/database/inmemory" @@ -427,7 +428,7 @@ func TestService_UpdateWhileRunning_BypassesRunningGuard(t *testing.T) { inst, err := service.Create(ctx, uuid.NewString(), procType, Parent{}, Config{}, ProvisionTypeAPI, "") is.NoErr(err) - inst.running = true // as it is inside a running pipeline + inst.running.Store(true) // as it is inside a running pipeline newConfig := Config{Settings: map[string]string{"k": "v"}} @@ -446,6 +447,145 @@ func TestService_UpdateWhileRunning_BypassesRunningGuard(t *testing.T) { is.Equal(newConfig, reread.Config) } +// TestService_MakeRunnableProcessorForReconfigure_BypassesRunningGuard is the +// regression test for the SECOND running-guard that blocked live in-place +// hot-reload. MakeRunnableProcessor refuses a RUNNING instance +// (ErrProcessorRunning) and flips i.running — correct for a fresh pipeline +// start, but wrong for the reconfigure swap, where the instance legitimately +// stays running across an open-before-teardown node replacement. Fixing only +// Update (guard #1, UpdateWhileRunning) was not enough: lifecycle.Reconfigure- +// Processor then called MakeRunnableProcessor on the still-running instance and +// got ErrProcessorRunning, so the swap never happened. This guard was invisible +// to the existing suite because every in-place test mocked ProcessorService. +// MakeRunnableProcessorForReconfigure must build the runnable WITHOUT the guard +// and WITHOUT touching i.running (the caller owns the running lifetime). +func TestService_MakeRunnableProcessorForReconfigure_BypassesRunningGuard(t *testing.T) { + is := is.New(t) + ctx := context.Background() + db := &inmemory.DB{} + + procType := "processor-type" + p := proc_mock.NewProcessor(gomock.NewController(t)) + p.EXPECT().Teardown(gomock.Any()).Return(nil).AnyTimes() + registry := newPluginService(t, map[string]sdk.Processor{procType: p}) + service := NewService(log.Nop(), db, registry) + + inst, err := service.Create(ctx, uuid.NewString(), procType, Parent{}, Config{}, ProvisionTypeAPI, "") + is.NoErr(err) + inst.running.Store(true) // as it is inside a running pipeline being reconfigured + + // MakeRunnableProcessor refuses a running instance — the guard the fresh- + // start path relies on so two nodes never wrap the same instance. + _, err = service.MakeRunnableProcessor(ctx, inst) + is.True(err != nil) + is.True(cerrors.Is(err, ErrProcessorRunning)) + + // MakeRunnableProcessorForReconfigure builds the runnable anyway, for the + // live swap, and leaves i.running exactly as it was (the instance stays + // running across the swap; the caller does not re-enter the running state). + rp, err := service.MakeRunnableProcessorForReconfigure(ctx, inst) + is.NoErr(err) + is.True(rp != nil) + is.True(inst.running.Load()) // unchanged: the swap does not toggle the running flag +} + +// TestReconfigureSwap_KeepsInstanceRunning_GuardsStayArmed is the regression test +// for the shared-Instance running-flag corruption in the live in-place swap. +// RunnableProcessor embeds a single *Instance that is SHARED across the two +// runnables of a swap (the old one and the MakeRunnableProcessorForReconfigure +// one). The plain Teardown clears that instance's running flag. During a swap the +// node tears down one runnable while the instance stays running via the other — +// so the swap must use TeardownForReconfigure, which tears down the plugin WITHOUT +// clearing running. If it used the plain Teardown, the instance would be marked +// stopped while still processing records, silently disarming the Update and Delete +// running-guards for the rest of the pipeline's life (config could then be mutated +// out from under the live node, or the instance deleted while running). +func TestReconfigureSwap_KeepsInstanceRunning_GuardsStayArmed(t *testing.T) { + is := is.New(t) + ctx := context.Background() + db := &inmemory.DB{} + + procType := "processor-type" + p := proc_mock.NewProcessor(gomock.NewController(t)) + p.EXPECT().Teardown(gomock.Any()).Return(nil).AnyTimes() + registry := newPluginService(t, map[string]sdk.Processor{procType: p}) + service := NewService(log.Nop(), db, registry) + + inst, err := service.Create(ctx, uuid.NewString(), procType, Parent{}, Config{}, ProvisionTypeAPI, "") + is.NoErr(err) + + // Pipeline start: build the live runnable R1 (sets running=true). + r1, err := service.MakeRunnableProcessor(ctx, inst) + is.NoErr(err) + is.True(inst.running.Load()) + + // Reconfigure: build R2 for the SAME instance (does not touch running). + r2, err := service.MakeRunnableProcessorForReconfigure(ctx, inst) + is.NoErr(err) + + // Successful-swap teardown of the old runnable: the instance stays running via + // R2, so running must remain true. + is.NoErr(r1.TeardownForReconfigure(ctx)) + is.True(inst.running.Load()) // regression: the plain Teardown would make this false + + // The real-world consequence: the running-guards must still be armed. + _, err = service.Update(ctx, inst.ID, procType, Config{Settings: map[string]string{"k": "v"}}) + is.True(cerrors.Is(err, ErrProcessorRunning)) + err = service.Delete(ctx, inst.ID) + is.True(cerrors.Is(err, ErrProcessorRunning)) + + // Failed-open path variant: tearing down the failed NEW runnable likewise must + // not clear running (the old one keeps running). + r3, err := service.MakeRunnableProcessorForReconfigure(ctx, inst) + is.NoErr(err) + is.NoErr(r3.TeardownForReconfigure(ctx)) + is.True(inst.running.Load()) + + // A real pipeline stop tears down the node's current processor (R2) via the + // plain Teardown, which clears running and re-arms ordinary Update/Delete. + is.NoErr(r2.Teardown(ctx)) + is.True(!inst.running.Load()) + _, err = service.Update(ctx, inst.ID, procType, Config{Settings: map[string]string{"k": "v2"}}) + is.NoErr(err) // now allowed: the instance is genuinely stopped +} + +// TestService_RunningFlag_ConcurrentUpdateAndTeardown_NoRace pins the +// synchronization of Instance.running: it is read on the API goroutine (Update's +// guard) and written on the pipeline's Run goroutine (Teardown at stop). With a +// plain bool this is a data race the -race detector fails on; running is an +// atomic.Bool so this is race-free. Run with -race for it to be meaningful. +func TestService_RunningFlag_ConcurrentUpdateAndTeardown_NoRace(t *testing.T) { + is := is.New(t) + ctx := context.Background() + db := &inmemory.DB{} + + procType := "processor-type" + p := proc_mock.NewProcessor(gomock.NewController(t)) + p.EXPECT().Teardown(gomock.Any()).Return(nil).AnyTimes() + registry := newPluginService(t, map[string]sdk.Processor{procType: p}) + service := NewService(log.Nop(), db, registry) + + inst, err := service.Create(ctx, uuid.NewString(), procType, Parent{}, Config{}, ProvisionTypeAPI, "") + is.NoErr(err) + proc, err := service.MakeRunnableProcessor(ctx, inst) // running = true + is.NoErr(err) + + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + // Reads inst.running via the guard; return value is irrelevant here. + _, _ = service.Update(ctx, inst.ID, procType, Config{}) + } + }() + go func() { + defer wg.Done() + _ = proc.Teardown(ctx) // writes inst.running + }() + wg.Wait() +} + func TestService_Update_NonExistentProcessor(t *testing.T) { is := is.New(t) ctx := context.Background() diff --git a/pkg/processor/store.go b/pkg/processor/store.go index adf1c37f9..cb3a2bb0b 100644 --- a/pkg/processor/store.go +++ b/pkg/processor/store.go @@ -133,7 +133,9 @@ func (*Store) trimKeyPrefix(key string) string { func (*Store) encode(i *Instance) ([]byte, error) { var b bytes.Buffer enc := json.NewEncoder(&b) - err := enc.Encode(*i) + // Encode the pointer, not *i: Instance now holds a sync/atomic value that must + // not be copied (go vet copylocks). JSON output is identical either way. + err := enc.Encode(i) if err != nil { return nil, err } diff --git a/pkg/provisioning/apply_plan_inplace_test.go b/pkg/provisioning/apply_plan_inplace_test.go index 2554da6c2..4499227a3 100644 --- a/pkg/provisioning/apply_plan_inplace_test.go +++ b/pkg/provisioning/apply_plan_inplace_test.go @@ -74,6 +74,7 @@ func TestApplyPlanLive_ProcessorUpdate_AppliesInPlace_NoRestart(t *testing.T) { applied, err := srv.ApplyPlanLive(ctx, desired, diff.Hash, true) // authorized is.NoErr(err) is.Equal(applied.Hash, diff.Hash) + is.Equal(applied.AppliedMode, ApplyModeInPlace) // ground truth: swapped live, no restart } // TestApplyPlanLive_ProcessorUpdate_InPlace_CompletesDespiteCallerCancel proves @@ -145,9 +146,11 @@ func TestApplyPlanLive_ProcessorUpdate_NotLiveReconfigurable_FallsBackToRestart( diff, err := srv.Plan(ctx, desired) is.NoErr(err) - _, err = srv.ApplyPlanLive(ctx, desired, diff.Hash, true) + applied, err := srv.ApplyPlanLive(ctx, desired, diff.Hash, true) is.NoErr(err) - is.Equal(order, []string{"stop", "start"}) // fell back to a restart + is.Equal(order, []string{"stop", "start"}) // fell back to a restart + is.Equal(applied.AppliedMode, ApplyModeRestart) // ground truth: reported as restart, NOT in_place, + is.True(diff.LiveEligible()) // even though the pre-apply plan was live-eligible } // TestApplyPlanLive_ProcessorUpdate_OpenFails_RollsBack: when the new processor diff --git a/pkg/provisioning/apply_plan_live_test.go b/pkg/provisioning/apply_plan_live_test.go index d1d5147db..7c9a4cac5 100644 --- a/pkg/provisioning/apply_plan_live_test.go +++ b/pkg/provisioning/apply_plan_live_test.go @@ -157,6 +157,7 @@ func TestApplyPlanLive_RunningPipeline_StopsAppliesRestarts(t *testing.T) { is.NoErr(err) is.Equal(applied.Hash, diff.Hash) is.Equal(order, []string{"stop", "import", "start"}) + is.Equal(applied.AppliedMode, ApplyModeRestart) // ground truth: non-live-eligible change restarted } // TestApplyPlanLive_StoppedPipeline_NoLifecycleCalls confirms ApplyPlanLive @@ -182,6 +183,7 @@ func TestApplyPlanLive_StoppedPipeline_NoLifecycleCalls(t *testing.T) { applied, err := srv.ApplyPlanLive(ctx, desired, diff.Hash, false) is.NoErr(err) is.Equal(applied.Hash, diff.Hash) + is.Equal(applied.AppliedMode, ApplyModeProvisioned) // ground truth: not running, imported without disruption } // TestApplyPlanLive_Idempotent_NoOp is the regression test for AC-8's @@ -204,6 +206,7 @@ func TestApplyPlanLive_Idempotent_NoOp(t *testing.T) { applied, err := srv.ApplyPlanLive(ctx, current, diff.Hash, false) is.NoErr(err) is.True(applied.Empty()) + is.Equal(applied.AppliedMode, ApplyModeUnknown) // no mutating apply ran, so no mode is claimed } // TestApplyPlanLive_StaleHash_RefusedNoMutation_RunningPipeline is the diff --git a/pkg/provisioning/plan.go b/pkg/provisioning/plan.go index 5f0818a8e..2f9674dea 100644 --- a/pkg/provisioning/plan.go +++ b/pkg/provisioning/plan.go @@ -164,6 +164,32 @@ func (c Change) liveSwappable() bool { } } +// ApplyMode reports how ApplyPlanLive actually applied a diff — the ground +// truth of the path taken, as distinct from the pre-apply expectation a caller +// can compute from Diff.LiveEligible() (which can be wrong: a live-eligible diff +// falls back to a restart when a processor cannot be swapped live, e.g. it runs +// parallel). Consumers that report the mode to a user (the `--dev` hot-reload +// watcher) must use this, not the plan-derived guess, or they will mislabel a +// fallback restart as an in-place swap. +type ApplyMode string + +const ( + // ApplyModeUnknown is the zero value: ApplyPlanLive did not run a mutating + // apply (an error/refusal return, or an idempotent empty diff), so no mode + // was determined. Callers fall back to their own labeling. + ApplyModeUnknown ApplyMode = "" + // ApplyModeProvisioned: the pipeline was not running, so the new config was + // imported without disrupting anything (no live pipeline to swap or restart). + ApplyModeProvisioned ApplyMode = "provisioned" + // ApplyModeInPlace: every change was swapped into the running pipeline's live + // node graph — no stop, no restart, no availability blip. + ApplyModeInPlace ApplyMode = "in_place" + // ApplyModeRestart: the change required a graceful drain-and-restart of the + // running pipeline — either a non-live-eligible diff, or a live-eligible one + // that fell back because a processor could not be swapped in place. + ApplyModeRestart ApplyMode = "restart" +) + // Diff is Plan's result: every Change needed to reconcile the pipeline // currently stored with the desired config, plus a Hash binding this exact // Diff — ApplyPlan refuses to run unless the caller presents this Hash. @@ -171,6 +197,13 @@ type Diff struct { PipelineID string `json:"pipelineID"` Changes []Change `json:"changes"` Hash string `json:"hash"` + + // AppliedMode is the ground-truth path ApplyPlanLive took, set only on its + // successful mutating returns (ApplyModeUnknown otherwise). It is a runtime + // outcome, not part of the planned/persisted/transmitted diff, so it is + // deliberately excluded from JSON (and thus from the hash's hashable view, + // which lists its own fields, and from the wire proto). Plan never sets it. + AppliedMode ApplyMode `json:"-"` } // Empty reports whether the Diff has no changes, i.e. the desired config @@ -519,6 +552,7 @@ func (s *Service) ApplyPlanLive(ctx context.Context, desired config.Pipeline, ha if err := s.transactionalImport(ctx, desired); err != nil { return fresh, err } + fresh.AppliedMode = ApplyModeProvisioned return fresh, nil } @@ -540,10 +574,13 @@ func (s *Service) ApplyPlanLive(ctx context.Context, desired config.Pipeline, ha return fresh, err } if swappedAll { + fresh.AppliedMode = ApplyModeInPlace return fresh, nil } // Fall through to the restart path: the config is committed, so - // StopAndWait -> (idempotent) import -> Start rebuilds from it. + // StopAndWait -> (idempotent) import -> Start rebuilds from it. The + // reported mode is restart, not in_place — this is exactly the fallback + // AppliedMode exists to report honestly. } // Invariant 7 / Tier-1 safety: StopAndWait — not Stop — is the only @@ -578,6 +615,7 @@ func (s *Service) ApplyPlanLive(ctx context.Context, desired config.Pipeline, ha return fresh, cerrors.Errorf("pipeline %q was updated but failed to restart, it remains stopped with the new config: %w", desired.ID, err) } + fresh.AppliedMode = ApplyModeRestart return fresh, nil }