Size each discharge pass's timeout for what it waits on - #5124
Merged
Conversation
refreshDischargeTokens makes up to two passes and each one built its own context from fly-go's default, so a command could spend twice that budget: 90 seconds waiting for servers, then 90 more waiting for a person. The two passes are not the same kind of wait, and now that fly-go takes WithDischargeTimeout they don't have to share a number. The pass without a UserURLCallback cannot wait on anyone. A third party that wants the user fails it immediately, so everything left is a server round trip; it gets 30 seconds. The retry that can open a browser keeps the 90 seconds, which is what an interactive login through an identity provider is sized for. Worst case goes from three minutes to two, and the agent, which never has a callback, stops holding a 90 second budget it can never use. Timeouts move behind doRefreshDischargeTokens so the split is testable without waiting on the real ones.
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.
refreshDischargeTokensmakes up to two passes over the undischarged tickets and each one builds its own context from fly-go's default, so a single command can spend twice that budget. This gives the two passes separate timeouts, sized for the kind of wait each one actually does: 30 seconds for the pass that can only talk to servers, 90 for the pass that waits on a person.Context
The first pass runs without a
UserURLCallback, which is what makes the macaroon client fetch every ticket in parallel. A third party that wants the user cannot be satisfied there and fails immediately, so everything that pass can still accomplish is a server round trip. It was holding the same 90 second budget as the interactive retry, which nothing in it can use.The retry is where the cost is: the browser opens, the user works through their identity provider, and the client polls until the discharge appears. That is the wait 90 seconds was chosen for in superfly/fly-go#286.
Consequences of splitting them:
agent/server/server.go) passesuucb = niland so only ever runs the non-interactive pass. It stops reserving a 90 second budget it can never spend.Details
The timeouts move behind
doRefreshDischargeTokens, following thefetchOrgTokens/doFetchOrgTokensseam already in this file, so the split can be tested in a couple of seconds instead of on the real durations. The tests drive a third party that never discharges and assert each pass stops at its own budget; each one fails if the other pass's timeout is wired in.