Skip to content

fix: close the newest capture when memory nears the container limit - #26

Merged
ChiragAgg5k merged 4 commits into
mainfrom
fix/memory-budget
Nov 1, 2026
Merged

ChiragAgg5k merged 4 commits into
mainfrom
fix/memory-budget

Conversation

@ChiragAgg5k

@ChiragAgg5k ChiragAgg5k commented Nov 1, 2026 •

Copy link
Copy Markdown
Member

Why

On 2026-11-01, both appwrite-browser pods in fra1 were OOMKilled twice. A customer's Astro site has a MapLibre map backed by a 7000×7000 webp (2.2 MB on the wire, 187 MiB decoded). Headless Chromium has no GPU, so the image is held as decoded pixels in the renderer, MapLibre's buffer, and a software WebGL texture in the GPU process. The worker captures light and dark in parallel, which takes about 1.55 GiB per job.

Kubernetes kills the whole container on OOM, so every capture in flight failed. The worker's retry then took down the other replica:

05:43:38  job 1 → pod f28pv   OOM  "Empty reply from server"
05:43:52  retry → pod qpbmn   OOM
06:11:05  new deployment, same site, same two OOMs

What

src/utils/memory.ts polls the container's working set (memory.current − inactive_file, the figure the OOM killer and kubelet use) every 100 ms while captures are in flight. When it passes MEMORY_BUDGET × memory.max (default 0.85), it closes the newest capture's context, waits 1 s for memory to drop, and checks again. That capture returns 507 {"error":"Page exceeded the memory budget and was closed."}. Everything else keeps running.

working set ─────────────╮ budget (85%) ┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄
                         ╰─▶ close newest capture → 507
                             others finish → 200
                             Chromium + container stay up

Memory can't be measured per page because all pages share one GPU process, so this works on the container total. Without a cgroup v2 memory limit, the guard is off.

Measured

docker run --memory 2g. Three concurrent captures of the customer page (light, dark, light) plus one example.com:

0.3.10 this branch
Peak working set 1990 MiB 1872 MiB
OOMKilled yes no
GTA captures 400, 200, 200 200, 507, 200
example.com 200 200
Next capture 200 200

Log: Memory budget exceeded: 1895 of 2048 MiB, closing the newest capture

Tests

Suite Covers
tests/recovery/memory.test.ts Runs appwrite/browser:local with --memory 1g -e MEMORY_BUDGET=0.05: capture → 507 + JSON error, /v1/health still 200. Fails on 0.3.10 (gets 200)

This branch includes #27's commit (SSRF fixtures moved to httpbin.dev; httpbin.org now refuses internal redirects), which main needs to pass CI. Merge #27 first or close it as superseded.

Notes

  • Reports are tracked too. Closing the context stops the page but not a tab Lighthouse opens on its own.
  • After release, bump the image in the cloud chart. The cloud-browser OOMKill alert should stop firing for this site.

One heavy page (a 7000x7000 image drawn by MapLibre in software WebGL)
pushed Chromium past the 2GiB limit in production. The kernel then killed
the whole container, failing every capture in flight on it, and the
worker's retry did the same to the other replica.
@hansi-codes

hansi-codes Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

🔵 Tier A · Mergeable after minor fixes

The change is otherwise consistent with existing callers, but removes automated coverage of the guard's multi-capture behavior.

The pull request adds a cgroup-v2 working-set guard that tracks screenshot and Lighthouse captures, closes the newest context when usage exceeds a configurable budget, and reports that capture as a 507; the README documents the setting and response. It adds constrained-container recovery coverage and updates SSRF fixtures to use httpbin.dev. The latest commits simplify the guard constructor, fix polling and settle intervals at their defaults, and remove the standalone memory unit tests.

Latest changes: The latest commit replaces the options-object constructor with positional arguments, fixes polling and settling at 100 ms and 1 s, and drops the memory guard unit tests.

Verdict New comments Fixed Still open
✅ Approved 1 0 0
Finding Where
🟡 [minor] Retain multi-capture guard coverage src/utils/memory.ts:91
Fix with agent prompt
### Issue 1
src/utils/memory.ts:91
**[minor] Retain multi-capture guard coverage**

The latest commit removes the configurable polling and settle intervals and drops `tests/unit/memory.test.ts`; the remaining recovery test exercises only one capture, so newest-first selection and waiting before closing another capture are no longer asserted. Could we keep equivalent automated coverage, using production timings or fake timers?

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
📂 Walkthrough · 6
File Change
README.md Documents the memory-budget setting, cgroup-v2 behavior, and 507 response.
src/routes/reports.ts Tracks Lighthouse contexts and maps memory-budget closures to 507 responses.
src/routes/screenshots.ts Tracks screenshot contexts and maps memory-budget closures to 507 responses.
src/utils/memory.ts Reads cgroup working-set usage and closes the newest tracked capture over budget; the latest commit fixes polling and settle intervals at their defaults.
tests/e2e/ssrf.test.ts Moves SSRF redirect and HTML fixtures to httpbin.dev.
tests/recovery/memory.test.ts Exercises a 507 under a constrained container memory limit and checks that health remains available.

Reviewed the commits since 8366e3a · Details · Comment @hansi-codes review to re-run, or mention @hansi-codes with a question.

@hansi-codes hansi-codes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Tier A · Looks good to merge. Summary

Comment thread src/utils/memory.ts Resolved
@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Docker Image Stats

Metric Value
Image Size 547MB
Memory Usage 106.5MiB
Cold Start Time 1.01s
Screenshot Time 1.00s

Screenshot benchmark: Average of 3 runs on https://appwrite.io

httpbin.org now refuses redirects to internal addresses and serves /base64
as text/plain with nosniff, so the browser never attempted the private hop
and the four redirect and script tests captured a 200 instead of a 403.

@hansi-codes hansi-codes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Tier S · Looks good to merge. Summary

@hansi-codes hansi-codes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Tier A · Looks good to merge. Summary

Comment thread src/utils/memory.ts
this.tracked.push(entry);

if (this.enabled && this.timer === null) {
this.timer = setInterval(() => this.check(), POLL_INTERVAL_MS);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[minor] Retain multi-capture guard coverage

The latest commit removes the configurable polling and settle intervals and drops tests/unit/memory.test.ts; the remaining recovery test exercises only one capture, so newest-first selection and waiting before closing another capture are no longer asserted. Could we keep equivalent automated coverage, using production timings or fake timers?

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/utils/memory.ts
Line: 91

Comment:
**[minor] Retain multi-capture guard coverage**

The latest commit removes the configurable polling and settle intervals and drops `tests/unit/memory.test.ts`; the remaining recovery test exercises only one capture, so newest-first selection and waiting before closing another capture are no longer asserted. Could we keep equivalent automated coverage, using production timings or fake timers?

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

🟡 Minor · testing · Reply if this doesn't apply.

@ChiragAgg5k
ChiragAgg5k merged commit 187fde0 into main Nov 1, 2026
4 checks passed
@ChiragAgg5k
ChiragAgg5k deleted the fix/memory-budget branch November 1, 2026 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant