mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-10-26 21:47:20 +00:00
rcourtman
ff984d19e2
`TestGuestWindowsMeminfoPollingContract` failed intermittently on the loaded `pulse-dev` box. Nothing in the polling logic was at fault: the fixture owned three wall-clock windows a starved machine overruns, and each failure cascades into the later assertions. This removes them. The change is test-only (two files). ## What raced 1. **A one second HTTP budget.** The real `proxmox` clients were built with `Timeout: time.Second` against a loopback server. The guest-agent guard defers a poll whose lock check fails (`lock-unverified`) and fences the guest for a minute after a command times out or its body is cut short (`agent-timeout`, `agent-response-incomplete`, then `agent-cooldown`). One late reply therefore broke the read counts and History the later assertions expect, or deferred every poll after it. The fixture now sets only hang guards, including the monitor's guest-agent timeouts it used to leave at zero (they silently fell back to 10s defaults and logged a warning per call). 2. **A one second History window.** `recordGuestMetrics` was passed `time.Now().Add(-time.Second)` *after* the build, `UpdateVMs` and `updateResourceStore`. A guest whose `LastSeen` precedes that cycle start is skipped (`guestObservedInCycle`), so a slow tail silently dropped the sample and History lost a point (poll 1 in three of the four cases I saw, poll 3 in the other). The poll loop takes `cycleStart` before building (`monitor_pve_guest_poll.go:50`); the fixture now does the same. 3. **Two timers in the shared History fixture** (`guest_history_observation_test.go`). `WaitForMaintenance(5 * time.Second)` now waits on the maintenance barrier itself (`WaitForMaintenance(0)`), and stored points are read back over the last hour instead of the last minute (the range still selects the raw tier). I checked the other users of the helper: none writes backdated points or relies on older points being excluded, and the whole `internal/monitoring` package passes. One lock check in `client/cluster` is answered 1.2s late, so the one second budget cannot come back unnoticed. That adds 1.2s to the test. ## Evidence Measured on `pulse-dev` (8 cores, load average 50-100, memory and I/O pressure high), each run a separate process, against the unmodified test: | run | variant | failures | | --- | --- | --- | | 40 runs, median load 80 | unmodified | 7 | | 40 runs, same window | deadline + cycle-start fix (without the fixture timers) | 0 | | 25 interleaved runs | unmodified | 3 | | 25 interleaved runs | first draft of this fix, deadline only | 3 | The unmodified failures had the signatures from the report: `prev-lock-unverified` with `fs=2` and `History = [30 0]` or `[30 40]`, `normal OS observation absent`, `agent-timeout` followed by `agent-cooldown`, and a History missing its first point. The first draft passing only partly is what exposed windows 2 and 3. Negative controls, deterministic at any load: - Restoring only the one second `Timeout`: fails every time (40 of 40, and again on the final code) with `poll 2 ... prev-lock-unverified ... fs=2`, `Windows disk History = [30 0], want [30 40 0]`, then the backup-lock and resume assertions. - Restoring the `now-1s` window with a 1.2s tail before recording: History is empty for every metric. The same tail with `cycleStart` passes. - A point written 90 seconds earlier is invisible to the old one minute query window and returned by the new one (scratch test, not committed). Checked on the final code (on `4471dcaaba`, before the comment-only edits that followed): the whole `internal/monitoring` package passes. I did **not** get a heavy-load window for the final build: the box's load fell to about 1 shortly after, so the final code was verified by the controls and the package run, not by a repeat of the 40-run comparison. ## What still can fail Two limits remain that the fixture cannot set: the proxmox transport allows ten seconds to dial and to receive response headers (`pkg/tlsutil/fingerprint.go`, `dnscache.go`), and a cluster client probes a new endpoint within three seconds (`cluster_client.go`). Both are production policy. Emulating a host stall by suspending the test process for 12 seconds, with the late reply still armed in all four subtests, made the fixed test fail 7 of 30 times, all at that lock check; a 3 second suspension passed 30 of 30 for both builds, since the test spends most of its time sleeping. That is also why the late reply is armed in one subtest, not four: the four share a client budget and each armed one widens the window a stall can land in. ## Found, not changed here The same `recordGuestMetrics(..., time.Now().Add(-time.Second))` window is in eight call sites across five sibling tests: `guest_history_observation_test.go` (4), `guest_filesystem_completeness_test.go`, `guest_filesystem_identity_test.go`, `guest_agent_optional_read_integration_test.go`, `guest_disk_expiry_test.go`. `monitor_pve_guest_cpu_authority_test.go:29` has its own `WaitForMaintenance(5 * time.Second)`. They share the latent race but are outside this contract test.