Skip to content

chore(sdk): cap browser heap in huge.spec.ts to catch memory leaks #925

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
eugenioenko wants to merge 8 commits into from
Closed

chore(sdk): cap browser heap in huge.spec.ts to catch memory leaks #925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fe115e6
test: cap browser heap in huge.spec.ts to catch memory leaks
eugenioenko Apr 23, 2026
151bccd
Merge branch 'main' into test/heap-cap-huge-spec
eugenioenko Apr 23, 2026
4cdfd40
ci: include huge test in playwright run
eugenioenko Apr 23, 2026
2859576
chore: large file added to roundtirp
eugenioenko Apr 23, 2026
c9359d2
chore: huge run
eugenioenko Apr 23, 2026
7a9ba69
🤖 🎨 Autoformat
eugenioenko Apr 23, 2026
a64703e
fix(tests): replace undefined appUrl with current page URL
eugenioenko Apr 23, 2026
66c2e73
fix(tests): replace undefined appUrl in roundtrip.spec.ts
eugenioenko Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/reusable_build-and-test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ jobs:
go env GOWORK
- run: docker compose up -d --wait --wait-timeout 240
- env:
PLAYWRIGHT_TESTS_TO_RUN: roundtrip
PLAYWRIGHT_TESTS_TO_RUN: roundtrip huge
run: |-
./wait-and-test.sh platform
Comment on lines 218 to 221
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.


Expand Down
11 changes: 10 additions & 1 deletion web-app/tests/tests/huge.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ import { test, expect, type Page } from '@playwright/test';
import fs from 'node:fs';
import { authorize, loadFile } from './acts.js';

test.use({
launchOptions: {
args: ['--js-flags=--max-old-space-size=512'],
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

},
});
Comment on lines +5 to +9
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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 -50

Repository: opentdf/web-sdk

Length of output: 1946

🏁 Script executed:

wc -l web-app/tests/tests/huge.spec.ts

Repository: opentdf/web-sdk

Length of output: 94

🏁 Script executed:

cat -n web-app/tests/tests/huge.spec.ts

Repository: 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.


test.beforeEach(async ({ page }) => {
page.on('pageerror', (err) => {
console.error(err);
Expand All @@ -13,7 +19,10 @@ test.beforeEach(async ({ page }) => {

test('Large File', async ({ page }) => {
await authorize(page);
await page.goto(`${appUrl}?segmentBatchSize=2&maxConcurrentSegmentBatches=1`);
const currentUrl = new URL(page.url());
currentUrl.searchParams.set('segmentBatchSize', '2');
currentUrl.searchParams.set('maxConcurrentSegmentBatches', '1');
await page.goto(currentUrl.toString());
await expect(page.locator('#sessionState')).toHaveText('loggedin');

const decryptTuningLogs: string[] = [];
Expand Down
57 changes: 57 additions & 0 deletions web-app/tests/tests/roundtrip.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,3 +100,60 @@ test('Remote Source Streaming', async ({ page }) => {
server.close();
}
});

test('Large File', async ({ page }) => {
await authorize(page);
const currentUrl = new URL(page.url());
currentUrl.searchParams.set('segmentBatchSize', '2');
currentUrl.searchParams.set('maxConcurrentSegmentBatches', '1');
await page.goto(currentUrl.toString());
await expect(page.locator('#sessionState')).toHaveText('loggedin');
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const decryptTuningLogs: string[] = [];
page.on('console', (message) => {
const text = message.text();
if (text.includes('Using decrypt read tuning')) {
decryptTuningLogs.push(text);
}
});

const threeGigs = 3 * 2 ** 30;
await page.locator('#randomSelector').fill(threeGigs.toString());

const downloadPromise = page.waitForEvent('download');
await page.locator('#fileSink').click();
await page.locator('#encryptButton').click();

const download = await downloadPromise;
const cipherTextPath = await download.path();
try {
expect(download.suggestedFilename()).toContain('bytes');
expect(cipherTextPath).toBeTruthy();
if (!cipherTextPath) {
throw new Error();
}

await page.locator('#randomSelector').clear();
await loadFile(page, cipherTextPath);
const plainDownloadPromise = await page.waitForEvent('download', { timeout: 60000 });
await page.locator('#fileSink').click();
await page.locator('#decryptButton').click();
const download2 = await plainDownloadPromise;
Comment on lines +138 to +141
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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.

expect(download2.suggestedFilename()).toContain('.decrypted');
expect(decryptTuningLogs).toEqual([
'Using decrypt read tuning {"segmentBatchSize":2,"maxConcurrentSegmentBatches":1}',
]);
const plainTextPath = await download2.path();
if (!plainTextPath) {
throw new Error();
}
try {
const stats = fs.statSync(plainTextPath);
expect(stats).toHaveProperty('size', threeGigs);
} finally {
plainTextPath && fs.unlinkSync(plainTextPath);
}
} finally {
cipherTextPath && fs.unlinkSync(cipherTextPath);
}
});
Loading