Skip to content

1.8.0 hardening review followups — fix H4 clock skew + backfill H7 test gaps

Placeholder ppxd requested to merge fix/hardening-review-followups into main

Summary

Post-merge self-audit of the 1.8.0 Upgrade Hardening initiative (8 PRs #354-#361) surfaced one real cosmetic bug and three test-coverage gaps that would not have caught a future regression. This PR addresses both in one go for 1.8.1.

🔴 Bug fix — H4 clock-skew negative elapsed

File: src/Squid.Core/Services/Machines/Upgrade/MachineUpgradeService.cs

BuildContentionMessageAsync was clamping remaining with Math.Max(0, ...) but the symmetric guard was missing for elapsed:

- $"... ~{elapsed.TotalSeconds:F0}s ago ... ~{Math.Max(0, remaining.TotalSeconds):F0}s remaining)"
+ $"... ~{Math.Max(0, elapsed.TotalSeconds):F0}s ago ... ~{Math.Max(0, remaining.TotalSeconds):F0}s remaining)"

Failure scenario: Two server pods disagree on UTC by seconds-to-minutes (NTP slew / fresh-pod startup before time-sync converges). Pod B reading metadata pod A wrote sees DispatchedAt in B's future → elapsed turns negative → operator sees the cosmetically wrong "~-3599s ago" in the contention message.

Cosmetic only — dispatch itself worked correctly. But operator UX regression. New test UpgradeAsync_Contention_DispatchedAtInFuture_ClockSkewGuarded_NoNegativeSeconds simulates 5-min skew and pins the guard.

⚠️ H7 test gaps backfilled

Gap 1 — CapabilityKeys.Role.* constants not pinned

Shell / Bin / Privilege namespaces each had a Theory-based pinning test in CapabilityKeysConstantsTests. The Role namespace shipped in 1.8.0 without one. A rename Role.IIS → Role.Iis OR a downgrade "role:iis" → "role:IIS" would have been a silent contract break with the agent's lowercase emission.

→ Added Theory pinning the 4-tuple (IIS / Docker / Nginx / Systemd).

Gap 2 — TentacleHealthCheckStrategy cache test missing installedRoles in metadata

The wire chain is: agent emits metadata["installedRoles"] → server reads into MachineRuntimeCapabilities.InstalledRoles → MachineCapabilitySet projects role:* slots → CapabilityValidator catches missing IIS at plan-time. If BuildCapabilitiesFromResponse's read of the installedRoles key broke, the whole chain silently optimistic-allows.

→ Added CheckHealth_InstalledRolesInMetadata_PopulatedIntoCache test verifying the read-side wire contract.

Gap 3 — MachineRuntimeCapabilitiesPersistence JSON shape didn't pin installedRoles

The DTO had the field but the serialisation/deserialisation tests didn't assert it appears in the canonical JSON. PascalCase regression OR [JsonPropertyName] drift would have caused cross-version hydration to silently skip roles.

→ Added installedRoles assertion to the existing serialisation + deserialisation tests. New Deserialise_PreH7Blob_WithoutInstalledRoles_BackwardCompatible test verifies 1.8.0 blobs (no installedRoles field) still deserialise with null on H7-aware servers — the in-place upgrade path from 1.8.0 to 1.8.1.

Test plan

  • +1 H4 clock-skew guard regression test
  • +4 H7 Role constant pinning (Theory rows)
  • +1 H7 cache-population read-side test with installedRoles
  • +1 H7 backward-compat deserialisation test
  • Modified 2 existing serialisation tests to pin installedRoles
  • 5603/5603 unit tests green (+7 net new vs 5596 baseline post-H8)
  • Operator-side: no operator-visible behaviour change beyond cosmetic contention-message fix (1.8.0 dispatches succeeded already)

Acceptance criteria

After 1.8.1 ships, this 8-PR initiative chain has BOTH:

  1. Behavioural correctness pinned by H1-H8 + this fix (the operator's 1.7.x failure chain is structurally impossible to reproduce)
  2. Contract stability pinned by the new test additions (a future refactor can't silently break the agent ↔️ server wire format without a test-time-visible decision)

What's NOT in this PR

  • Other minor test gaps mentioned in audit (e.g. LoadAllAsync mixed corrupted/valid rows, BuildMetadataDictionary keys symmetry) — deferred to a separate audit-coverage PR if needed. The three highest-value gaps (wire-shape stability for the new H7 installedRoles field) are the ones that would silently regress the operator's IIS deploy plan-time validation if not pinned.

Merge request reports