fix(bigquery): wait on job status instead of getQueryResults (BRU-5103)#2294
fix(bigquery): wait on job status instead of getQueryResults (BRU-5103)#2294batikankarakan wants to merge 6 commits into
Conversation
RunQueryWithoutResult/Select/SelectWithSchema waited for a query via job.Read, which polls jobs.getQueryResults. When a job fails for a retryable reason (e.g. "rateLimitExceeded" on a table over its update quota), getQueryResults surfaces that terminal failure as a retryable API error, so the BigQuery client's retryer keeps polling the already-failed job. With no context deadline this hangs indefinitely: in BRU-5103 a CREATE TABLE hit the rate limit, failed after 0s, but the task stayed RUNNING for a full 24h (worker kept heartbeating a blocked, idle process) until the orchestrator's 24h run-timeout finally cancelled it. Add waitForJobCompletion, which polls job.Status (jobs.get) until the job reaches a terminal state and returns the job's own error. jobs.get reports a failed job's state immediately rather than as a retryable error, so failures surface at once; genuinely running queries keep being polled until they finish, so long-running queries are not cut short. Select/SelectWithSchema call it before job.Read, so Read only runs once the job is known-complete and returns without retrying. Also add the jobs.get route to mockBqHandler (previously unmocked, which made status polling hang in tests) and a regression test covering a job that completes with a rate-limit error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
pkg/bigquery/db.go:290-292
**Transient Status Errors Abort Jobs**
When a long-running query is still active, any temporary `jobs.get` failure now returns from `waitForJobCompletion` and fails the Bruin task. The previous `job.Read` polling path retried transient BigQuery API errors, so a short 5xx or network blip can now fail a query that would otherwise complete successfully.
### Issue 2 of 3
pkg/bigquery/db.go:297
**Terminal Errors Lose Shape**
Terminal job failures now return `status.Err()`, which is a BigQuery job error rather than the `googleapi.Error` shape callers previously received from `job.Read`. Any caller that classifies BigQuery failures with `errors.As` for HTTP code or structured API errors can no longer detect those fields for failed jobs.
### Issue 3 of 3
pkg/bigquery/db_test.go:541-542
**Running Mock Never Completes**
This handler returns the same response for `jobs.insert` and `jobs.get`. A test that submits a `RUNNING` job through `mockBqHandler` will keep receiving `RUNNING` from status polling and hang until its context expires, so the shared mock is unsafe for the new polling behavior.
Reviews (1): Last reviewed commit: "fix lint (ifElseChain->switch) and short..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fresh, err := job.Status(ctx) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
When a long-running query is still active, any temporary jobs.get failure now returns from waitForJobCompletion and fails the Bruin task. The previous job.Read polling path retried transient BigQuery API errors, so a short 5xx or network blip can now fail a query that would otherwise complete successfully.
Rule Used: What: Comments should be concise and targeted to s... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/bigquery/db.go
Line: 290-292
Comment:
**Transient Status Errors Abort Jobs**
When a long-running query is still active, any temporary `jobs.get` failure now returns from `waitForJobCompletion` and fails the Bruin task. The previous `job.Read` polling path retried transient BigQuery API errors, so a short 5xx or network blip can now fail a query that would otherwise complete successfully.
**Rule Used:** What: Comments should be concise and targeted to s... ([source](https://app.greptile.com/bruin/-/custom-context?memory=6adfbd2b-1b23-4560-9e00-6254e7cb7c70))
How can I resolve this? If you propose a fix, please make it concise.| status = fresh | ||
| } | ||
| if status.Done() { | ||
| return status.Err() |
There was a problem hiding this comment.
Terminal job failures now return status.Err(), which is a BigQuery job error rather than the googleapi.Error shape callers previously received from job.Read. Any caller that classifies BigQuery failures with errors.As for HTTP code or structured API errors can no longer detect those fields for failed jobs.
Rule Used: What: Comments should be concise and targeted to s... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/bigquery/db.go
Line: 297
Comment:
**Terminal Errors Lose Shape**
Terminal job failures now return `status.Err()`, which is a BigQuery job error rather than the `googleapi.Error` shape callers previously received from `job.Read`. Any caller that classifies BigQuery failures with `errors.As` for HTTP code or structured API errors can no longer detect those fields for failed jobs.
**Rule Used:** What: Comments should be concise and targeted to s... ([source](https://app.greptile.com/bruin/-/custom-context?memory=6adfbd2b-1b23-4560-9e00-6254e7cb7c70))
How can I resolve this? If you propose a fix, please make it concise.| case r.Method == http.MethodGet && strings.HasPrefix(r.RequestURI, fmt.Sprintf("/projects/%s/jobs/%s", projectID, jobID)): | ||
| write(w, jsr.statusCode, jsr.response) // jobs.get (job.Status) |
There was a problem hiding this comment.
This handler returns the same response for jobs.insert and jobs.get. A test that submits a RUNNING job through mockBqHandler will keep receiving RUNNING from status polling and hang until its context expires, so the shared mock is unsafe for the new polling behavior.
Rule Used: What: Comments should be concise and targeted to s... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/bigquery/db_test.go
Line: 541-542
Comment:
**Running Mock Never Completes**
This handler returns the same response for `jobs.insert` and `jobs.get`. A test that submits a `RUNNING` job through `mockBqHandler` will keep receiving `RUNNING` from status polling and hang until its context expires, so the shared mock is unsafe for the new polling behavior.
**Rule Used:** What: Comments should be concise and targeted to s... ([source](https://app.greptile.com/bruin/-/custom-context?memory=6adfbd2b-1b23-4560-9e00-6254e7cb7c70))
How can I resolve this? If you propose a fix, please make it concise.Addresses Greptile P3 review note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Reviews (2): Last reviewed commit: "test: document mockBqHandler jobs.get/in..." | Re-trigger Greptile |
Terminal job failures come back as *bigquery.Error (via job.Status), whose default string is a verbose struct dump. Handle it in formatError so callers get the same clean message the googleapi path produces. Addresses Greptile P2 review note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
pkg/bigquery/db.go:775-776
**Preserve job errors** This converts terminal `*bigquery.Error` values into plain string errors. When a completed job fails with `rateLimitExceeded`, callers can no longer use `errors.As` to read `Reason` or `Location`, so retry and classification logic loses the BigQuery failure details even though the message is cleaner.
Reviews (3): Last reviewed commit: "fix(bigquery): surface clean message for..." | Re-trigger Greptile |
| if errors.As(err, &bqError) && bqError.Message != "" { | ||
| return fmt.Errorf("%s", bqError.Message) |
There was a problem hiding this comment.
*bigquery.Error values into plain string errors. When a completed job fails with rateLimitExceeded, callers can no longer use errors.As to read Reason or Location, so retry and classification logic loses the BigQuery failure details even though the message is cleaner.
Rule Used: What: Comments should be concise and targeted to s... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/bigquery/db.go
Line: 775-776
Comment:
**Preserve job errors** This converts terminal `*bigquery.Error` values into plain string errors. When a completed job fails with `rateLimitExceeded`, callers can no longer use `errors.As` to read `Reason` or `Location`, so retry and classification logic loses the BigQuery failure details even though the message is cleaner.
**Rule Used:** What: Comments should be concise and targeted to s... ([source](https://app.greptile.com/bruin/-/custom-context?memory=6adfbd2b-1b23-4560-9e00-6254e7cb7c70))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Wrap the terminal *bigquery.Error in jobError, which presents a clean message via Error() but Unwraps to the original, so callers can still errors.As it for Reason/Location. Addresses Greptile review note on the previous stringify. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Reviews (4): Last reviewed commit: "fix(bigquery): preserve structured job e..." | Re-trigger Greptile |
1 participant
Problem (BRU-5103)
A
CREATE TABLE IF NOT EXISTSin theenergy_unlicensedpipeline hit BigQuery's per-table update-rate quota and failed after 0 seconds — yet the task stayedRUNNINGfor a full 24 hours, delaying client deliveries, until the orchestrator's 24h run-timeout finally cancelled it.Confirmed from OXR logs for the stuck task:
Root cause
RunQueryWithoutResult/Select/SelectWithSchemawaited for the query viajob.Read, which pollsjobs.getQueryResults. When a job fails for a retryable reason (rateLimitExceeded,backendError, …),getQueryResultssurfaces that terminal failure as a retryable API error, so the BigQuery Go client's retryer keeps polling the already-failed job. The execution context has no deadline, so it retries ~once/minute forever. The process sits blocked-but-alive, so the worker keeps heartbeating and OXR's zombie detector (1-min heartbeat staleness) never fires.Fix
Add
waitForJobCompletion, which pollsjob.Status(jobs.get) until the job reaches a terminal state and returns the job's own error:jobs.getreports a failed job's state as data (status.Err()), not a retryable API error — so failures surface immediately.Select/SelectWithSchemacall it beforejob.Read, soReadruns only once the job is known-complete and returns without retrying.job.LastStatus()avoids an extra RPC when the submit/prior poll already reports a terminal state.Tests
jobs.getroute tomockBqHandler(previously unmocked → status polling would hang in tests).TestDB_RunQueryWithoutResultSurfacesTerminalJobError: submit returnsRUNNING,jobs.getreturnsDONE+rateLimitExceeded→ error surfaces immediately.pkg/bigquerysuite passes in ~1s (before this fix the same suite hit the 10-min test timeout on the hang).🤖 Generated with Claude Code