Repository navigation
test(ci): make the lost-session and coordinator-restart scenarios deterministic - #477
Merged
Merged
Conversation
…erministic TestForegroundRemoteLostSession: the fixture's drop cancelled the agent before closing the pipes, so the agent's cancelled result could cross the wire first and the run (correctly) read it as an operator cancel. Close the wire first, then stop the agent, as a dropped ssh connection does. TestPullCoordinatorKillResumesWithoutDuplicates: it asserted the first token moved to the new session, but after the restart the re-adopted run offers the lease again with a token of its own and the carrier may switch to it before or after its reconcile heartbeat. Assert that a token of the lease is bound to the new session of the same certificate instead.
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
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.
Both are test assumptions, not product races. No product code changes.
TestForegroundRemoteLostSession (free/ci/test/sim)
Root cause: the fixture's
drop()calledcancel()on the agent session before closing the pipes. The fixture runner answers a cancelled context with acancelledresult, so that result could cross the pipe before it closed and the run read it as an operator cancel:cancelled unit: not run (cancelled by the operator), exit 1. A real ssh drop never makes the agent stop first.Fix: close the wire (agent output, then agent input), then stop the agent.
Proof: with a 300 ms sleep between
cancel()and the pipe close, the original fixture fails every time with the exact symptom. With the fix the same injected delay (moved beforecancel()) passes. Natural rate is low: original 0/60 at -cpu 1 and 8 (race, under load); fixed 0/100 at -cpu 1 and 8.TestPullCoordinatorKillResumesWithoutDuplicates (free/ci/internal/transport/agent)
Failure in run 40198123258:
the token must move to the new session of the same certificate.Root cause: the test asserted that the first token JTI moved to the new session. After the restart the re-adopted run offers the lease again with a token it mints itself, and the carrier swaps to it. Whether the agent's reconcile heartbeat reaches the new coordinator before or after that offer is a race between two streams. Heartbeat first: the old token moves. Offer first: the old token stays on the dead session and the new one is bound fresh (instrumented gate:
TokenBind created=trueon the first heartbeat of the new session; lines 13-16 still arrive exactly once, durable log intact). Both orders are correct product behaviour.Fix: assert that a token of the lease (the original or the one the carrier holds now) is bound to a new session with the same certificate serial, polled with
eventually.Proof (race detector, test binary built once, run under CPU load):
Also:
go test -count=1 ./free/ci/internal/transport/agent/ ./free/ci/test/sim/all pass (187 tests).