This is a well-targeted fix for a real bootstrap ordering problem. On a fresh install with DistributedRunners enabled, SetupEtcdTLS was called before Coordinator.Start had a chance to run LoadCA — meaning there was no CA on disk yet, causing a "CA certificate not found" crash-loop. The fix is clean: extract the create-if-missing CA logic into a package-level EnsureCA, then call it in server.go immediately before SetupEtcdTLS. Coordinator.LoadCA now delegates to EnsureCA instead of duplicating the logic, which is a genuine improvement.
The logic is correct. EnsureCA is idempotent (reads existing file first, only generates+writes when absent), so calling it twice — once early in Server(), once again when Coordinator.Start → LoadCA → EnsureCA runs — is harmless. The both-CA-and-key-on-disk cases are handled correctly, and file permissions are preserved (0644 for certs, 0600 for keys).
The tests are solid for what they cover. TestEtcdTLSColdBoot faithfully reproduces MIR-1464: it fails SetupEtcdTLS with no CA, calls EnsureCA, then verifies the issued certs chain properly. TestEnsureCAIdempotent pins that the second call doesn't regenerate. That's the right set of tests for this change.
One stale comment remains: SetupEtcdTLS's doc says "The CA must already exist (created by the coordinator's LoadCA)" at line 179, but that's no longer true — the callsite in server.go now ensures it via EnsureCA. The comment should be updated to reflect the new invariant (callers must call EnsureCA or LoadCA first). It's not a correctness issue, but it will confuse the next person to read that function.
Coordinator.LoadCA; it's guaranteed by EnsureCA, which server.go calls before this function. Consider updating to something like: "The CA must already exist on disk; call EnsureCA (or LoadCA) before SetupEtcdTLS."Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 964,
"verdict": "ready",
"event": "comment",
"summary": "This is a well-targeted fix for a real bootstrap ordering problem. On a fresh install with `DistributedRunners` enabled, `SetupEtcdTLS` was called before `Coordinator.Start` had a chance to run `LoadCA` — meaning there was no CA on disk yet, causing a \"CA certificate not found\" crash-loop. The fix is clean: extract the create-if-missing CA logic into a package-level `EnsureCA`, then call it in `server.go` immediately before `SetupEtcdTLS`. `Coordinator.LoadCA` now delegates to `EnsureCA` instead of duplicating the logic, which is a genuine improvement.\n\nThe logic is correct. `EnsureCA` is idempotent (reads existing file first, only generates+writes when absent), so calling it twice — once early in `Server()`, once again when `Coordinator.Start` → `LoadCA` → `EnsureCA` runs — is harmless. The both-CA-and-key-on-disk cases are handled correctly, and file permissions are preserved (0644 for certs, 0600 for keys).\n\nThe tests are solid for what they cover. `TestEtcdTLSColdBoot` faithfully reproduces MIR-1464: it fails `SetupEtcdTLS` with no CA, calls `EnsureCA`, then verifies the issued certs chain properly. `TestEnsureCAIdempotent` pins that the second call doesn't regenerate. That's the right set of tests for this change.\n\nOne stale comment remains: `SetupEtcdTLS`'s doc says \"The CA must already exist (created by the coordinator's LoadCA)\" at line 179, but that's no longer true — the callsite in `server.go` now ensures it via `EnsureCA`. The comment should be updated to reflect the new invariant (callers must call `EnsureCA` or `LoadCA` first). It's not a correctness issue, but it will confuse the next person to read that function.",
"comments": [
{
"path": "components/coordinate/coordinate.go",
"line": 179,
"side": "RIGHT",
"body": "This doc comment is now stale. The CA is no longer guaranteed by `Coordinator.LoadCA`; it's guaranteed by `EnsureCA`, which `server.go` calls before this function. Consider updating to something like: \"The CA must already exist on disk; call EnsureCA (or LoadCA) before SetupEtcdTLS.\"",
"ai_prompt": "In components/coordinate/coordinate.go at line 179, the doc comment on SetupEtcdTLS says 'The CA must already exist (created by the coordinator's LoadCA).' This is stale — the caller in server.go now uses EnsureCA directly. Update the comment to say the CA must already exist on disk, and that callers should call EnsureCA or Coordinator.LoadCA before invoking this function."
}
],
"posted_to_pr": true,
"draft": true
}