feat(hybridcloud): Bucket webhook mailboxes on the key, not on volume - #123431
Closed
vaind wants to merge 3 commits into
Closed
feat(hybridcloud): Bucket webhook mailboxes on the key, not on volume#123431vaind wants to merge 3 commits into
vaind wants to merge 3 commits into
Conversation
An integration was only split into sub-mailboxes once it exceeded 3,000 payloads in an hour, measured by a fixed-window Redis counter whose windows align to the clock hour. That counter resets on the hour, so the decision needed a second piece of state -- a `use_buckets` cache key with a one-day TTL -- to survive the reset, and a burst straddling the boundary evades it entirely: 2,900 payloads at 11:59 and 2,900 at 12:01 never exceed the limit in either window. It also fails open, routing unbucketed whenever Redis errors. GitHub already bypassed all of it with `always_bucket`, so unconditional bucketing is what the highest-volume forwarding provider has been doing all along. The bucket key now decides on its own: a payload carrying one is bucketed, one without falls back to the integration-level mailbox. That leaves the bucket count as the only knob, and it belongs to the key's cardinality rather than the provider's volume. A key that repeats across payloads -- a repository, a project -- already coalesces them onto one mailbox per distinct value, so 100 costs nothing. A key that barely repeats -- an issue, a work item -- puts one payload in a bucket and never returns to it, so a wide split buys shallow mailboxes that each still cost a scheduler row and a dispatch slot, and dispatch is the binding constraint rather than throughput. jira, jira_server and vsts take 10; github and gitlab keep 100. The five hand-rolled key readers disagreed about failure. github required an int and rejected a numeric string, gitlab returned the value uncoerced so a string id raised TypeError at the modulo, and jira_server caught ValueError but not TypeError. All five now read through `BaseRequestParser.bucket_key_at`, so a key that is missing, nested under a non-object, or not numeric falls back to the integration-level mailbox instead of raising out of the parser. Two routing changes follow. github buckets payloads whose `repository.id` arrives as a JSON string, which the isinstance check used to reject. gitlab and jira_server bucket every payload rather than only those past the gate. Refs CW-1887
The per-parser comments restated the cardinality rule that the `mailbox_bucket_count` docstring already carries, and that docstring now records that a static count is the interim answer and points at the issue replacing it. Refs CW-1887
…fixture
`test_issue_deleted_routing` posted `"repository": {"id": "1"}` as a string.
GitHub sends numeric ids -- the cell handler reads `str(event["repository"]["id"])`
and every other test in the file uses an int -- so the fixture, not the parser,
was wrong. Under the previous isinstance check the string silently produced no
bucket, which made the earlier commit look like it changed GitHub's routing. It
does not: GitHub already bucketed every payload.
Refs CW-1887
vaind
marked this pull request as draft
September 2, 2026 15:12
vaind
marked this pull request as ready for review
September 2, 2026 16:52
Contributor
Author
|
Folded into #123449, which now removes the volume gate and sizes the split from a rolling rate in one change. Splitting them here was a commit boundary, not a behavioural one: landing this alone would have fanned gitlab, jira, jira_server and vsts out to a fixed 10 or 100 buckets each, and #123449 would then have collapsed the quiet ones straight back — two re-mappings of the same keys in opposite directions, for a state nobody wanted to run. The uniform bucket-key reader is #123678, on its own, because it changes malformed-input handling rather than routing. |
vaind
deleted the
ivandlugos/bucket-webhook-mailboxes-whenever-key-exists
branch
September 4, 2026 10:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An integration was only split into sub-mailboxes once it exceeded 3,000 payloads in an hour. That gate is a fixed-window Redis counter aligned to the clock hour, which makes it the wrong shape three ways: it resets on the hour, so the decision needs a second piece of state — a
use_bucketskey with a one-day TTL — to survive the reset; a burst straddling the boundary evades it, since 2,900 payloads at 11:59 and 2,900 at 12:01 never exceed the limit in either window; and it fails open on a Redis error.GitHub already bypassed it with
always_bucket, so unconditional bucketing is what the highest-volume forwarding provider has done all along. The key now decides on its own: a payload carrying one is bucketed, one without falls back to the integration-level mailbox. The gate andalways_bucketboth go.Ordering
The divisor is the key-to-mailbox map, so a fixed divisor means a key never moves: same repository, same bucket, always. Per-key ordering — what the cell-side handlers require — is preserved. What is given up is ordering across keys within an integration, which no consumer needs, and which
githubandjira_serverhad already given up.issue.id,resource.workItemIdrepository.id,project.idCW-1987 replaces these counts with one sized from a rolling per-integration rate — but only for providers that deliver with
skip_on_failureand so tolerate reordering. jira, jira_server and vsts keep a fixed count: they deliver strictly ordered, and a divisor that moves re-maps half the keys, leaving one issue's backlog in one mailbox while its new payloads go to another.Uniform failure handling
The five hand-rolled key readers each disagreed on bad input:
githubrejected a numeric string,gitlabreturned the value uncoerced so a string id would raiseTypeErrorat the modulo, andjira_servercaughtValueErrorbut notTypeError. All five now read throughBaseRequestParser.bucket_key_at, which falls back to the integration-level mailbox instead.This is a consistency fix, not a fix for anything observed. GitHub and GitLab send
repository.idandproject.idas JSON numbers, so those divergent branches were latent. Jira Server is the one provider whoseissue.idreally does arrive as a string, and its reader already coerced it.Reviewer notes
gitlab and jira_server now bucket every payload, not only those from integrations past the gate. Their fixtures carry
project.id15 andissue.id101, so tests assertgitlab:<id>:15:pushandjira_server:<id>:1. GitHub's routing is unchanged, since it already bucketed everything.A payload carrying no key still lands on the unsplit mailbox, so it stays unordered against the keyed ones. That gap is not new, but it now applies to every integration rather than only those past the gate.
One GitHub fixture claimed
"repository": {"id": "1"}. Real payloads send a number — the cell handler doesstr(event["repository"]["id"])— so that fixture was wrong and is now1, which is what every other GitHub test already used.The low-volume, high-volume, and cache-primed test variants differed only in how they tripped the gate, so they collapse into one; the freed slots cover a GitLab
mailbox_bucket_idunit test and a VSTS payload with no work item id.314 tests pass across the base parser, every integration parser, and the delivery tasks.
Refs CW-1887