fix: address Copilot review — correct totalAttempts counting and drop batch on maxRateLimitDuration exceeded

- Move totalAttempts++ after rate-limit sleep check so it only counts actual HTTP requests
- Detect when maxRateLimitDuration is exceeded and drop the batch instead of silently retrying
- Update test expectations for X-Retry-Count and maxRateLimitDuration behavior
- Clamp token-manager backoff attempt to non-negative

Author: MichaelGHSeg

packages/node/src/lib/token-manager.ts

@@ -179,7 +179,7 @@ export class TokenManager implements ITokenManager {
     }
 
     const timeUntilRefreshInMs = backoff({
-      attempt: this.retryCount - 1,
+      attempt: Math.max(this.retryCount - 1, 0),
       minTimeout: 100,
       maxTimeout: 60 * 1000,
     })

packages/node/src/plugins/segmentio/__tests__/publisher.test.ts

@@ -697,7 +697,7 @@ describe('retry semantics', () => {
     expect(makeReqSpy).toHaveBeenCalledTimes(2)
     const [first, second] = getAllRequests()
     expect(first.headers['X-Retry-Count']).toBeUndefined()
-    expect(second.headers['X-Retry-Count']).toBe('2')
+    expect(second.headers['X-Retry-Count']).toBe('1')
   })
 
   it('T07 408 uses backoff (Retry-After header ignored)', async () => {
@@ -964,7 +964,7 @@ describe('retry semantics', () => {
     expect(makeReqSpy).toHaveBeenCalledTimes(2)
     const [first, second] = getAllRequests()
     expect(first.headers['X-Retry-Count']).toBeUndefined()
-    expect(second.headers['X-Retry-Count']).toBe('2')
+    expect(second.headers['X-Retry-Count']).toBe('1')
     expect(updated.failedDelivery()).toBeFalsy()
   })
 
@@ -1204,35 +1204,37 @@ describe('retry semantics', () => {
     expect(makeReqSpy.mock.calls.length).toBeLessThan(100)
   })
 
-  it('T20 maxRateLimitDuration: clears rate-limit window and resumes send', async () => {
+  it('T20 maxRateLimitDuration: drops batch after duration exceeded', async () => {
     jest.useFakeTimers()
     const headers = new TestHeaders()
     headers.set('Retry-After', '60')
 
-    makeReqSpy
-      .mockReturnValueOnce(
-        createError({
-          status: 429,
-          statusText: 'Too Many Requests',
-          ...headers,
-        })
-      )
-      .mockReturnValue(createSuccess())
+    makeReqSpy.mockReturnValue(
+      createError({
+        status: 429,
+        statusText: 'Too Many Requests',
+        ...headers,
+      })
+    )
 
     const { plugin: segmentPlugin } = createTestNodePlugin({
       maxRetries: 3,
       flushAt: 1,
       maxRateLimitDuration: 1, // 1 second
     })
 
-    // First event gets 429, then resumes after maxRateLimitDuration elapses.
+    // First event gets 429, rate-limit state is set
     const ctx1 = trackEvent()
     const pending = segmentPlugin.track(ctx1)
     expect(makeReqSpy).toHaveBeenCalledTimes(1)
 
+    // Advance past maxRateLimitDuration — batch should be dropped
     await jest.advanceTimersByTimeAsync(1000)
     const updated1 = await pending
-    expect(updated1.failedDelivery()).toBeFalsy()
-    expect(makeReqSpy).toHaveBeenCalledTimes(2)
+    expect(updated1.failedDelivery()).toBeTruthy()
+    const err = updated1.failedDelivery()!.reason as Error
+    expect(err.message).toContain('Rate limit duration exceeded')
+    // Only 1 actual HTTP request — the initial 429
+    expect(makeReqSpy).toHaveBeenCalledTimes(1)
   })
 })

packages/node/src/plugins/segmentio/publisher.ts

@@ -262,12 +262,12 @@ export class Publisher {
   private _isRateLimited(): boolean {
     if (this._rateLimitedUntil === undefined) return false
 
-    // Check if maxRateLimitDuration has been exceeded
+    // Check if maxRateLimitDuration has been exceeded.
+    // Returns false so the caller can detect the cleared state and drop the batch.
     if (
       this._rateLimitStartTime !== undefined &&
       Date.now() - this._rateLimitStartTime >= this._maxRateLimitDuration * 1000
     ) {
-      // Clear rate-limit state; caller will drop batch
       this._rateLimitedUntil = undefined
       this._rateLimitStartTime = undefined
       return false
@@ -313,9 +313,8 @@ export class Publisher {
 
     // eslint-disable-next-line no-constant-condition
     while (true) {
-      totalAttempts++
-
       // Check rate-limit state before making a request
+      const wasRateLimited = this._rateLimitStartTime !== undefined
       if (this._isRateLimited()) {
         const untilRetryAfter = Math.max(
           0,
@@ -334,10 +333,12 @@ export class Publisher {
         continue
       }
 
-      // Check if maxRateLimitDuration was exceeded (cleared by _isRateLimited)
-      // If we had a rateLimitStartTime but it got cleared due to duration,
-      // and we're in a 429 requeue cycle, drop the batch
-      // (This is handled by _isRateLimited returning false after clearing state)
+      // If we were rate-limited but _isRateLimited() now returns false with
+      // cleared state, maxRateLimitDuration was exceeded — drop the batch.
+      if (wasRateLimited && this._rateLimitStartTime === undefined) {
+        resolveFailedBatch(batch, new Error('Rate limit duration exceeded'))
+        return
+      }
 
       let failureReason: unknown
       let shouldRetry = false
@@ -355,6 +356,8 @@ export class Publisher {
           }
         }
 
+        totalAttempts++
+
         const headers: Record<string, string> = {
           'Content-Type': 'application/json',
           'User-Agent': 'analytics-node-next/latest',