Repository navigation
fixes from ladder 17: key rotation deadline in the store, truthful /readyz, portless bench, counted requests, re-admission retry - #730
Merged
Conversation
A sweep in flight when the server closes logged one error per remaining step, each a context canceled; the steps after a cancelled one are skipped.
…e process uptime The rotation loop ticked once per --key-rotation-interval from the process start and asked the store for the window on each tick. A control plane that never lived a full interval therefore never rotated: bananas redeploys on every merge to main and had not rotated since 2026-10-09, with rotation_lock.next_rotation_at five days in the past, and a control plane that crash-loops or is drained daily would behave the same. Init now schedules the first rotation one interval from the first start when the store has no deadline (ScheduleKeyRotationIfUnset), and the loop checks the store's deadline at a tenth of the interval, at most once a minute. The deadline is shared by the replicas and kept across restarts; a rotation that fell due while no replica was up happens on the first check after a start. The claim stays the single UPDATE that lets exactly one replica rotate per window. agentmesh_control_plane_next_key_rotation_timestamp_seconds exports the deadline, so a rotation that is not happening is a value in the past on a dashboard instead of a psql query.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request moves the key rotation deadline tracking into the database store, allowing replicas to coordinate rotation schedules across restarts and multiple instances. It introduces new store methods, updates the control plane server to check the rotation deadline periodically, adds a Prometheus metric for monitoring the next rotation timestamp, and updates the documentation. The review feedback highlights several test cases where returned errors are ignored using blank identifiers, violating the repository style guide rule against shallowing errors in tests.
Comment on lines
+90
to
+92
| if next, _ = store.NextKeyRotation(ctx); !next.Equal(late.Add(interval).Truncate(time.Millisecond)) { | ||
| t.Fatalf("deadline after the late rotation = %s, want %s", next, late.Add(interval)) | ||
| } |
Contributor
There was a problem hiding this comment.
3. Test coverage must follow the pyramid
The test ignores the error returned by store.NextKeyRotation(ctx) using _. According to the repository style guide, tests must not shallow errors; any returned error on the path under test must be handled and fail the test.
Please check the error explicitly.
Suggested change
| if next, _ = store.NextKeyRotation(ctx); !next.Equal(late.Add(interval).Truncate(time.Millisecond)) { | |
| t.Fatalf("deadline after the late rotation = %s, want %s", next, late.Add(interval)) | |
| } | |
| next, err = store.NextKeyRotation(ctx) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if !next.Equal(late.Add(interval).Truncate(time.Millisecond)) { |
References
- Rule 8 of Section 3 (Test coverage must follow the pyramid) states that tests must not shallow errors, and any returned error on the path under test must be handled and fail the test. (link)
Comment on lines
+259
to
+261
| if next, _ = store.NextKeyRotation(ctx); !next.Equal(t0.Add(interval)) { | ||
| t.Fatalf("deadline = %s, want %s", next, t0.Add(interval)) | ||
| } |
Contributor
There was a problem hiding this comment.
3. Test coverage must follow the pyramid
The test ignores the error returned by store.NextKeyRotation(ctx) using _. According to the repository style guide, tests must not shallow errors; any returned error on the path under test must be handled and fail the test.
Please check the error explicitly.
next, err = store.NextKeyRotation(ctx)
if err != nil {
t.Fatal(err)
}
if !next.Equal(t0.Add(interval)) {References
- Rule 8 of Section 3 (Test coverage must follow the pyramid) states that tests must not shallow errors, and any returned error on the path under test must be handled and fail the test. (link)
Comment on lines
+264
to
+269
| if claimed, _ := store.ClaimKeyRotation(ctx, t0.Add(interval-time.Second), interval); claimed { | ||
| t.Fatal("claim before the deadline succeeded") | ||
| } | ||
| if claimed, _ := store.ClaimKeyRotation(ctx, t0.Add(interval), interval); !claimed { | ||
| t.Fatal("claim at the deadline failed") | ||
| } |
Contributor
There was a problem hiding this comment.
3. Test coverage must follow the pyramid
The test ignores the errors returned by store.ClaimKeyRotation(...) using _. According to the repository style guide, tests must not shallow errors; any returned error on the path under test must be handled and fail the test.
Please check the errors explicitly.
claimed, err := store.ClaimKeyRotation(ctx, t0.Add(interval-time.Second), interval)
if err != nil {
t.Fatal(err)
}
if claimed {
t.Fatal("claim before the deadline succeeded")
}
claimed, err = store.ClaimKeyRotation(ctx, t0.Add(interval), interval)
if err != nil {
t.Fatal(err)
}
if !claimed {References
- Rule 8 of Section 3 (Test coverage must follow the pyramid) states that tests must not shallow errors, and any returned error on the path under test must be handled and fail the test. (link)
Comment on lines
+270
to
+272
| if next, _ = store.NextKeyRotation(ctx); !next.Equal(t0.Add(2 * interval)) { | ||
| t.Fatalf("deadline after the claim = %s, want %s", next, t0.Add(2*interval)) | ||
| } |
Contributor
There was a problem hiding this comment.
3. Test coverage must follow the pyramid
The test ignores the error returned by store.NextKeyRotation(ctx) using _. According to the repository style guide, tests must not shallow errors; any returned error on the path under test must be handled and fail the test.
Please check the error explicitly.
next, err = store.NextKeyRotation(ctx)
if err != nil {
t.Fatal(err)
}
if !next.Equal(t0.Add(2 * interval)) {References
- Rule 8 of Section 3 (Test coverage must follow the pyramid) states that tests must not shallow errors, and any returned error on the path under test must be handled and fail the test. (link)
…s no loopback port The sidecar's /readyz answered 200 from the moment the process was up, on the socket and on TCP, while the metrics listener's /readyz waited for an authenticated, connected router. A probe or a harness reading the sidecar learned nothing. Both listeners now give the same answer, and a node without an identity yet is not ready. agentmesh-bench gave member i the loopback port 20000+i for /readyz. On ladder 17 the minions' ephemeral range covered those ports, the kernel handed some of them to other members' libp2p listeners, and 4% of the fleet died at start with "address already in use". The bench now reads readiness over the member's Unix socket, as it already does for discovery and the first call, and passes no --metrics-addr: a fleet of any size binds no loopback port. --metrics-base-port is gone with it. The bench's fake node refuses any --metrics-addr or --bind-addr, so a member that binds a port again fails every bench test; the sidecar test pins 503 then 200 on both listeners as a router is authenticated.
…s instead of listing them The mesh state snapshot, read every 30s, listed every enrollment request with its public key and biscuit to count them by status. Within the retention a busy mesh holds one row per enrollment, tens of thousands, and on ladder 17 the snapshot missed its 5s budget once at full load. CountEnrollmentRequests is one GROUP BY, as CountNodes is for nodes; the admin view keeps the list.
…ore it is dropped After a key rotation a node refreshes its credential, now signed by the new key, and shows it to every router it holds a session with. A router that has not synced the new key yet refuses it. The node dropped the router from its authenticated set and left the re-handshake to the connection monitor, a minute later by default: a minute with no router, during which its API answered 503 "not connected to the mesh" to every call. TestKeyRotationIntegration, with a 2s interval and a 500ms router sync, hit that window in about half its runs. A refused re-admission is retried after 500ms, 1s and 2s, which is longer than a router takes to learn a key from the rotation event or its sync, and the router is dropped only after that. The integration test prints the node log when the final call fails, so the next failure can be read instead of reproduced.
The hold ran while pgrep over ssh found agentmesh-bench on the first minion, so one IAP tunnel failure read as the run having ended: ladder 17's 90-minute hold was cut at 35. Exit 1 from pgrep ends the hold; any other exit is logged and checked again a minute later.
aojea
changed the title
TestUserStatusAndTenancy failed in CI with "expected 1 node for User A, got: 0". The test, like fifteen others in the package, enrolls a node with no EnrolledAt; the store wrote the zero time, and the storage sweep that runs when the server starts read it as a bootstrap member silent for two thousand years and collected it, when its goroutine won the race against the test's insert. An unset EnrolledAt is recorded as now. A receiver treats an unset instant as invalid, never as the epoch; every handler sets it, and a row written without one is a member seen at the write. Twelve runs of the test pinned to one CPU pass. Review: the rotation tests check every error they used to discard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One PR for the bugs ladder 17 and the rc10 rotation check surfaced, each with a regression test. Nothing here changes the wire protocol or the schema.
1. The key rotation deadline lives in the store, not in the process uptime
The rotation loop ticked once per
--key-rotation-intervalfrom process start and only then asked the store whether the window was due. A control plane that never lived a full interval never rotated: bananas redeploys on every merge to main and had not rotated since 2026-10-09, withrotation_lock.next_rotation_atfive days in the past. A control plane that crash-loops or is drained daily behaves the same.Initschedules the first rotation one interval from the first start when the store has no deadline; the loop checks the store's deadline at a tenth of the interval, at most once a minute. The claim stays the single UPDATE that lets exactly one replica rotate per window. A rotation that fell due while no replica was up happens on the first check after a start; a restart before the deadline does not push it back.agentmesh_control_plane_next_key_rotation_timestamp_secondsexports the deadline: a value in the past is a rotation that is not happening.Tests:
TestScheduleKeyRotationIfUnset,TestKeyRotationCheckInterval,TestRotateKeysIfDueFollowsTheStoredDeadline(a check five days late rotates once and sets the next deadline from that time).2.
/readyztells the truth on every listener; a bench member binds no loopback portThe sidecar's
/readyzanswered 200 from the moment the process was up, on the socket and on TCP, while the metrics listener's waited for an authenticated, connected router. Both now give the same answer, and a node with no identity yet is not ready.agentmesh-benchgave member i loopback port 20000+i for/readyz. On ladder 17 the minions' ephemeral range covered those ports, the kernel handed some of them to other members' libp2p listeners, and 4% of the fleet died at start withaddress already in use. The bench reads readiness over the member's Unix socket, as it already did for discovery and the first call, and passes no--metrics-addr;--metrics-base-portis gone.Tests: the bench's fake node refuses any
--metrics-addr/--bind-addr, so a member that binds a port again fails every bench test;TestSidecarReadyzFollowsTheMeshConnectionpins 503 then 200 on both listeners;TestMemberBurstJoinsConcurrentlyruns real nodes through the socket.3. The metrics snapshot counts enrollment requests instead of listing them
The snapshot, read every 30s, listed every enrollment request with its key and biscuit to count them by status; within the retention a busy mesh holds tens of thousands, and on ladder 17 the snapshot missed its 5s budget once at full load.
CountEnrollmentRequestsis oneGROUP BY, asCountNodesis for nodes. Test:TestCountEnrollmentRequestsByStatus.4. A router that refuses a refreshed credential is asked again before it is dropped
After a rotation a node refreshes its credential, now under the new key, and shows it to its routers at once. A router that had not synced the key yet refused it; the node dropped the router and left the re-handshake to the connection monitor, a minute later by default, during which its API answered 503 to every call.
TestKeyRotationIntegration(2s interval, 500ms router sync) hit that window in about half its runs; it was failing on main before this PR. A refused re-admission is retried after 500ms, 1s and 2s before the router is dropped, and the integration test prints the node log when the final call fails.Tests:
TestReadmitRetriesARouterThatHasNotSyncedTheNewKey,TestReadmitDropsARouterThatKeepsRefusing;TestKeyRotationIntegration6/6 under the CPU pin that made it fail before.5. A node enrolled without a last-seen instant is seen now
TestUserStatusAndTenancyfailed on the first CI run of this PR ("expected 1 node for User A, got: 0"). The test, like fifteen others in the package, enrolls a node with noEnrolledAt; the store wrote the zero time, and the storage sweep that runs at server start read it as a bootstrap member silent for two thousand years and collected it when its goroutine won the race against the insert. The store records an unsetEnrolledAtas now (an unset instant is never the epoch);TestEnrollNodeWithoutEnrolledAtIsSeenNowpins it, and the flaky test passes 12/12 pinned to one CPU.6. Smaller
context cancelederror per remaining step.Validation
make lint0 issues; unit suite; full integration suite (232s); Bats e2e 32/32.