From 94e10d65d8330c35f275fe4b693193c64ae736e7 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Mon, 5 Oct 2026 17:17:29 +0100 Subject: [PATCH] Give organization deletion time for a busy tenant loop to stop 244d322dce made organization deletion refuse, and keep the data, when the tenant's monitoring loop has not exited. That is the right guard, but it reused tenantMonitorShutdownTimeout, a two-second budget that had only ever been a best-effort wait before logging and carrying on. A loop that is still starting up or finishing a tick can take longer to see its cancelled context. In E2E run 37329726585 a just-used organization's loop took 8-10 seconds, so both attempts of 13-api-token-scope's org cleanup got 503 tenant_shutdown_incomplete. Any real delete shortly after a tenant was busy hits the same refusal until a retry. Deletion now waits on its own tenantDeletionShutdownTimeout (30s) before it refuses. Process shutdown and RemoveTenant keep the two-second budget, and a loop that never exits still refuses and retains the data. Both budgets are variables so tests can shorten them; the never-stopping-loop test no longer waits on the real budget. --- .../v6/internal/subsystems/monitoring.md | 8 +++ .../guest_metadata_shutdown_test.go | 1 + .../monitoring/monitor_additional_test.go | 54 +++++++++++++++++++ internal/monitoring/multi_tenant_monitor.go | 26 +++++++-- 4 files changed, 84 insertions(+), 5 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 0e47f9e058..c0ac04025a 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1403,6 +1403,14 @@ reporting source of truth. `MultiTenantMonitor.ListOrganizationIDs` may expose persisted organization IDs to API-owned background workers, but it must not initialize monitors, start pollers, or reinterpret tenant IDs as monitored resource health. +Organization deletion waits up to `tenantDeletionShutdownTimeout` (30 seconds) +for the tenant monitoring loop to exit before it refuses with +`tenant_shutdown_incomplete` and keeps the data. Process shutdown and +`RemoveTenant` keep the two-second `tenantMonitorShutdownTimeout`. Deletion +gates the data on that wait, so the budget must cover a loop that is still +starting up or finishing a tick: under CI load a just-used organization's loop +took 8-10 seconds to see its cancelled context, and the two-second budget turned +an ordinary delete into a refusal. A loop that never exits still refuses. Proxmox physical-disk polling is also a continuity boundary. A failed or permission-denied `disks/list` call must remain an error so the monitor can use linked host-agent inventory or retain same-instance, same-node prior evidence; diff --git a/internal/monitoring/guest_metadata_shutdown_test.go b/internal/monitoring/guest_metadata_shutdown_test.go index 8c9ad302ee..074518a4b9 100644 --- a/internal/monitoring/guest_metadata_shutdown_test.go +++ b/internal/monitoring/guest_metadata_shutdown_test.go @@ -79,6 +79,7 @@ func TestTenantDeletionRetainsBlockedGuestWriterOwnership(t *testing.T) { } } func TestTenantDeletionRetainsUnfinishedMonitorLoop(t *testing.T) { + shortenTenantShutdownBudgets(t, 50*time.Millisecond, 100*time.Millisecond) mtm := NewMultiTenantMonitor(&config.Config{}, nil, nil) _, cancel := context.WithCancel(context.Background()) done := make(chan struct{}) diff --git a/internal/monitoring/monitor_additional_test.go b/internal/monitoring/monitor_additional_test.go index 15a9875f7b..eaaa70230a 100644 --- a/internal/monitoring/monitor_additional_test.go +++ b/internal/monitoring/monitor_additional_test.go @@ -1102,3 +1102,57 @@ func TestAutomaticGuestMemoryLinkNextPoll(t *testing.T) { t.Fatalf("ambiguous hostname selected %q/%q/%q", node, linkedVM, ct) } } + +// shortenTenantShutdownBudgets makes the shutdown waits fast enough for tests +// while keeping deletion's budget longer than the plain one, as in production. +func shortenTenantShutdownBudgets(t *testing.T, plain, deletion time.Duration) { + t.Helper() + previousPlain, previousDeletion := tenantMonitorShutdownTimeout, tenantDeletionShutdownTimeout + tenantMonitorShutdownTimeout, tenantDeletionShutdownTimeout = plain, deletion + t.Cleanup(func() { + tenantMonitorShutdownTimeout, tenantDeletionShutdownTimeout = previousPlain, previousDeletion + }) +} + +// A tenant monitor that is still starting up or finishing a tick can take +// longer than the plain shutdown budget to see its cancelled context. In CI a +// just-used organization's loop took 8-10s, so deletion refused with +// tenant_shutdown_incomplete. Deletion waits on its own, longer budget instead. +func TestTenantDeletionWaitsForSlowMonitoringLoop(t *testing.T) { + shortenTenantShutdownBudgets(t, 50*time.Millisecond, 2*time.Second) + + slowLoop := func(mtm *MultiTenantMonitor, orgID string) *Monitor { + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + <-ctx.Done() + // Exits well after the plain budget but inside deletion's. + time.Sleep(300 * time.Millisecond) + close(done) + }() + monitor := &Monitor{} + mtm.monitors[orgID] = monitor + mtm.tenantCancel[orgID] = cancel + mtm.tenantDone[orgID] = done + return monitor + } + + mtm := NewMultiTenantMonitor(&config.Config{}, nil, nil) + t.Cleanup(mtm.Stop) + + // RemoveTenant keeps the plain budget, gives up and retains the owner. + retained := slowLoop(mtm, "removal") + mtm.RemoveTenant("removal") + if got, ok := mtm.PeekMonitor("removal"); !ok || got != retained { + t.Fatal("plain removal discarded a monitor whose loop had not stopped") + } + + slowLoop(mtm, "deletion") + if err := mtm.BeginTenantDeletion("deletion"); err != nil { + t.Fatalf("deletion refused a loop that stopped inside its budget: %v", err) + } + if _, ok := mtm.PeekMonitor("deletion"); ok { + t.Fatal("completed deletion left the monitor registered") + } + mtm.FinishTenantDeletion("deletion") +} diff --git a/internal/monitoring/multi_tenant_monitor.go b/internal/monitoring/multi_tenant_monitor.go index 248b842239..a11950ad1f 100644 --- a/internal/monitoring/multi_tenant_monitor.go +++ b/internal/monitoring/multi_tenant_monitor.go @@ -47,7 +47,19 @@ func NewMultiTenantMonitor(baseCfg *config.Config, persistence *config.MultiTena } } -const tenantMonitorShutdownTimeout = 2 * time.Second +// Both shutdown budgets are variables so tests can shorten them. +var ( + // tenantMonitorShutdownTimeout bounds how long process shutdown and + // RemoveTenant wait for a tenant's monitoring loop to exit. + tenantMonitorShutdownTimeout = 2 * time.Second + + // tenantDeletionShutdownTimeout bounds how long organization deletion + // waits for the loop. Deletion refuses, and keeps the data, while the loop + // still runs, so this is a correctness gate rather than a best-effort + // wait: a loop that is still starting up or finishing a tick can take + // several seconds to see its cancelled context. + tenantDeletionShutdownTimeout = 30 * time.Second +) // SetRecoveryManager wires a recovery store manager into all existing and future tenant monitors. func (mtm *MultiTenantMonitor) SetRecoveryManager(manager *recoverymanager.Manager) { @@ -377,7 +389,11 @@ func (mtm *MultiTenantMonitor) removeTenant(orgID string, deleting bool) error { if cancel != nil { cancel() } - if !waitForTenantMonitorShutdown(orgID, done) { + shutdownTimeout := tenantMonitorShutdownTimeout + if deleting { + shutdownTimeout = tenantDeletionShutdownTimeout + } + if !waitForTenantMonitorShutdown(orgID, done, shutdownTimeout) { return fmt.Errorf("organization %q monitoring loop has not stopped", orgID) } if monitor != nil { @@ -397,19 +413,19 @@ func (mtm *MultiTenantMonitor) removeTenant(orgID string, deleting bool) error { return nil } -func waitForTenantMonitorShutdown(orgID string, done <-chan struct{}) bool { +func waitForTenantMonitorShutdown(orgID string, done <-chan struct{}, timeout time.Duration) bool { if done == nil { return true } - timer := time.NewTimer(tenantMonitorShutdownTimeout) + timer := time.NewTimer(timeout) defer timer.Stop() select { case <-done: return true case <-timer.C: - logger := log.Warn().Dur("timeout", tenantMonitorShutdownTimeout) + logger := log.Warn().Dur("timeout", timeout) if orgID != "" { logger = logger.Str("org_id", orgID) }