Add build timeout#1339
Conversation
- Introduced a new configuration option `buildTimeout` in `config.example.jsonc` to specify the maximum time allowed for a single build task, defaulting to 10 minutes. - Updated the `BuildContext` and related methods to support context cancellation, allowing for more robust handling of build operations. - Refactored various functions across the codebase to accept a context parameter, improving the ability to manage timeouts and cancellations during HTTP requests and package installations. - Enhanced error handling to provide clearer messages when build tasks exceed the specified timeout.
There was a problem hiding this comment.
Pull request overview
This PR adds a configurable build-level timeout and threads context.Context through key build, fetch, and package-install paths so long-running builds can be canceled deterministically (e.g., on timeout).
Changes:
- Added
buildTimeoutto server configuration (default 600s) and enforced it in the build queue viacontext.WithTimeout. - Extended
BuildContextto carry a context and propagated it through npm metadata/tarball fetching, GitHub installs, loader execution, and supporting utilities. - Added context-aware HTTP fetching (
FetchWithContext) and made supporting operations (sleep/backoff, downloads, extraction) cancellable.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
server/build_queue.go |
Applies per-build timeout and maps deadline exceeded into a clearer timeout error. |
server/build.go |
Adds context to BuildContext and propagates cancellation checks throughout build stages. |
server/build_resolver.go |
Propagates build context into dependency resolution and nested build contexts. |
server/build_analyzer.go |
Ensures analyzer sub-builds inherit the parent build context. |
server/npmrc.go |
Adds context-aware npm metadata fetch, tarball download/extract, GitHub install hook, and cancellable backoff. |
server/git.go |
Adds context-aware GitHub ls-remote and GitHub tarball installation. |
server/loader.go |
Runs Deno loader under a context-derived timeout rather than always using Background(). |
server/loader_implements.go |
Threads context through Svelte/Vue loader compilation and execution. |
server/cjs_module_lexer.go |
Uses build context for subprocess timeouts and adds context-aware download/decompress for the lexer binary. |
server/dts_transform.go |
Passes build context into nested type-build contexts. |
internal/fetch/fetch.go |
Introduces FetchWithContext and makes Fetch delegate to it. |
server/config.go |
Adds BuildTimeout config with default normalization. |
config.example.jsonc |
Documents and provides an example value for buildTimeout. |
Comments suppressed due to low confidence (1)
server/npmrc.go:645
gzip.NewReaderreturns a*gzip.Readerthat should be closed to release resources and validate the gzip footer/CRC. Consider addingdefer unziped.Close()after creation inextractPackageTarballContext.
unziped, err := gzip.NewReader(&contextReader{ctx: ctx, reader: tarball})
if err != nil {
return
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mark.Add(markId) | ||
| installed, err := npmrc.installPackage(pkg) | ||
| installed, err := npmrc.installPackageContext(ctx, pkg) | ||
| if err != nil { | ||
| return |
There was a problem hiding this comment.
installDependenciesContext now returns an error, but errors from dependency resolution/installation inside the goroutines are silently dropped (e.g. installPackageContext errors just return). This means callers may see nil even when dependency installs failed, which undermines the new error-handling path. Consider capturing the first non-context error from any goroutine (and optionally canceling the context) and returning it after wg.Wait().
| if err != nil { | ||
| return err | ||
| } | ||
| defer f.Close() | ||
| n, err := io.Copy(f, tr) | ||
| n, err := io.Copy(f, &contextReader{ctx: ctx, reader: tr}) |
There was a problem hiding this comment.
defer f.Close() is inside the tar extraction loop, so for large tarballs it can accumulate many open file descriptors until the function returns. Close the file explicitly after the copy (or use an inline func scope) to avoid FD exhaustion.
|
|
||
| meta, err := task.ctx.Build(buildCtx) | ||
| if errors.Is(err, context.DeadlineExceeded) { | ||
| err = fmt.Errorf("build timeout after %d seconds", buildTimeout/time.Second) |
There was a problem hiding this comment.
The timeout handling replaces context.DeadlineExceeded with a new error string, which loses the original cause and prevents downstream errors.Is(err, context.DeadlineExceeded) checks. Consider wrapping the original error (e.g. include %w) or using a typed error so the timeout remains programmatically detectable while still improving the message.
| err = fmt.Errorf("build timeout after %d seconds", buildTimeout/time.Second) | |
| err = fmt.Errorf("build timeout after %d seconds: %w", buildTimeout/time.Second, err) |
buildTimeoutinconfig.example.jsoncto specify the maximum time allowed for a single build task, defaulting to 10 minutes.BuildContextand related methods to support context cancellation, allowing for more robust handling of build operations.