elastic / elastic/elastic-agent

[upgrade-lifecycle] Watcher skips monitoring when system clock moves backward after upgrade marker write

Open
#14,545 2 comments 0 reactions 0 assignees View on GitHub
bug Team:Elastic-Agent-Control-Plane
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.