Clock-skew offset is not updated after time re-sync, first attempts always use wrong timestamps

[eattker]

Acknowledgements

• I have searched (https://github.com/aws/aws-sdk/issues?q=is%3Aissue) for past instances of this issue
• I have verified all of my SDK modules are up-to-date (you can perform a bulk update with go get -u github.com/aws/aws-sdk-go-v2/...)

Describe the bug

Overview

This library automatically adjusts the signing timestamp when the server clock differs from the client clock and responds with "RequestTimeTooSkewed" error. However when the clocks became synchronized again, the library does not update the offset back to the newly calculated skew (close to zero).

When we shift the client time to the future, we can see that the signing timestamps of the first attempts are in the future, then it adjust the timestamp (using negative offset) to be in the present in the retries. This works when the offset is not close to zero.
However it can't adjust the timestamp for the second try, the proper timestamp is sent only in the third attempt, see later. All subsequent requests will use the correct timestamp at the first attempt. Requests without a "retryer" will be succeeded.

When we shift back the client time to present, it always sends the first request with timestamp in the past (due to negative offset), the second attempt will use the correct timestamp. But all subsequent requests still uses timestamp in past (due to negative timestamp) at the first attempt and adjust it in the second attempt. Requests without "retryer" will fail because of the wrong timestamp.

Observer timeline:

Time on both side: 20250910050607Z
Shift the client time forward by 1 year to 20260910050607Z

Request 1
-> Attempt 1 : 20260910050607Z
<- Response 1 : RequestTimeTooSkewed, 20250910050607Z
-> Attempt 2 : 20260910050607Z
<- Response 2 : RequestTimeTooSkewed, 20250910050607Z
-> Attempt 3 : 20250910050607Z
<- Response 3 : OK

Request 2
-> Attempt 1 : 20260910050607Z
<- Response 1 : OK

Shifting the client time back to present (20250910050607Z)

Request 3
-> Attempt 1 : 20240910050607Z
<- Response 1 : RequestTimeTooSkewed, 20250910050607Z
-> Attempt 2 : 20250910050607Z
<- Response 2 : OK

Request 4 ...
-> Attempt 1 : 20240910050607Z
<- Response 1 : RequestTimeTooSkewed, 20250910050607Z
-> Attempt 2 : 20250910050607Z
<- Response 2 : OK

Analysis

I have checked (and debugged) the source code and I belive the problem is in the retry mechanism in /aws/retry/middleware.go:

// HandleFinalize utilizes the provider Retryer implementation to attempt
// retries over the next handler
func (r *Attempt) HandleFinalize(ctx context.Context, in smithymiddle.FinalizeInput, next smithymiddle.FinalizeHandler) (
	out smithymiddle.FinalizeOutput, metadata smithymiddle.Metadata, err error,
) {
	var attemptNum int
	var attemptClockSkew time.Duration
	var attemptResults AttemptResults

	maxAttempts := r.retryer.MaxAttempts()
	releaseRetryToken := nopRelease

	retryMetrics, err := newAttemptMetrics(r.OperationMeter)
	if err != nil {
		return out, metadata, err
	}

	for {
		attemptNum++
		attemptInput := in
		attemptInput.Request = r.requestCloner(attemptInput.Request)

		// Record the metadata for the for attempt being started.
		attemptCtx := setRetryMetadata(ctx, retryMetadata{
			AttemptNum:       attemptNum,
			AttemptTime:      sdk.NowTime().UTC(),
			MaxAttempts:      maxAttempts,
			AttemptClockSkew: attemptClockSkew,
		})

		// Setting clock skew to be used on other context (like signing)
		ctx = internalcontext.SetAttemptSkewContext(ctx, attemptClockSkew)

		var attemptResult AttemptResult
                 ...
}

First, attemptClockSkew should not be initialized to zero, it should be read from internalcontext:

        attemptClockSkew := internalcontext.GetAttemptSkewContext(ctx)

Second, clock skew should be set before creating the attemptCtx, because attemptCtx is used for the request and the offset is read from that context.

                // Setting clock skew to be used on other context (like signing)
                ctx = internalcontext.SetAttemptSkewContext(ctx, attemptClockSkew)

		// Record the metadata for the for attempt being started.
		attemptCtx := setRetryMetadata(ctx, retryMetadata{
			AttemptNum:       attemptNum,
			AttemptTime:      sdk.NowTime().UTC(),
			MaxAttempts:      maxAttempts,
			AttemptClockSkew: attemptClockSkew,
		})

Note: the AttemptClockSkew in retryMetadata is used for adjusting the TTL.

The suggested fix addresses the retry case. I have not verified the non-retry case.

Regression Issue

• Select this option if this issue appears to be a regression.

Expected Behavior

After clocks are synchronized again, the clock-skew offset should be updated with the newly calculated value and should be used at the first attempt.

Current Behavior

After clocks are synchronized again, the client uses wrong timestamps at the first attempt. Requests without retry always fails.

Reproduction Steps

1. shift the client time forward by 1 hour
2. send requests to the server, first will succeed after retries and the subsequent requests will succeed for the first attempt
3. shif the client time back to present
4. send requests to the server, they will succeed after retries but never for the first attempt

Easier way is to extend the unit test in /internal/middleware/midleware_test.go:

func TestSkewIsSetOnTheWholeClient(t *testing.T) {
	defer resetDefaults(sdk.TestingUseNopSleep())
	fiveMinuteSkew := func() time.Time {
		return time.Now().Add(5 * time.Minute)
	}
	sdk.NowTime = fiveMinuteSkew
	var offset atomic.Int64
	offset.Store(0)
	c := MockClient{
		options{
			HTTPClient: failRequestIfSkewed(),
			Retryer: retry.NewStandard(func(s *retry.StandardOptions) {
			}),
			Offset: &offset,
		},
	}
	resp, err := c.Do(context.Background())
	if err != nil {
		t.Errorf("Expected request to succeed on retry. Got %v and err %v", resp, err)
	}
	// Remove retryer so it has to succeed on first call
	c.options.Retryer = nil
	// same client, new request
	resp, err = c.Do(context.Background())
	if err != nil {
		t.Errorf("Expected second request to succeed since the skew should be set on the client. Got %v and err %v", resp, err)
	}

	// restore time
	sdk.NowTime = func() time.Time {
		return time.Now()
	}
	// restore retryer
	c.options.Retryer = retry.NewStandard(func(s *retry.StandardOptions) {
	})

	// same client, new request
	resp, err = c.Do(context.Background())
	if err != nil {
		t.Errorf("Expected request to succeed on retry. Got %v and err %v", resp, err)
	}
	// Remove retryer so it has to succeed on first call
	c.options.Retryer = nil
	// same client, new request
	resp, err = c.Do(context.Background())
	if err != nil {
		t.Errorf("Expected second request to succeed since the skew should be set on the client. Got %v and err %v", resp, err)
	}
}

This extended unit test fails with the current code, but pass with the suggested fixes.

Possible Solution

1. Read the prior attempt skew from context when starting the loop.
2. Set the skew on the context before creating attemptCtx, so all downstream consumers (signing, TTL) see the correct value.

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

aws-sdk-go-v2@1.38.2

Compiler and Version used

go1.24.0 darwin/arm64

Operating System and version

mac osx, linux (doesn't matter)

[Madrigal]

However when the clocks became synchronized again, the library does not update the offset back to the newly calculated skew (close to zero).

This is unfortunately the problem with this approach. Your fix suggests reading from the internal context before starting the retry loop. However, the lifecycle of this context object is scoped to a request, so when a new request is started, all values that were set on this context are lost and a new request starts with these values undefined/nil

Read the prior attempt skew from context when starting the loop.

You might say "well, we can store that somewhere on the client and use it across all instances". This was discussed when this was implemented, however it has 2 downsides

1. You have to either leave this open for race conditions or mutex after every call, since a single client has multiple concurrent operation calls.
2. If you're calling an AWS service you're unlikely to be hitting a single host, but rather multiple of them. If the clock is different on only one of the hosts, you'll be having problems calling the rest of the hosts with the adjusted time.

Due to the complexity of 1) we decided to only apply this at the request level.

Are you experiencing any issues on your application due to this setup?

[eattker]

Maybe I’m missing something, but the offset is already stored at the client level as an atomic Int64 pointer.

It is added to each request’s context in the build phase via the AddTimeOffsetMiddleware struct.
So by the finalize phase, it is already set in the context.

As far as I understand the code, the global offset is set based on the offset (clock skew) value stored in the context, and never from the newly calculated offset. The newly calculated offset is used in the retry mechanism to set the local offset, which is then stored in the next attempt’s context (in the current code not in the next, but in the third attempt).

Let’s imagine the client time is shifted forward by 1 hour, then shifted back to the present.
In the first part, the global offset is correctly set to -1 hour eventually.

In the second part, with the current retry code (handleFinalize), it works like this in my understanding:

Initially:

• the local offset is zero
• the global offset (-1 hour) is already set in the retry context since it comes from the build phase

First attempt:

• the offset for the first attempt comes from the retry context since it is the parent, so the offset in the attempt’s context is -1 hour
• a zero offset value is set to the retry context
• we get a RequestTimeTooSkewed error in the response
• at the response deserialize phase (internal/middleware), the global offset is set again to -1 hour, since that value was set in the attempt’s context
• at the response deserialize phase (aws/middleware), a new offset is calculated (a few milliseconds)
• the local offset is updated to this new offset (a few milliseconds)

Second attempt:

• in the retry context, the offset is still 0, and this is inherited into the attempt’s context
• the correct few-millisecond offset is set to the retry context
• we get an OK response
• at the response deserialize phase (internal/middleware), the global offset remains unchanged (-1 hour) since the zero value was set in the attempt’s context (see internal/middleware/middleware.go)
• at the response deserialize phase (aws/middleware), no new offset is calculated since there was no error

As you can see, the global offset is never updated to the correct value, because in the second attempt the offset in the attempt’s context is always set to zero. This happens to work in this case, but due to the zero value the global offset is never updated.

We encounter issues when the client time is shifted for some reason and then re-synced. After the re-sync, every request fails on the first attempt and only succeeds on the second.

[eattker]

Hi @Madrigal,

Did you have time to go through my comments and rethink your view?

Regards,
Attila

[c5mao]

Hi @eattker, do you mean that Request 4 and after should succeed in the first try? Since in request 3, the global offset should be updated after the 1st try.

[eattker]

Hi @c5mao,

Yes, the logic should work the same way for requests 3–4 as for requests 1–2.

To make the picture clearer: if we shift the client time two hours into the future before the first request, then
shift it back by only one hour before the third request (so the clock difference is one hour),
then the extended test will pass. It will update the global offset to one hour during the third request.
It fails only when a “0” offset is appropriate.