Repository navigation
feat(appkit): configurable time limit for generate-types --wait - #735
atilafassina wants to merge 6 commits into
Conversation
`appkit generate-types --wait` waited for the SQL warehouse with a hard-coded 5-minute cap per path: queries and metric views each had their own, so a warehouse stuck in STARTING could block a build for 10 minutes. When the cap ran out, the message didn't say the warehouse was stuck or which limit applied. - Add `--wait-timeout <seconds>` and `APPKIT_TYPEGEN_WAIT_TIMEOUT` (flag wins; default 300s; 0 = check once, don't wait). - One budget per run: the metric-view wait gets what the query wait left. - Backoff sleeps no longer run past the deadline. - On timeout with committed types: keep them, exit 0, and warn with `warehouse_unavailable: warehouse <id> is still <STATE> after the <N>s wait limit; kept existing <files>`. - On timeout without committed types: exit 1 with a dedicated `warehouse_unavailable` failure naming the warehouse, its state, the limit and how to raise it (TypegenFatalError.code). - Scale the background worker's lock staleness with a longer limit so a live worker isn't mistaken for an orphan. A running warehouse is unaffected. Refs: LA-2399, LA-2384 Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Atila Fassina <atila@fassina.eu>
📦 Bundle size reportCompared against
|
| dist | raw | gzip |
|---|---|---|
| JS (runtime) | 1.3 MB (+13 KB) | 486 KB (+3.5 KB) |
| Type declarations | 472 KB (+1.2 KB) | 170 KB (+448 B) |
| Source maps | 2.7 MB (+21 KB) | 901 KB (+5.9 KB) |
| Other | 11 KB | 3.7 KB |
| Total | 4.5 MB (+35 KB) | 1.5 MB (+9.8 KB) |
Per-entry composition (own code — deps external (as shipped))
| Entry | Initial (gz) | Lazy (gz) | Total (gz) | node_modules (min) | Own code (min) |
|---|---|---|---|---|---|
. |
111 KB (+1.4 KB) | 2.5 KB | 114 KB (+1.4 KB) | external | 359 KB (+3.8 KB) |
./beta |
113 KB | 483 B | 114 KB | external | 339 KB |
./testing |
46 KB (+2 B) | 34 KB (+1.4 KB) | 80 KB (+1.4 KB) | external | 233 KB (+3.8 KB) |
./tsdown |
520 B | 0 B | 520 B | external | 813 B |
./type-generator |
27 KB (+1.2 KB) | 0 B | 27 KB (+1.2 KB) | external | 78 KB (+3.8 KB) |
Chunks:
| Entry | Chunk | Load | Size (gz) |
|---|---|---|---|
. |
index.js |
initial | 107 KB |
. |
utils.js |
initial | 4.6 KB |
. |
remote-tunnel-manager.js |
lazy | 2.5 KB |
./beta |
beta.js |
initial | 93 KB |
./beta |
modular.js |
initial | 6.0 KB |
./beta |
stream-manager.js |
initial | 5.9 KB |
./beta |
service-context.js |
initial | 4.3 KB |
./beta |
databricks.js |
initial | 3.3 KB |
./beta |
client.js |
initial | 648 B |
./beta |
index.js |
initial | 20 B |
./beta |
supervisor-api.js |
lazy | 192 B |
./beta |
databricks.js |
lazy | 177 B |
./beta |
index.js |
lazy | 114 B |
./testing |
manifest.js |
initial | 33 KB |
./testing |
index.js |
initial | 10 KB |
./testing |
wide-event-emitter.js |
initial | 2.9 KB |
./testing |
index.js |
lazy | 30 KB |
./testing |
remote-tunnel-manager.js |
lazy | 2.5 KB |
./testing |
utils.js |
lazy | 1.8 KB |
./tsdown |
index.js |
initial | 520 B |
./type-generator |
index.js |
initial | 27 KB |
@databricks/appkit-ui
npm tarball (packed): 409 KB (+4 B) — gzipped download (dist + bin; excludes release-only docs/NOTICE).
| dist | raw | gzip |
|---|---|---|
| JS (runtime) | 440 KB | 151 KB |
| Type declarations | 272 KB | 100 KB (+1 B) |
| Source maps | 869 KB | 293 KB |
| CSS | 16 KB | 3.2 KB |
| Total | 1.6 MB | 547 KB (+1 B) |
Per-entry composition (consumer bundle — deps bundled, peerDeps external)
| Entry | Initial (gz) | Lazy (gz) | Total (gz) | node_modules (min) | Own code (min) |
|---|---|---|---|---|---|
./js |
5.3 KB | 49 KB | 55 KB | 208 KB | 14 KB |
./js/beta |
2.1 KB | 0 B | 2.1 KB | 0 B | 5.0 KB |
./react |
441 KB | 49 KB | 489 KB | 1.4 MB | 179 KB |
./react/beta |
14 KB | 0 B | 14 KB | 25 KB | 16 KB |
Chunks:
| Entry | Chunk | Load | Size (gz) |
|---|---|---|---|
./js |
index.js |
initial | 5.2 KB |
./js |
chunk |
initial | 120 B |
./js |
apache-arrow |
lazy | 49 KB |
./js/beta |
beta.js |
initial | 2.1 KB |
./react |
index.js |
initial | 439 KB |
./react |
tslib |
initial | 2.1 KB |
./react |
apache-arrow |
lazy | 49 KB |
./react/beta |
beta.js |
initial | 14 KB |
|
Review 1 · Pass · 1 minor Does the change do what the task asked? Yes. Gate: major and worse must be fixed. The configurable wait limit, cached-type fallback, and clear no-cache failure work in the exercised paths, and the requested PR is open for review. A metric-view-only timeout has a misleading line saying no type files were written even though it wrote analytics.d.ts. Not required (below the gate)
|
🤖 AppKit PR bot🔬 Run evalsStart an eval for this PR from the evals-monitor app: Go to Evals Monitor → 📦 Try this PR's app templateScaffolds a new app from this PR's SDK build. Run it in any folder (requires the GitHub CLI — gh run download 43416607598 -R databricks/appkit -n appkit-template-0.95.0-pr.ff6d303-work-17-time-limit-for-generate-types-735 -D appkit-pr-735 \
&& unzip -o "appkit-pr-735/appkit-template-0.95.0-pr.ff6d303-work-17-time-limit-for-generate-types-735.zip" -d "appkit-pr-735" \
&& databricks apps init --template "appkit-pr-735"The template pins |
|
Review 2 · Fail · 1 critical, 1 major Does the change do what the task asked? No. Gate: major and worse must be fixed. The STARTING warehouse timeout, cached type fallback, and clear failure work in the exercised tests, and the repository checks pass. The task is not complete because PR #735 remains a draft, and an in-flight warehouse status request can still exceed the claimed wait limit indefinitely. Required
|
A `warehouses.get` that never answered could keep `generate-types --wait` pending past the wait limit: the loop only checked the deadline between polls, so an in-flight request had no bound. - Each poll inside `waitUntilRunning` now gets only the budget left; when it runs out the request is aborted and the wait fails with the usual `warehouse_unavailable` timeout, naming the last state seen. - The preflight status check and the start request for a stopped warehouse are bounded too (the time left, kept between 10s and 30s), and a timeout there degrades to the same committed-types gate with a `warehouse_unavailable: ... did not answer a status request` detail. - The deadline is enforced in AppKit as well as passed to the SDK as a signal, so it holds even if a transport ignores the signal. Refs: LA-2399, LA-2384 Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Atila Fassina <atila@fassina.eu>
…prerelease id) Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Atila Fassina <atila@fassina.eu>
|
Review 3 · Pass · 1 pre-existing Does the change do what the task asked? Yes. Gate: major and worse must be fixed. The two earlier findings are resolved: PR #735 is ready for review, and an unresponsive status request now settles at the wait limit. Focused tests pass for readiness, timeout fallback, failure without cached types, and a hung request; the separate prerelease version issue predates this change. Not required (there before this change)
|
|
Review 4 · Pass · 1 pre-existing Does the change do what the task asked? Yes. Gate: major and worse must be fixed. The change meets the requested warehouse wait limit, cached-type fallback, clear failure, tests, documentation, and ready-for-review PR requirements. Local focused and full checks pass, and the only issue observed is an unrelated docs anchor that predates this branch. Not required (there before this change)
|
…time-limit-for-generate-types-
calvarjorge
left a comment
There was a problem hiding this comment.
LGTM. One wait limit for the whole run, plus per-request deadlines, fixes the stuck-STARTING hang, and the new messages are clear. I left a few non-blocking comments inline. Only the first one changes behaviour.
| let state: WarehouseState; | ||
| try { | ||
| state = await getWarehouseState(client, warehouseId, { | ||
| timeoutMs: Math.max(0, maxMs - (Date.now() - start)), |
There was a problem hiding this comment.
The last poll runs at the deadline with timeoutMs ≈ 0, so it's aborted before a real API can answer. A warehouse that turns RUNNING during the last backoff (up to 15s) is reported as timed out, and without committed types the run exits 1. The test at warehouse-status.test.ts:200 passes only because its stub answers on a microtask.
Suggestion: give the poll a floor, e.g. Math.max(maxMs - (Date.now() - start), 5_000) (goes over the limit by at most 5s), and make the stub answer after a short delay.
| code: WAREHOUSE_UNAVAILABLE_CODE, | ||
| summary: [ | ||
| `Warehouse ${pc.bold(warehouseId)} was not ready in time, and there are no committed types to use instead.`, | ||
| "No type files were written.", |
There was a problem hiding this comment.
This is wrong for a metric-view-only app: analytics.d.ts has already been written by the time this throws. Suggestion: say what was skipped (e.g. `${missingFiles.join(", ")} not written.`) or drop the line.
| isEnvironmental = true; | ||
| environmentalCause = "unavailable"; | ||
| environmentalDetail = describeWaitTimeout(err, waitTimeoutMs); | ||
| } else if (err instanceof WarehouseStatusTimeoutError) { |
There was a problem hiding this comment.
No test reaches this branch or its twin at index.ts:813: none makes the first getWarehouse or startWarehouse hang. One test per path would cover it: use fake timers to go past the 10s bound, then check the fallback warning and the warehouse_unavailable failure.
|
|
||
| The limit also holds when the warehouse API stops answering: every status request made while waiting is cut off when the limit runs out. The first status check, and the start request for a stopped warehouse, each get the time left in the limit, but at least 10 seconds and at most 30 seconds. So with a limit under 10 seconds, a slow API can still take up to 10 seconds to answer. | ||
|
|
||
| Set the limit in seconds with the `--wait-timeout` flag or the `APPKIT_TYPEGEN_WAIT_TIMEOUT` environment variable (the flag wins). `0` checks the warehouse once and doesn't wait. The environment variable also applies to `appKitTypesPlugin()` when it runs in a production Vite build. |
There was a problem hiding this comment.
0 still starts a stopped warehouse. Suggest: "0 checks the warehouse once (starting it if it's stopped) and doesn't wait."
…time-limit-for-generate-types-
…time-limit-for-generate-types-
Jira: LA-2399 (fix direction 2 of LA-2384: deploy hangs on
appkit generate-types --waitwhen the SQL warehouse never starts)Problem
The app template's
prebuildrunsappkit generate-types --wait. When the SQL warehouse stays inSTARTING, the build blocks and the error doesn't say why. In the NFL-Scout eval the App Builder'sdeploy.shhung in this step until the agent hit tool-call timeouts.The wait already had a hard-coded 5-minute cap, but:
result: unknowntypes were written when they weren't.Change
--wait-timeout <seconds>andAPPKIT_TYPEGEN_WAIT_TIMEOUT(seconds). The flag wins over the env var. Default is 300 s (same as before).0checks the warehouse once and doesn't wait. An invalid env value is ignored with a warning; an invalid flag value is rejected by the CLI.warehouses.getinside the wait gets only the time left in the limit. If it hasn't answered by then, it's aborted and the run fails with the usualwarehouse_unavailabletimeout naming the last state seen. The first status check and the start request for a stopped warehouse get the time left too, but at least 10 s and at most 30 s. AppKit enforces the deadline itself as well as passing a signal to the SDK, so it holds even if a transport ignores the signal.TypegenFatalErrorwithcode: "warehouse_unavailable"). The message names the warehouse, its state and the limit, and says how to raise the limit:warehouse_unavailableis the same one used by LA-2398 (deploy.sh check) and LA-2400 (deploy skill docs).--wait-timeout. Its single-flight lock now stays valid for longer when the limit is longer than the default, so a live worker isn't taken for an orphan.warehouses.get, then the DESCRIBEs.docs/docs/development/type-generation.md.Tests
type-generator/tests/warehouse-wait-limit.test.ts(new) runs the real query and metric-view paths and the real wait loop, with only the SDK client stubbed. It covers: a running warehouse (no wait); a warehouse that becomes ready within the limit; limit reached with committed types (fallback and warning); limit reached without committed types (clear failure); a stopped warehouse that is started and never comes up; a status request that hangs mid-wait; the env var; and the shared budget across queries and metric views.warehouse-status.test.tscovers the typed timeout error, a status request that never answers (the wait still ends at the limit and the request is aborted), bounded status and start requests, no sleeping past the deadline, the final check at the limit,maxMs: 0, and the resolve order (flag, then env, then default, with invalid env values ignored).--wait-timeout, passing it through in ms, the worker inheriting it, and the lock's staleness window. They also reset commander's singleton option state between tests: one existing worker test only passed because--waitleaked in from the previous test.pnpm build,pnpm docs:build,pnpm -r typecheck,pnpm check,pnpm test(6160 passed) all pass locally.This pull request and its description were written by Isaac.