chore(sdk): cap browser heap in huge.spec.ts to catch memory leaks#925
chore(sdk): cap browser heap in huge.spec.ts to catch memory leaks#925eugenioenko wants to merge 8 commits intomainfrom
Conversation
Limit V8 old-space to 512 MB so the 3 GB encrypt/decrypt e2e test will OOM if decrypted segments are retained instead of released. This enforces that peak heap scales with segment size, not file size. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Playwright test adjustments and a new large-file roundtrip test: increases Chromium V8 old-space heap for test runs, adds a ~3 GiB encrypt/decrypt roundtrip with console-log assertions and file cleanup, and updates CI workflow to run both Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant WebApp
participant Storage
participant Browser as Playwright-Browser
Tester->>Browser: launch with --js-flags=--max-old-space-size=512
Tester->>WebApp: authorize / open app (with segment batch params)
Tester->>WebApp: upload plaintext (~3 GiB)
WebApp->>Storage: store ciphertext
Tester->>Storage: download ciphertext
Tester->>WebApp: request decrypt of downloaded ciphertext
WebApp->>Tester: emit console logs (includes "Using decrypt read tuning")
Tester->>Tester: assert logs, verify decrypted file exists & size, cleanup files
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
eugenioenko
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a memory limit for the V8 engine in the huge.spec.ts test file by configuring Playwright's launchOptions. The review feedback correctly identifies that the --js-flags argument is specific to Chromium and will not apply to Firefox or WebKit, suggesting that this limitation should be documented or handled via browser-specific test skipping.
|
|
||
| test.use({ | ||
| launchOptions: { | ||
| args: ['--js-flags=--max-old-space-size=512'], |
There was a problem hiding this comment.
The --js-flags argument is specific to the V8 engine (Chromium). This memory cap will not be enforced when running the test suite against Firefox or WebKit, which are both enabled in your playwright.config.ts. Consequently, memory leaks might go undetected on those platforms. Consider adding a comment to document this limitation or using test.skip for non-Chromium browsers if the test's primary purpose is to validate heap constraints.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-app/tests/tests/huge.spec.ts`:
- Around line 5-9: The test sets Chromium-specific V8 flags via test.use({
launchOptions: { args: ['--js-flags=--max-old-space-size=512'] } }) in
huge.spec.ts but that flag is ignored by Firefox/WebKit; restrict this spec to
Chromium only by either (A) wrapping or guarding the test file so the
test.use/launchOptions args are applied only when the running project is
'chromium' (use a project-specific filter/skip around the tests referencing
test.use and args), or (B) change the Playwright config so huge.spec.ts is only
included in the chromium project (remove it from firefox/webkit test lists);
update test.use, launchOptions, args, and the Playwright config accordingly.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf1146fa-7301-429a-8318-b70efbe5bec8
📒 Files selected for processing (1)
web-app/tests/tests/huge.spec.ts
| test.use({ | ||
| launchOptions: { | ||
| args: ['--js-flags=--max-old-space-size=512'], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'playwright.config' web-app/tests
fd -t f 'playwright.config' web-app/tests --exec cat {}Repository: opentdf/web-sdk
Length of output: 2796
🏁 Script executed:
cat -n web-app/tests/tests/huge.spec.ts | head -50Repository: opentdf/web-sdk
Length of output: 1946
🏁 Script executed:
wc -l web-app/tests/tests/huge.spec.tsRepository: opentdf/web-sdk
Length of output: 94
🏁 Script executed:
cat -n web-app/tests/tests/huge.spec.tsRepository: opentdf/web-sdk
Length of output: 2207
The heap cap applies only to Chromium; Firefox and WebKit will ignore --js-flags.
The Playwright config runs huge.spec.ts across all three browser projects (chromium, firefox, webkit), but --js-flags=--max-old-space-size=512 is a Chromium/V8-specific flag. Firefox and WebKit will silently ignore it, meaning the memory leak detection guarantee from this change only holds for the Chromium run — the other browsers could still pass despite the leak in #919.
Limit this test to the chromium project, either by wrapping the test.use() call with a project-specific filter or by modifying the Playwright config to exclude firefox/webkit for huge.spec.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-app/tests/tests/huge.spec.ts` around lines 5 - 9, The test sets
Chromium-specific V8 flags via test.use({ launchOptions: { args:
['--js-flags=--max-old-space-size=512'] } }) in huge.spec.ts but that flag is
ignored by Firefox/WebKit; restrict this spec to Chromium only by either (A)
wrapping or guarding the test file so the test.use/launchOptions args are
applied only when the running project is 'chromium' (use a project-specific
filter/skip around the tests referencing test.use and args), or (B) change the
Playwright config so huge.spec.ts is only included in the chromium project
(remove it from firefox/webkit test lists); update test.use, launchOptions,
args, and the Playwright config accordingly.
X-Test Failure Report |
Temporarily add huge to PLAYWRIGHT_TESTS_TO_RUN to validate the heap cap catches the memory leak on main. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
X-Test Failure Report |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
web-app/tests/tests/roundtrip.spec.ts (1)
157-160: Minor: thefoo && fs.unlinkSync(foo)guard is redundant.At these points
cipherTextPath/plainTextPathhave already been narrowed/validated to truthy values by the earlierthrow new Error()guards, so the short-circuit is dead code. Either drop the guard or usefs.rmSync(path, { force: true })for robustness against files that may already be gone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-app/tests/tests/roundtrip.spec.ts` around lines 157 - 160, The two cleanup calls using the redundant short-circuit guards (plainTextPath && fs.unlinkSync(plainTextPath) and cipherTextPath && fs.unlinkSync(cipherTextPath)) should be made deterministic: either remove the unnecessary "&&" guard since plainTextPath and cipherTextPath are already validated earlier, or replace fs.unlinkSync(...) with fs.rmSync(path, { force: true }) to safely tolerate missing files; update the cleanup in the finally block accordingly, touching the references to plainTextPath and cipherTextPath and the fs.unlinkSync calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/reusable_build-and-test.yaml:
- Around line 218-221: The workflow currently sets the environment variable
PLAYWRIGHT_TESTS_TO_RUN but wait-and-test.sh (which calls npm test) does not
consume it; update either the workflow or the script so the variable actually
filters tests: either remove PLAYWRIGHT_TESTS_TO_RUN from the workflow, or
modify wait-and-test.sh (or the npm test invocation inside it) to read
PLAYWRIGHT_TESTS_TO_RUN and forward it to the test runner (for example pass it
as a grep/-g argument or as a Playwright project/test filter) so the value of
PLAYWRIGHT_TESTS_TO_RUN affects which tests run.
In `@web-app/tests/tests/roundtrip.spec.ts`:
- Around line 141-144: The test currently awaits page.waitForEvent('download')
before triggering the download, causing a timeout; change to start the waiter
without awaiting (use const plainDownloadPromise = page.waitForEvent('download',
{ timeout: 60000 });), then perform the actions that trigger the download (click
the '#fileSink' and '#decryptButton' locators), and finally await the promise
into download2 (const download2 = await plainDownloadPromise); update the
sequence around page.waitForEvent, the '#fileSink' and '#decryptButton' clicks,
and the plainDownloadPromise/download2 usage so the wait is non-blocking until
after the clicks.
- Around line 11-15: The global test.use({ launchOptions: { args:
['--js-flags=--max-old-space-size=512'] } }) is currently applied to the whole
file; restrict the 512MB V8 cap to only the heavy test by either wrapping the
"Large File" test in a test.describe block and calling test.describe(..., () =>
test.use({ launchOptions: { args: ['--js-flags=--max-old-space-size=512'] } }))
around that block, or move the "Large File" test into a new file (e.g.,
huge.spec.ts) and place the test.use launchOptions there so tests like "login",
"roundtrip ztdf", and "Remote Source Streaming" are unaffected.
- Around line 110-113: The test "Large File" fails because appUrl is not
defined; update the test in roundtrip.spec.ts (the test function named 'Large
File' that calls authorize(page) and
page.goto(`${appUrl}?segmentBatchSize=2&maxConcurrentSegmentBatches=1`) ) to use
a defined URL: either import the shared appUrl constant used elsewhere or
replace appUrl with Playwright's base URL/origin (e.g., use
page.context().baseURL or construct from process.env / test fixtures) and ensure
the page.goto call uses that resolved value so the test no longer throws a
ReferenceError.
---
Nitpick comments:
In `@web-app/tests/tests/roundtrip.spec.ts`:
- Around line 157-160: The two cleanup calls using the redundant short-circuit
guards (plainTextPath && fs.unlinkSync(plainTextPath) and cipherTextPath &&
fs.unlinkSync(cipherTextPath)) should be made deterministic: either remove the
unnecessary "&&" guard since plainTextPath and cipherTextPath are already
validated earlier, or replace fs.unlinkSync(...) with fs.rmSync(path, { force:
true }) to safely tolerate missing files; update the cleanup in the finally
block accordingly, touching the references to plainTextPath and cipherTextPath
and the fs.unlinkSync calls.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38be07b3-393b-4201-800e-96c13e24b17a
📒 Files selected for processing (3)
.github/workflows/reusable_build-and-test.yamlweb-app/tests/tests/huge.spec.tsweb-app/tests/tests/roundtrip.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web-app/tests/tests/huge.spec.ts
| - env: | ||
| PLAYWRIGHT_TESTS_TO_RUN: roundtrip | ||
| PLAYWRIGHT_TESTS_TO_RUN: roundtrip huge | ||
| run: |- | ||
| ./wait-and-test.sh platform |
There was a problem hiding this comment.
PLAYWRIGHT_TESTS_TO_RUN is not consumed by wait-and-test.sh.
Per .github/workflows/roundtrip/wait-and-test.sh (lines 183–198), the script invokes npm test with no grep/filter arguments and does not read PLAYWRIGHT_TESTS_TO_RUN. Setting it to roundtrip huge has no effect on which tests run — Playwright will execute the full suite regardless. Either wire the env var into the script (e.g., forward it as -g or as a positional project/test filter), or drop it here to avoid the false impression of selective targeting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/reusable_build-and-test.yaml around lines 218 - 221, The
workflow currently sets the environment variable PLAYWRIGHT_TESTS_TO_RUN but
wait-and-test.sh (which calls npm test) does not consume it; update either the
workflow or the script so the variable actually filters tests: either remove
PLAYWRIGHT_TESTS_TO_RUN from the workflow, or modify wait-and-test.sh (or the
npm test invocation inside it) to read PLAYWRIGHT_TESTS_TO_RUN and forward it to
the test runner (for example pass it as a grep/-g argument or as a Playwright
project/test filter) so the value of PLAYWRIGHT_TESTS_TO_RUN affects which tests
run.
| const plainDownloadPromise = await page.waitForEvent('download', { timeout: 60000 }); | ||
| await page.locator('#fileSink').click(); | ||
| await page.locator('#decryptButton').click(); | ||
| const download2 = await plainDownloadPromise; |
There was a problem hiding this comment.
waitForEvent is awaited before the action that triggers it — will time out.
const plainDownloadPromise = await page.waitForEvent('download', { timeout: 60000 }); blocks here waiting for a download event, but the clicks that initiate the download are on lines 142–143, after this await. No event can arrive, so this will hang until the 60 s timeout and the test will fail (or worse, any download actually produced is from an earlier action). Also note the variable name suggests a promise but after await it's already a Download, and line 144 then awaits it again.
Mirror the pattern used on lines 39–42 / 53–56:
Proposed fix
- const plainDownloadPromise = await page.waitForEvent('download', { timeout: 60000 });
- await page.locator('#fileSink').click();
- await page.locator('#decryptButton').click();
- const download2 = await plainDownloadPromise;
+ const plainDownloadPromise = page.waitForEvent('download', { timeout: 60000 });
+ await page.locator('#fileSink').click();
+ await page.locator('#decryptButton').click();
+ const download2 = await plainDownloadPromise;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const plainDownloadPromise = await page.waitForEvent('download', { timeout: 60000 }); | |
| await page.locator('#fileSink').click(); | |
| await page.locator('#decryptButton').click(); | |
| const download2 = await plainDownloadPromise; | |
| const plainDownloadPromise = page.waitForEvent('download', { timeout: 60000 }); | |
| await page.locator('#fileSink').click(); | |
| await page.locator('#decryptButton').click(); | |
| const download2 = await plainDownloadPromise; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-app/tests/tests/roundtrip.spec.ts` around lines 141 - 144, The test
currently awaits page.waitForEvent('download') before triggering the download,
causing a timeout; change to start the waiter without awaiting (use const
plainDownloadPromise = page.waitForEvent('download', { timeout: 60000 });), then
perform the actions that trigger the download (click the '#fileSink' and
'#decryptButton' locators), and finally await the promise into download2 (const
download2 = await plainDownloadPromise); update the sequence around
page.waitForEvent, the '#fileSink' and '#decryptButton' clicks, and the
plainDownloadPromise/download2 usage so the wait is non-blocking until after the
clicks.
X-Test Failure Report |
eugenioenko
force-pushed
the
test/heap-cap-huge-spec
branch
from
fde2b4f to
c9359d2
Compare
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
X-Test Failure Report✅ java-v0.9.0 |
X-Test Failure Report |
appUrl was never defined — introduced in #920. Use page.url() and append query params to navigate to the current origin instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Same fix as huge.spec.ts — appUrl was never defined. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
X-Test Failure Report |
1 participant
Summary
huge.spec.ts)Purpose
This PR is a test to see if the heap cap causes the existing
huge.spec.tsto fail onmain(before the fix in #922 lands). If it does fail, this validates the approach and should be merged alongside or after #922.Test plan
huge.spec.tstest fails on CI with the 512 MB heap capSummary by CodeRabbit