elastic / elastic/elastic-agent
[upgrade-lifecycle] Watcher skips monitoring when system clock moves backward after upgrade marker write
- Dominant language
- Go
- Stars
- 275
- Forks
- 264
- Avg merge
- 1d 23h
- Merged PRs (30d)
- 312
Description
## Findings
### 1. Grace-period calculation treats negative elapsed time as outside grace and exits watcher early (**HIGH**)
**Priority:** P1 (common upgrade path risk; impacts Fleet/CLI upgrades on all platforms when time is corrected backward)
**Platform(s):** Linux, Windows, macOS (shared watcher command path)
**Location**
- `internal/pkg/agent/cmd/watch.go:192-229`
- `internal/pkg/agent/cmd/watch.go:370-377`
**Evidence**
- `gracePeriod()` computes `sinceUpdate := time.Since(marker.UpdatedOn)` and only returns in-grace when `0 < sinceUpdate && sinceUpdate < gracePeriodDuration`.
- Any `sinceUpdate <= 0` returns `false` (`return false, gracePeriodDuration`).
- `watchCmd()` treats `!isWithinGrace` as stale/finished and immediately runs cleanup + exits (`Cleanup(..., true, ...)` and `return nil`).
**What is wrong**
If host time is stepped backward after marker creation (NTP/manual correction), `time.Since(marker.UpdatedOn)` becomes negative. The watcher then decides it is outside grace, skips health monitoring, and exits before the grace window is actually elapsed.
**Why it matters**
This disables rollback protection during normal upgrades under realistic clock adjustments. A newly upgraded agent that fails shortly after startup can go unrolled back because monitoring has already stopped and the marker may be cleaned up.
**Suggested fix direction**
- Treat `sinceUpdate <= 0` as still within grace (clamp to zero elapsed), e.g.:
- `elapsed := time.Since(marker.UpdatedOn)`
- `if elapsed < 0 { elapsed = 0 }`
- then apply grace comparison.
- Keep cleanup/early-exit behavior only for true elapsed-out-of-grace states.
**Test gap and failing-test direction**
`watch_test.go` currently exercises current/past timestamps but not a future `UpdatedOn` (negative elapsed). Add a case where:
1. `marker.UpdatedOn = time.Now().Add(+2 * time.Minute)`
2. `cfg.GracePeriod = 5 * time.Minute`
3. Expect watcher path to run (no immediate cleanup branch), i.e., no premature `Cleanup(..., true, false, ...)` call due solely to negative elapsed.
## Upgrade paths audited and found safe in this run
- Marker write/read atomicity and corruption fallback paths in `internal/pkg/agent/application/upgrade/step_mark.go` + marker access helpers.
- Watcher PID/connection loss thresholds in `internal/pkg/agent/application/upgrade/watcher.go`.
- Rollback symlink/cleanup TTL preservation flow in `internal/pkg/agent/application/upgrade/rollback.go` and `cleanup.go`.
- Download retry clamp and bounded retry window in `internal/pkg/agent/application/upgrade/artifact/config.go` and `step_download.go`.
---
[What is this?](https://ela.st/github-ai-tools) | [From workflow: Sweeper: Upgrade and Rollback Lifecycle](https://github.com/elastic/elastic-agent/actions/runs/26446607779)
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
> - [x] expires on Jun 2, 2026, 10:36 AM UTC
Contributor guide
Research direction
Start in internal/pkg/agent/cmd/watch.go, especially gracePeriod() and watchCmd(), and inspect the existing cases in watch_test.go. Run the watcher tests, then add coverage for a future marker UpdatedOn with a five-minute grace period. Done means negative elapsed time follows the in-grace watcher path and does not trigger premature cleanup.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- release
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100