Skip to content

Reduce Smoke CI REST churn by caching mention-resolution collaborator lookups #36960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 7 commits into from
Jun 4, 2026
+87 −16
Merged

Reduce Smoke CI REST churn by caching mention-resolution collaborator lookups #36960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a18947f
Initial plan
Copilot Jun 4, 2026
69f09d6
chore: start smoke CI API footprint investigation
Copilot Jun 4, 2026
477d527
perf: cache mention-collaborator lookups in safe outputs
Copilot Jun 4, 2026
eead3c6
test: avoid repeated collaborator API calls in mention resolution
Copilot Jun 4, 2026
66e28b2
fix: resolve TypeScript typecheck error in resolve_mentions.cjs
Copilot Jun 4, 2026
b1dcbf2
refactor: improve TypeScript type handling in checkUserPermission
Copilot Jun 4, 2026
291442f
refactor: simplify type handling with documented @ts-expect-error
Copilot Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions .github/workflows/dead-code-remover.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

56 changes: 55 additions & 1 deletion actions/setup/js/resolve_mentions.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,20 @@
/// <reference types="@actions/github-script" />

const { getErrorMessage } = require("./error_helpers.cjs");
/** @type {Map<string, Map<string, boolean>>} */
const recentCollaboratorsCache = new Map();
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 4, 2026

Choose a reason for hiding this comment

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

[/improve-codebase-architecture] Two module-level Map instances give the caches process lifetime, which is correct for a single-run Action but couples the module to a global singleton. The only way to reset them is resetMentionResolutionCaches, which is a test-only hook that leaks through the public module.exports.

💡 Alternatives that avoid exporting test infrastructure

Option A — factory function (minimal change, caches scoped to the resolver instance):

function createMentionResolver() {
  const collaboratorsCache = new Map();
  const permissionCache = new Map();
  return {
    resolveMentionsLazily: (text, knownAuthors, owner, repo, github, core) =>
      _resolveMentionsLazily(text, knownAuthors, owner, repo, github, core,
                             collaboratorsCache, permissionCache),
  };
}
// Tests just call createMentionResolver() per test — no reset hook needed.

Option B — injected cache bag (most explicit):
Pass { collaboratorsCache, permissionCache } as a parameter with a sensible default (module-scoped Maps for production, fresh Maps in tests). This avoids exporting resetMentionResolutionCaches entirely.

The current approach works, but the reset-hook-in-exports pattern is a signal worth addressing before it multiplies.

/** @type {Map<string, Map<string, boolean>>} */
const userPermissionCache = new Map();

/**
* Build a stable cache key for repository-scoped lookups.
* @param {string} owner
* @param {string} repo
* @returns {string}
*/
function getRepoCacheKey(owner, repo) {
return `${owner}/${repo}`.toLowerCase();
}

/**
* @typedef {Object} MentionResolutionResult
Expand Down Expand Up @@ -58,6 +72,11 @@ function isPayloadUserBot(user) {
* @returns {Promise<Map<string, boolean>>} Map of username (lowercase) to whether they're allowed (any collaborator, not bot)
*/
async function getRecentCollaborators(owner, repo, github, core) {
const repoCacheKey = getRepoCacheKey(owner, repo);
const cachedCollaborators = recentCollaboratorsCache.get(repoCacheKey);
if (cachedCollaborators) {
return cachedCollaborators;
}
try {
// Fetch only first page (30 collaborators) for optimistic resolution
const collaborators = await github.rest.repos.listCollaborators({
Expand All @@ -75,6 +94,7 @@ async function getRecentCollaborators(owner, repo, github, core) {
allowedMap.set(lowercaseLogin, isAllowed);
}

recentCollaboratorsCache.set(repoCacheKey, allowedMap);
return allowedMap;
} catch (error) {
core.warning(`Failed to fetch recent collaborators: ${getErrorMessage(error)}`);
Expand All @@ -92,13 +112,26 @@ async function getRecentCollaborators(owner, repo, github, core) {
* @returns {Promise<boolean>} True if user is allowed (any collaborator, not bot)
*/
async function checkUserPermission(username, owner, repo, github, core) {
const repoCacheKey = getRepoCacheKey(owner, repo);
const usernameKey = username.toLowerCase();
const cachedPermissions = userPermissionCache.get(repoCacheKey);
if (cachedPermissions && cachedPermissions.has(usernameKey)) {
// TypeScript doesn't recognize Map.has() as a type guard for Map.get()
// @ts-expect-error - .has() guarantees .get() returns boolean, not undefined
return cachedPermissions.get(usernameKey);
}

/** @type {Map<string, boolean>} */
const permissionsForRepo = cachedPermissions || new Map();
try {
// First check if user exists and is not a bot
const { data: user } = await github.rest.users.getByUsername({
username: username,
});

if (user.type === "Bot") {
permissionsForRepo.set(usernameKey, false);
userPermissionCache.set(repoCacheKey, permissionsForRepo);
return false;
}

Expand All @@ -110,9 +143,14 @@ async function checkUserPermission(username, owner, repo, github, core) {
});

// Allow any permission level (read, triage, write, maintain, admin)
return permissionData.permission !== "none";
const isAllowed = permissionData.permission !== "none";
permissionsForRepo.set(usernameKey, isAllowed);
userPermissionCache.set(repoCacheKey, permissionsForRepo);
return isAllowed;
} catch (error) {
// User doesn't exist, not a collaborator, or API error - deny
permissionsForRepo.set(usernameKey, false);
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 4, 2026

Choose a reason for hiding this comment

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

[/diagnose] Any transient API failure (rate limit, 503, network blip) in this catch block is cached as a permanent false for the rest of the process — the same as a deterministic "user not found" result. A retry that would succeed is silently suppressed.

💡 Suggested fix: only cache deterministic failures 
} catch (error) {
  // Only cache 404 (user doesn’t exist) — deterministic, safe to memoize
  const status = error?.status ?? error?.response?.status;
  if (status === 404) {
    permissionsForRepo.set(usernameKey, false);
    userPermissionCache.set(repoCacheKey, permissionsForRepo);
  }
  // Transient errors (5xx, network): fall through without caching so a later
  // call within the same run gets a fresh attempt.
  return false;
}

In a short-lived Action process the blast radius is limited to one run, but this could silently deny a valid contributor if the first lookup races against a transient outage.

userPermissionCache.set(repoCacheKey, permissionsForRepo);
return false;
}
}
Expand Down Expand Up @@ -142,6 +180,16 @@ async function resolveMentionsLazily(text, knownAuthors, owner, repo, github, co
core.warning(`Mention limit exceeded: ${totalMentions} mentions found, processing only first 50`);
}

if (mentionsToProcess.length === 0) {
core.info("No mentions found in text");
return {
allowedMentions: [],
totalMentions,
resolvedCount: 0,
limitExceeded,
};
}

// Build set of known allowed authors (case-insensitive)
const knownAuthorsLowercase = new Set(knownAuthors.filter(a => a).map(a => a.toLowerCase()));

Expand Down Expand Up @@ -189,10 +237,16 @@ async function resolveMentionsLazily(text, knownAuthors, owner, repo, github, co
};
}

function resetMentionResolutionCaches() {
recentCollaboratorsCache.clear();
userPermissionCache.clear();
}

module.exports = {
extractMentions,
isPayloadUserBot,
getRecentCollaborators,
checkUserPermission,
resolveMentionsLazily,
resetMentionResolutionCaches,
};
18 changes: 17 additions & 1 deletion actions/setup/js/resolve_mentions.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@ import { describe, it, expect, beforeEach, vi } from "vitest";
const mockCore = { info: vi.fn(), warning: vi.fn() },
mockGithub = { rest: { repos: { listCollaborators: vi.fn(), getCollaboratorPermissionLevel: vi.fn() }, users: { getByUsername: vi.fn() } } };
((global.core = mockCore), (global.github = mockGithub));
const { extractMentions, isPayloadUserBot, getRecentCollaborators, checkUserPermission, resolveMentionsLazily } = require("./resolve_mentions.cjs");
const { extractMentions, isPayloadUserBot, getRecentCollaborators, checkUserPermission, resolveMentionsLazily, resetMentionResolutionCaches } = require("./resolve_mentions.cjs");
describe("resolve_mentions.cjs", () => {
(beforeEach(() => {
vi.clearAllMocks();
resetMentionResolutionCaches();
}),
describe("extractMentions", () => {
(it("should extract single mention", () => {
Expand Down Expand Up @@ -157,6 +158,21 @@ describe("resolve_mentions.cjs", () => {
(await resolveMentionsLazily("Hello @author1 @maintainer1", ["author1"], "owner", "repo", mockGithub, mockCore),
expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Found 2 unique mentions")),
expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Total allowed mentions")));
}),
it("should skip collaborator lookups when text has no mentions", async () => {
const result = await resolveMentionsLazily("Hello world", [], "owner", "repo", mockGithub, mockCore);
expect(result).toMatchObject({ allowedMentions: [], totalMentions: 0, resolvedCount: 0, limitExceeded: false });
expect(mockGithub.rest.repos.listCollaborators).not.toHaveBeenCalled();
expect(mockGithub.rest.users.getByUsername).not.toHaveBeenCalled();
expect(mockGithub.rest.repos.getCollaboratorPermissionLevel).not.toHaveBeenCalled();
}),
it("should reuse cached collaborator list across repeated resolution calls", async () => {
mockGithub.rest.repos.listCollaborators.mockResolvedValue({ data: [{ login: "maintainer1", type: "User", permissions: { maintain: true, admin: false, push: false } }] });

await resolveMentionsLazily("Hello @maintainer1", [], "owner", "repo", mockGithub, mockCore);
await resolveMentionsLazily("Hello @maintainer1", [], "owner", "repo", mockGithub, mockCore);

expect(mockGithub.rest.repos.listCollaborators).toHaveBeenCalledTimes(1);
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 4, 2026

Choose a reason for hiding this comment

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

[/tdd] The two new tests cover getRecentCollaborators cache-hit and the no-mention early exit, but userPermissionCache — the more complex code path — has no cache-hit test. This path is reached when a mentioned user is absent from the collaborator list and falls through to checkUserPermission, which also caches bot results, permission levels, and errors.

💡 Suggested test skeleton 
it("should reuse cached permission when user is not in collaborator list", async () => {
  // User not in collaborators → falls through to checkUserPermission
  mockGithub.rest.repos.listCollaborators.mockResolvedValue({ data: [] });
  mockGithub.rest.users.getByUsername.mockResolvedValue({ data: { login: "external1", type: "User" } });
  mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue(
    { data: { permission: "write" } }
  );

  await resolveMentionsLazily("Hello `@external1`", [], "owner", "repo", mockGithub, mockCore);
  await resolveMentionsLazily("Hello `@external1`", [], "owner", "repo", mockGithub, mockCore);

  // Second resolution should hit userPermissionCache, not call the API again
  expect(mockGithub.rest.repos.getCollaboratorPermissionLevel).toHaveBeenCalledTimes(1);
});

Without this, the error-path memoisation (catch → false) and bot-user caching are also untested.

}));
}));
});