AdjustClockSkew logs a "time skew / signature check failed" WARN on every non-2xx outcome (incl. 404/412), before the error-type / ShouldRetry check
- Dominant language
- C++
- Stars
- 2.2k
- Forks
- 1.2k
- Avg merge
- 3d 14h
- Merged PRs (30d)
- 12
Description
### Describe the bug
In the retry loop, `AWSClient::AdjustClockSkew()` is called on **every** non-successful HTTP outcome — *before* `RetryStrategy::ShouldRetry()` and without any error-type gating — and it emits, **unconditionally at the top of the function**:
> `If the signature check failed. This could be because of a time skew. Attempting to adjust the signer.`
before it has determined whether the error is actually clock-skew / signature related. So ordinary, expected non-2xx responses that have nothing to do with signatures or clocks — `404 NoSuchKey` (HeadObject on a missing key), `412 PreconditionFailed` (conditional `PutObject If-None-Match`), etc. — each emit this alarming WARN and do a wasted `Date` header parse. Workloads that use conditional requests / existence checks as normal control flow produce huge volumes of this misleading log and it falsely suggests a signature/clock defect where none exists.
### Regression Issue
- [ ] Select this option if this issue appears to be a regression.
### Expected Behavior
The "…time skew… adjust the signer" WARN should be emitted only when a clock skew is actually detected (i.e. inside the branch where `diff` exceeds the threshold and the signer is adjusted). Non-skew errors (404, 412, …) should not produce a signature/clock-skew warning and ideally should not enter the skew-detection / Date-parsing path at all.
### Current Behavior
`src/aws-cpp-sdk-core/source/client/AWSClient.cpp` (identical in 1.11.771 and `main`):
```cpp
// retry loop — called on any error outcome, BEFORE ShouldRetry:
bool shouldSleep = !AdjustClockSkew(outcome, signerName) && !retryWithCorrectRegion;
if (!retryWithCorrectRegion && !m_retryStrategy->ShouldRetry(outcome.GetError(), retries))
break;
bool AWSClient::AdjustClockSkew(HttpResponseOutcome& outcome, const char* signerName) const {
if (m_enableClockSkewAdjustment) {
auto signer = GetSignerByName(signerName);
AWS_LOGSTREAM_WARN(AWS_CLIENT_LOG_TAG,
"If the signature check failed. This could be because of a time skew. Attempting to adjust the signer."); // <-- unconditional, before any skew check
DateTime serverTime = GetServerTimeFromError(outcome.GetError());
...
if (diff >= TIME_DIFF_MAX || diff <= TIME_DIFF_MIN) { /* real adjust + retry */ return true; }
}
return false;
}
```
With clocks in sync, a 404/412 logs the WARN, parses the Date header, computes `diff ≈ 0`, and returns `false` (no adjustment, no retry). Net effect: one misleading WARN + a wasted Date parse per non-2xx outcome.
### Reproduction Steps
Point an `S3Client` at any S3-compatible endpoint whose clock matches the client, enable logging at `Warn`, and HEAD a non-existent key (→404). The WARN is logged even though there is no skew.
```cpp
#include
#include
#include
int main() {
Aws::SDKOptions options;
options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Warn; // logs to aws_sdk_*.log
Aws::InitAPI(options);
{
Aws::Client::ClientConfiguration cfg;
cfg.region = "us-east-1";
// cfg.endpointOverride = "http://127.0.0.1:9000"; // any reachable S3 / S3-compatible endpoint
Aws::S3::S3Client s3(cfg);
Aws::S3::Model::HeadObjectRequest req;
req.SetBucket("an-existing-bucket");
req.SetKey("a-definitely-missing-key"); // -> 404 NoSuchKey
(void) s3.HeadObject(req); // logs "...time skew... Attempting to adjust the signer."
}
Aws::ShutdownAPI(options);
return 0;
}
```
A conditional `PutObject` with `If-None-Match: *` returning `412` reproduces it the same way.
### Possible Solution
Only log / enter the skew path when a skew is plausible or confirmed. Minimal fix — move the WARN into the branch that actually detects skew:
```cpp
if (diff >= TIME_DIFF_MAX || diff <= TIME_DIFF_MIN) {
AWS_LOGSTREAM_WARN(AWS_CLIENT_LOG_TAG,
"Signature check may have failed due to a clock skew of " << diff.count() << " ms; adjusting the signer.");
... // adjust
}
```
Better: gate `AdjustClockSkew` (or its body) on clock-skew-related conditions (e.g. `RequestTimeTooSkewed` / `RequestExpired` / signature errors) so unrelated errors (404/412/…) never parse the Date header or log.
### Additional Information/Context
Found in a high-conditional-request S3 workload (content-addressed storage: HEAD-before-PUT existence checks, `PUT If-None-Match` dedup → 412, `DELETE If-Match`). Every such non-2xx emitted the WARN; at debug/trace log level this was ~184k lines / ~98 MB in a single test run and initially looked like a signature/clock defect (it is not — the requests succeed, non-retryable errors break out without a retry). The impact is misleading logs + wasted Date parsing, not extra network retries.
### AWS CPP SDK version used
1.11.771 (also reproduces on `main`)
### Compiler and Version used
gcc (Ubuntu 14.2.0-19ubuntu2) 14.2.0
### Operating System and version
Ubuntu 25.04 (x86_64, kernel 6.14.0-37-generic)
Contributor guide
Research direction
Start in src/aws-cpp-sdk-core/source/client/AWSClient.cpp at the retry loop and AWSClient::AdjustClockSkew(), then trace the existing error and retry handling. Reproduce the behavior with the documented S3 HeadObject request for a missing key; done means ordinary 404/412 responses no longer emit the clock-skew warning or enter unnecessary skew detection, while actual skew handling still works.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- api
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 74/100