Response status updates

[MichaelGHSeg]

No description provided.

[Copilot]

Pull request overview

This pull request implements comprehensive improvements to the response status handling and retry mechanism for the Segment Analytics client. The changes introduce Retry-After header support, update the retry logic to distinguish between server-directed and backoff-based retries, add an X-Retry-Count header to all requests, and move authentication from URL to Authorization header using Basic auth.

Changes:

• Implemented Retry-After header parsing and handling for status codes 429, 408, and 503, with retries based on server-specified delays not counting against the client's retry budget
• Added X-Retry-Count header to all upload requests to communicate retry attempt number (starting at 0)
• Moved authentication from URL-based to Authorization header using HTTP Basic auth with the write key
• Expanded retryable status codes to include specific 4xx errors (408, 410, 429, 460) and refined 5xx retry logic to exclude 501, 505, and 511
• Adjusted backoff configuration (100ms base, 1min cap) and increased default maximumFlushAttempts from 3 to 1000
• Added comprehensive test coverage for new retry scenarios

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
analytics-core/src/main/java/com/segment/analytics/http/SegmentService.java	Added X-Retry-Count header parameter to upload method signature
analytics/src/main/java/com/segment/analytics/AnalyticsRequestInterceptor.java	Added writeKey parameter and Authorization header construction using Basic auth
analytics/src/main/java/com/segment/analytics/Analytics.java	Updated interceptor instantiation with writeKey and increased default maximumFlushAttempts to 1000
analytics/src/main/java/com/segment/analytics/internal/AnalyticsClient.java	Implemented retry strategy enum, Retry-After header parsing, updated backoff configuration, refined status code retry logic, and separated tracking of total attempts vs backoff attempts
analytics/src/test/java/com/segment/analytics/internal/AnalyticsClientTest.java	Updated all mock calls to include retry count parameter and added 7 new test cases covering retryable client errors, non-retryable 5xx errors, Retry-After handling, X-Retry-Count header verification, and 413 Payload Too Large behavior
analytics/src/test/java/com/segment/analytics/AnalyticsRequestInterceptorTest.java	Added assertion to verify Authorization header is correctly set with Basic auth credentials

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The default value increase from 3 to 1000 for maximumFlushAttempts seems excessive and potentially problematic. This change dramatically increases the number of retry attempts from 3 to 1000 by default, which could lead to: 1) Significantly longer delays before giving up on failed requests, 2) Increased resource consumption for repeatedly failing requests, and 3) Potential operational issues if the service is down. While the comment mentions this is to "accommodate shorter max retry backoff", the backoff was changed from 15s base/1h cap to 100ms base/1m cap, which means retries will happen faster but the sheer number (1000) seems unreasonably high. Consider a more balanced value that accounts for the faster backoff without creating an unbounded retry scenario (e.g., 10-50 attempts).

Suggested change

	// Adjusted upward to accomodate shorter max retry backoff.
	maximumFlushAttempts = 1000;
	// Bounded default to accommodate shorter max retry backoff without excessive retries.
	maximumFlushAttempts = 50;

[Copilot]

The parseRetryAfterSeconds method handles several edge cases (null, empty, negative, zero, over cap) but lacks explicit test coverage for these scenarios. While the happy path is tested in retryAfterHeaderRespectsShortDelay, consider adding tests for edge cases such as: invalid numeric format, negative values, zero, values exceeding MAX_RETRY_AFTER_SECONDS, empty strings, and whitespace-only strings to ensure the method is robust.

[Copilot]

Missing test coverage for HTTP status codes 410 and 460, which are explicitly marked as retryable in the isStatusRetryWithBackoff method. Add tests to verify that these status codes are indeed retried with backoff as intended.

[Copilot]

Missing test coverage for HTTP status code 503 (Service Unavailable), which is both Retry-After eligible and retryable with backoff. Add tests to verify: 1) 503 with a valid Retry-After header uses the RETRY_AFTER strategy, and 2) 503 without a Retry-After header falls back to the BACKOFF strategy.

[Copilot]

Missing test coverage for HTTP status codes 505 (HTTP Version Not Supported) and 511 (Network Authentication Required), which are explicitly excluded from retry logic in the isStatusRetryWithBackoff method. While 501 is tested, add tests for 505 and 511 to ensure these status codes are not retried as intended.

[Copilot]

HTTP status 410 (Gone) is being treated as retryable, but this status code typically indicates that a resource has been permanently removed and will never be available again. Retrying such requests is unlikely to succeed and wastes resources. Consider removing 410 from the list of retryable client errors unless there's a specific reason the Segment API uses 410 in a non-standard way to indicate a temporary condition.

Suggested change

	if (status == 408 || status == 410 || status == 429 || status == 460) {
	if (status == 408 || status == 429 || status == 460) {

[Copilot]

The Retry-After header parsing only handles numeric (delay-seconds) format but not HTTP-date format. According to RFC 7231, the Retry-After header can be either a delay in seconds or an HTTP-date. While delay-seconds is more common, some servers may use HTTP-date format. Consider adding support for HTTP-date parsing to be fully RFC-compliant, or document that only delay-seconds format is supported.

[Copilot]

Missing test coverage for the fallback behavior when a status code is Retry-After eligible (429, 408, 503) but the Retry-After header is missing or invalid. In this case, the code should fall back to the BACKOFF strategy. Add a test to verify this fallback behavior works correctly, ensuring that requests with invalid Retry-After headers still get retried using exponential backoff instead of failing immediately.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

There's a race condition in the setRateLimitState method. Multiple threads could call this method concurrently (from different BatchUploadTask instances), and the compound operation of checking rateLimitStartTime and setting it is not atomic. This could result in rateLimitStartTime being set incorrectly or multiple times, leading to inaccurate maxRateLimitDuration checks.

The check-then-act pattern at lines 234-236 should be synchronized or use atomic operations to ensure thread safety.

[Copilot]

Status code 429 is checked in both isStatusRetryAfterEligible (line 594) and isStatusRetryWithBackoff (line 706). Since isStatusRetryAfterEligible is checked first in the upload method (line 543), any 429 response will always be handled by the RATE_LIMITED strategy before isStatusRetryWithBackoff is ever called. This makes the inclusion of 429 in isStatusRetryWithBackoff redundant, though it may serve as defensive programming.

Consider removing 429 from isStatusRetryWithBackoff to avoid confusion, or add a comment explaining it's defensive.

Suggested change

	// Explicitly retry these client errors
	// Explicitly retry these client errors.
	// Note: 429 is also handled earlier via isStatusRetryAfterEligible (RATE_LIMITED strategy).
	// Including 429 here is defensive, so that if call-site ordering changes it will still be retried.

[Copilot]

HTTP status code 410 (Gone) is marked as retryable in the isStatusRetryWithBackoff method. However, 410 typically indicates that a resource is permanently gone and will not come back, making it semantically different from transient errors. Retrying a 410 is unlikely to succeed and may waste resources.

Consider removing 410 from the retryable status codes unless there's a specific reason why the Segment API uses 410 for transient errors. If 410 is intentionally retryable for this API, add a comment explaining why.

Suggested change

	// Explicitly retry these client errors
	if (status == 408 || status == 410 || status == 429 || status == 460) {
	// Explicitly retry these client errors (transient or rate-limited)
	if (status == 408 || status == 429 || status == 460) {

[Copilot]

The maxRateLimitDuration check at lines 640-644 happens AFTER setRateLimitState is called (line 637). This means that if maxRateLimitDuration is very short (e.g., 1ms as tested), and the first 429 response is received, the rate limit state is set, then immediately the check might pass and the batch is dropped. However, the batch hasn't even attempted to wait for the Retry-After period yet (that happens at line 650).

Consider moving the maxRateLimitDuration check to AFTER the sleep, or restructure the logic so that the batch gets at least one chance to retry after the Retry-After period expires.

[Copilot]

The default maximumFlushAttempts has been increased from 3 to 1000. While the comment mentions this is to "accommodate shorter max retry backoff," this is a dramatic increase (over 300x) that significantly changes the retry behavior. Combined with the new maxTotalBackoffDurationMs limit (12 hours by default), this could result in many more retry attempts within that window.

Consider whether 1000 is the appropriate value, or if a more moderate increase would be sufficient. Also, ensure this change is documented in release notes as it significantly affects operational behavior.

Suggested change

	// Adjusted upward to accommodate shorter max retry backoff.
	maximumFlushAttempts = 1000;
	// Increased from 3 to accommodate shorter max retry backoff while keeping retries bounded.
	maximumFlushAttempts = 10;

[Copilot]

There's a race condition in the isRateLimited method. The compound operation at lines 249-251 (checking the time and clearing the rateLimited flag) is not atomic. Multiple threads calling this method concurrently could result in inconsistent state where some threads see the rate limit as active while others see it as inactive, even though they're checking at the same time.

This should be synchronized or use atomic operations to ensure thread safety.

[Copilot]

The status code 460 is checked in isStatusRetryWithBackoff at line 706. However, HTTP status code 460 is not a standard HTTP status code (it's a non-standard code used by some specific services). This appears to be intentional for compatibility with specific backend behavior, but it would be helpful to add a comment explaining why this non-standard status code is included in the retry logic.

Suggested change

	// Explicitly retry these client errors
	// Explicitly retry these client errors.
	// Note: 460 is a non-standard status code used by certain backends to indicate
	// transient, retryable failures; it is intentionally included here for compatibility.

[Copilot]

The validation converts the duration to seconds for validation but stores the value in milliseconds. This could allow an edge case where a duration less than 1 second but greater than 0 milliseconds (e.g., 500 milliseconds) would pass validation because unit.toSeconds(duration) would round down to 0 and fail the check. However, a duration of exactly 1000 milliseconds would pass (1 second) but a duration of 999 milliseconds would incorrectly fail even though it's valid in milliseconds.

Consider validating in milliseconds instead: if (unit.toMillis(duration) < 1000) or documenting that the minimum is 1 second (not 1 millisecond).

[Copilot]

The validation converts the duration to seconds for validation but stores the value in milliseconds. This could allow an edge case where a duration less than 1 second but greater than 0 milliseconds (e.g., 500 milliseconds) would pass validation because unit.toSeconds(duration) would round down to 0 and fail the check. However, a duration of exactly 1000 milliseconds would pass (1 second) but a duration of 999 milliseconds would incorrectly fail even though it's valid in milliseconds.

Consider validating in milliseconds instead: if (unit.toMillis(duration) < 1000) or documenting that the minimum is 1 second (not 1 millisecond).

[Copilot]

When rate-limited and the batch is deferred in the Looper (lines 407-419), the messages remain in the LinkedList but are never submitted if rate-limiting persists indefinitely. While the Looper thread will eventually process these on the next flush trigger or shutdown, if the rate-limit state lasts a very long time, these messages accumulate in memory.

Consider adding monitoring or logging to track how long messages remain in the deferred state, especially if rate-limiting persists for extended periods.