Comment cleanup pass

Against the team convention: say why not what, no archaeology, keep a warning
only where it stops someone undoing the thing it guards.

- DEFAULT_MAX_RATE_LIMIT_DURATION: states the invariant as an instruction to
  whoever changes it next, and adds the second consequence of parity -- the cap
  stops binding, because the remaining budget is always the smaller term.
- The drop branch: cut the rhetorical tail; the reason stands without it.
- The episode-clearing comment: reordered so the hazard leads and the mechanism
  supports it, rather than the other way round.
- "Same reasoning as above" now names the branch it refers to.
- request.py: dropped a line that restated the constant in words, directly above
  the line that gives its reason.
- test_client_defaults_to_the_documented_rate_limit_budget: rewritten in the
  present tense. It described how the bug had happened; it now states the
  constraint that makes asserting Consumer's default insufficient, which is what
  stops the test being "simplified" back.

140 passed, ruff and format clean.

Author: MichaelGHSeg

segment/analytics/consumer.py

@@ -25,9 +25,9 @@ class ShutdownInterrupted(Exception):
 # Default duration limits (12 hours in seconds)
 DEFAULT_MAX_TOTAL_BACKOFF_DURATION = 43200
 # Rate-limited attempts are deliberately uncounted, so this duration is the only
-# thing bounding them. It is deliberately several times MAX_RETRY_AFTER_SECONDS:
-# when the two are equal a single maximal Retry-After consumes the whole budget,
-# leaving one attempt and no retry at all.
+# thing bounding them. Keep it several times MAX_RETRY_AFTER_SECONDS: at parity a
+# single maximal Retry-After consumes the whole budget, leaving one attempt and no
+# retry, and the cap stops binding because the remaining budget is always smaller.
 DEFAULT_MAX_RATE_LIMIT_DURATION = 1800
 
 
@@ -173,11 +173,10 @@ def upload(self):
                 remaining = self.max_rate_limit_duration - (now - self.rate_limit_start_time)
                 wait_time = self.rate_limited_until - now
                 if wait_time > remaining:
-                    # Shortening the wait to fit the budget would send the next request
-                    # inside the window the server asked us to wait out — a request it
-                    # has already said it will not serve — and the budget would then be
-                    # spent, so it would be the last one anyway. Give up here instead of
-                    # spending a request to be told the same thing.
+                    # Shortening the wait to fit sends the next request inside the
+                    # window the server asked us to wait out, which it has already said
+                    # it will not serve, and the budget is spent by then so it would be
+                    # the last attempt either way.
                     self.log.error(
                         "Rate limit budget (%ds) cannot accommodate the requested wait; dropping batch.",
                         self.max_rate_limit_duration,
@@ -222,19 +221,19 @@ def upload(self):
                 self._requeue(batch)
                 success = False
             else:
-                # The request completed and carried no rate-limit signal, so the
-                # episode is over. Leaving the marker set strands it: this consumer
-                # outlives the batch, upload() returns at the empty-batch guard
-                # before the budget block, and nothing else clears it — so the next
-                # batch to arrive after the budget elapses is dropped for a rate
-                # limit that ended here, without ever being sent.
+                # The request completed carrying no rate-limit signal, so the
+                # episode is over. The marker outlives the batch and nothing else
+                # clears it — upload() returns at the empty-batch guard above,
+                # before the budget block — so leaving it set means the next batch
+                # to arrive after the budget elapses is dropped for a rate limit
+                # that ended here, without ever being sent.
                 self.clear_rate_limit_state()
                 self.log.error("error uploading: %s", e)
                 success = False
                 if self.on_error:
                     self.on_error(e, batch)
         except Exception as e:
-            # Same reasoning as above.
+            # Same reasoning as the non-rate-limited APIError branch above.
             self.clear_rate_limit_state()
             self.log.error("error uploading: %s", e)
             success = False

segment/analytics/request.py

@@ -15,7 +15,6 @@
 
 _session = sessions.Session()
 
-# Maximum Retry-After delay to respect (5 minutes)
 # A guard against an absurd header, not a second budget. Waiting less than the
 # server asked for does not make the next attempt more likely to succeed, it just
 # sends more requests at something already rate-limiting us; how long we keep

segment/analytics/test/test_consumer.py

@@ -1204,9 +1204,9 @@ def test_default_rate_limit_budget_exceeds_the_retry_after_cap(self):
     def test_client_defaults_to_the_documented_rate_limit_budget(self):
         """Pins what a real caller gets, which is not the same as Consumer's default.
 
-        Client.DefaultConfig carried its own literal and Client passes it into every
-        Consumer it builds, so raising only the Consumer default left every real user
-        on the old value while this suite stayed green.
+        Client passes its own DefaultConfig value into every Consumer it builds, so
+        asserting Consumer's default here would stay green while a drifted Client
+        default shipped.
         """
         client = Client("testsecret", send=False)
         try: