Skip to content

Rethinking cgroup Memory Enforcement#1485

Open
josephjclark wants to merge 15 commits into
mainfrom
cgroup-oom-limit-2
Open

Rethinking cgroup Memory Enforcement#1485
josephjclark wants to merge 15 commits into
mainfrom
cgroup-oom-limit-2

Conversation

@josephjclark

@josephjclark josephjclark commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Re-implementation of #1467

This rework takes a different approach to cgroup enforcement than #1467.

#1467 had the engine self-provision the whole cgroup hierarchy, starting from
the real cgroup root (/sys/fs/cgroup) and building a fixed path down from
there. That requires root, and if the root cgroup is already populated (which
it always is), it relocates every process on the host/container into a leader
cgroup just to make room — a much bigger footprint than we need.

This branch instead has the engine just use whatever cgroup it's already
running in (read from /proc/self/cgroup). No root needed, no touching the
host root — we rely on the environment (container runtime, systemd Delegate=,
etc.) to have already put us somewhere writable, and only create our per-run
leaves under that.

Other simplifications that came out of this:

  • Cgroup enforcement is now off by default, and fully separate from the V8
    heap limit (--max-old-space-size) — you can toggle either independently.
  • No more separate "cgroup memory limit" and "cgroup parent" options — the
    ceiling is just derived from the existing run-memory setting, and there's
    no legitimate case where you'd want the leaf cgroup created anywhere other
    than wherever the engine already lives.
  • ws-worker gets a single --cgroup flag instead of two separate
    options.
  • The enforcement test is gated on actually detecting a writable cgroup
    (not just "is this Linux"), and is excluded from the normal test run since
    it's only safe inside an isolated cgroup — running it in an ordinary dev
    shell can take down more than just the test.

Claude notes:

What changed
cgroup.ts — the core rework. ensureParent and all root-hierarchy mutation deleted. New resolveSelfCgroup() reads /proc/self/cgroup to default the parent to the cgroup the worker was started in. detect() is now read-only (exists, under the mount, memory controller present, writable); prepare() delegates the memory controller with the leader move confined strictly to the given parent. Also: the rmdir-retry timer is unref'd.

pool.ts — default parent is now resolveSelfCgroup().

lifecycle.ts / events.ts — source is now forwarded on WORKFLOW_ERROR, closing the propagation gap from my review. One discovery here: EngineError already had an unused source = 'engine' field, which the PR's 'heap' | 'cgroup' silently overloads.

@github-project-automation github-project-automation Bot moved this to New Issues in Core Jul 16, 2026
@josephjclark josephjclark mentioned this pull request Jul 16, 2026
8 tasks
Comment thread packages/engine-multi/src/worker/cgroup.ts
// becomes empty and its controllers can be delegated. Within a delegated
// subtree the only member processes are our own (typically just the worker,
// when it was started inside `parent`)
function moveProcsToLeader(parent: string, logger: Logger) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Rename procs to Processes

Comment thread packages/engine-multi/src/worker/pool.ts Outdated

// Hard memory.max ceiling (mb) applied to each child via a cgroup v2 leaf.
// Best-effort: ignored on hosts without a writable cgroup v2 hierarchy.
cgroupMemoryLimitMb?: number;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The cgroup limit should be derived from the memoryLimit, no? Maybe we accept a multiplier or overhead as an argument, but I think this should be automated

Comment thread packages/engine-multi/src/worker/pool.ts Outdated
Comment thread packages/engine-multi/src/worker/pool.ts Outdated
@josephjclark josephjclark changed the title Cgroup oom limit 2 Rethinking cgroup Memory Enforcement Jul 17, 2026
@josephjclark
josephjclark marked this pull request as ready for review July 17, 2026 13:50
@josephjclark
josephjclark requested a review from doc-han July 17, 2026 13:50
@josephjclark

Copy link
Copy Markdown
Collaborator Author

@doc-han I'd appreciate you spending an hour with this and letting me know what you think. There are pretty good docs (I hope!) in the worker and engine readmes

The outstanding gotcha is permissions. This will only work if the docker image runs with --privileged right now (I presume this is the same change we need in your original PR?). I've opened a thread in deployment to try and work out a solution: https://github.com/OpenFn/deployment/issues/482

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New Issues

Development

Successfully merging this pull request may close these issues.

3 participants