Skip to content

Reduce bugs.webkit.org API calls#271

Open
gsnedders wants to merge 3 commits into
web-platform-tests:mainfrom
gsnedders:reduce-webkit-api-calls
Open

Reduce bugs.webkit.org API calls#271
gsnedders wants to merge 3 commits into
web-platform-tests:mainfrom
gsnedders:reduce-webkit-api-calls

Conversation

@gsnedders

Copy link
Copy Markdown
Member

Instead of polling each and every WebKit bug for every open GitHub PR, this instead uses a single query to fetch all bugs which have ever been marked as fixed. This has a significant impact on the number of requests made to the Bugzilla API, and should make it practical to enforce a rate limit which we previously did not have.

As a precursor to this, we also drop support for r+ and commit-queue+, given these are largely unused nowadays.

When almost all WebKit contributions are in GitHub PRs, looking at
these flags on Bugzilla is near useless.
Instead of fetching bug details individually for each WebKit export
PR, group them by bug ID and query for all bugs that have ever been
marked FIXED in a single API call. This significantly reduces the
number of API calls in the common case.
Serialize all bugs.webkit.org requests through a shared throttle so
outgoing calls are spaced at least 1 second apart, regardless of
caller.  This protects bugs.webkit.org from accidental bursts
(e.g. from the WebKit poller fanning out per-bug history fetches).

@DanielRyanSmith DanielRyanSmith left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some outdated text in lib/comment.js that needs to be updated:

const message = 'This patch has been exported from WebKit; it will be approved ' +
'automatically once the downstream patch is r+.';

Comment thread index.js
bugToPRs[bugId].push(pull_request);
});

const everFixedBugs = await webkit.everFixed(Object.keys(bugToPRs).map(Number));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gemini found a possible regression:

Webhook Failure Recovery (Potential Regression)

  • How it worked before: If the webhook for a newly opened WebKit PR failed, the periodic pullRequestPoller would eventually pick it up, fetch its metadata, label it webkit-export, and post the initial explanation comment.
  • How it works now: The new pullRequestPoller only processes PRs whose associated WebKit bugs are already FIXED (via webkit.everFixed). If a webhook fails for an unfixed WebKit PR, the bot will never label it or leave the initial explanation comment until the bug actually lands in WebKit (i.e. becomes FIXED).
  • Recommendation: Optimize the filter in pullRequestPoller to also process PRs that do not yet have the webkit-export label. The label status can be read directly from the GitHub PR payload (available in pull_requests), which does not require additional Bugzilla API calls.

Comment thread index.js
.filter(bugId => everFixedBugs.has(Number(bugId)))
.flatMap(bugId => bugToPRs[bugId].map(pull_request => ({pull_request, webkitBug: everFixedBugs.get(Number(bugId))})));

everFixedPRs.forEach(async function({pull_request, webkitBug}) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Sam,

Using forEach with an async callback means these promises aren't awaited by pullRequestPoller

pullRequestPoller() will exit its try block immediately and schedule the next 15-minute waitFor() timer while PRs are still running in the background. If get_metadata throws (e.g. on a network timeout or API error), the rejection won't be caught by the outer try/catch and will emit an unhandled promise rejection.

Swapping this to a standard for (const item of everFixedPRs) loop should fix both the unawaited scheduling and error catching.

Comment thread lib/metadata/webkit.js
return new Map();
}
const params = new URLSearchParams({
id: bugIds.join(","),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have a large number of open WebKit export PRs (e.g., 100+), joining all bug IDs into a single id query parameter:

const params = new URLSearchParams({
    id: bugIds.join(","),
    ...
});

will push the GET request URI past 4KB–8KB, which will get rejected by Cloudflare/Apache with 414 Request-URI Too Large or 400 Bad Request.

We should probably chunk bugIds in batches (e.g., max 50 IDs per request) inside everFixed to keep the URI length well within safe limits:

const CHUNK_SIZE = 50;
const result = new Map();
for (let i = 0; i < bugIds.length; i += CHUNK_SIZE) {
    const chunk = bugIds.slice(i, i + CHUNK_SIZE);
    const params = new URLSearchParams({ id: chunk.join(",") /* ... */ });
    const body = await bugsWebkit.get("/rest/bug?" + params);
    if (body && Array.isArray(body.bugs)) {
        for (const b of body.bugs) result.set(b.id, b);
    }
}
return result;

Comment thread index.js
});

const everFixedBugs = await webkit.everFixed(Object.keys(bugToPRs).map(Number));
const everFixedPRs = Object.keys(bugToPRs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI / Heads-up: Since the poller now only queries everFixedBugs, if an export PR is opened before see_also is added on Bugzilla (or if the initial opened webhook fails for any reason), the poller won't pick it up to add the webkit-export label or post the initial comment until the patch actually lands on WebKit.

I think that's fine given the API savings, but just wanted to make sure that trade-off was intentional!

Sign up for free to 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.

3 participants