fix: clear upgrade_started_at when agent upgrades faster than one checkin interval

[ycombinator]

What is the problem?

When an Elastic Agent upgrades faster than one checkin interval (~60 seconds), Fleet Server never observes any intermediate upgrade_details states (downloading, extracting, watching). The agent completes its upgrade and restarts at the new version within a single checkin gap. In practice, this is unlikely to happen with an actual Elastic Agent. But it is possible with Horde drones and, in theory, it could happen with Elastic Agents too.

In this case:

• upgrade_started_at is set on the agent document (written by Kibana's bulk_upgrade API) when the upgrade action is dispatched;
• upgrade_details is null on the agent document;
• The agent checks in with upgrade_details: null in the checkin body.

processUpgradeDetails calls markUpgradeComplete when the checkin body has upgrade_details: null. However, markUpgradeComplete had an early-return guard:

if agent.UpgradeDetails == nil {
    return nil
}

Since agent.UpgradeDetails is also nil (no intermediate states were ever stored), this was a no-op. upgrade_started_at was never cleared, leaving the agent permanently in the "updating" state in Fleet UI — even though it had successfully upgraded to the target version.

This was observed in a 30,000-agent scale test (build #6981) where one agent (eh-ThoseChillwave-LHTb) remained stuck in "updating" state, blocking Step10 from converging to online == 30,000.

How does this PR solve the problem?

The NOP guard is extended to also require upgrade_started_at to be empty:

if agent.UpgradeDetails == nil && agent.UpgradeStartedAt == "" {
    return nil
}

If upgrade_started_at is set but upgrade_details is nil, an upgrade was dispatched (by Kibana) but no intermediate states were ever recorded by Fleet Server. When the agent then checks in with upgrade_details: null, Fleet Server now correctly clears upgrade_started_at, sets upgraded_at, and marks the upgrade complete — regardless of whether any intermediate upgrade_details states were observed.

How to test locally

1. Start a Fleet Server with an enrolled agent.
2. Trigger an upgrade via Kibana's POST /api/fleet/agents/bulk_upgrade. This sets upgrade_started_at on the agent document.
3. Before the agent sends any checkin with upgrade_details, manually set upgrade_details: null and the new version on the agent document (simulating a fast upgrade).
4. Send a checkin with upgrade_details: null (the default for a running, non-upgrading agent).
5. Verify that Fleet Server clears upgrade_started_at and sets upgraded_at on the agent document.

Alternatively, run the unit tests:

go test ./internal/pkg/api/ -run TestProcessUpgradeDetails -v

The new test case "agent has upgrade_started_at but no upgrade_details, checkin details are nil (fast upgrade race)" covers this scenario.

Design Checklist

• The code is stateless (no local caching that could cause inconsistency between instances)
• The code has been tested with more than 100K agents
• There are fail safes in the code (e.g. timeouts, max values, graceful degradation)

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding changes to the tests
• I have added an entry in ./changelog/fragments using the changelog tool

[mergify]

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit
• backport-active-all is the label that automatically backports to all active branches.
• backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
• backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

[Copilot]

🟡 Changes recommended

The newly added unit test matcher can pass even if upgraded_at is missing, reducing the reliability of the regression test.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a fast-upgrade race where an Elastic Agent can finish upgrading within a single check-in interval, leaving upgrade_started_at stuck on the agent document and the agent permanently shown as “updating” in Fleet UI.

Changes:

• Adjusts markUpgradeComplete to treat upgrade_started_at without upgrade_details as a completed upgrade and clear upgrade fields accordingly.
• Adds a unit test covering the “fast upgrade race” scenario.
• Adds a changelog fragment describing the bug and fix.

File summaries

File	Description
internal/pkg/api/handleCheckin.go	Extends the no-op guard so upgrade_started_at is cleared even when no upgrade_details were ever persisted.
internal/pkg/api/handleCheckin_test.go	Adds a test case covering the fast-upgrade scenario (with one matcher issue noted in review).
changelog/fragments/1788912764-fix-upgrade-started-at-stuck-fast-upgrade.yaml	Documents the bug and the behavior change in a release fragment.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The updated guard can incorrectly mark upgrades complete on the first check-in after bulk_upgrade (before the agent receives the upgrade action), potentially clearing upgrade_started_at prematurely.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new NOP guard can prevent self-healing of already-stuck agents (including during mixed-version rolling upgrades), leaving upgrade_started_at stuck indefinitely in some deployments.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The functional change is narrowly scoped, aligns with the described failure mode, and is covered by targeted unit tests.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new staleness/self-heal logic can incorrectly mark upgrades complete (including setting upgraded_at) without evidence of a successful upgrade, and the configured staleness threshold/comment mismatch needs correction.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

internal/pkg/api/handleCheckin.go:804

• markUpgradeComplete sets upgraded_at even when there’s no evidence the upgrade actually completed (no stored upgrade_details and ver == ""). In the staleness branch this can incorrectly mark an upgrade as successful for agents that simply haven’t received the upgrade action yet (e.g., offline/delayed), producing a false upgraded_at timestamp.

	doc := bulk.UpdateFields{
		dl.FieldUpgradeDetails:   nil,
		dl.FieldUpgradeStartedAt: nil,
		dl.FieldUpgradedAt:       time.Now().UTC().Format(time.RFC3339),
	}

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new staleness threshold logic can become too small (or zero) with certain CheckinMaxPoll configurations, potentially clearing upgrade_started_at prematurely.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/pkg/api/handleCheckin.go:651

• The comment references upgradeStartedAtStalenessThreshold, but there is no such identifier in this file; the implementation uses stalenessThreshold(). This makes the doc misleading for future maintainers.

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new staleness guard currently treats an unparseable upgrade_started_at as a NOP (preventing self-heal) and the PR description doesn’t reflect the added staleness-clearing behavior.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

internal/pkg/api/handleCheckin.go:652

• The comment mentions bypassing the guard when upgrade_started_at is older than "upgradeStartedAtStalenessThreshold", but that identifier doesn't exist here (the logic uses ct.stalenessThreshold()). Update the comment to match the actual implementation to avoid confusion.

This issue also appears in the following locations of the same file:

• line 775
• line 781

internal/pkg/api/handleCheckin.go:787

• In the staleness guard, a parse failure of upgrade_started_at currently causes an early return, which can leave agents permanently stuck in "updating" if the timestamp is malformed (no self-heal path). Treat an unparseable value as stale so upgrade_started_at is cleared (without setting upgraded_at).

		t, err := time.Parse(time.RFC3339, agent.UpgradeStartedAt)
		if err != nil {
			t, err = time.Parse(time.RFC3339Nano, agent.UpgradeStartedAt)
		}
		if err != nil || time.Since(t) < ct.stalenessThreshold() {
			return nil
		}

internal/pkg/api/handleCheckin.go:780

• The PR description says the fix is extending the NOP guard, but the implementation also adds a new "stale upgrade_started_at" self-heal path (clearing after 2× CheckinMaxPoll). If this behavior is intended, it should be called out explicitly in the PR description and release notes; otherwise consider removing it to keep the change narrowly scoped.

	// Exception: if upgrade_started_at is clearly stale (older than 2× CheckinMaxPoll), clear it
	// to self-heal agents stuck in the updating state after a rolling fleet-server upgrade where
	// the version was already updated by an older instance. The threshold uses 2× CheckinMaxPoll
	// so a legitimately long-running poll (up to CheckinMaxPoll) cannot be mistaken for staleness.
	// In the stale case upgraded_at is NOT set because the upgrade outcome is unknown.
	if agent.UpgradeDetails == nil && agent.UpgradeStartedAt != "" && ver == "" {

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new staleness self-heal path currently becomes a no-op on upgrade_started_at parse errors, which can still leave agents stuck in “updating” indefinitely.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

internal/pkg/api/handleCheckin.go:652

• The comment references upgradeStartedAtStalenessThreshold, but no such identifier exists in this file; it looks like the intended reference is the stalenessThreshold() logic below. This can confuse future readers when debugging upgrade state transitions.

internal/pkg/api/handleCheckin.go:787

• In the staleness self-heal branch, an unparseable upgrade_started_at currently triggers an early return, which can leave the agent stuck in an "updating" state indefinitely. Since this branch is explicitly intended to self-heal stuck agents, consider treating parse failures as stale and clearing the field.

		t, err := time.Parse(time.RFC3339, agent.UpgradeStartedAt)
		if err != nil {
			t, err = time.Parse(time.RFC3339Nano, agent.UpgradeStartedAt)
		}
		if err != nil || time.Since(t) < ct.stalenessThreshold() {
			return nil
		}

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The behavior change is well-scoped, covered by new unit tests, and only a minor doc-comment wording nit was found.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/pkg/api/handleCheckin.go:652

• The comment mentions upgradeStartedAtStalenessThreshold, but that identifier doesn’t exist; the staleness guard is implemented via stalenessThreshold(). Updating the wording avoids confusion for future readers.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

TL;DR

E2E Test failed on two subtests: TestOpAMPWithUpstreamCollector and TestAgentGracefulForceUnenroll. The first is a known flaky path that builds upstream OTel Collector from source inside a 5-minute test window; the second failed without assertion text in the provided log, so its precise assertion point is not observable from this artifact alone.

Remediation

• Re-run the Buildkite E2E step once to confirm flake behavior, with priority on TestOpAMPWithUpstreamCollector (already tracked as flaky in [Flaky Test] TestStandAloneRunningSuite/TestOpAMP signal: killed #6590).
• For durable stability, avoid in-test upstream source build in testing/e2e/stand_alone_test.go (prebuilt/pinned collector artifact or cache strategy) and capture full verbose output (go test -v + subtest logs) for TestAgentGracefulForceUnenroll to identify its exact timeout/assertion boundary.

Investigation details

Root Cause

I verified one confirmed flaky-test pattern and one inconclusive test assertion due limited logs:

1. TestStandAloneRunningSuite/TestOpAMPWithUpstreamCollector (test flakiness / infrastructure-sensitive)

   • Source path performs external network + build work in-test:
     • testing/e2e/stand_alone_test.go:675 clones https://github.com/open-telemetry/opentelemetry-collector-contrib
     • testing/e2e/stand_alone_test.go:684 runs make otelcontribcol
     • test-wide context is 5*time.Minute at testing/e2e/stand_alone_test.go:653
   • This makes the subtest sensitive to transient network/module/download/build slowness and is consistent with prior detective finding on this PR.

2. TestStandAloneRunningSuite/TestAgentGracefulForceUnenroll (test failure, assertion point not present in provided artifact)

   • The subtest failed after 221.29s, but the supplied Buildkite log excerpt contains only the subtest summary line and no require/assertion message or panic stack.
   • The test itself includes several long Eventually waits (2–3 minutes) and external process orchestration (elastic-agent download/run + Kibana/ES status transitions), so timeout-style failure is plausible, but I cannot assert the exact failing check without the missing assertion output.

PR scope check: this PR changes only internal/pkg/api/handleCheckin.go, internal/pkg/api/handleCheckin_test.go, and a changelog fragment. Neither failing e2e test is directly edited by this PR.

Evidence

• Build: https://buildkite.com/elastic/fleet-server/builds/16712
• Job/step: E2E Test (.buildkite/scripts/e2e_test.sh)
• Key log excerpt:
  • --- FAIL: TestStandAloneRunningSuite/TestAgentGracefulForceUnenroll (221.29s)
  • --- FAIL: TestStandAloneRunningSuite/TestOpAMPWithUpstreamCollector (300.68s)
• Flaky tracking:
  • [Flaky Test] TestStandAloneRunningSuite/TestOpAMP signal: killed #6590 ([Flaky Test] TestStandAloneRunningSuite/TestOpAMP signal: killed)

Verification

• Not run locally here (the provided workflow context only includes the summarized Buildkite failure artifact).

Follow-up

If you can provide the full failing go test -v output or junit case details for TestAgentGracefulForceUnenroll from build 16712, I can narrow it to the exact failing assertion and give a surgical code/test fix recommendation instead of a flake classification.

---

What is this? | From workflow: PR Buildkite Detective

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

[Copilot]

🟢 Approval recommended

The functional change is small, well-targeted, and includes unit test coverage for the newly handled upgrade edge cases.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/pkg/api/handleCheckin.go:651

• The comment references upgradeStartedAtStalenessThreshold, but the implementation uses the stalenessThreshold() helper. Using a non-existent identifier in comments is misleading when searching/grepping for the actual logic.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are documentation/scope discrepancies in the updated comments/PR description that should be corrected to accurately reflect the newly introduced behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/pkg/api/handleCheckin.go:651

• The doc comment refers to upgradeStartedAtStalenessThreshold, but there is no such identifier in this file (the helper is stalenessThreshold). This makes the behavior harder to follow and suggests a stale rename in the comment.

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The change is narrowly scoped, includes unit test coverage for the new edge cases, and no blocking correctness issues were found.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/pkg/api/handleCheckin.go:652

• This doc comment mentions upgradeStartedAtStalenessThreshold, but the code uses stalenessThreshold(); referencing a non-existent identifier makes the behavior harder to follow when reading the function docs.

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite