Move credit expiration to scheduled cron - #6192
chrarnoldus wants to merge 1 commit into
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| const userLimit = pLimit(USER_CONCURRENCY); | ||
| const userResults = await Promise.allSettled( | ||
| dueUsers.map(user => userLimit(() => processLocalExpirations(user, now))) |
There was a problem hiding this comment.
CRITICAL: processLocalExpirations requires updated_at, but the query above does not select it
UserForLocalExpiration (apps/web/src/lib/creditExpiration.ts:29-36) is Pick<User, 'id' | 'microdollars_used' | 'next_credit_expiration_at' | 'updated_at' | 'total_microdollars_acquired'>, and kilocode_users.updated_at is notNull() (packages/db/src/schema.ts:415-418). The dueUsers select only projects id, microdollars_used, next_credit_expiration_at, and total_microdollars_acquired, so this argument is missing the required updated_at property and will fail TypeScript assignability, breaking pnpm typecheck in CI. Add updated_at: kilocode_users.updated_at to the select, or drop updated_at from UserForLocalExpiration (it is not read inside the function body).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| state.next_credit_expiration_at !== organization.next_credit_expiration_at | ||
| ); | ||
| }).length; | ||
| } catch { |
There was a problem hiding this comment.
WARNING: Organization failures are swallowed without logging, and every due org is reported as failed
The bare catch discards the error and marks all due organizations as failed. processOrganizationExpirationsBatch commits its set-based updates in a transaction and retries on conflicts, so a throw after a partial commit (or a transient DB error) can mean most orgs were actually processed, yet the summary reports all of them failed and the route returns 500. Conversely, orgs that lost the optimistic race return normally and are counted as neither processed nor failed. Capture the error and log it (e.g. sentryLogger) and record only the ids that genuinely failed. The user path has the same observability gap: Promise.allSettled above keeps only ids and drops every rejection reason, so a systematic per-user failure is invisible apart from a count.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| }) | ||
| .from(kilocode_users) | ||
| .where(lte(kilocode_users.next_credit_expiration_at, expirationBoundary)) | ||
| .orderBy(asc(kilocode_users.next_credit_expiration_at), asc(kilocode_users.id)) |
There was a problem hiding this comment.
SUGGESTION: No dead-lettering for persistently failing rows
The batch always selects the batchSize oldest due rows ordered by next_credit_expiration_at, and any row that failed processing is re-selected on the next run. A user or organization whose processing throws deterministically stays at the head of the queue and consumes a batch slot every tick; once batchSize such rows accumulate for a table, that table stops making progress entirely. Consider tracking attempt counts or skipping recently-failed ids so one poison row cannot starve the rest of the queue.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return NextResponse.json({ error: 'User not found: ' + userId }, { status: 404 }); | ||
| } | ||
| await getBalanceForUser(user, { forceRefresh: true }); | ||
| await getBalanceForUser(user); |
There was a problem hiding this comment.
WARNING: Admin "Force expiration check" silently becomes a no-op
{ forceRefresh: true } was what previously made this endpoint run expiration for the user. getBalanceForUser(user) now only computes (total_microdollars_acquired - microdollars_used) / 1_000_000 in memory and schedules after(() => maybePerformAutoTopUp(user)); it writes nothing and recomputes no expirations. The route still returns { success: true }, so the admin UI button "Force expiration check" (apps/web/src/app/admin/components/UserAdmin/UserAdminUsageBilling.tsx:233-243) now reports success while doing nothing. Either remove/rename the endpoint and button, or have it invoke expiration for that user directly.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| throw new Error('CRON_SECRET is not configured in environment variables'); | ||
| } | ||
|
|
||
| export const maxDuration = 300; |
There was a problem hiding this comment.
SUGGESTION: maxDuration = 300 equals the 5-minute cron interval
The route allows a 5-minute execution while apps/web/vercel.json schedules it every 5 minutes, so a slow run can still be in flight when the next invocation starts, re-selecting the same due rows and doubling database load. The sibling 5-minute cron api/cron/sync-providers deliberately caps at maxDuration = 240 with a comment tying the value to the schedule. Consider a timeout below the interval or an explicit overlap guard.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
The highest-risk issue is in Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (10 files)
Notes
Fix these issues in Kilo Cloud Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
1 participant
Summary
Verification
git diff --check