Enhancement/scale hardening 6.0.0 - #684
Merged
Merged
Conversation
Adds tests/other/load_test.js, a closed-loop saturation harness that drives
a fixed number of concurrent virtual users against a running server, samples
/health for pool state, and reports latency percentiles, the error breakdown
and the peak acquire queue depth.
The existing stress_test.js fires a fixed trickle of one request per 150ms
and only logs the responses, so it cannot show the failure modes that appear
under saturation. The new harness can reproduce them locally:
- unbounded growth of the acquire queue
- work continuing for clients that have already disconnected
(--abort-after)
- a server that stops answering altogether
Tarn's release() is synchronous: it runs the 'release' event handlers and then returns the resource to the free list in the same tick, without awaiting them. Clearing the page inside that handler therefore overlapped the next export whenever an acquire was already waiting, which is the normal case under saturation. The clearing is now started in the release handler but its promise is stored on the worker and awaited in factory.validate, which tarn does await. That keeps the work overlapped with the worker's idle time while making the handoff ordered. A page that fails to clear now returns false from validate, so tarn destroys and replaces the worker instead of exporting onto a page in an unknown state. Adds tests/other/page_isolation_test.js, which drives concurrent alternating SVG exports and asserts no response contains another chart's content. Note it passes both with and without this change: export.js already calls clearPageResources at the end of every export, which destroys the old charts and masks the overlap. The race was real but its visible effect was being covered by that second cleanup path, so this is a latent defect rather than an active one. The test is kept as a guard on the invariant. Measured no throughput cost: 19.17 req/s at concurrency 4 against a 18.95 req/s baseline, p50 unchanged at 200ms.
newPage() created a browser page and then configured it without a try/catch, so a throw from any of the configuration steps left that page open for the lifetime of the browser. The caller only ever receives the error and never had a reference to the page, so nothing else could close it. setPageContent is the likely thrower, since it injects the entire Highcharts bundle and is therefore sensitive to a CPU starved instance. That is what makes this matter: on a sustained create failure the pool retries every createRetryInterval, 200ms by default, so every attempt leaked another browser tab until the browser ran out of memory. Note this is our leak, not tarn's. Tarn does destroy a resource that arrives after its own create timeout has fired (Pool.js:398), but that only covers the case where the factory resolves late, not where it throws. Adds tests/other/page_leak_test.js, which induces the failure by pointing the Highcharts cache path at a directory that does not exist and then counts the browser's open pages. Verified it fails before this change (5 attempts, 5 pages left open, exit 1) and passes after (0 pages left open, exit 0). It drives the browser module directly, so it does not need a running server.
create() guarded the launch with `if (!browser)`, and nothing ever cleared that variable, so the guard could never fire again. When the browser process went away - an out of memory kill being the case that matters - the server stayed up but every export failed for the rest of its life, while /health continued to report healthy workers. On a load balanced deployment that makes the instance a black hole that still passes health checks, so autoscaling cannot route around it. The browser now emits into a disconnect handler that clears the reference and advances a generation counter, and newPage() relaunches when it finds no connected browser. Concurrent callers share one launch promise, since after a disconnect every pool worker discovers the missing browser at the same moment. The generation counter is what makes the pool recover. A page whose browser has gone away still returns false from isClosed(), so the existing validation cannot detect it and the pool keeps handing out pages belonging to a dead process - which is why the failure presented as an instant error rather than an acquire timeout. Workers are stamped with the generation they were created against and fail validation when it no longer matches, so tarn destroys and replaces them. close() now marks the shutdown as deliberate so that an intentional close is not mistaken for a crash and does not trigger a relaunch. Adds tests/other/browser_recovery_test.js. Verified it fails before this change (9 attempts across 30s, never recovered) and passes after (recovered after 2 attempts, 5/5 subsequent exports fine). It only kills a browser process parented to the server under test, so it cannot disturb an unrelated browser, and it skips itself on Windows.
Every internal failure is reported as HTTP 400, the same status as a malformed request, so nothing downstream could tell a capacity problem from callers sending bad data. That matters because the two need opposite responses: one means scale up or shed load, the other means fix the caller. The message text was the only signal, which is not something a dashboard or a client can branch on. Errors now optionally carry an errorCode, surfaced as a property of the error response. The codes live in lib/errors/codes.js with notes on what each one means for a caller, and are treated as a stable contract. ExportError.setError carries a code up from a wrapped error, since errors are wrapped as they travel up the stack and the reason would otherwise be lost at the first wrap. Status codes are deliberately unchanged - this server must not return 5xx. The property is omitted entirely when an error carries no code, so existing response shapes are untouched. Verified against a running server: a request with no body and one with no chart data both return EXPORT_INVALID_REQUEST, a valid export still returns 200 with the image, and under saturation 99 of 99 captured failures returned EXPORT_ACQUIRE_TIMEOUT.
The acquire queue was unbounded. Measured against the previous behaviour,
150 concurrent clients drove the queue to 144 waiting exports, and the
server accepted more than twenty times the work it could complete: 58.9%
of requests succeeded, the rest failing only after a full 5 second
acquire timeout, with each one holding its parsed body - up to
maxUploadSize - in memory while it waited. That memory pressure is what
eventually gets the browser killed, at which point the previous commit's
recovery path is all that saves the instance.
Throughput does not improve past the pool size. Measured, it peaks at
about 19 exports/s around concurrency 4 and then falls, so a deeper queue
cannot buy capacity - only latency and memory. The limit defaults to four
times maxWorkers, 32 with default settings, and requests beyond it are
refused before express.json parses the body, so a refusal is cheap.
The refusal is deliberately delayed, which is the non-obvious part. A
first attempt refused instantly and made things much worse: clients that
retry the moment they are refused pushed the request rate to 11000/s, the
event loop went entirely to producing refusals, and goodput collapsed from
~19 exports/s to 0.65. The 5 second acquire timeout had been providing
backpressure by accident, simply by making every client wait before it
could retry. queueRejectDelay puts that back on purpose, at a fraction of
the cost, holding only a socket and a timer. The timer is cleared if the
client disconnects first.
Measured, PNG, default pool:
concurrency 4 18.95 req/s p50 200ms -> 19.03 req/s p50 200ms
concurrency 40 15.82 goodput -> 16.18 goodput, 95.9% ok
concurrency 150 15.58 goodput -> 15.26 goodput
58.9% ok, p50 5105ms -> p50 502ms
peak queue 144 -> peak queue 32
Goodput is preserved, the queue is bounded, and the latency of a refusal
drops by an order of magnitude.
Adds --min-goodput and --max-queue assertions to the load harness so this
is a gate rather than something to eyeball. Verified the same invocation
fails against the unbounded queue (peak 144, exit 1) and passes with the
limit in place (peak 32).
This server must never answer with a 5xx. Nothing here sets one deliberately - the only statuses in the codebase are 200, 400, 401 and 429 - but the status of an error is not always ours to begin with. setError copies statusCode up from a wrapped error, and wrapped errors include ones raised while fetching from the CDN, which can carry whatever status a remote returned. Clamping at the single point every error response passes through makes a 5xx structurally impossible rather than something every future call site has to be careful about. Anything outside 1xx-4xx is answered as 400 and logged, since arriving there means an error carried a status it should not have. An error raised after the response has already begun now ends the response instead of being passed on. There is no status left to set at that point, and handing it onwards let the framework's own handler answer, which answers 500. Also reverts ExportError's constructor to taking only a message. Giving it a second parameter for the error code was wrong: cache.js:146 and cache.js:181 already pass a number there, intending a status, which the constructor has always ignored. Accepting a second parameter silently gave those a meaning and would have put numeric errorCode values into responses. Codes are now set with an explicit setCode() instead, and HttpError keeps its own unambiguous third parameter. Adds tests/unit/server_error.test.js, the first unit coverage of the server layer, including a sweep asserting no status from 100 to 599 produces a 5xx response. Verified 8 of its tests fail without the clamp and all pass with it. Verified against a running server: a valid export returns 200, a malformed request returns 400 with EXPORT_INVALID_REQUEST, and 214 refusals under saturation all returned 400 with EXPORT_QUEUE_FULL. No clamp warnings were logged, so nothing attempted a 5xx in normal operation.
Three changes that only make sense together: without correct detection
there is nothing to propagate, and without propagation detection only
suppresses a response.
Detection was wrong. The socket close listener set its flag only when the
close carried an error, so a clean disconnect - a proxy idle timeout, or a
caller cancelling - was never detected. It now listens on the response and
uses writableFinished to tell a client leaving early from the normal close
after a completed response. That also lets the removeAllListeners('close')
call go, which stripped Node's and Express's own socket listeners along
with ours.
The signal is now carried to the pool, which drops the work before it
takes a queue slot or a worker. Note the honest limit: an export already
being rendered when its client leaves still runs to completion, because
the browser operations cannot be cancelled. The win is entirely in not
starting work for someone who has gone.
Abandoned work is counted separately rather than as a failure, and
excluded from the success ratio. Without that, a server whose callers were
timing out reported itself as failing when nothing had gone wrong on its
side - measured, the ratio read 49% on a healthy server during an abandon
storm, and now reads 100%.
Measured, 60 clients all abandoning after 150ms over 8s, against the
bounded queue from the previous commit:
pool when the load stopped used=8 pending=7 -> used=0 pending=0
exports finished after the
last client had left 14 -> 0
discarded before taking
a worker 0 -> 848
Checked for the feedback effect that made the first queue-limit attempt
worse: request count was unchanged (3232 -> 3221), so a cheaper discard
path did not increase the load it has to absorb.
No regression on the normal path, which now runs this listener on every
request: concurrency 4 gives 18.68 req/s at p50 201ms against an
18.95 req/s baseline, the concurrency 150 gate passes, page isolation
passes, and droppedExports stays 0 throughout.
Worth noting the queue limit had already removed most of this problem -
the unbounded queue left 1367 exports queued and took 5s to drain, versus
7 and 2s here. This closes the remainder.
Closing a browser does not guarantee its process has gone. Observed directly: a process survived a close that resolved without error, kept running as a child of the server, and held Chrome's lock on the user data directory - which every launch here shares - so no later browser could start at all. The launch options deliberately leave SIGINT, SIGTERM and SIGHUP unhandled, so nothing cleans up on our behalf either. The close is now followed by confirming the process has exited, killing it if it has not. Liveness is checked with signal 0 rather than the child process object's exitCode, because that reads as undefined in some cases and `undefined !== null` would have been taken as exited - silently skipping both the wait and the kill, which is exactly the bug this is meant to catch. Adds browserConnected and consecutiveCreateFailures to /health. These distinguish a server that has lost its browser from one that has a browser but cannot make pages with it, which previously looked identical from outside and cost real time to tell apart while investigating. Deliberately not included: a circuit breaker that replaced the browser after repeated worker creation failures. Two reasons. The premise was wrong. It assumed tarn's createRetryInterval drives retrying at five attempts a second indefinitely. Measured under sustained load with page setup failing, creation was attempted 20 times in 15 seconds, about 1.3/s, because attempts are bounded by demand and by acquireTimeout rather than by the retry interval. An A/B with a growing backoff produced 20 attempts either way - the delays were applied, and the count did not move. There is no storm to cap. It also made things worse. Replacing a browser means closing and relaunching against the same user data directory, and the replacement loses a race with the outgoing process for that lock. Two attempts at sequencing it left the pool unable to start any browser at all, and the browser was never the fault in the first place: only page setup was failing. Where the browser genuinely dies, the generation check added earlier already handles it. Verified: normal load unchanged at 18.86 req/s and p50 200ms, the browser recovery test still passes, and a SIGTERM under load now leaves no browser process behind.
A JSDOM window and a DOMPurify instance were built on every call, which means on every SVG export. That is the bulk of the work sanitizing does, and it is synchronous, so the time went on the event loop and delayed every other request in flight rather than only the one being sanitized. Only the instance is shared. The options stay per call, because FORBID_ATTR depends on configuration that can be read at any time and DOMPurify applies whatever options each call passes. Measured 2.84ms per call before, 0.27ms after. The existing sanitize unit tests pass unchanged, and a check that the same input still sanitizes identically after 50 intervening calls confirms nothing carries over between them.
The timeout in createImage was raced against the screenshot and then left alone. When the screenshot won, which is the normal case, the timer stayed armed for the rest of its duration, holding itself and everything its callback closed over. Bounded, and small at the throughputs measured here, but there is no reason to keep it. Verified all four export types still work (png, jpeg, pdf, svg), that the timeout path itself still behaves - with EXPORT_RASTERIZATION_TIMEOUT=1 every export returns 400 with EXPORT_RASTERIZATION_TIMEOUT and its worker is recycled - and that load is unchanged at 18.92 req/s, p50 200ms.
Twenty five attempts four seconds apart meant a browser that could never start took about 100 seconds to say so. That is longer than an orchestrator normally waits before deciding an instance is unhealthy, so the instance was replaced while still reporting that it was starting and the actual reason never surfaced. Retrying is now bounded by a time budget, configurable and defaulting to 30 seconds. Delays grow and carry jitter so that instances restarting together do not retry in lockstep against whatever they are all contending for. The loop no longer recurses into itself either, which had been nesting the async stack once per attempt and made the failures awkward to read. Verified: a healthy start still takes one attempt and two seconds, and with the window set to 8000ms an unlaunchable browser is reported after 8 seconds and 6 attempts, with the backoff visible in the countdown of remaining window.
Neither keepAliveTimeout nor headersTimeout was set, so both took Node's defaults. Measured with a client watching its own socket, the server closed an idle keep-alive connection after 6001ms. A proxy or load balancer in front of it typically holds the same connection for 60 seconds, and whichever side has the shorter timeout closes first without the other knowing - so the proxy sends a request into a connection that is already going away, and the caller sees a gateway error unrelated to the request. Both are now set from configuration, defaulting to 65 seconds, which puts the closing on the proxy's side. headersTimeout is kept above keepAliveTimeout deliberately: shorter, and it would fire while a kept-alive connection was legitimately idle between requests. Verified with the same probe: at 5000ms the idle connection is closed after 6001ms, at the new default it is still held past 12 seconds, and the timeouts appear in the log at startup. Load is unchanged at 18.79 req/s, p50 200ms. Note this demonstrates the mechanism rather than the gateway error itself, which needs a real proxy in front to reproduce.
The three cleanup steps were started together in a Promise.allSettled, and closeServers() was not async and returned undefined - so it was treated as already finished and its callback, the only signal that the servers had actually closed, was discarded. The process exited as soon as killPool() resolved, cutting off every export still being served. That path runs on every restart, deployment and scale-in, so the dropped requests were not a rare occurrence. The steps are now ordered. Closing the servers first stops new work arriving and resolves once the requests already in flight have been answered, which is the drain; only then is the pool taken away. The wait is raced against a configurable timeout so one request that never completes cannot hold the shutdown open. closeIdleConnections() matters here because of the previous commit: with keepAliveTimeout raised to 65 seconds, close() would otherwise sit waiting on connections that are merely idle. Idle connections have no work worth waiting for, so they are ended and close() waits only on requests actually being served. Measured, SIGTERM sent five seconds into a twelve second load run: before 82 served, 189853 failed, including 7 ECONNRESET after 189 served, 0 failed, 0 aborted The ECONNRESETs are the requests that were mid-flight when the process exited - the part this fixes. The very large ECONNREFUSED count behind that figure is an artifact of the harness spinning against a closed port once the server had gone, not a production quantity; what it shows is that the server stopped answering immediately instead of draining. Afterwards the server keeps serving its established connections through the drain and exits with no browser process left behind.
It shared the default one, so Chrome refused to start whenever a server was already running against it - the test crashed on a ProcessSingleton error rather than reporting anything about page leaks. Now it uses a directory of its own, keyed by pid, and removes it afterwards. Verified it passes alongside a running server, leaves no directory behind, and does not disturb that server.
Nothing here could tell whether a change moved the rendered output. The functional tests establish that a request returns an image, not that it returns the same image, so a shifted margin or a resized plot area would pass unnoticed - which is the main risk in the dependency upgrade still to come. Layout is compared rather than pixels, deliberately. A different browser build shifts antialiasing and font hinting across an entire image while changing no layout at all, so pixel comparison reports differences that do not matter and buries the ones that do. For SVG the plot background rectangle is read out of the markup: its position and size within the SVG are exactly the margin geometry, and it is exact rather than inferred. For PNG the image is decoded - zlib inflate plus undoing the scanline filters, no new dependency - and the bounding box of non-transparent pixels gives the margins around the drawn content. Thirteen scenarios chosen for the things that move margins: wrapping titles, missing titles, subtitles and credits, legend in three placements and disabled, wide y axis labels, rotated x labels, explicit margin and spacing, stacked columns and a pie with data labels. Each is exported as SVG, PNG, PNG at a fixed small size and PNG at 2x scale, for 52 cases. Verified both ways. Against a frozen worktree of master with its own node_modules, all 52 cases report identical layout, so none of the hardening on this branch altered rendering. As a negative control, giving the candidate EXPORT_DEFAULT_HEIGHT=500 is caught on every case that relies on the default height and correctly reported as unchanged for the one variant that passes an explicit size, with differences named down to plotBackground.height and the PNG content box. The file header records how to set up the frozen reference, including sharing .cache so that Highcharts is held constant and only the browser stack varies, and is explicit that comparing browser versions needs an environment where each server uses its own Puppeteer's browser rather than one shared system install.
Declares `^22.22.2 || ^24.15.0 || >=26.0.0`, up from `>=18.12.0`. Node.js 18 and 20 have both reached end of life, and this range is what the dependency versions coming in the following commits support - the bump has to happen first so that it can be verified on its own rather than mixed in with them. No dependency changes here, deliberately. Node 18 to 24 is several years of V8 and a different garbage collector, so it gets a clean before and after with nothing else moving. The odd-numbered Node.js 25 line is excluded because jsdom does not support it, so declaring it would promise something untested. CI now runs unit tests against both supported lines rather than a single version, since that is where runtime behaviour can differ. Lint and the build job run on 24 only - neither varies by runtime, and the build job commits dist, which a matrix would have done more than once. Verified on Node 22.22.3 as well as 24.15.0: 57 unit tests pass on both, all four export types return images, load holds at 18.83 req/s and p50 201ms against 18.96 and 200ms on Node 24, and the saturation gate passes. The render comparison reports all 52 cases identical between this branch on Node 22 and frozen master on Node 24, so neither the branch nor the Node version changes layout.
A rasterization timeout marked its worker for recycling by setting the page reference to null. But factory.destroy only closes a page it can still see, so nulling the reference meant the page was never closed. With a process per tab, every timeout left a renderer process behind for the lifetime of the browser - and those processes then took CPU from the exports still running, so more of them timed out. A slow patch of traffic could turn itself into a sustained one. The worker now carries a flag instead. Validation fails on it, so the pool still recycles the worker immediately, and destroy sees a real page and closes it. Measured on a load that produces timeouts: browser processes went from 119 after a twenty second run to 12, with the pool ending in the same state either way. This is a pre-existing bug rather than anything to do with an upgrade. It was found while investigating a separate slowdown, which made timeouts frequent enough for the leak to become obvious - at the previous rate of timeouts it would leak just as reliably, only slower.
jsdom is used in one place, sanitizing incoming SVGs, so the surface across six major versions is small. Jest had to move with it: jsdom 30 depends on html-encoding-sniffer, which now requires an ES module that Jest 29 cannot load, and the sanitize suite failed to run at all under the old version. The product itself was unaffected - sanitizing worked correctly the whole time, including stripping xlink:href - it was only the test harness that could not load it. Verified: 57 unit tests pass on Jest 30, sanitizing still strips scripts, event handler attributes and xlink:href, and the SVG input path works end to end through a running server. Load holds at 18.4 req/s and p50 200ms, the saturation gate passes, page isolation and browser recovery pass, and the render comparison reports all 52 cases identical against frozen master. Puppeteer is deliberately not updated here. See the next commit note.
Verified in a container where each version uses the browser it ships with, which is what made the difference. Measured against Puppeteer 22.15 in the same container, concurrency 4, PNG: puppeteer 22.15 + bundled Chrome 127 28.25 req/s, p50 134ms, 100%, 0 timeouts puppeteer 25.4 + bundled Chrome 151 27.16 req/s, p50 134ms, 100%, 0 timeouts Layout is unchanged. The render comparison reports all 52 cases identical between the two, so neither the Puppeteer bump nor the Chrome bump underneath it moves margins, plot area or overall size. Saturation gate passes at 27.13 exports/s goodput, page isolation passes, all four export types return images, and 57 unit tests pass. captureBeyondViewport and waitForInitialPage both still exist and are still documented in 25, so neither needed changing. An earlier assumption that captureBeyondViewport had been removed was wrong. Worth recording why this looked like a blocker first time round. Driven against an unrelated, older system browser rather than its own, Puppeteer 23 and later collapse to about 3 req/s with roughly half of exports failing on rasterization timeouts - and 23, 24 and 25 all behave the same way, so it looked like a genuine regression rather than a mismatch. Given its own browser, 25 performs as 22 does. The lesson for future upgrades is that measuring Puppeteer against a browser it did not ship with says nothing useful about it.
Checked against each of the changes that could have bitten, rather than
assuming. No code needed altering:
- Route matching moved to path-to-regexp v8. Every route here is literal or
a single named parameter - /, /health, /:filename,
/version/change/:newVersion - so none of the wildcard syntax that changed
is in use.
- express.urlencoded's extended default flipped, but it is passed explicitly.
- request.query became a getter; nothing assigns to it.
- None of the APIs dropped in 5 are used: no res.send(status, body), no
res.json(body, status), no req.param(), no app.del().
- express-rate-limit already declares support for Express 5, so it did not
need to move with this.
Async handler rejections now forward to next() automatically, which changes
the error path - the export handler throws from inside the startExport
callback and relied on its own try/catch. Verified end to end in a container:
an empty body and a body with no chart data both return 400 with
EXPORT_INVALID_REQUEST, malformed JSON returns 400, and a body over the upload
limit returns 413 rather than anything in the 5xx range. Under saturation, 80
of 80 sampled refusals returned 400 with EXPORT_QUEUE_FULL and none returned a
5xx, so the capacity middleware's next(error) path still lands where intended.
Also verified: all four export types, the /:filename route with its
Content-Disposition, multipart and urlencoded form bodies, /health, the UI
route, 57 unit tests, and a SIGTERM mid-load draining 332 requests with none
dropped. Load is 27.28 req/s at p50 134ms against 28.25 before. The render
comparison reports all 52 cases identical against frozen master.
Grouped because each has a small, checked surface: uuid is two v4() calls, https-proxy-agent is one constructor, dotenv is one config() call, and cors, dompurify and tarn are patch or minor moves. dotenv 17 needed a code change. From that version it prints a summary of what it loaded to stdout on every start, which is noise for anything parsing this server's log output, so quiet: true now suppresses it. Verified the banner is gone both directly and in a server's own log. uuid 14 and https-proxy-agent 9 also needed an eslint settings change, which is worth explaining. Both now declare their entry points through a package.json "exports" map with conditional keys, and the resolver eslint-plugin-import uses does not implement "exports" - so import/no-unresolved reports them as unresolvable even though Node resolves them, they work at runtime, and the tests pass. Left alone this fails the lint job rather than just being untidy. Bumping eslint-plugin-import to 2.32.0 does not help; it is the resolver, not the plugin. The purpose-built resolver for this is still at 1.0.0-beta, which is not worth taking on for a lint detail. Naming the two packages in import/core-modules keeps the rule working everywhere else, which is better than switching it off. Verified with the same invocation CI uses: no errors. Verified uuid 14 still produces valid v4 UUIDs through the same import, and https-proxy-agent 9 still constructs through the same named export. Gate in a container: 57 unit tests, load 27.15 req/s at p50 134ms, saturation gate passes at 27.17 exports/s goodput, page isolation passes, all four export types return images, and the render comparison reports all 52 cases identical against frozen master.
Closes the advisories against the 1.4.5 line, which is the reason for moving. Only upload.none() is used here, for non-file multipart fields, so the surface is small - but the error paths are the part worth checking, since Multer's errors reach the client and this server must not answer 5xx. Verified against a running server: a simple multipart field exports, a multipart svg field exports both to png and as sanitized svg passthrough, a file upload is refused with 400 "Unexpected field" as upload.none() should, and a field over the upload limit is refused with 400 "Field value too long". JSON bodies are unaffected. The 5xx clamp was not triggered once, so no Multer error carries a status outside the contract. Gate: 57 unit tests, load 27 req/s at p50 134ms, saturation gate passes, and the render comparison reports all 52 cases identical against frozen master.
zod 4 no longer honours the zod 3 form for customising a refine message - a
function returning { message } - and silently falls back to "Invalid input".
Validation still rejected everything it should, and all 57 unit tests passed,
so nothing failed: every explanation of *why* a value was rejected had simply
gone. For a server whose startup gate is environment validation that is a real
loss, since a bad value in a deployment would give an operator nothing to work
from.
All seven refine calls now use zod 4's params object, taking `error` as a
function of the issue where the message includes the offending value. Verified
each one individually:
POOL_MAX_WORKERS=not-a-number The value must be numeric and non-negative, received 'not-a-number'
POOL_WORK_LIMIT=-3 The value must be numeric and positive, received '-3'
SERVER_HOST=undefined The string contains forbidden values, received 'undefined'
HIGHCHARTS_VERSION=not.a.version must be 'latest', a major version, or in the form XX.YY.ZZ
HIGHCHARTS_CDN_URL=ftp://... should start with http:// or https://
LOGGING_LEVEL=9 only accept values from 0 to 5 as logging levels
PUPPETEER_TEMP_DIR=!!!bad!!! The string is an invalid path directory string.
The last of those had never worked. Its refine was called with an empty object
where the message belongs and the message as a third argument, which zod 3
ignored, so an invalid temporary directory was rejected with a generic error
instead of the explanation written for it. Migrating to the new form fixed it
as a side effect.
Also verified that a bad value still stops startup rather than being accepted,
and valid values are unaffected. Gate: 57 unit tests, load 27.17 req/s at p50
134ms, saturation gate passes, render comparison reports all 52 cases identical.
v8 still accepts the options this passed, but two of them were wrong to keep. `max` was renamed `limit` in v7, and `delayMs` has done nothing since v7 - it was being passed a value from a documented config option, `delay`, which therefore promised behaviour that had already stopped existing. Since this is a major release, the option is removed rather than left in place misleading anyone who sets it. Verified live with the limiter actually enabled at five requests a minute: requests one through five return 200, six onwards return 429, and the response body is unchanged. No deprecation warnings, and none of v8's validation complaints about IPv6 key generation appeared. Gate: 57 unit tests, load 27.15 req/s at p50 134ms, saturation gate passes, page isolation passes, and the render comparison reports all 52 cases identical against frozen master. With this, every runtime dependency except Highcharts is current, and `npm audit --omit=dev` reports no vulnerabilities where 5.1.0 had eleven.
This dependency only decides what a useNpm deployment loads. Everywhere else the runtime version comes from HIGHCHARTS_VERSION, which defaults to latest and is fetched from the CDN, so most deployments were already on 13 before this commit - the bump aligns useNpm with them rather than changing anything for them. All 67 core, module and indicator scripts this server asks for are present in 13, so nothing was renamed or dropped out from under the configuration. Verified useNpm mode end to end: the cache builds from the package, /health reports 13.0.0, png, svg and pdf all export, and a stockChart exports, which exercises a module rather than just the core. The render comparison did find a difference, which is the first time it has, and it is worth recording precisely. Comparing 12.5.0 against 13.0.0, 50 of 52 cases are identical. The two that differ are both the rotated x axis label scenario, at 1x and 2x: 12.5.0 drawn content spans x=0 to x=590 in a 600px image 13.0.0 drawn content spans x=14 to x=590 The right extent is identical and the SVG plot background geometry is identical, so the chart layout itself did not move. Only the left extent of what is drawn changed. Content reaching column 0 means something was being drawn at the very edge of the image, which should not happen at default spacing, so the most likely reading is that a rotated label overflowed the left edge in 12.5.0 and is contained in 13.0.0 - an improvement rather than a regression. It is also a change that has already reached anyone fetching Highcharts from the CDN.
The changelog entries were several times longer than the rest of the file -
averaging 63 words against 14 to 18 in previous releases. Rewritten to match,
now averaging 27, with the longest at 50 against an existing 60. Rationale and
measurements that belong in commit messages have been dropped; what a reader
needs to know has not.
README:
- Adds a v5.x.x to v6.x.x upgrade note alongside the existing ones.
- Documents queueLimit, queueRejectDelay, keepAliveTimeout,
shutdownDrainTimeout and launchRetryWindow in all three places they belong:
the default JSON config, the environment variables and the command line
arguments. Verified no option in the schema is left undocumented.
- Removes the delay/SERVER_RATE_LIMITING_DELAY option, which no longer exists.
- Adds an Error Responses section covering the response shape and each
errorCode, and what each means for a caller.
- Notes the new /health fields on the endpoint description.
- Extends the worker count note to cover queueing, and corrects the reference
to tests/other/stress-test.js, which is not a file that exists, pointing at
load_test.js instead.
One deliberate difference from the existing upgrade notes: their bullets are
indented four spaces, which CommonMark reads as an indented code block rather
than a list, so the backticks in them render literally. The new section is not
indented so that it renders as intended. The existing sections are left as they
are rather than reformatted here.
All within their existing major versions. Checked for the two things that could have caused trouble: Prettier 3.3.2 to 3.9.6 reformats nothing. A minor bump can change formatting across a whole codebase, so this was verified in a scratch copy before being applied. The files prettier still reports are the same ones it reported before - workflow files, an issue template, two test fixtures, and two that are deliberately not valid JSON, one of them a fixture for invalid input. Rollup 4.26.0 to 4.62.3 still builds. Verified in a scratch copy rather than here, so as not to overwrite the uncommitted dist files in the working tree: the build completes and both the ESM and CJS bundles load with the expected exports. Lint, formatting and 57 unit tests unchanged.
A major, and it decides what the published bundle looks like, so verified rather than assumed. Its Node requirement of >=20 is satisfied by the baseline raised earlier in this release. Built in a scratch copy so as not to overwrite the uncommitted dist files in the working tree. The build completes, both bundles load, output is still minified, and the exports are byte-for-byte the same shape as the 0.4.4 build: one top-level export, default, carrying the same 18 keys. That single top-level export is correct - lib/index.js only has export default - not something this change introduced.
Its Node requirement of >=22.22.1 is satisfied by the baseline raised earlier in this release. Removes .lintstagedrc, which has never worked. It contains `\*` inside a JSON string, which is not a valid escape, so lint-staged fails to parse it and falls back to the configuration in package.json - printing a parse error each time it runs. The file has been in the repository since the Puppeteer refactor. The package.json configuration is both valid and broader, covering all JavaScript rather than only lib, so nothing is lost beyond running prettier over JSON files in lib, which was never happening anyway. Verified lint-staged now loads its configuration without an error, and still runs to completion over a staged file. Worth noting separately: nothing invokes lint-staged. The husky pre-commit hook runs `npm run lint` and `npm run unit:test` directly, over the whole project rather than staged files. So this dependency is currently unused, and the choice is either to wire it into the hook or to drop it - left alone here, as that is a change to how the project is worked on rather than a dependency update.
Replaces .eslintrc.cjs with eslint.config.js, preserving the same behaviour: the recommended sets from eslint, eslint-plugin-import and eslint-plugin-prettier, no-unused-vars off, import/no-cycle as an error, prettier/prettier with the platform line ending, the import/core-modules entry added earlier for uuid and https-proxy-agent, and jest globals for test files. Verified by printing the resolved config rather than by inspection. eslint 10 is not possible yet. eslint-plugin-import declares support only up to eslint 9, so 10 would mean also replacing it with the import-x fork - a change to the linting itself rather than a version bump, and better done deliberately. The `env` blocks become `globals`, which is why the `globals` package is now a dependency. Flat config also has no .eslintignore, so the ignore list moved into the config. eslint 9 reports unused eslint-disable directives by default, where 8 did not. That surfaced nine dead directives, all suppressing no-undef for browser globals inside page.evaluate callbacks, which the browser globals already cover and have therefore never been doing anything. Removed by --fix, and the blank lines it left behind were rejoined by hand so the comments still sit against the statements they describe. The resulting diff in lib is nine deleted comment lines and nothing else. Dropped the --ext flag from the lint workflow, as flat config ignores it. Verified: lint clean with the same two pre-existing warnings, formatting unchanged, 57 unit tests pass, and the server still starts and exports png, svg and pdf at 26.47 req/s.
Flat config does not read it, and eslint warned about it on every run. Its two entries are already covered: dist/** is in the config's ignores, and eslint 9 ignores node_modules by default. Verified the same 65 files are linted before and after, with nothing from dist or node_modules in either set.
…ardening-6.0.0 # Conflicts: # package-lock.json
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.
No description provided.