fix(storage): budget create-table/swap-tables job waits, add --timeout (#713) - #720
Draft
padak wants to merge 2 commits into
Draft
fix(storage): budget create-table/swap-tables job waits, add --timeout (#713)#720padak wants to merge 2 commits into
--timeout (#713)#720padak wants to merge 2 commits into
Conversation
#713) `storage create-table` and `storage swap-tables` -- the two halves of the BigQuery repartition path -- both move real data, yet both inherited the 60s STORAGE_JOB_MAX_WAIT meant for metadata jobs. Measured on an 800 MB BigQuery table: create ~15s, swap ~31s, so a table a few times larger blew the budget and reported STORAGE_JOB_TIMEOUT for a job that was still running and would succeed. Giving up locally never cancels the Storage job, so a short budget buys nothing and costs a false negative on the only in-place repartition path available today (the Storage API's PUT .../definition has no clustering field, so the copy+swap workaround cannot be replaced). - New TABLE_DATA_JOB_MAX_WAIT (300s) default for both commands. - New --timeout SECONDS on both, plus an optional `timeout` on the two REST routes (gt=0, so a bad budget is a 422 at the boundary). - The `None`-sentinel/reject-non-positive guard is now one shared normalize_job_timeout() in services/base.py; workspace_service's private copy is migrated onto it. The client-level default is unchanged (max_wait=None still means 60s), so the raw SDK client and the upload-table auto-create path keep their existing behaviour.
The new test armed a 0.0001s budget and patched out time.sleep, so it depended on real elapsed time between two loop iterations. time.monotonic() advances in ~15.6ms ticks on Windows, so the deadline never registered as expired there: the poller kept polling and the failure surfaced as ErrorCode.TIMEOUT plus unmatched GET /jobs/777 requests. It passed on macOS only because one HTTP roundtrip happens to exceed 100us. Use an already-expired budget (max_wait=-1.0) instead: the deadline is behind the first check, so the timeout branch is reached with no poll, no sleep and no dependence on the clock. Reproduced both shapes under a simulated 15.6ms tick -- old: TIMEOUT after 8 polls; new: STORAGE_JOB_TIMEOUT after 0. Stubbing time.monotonic was rejected as the fix: patching it on the client module patches the global time module, which httpx reads too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
storage create-tableandstorage swap-tablesnow default to a 300s job budget and accept--timeout SECONDS. The matching REST routes take an optionaltimeoutfield.Why
Issue #713 asks for a metadata-only clustering update on an existing BigQuery table. That part is blocked upstream, not here — verified independently against the canonical Storage API PHP client, whose own type for the update endpoint is:
No
clustering, no partitioning.PUT /v2/storage/tables/{id}/definitioncannot express the change, so astorage set-clusteringcommand would have to be written against an endpoint that does not exist. Not implemented here.What is fixable on the kbagent side is the second cost the issue lists: the copy+swap workaround — the only repartition path available today — has a job budget that is too short to survive it.
Both
create-table --source-table-id(a full data copy) andswap-tablesinheritedSTORAGE_JOB_MAX_WAIT = 60s, which exists for metadata jobs. Measured on an 800 MB BigQuery table: create ~15s, swap ~31s. A table a few times larger blows the budget, and because giving up locally never cancels the Storage job, the result is aSTORAGE_JOB_TIMEOUTreported for an operation that is still running and will succeed. A short budget buys nothing and costs a false negative.How
TABLE_DATA_JOB_MAX_WAIT = 300.0inconstants.py— the new default for both commands, with the measurements recorded as the rationale.--timeout SECONDSon both commands;max_waitthreaded through service → client →_wait_for_storage_job.timeout: float | None = Field(default=None, gt=0)on theCreateTable/SwapTablesrequest models, so a bad budget is a 422 at the boundary rather than anINVALID_ARGUMENTfrom the service.None-sentinel guard (reject<= 0, nevertimeout or DEFAULT—0.0is falsy) is now a single sharednormalize_job_timeout()inservices/base.py.workspace_service's private_normalize_timeoutis migrated onto it rather than duplicated.Deliberately unchanged: the client-level default.
max_wait=Nonestill resolves to 60s, so the raw SDK client and theupload-tableauto-create path (an empty-table metadata create) keep their current behaviour. Only the two data-moving commands get the larger budget.Behaviour change
A create/swap that previously failed at 60s now waits up to 300s before reporting a timeout. There is no case where the earlier failure was the desired outcome — it was a false negative on a live job — and
--timeout 60restores the old budget.Testing
make checkgreen (6383 passed, 12 skipped). New coverage:tests/test_base_service.py—normalize_job_timeout:None→ default, positive override wins,0.0/negative rejected asINVALID_ARGUMENT.tests/test_storage_swap.py— default is the data budget (asserted to exceedSTORAGE_JOB_MAX_WAIT), explicit forwarding, non-positive rejected before any HTTP (including on--dry-run), budget reaches the poller, and an exhausted budget raisesSTORAGE_JOB_TIMEOUTwithretryable=Truenaming the still-running job.tests/test_storage_write.py— same matrix forcreate-table, plus CLI forwarding.tests/test_server_router_calls.py— both routes forwardtimeout, omit →None, non-positive → 422 with the service never called.Four pre-existing
assert_called_once_withassertions were updated for the new kwarg.No live-project verification: that needs a BigQuery project and credentials I do not handle.
Docs
All silent-drift surfaces from CLAUDE.md convention #17 updated:
context.py(AGENT_CONTEXT), the CLAUDE.md command list,keboola-expert.md,commands-reference.md,gotchas.md(new entry: aSTORAGE_JOB_TIMEOUTis not a failure — do not blindly re-issue a swap, repeating one that landed swaps the tables back), andstorage-types-workflow.md. Tagged(since vNEXT, #713)per the feature-PR rule; no version bump, no changelog entry.Note: inside CLAUDE.md's command fence every
#line reads as an ATX heading tocheck_version_gates.py, so that blurb is indented past the 3-space allowance. Backticking the placeholder would also have passed, but the release-time residue scan applies the same quotation rule and would then never resolve it.Not addressed
The clustering endpoint itself. #713 should stay open against the Storage API; this PR only makes its documented workaround survive a large table.
Refs #713 — deliberately not
Fixes: the issue's actual ask (a metadata-onlyclustering update) needs a Storage API change that does not exist yet, so merging this
must not auto-close it.