Skip to content

Commit 21ec9db

Browse files
alex-alecualex-alecu
authored
fix(code-review): ignore stale running slots (#2824)
1 parent 2fba9db commit 21ec9db

2 files changed

Lines changed: 164 additions & 8 deletions

File tree

‎apps/web/src/lib/code-reviews/dispatch/dispatch-pending-reviews.test.ts‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
const mockDispatchReview = jest.fn();
2+
3+
jest.mock('@/lib/code-reviews/client/code-review-worker-client', () => ({
4+
codeReviewWorkerClient: {
5+
dispatchReview: (...args: unknown[]) => mockDispatchReview(...args),
6+
},
7+
}));
8+
9+
jest.mock('@sentry/nextjs', () => ({
10+
captureException: jest.fn(),
11+
}));
12+
13+
import { db } from '@/lib/drizzle';
14+
import { insertTestUser } from '@/tests/helpers/user.helper';
15+
import { cloud_agent_code_reviews, kilocode_users, type User } from '@kilocode/db/schema';
16+
import { eq } from 'drizzle-orm';
17+
import { tryDispatchPendingReviews } from './dispatch-pending-reviews';
18+
19+
const REPO = `test-org/dispatch-pending-${Date.now()}`;
20+
21+
function minutesAgo(minutes: number) {
22+
return new Date(Date.now() - minutes * 60 * 1000).toISOString();
23+
}
24+
25+
describe('tryDispatchPendingReviews', () => {
26+
let testUser: User;
27+
let reviewSequence = 0;
28+
29+
beforeAll(async () => {
30+
testUser = await insertTestUser();
31+
});
32+
33+
afterEach(async () => {
34+
await db
35+
.delete(cloud_agent_code_reviews)
36+
.where(eq(cloud_agent_code_reviews.owned_by_user_id, testUser.id));
37+
mockDispatchReview.mockClear();
38+
});
39+
40+
afterAll(async () => {
41+
await db.delete(kilocode_users).where(eq(kilocode_users.id, testUser.id));
42+
});
43+
44+
function reviewValues({
45+
status,
46+
createdAt,
47+
updatedAt,
48+
startedAt = null,
49+
}: {
50+
status: 'queued' | 'running';
51+
createdAt: string;
52+
updatedAt: string;
53+
startedAt?: string | null;
54+
}) {
55+
const sequence = reviewSequence++;
56+
57+
return {
58+
owned_by_user_id: testUser.id,
59+
repo_full_name: REPO,
60+
pr_number: sequence + 1,
61+
pr_url: `https://github.com/${REPO}/pull/${sequence + 1}`,
62+
pr_title: `Test PR ${sequence + 1}`,
63+
pr_author: 'octocat',
64+
base_ref: 'main',
65+
head_ref: `feature/test-${sequence}`,
66+
head_sha: `sha-${sequence}`,
67+
status,
68+
started_at: startedAt,
69+
created_at: createdAt,
70+
updated_at: updatedAt,
71+
};
72+
}
73+
74+
it('does not count stale running reviews against owner capacity', async () => {
75+
const recentTimestamp = minutesAgo(1);
76+
const staleRunningTimestamp = minutesAgo(91);
77+
78+
await db.insert(cloud_agent_code_reviews).values([
79+
reviewValues({
80+
status: 'running',
81+
createdAt: recentTimestamp,
82+
updatedAt: recentTimestamp,
83+
startedAt: recentTimestamp,
84+
}),
85+
...Array.from({ length: 19 }, () =>
86+
reviewValues({
87+
status: 'queued',
88+
createdAt: recentTimestamp,
89+
updatedAt: recentTimestamp,
90+
})
91+
),
92+
reviewValues({
93+
status: 'running',
94+
createdAt: staleRunningTimestamp,
95+
updatedAt: staleRunningTimestamp,
96+
startedAt: staleRunningTimestamp,
97+
}),
98+
]);
99+
100+
const result = await tryDispatchPendingReviews({
101+
type: 'user',
102+
id: testUser.id,
103+
userId: testUser.id,
104+
});
105+
106+
expect(result).toEqual({
107+
dispatched: 0,
108+
pending: 0,
109+
activeCount: 20,
110+
});
111+
expect(mockDispatchReview).not.toHaveBeenCalled();
112+
});
113+
114+
it('does not count stale queued reviews against owner capacity', async () => {
115+
const recentTimestamp = minutesAgo(1);
116+
const staleQueuedTimestamp = minutesAgo(6);
117+
118+
await db.insert(cloud_agent_code_reviews).values([
119+
...Array.from({ length: 20 }, () =>
120+
reviewValues({
121+
status: 'running',
122+
createdAt: recentTimestamp,
123+
updatedAt: recentTimestamp,
124+
startedAt: recentTimestamp,
125+
})
126+
),
127+
reviewValues({
128+
status: 'queued',
129+
createdAt: staleQueuedTimestamp,
130+
updatedAt: staleQueuedTimestamp,
131+
}),
132+
]);
133+
134+
const result = await tryDispatchPendingReviews({
135+
type: 'user',
136+
id: testUser.id,
137+
userId: testUser.id,
138+
});
139+
140+
expect(result).toEqual({
141+
dispatched: 0,
142+
pending: 0,
143+
activeCount: 20,
144+
});
145+
expect(mockDispatchReview).not.toHaveBeenCalled();
146+
});
147+
});

‎apps/web/src/lib/code-reviews/dispatch/dispatch-pending-reviews.ts‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ const MAX_CONCURRENT_REVIEWS_PER_OWNER = 20;
2727
// window are considered abandoned (e.g. process crashed after claim) and
2828
// become eligible for re-dispatch.
2929
const STALE_CLAIM_MINUTES = 5;
30+
const STALE_RUNNING_MINUTES = 90;
3031

3132
export interface DispatchResult {
3233
dispatched: number;
@@ -42,10 +43,11 @@ export async function tryDispatchPendingReviews(owner: Owner): Promise<DispatchR
4243
try {
4344
logExceptInTest(`[tryDispatchPendingReviews] Starting dispatch check`, { owner });
4445

45-
const staleCutoff = sql`now() - interval '${sql.raw(String(STALE_CLAIM_MINUTES))} minutes'`;
46+
const staleQueuedCutoff = sql`now() - interval '${sql.raw(String(STALE_CLAIM_MINUTES))} minutes'`;
47+
const staleRunningCutoff = sql`now() - interval '${sql.raw(String(STALE_RUNNING_MINUTES))} minutes'`;
4648

4749
// 1. Get active review count for this owner.
48-
// Stale queued rows are excluded so abandoned claims do not block recovery.
50+
// Stale queued and running rows are excluded so abandoned work does not block recovery.
4951
const activeCountResult = await db
5052
.select({ count: count() })
5153
.from(cloud_agent_code_reviews)
@@ -55,10 +57,17 @@ export async function tryDispatchPendingReviews(owner: Owner): Promise<DispatchR
5557
? eq(cloud_agent_code_reviews.owned_by_organization_id, owner.id)
5658
: eq(cloud_agent_code_reviews.owned_by_user_id, owner.id),
5759
or(
58-
eq(cloud_agent_code_reviews.status, 'running'),
60+
and(
61+
eq(cloud_agent_code_reviews.status, 'running'),
62+
sql`COALESCE(
63+
${cloud_agent_code_reviews.started_at},
64+
${cloud_agent_code_reviews.updated_at},
65+
${cloud_agent_code_reviews.created_at}
66+
) >= ${staleRunningCutoff}`
67+
),
5968
and(
6069
eq(cloud_agent_code_reviews.status, 'queued'),
61-
gte(cloud_agent_code_reviews.updated_at, staleCutoff)
70+
gte(cloud_agent_code_reviews.updated_at, staleQueuedCutoff)
6271
)
6372
)
6473
)
@@ -94,7 +103,7 @@ export async function tryDispatchPendingReviews(owner: Owner): Promise<DispatchR
94103
eq(cloud_agent_code_reviews.status, 'pending'),
95104
and(
96105
eq(cloud_agent_code_reviews.status, 'queued'),
97-
lt(cloud_agent_code_reviews.updated_at, staleCutoff)
106+
lt(cloud_agent_code_reviews.updated_at, staleQueuedCutoff)
98107
)
99108
)
100109
)
@@ -115,7 +124,7 @@ export async function tryDispatchPendingReviews(owner: Owner): Promise<DispatchR
115124

116125
// 5. Dispatch all pending reviews in parallel
117126
const results = await Promise.allSettled(
118-
pendingReviews.map(review => dispatchReview(review, owner, staleCutoff))
127+
pendingReviews.map(review => dispatchReview(review, owner, staleQueuedCutoff))
119128
);
120129

121130
let dispatched = 0;
@@ -180,7 +189,7 @@ export async function tryDispatchPendingReviews(owner: Owner): Promise<DispatchR
180189
async function dispatchReview(
181190
review: CloudAgentCodeReview,
182191
owner: Owner,
183-
staleCutoff: ReturnType<typeof sql>
192+
staleQueuedCutoff: ReturnType<typeof sql>
184193
): Promise<boolean> {
185194
// Get platform from review (defaults to 'github' for backward compatibility)
186195
const platform = (review.platform || 'github') as CodeReviewPlatform;
@@ -222,7 +231,7 @@ async function dispatchReview(
222231
eq(cloud_agent_code_reviews.status, 'pending'),
223232
and(
224233
eq(cloud_agent_code_reviews.status, 'queued'),
225-
lt(cloud_agent_code_reviews.updated_at, staleCutoff)
234+
lt(cloud_agent_code_reviews.updated_at, staleQueuedCutoff)
226235
)
227236
)
228237
)

0 commit comments

Comments
 (0)