Repository navigation
fix(agent-vault): count sleep time in leaf, session cache and upload link expiry - #448
Conversation
…link expiry Go's monotonic clock pauses while macOS (and Linux) sleeps, so deadlines built from time.Now() ignored time asleep. Strip the monotonic reading with Round(0) so the leaf cache, the session cache's grace and idle windows, and the upload link expiry follow the wall clock.
|
💬 Discussion in Slack: #pr-review-cli-448-fix-agent-vault-count-sleep-time-in-leaf-session-cache-and-upl Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
 [Medium impact] The PR appears safe to merge; no actionable new issue was found.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. This is a small, targeted fix that makes leaf, session-cache and upload-link deadlines follow the wall clock, so time spent asleep now counts toward them.
What was reviewed:
- Every reader of the changed fields:
time.Until(entry.expiration)/now.Afterin ca.go,time.Since(fetchedAt)andnow.Sub(lastSeen)/Beforein cache.go, andurlExpiresin session_log.go. Go uses wall-clock comparison when either operand has no monotonic reading, so all of these now count sleep time. - The NotAfter clamp in mintLeaf still applies after Round(0). The root's NotAfter has no monotonic reading, so the clamped value has none either.
- The new tests use
==againstRound(0), which does catch a leftover monotonic reading. The upload-link test uses the real clock on purpose, because the fake clock would always pass.
Extended reasoning...
The change adds .Round(0) to four timestamp assignments in packages/agentvault (ca.go mintLeaf, cache.go lastSeen/fetchedAt, session_log.go postedExpires), plus a regression test for each. It touches when cached TLS leaves and cached session credentials expire, but it only makes those deadlines stricter, because sleep time now counts. It changes no cert contents, auth checks or credential handling. The diff is 70 lines, mostly tests, and every comparison site was checked to behave correctly with values that have no monotonic reading. The repo has no CODEOWNERS file, and no other reviewer has raised an objection.
scott-ray-wilson
left a comment
scott-ray-wilson
left a comment
There was a problem hiding this comment.
Reviewed and tested this locally against a dev stack. The leaf fix works.
Live sleep test. I built the base commit (bf67dc4) and this PR with leafTTL = 4m and leafReuseMargin = 1m (nothing else changed), ran both proxies side by side, and sent a request through each one every 20s to https://httpbin.org/headers. Then the laptop went into a real clamshell sleep for 683s (14:57:22 to 15:08:45, per pmset -g log).
| Build | Before sleep (14:57:22) | First request after wake (15:09:03) |
|---|---|---|
| base | OK, leaf NotAfter 15:00:06 | curl: (60) SSL certificate problem: certificate has expired |
| this PR | OK, leaf NotAfter 15:00:06 | OK, new leaf NotAfter 15:13:03, credential injected |
The base build was still serving the expired leaf on the next request (15:09:21). This PR re-minted on the first request after wake.
I did not test the session cache and upload link changes live, because their windows are 10 to 15 min or need the control plane to be down. I reviewed those by reading the code. The inline comments cover a few deadlines that still use the monotonic clock, and a suggestion to fix this at the clock source so the next new field can't regress.
2 participants
Description 📣
Go's monotonic clock pauses while the machine sleeps (on macOS it's
mach_absolute_time, see golang/go#66870), and deadlines built fromtime.Now()were compared on it, so time asleep didn't count. After a laptop slept overnight the proxy kept serving an expired leaf cert for a host until it was restarted.Strips the monotonic reading with
Round(0)so these follow the wall clock:mintLeaf, the reported bugfetchedAt, otherwise credentials fetched before a sleep kept being served past the grace window on wake, including after access was revokedlastSeen, otherwise sessions idle overnight weren't dropped as inactive and kept getting re-resolvedType ✨
Tests 🛠️
# Here's some code block to paste some code snippets