hashicorp / hashicorp/consul-template

VaultPKIQuery and VaultWriteQuery cannot be stopped during refresh sleep, leaking goroutines on runner restart

Open
#2,170 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
4.8k
Forks
801
Avg merge
4h 5m
Merged PRs (30d)
6

Description

### Consul Template version

Observed with Vault Agent 1.21.1, which embeds `github.com/hashicorp/consul-template v0.41.1`.

The same blocking code is still present in consul-template v0.42.1 and at current `main` (`0be299ad`). The behavior dates back to the introduction of `pkiCert` and is present in releases starting with v0.29.0.

### Summary

`VaultPKIQuery.Fetch` and `VaultWriteQuery.Fetch` wait for their next refresh with an uninterruptible `time.Sleep`. Their `Stop` methods close `stopCh`, but a `Fetch` that is already sleeping does not observe that channel until the sleep expires.

When a runner containing `pkiCert` or non-renewable Vault write dependencies is stopped and replaced, the old fetch goroutines remain alive—potentially for most of the secret lifetime. Repeated runner replacement therefore creates an unbounded goroutine and memory leak, one goroutine per sleeping dependency per replacement.

This was observed in a long-running Vault Agent where failed token renewal caused repeated authentication and template-runner replacement. Runner replacement was the trigger in this incident, but any code path that stops/replaces a runner while a PKI fetch is sleeping can reproduce the leak.

### Configuration

The affected Vault Agent had three templates using long-lived PKI certificates, conceptually:

```hcl
template {
contents = <<-EOF
{{ with pkiCert "pki/issue/example" "common_name=example.internal" "ttl=2160h" }}
{{ .Cert }}
{{ .Key }}
{{ range .CAChain }}{{ . }}{{ end }}
{{ end }}
EOF
destination = "/path/to/certificate.pem"
}
```

The incident also had a short-lived AppRole token that could not renew itself. That caused Vault Agent to authenticate again and replace the embedded consul-template runner approximately every 4–5 minutes. This token configuration is not required for the underlying consul-template bug; it only makes the leak accumulate quickly.

### Expected behavior

Stopping a runner/view/dependency should promptly unblock every in-progress `Fetch` and return `dependency.ErrStopped`, including when a PKI dependency is waiting for its next certificate rotation.

### Actual behavior

The `VaultPKIQuery.Fetch` and `VaultWriteQuery.Fetch` goroutines use the same blocking pattern and remain in `time.Sleep(dur)` until their scheduled refresh:

```go
select {
case dur := <-d.sleepCh:
time.Sleep(dur)
default:
}
```

Their `Stop` methods close `d.stopCh`, but the sleeping code never selects on it:

```go
func (d *VaultPKIQuery) Stop() {
close(d.stopCh)
}
```

For a newly issued 90-day certificate, `goodFor` schedules rotation at roughly 87–92% of the original lifetime. A stranded PKI fetch can therefore sleep for approximately 78–83 days. `VaultWriteQuery` has the same lifecycle when `leaseCheckWait` queues a delay for a non-renewable write response.

### Why this leaks after runner replacement

The lifecycle is:

1. The first `VaultPKIQuery.Fetch` reads/issues the certificate and puts the calculated rotation delay into the buffered `sleepCh`.
2. `watch.View.fetch` loops and calls `Fetch` again.
3. The second call consumes `sleepCh` and enters the long `time.Sleep`.
4. Runner shutdown calls `Watcher.Stop` → `View.stop` → `VaultPKIQuery.Stop`, closing the dependency's `stopCh`.
5. The view/poll goroutines can stop, but the goroutine currently inside `VaultPKIQuery.Fetch` cannot observe `stopCh` and remains blocked until the certificate rotation deadline.
6. A replacement runner creates a new dependency/view/fetch goroutine. Repeating the cycle accumulates one sleeper per PKI dependency each time.

`VaultWriteQuery` follows the same sequence: a fetch of a non-renewable write response queues `leaseCheckWait` in `sleepCh`, the next fetch enters `time.Sleep`, and runner shutdown cannot interrupt it.

At minimum, each blocked fetch retains its `View`, `VaultPKIQuery`, cached `View.data` (including the PEM material), Vault client state, channels, and goroutine stack. The incident heap also contained large numbers of repeated template-source and PEM strings; establishing the complete retention path from every template source to the blocked goroutine would require pointer-level heap analysis, but the source directly proves the retained dependency/view/PEM path.

### Production evidence

A core dump from the affected agent showed:

- 7.7 GB core; approximately 1.13 GB of live Go heap data at capture time, with RSS around 1.3 GB and rising again.
- 69,059 structurally identified goroutine descriptors; 69,030 had the same 4 KB stack shape corresponding to the stranded fetch path.
- 92,055 copies of the PKI template-source string.
- 69,796 RSA private-key PEM blocks but only four distinct keys, and approximately 70,000 certificate PEM blocks with only a few distinct certificates.
- Approximately 19,000 service-token strings and repeated `403 permission denied` responses from `auth/token/renew-self`, matching the reauthentication/runner-replacement loop.
- Three PKI templates × approximately 23,000 runner replacements ≈ 69,000 stranded goroutines, matching the independently counted goroutines in the dump.

After changing the AppRole to issue renewable periodic tokens, Vault Agent renewed the same token successfully and stopped replacing the runner every few minutes. Memory-leak growth effectively stopped. This is an operational mitigation, not a fix for the uninterruptible PKI sleep.

### Minimal deterministic reproduction

Neither bug requires a real Vault server once the refresh delay is queued:

1. Construct a `VaultPKIQuery` or `VaultWriteQuery`.
2. Put a long duration (for example, one hour) into `d.sleepCh`.
3. Start `d.Fetch(nil, nil)` in a goroutine.
4. Wait until `Fetch` consumes the duration.
5. Call `d.Stop()`.
6. Observe that `Fetch` does not return.

Regression tests implementing exactly those steps fail on the current implementation after one second:

```text
--- FAIL: TestVaultPKIQuery_FetchStopsWhileSleeping (1.00s)
vault_pki_stop_test.go:46: Fetch did not return after Stop (goroutine leak)
```

With an interruptible timer/select implementation, both tests pass 100 consecutive runs.

### Proposed fix

Wait on the rotation timer and `stopCh` together:

```go
timer := time.NewTimer(dur)
select {
case <-timer.C:
case <-d.stopCh:
timer.Stop()
return nil, nil, ErrStopped
}
```

Apply this wait to both `VaultPKIQuery` and `VaultWriteQuery`. It preserves the existing refresh timing while allowing runner shutdown to terminate either fetch immediately.

### References

- #1644 fixed the same uninterruptible-sleep leak pattern for `VaultReadQuery` and was merged.
- #2124 proposed the same fix for `VaultPKIQuery` and `VaultWriteQuery`, but was closed by its author without review and without a regression test.
- hashicorp/vault#28876 is an adjacent Vault Agent memory-growth report involving `pkiCert`; it did not include a goroutine dump proving this root cause.
- hashicorp/vault#32031 is a separate adjacent `pkiCert`/`writeToFile` render-churn issue, not the cause of the stranded fetch goroutines described here.

Contributor guide

Open the contributing guide

Research direction

Start at VaultPKIQuery.Fetch and VaultWriteQuery.Fetch, especially the refresh wait and Stop paths described in the issue. Run the regression tests in vault_pki_stop_test.go, then verify that stopping either fetch returns dependency.ErrStopped promptly while sleeping and that the existing refresh timing remains unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.