Skip to content

Use listing-scoped concurrency group for PrCard creation on shared submission realm #4251

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

Open
richardhjtan wants to merge 1 commit into
base: main
Choose a base branch
from
+48 −7
Open

Use listing-scoped concurrency group for PrCard creation on shared submission realm #4251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions packages/bot-runner/lib/command-runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,13 @@ export class CommandRunner {
realmURL,
command,
commandInput,
concurrencyGroup,
}: {
runAs: string;
realmURL: string;
command: string;
commandInput: Record<string, any> | null;
concurrencyGroup?: string;
}): Promise<RunCommandResponse> {
let job = await enqueueRunCommandJob(
{
Expand All @@ -151,6 +153,7 @@ export class CommandRunner {
this.queuePublisher,
this.dbAdapter,
userInitiatedPriority,
concurrencyGroup ? { concurrencyGroup } : undefined,
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

enqueueRunCommand drops the override when concurrencyGroup is an empty string because it uses a truthiness check (concurrencyGroup ? ...). Since the parameter type is string | undefined, an empty string is a valid value and should be forwarded (or rejected via validation). Consider switching to an explicit concurrencyGroup !== undefined check (and, if supported, forwarding null as well) so the function doesn’t silently fall back to the default group.

Suggested change
concurrencyGroup ? { concurrencyGroup } : undefined,
concurrencyGroup !== undefined ? { concurrencyGroup } : undefined,

Copilot uses AI. Check for mistakes.
);
return await job.done;
}
Expand Down Expand Up @@ -191,10 +194,12 @@ export class CommandRunner {
prResult: CreatedListingPRResult;
}): Promise<void> {
let submissionRealm = new URL('/submissions/', realmURL).href;
let listingConcurrencyGroup = `command:${submissionRealm}:listing:${prResult.branchName}`;
let prCardResult = await this.enqueueRunCommand({
runAs: this.submissionBotUserId,
realmURL: submissionRealm,
command: CREATE_PR_CARD_COMMAND,
concurrencyGroup: listingConcurrencyGroup,
commandInput: {
realm: submissionRealm,
prNumber: prResult.prNumber,
Expand Down
47 changes: 41 additions & 6 deletions packages/bot-runner/tests/command-runner-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ module('command runner', () => {
destroy: async () => {},
};
let githubClient: GitHubClient = {
openPullRequest: async () => ({ number: 1, html_url: 'https://example/pr/1' }),
openPullRequest: async () => ({
number: 1,
html_url: 'https://example/pr/1',
}),
createBranch: async () => ({ ref: 'refs/heads/test', sha: 'abc123' }),
writeFileToBranch: async () => ({ commitSha: 'def456' }),
writeFilesToBranch: async () => ({ commitSha: 'def456' }),
Expand Down Expand Up @@ -84,7 +87,11 @@ module('command runner', () => {
'bot-registration-1',
);

assert.strictEqual(publishedJobs.length, 1, 'published one run-command job');
assert.strictEqual(
publishedJobs.length,
1,
'published one run-command job',
);
assert.deepEqual(
publishedJobs[0],
{
Expand Down Expand Up @@ -220,8 +227,8 @@ module('command runner', () => {
input: {
roomId: '!abc123:localhost',
listingName: 'My Listing Name',
listingSummary: 'My listing Summary'
},
listingSummary: 'My listing Summary',
},
},
'bot-registration-2',
);
Expand All @@ -234,6 +241,27 @@ module('command runner', () => {
assert.strictEqual(createdBranches.length, 1, 'creates branch');
assert.strictEqual(branchWrites.length, 1, 'writes files to branch');
assert.strictEqual(openedPRs.length, 1, 'opens pull request');

// Job 1 (create-submission) targets the user's realm — default concurrency group
assert.strictEqual(
(publishedJobs[0] as { concurrencyGroup: string }).concurrencyGroup,
'command:http://localhost:4201/test/',
'Job 1 (create-submission) uses default realm concurrency group',
);
// Job 2 (create-pr-card) targets the shared submission realm — listing-scoped
// concurrency group so different listings can run in parallel
assert.strictEqual(
(publishedJobs[1] as { concurrencyGroup: string }).concurrencyGroup,
`command:${SUBMISSION_REALM_URL}:listing:room-IWFiYzEyMzpsb2NhbGhvc3Q/my-listing-name`,
'Job 2 (create-pr-card) uses listing-scoped concurrency group',
);
// Job 3 (patch-card-instance) targets the user's realm — default concurrency group
assert.strictEqual(
(publishedJobs[2] as { concurrencyGroup: string }).concurrencyGroup,
'command:http://localhost:4201/test/',
'Job 3 (patch-card-instance) uses default realm concurrency group',
);

assert.deepEqual(
(publishedJobs[1] as { args: Record<string, unknown> }).args,
{
Expand Down Expand Up @@ -275,7 +303,10 @@ module('command runner', () => {
},
'enqueues submission card patch in the user realm',
);
let prBody = (openedPRs[0] as { params: Record<string, unknown> }).params.body?.toString() ?? '';
let prBody =
(
openedPRs[0] as { params: Record<string, unknown> }
).params.body?.toString() ?? '';
assert.true(
prBody.includes(`[${submissionCardUrl}](${submissionCardUrl})`),
'PR body includes submission card URL as markdown link',
Expand Down Expand Up @@ -477,7 +508,11 @@ module('command runner', () => {

assert.strictEqual(publishedJobs.length, 1, 'enqueues run-command job');
assert.strictEqual(createdBranches.length, 0, 'does not create branch');
assert.strictEqual(branchWrites.length, 0, 'does not write files to branch');
assert.strictEqual(
branchWrites.length,
0,
'does not write files to branch',
);
assert.strictEqual(openedPRs.length, 0, 'does not open pull request');
});
});
3 changes: 2 additions & 1 deletion packages/runtime-common/jobs/run-command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@ export async function enqueueRunCommandJob(
queue: QueuePublisher,
_dbAdapter: DBAdapter,
priority: number,
opts?: { concurrencyGroup?: string },
) {
let job = await queue.publish<RunCommandResponse>({
jobType: 'run-command',
concurrencyGroup: `command:${args.realmURL}`,
concurrencyGroup: opts?.concurrencyGroup ?? `command:${args.realmURL}`,
timeout: RUN_COMMAND_JOB_TIMEOUT_SEC,
priority,
args,
Expand Down
Loading