Handle Retry-After on every retryable status, including 529 - #148

Open
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after
Open

Handle Retry-After on every retryable status, including 529#148
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after

Conversation

@MichaelGHSeg

Copy link
Copy Markdown
Contributor

Stacked on #147 — review that first; this PR's diff is the single 529 commit.

Why

We were asked to add 529 support with Retry-After headers. The conclusion was to check Retry-After on every retryable status rather than special-casing 429 — 529 is simply one more member of the retryable set.

This is the same change already shipped in analytics-java 3.5.5, and matches the generic-retry-after conformance suite merged in sdk-e2e-tests#16.

What

  • Any retryable response carrying a valid Retry-After routes through the rate-limit path, which does not consume retry budget.
  • Retryable statuses withoutRetry-After continue to use counted exponential backoff.
  • Adds RetryAfterParser, handling both delta-seconds and RFC 1123 HTTP-date forms (returning null for dates in the past).

Testing

  • 229 unit tests pass on the combined branch (this commit plus Expose HttpConfig so retry behaviour is user-configurable #147), including new coverage for 503/529 with and without Retry-After, and clamping to maxRetryInterval.
  • Full e2e suite passes locally: 79 passed / 0 skipped across 11 files, including the 11-test generic-retry-after (529) suite and all four retry-settings suites.

E2E was run locally because CI cannot currently check out the private sdk-e2e-tests repo — the token: ${{ secrets.E2E_TESTS_TOKEN }} line was dropped from that checkout step during CI hardening. That's tracked separately; note the same omission is present in this repo's ci/harden-build-publish branch and in go/ruby/php.

Related

The equivalent change is open for the other SDKs: analytics-python#520, analytics-go#206, analytics-ruby#280, analytics-php#251.

Route any retryable response carrying a valid Retry-After header through the
rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable
statuses without Retry-After continue to use counted exponential backoff. Adds
529 to the retryable set and covers both paths with tests.
Matches the behaviour already shipped in analytics-java 3.5.5 and the
generic-retry-after conformance suite in sdk-e2e-tests.
Routing any retryable status with Retry-After to the rate-limit path left
ShouldDeleteBatch inconsistent with HandleResponse. With rate limiting on and
backoff off, a 503 or 529 carrying Retry-After would rate-limit the pipeline
(WaitUntilTime set, uploads blocked) while ShouldDeleteBatch still reported
true, so the batch file was deleted and the pipeline then stalled waiting to
retry events that no longer existed.
That configuration is reachable from CDN settings and directly from
Configuration.HttpConfig — it is the config ConfigurationHttpConfigTest builds.
ShouldDeleteBatch now keeps a retryable batch whenever rate limiting is
enabled, matching swift's shouldDropBatch ("Rate limit config handles retryable
codes that carry Retry-After — don't drop"). Non-retryable statuses are still
dropped, and a retryable status with neither rate limiting nor backoff enabled
is still dropped since nothing would retry it.
The previous commit kept a retryable batch whenever rate limiting was enabled,
which was too broad: a 500 with no Retry-After and backoff disabled was also
kept, so the file was re-uploaded even though nothing had scheduled a retry.
The sdk-e2e-tests "backoffConfig.enabled: false" case caught this — it expects
exactly one request and saw two.
ShouldDeleteBatch now takes the same retryAfterSeconds value handed to
HandleResponse, so the two agree on whether the response actually took the
rate-limit path. A retryable status keeps its batch only when it carries a
usable Retry-After and rate limiting is on; otherwise only backoff can retry
it, and with backoff off the batch is dropped as before. The single-argument
overload is retained.
232 tests pass.
Analytics-CSharp-plan.md states 'Spec item 1: 2xx and 3xx are success', but
IsSuccessStatusCode and the two status checks in RetryStateMachine were
2xx-only, so a 3xx fell through to the retry classifier. analytics-go,
analytics-python and analytics-php already follow the spec here; this brings C#
into line with them and with its own plan.
236 tests pass, including new cases covering 200, 201, 301 and 304.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@MichaelGHSeg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Handle Retry-After on every retryable status, including 529 - #148

Open
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after
Open

Handle Retry-After on every retryable status, including 529#148
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after

Conversation

@MichaelGHSeg

Copy link
Copy Markdown
Contributor

Stacked on #147 — review that first; this PR's diff is the single 529 commit.

Why

We were asked to add 529 support with Retry-After headers. The conclusion was to check Retry-After on every retryable status rather than special-casing 429 — 529 is simply one more member of the retryable set.

This is the same change already shipped in analytics-java 3.5.5, and matches the generic-retry-after conformance suite merged in sdk-e2e-tests#16.

What

  • Any retryable response carrying a valid Retry-After routes through the rate-limit path, which does not consume retry budget.
  • Retryable statuses withoutRetry-After continue to use counted exponential backoff.
  • Adds RetryAfterParser, handling both delta-seconds and RFC 1123 HTTP-date forms (returning null for dates in the past).

Testing

  • 229 unit tests pass on the combined branch (this commit plus Expose HttpConfig so retry behaviour is user-configurable #147), including new coverage for 503/529 with and without Retry-After, and clamping to maxRetryInterval.
  • Full e2e suite passes locally: 79 passed / 0 skipped across 11 files, including the 11-test generic-retry-after (529) suite and all four retry-settings suites.

E2E was run locally because CI cannot currently check out the private sdk-e2e-tests repo — the token: ${{ secrets.E2E_TESTS_TOKEN }} line was dropped from that checkout step during CI hardening. That's tracked separately; note the same omission is present in this repo's ci/harden-build-publish branch and in go/ruby/php.

Related

The equivalent change is open for the other SDKs: analytics-python#520, analytics-go#206, analytics-ruby#280, analytics-php#251.

Route any retryable response carrying a valid Retry-After header through the
rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable
statuses without Retry-After continue to use counted exponential backoff. Adds
529 to the retryable set and covers both paths with tests.
Matches the behaviour already shipped in analytics-java 3.5.5 and the
generic-retry-after conformance suite in sdk-e2e-tests.
Routing any retryable status with Retry-After to the rate-limit path left
ShouldDeleteBatch inconsistent with HandleResponse. With rate limiting on and
backoff off, a 503 or 529 carrying Retry-After would rate-limit the pipeline
(WaitUntilTime set, uploads blocked) while ShouldDeleteBatch still reported
true, so the batch file was deleted and the pipeline then stalled waiting to
retry events that no longer existed.
That configuration is reachable from CDN settings and directly from
Configuration.HttpConfig — it is the config ConfigurationHttpConfigTest builds.
ShouldDeleteBatch now keeps a retryable batch whenever rate limiting is
enabled, matching swift's shouldDropBatch ("Rate limit config handles retryable
codes that carry Retry-After — don't drop"). Non-retryable statuses are still
dropped, and a retryable status with neither rate limiting nor backoff enabled
is still dropped since nothing would retry it.
The previous commit kept a retryable batch whenever rate limiting was enabled,
which was too broad: a 500 with no Retry-After and backoff disabled was also
kept, so the file was re-uploaded even though nothing had scheduled a retry.
The sdk-e2e-tests "backoffConfig.enabled: false" case caught this — it expects
exactly one request and saw two.
ShouldDeleteBatch now takes the same retryAfterSeconds value handed to
HandleResponse, so the two agree on whether the response actually took the
rate-limit path. A retryable status keeps its batch only when it carries a
usable Retry-After and rate limiting is on; otherwise only backoff can retry
it, and with backoff off the batch is dropped as before. The single-argument
overload is retained.
232 tests pass.
Analytics-CSharp-plan.md states 'Spec item 1: 2xx and 3xx are success', but
IsSuccessStatusCode and the two status checks in RetryStateMachine were
2xx-only, so a 3xx fell through to the retry classifier. analytics-go,
analytics-python and analytics-php already follow the spec here; this brings C#
into line with them and with its own plan.
236 tests pass, including new cases covering 200, 201, 301 and 304.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@MichaelGHSeg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Handle Retry-After on every retryable status, including 529 - #148

Open
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after
Open

Handle Retry-After on every retryable status, including 529#148
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after

Conversation

@MichaelGHSeg

Copy link
Copy Markdown
Contributor

Stacked on #147 — review that first; this PR's diff is the single 529 commit.

Why

We were asked to add 529 support with Retry-After headers. The conclusion was to check Retry-After on every retryable status rather than special-casing 429 — 529 is simply one more member of the retryable set.

This is the same change already shipped in analytics-java 3.5.5, and matches the generic-retry-after conformance suite merged in sdk-e2e-tests#16.

What

  • Any retryable response carrying a valid Retry-After routes through the rate-limit path, which does not consume retry budget.
  • Retryable statuses withoutRetry-After continue to use counted exponential backoff.
  • Adds RetryAfterParser, handling both delta-seconds and RFC 1123 HTTP-date forms (returning null for dates in the past).

Testing

  • 229 unit tests pass on the combined branch (this commit plus Expose HttpConfig so retry behaviour is user-configurable #147), including new coverage for 503/529 with and without Retry-After, and clamping to maxRetryInterval.
  • Full e2e suite passes locally: 79 passed / 0 skipped across 11 files, including the 11-test generic-retry-after (529) suite and all four retry-settings suites.

E2E was run locally because CI cannot currently check out the private sdk-e2e-tests repo — the token: ${{ secrets.E2E_TESTS_TOKEN }} line was dropped from that checkout step during CI hardening. That's tracked separately; note the same omission is present in this repo's ci/harden-build-publish branch and in go/ruby/php.

Related

The equivalent change is open for the other SDKs: analytics-python#520, analytics-go#206, analytics-ruby#280, analytics-php#251.

Route any retryable response carrying a valid Retry-After header through the
rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable
statuses without Retry-After continue to use counted exponential backoff. Adds
529 to the retryable set and covers both paths with tests.
Matches the behaviour already shipped in analytics-java 3.5.5 and the
generic-retry-after conformance suite in sdk-e2e-tests.
Routing any retryable status with Retry-After to the rate-limit path left
ShouldDeleteBatch inconsistent with HandleResponse. With rate limiting on and
backoff off, a 503 or 529 carrying Retry-After would rate-limit the pipeline
(WaitUntilTime set, uploads blocked) while ShouldDeleteBatch still reported
true, so the batch file was deleted and the pipeline then stalled waiting to
retry events that no longer existed.
That configuration is reachable from CDN settings and directly from
Configuration.HttpConfig — it is the config ConfigurationHttpConfigTest builds.
ShouldDeleteBatch now keeps a retryable batch whenever rate limiting is
enabled, matching swift's shouldDropBatch ("Rate limit config handles retryable
codes that carry Retry-After — don't drop"). Non-retryable statuses are still
dropped, and a retryable status with neither rate limiting nor backoff enabled
is still dropped since nothing would retry it.
The previous commit kept a retryable batch whenever rate limiting was enabled,
which was too broad: a 500 with no Retry-After and backoff disabled was also
kept, so the file was re-uploaded even though nothing had scheduled a retry.
The sdk-e2e-tests "backoffConfig.enabled: false" case caught this — it expects
exactly one request and saw two.
ShouldDeleteBatch now takes the same retryAfterSeconds value handed to
HandleResponse, so the two agree on whether the response actually took the
rate-limit path. A retryable status keeps its batch only when it carries a
usable Retry-After and rate limiting is on; otherwise only backoff can retry
it, and with backoff off the batch is dropped as before. The single-argument
overload is retained.
232 tests pass.
Analytics-CSharp-plan.md states 'Spec item 1: 2xx and 3xx are success', but
IsSuccessStatusCode and the two status checks in RetryStateMachine were
2xx-only, so a 3xx fell through to the retry classifier. analytics-go,
analytics-python and analytics-php already follow the spec here; this brings C#
into line with them and with its own plan.
236 tests pass, including new cases covering 200, 201, 301 and 304.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@MichaelGHSeg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Handle Retry-After on every retryable status, including 529 - #148

Open
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after
Open

Handle Retry-After on every retryable status, including 529#148
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after

Conversation

@MichaelGHSeg

Copy link
Copy Markdown
Contributor

Stacked on #147 — review that first; this PR's diff is the single 529 commit.

Why

We were asked to add 529 support with Retry-After headers. The conclusion was to check Retry-After on every retryable status rather than special-casing 429 — 529 is simply one more member of the retryable set.

This is the same change already shipped in analytics-java 3.5.5, and matches the generic-retry-after conformance suite merged in sdk-e2e-tests#16.

What

  • Any retryable response carrying a valid Retry-After routes through the rate-limit path, which does not consume retry budget.
  • Retryable statuses withoutRetry-After continue to use counted exponential backoff.
  • Adds RetryAfterParser, handling both delta-seconds and RFC 1123 HTTP-date forms (returning null for dates in the past).

Testing

  • 229 unit tests pass on the combined branch (this commit plus Expose HttpConfig so retry behaviour is user-configurable #147), including new coverage for 503/529 with and without Retry-After, and clamping to maxRetryInterval.
  • Full e2e suite passes locally: 79 passed / 0 skipped across 11 files, including the 11-test generic-retry-after (529) suite and all four retry-settings suites.

E2E was run locally because CI cannot currently check out the private sdk-e2e-tests repo — the token: ${{ secrets.E2E_TESTS_TOKEN }} line was dropped from that checkout step during CI hardening. That's tracked separately; note the same omission is present in this repo's ci/harden-build-publish branch and in go/ruby/php.

Related

The equivalent change is open for the other SDKs: analytics-python#520, analytics-go#206, analytics-ruby#280, analytics-php#251.

Route any retryable response carrying a valid Retry-After header through the
rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable
statuses without Retry-After continue to use counted exponential backoff. Adds
529 to the retryable set and covers both paths with tests.
Matches the behaviour already shipped in analytics-java 3.5.5 and the
generic-retry-after conformance suite in sdk-e2e-tests.
Routing any retryable status with Retry-After to the rate-limit path left
ShouldDeleteBatch inconsistent with HandleResponse. With rate limiting on and
backoff off, a 503 or 529 carrying Retry-After would rate-limit the pipeline
(WaitUntilTime set, uploads blocked) while ShouldDeleteBatch still reported
true, so the batch file was deleted and the pipeline then stalled waiting to
retry events that no longer existed.
That configuration is reachable from CDN settings and directly from
Configuration.HttpConfig — it is the config ConfigurationHttpConfigTest builds.
ShouldDeleteBatch now keeps a retryable batch whenever rate limiting is
enabled, matching swift's shouldDropBatch ("Rate limit config handles retryable
codes that carry Retry-After — don't drop"). Non-retryable statuses are still
dropped, and a retryable status with neither rate limiting nor backoff enabled
is still dropped since nothing would retry it.
The previous commit kept a retryable batch whenever rate limiting was enabled,
which was too broad: a 500 with no Retry-After and backoff disabled was also
kept, so the file was re-uploaded even though nothing had scheduled a retry.
The sdk-e2e-tests "backoffConfig.enabled: false" case caught this — it expects
exactly one request and saw two.
ShouldDeleteBatch now takes the same retryAfterSeconds value handed to
HandleResponse, so the two agree on whether the response actually took the
rate-limit path. A retryable status keeps its batch only when it carries a
usable Retry-After and rate limiting is on; otherwise only backoff can retry
it, and with backoff off the batch is dropped as before. The single-argument
overload is retained.
232 tests pass.
Analytics-CSharp-plan.md states 'Spec item 1: 2xx and 3xx are success', but
IsSuccessStatusCode and the two status checks in RetryStateMachine were
2xx-only, so a 3xx fell through to the retry classifier. analytics-go,
analytics-python and analytics-php already follow the spec here; this brings C#
into line with them and with its own plan.
236 tests pass, including new cases covering 200, 201, 301 and 304.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@MichaelGHSeg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Handle Retry-After on every retryable status, including 529 - #148

Open
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after
Open

Handle Retry-After on every retryable status, including 529#148
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after

Conversation

@MichaelGHSeg

Copy link
Copy Markdown
Contributor

Stacked on #147 — review that first; this PR's diff is the single 529 commit.

Why

We were asked to add 529 support with Retry-After headers. The conclusion was to check Retry-After on every retryable status rather than special-casing 429 — 529 is simply one more member of the retryable set.

This is the same change already shipped in analytics-java 3.5.5, and matches the generic-retry-after conformance suite merged in sdk-e2e-tests#16.

What

  • Any retryable response carrying a valid Retry-After routes through the rate-limit path, which does not consume retry budget.
  • Retryable statuses withoutRetry-After continue to use counted exponential backoff.
  • Adds RetryAfterParser, handling both delta-seconds and RFC 1123 HTTP-date forms (returning null for dates in the past).

Testing

  • 229 unit tests pass on the combined branch (this commit plus Expose HttpConfig so retry behaviour is user-configurable #147), including new coverage for 503/529 with and without Retry-After, and clamping to maxRetryInterval.
  • Full e2e suite passes locally: 79 passed / 0 skipped across 11 files, including the 11-test generic-retry-after (529) suite and all four retry-settings suites.

E2E was run locally because CI cannot currently check out the private sdk-e2e-tests repo — the token: ${{ secrets.E2E_TESTS_TOKEN }} line was dropped from that checkout step during CI hardening. That's tracked separately; note the same omission is present in this repo's ci/harden-build-publish branch and in go/ruby/php.

Related

The equivalent change is open for the other SDKs: analytics-python#520, analytics-go#206, analytics-ruby#280, analytics-php#251.

Route any retryable response carrying a valid Retry-After header through the
rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable
statuses without Retry-After continue to use counted exponential backoff. Adds
529 to the retryable set and covers both paths with tests.
Matches the behaviour already shipped in analytics-java 3.5.5 and the
generic-retry-after conformance suite in sdk-e2e-tests.
Routing any retryable status with Retry-After to the rate-limit path left
ShouldDeleteBatch inconsistent with HandleResponse. With rate limiting on and
backoff off, a 503 or 529 carrying Retry-After would rate-limit the pipeline
(WaitUntilTime set, uploads blocked) while ShouldDeleteBatch still reported
true, so the batch file was deleted and the pipeline then stalled waiting to
retry events that no longer existed.
That configuration is reachable from CDN settings and directly from
Configuration.HttpConfig — it is the config ConfigurationHttpConfigTest builds.
ShouldDeleteBatch now keeps a retryable batch whenever rate limiting is
enabled, matching swift's shouldDropBatch ("Rate limit config handles retryable
codes that carry Retry-After — don't drop"). Non-retryable statuses are still
dropped, and a retryable status with neither rate limiting nor backoff enabled
is still dropped since nothing would retry it.
The previous commit kept a retryable batch whenever rate limiting was enabled,
which was too broad: a 500 with no Retry-After and backoff disabled was also
kept, so the file was re-uploaded even though nothing had scheduled a retry.
The sdk-e2e-tests "backoffConfig.enabled: false" case caught this — it expects
exactly one request and saw two.
ShouldDeleteBatch now takes the same retryAfterSeconds value handed to
HandleResponse, so the two agree on whether the response actually took the
rate-limit path. A retryable status keeps its batch only when it carries a
usable Retry-After and rate limiting is on; otherwise only backoff can retry
it, and with backoff off the batch is dropped as before. The single-argument
overload is retained.
232 tests pass.
Analytics-CSharp-plan.md states 'Spec item 1: 2xx and 3xx are success', but
IsSuccessStatusCode and the two status checks in RetryStateMachine were
2xx-only, so a 3xx fell through to the retry classifier. analytics-go,
analytics-python and analytics-php already follow the spec here; this brings C#
into line with them and with its own plan.
236 tests pass, including new cases covering 200, 201, 301 and 304.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@MichaelGHSeg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Handle Retry-After on every retryable status, including 529 - #148

Open
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after
Open

Handle Retry-After on every retryable status, including 529#148
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after

Conversation

@MichaelGHSeg

Copy link
Copy Markdown
Contributor

Stacked on #147 — review that first; this PR's diff is the single 529 commit.

Why

We were asked to add 529 support with Retry-After headers. The conclusion was to check Retry-After on every retryable status rather than special-casing 429 — 529 is simply one more member of the retryable set.

This is the same change already shipped in analytics-java 3.5.5, and matches the generic-retry-after conformance suite merged in sdk-e2e-tests#16.

What

  • Any retryable response carrying a valid Retry-After routes through the rate-limit path, which does not consume retry budget.
  • Retryable statuses withoutRetry-After continue to use counted exponential backoff.
  • Adds RetryAfterParser, handling both delta-seconds and RFC 1123 HTTP-date forms (returning null for dates in the past).

Testing

  • 229 unit tests pass on the combined branch (this commit plus Expose HttpConfig so retry behaviour is user-configurable #147), including new coverage for 503/529 with and without Retry-After, and clamping to maxRetryInterval.
  • Full e2e suite passes locally: 79 passed / 0 skipped across 11 files, including the 11-test generic-retry-after (529) suite and all four retry-settings suites.

E2E was run locally because CI cannot currently check out the private sdk-e2e-tests repo — the token: ${{ secrets.E2E_TESTS_TOKEN }} line was dropped from that checkout step during CI hardening. That's tracked separately; note the same omission is present in this repo's ci/harden-build-publish branch and in go/ruby/php.

Related

The equivalent change is open for the other SDKs: analytics-python#520, analytics-go#206, analytics-ruby#280, analytics-php#251.

Route any retryable response carrying a valid Retry-After header through the
rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable
statuses without Retry-After continue to use counted exponential backoff. Adds
529 to the retryable set and covers both paths with tests.
Matches the behaviour already shipped in analytics-java 3.5.5 and the
generic-retry-after conformance suite in sdk-e2e-tests.
Routing any retryable status with Retry-After to the rate-limit path left
ShouldDeleteBatch inconsistent with HandleResponse. With rate limiting on and
backoff off, a 503 or 529 carrying Retry-After would rate-limit the pipeline
(WaitUntilTime set, uploads blocked) while ShouldDeleteBatch still reported
true, so the batch file was deleted and the pipeline then stalled waiting to
retry events that no longer existed.
That configuration is reachable from CDN settings and directly from
Configuration.HttpConfig — it is the config ConfigurationHttpConfigTest builds.
ShouldDeleteBatch now keeps a retryable batch whenever rate limiting is
enabled, matching swift's shouldDropBatch ("Rate limit config handles retryable
codes that carry Retry-After — don't drop"). Non-retryable statuses are still
dropped, and a retryable status with neither rate limiting nor backoff enabled
is still dropped since nothing would retry it.
The previous commit kept a retryable batch whenever rate limiting was enabled,
which was too broad: a 500 with no Retry-After and backoff disabled was also
kept, so the file was re-uploaded even though nothing had scheduled a retry.
The sdk-e2e-tests "backoffConfig.enabled: false" case caught this — it expects
exactly one request and saw two.
ShouldDeleteBatch now takes the same retryAfterSeconds value handed to
HandleResponse, so the two agree on whether the response actually took the
rate-limit path. A retryable status keeps its batch only when it carries a
usable Retry-After and rate limiting is on; otherwise only backoff can retry
it, and with backoff off the batch is dropped as before. The single-argument
overload is retained.
232 tests pass.
Analytics-CSharp-plan.md states 'Spec item 1: 2xx and 3xx are success', but
IsSuccessStatusCode and the two status checks in RetryStateMachine were
2xx-only, so a 3xx fell through to the retry classifier. analytics-go,
analytics-python and analytics-php already follow the spec here; this brings C#
into line with them and with its own plan.
236 tests pass, including new cases covering 200, 201, 301 and 304.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@MichaelGHSeg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Handle Retry-After on every retryable status, including 529 - #148

Open
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after
Open

Handle Retry-After on every retryable status, including 529#148
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after

Conversation

@MichaelGHSeg

Copy link
Copy Markdown
Contributor

Stacked on #147 — review that first; this PR's diff is the single 529 commit.

Why

We were asked to add 529 support with Retry-After headers. The conclusion was to check Retry-After on every retryable status rather than special-casing 429 — 529 is simply one more member of the retryable set.

This is the same change already shipped in analytics-java 3.5.5, and matches the generic-retry-after conformance suite merged in sdk-e2e-tests#16.

What

  • Any retryable response carrying a valid Retry-After routes through the rate-limit path, which does not consume retry budget.
  • Retryable statuses withoutRetry-After continue to use counted exponential backoff.
  • Adds RetryAfterParser, handling both delta-seconds and RFC 1123 HTTP-date forms (returning null for dates in the past).

Testing

  • 229 unit tests pass on the combined branch (this commit plus Expose HttpConfig so retry behaviour is user-configurable #147), including new coverage for 503/529 with and without Retry-After, and clamping to maxRetryInterval.
  • Full e2e suite passes locally: 79 passed / 0 skipped across 11 files, including the 11-test generic-retry-after (529) suite and all four retry-settings suites.

E2E was run locally because CI cannot currently check out the private sdk-e2e-tests repo — the token: ${{ secrets.E2E_TESTS_TOKEN }} line was dropped from that checkout step during CI hardening. That's tracked separately; note the same omission is present in this repo's ci/harden-build-publish branch and in go/ruby/php.

Related

The equivalent change is open for the other SDKs: analytics-python#520, analytics-go#206, analytics-ruby#280, analytics-php#251.

Route any retryable response carrying a valid Retry-After header through the
rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable
statuses without Retry-After continue to use counted exponential backoff. Adds
529 to the retryable set and covers both paths with tests.
Matches the behaviour already shipped in analytics-java 3.5.5 and the
generic-retry-after conformance suite in sdk-e2e-tests.
Routing any retryable status with Retry-After to the rate-limit path left
ShouldDeleteBatch inconsistent with HandleResponse. With rate limiting on and
backoff off, a 503 or 529 carrying Retry-After would rate-limit the pipeline
(WaitUntilTime set, uploads blocked) while ShouldDeleteBatch still reported
true, so the batch file was deleted and the pipeline then stalled waiting to
retry events that no longer existed.
That configuration is reachable from CDN settings and directly from
Configuration.HttpConfig — it is the config ConfigurationHttpConfigTest builds.
ShouldDeleteBatch now keeps a retryable batch whenever rate limiting is
enabled, matching swift's shouldDropBatch ("Rate limit config handles retryable
codes that carry Retry-After — don't drop"). Non-retryable statuses are still
dropped, and a retryable status with neither rate limiting nor backoff enabled
is still dropped since nothing would retry it.
The previous commit kept a retryable batch whenever rate limiting was enabled,
which was too broad: a 500 with no Retry-After and backoff disabled was also
kept, so the file was re-uploaded even though nothing had scheduled a retry.
The sdk-e2e-tests "backoffConfig.enabled: false" case caught this — it expects
exactly one request and saw two.
ShouldDeleteBatch now takes the same retryAfterSeconds value handed to
HandleResponse, so the two agree on whether the response actually took the
rate-limit path. A retryable status keeps its batch only when it carries a
usable Retry-After and rate limiting is on; otherwise only backoff can retry
it, and with backoff off the batch is dropped as before. The single-argument
overload is retained.
232 tests pass.
Analytics-CSharp-plan.md states 'Spec item 1: 2xx and 3xx are success', but
IsSuccessStatusCode and the two status checks in RetryStateMachine were
2xx-only, so a 3xx fell through to the retry classifier. analytics-go,
analytics-python and analytics-php already follow the spec here; this brings C#
into line with them and with its own plan.
236 tests pass, including new cases covering 200, 201, 301 and 304.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@MichaelGHSeg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Handle Retry-After on every retryable status, including 529 - #148

Open
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after
Open

Handle Retry-After on every retryable status, including 529#148
MichaelGHSeg wants to merge 4 commits into
csharp-public-httpconfigfrom
csharp-529-retry-after

Conversation

@MichaelGHSeg

Copy link
Copy Markdown
Contributor

Stacked on #147 — review that first; this PR's diff is the single 529 commit.

Why

We were asked to add 529 support with Retry-After headers. The conclusion was to check Retry-After on every retryable status rather than special-casing 429 — 529 is simply one more member of the retryable set.

This is the same change already shipped in analytics-java 3.5.5, and matches the generic-retry-after conformance suite merged in sdk-e2e-tests#16.

What

  • Any retryable response carrying a valid Retry-After routes through the rate-limit path, which does not consume retry budget.
  • Retryable statuses withoutRetry-After continue to use counted exponential backoff.
  • Adds RetryAfterParser, handling both delta-seconds and RFC 1123 HTTP-date forms (returning null for dates in the past).

Testing

  • 229 unit tests pass on the combined branch (this commit plus Expose HttpConfig so retry behaviour is user-configurable #147), including new coverage for 503/529 with and without Retry-After, and clamping to maxRetryInterval.
  • Full e2e suite passes locally: 79 passed / 0 skipped across 11 files, including the 11-test generic-retry-after (529) suite and all four retry-settings suites.

E2E was run locally because CI cannot currently check out the private sdk-e2e-tests repo — the token: ${{ secrets.E2E_TESTS_TOKEN }} line was dropped from that checkout step during CI hardening. That's tracked separately; note the same omission is present in this repo's ci/harden-build-publish branch and in go/ruby/php.

Related

The equivalent change is open for the other SDKs: analytics-python#520, analytics-go#206, analytics-ruby#280, analytics-php#251.

Route any retryable response carrying a valid Retry-After header through the
rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable
statuses without Retry-After continue to use counted exponential backoff. Adds
529 to the retryable set and covers both paths with tests.
Matches the behaviour already shipped in analytics-java 3.5.5 and the
generic-retry-after conformance suite in sdk-e2e-tests.
Routing any retryable status with Retry-After to the rate-limit path left
ShouldDeleteBatch inconsistent with HandleResponse. With rate limiting on and
backoff off, a 503 or 529 carrying Retry-After would rate-limit the pipeline
(WaitUntilTime set, uploads blocked) while ShouldDeleteBatch still reported
true, so the batch file was deleted and the pipeline then stalled waiting to
retry events that no longer existed.
That configuration is reachable from CDN settings and directly from
Configuration.HttpConfig — it is the config ConfigurationHttpConfigTest builds.
ShouldDeleteBatch now keeps a retryable batch whenever rate limiting is
enabled, matching swift's shouldDropBatch ("Rate limit config handles retryable
codes that carry Retry-After — don't drop"). Non-retryable statuses are still
dropped, and a retryable status with neither rate limiting nor backoff enabled
is still dropped since nothing would retry it.
The previous commit kept a retryable batch whenever rate limiting was enabled,
which was too broad: a 500 with no Retry-After and backoff disabled was also
kept, so the file was re-uploaded even though nothing had scheduled a retry.
The sdk-e2e-tests "backoffConfig.enabled: false" case caught this — it expects
exactly one request and saw two.
ShouldDeleteBatch now takes the same retryAfterSeconds value handed to
HandleResponse, so the two agree on whether the response actually took the
rate-limit path. A retryable status keeps its batch only when it carries a
usable Retry-After and rate limiting is on; otherwise only backoff can retry
it, and with backoff off the batch is dropped as before. The single-argument
overload is retained.
232 tests pass.
Analytics-CSharp-plan.md states 'Spec item 1: 2xx and 3xx are success', but
IsSuccessStatusCode and the two status checks in RetryStateMachine were
2xx-only, so a 3xx fell through to the retry classifier. analytics-go,
analytics-python and analytics-php already follow the spec here; this brings C#
into line with them and with its own plan.
236 tests pass, including new cases covering 200, 201, 301 and 304.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@MichaelGHSeg