docs(deploy): document deploy.buildTimeout in seconds or with a unit suffix - #3483
Conversation
avocet-bot
left a comment
There was a problem hiding this comment.
Review: #3483 — document deploy.buildTimeout in seconds or with a unit suffix
Reviewed head SHA: 063acf4eeff9dcbed9cf1d60b5a7fc6602cb5b9e
Verdict: ✅ Approve — no blocking issues. The documentation accurately reflects the upstream implementation; examples are valid and internally consistent.
Review skill evidence
Invoked the pr-review-toolkit:code-reviewer subagent (via the Task tool) against the full diff for a correctness/consistency pass. It returned no findings at or above threshold and one sub-threshold wording observation (noted below). I then independently cross-checked every documented rule against the upstream implementation and tests in denoland/deployng#3772 to confirm the prose matches real parser behavior.
What this PR changes (background for a reader new to this area)
Deno Deploy lets an application declare its build configuration in deno.json under a deploy section (build command, framework preset, runtime, etc.). One of those keys, deploy.buildTimeout, bounds how long the build step may run before the platform aborts it. This PR edits a single documentation file, deploy/reference/builds.md, to describe a new accepted format for that key and to update the Next.js example accordingly. No code is changed in this repository.
The format change itself lives upstream in denoland/deployng#3772 ("take the deno.json build timeout in seconds or with a unit suffix"). Previously buildTimeout was a whole number interpreted as minutes. The upstream change reinterprets a bare number as a whole number of seconds and additionally accepts a string with an s/m/h unit suffix. This is a breaking reinterpretation of bare numbers, which is why the docs and the example both needed to change.
Correctness verification (docs vs. actual parser)
I confirmed each documented claim against the upstream parser parseDurationSeconds and the timeout-clamping function getEffectiveBuildTimeout, plus their tests:
- "a whole number of seconds (
600)" — matchestypeof value === "number"→Number.isSafeInteger(value) && value >= 1.600is accepted (= 10 minutes). ✓ - "a whole number with an
s,morhsuffix ("90s","10m","1h")" — matches the regex/^([1-9][0-9]*)([smh])$/. All three examples are valid. ✓ - Invalid examples "(such as
0,"10"or"1.5m")" cause the wholedeploysection to be ignored — each maps to a real rejection path and each appears (directly or equivalently) in the upstreamreturns null for invalid buildTimeouttest loop:0→ fails thevalue >= 1number guard →null."10"→ a string with no unit suffix → no regex match →null."1.5m"→ the integer group[1-9][0-9]*rejects the fractional1.5→null.
ReturningnullfrombuildConfigFromDenoJsonis what "causes the wholedeploysection to be ignored," so the prose is accurate. ✓
- "A value above your plan's maximum (5 min Free / 30 min Pro) is capped to that maximum, and a value below 1 minute is raised to 1 minute" — matches
getEffectiveBuildTimeout'sMath.max(MIN_BUILD_TIMEOUT /* 1 */, Math.min(configuredTimeout, maxTimeout)). The new upstream test confirms0.5→1,60→30(Pro), and20→5(Free). ✓ - "Defaults to 5 minutes" — unchanged from the prior docs and consistent with
DEFAULT_BUILD_TIMEOUT. ✓
The Next.js example change (buildTimeout: 15 → "15m")
This change is correct and necessary. Under the old semantics a bare 15 meant 15 minutes. Under the new semantics a bare 15 means 15 seconds, which would then be raised to the 1-minute floor — a silent behavior change if left as-is. Switching to the string "15m" preserves the original 15-minute intent, and 15 minutes is within the 30-minute Pro-plan maximum. ✓
Markdown / rendering
List structure, 2-space continuation-line indentation, and inline code spans all match the surrounding bullets and are balanced. No rendering breakage.
Non-blocking observations (not reasons to withhold approval)
- Dropped word "positive". The old text read "positive whole number of minutes"; the new text reads "a whole number of seconds." Taken in isolation,
0is a whole number, which creates a slight tension with the separate rule "a value below 1 minute is raised to 1 minute." In practice the doc resolves this by explicitly listing0among the ignored values, so a reader is not misled (positive sub-minute values like30are raised;0is rejected). If an easy tweak is desired, restoring "positive" (e.g. "a positive whole number of seconds") would remove the ambiguity entirely. Optional. - Merge ordering. Upstream
denoland/deployng#3772is still open/unmerged at the time of review, and the parser behavior documented here is not yet deployed. The PR description already acknowledges this ("Should merge once denoland/deployng#3772 is deployed."), so this is a release-sequencing note for the author rather than a docs-correctness defect. Worth confirming the upstream change actually ships with the behavior as written before/at merge, since the docs become live immediately on merge.
Summary
The documentation is accurate against the real upstream parser and clamping logic, all six examples (3 valid, 3 invalid) are internally consistent with the stated rules, the Next.js example migration is correct, and the markdown is clean. Approving. The only caveat is the process note that the upstream feature should be deployed before (or together with) merging these docs.
Documents the new
deploy.buildTimeoutformat fromdenoland/deployng#3772: a whole number of seconds, or a whole number
with an
s,morhsuffix, raised to at least one minute and cappedto the plan maximum. The Next.js example uses
"15m".Should merge once denoland/deployng#3772 is deployed.