feat(webdriver): bound memory per render request - #23
Conversation
Peak memory is now the product of a concurrency cap and a per-request allocation, so a caller holding a slot keeps its full budget no matter how many others are active. Excess callers queue rather than being rejected: shedding would make the outcome depend on what else a node happened to be running. Both new limits report through Resulting-Status, which callers treat as recoverable, rather than failing at the transport level. The oversized-response check runs inside the page because measuring after the transfer would already have paid the memory cost it exists to prevent. Also adds a missing await in renderPage: the browser refcount was released before the render finished, so rotation could close a browser out from under an in-flight page.
|
👋 This PR targeted
|
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe WebDriver project adds a FIFO semaphore for render concurrency, queue timeouts, abort handling, bounded text/HTML/screenshot extraction, and status handling for unavailable or oversized responses. It also adds semaphore tests and npm scripts for testing and type checking. ChangesWebDriver render controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RenderRequest
participant renderSemaphore
participant Browser
participant PageExtraction
RenderRequest->>renderSemaphore: acquire render permit
renderSemaphore-->>RenderRequest: permit or queue failure
RenderRequest->>Browser: render page
Browser->>PageExtraction: extract bounded content
PageExtraction-->>RenderRequest: content or ResponseLimitExceeded
RenderRequest->>renderSemaphore: release render permit
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GenVM PR actionsTick a box to run it (the box unticks itself when handled). Actions only run while the PR has the
MergeRequires, on the exact head commit:
Full CI runs only when Every repo lands ONE squashed commit, subject Commands
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webdriver/src/prj/src/index.ts`:
- Around line 53-54: Validate the value returned by envInt for
GVM_WEBDRIVER_MAX_RESPONSE_MB during startup, requiring a positive integer
before calculating MAX_RESPONSE_CHARS. Reject zero, negative, and non-integer
values with an error message that explicitly names
GVM_WEBDRIVER_MAX_RESPONSE_MB.
In `@webdriver/src/prj/src/semaphore.ts`:
- Around line 15-19: Add a configurable maximum queue depth to the semaphore
used by renderPage, reject new waiters once that limit is reached so the HTTP
handler returns 503, and preserve existing enqueue behavior below the limit.
Track each queued request’s close event and remove its waiter and timeout when
the request closes, updating the waiter bookkeeping and cleanup logic
consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 960299a1-d212-4196-b10d-364ec720a203
📒 Files selected for processing (4)
webdriver/src/prj/package.jsonwebdriver/src/prj/src/index.tswebdriver/src/prj/src/semaphore.test.tswebdriver/src/prj/src/semaphore.ts
An unbounded queue only defers overload: callers pile up holding live connections until each trips its own transport timeout. Refusing past a configured depth fails fast through the same recoverable status, and a caller that disconnects now frees its slot instead of being handed a permit for a render nobody will read. Also validates the response and concurrency limits at startup, since a zero or negative value silently rejected every page.
|
Addressed both review findings. Queue depth — you're right, and it made me re-examine my own reasoning in the PR body. I'd argued for "queue, don't shed" on the grounds that shedding makes the outcome depend on node-local load. But the 120 s timeout already has exactly that property, so a depth cap adds no new class of problem — it just makes the failure explicit and fast instead of making 64 callers each wait two minutes and then fail anyway. Added Abandoned waiters — also added. Validation — added Tests: 17 unit tests now, up from 10 — new coverage for queue-full refusal, fast-fail rather than waiting out the timeout, queue reacceptance after drain, abort-while-queued, already-aborted signal, and abort-after-permit-granted being a no-op. Re-verified end to end against real Chrome: oversized page still returns 507 with the payload withheld, queueing still observed under saturation, and a mid-flight client disconnect leaves the server healthy. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webdriver/src/prj/src/duration.ts`:
- Around line 75-80: Update envPositiveInt to validate the raw environment value
before conversion, using Number(raw) rather than envInt so fractional and
trailing-character inputs are rejected. Require the resulting value to be a safe
positive integer, while preserving default handling for an unset variable and
the existing error behavior.
In `@webdriver/src/prj/src/index.ts`:
- Around line 454-464: The disconnect detection in disconnectSignal should
listen once to res.close instead of req.close while preserving the
!res.writableEnded guard. Update handleRenderRequest and handleHealthcheck to
call disconnectSignal with only res, and adjust its signature accordingly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 714dbca5-2482-42f8-a320-d187040d059c
📒 Files selected for processing (4)
webdriver/src/prj/src/duration.tswebdriver/src/prj/src/index.tswebdriver/src/prj/src/semaphore.test.tswebdriver/src/prj/src/semaphore.ts
| export function envPositiveInt(envName: string, defaultValue: number): number { | ||
| const value = envInt(envName, defaultValue); | ||
| if (!Number.isInteger(value) || value < 1) { | ||
| throw new Error( | ||
| `env ${envName} must be a positive integer, got "${process.env[envName]}"`, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked candidate file:\n'
git ls-files | grep -F 'webdriver/src/prj/src/duration.ts' || true
printf '\nRelevant duration.ts outline and target lines:\n'
if [ -f webdriver/src/prj/src/duration.ts ]; then
ast-grep outline webdriver/src/prj/src/duration.ts || true
printf '\nLines 1-130:\n'
sed -n '1,130p' webdriver/src/prj/src/duration.ts | nl -ba
fi
printf '\nInspect parseInt behavior used by this envInt implementation:\n'
node - <<'JS'
for (const raw of ['4.5', '4invalid', '4invalid.5', '0.5', '3', '3.14159']) {
console.log(`${JSON.stringify(raw)} -> parseInt 10=${parseInt(raw, 10)}, Number=${Number(raw)}, isInteger=${Number.isInteger(Number(raw))}, safe=${Number.isSafeInteger(Number(raw))}`);
}
JS
printf '\nSearch for envPositiveInt/envInt usages:\n'
rg -n "envPositiveInt|envInt" webdriver/src/prj/src -S || trueRepository: genlayerlabs/genvm-manager
Length of output: 1056
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Lines 1-120 of webdriver/src/prj/src/duration.ts:\n'
sed -n '1,120p' webdriver/src/prj/src/duration.ts | awk '{printf "%6d\t%s\n", NR, $0}'
printf '\nInspect parseInt behavior used by this envInt implementation:\n'
node - <<'JS'
for (const raw of ['4.5', '4invalid', '4invalid.5', '0.5', '3', '3.14159']) {
console.log(`${JSON.stringify(raw)} -> parseInt 10=${parseInt(raw, 10)}, Number=${Number(raw)}, isInteger=${Number.isInteger(Number(raw))}, safe=${Number.isSafeInteger(Number(raw))}`);
}
JS
printf '\nSearch for envPositiveInt/envInt usages:\n'
rg -n "envPositiveInt|envInt" webdriver/src/prj/src -S || trueRepository: genlayerlabs/genvm-manager
Length of output: 6437
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const cases = [
'4.5',
'4invalid',
'4invalid.5',
'0.5',
'3',
'3.14159',
'4.5invalid',
'9007199254740991',
'9007199254740992',
];
for (const raw of cases) {
const parseInt10 = parseInt(raw, 10);
const numberVal = Number(raw);
console.log(`${raw}: parseInt10=${parseInt10}, Number=${numberVal}, isInteger=${Number.isInteger(numberVal)}, safe=${Number.isSafeInteger(numberVal)}`);
}
JSRepository: genlayerlabs/genvm-manager
Length of output: 795
Validate the raw environment value before integer conversion.
envInt delegates to parseInt, so inputs like 4.5, 4invalid, and 4.5invalid accept only the integer prefix and use 4. Since envPositiveInt checks the parsed value, fractional or trailing-character configs bypass the intended fail-fast behavior for response and concurrency limits. Parse raw input strictly with Number(raw) and fail unless the value is a safe positive integer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webdriver/src/prj/src/duration.ts` around lines 75 - 80, Update
envPositiveInt to validate the raw environment value before conversion, using
Number(raw) rather than envInt so fractional and trailing-character inputs are
rejected. Require the resulting value to be a safe positive integer, while
preserving default handling for an unset variable and the existing error
behavior.
| function disconnectSignal( | ||
| req: http.IncomingMessage, | ||
| res: http.ServerResponse, | ||
| ): AbortSignal { | ||
| const controller = new AbortController(); | ||
| req.on('close', () => { | ||
| if (!res.writableEnded) { | ||
| controller.abort(); | ||
| } | ||
| }); | ||
| return controller.signal; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | fgrep 'webdriver/src/prj/src/index.ts' || true
echo "== relevant lines =="
if [ -f webdriver/src/prj/src/index.ts ]; then
nl -ba webdriver/src/prj/src/index.ts | sed -n '420,500p'
fi
echo "== usages of disconnectSignal =="
rg -n "disconnectSignal|ServerResponse|IncomingMessage|writableEnded|close" webdriver/src/prj/src/index.ts || true
echo "== package/runtime hints =="
for f in package.json webdriver/src/prj/package.json; do
[ -f "$f" ] && { echo "--- $f"; cat "$f"; }
doneRepository: genlayerlabs/genvm-manager
Length of output: 274
🌐 Web query:
Node.js http IncomingMessage response events 'close' request completed documentation
💡 Result:
In Node.js, the 'close' event on an http.IncomingMessage object indicates that the request has either been completed or that its underlying connection was terminated prematurely before completion [1][2][3]. Starting in Node.js v16.0.0, the behavior of the 'close' event was changed [4][5]. Before this version, 'close' typically signaled that the underlying socket connection had closed [6][5]. As of v16.0.0 and in all current versions, 'close' is emitted when the request is considered completed—meaning it is no longer strictly tied to the state of the underlying socket [4][5]. Key points regarding the 'close' event for IncomingMessage: 1. Meaning: The event indicates successful completion of the message reception or, alternatively, that the connection was terminated prematurely (e.g., client disconnection or network failure) before the response was fully processed [1][2][3]. 2. Version History: In Node.js v16.0.0 and later, the 'close' event behavior was updated to emit upon request completion rather than underlying socket closure [4][5]. This was a breaking change [6]. 3. Relation to Other Events: Because the 'close' event is a standard stream event, it can sometimes be emitted after other stream events like 'end' [7]. Developers should be aware that 'close' does not guarantee the request was successful; it only signals that no further data will be emitted for that specific request [1][8]. For logic requiring confirmation of successful data receipt, ensure you are also handling the 'end' event and checking for error conditions, as 'close' may trigger regardless of whether the request finished normally or was interrupted [1][3].
Citations:
- 1: https://nodejs.org/api/http.html
- 2: https://nodejs.org/download/release/latest-v24.x/docs/api/http.html
- 3: https://nodejs.org/dist/latest-v24.x/docs/api/http.html
- 4: https://nodejs.org/docs/latest-v16.x/api/http.html
- 5: nodejs/node@b3a7a689f9
- 6: Undocumented breaking change on v16.0.0 nodejs/node#38924
- 7: http: IncomingMessage emits 'end' after 'close' nodejs/node#29295
- 8: https://stackoverflow.com/questions/54526179/node-js-difference-between-http-finish-event-response-close-event-and-respons
🌐 Web query:
Node.js http ServerResponse close event finished premature connection termination documentation
💡 Result:
In Node.js, the close event on an http.ServerResponse instance indicates that the response has either completed or that its underlying connection was terminated prematurely before the response could be finished [1][2]. Key technical details regarding this behavior include: Event Purpose and Distinction: The close event serves as a general indicator that the response object has reached its final state [3]. Because it is emitted in both successful completion and premature termination scenarios, it is distinct from the finish event [3]. The finish event specifically signals that the response has been fully flushed to the underlying system for transmission [3][4]. Detecting Premature Termination: To differentiate between a successful completion and a premature termination, you can check the status of the response object when the close event fires [5][3]. A reliable way to do this is by checking the finished property [5]: res.on('close', => { if (!res.finished) { // The connection was terminated prematurely // Perform cleanup or logging here } }); Lifecycle Nuances: 1. Finish vs. Close: The finish event records that Node.js has handed the final bytes of the response to the underlying system [3]. It does not guarantee that the client has received those bytes [3]. 2. Error Handling: Relying solely on the close event for error handling can be complex, as historical behavior and ongoing discussions in the Node.js project have highlighted that the response object does not always emit an error event upon premature closure [6][7]. Checking res.finished is the standard approach to identify these clean aborts [5][6]. Documentation Note: Historically, the documentation for the close event underwent clarifications to distinguish its behavior from the finish event, ensuring developers understand that close is emitted regardless of whether the response finished successfully or was cut short [8][9][5]. Always refer to the specific version of the official Node.js HTTP API documentation for your project, as minor behavioral adjustments regarding event emission and error handling are occasionally introduced [1][10][11][12].
Citations:
- 1: https://nodejs.org/api/http.html
- 2: https://github.com/nodejs/node/blob/main/doc/api/http.md
- 3: https://www.thenodebook.com/http/http-server-lifecycle
- 4: https://stackoverflow.com/questions/54526179/node-js-difference-between-http-finish-event-response-close-event-and-respons
- 5: doc: fix HTTP res 'finish' description nodejs/node#21670
- 6: http: emit error on aborted ServerResponse nodejs/node#63448
- 7: http: response does not emit error on premature close nodejs/node#28172
- 8: doc: http.ServerResponse close event nodejs/node#30489
- 9: doc: document the 'close' and 'finish' events nodejs/node#42704
- 10: https://nodejs.org/docs/latest-v24.x/api/http.html
- 11: https://nodejs.org/docs/latest-v16.x/api/http.html
- 12: https://nodejs.org/download/release/v22.13.1/docs/api/http.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant file section =="
awk '{printf "%6d\t%s\n", NR, $0}' webdriver/src/prj/src/index.ts | sed -n '428,480p'
echo "== disconnectSignal call sites =="
python3 - <<'PY'
from pathlib import Path
p=Path('webdriver/src/prj/src/index.ts')
text=p.read_text()
for i,line in enumerate(text.splitlines(),1):
print(f"{i}: {line}") if 'disconnectSignal' in line or 'disconnectSignal(' in line else None
PY
echo "== queue/semaphore-related calls around queued requests =="
python3 - <<'PY'
from pathlib import Path
p=Path('webdriver/src/prj/src/index.ts')
for term in ['semaphore', 'permit', 'queue', 'AbortSignal', 'render', 'healthcheck', 'disconnectSignal']:
print(f"\n--- {term} ---")
text=p.read_text()
lines=text.splitlines()
for i,line in enumerate(lines,1):
if term.lower() in line.lower():
start=max(1,i-3); end=min(len(lines),i+3)
for n in range(start,end+1):
print(f"{n}: {lines[n-1]}")
print()
PY
echo "== declared node version hints =="
python3 - <<'PY'
from pathlib import Path
for p in [Path('package.json'), Path('webdriver/src/prj/package.json'), Path('package-lock.json'), Path('webdriver/src/prj/package-lock.json'), Path('yarn.lock'), Path('pnpm-lock.yaml')]:
if p.exists():
text=p.read_text(errors='ignore')
vals = []
if p.name != 'package-lock.json' and p.name != 'webdriver/src/prj/package-lock.json' and p.name != 'pnpm-lock.yaml':
vals += [t.strip() for t in text.split() if 'node' in t.lower() and ('version' in t.lower() or 'engines' in t.lower())]
else:
vals += [t.strip() for t in text.split() if 'node@' in t or 'engines:' in t or p.name == 'yarn.lock' and ('node' in t or p.name == 'package-lock.json')]
print(f"{p}: {' '.join(vals[:50]) if vals else '<no node version hints>'}")
PYRepository: genlayerlabs/genvm-manager
Length of output: 22919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all visible node version strings =="
python3 - <<'PY'
from pathlib import Path
terms = ['.node-version', 'Dockerfile', 'renovate.json', 'mise.toml', '.tool-versions', '.github/workflows']
files = []
for p in Path('.').glob('**/*'):
if not p.is_file() or '__pycache__' in p.parts or 'node_modules' in p.parts or '.git' in p.parts:
continue
if any(x in str(p) for x in terms) or p.name in ['package.json', 'package-lock.json']:
files.append(p)
for p in files:
text = p.read_text(errors='ignore')
vals = [l.strip() for l in text.splitlines() if any(s in l.lower() for s in ['node', '18', 'lts'] ) and (s not in p.name or 'engines' in l.lower())][:3]
if vals:
print(f'--- {p} ---')
print(vals)
PY
echo "== semaphore acquire implementation =="
fd -a 'semaphore(\.ts|\.js)$' . | head -20
while IFS= read -r f; do
echo "--- $f ---"
wc -l "$f"
cat -n "$f" | sed -n '1,220p'
done < <(fd -a 'semaphore(\.ts|\.js)$' .)
echo "== exact response close writableEnded / finished docs behavior from Node v18 docs =="
python3 - <<'PY'
import urllib.request
url = "https://registry.npmjs.org/@types/node/-/node-18.20.0.tgz"
tarball = urllib.request.urlopen(url, timeout=20).read()
print(f"tarball_len={len(tarball)}")
import tarfile, io, re
tf = tarfile.open(fileobj=io.BytesIO(tarball), mode='r:gz')
members = [m.name for m in tf.getmembers() if 'http.d.ts' in m.name.lower()]
print("http.d.ts candidates:", members)
for m in members:
text=tf.extractfile(m).read().decode('utf-8','replace')
hits=[line for line in text.splitlines() if "close" in line and ("ServerResponse" in line or "IncomingMessage" in line) or "writableEnded" in line or "finished" in line]
# Print around HTTP response interfaces
lines=text.splitlines()
for i,line in enumerate(lines[:2000],1):
if "interface Server extends" in line or "interface ServerResponse" in line or "interface IncomingMessage" in line:
print(f"\n-- {m}:{i} --")
for j in range(max(1,i-120), min(len(lines),i+220)+1):
if "close" in lines[j-1] or "writableEnded" in lines[j-1] or "finished" in lines[j-1]:
print(f"{j}: {lines[j-1]}")
PYRepository: genlayerlabs/genvm-manager
Length of output: 353
🏁 Script executed:
#!/bin/bash
set -u
echo "== likely node version config files =="
git ls-files | grepE '^(Dockerfile|\.node-version|\.tool-versions|\.nvmrc|renovate\.json|mise\.toml|package\.json|\.github/workflows/)' | sort || true
echo "== node version references =="
python3 - <<'PY'
from pathlib import Path
terms = ['node', 'node-version', 'engines', 'lts']
for p in Path('.').glob('*'):
if p.name in terms or p.name == 'package.json' or p.name == '.nvmrc':
print(f'--- {p} ---')
print(p.read_text(errors='ignore')[:3000])
PY
echo "== semaphore implementation files =="
fd -a 'semaphore(\.ts|\.js)$' . || true
while IFS= read -r f; do
[ -z "$f" ] && continue
echo "--- $f ---"
wc -l "$f"
cat -n "$f" | sed -n '1,240p'
done < <(fd -a 'semaphore(\.ts|\.js)$' .)
echo "== Node TypeScript typings v18 close/event signatures =="
python3 - <<'PY'
import urllib.request
url = "https://registry.npmjs.org/@types/node/-/node-18.20.0.tgz"
try:
data = urllib.request.urlopen(url, timeout=20).read()
except Exception as e:
print(f"FAIL downloading typings: {e}")
raise SystemExit
print(f"tarball_len={len(data)}")
import tarfile, io
tf = tarfile.open(fileobj=io.BytesIO(data), mode='r:gz')
for m in tf.getmembers():
if 'http.d.ts' in m.name.lower():
text = tf.extractfile(m).read().decode('utf-8', 'replace')
print(f"\n--- {m.name} ---")
for i,l in enumerate(text.splitlines(),1):
if 'IncomingMessage' in l or 'ServerResponse' in l or l.strip().startswith('listen('):
start=max(1,i-80); end=min(len(text.splitlines()),i+100)
print(f"\n---- around line {i} ----")
for j in range(start,end+1):
if 'close' in text.splitlines()[j-1] or 'writableEnded' in text.splitlines()[j-1] or 'finished' in text.splitlines()[j-1]:
print(f"{j}: {text.splitlines()[j-1]}")
PYRepository: genlayerlabs/genvm-manager
Length of output: 634
Use the response’s close event for premature disconnects.
IncomingMessage emits close in Node 18+ when the request is deemed complete, so a normal queued request can cancel renderPage while res.writableEnded is still false. Listen on res.once('close') and keep the existing !res.writableEnded guard, then update both handleRenderRequest and handleHealthcheck callers to satisfy the handler with only res.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webdriver/src/prj/src/index.ts` around lines 454 - 464, The disconnect
detection in disconnectSignal should listen once to res.close instead of
req.close while preserving the !res.writableEnded guard. Update
handleRenderRequest and handleHealthcheck to call disconnectSignal with only
res, and adjust its signature accordingly.
How loaded a host happens to be is not a property of the page, so it must not reach the contract. Reporting saturation as a render result let two hosts under different load answer the same request differently, with both answers looking legitimate; a transport failure instead says plainly that this host could not run it. The page-size and heap limits keep reporting as render results, since any host with the same configuration reaches the same verdict on them. That does mean those two must be configured identically across hosts.
|
Pushed a correction that reverses a decision I defended earlier in this PR. Prompted by #21 and Kira's question on it: I had routed both new failure classes through
Saturation is host-local. Making it contract-visible means a busy host and an idle host return different answers for the same request, and both look legitimate — silent divergence, strictly worse than an honest "this host could not run it". The Lua's existing The irony is that CodeRabbit pushed me toward a bounded queue, I defended the non-fatal channel as "the part worth preserving", and that was the part that was wrong. The bounded queue was right; the channel wasn't.
Verified against real Chrome under two configs, since queueing and shedding need opposite settings: 17/17 unit tests and Follow-up worth its own issue: |
`newPage` ran before the try whose finally disposes of the context, so a failure there stranded the context for the shared browser's whole lifetime. Page creation fails precisely when the browser is under pressure, which made the response to pressure be to leak more of it.
The test is not "the page is big" but "the page is bigger than this host's limit", and that threshold is local configuration. Reporting it as a render result let hosts with different settings answer the same request differently, each answer looking legitimate, and handed anyone who knew the spread a way to craft a page landing in the gap between them. This also corrects the pre-existing heap limit, which reported the same way. Both can become contract-visible once their values are agreed protocol constants rather than per-host settings.
|
Correcting the previous comment — the split I described was still wrong, in the same way. I claimed the size caps are "properties of the page, so any host with the same configuration reaches the same verdict". The flaw is that the test isn't "the page is big", it's "the page is bigger than this host's limit" — and that threshold is local configuration. The qualifier "with the same configuration" was carrying the whole argument while nothing enforces it. That makes it an attack, not just a hazard: someone who knows the config spread across validators crafts a page sized into the gap and gets them to return different results, each individually legitimate. It also contradicts the standard applied to storage, where the page cap is a compile-time Everything now leaves through the transport channel:
The only thing that stays contract-visible is what the remote site did, which is the one category that is genuinely an observation about the world. This corrects pre-existing behaviour, not just my additions. The heap limit already reported as The actual fix, for later: make both values agreed protocol constants instead of per-host env vars, at which point they can legitimately become contract-visible. Until then, fatal is the safe failure mode. Note the residual — a config mismatch still causes divergence, just as timeout-vs-result rather than result-vs-result. Much weaker, not nothing. Uniformity is what actually closes it. Verified: oversized page now returns a real HTTP 507 with no |
|
I am going to integrate it into #24 (1fc7576). I can not merge it here because:
For this reason, I am closing it in favor of that (well, consider it merging). Thanks for improving the webdriver! |
2 participants
Why
The webdriver has no bound on the memory one render can cost, and no bound on how many renders run at once. Both are needed before peak RAM per genvm is a number anyone can state.
Two gaps specifically:
asText/asHTMLpull the entire page into the Node process.page.evaluate(() => document.body.innerText)returns the whole string over CDP with no cap. A downstream limit cannot help — by the time the caller aborts its read, this process has already built the string.What this does
Peak memory becomes the product of two limits, so each in-flight render is guaranteed its full allocation regardless of how many other callers are active:
GVM_WEBDRIVER_MAX_CONCURRENT_RENDERS4--renderer-process-limit=4, so it enforces what the flags already implied rather than reducing capability.GVM_WEBDRIVER_MAX_RESPONSE_MB32GVM_WEBDRIVER_RENDER_QUEUE_TIMEOUT120sThree design points worth reviewing:
extractBoundedcompares the length browser-side and returns only a number when the document is too large.Resulting-Statuswith HTTP 200.genvm-web-default.lua:36-46maps a badResulting-Statusto a non-fatalWEBPAGE_LOAD_FAILED, whereas an HTTP-level failure is fatal and aborts the caller's whole execution. New limits must not be a new way to abort an execution. No Lua changes needed./healthcheckshares the semaphore, since it performs real renders and would otherwise bypass the cap.Drive-by fix
renderPagedidreturn renderPageWithBrowser(...)withoutawaitinsidetry/finally, sobrowserInstance.close()decremented the refcount before the render completed. That defeats theBrowserHolderrefcount that exists to stop rotation closing a browser mid-render. Added theawait.Testing
npm test— 10 unit tests for the semaphore (FIFO ordering, timeout removes the waiter, no permit leak on throw or double-release, concurrency never exceeds the limit under 50 concurrent tasks).Verified end-to-end against real Chrome:
npx tsc --noEmitclean. Test runner is Node's built-innode:testvia the existingts-node/esmloader — no new dependencies.Behaviour change to weigh
GVM_WEBDRIVER_MAX_RESPONSE_MB=32is the one default that changes what succeeds: a page whose text exceeds 32 M characters now returns 507 instead of being rendered. Typical page text is tens of KB, so this is ~300× the largest realistic page — but it is a real change and the value is worth a second opinion. Everything else either matches an existing implied limit or only takes effect under saturation.Not in scope
Container memory limit (lives in the deployment manifests, not this repo), Chromium pinning, lowering
--max-old-space-size/--renderer-process-limit, and makingRenderfailures non-fatal in the Lua. The last one matters more now that limits exist — worth doing next.Summary by CodeRabbit