Conversation
There was a problem hiding this comment.
This PR fixes a real race condition in certificate lifecycle management and the logic is sound, but certificate issuance code is operationally important enough to warrant a human look.
Extended reasoning...
Overview
The PR modifies packages/cmd/agent.go and adds packages/cmd/agent_cert_resolution_test.go. It fixes a race condition in both agentCmd and certManagerAgentCmd where MonitorCertificates (and the sync.Once-guarded initial issuance) could fire before resolveCertificateNameReferences completed. The fix merges the name-resolution goroutine and the MonitorCertificates call into a single sequential goroutine. The certificateFirstIssueOnce map is removed because the structural sequencing now provides the same single-call guarantee without it.
Security risks
No new security risks are introduced. The change does not touch authentication, token handling, or cryptographic logic. The certificate issuance flow itself is unchanged; only the sequencing of when it starts is fixed.
Level of scrutiny
Moderate. This is a bug fix with clear intent and tests, but it touches certificate lifecycle management which has operational significance. The sync.Once removal is correct given the new structure, but deserves a human confirming the invariant holds under all startup paths (e.g., restarts, reloads).
Other factors
Both inline-commented bugs (goroutine leak and getTokenUnsafe data race) are pre-existing and not introduced or worsened by this PR. The new tests cover the core resolveCertificateNameReferences function well. The PR description is sparse (checkboxes unchecked, no summary prose), which makes it harder to assess intent without reading the diff carefully.
Description 📣
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippets