Skip to content

Commit a252e94

Browse files
MichaelGHSegclaude
andcommitted
Address PR review: thread safety, comments, and test fixes
- Add synchronized to rate-limit state methods to fix check-then-act race condition - Add comments explaining defensive 429 and non-standard 460 in isStatusRetryWithBackoff - Fix outdated test comments and line references - Widen timing assertion upper bound (3s → 5s) to reduce CI flakiness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 9e7d140 commit a252e94

2 files changed

Lines changed: 10 additions & 8 deletions

File tree

analytics/src/main/java/com/segment/analytics/internal/AnalyticsClient.java

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ public void flush() {
229229
}
230230
}
231231

232-
void setRateLimitState(long retryAfterSeconds) {
232+
synchronized void setRateLimitState(long retryAfterSeconds) {
233233
long now = System.currentTimeMillis();
234234
if (rateLimitStartTime == 0) {
235235
rateLimitStartTime = now;
@@ -238,13 +238,13 @@ void setRateLimitState(long retryAfterSeconds) {
238238
rateLimited = true;
239239
}
240240

241-
void clearRateLimitState() {
241+
synchronized void clearRateLimitState() {
242242
rateLimited = false;
243243
rateLimitWaitUntil = 0;
244244
rateLimitStartTime = 0;
245245
}
246246

247-
boolean isRateLimited() {
247+
synchronized boolean isRateLimited() {
248248
if (!rateLimited) return false;
249249
if (System.currentTimeMillis() >= rateLimitWaitUntil) {
250250
rateLimited = false;
@@ -702,7 +702,10 @@ public void run() {
702702
}
703703

704704
private static boolean isStatusRetryWithBackoff(int status) {
705-
// Explicitly retry these client errors
705+
// Explicitly retry these client errors.
706+
// 429 is also handled earlier via isStatusRetryAfterEligible (RATE_LIMITED strategy);
707+
// including it here is defensive in case call-site ordering changes.
708+
// 460 is a non-standard, Segment-specific status code for transient retryable failures.
706709
if (status == 408 || status == 410 || status == 429 || status == 460) {
707710
return true;
708711
}

analytics/src/test/java/com/segment/analytics/internal/AnalyticsClientTest.java

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -618,8 +618,7 @@ public Call<UploadResponse> answer(InvocationOnMock invocation) {
618618
BatchUploadTask batchUploadTask = new BatchUploadTask(client, BACKO, batch, 10);
619619
batchUploadTask.run();
620620

621-
// DEFAULT_RETRIES == maxRetries
622-
// tries 11(one normal run + 10 retries) even though default is 50 in AnalyticsClient.java
621+
// maxRetries=10 => tries 11 (one initial attempt + 10 retries)
623622
verify(segmentService, times(11)).upload(anyInt(), isNull(), eq(batch));
624623
verify(callback)
625624
.failure(
@@ -732,7 +731,7 @@ public void retryAfterHeaderRespectsShortDelay() {
732731
Batch batch = batchFor(trackMessage);
733732

734733
// Test with a short Retry-After delay (2 seconds) to verify the mechanism works
735-
// The cap at 300 seconds is verified by code inspection at AnalyticsClient.java:556-558
734+
// The cap at 300 seconds is verified by code inspection at AnalyticsClient.java:610-612
736735
Response<UploadResponse> rateLimitedWithShortDelay = errorWithRetryAfter(429, "2");
737736
Response<UploadResponse> successResponse = Response.success(200, response);
738737

@@ -747,7 +746,7 @@ public void retryAfterHeaderRespectsShortDelay() {
747746

748747
// Should wait approximately 2 seconds
749748
long elapsedMs = endTime - startTime;
750-
assertThat(elapsedMs).isGreaterThanOrEqualTo(2000L).isLessThan(3000L);
749+
assertThat(elapsedMs).isGreaterThanOrEqualTo(2000L).isLessThan(5000L);
751750

752751
verify(segmentService, times(2)).upload(anyInt(), isNull(), eq(batch));
753752
verify(callback).success(trackMessage);

0 commit comments

Comments
 (0)