mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-10-24 04:33:17 +00:00
rcourtman
690d0ef31e
## Problem
244d322dce, merged at 15:01:08 UTC today in #2518, made organization
deletion refuse with `503 tenant_shutdown_incomplete`, and keep the
data, when the tenant's monitoring loop hasn't exited. That guard is
right, but it reused `tenantMonitorShutdownTimeout`. That 2-second
budget dates from March, when it was only a best-effort wait before
logging a warning and carrying on.
A loop that is still starting up or finishing a tick can take longer
than that to see its cancelled context. The observed case is mock mode,
which CI uses: `Monitor.Start` runs `startMockMetricsSampler`
synchronously before its select loop, and `prepareMockMetricsHistory`
seeds seven days of fixture history without a context. The loop only
sees cancellation once seeding finishes; "Mock metrics history sampler
started" and "monitoring loop stopped" log in the same second. E2E run
[37329726585](https://github.com/rcourtman/Pulse/actions/runs/37329726585/job/111833069435),
shard 7, shows it:
| Time (UTC) | Event |
|---|---|
| 15:27:57 | Organization A's monitor initializes |
| 15:28:00 | Delete requested |
| 15:28:02 | `timed out waiting for tenant monitor loop to exit` → 503 |
| 15:28:10 | `monitoring loop stopped` |
| 15:28:20 → 15:28:29 | The retry, with a new organization, repeats the
same sequence |
So `13-api-token-scope` "org-bound token stays inside the issuing org"
failed its org cleanup on both attempts. Run 37331626618 failed the same
way: the first Core E2E failure came two minutes after #2518 merged. A
real delete shortly after a tenant was busy gets the same refusal, with
its alarming "data has been retained" message, until someone retries.
## Change
- **New budget:** `removeTenant` waits on
`tenantDeletionShutdownTimeout` (30s) when deleting. Process `Stop` and
`RemoveTenant` keep the 2-second `tenantMonitorShutdownTimeout`.
- **Still refused:** a loop that never exits, and a failed metadata
close, both still refuse and retain the data. The guard's semantics
don't change.
- **Tests:** both budgets are now variables so tests can shorten them.
`TestTenantDeletionRetainsUnfinishedMonitorLoop` uses a short deletion
budget instead of the real 30 seconds.
- **No timeout conflicts:** the server's `WriteTimeout` is disabled, the
frontend's org delete has no request timeout, and Playwright's request
timeout is 30s.
## Verification
- **New test:** `TestTenantDeletionWaitsForSlowMonitoringLoop` (in the
registered `monitor_additional_test.go`) uses a loop that exits after
300ms, with a 50ms plain budget and a 2s deletion budget. Plain removal
gives up and retains the monitor; deletion waits and completes. Giving
deletion the plain budget makes it fail, so it checks the behaviour
itself.
- **Existing tests:** the deletion safety tests in `internal/monitoring`
(`TestTenantDeletion*`, `TestMultiTenantMonitor*`) and the org-delete
handler tests in `internal/api` all pass, including
`TestOrgDeleteRetainsDataWhenGuestMetadataPersistenceFails`.
**Follow-up in a separate PR:** another session is making the mock
history seeding honour cancellation
(`internal/monitoring/mock_metrics_history.go`), so CI deletions finish
almost at once instead of waiting out the seeding. This PR keeps a
realistic deletion budget either way.