Skip to content

H4: Enrich upgrade-lock contention message with dispatch metadata

Placeholder ppxd requested to merge feat/upgrade-hardening-h4-lock-ttl-idempotency into main

Summary

H4 of the 1.8.0 Upgrade Hardening initiative (H1 #354 + H2 #355 + H3 #356 already merged).

Fixes the misleading contention UX from the operator-reported "之前點過幾次 upgrade 後就這樣了" symptom. When two dispatch attempts race, the second got a hardcoded "wait under 2 minutes" message — which was both wrong (worst case is LockExpiry=7min) and uninformative (no start time, no target version).

Behaviour change

Click sequence Pre-H4 message Post-H4 message
Operator double-clicks Upgrade, second click hits lock contention "Machine 'X' is currently being upgraded by another request. Wait for it to complete (typically under 2 minutes) and retry." "Machine 'X' is currently being upgraded by another request (dispatched at 12:34:56 UTC, ~45s ago, targeting 1.8.0). Expected to complete by 12:41:56 UTC (~415s remaining). Retry after that, or contact your operator if it appears genuinely stuck."
Same, but metadata read fails (Redis hiccup) Same wrong "2 minutes" message Falls back to generic message with correct LockExpiry (7 min, not the pre-H4 fib)

Components

  • IUpgradeDispatchMetadataStore (new) — companion Redis key squid:upgrade:machine:{id}:meta (derived from UpgradeDispatchLockReconciler.BuildLockKey + :meta suffix; drift detector pinned). Same TTL as the lock so crashed dispatcher's metadata is auto-cleaned by Redis — no separate expiry bookkeeping needed.
  • UpgradeDispatchMetadata DTO — stable camelCase JSON shape (matches H2/H3 convention)
  • MachineUpgradeService.DispatchUnderLockAsync — wraps RunStrategyAsync in try/finally that writes metadata before strategy + deletes after release
  • BuildContentionMessageAsync — reads metadata on contention path; falls back gracefully when unavailable

Why not a periodic Redis sweep job?

Considered but rejected — over-engineering for marginal value:

  • RedLockNet already auto-expires the lock at LockExpiry=7min
  • The existing UpgradeDispatchLockReconciler already clears stale locks reactively on health check (10-min staleness threshold per ShouldClearLockForStatus)
  • A periodic SCAN would add Redis load + a new failure surface (Hangfire job crash) for at-most a 7-min wait improvement
  • Genuinely-stuck locks are extremely rare given existing TTL + reactive reconciler

If H8's E2E matrix surfaces a real recurring stuck-lock case, a follow-up PR can add the sweep.

Test plan

  • +5 UpgradeDispatchMetadataTests (key format, derivation pattern, JSON shape, round-trip, backward-compat)
  • +2 enriched contention scenarios (with metadata; fallback when null)
  • Existing 64 MachineUpgradeService tests continue to pass
  • 5561/5561 unit tests green (vs 5554 baseline → +7 net new)
  • Integration with real Redis: deferred to H8 (E2E matrix exercises full dispatch round-trip)

Backward compatibility

  • MachineUpgradeService constructor adds optional IUpgradeDispatchMetadataStore parameter (defaults to null). Existing test fixtures + DI configs without the store continue to work — service degrades to the pre-H4 generic fallback message.
  • Existing contention test (UpgradeAsync_LockAcquisitionFails_ReturnsFailedWithRetryHint) continues to pass — it uses the constructor that doesn't pass dispatchMetadata, exercising the fallback path.

What's NOT in this PR

  • H5: Cross-OS unified upgrade pipeline (next)
  • H6: Agent rollback + abandonment recovery
  • H7: Role/feature capability slots
  • H8: Comprehensive E2E test matrix

Merge request reports