-
-
Notifications
You must be signed in to change notification settings - Fork 71
feat(cron): add build status reconciliation cron job #1641
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.
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
Changes from 1 commit
49d12b1
d6a80a7
cdcb5f4
2a80a67
3446ae3
7048e8c
be09942
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,150 @@ | ||||||
| import type { MiddlewareKeyVariables } from '../utils/hono.ts' | ||||||
| import { Hono } from 'hono/tiny' | ||||||
| import { BRES, middlewareAPISecret } from '../utils/hono.ts' | ||||||
| import { cloudlog, cloudlogErr } from '../utils/logging.ts' | ||||||
| import { recordBuildTime, supabaseAdmin } from '../utils/supabase.ts' | ||||||
| import { getEnv } from '../utils/utils.ts' | ||||||
|
|
||||||
| interface BuilderStatusResponse { | ||||||
| job: { | ||||||
| status: string | ||||||
| started_at: number | null | ||||||
| completed_at: number | null | ||||||
| error: string | null | ||||||
| } | ||||||
| machine: Record<string, unknown> | null | ||||||
| uploadUrl?: string | ||||||
| } | ||||||
|
|
||||||
| const TERMINAL_STATUSES = new Set(['succeeded', 'failed', 'expired', 'released', 'cancelled']) | ||||||
| const STALE_THRESHOLD_MINUTES = 5 | ||||||
| const ORPHAN_THRESHOLD_HOURS = 1 | ||||||
| const BATCH_LIMIT = 50 | ||||||
|
|
||||||
| export const app = new Hono<MiddlewareKeyVariables>() | ||||||
|
|
||||||
| app.post('/', middlewareAPISecret, async (c) => { | ||||||
| const startTime = Date.now() | ||||||
| let reconciled = 0 | ||||||
| let orphaned = 0 | ||||||
| let errors = 0 | ||||||
|
|
||||||
| const supabase = supabaseAdmin(c) | ||||||
| const builderUrl = getEnv(c, 'BUILDER_URL') | ||||||
| const builderApiKey = getEnv(c, 'BUILDER_API_KEY') | ||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| const { data: staleBuilds, error: queryError } = await supabase | ||||||
| .from('build_requests') | ||||||
| .select('id, builder_job_id, app_id, owner_org, requested_by, platform, status, created_at') | ||||||
| .not('status', 'in', `(${[...TERMINAL_STATUSES].join(',')})`) | ||||||
| .lt('updated_at', new Date(Date.now() - STALE_THRESHOLD_MINUTES * 60 * 1000).toISOString()) | ||||||
| .order('updated_at', { ascending: true }) | ||||||
| .limit(BATCH_LIMIT) | ||||||
|
|
||||||
| if (queryError) { | ||||||
| cloudlogErr({ requestId: c.get('requestId'), message: 'Failed to query stale build_requests', error: queryError.message }) | ||||||
| return c.json(BRES) | ||||||
| } | ||||||
|
|
||||||
| if (!staleBuilds || staleBuilds.length === 0) { | ||||||
| cloudlog({ requestId: c.get('requestId'), message: 'No stale builds to reconcile' }) | ||||||
| return c.json(BRES) | ||||||
| } | ||||||
|
|
||||||
| cloudlog({ requestId: c.get('requestId'), message: `Found ${staleBuilds.length} stale builds to reconcile` }) | ||||||
|
|
||||||
| for (const build of staleBuilds) { | ||||||
| if (!build.builder_job_id) { | ||||||
| const createdAt = new Date(build.created_at).getTime() | ||||||
| const orphanCutoff = Date.now() - ORPHAN_THRESHOLD_HOURS * 60 * 60 * 1000 | ||||||
| if (createdAt < orphanCutoff) { | ||||||
| const { error: updateError } = await supabase | ||||||
| .from('build_requests') | ||||||
| .update({ | ||||||
| status: 'failed', | ||||||
| last_error: 'Build request was never submitted to builder', | ||||||
| updated_at: new Date().toISOString(), | ||||||
| }) | ||||||
| .eq('id', build.id) | ||||||
|
|
||||||
| if (updateError) { | ||||||
| cloudlogErr({ requestId: c.get('requestId'), message: 'Failed to mark orphan build as failed', buildId: build.id, error: updateError.message }) | ||||||
| errors++ | ||||||
| } | ||||||
| else { | ||||||
| orphaned++ | ||||||
| } | ||||||
| } | ||||||
| continue | ||||||
| } | ||||||
|
|
||||||
| try { | ||||||
| const response = await fetch(`${builderUrl}/jobs/${build.builder_job_id}`, { | ||||||
| method: 'GET', | ||||||
| headers: { 'x-api-key': builderApiKey }, | ||||||
| }) | ||||||
|
|
||||||
| if (!response.ok) { | ||||||
| cloudlogErr({ requestId: c.get('requestId'), message: 'Builder status fetch failed', buildId: build.id, jobId: build.builder_job_id, status: response.status }) | ||||||
| errors++ | ||||||
| continue | ||||||
| } | ||||||
|
||||||
|
|
||||||
| const builderJob = await response.json() as BuilderStatusResponse | ||||||
| const jobStatus = builderJob.job.status | ||||||
|
|
||||||
| const { error: updateError } = await supabase | ||||||
| .from('build_requests') | ||||||
| .update({ | ||||||
| status: jobStatus, | ||||||
| last_error: builderJob.job.error || null, | ||||||
| updated_at: new Date().toISOString(), | ||||||
| }) | ||||||
| .eq('id', build.id) | ||||||
|
Comment on lines
+101
to
+111
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unbounded external status value written directly to the database.
Consider validating or mapping 🤖 Prompt for AI Agents |
||||||
|
|
||||||
| if (updateError) { | ||||||
| cloudlogErr({ requestId: c.get('requestId'), message: 'Failed to update build_requests status', buildId: build.id, error: updateError.message }) | ||||||
| errors++ | ||||||
| continue | ||||||
| } | ||||||
|
|
||||||
| reconciled++ | ||||||
|
|
||||||
| if ( | ||||||
| TERMINAL_STATUSES.has(jobStatus) | ||||||
|
||||||
| TERMINAL_STATUSES.has(jobStatus) | |
| (jobStatus === 'succeeded' || jobStatus === 'failed') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restrict build-time billing to billable terminal statuses
The reconciliation path currently records build time for any status in TERMINAL_STATUSES, so jobs that end as cancelled, expired, or released will be billed whenever they include timestamps. In contrast, the normal /public/build/status.ts flow only calls recordBuildTime for succeeded/failed, so this introduces inconsistent and potentially inflated billing depending on which path updates the row first.
Useful? React with 👍 / 👎.
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| SELECT pgmq.create('cron_reconcile_build_status'); | ||
|
|
||
| INSERT INTO public.cron_tasks ( | ||
| name, | ||
| description, | ||
| task_type, | ||
| target, | ||
| batch_size, | ||
| second_interval, | ||
| minute_interval, | ||
| hour_interval, | ||
| run_at_hour, | ||
| run_at_minute, | ||
| run_at_second, | ||
| run_on_dow, | ||
| run_on_day | ||
| ) VALUES ( | ||
| 'reconcile_build_status', | ||
| 'Send build status reconciliation job to queue every 15 minutes', | ||
| 'queue', | ||
| 'cron_reconcile_build_status', | ||
| null, | ||
| null, | ||
| 15, | ||
| null, | ||
| null, | ||
| null, | ||
| 0, | ||
| null, | ||
| null | ||
| ) | ||
| ON CONFLICT (name) DO UPDATE SET | ||
| description = EXCLUDED.description, | ||
| task_type = EXCLUDED.task_type, | ||
| target = EXCLUDED.target, | ||
| minute_interval = EXCLUDED.minute_interval, | ||
| run_at_second = EXCLUDED.run_at_second, | ||
| updated_at = NOW(); | ||
|
|
||
| INSERT INTO public.cron_tasks ( | ||
| name, | ||
| description, | ||
| task_type, | ||
| target, | ||
| batch_size, | ||
| second_interval, | ||
| minute_interval, | ||
| hour_interval, | ||
| run_at_hour, | ||
| run_at_minute, | ||
| run_at_second, | ||
| run_on_dow, | ||
| run_on_day | ||
| ) VALUES ( | ||
| 'reconcile_build_status_queue', | ||
| 'Process build status reconciliation queue', | ||
| 'function_queue', | ||
| '["cron_reconcile_build_status"]', | ||
| null, | ||
| null, | ||
| 5, | ||
| null, | ||
| null, | ||
| null, | ||
| 0, | ||
| null, | ||
| null | ||
| ) | ||
| ON CONFLICT (name) DO UPDATE SET | ||
| description = EXCLUDED.description, | ||
| task_type = EXCLUDED.task_type, | ||
| target = EXCLUDED.target, | ||
| minute_interval = EXCLUDED.minute_interval, | ||
| run_at_second = EXCLUDED.run_at_second, | ||
| updated_at = NOW(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Use
createHono()instead ofnew Hono<MiddlewareKeyVariables>().The coding guidelines require using
createHonofromutils/hono.tsfor all Hono app initialization. This likely wires in shared middleware (e.g., request ID generation) that you rely on viac.get('requestId').Proposed fix
As per coding guidelines: "Use
createHonofromutils/hono.tsfor all Hono framework application initialization and routing." Based on learnings: "UsecreateHonofromutils/hono.tsfor all Hono framework application initialization and routing."🤖 Prompt for AI Agents