Skip to content

Phase 12.J.E.8 — WaitForStatus timeout env var (audit Risk A) + _clean field cleanup

Placeholder ppxd requested to merge phase12.J.E.8-start-timeout-and-cleanup into main

Summary

Closes audit Risk A identified in PR #201's deep review: heavyweight agents (heavy plugin enumeration / .NET tiered JIT cold start / AV scanning a 50MB binary) can take >30s to reach SCM RUNNING state. The Phase B Start-Service WaitForStatus is inside J.E.6's try/catch that triggers Invoke-Rollback → false rollback on a slow-but-successful upgrade.

Fix: operator-tunable env var following the established Rule 8 pattern.

# heavyweight agent (3min plugin enumeration on first start)
export SQUID_TARGET_WINDOWS_TENTACLE_SERVICE_TIMEOUT_SECONDS=240

Single var covers all 4 WaitForStatus call sites; default 30s preserves pre-PR behaviour exactly.

Production changes

WindowsTentacleUpgradeStrategy

  • New public const ServiceTimeoutSecondsEnvVar
  • New internal const DefaultServiceTimeoutSeconds = 30
  • ResolveServiceTimeoutSeconds() — matches sibling Resolve* shape (positive-integer parse + invalid-fallback-with-warning)
  • {{SERVICE_TIMEOUT_SECONDS}} placeholder wired into RenderInnerScript

upgrade-windows-tentacle.ps1

  • $SERVICE_TIMEOUT_SECONDS = {{SERVICE_TIMEOUT_SECONDS}} numeric literal
  • $SERVICE_TIMEOUT_SPAN = [TimeSpan]::FromSeconds($SERVICE_TIMEOUT_SECONDS) constructed once at top (avoids 4× construction per upgrade)
  • All 4 hardcoded '00:00:30' replaced with $SERVICE_TIMEOUT_SPAN:
    1. Phase B Stop-Service
    2. Phase B Start-Service (the audit-Risk-A target)
    3. Invoke-Rollback Stop new
    4. Invoke-Rollback Restart old

Unit tests (6 new)

Test What it pins
ServiceTimeoutSecondsEnvVar_ConstantNamePinned Rule 8 — silent rename breaks operators
ResolveServiceTimeoutSeconds_NoEnvVar_ReturnsDefault30 Default preserves pre-PR behaviour
ResolveServiceTimeoutSeconds_ValidValue_RoundTripsAsInteger (Theory) 60 / 180 / 3600 / whitespace-padded
ResolveServiceTimeoutSeconds_InvalidValue_FallsBackToDefault30 (Theory) 0 / -30 / "30s" / non-numeric / empty — including the common typo where operators write a unit suffix
RenderInnerScript_ServiceTimeoutSecondsPlaceholder_SubstitutedFromEnv Substitution + reverse-pin ("'00:00:30'" stale literal must NOT remain)
RenderInnerScript_ServiceTimeoutSpanIsTimeSpanInstance Pins the TimeSpan construction shape — a "simplification" that passes int to WaitForStatus (which expects TimeSpan) would crash at runtime

Drift detector updates

  • Unit: RenderInnerScript_PlaceholderTokens_AppearExactlyOnceInTemplate extended
  • E2E: UpgradeScript_PlaceholderSet_PinnedToProductionContract extended

Bonus: _clean field finally has a purpose

Pre-this-PR _clean was assigned but never read (CS0414 warning since J.E.3). Now used in Dispose() to emit a diagnostic Console.WriteLine when cleanup runs without MarkClean having fired (= test failed before happy-path completion). Useful when reading CI logs to disambiguate "test passed but cleanup failed" from "test failed → MarkClean never reached".

Local verification

  • 262/262 unit tests pass
  • 87/87 cross-platform E2E pass

Deferred

The 5th env var triggers consideration of an IUpgradeOverride[] refactor (Rule 7). Currently:

  1. DownloadBaseUrlEnvVar
  2. HealthcheckUrlEnvVar
  3. HealthcheckRetriesEnvVar
  4. HealthcheckFatalEnvVar
  5. ServiceTimeoutSecondsEnvVar ← THIS PR

Deferred until 6th — refactoring at exactly the threshold is over-eager.

🤖 Generated with Claude Code

Merge request reports