Skip to content

fix: automemlimit - #5180

Merged
jbsouvestre merged 3 commits into
mainfrom
feat/gomemlimit
Oct 12, 2026
Merged

fix: automemlimit#5180
jbsouvestre merged 3 commits into
mainfrom
feat/gomemlimit

Conversation

@jbsouvestre

@jbsouvestre jbsouvestre commented Oct 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Should fix some OOMKilled issues by setting the go memory limit to 90%

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation change (pure documentation change)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking changes to code which doesn't change any behaviour)
  • Performance improvement (non-breaking change which improves performance)
  • CI (any automation pipeline changes)
  • Chore (changes which are not directly related to any business logic)
  • Test changes (add, refactor, improve or change a test)
  • This change requires a documentation update

Checklist

Changes have been:

  • Documented (where applicable)
  • Added to CHANGELOG (where applicable) -- see Keep a Changelog
🤖 AI Disclosure
  • I acknowledge that an LLM was used in the creation of this Pull Request, in accordance with Hatchet's AI_POLICY.md.
  • Details: [e.g. generating tests, writing docs]

@vercel

vercel Bot commented Oct 12, 2026 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
hatchet-docs Ready Ready Preview Oct 12, 2026 1:45pm UTC

Request Review

@github-actions github-actions Bot added the engine Related to the core Hatchet engine label Oct 12, 2026
@jbsouvestre jbsouvestre changed the title fix:automemlimit fix: automemlimit Oct 12, 2026
@jbsouvestre
jbsouvestre marked this pull request as ready for review October 12, 2026 12:41
@jbsouvestre jbsouvestre self-assigned this Oct 12, 2026
@greptile-apps

greptile-apps Bot commented Oct 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium impact] The PR appears safe to merge, with no outstanding finding or accepted new issue.

Summary

The PR adds automatic Go memory limits to the API, engine, and hatchet-lite.

  • Hatchet services set Go’s memory limit from their container limit.

Reviews (3) · Last reviewed commit: "document GOMEMLIMIT/AUTOMEMLIMIT and tid..." · Reviewed by Greptile

greptile-apps[bot]
greptile-apps Bot reviewed Oct 12, 2026
View reviewed changes
Comment thread pkg/cmdutils/memlimit.go Outdated
igor-kupczynski
igor-kupczynski approved these changes Oct 12, 2026
View reviewed changes

@igor-kupczynski igor-kupczynski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, with some nits around the comments.

FWIIW there's a lot of prior art in using this lib in dev tools

Maybe one more thing to consider -- should we allow an opt out? (e.g. via an env var?). I don't see why would anyone NOT want it, esp if GOMEMLIMIT takes precedence, but maybe? WDYT @jbsouvestre

Project How it uses automemlimit
Caddy (cmd/main.go) Always on. Uses the cgroup limit, falls back to system memory, logs through its zap logger. Opt-out is the library's env vars.
Prometheus (cmd/prometheus/main.go) --auto-gomemlimit flag (on by default in 3.x, as far as I remember), plus .ratio and .refresh-interval flags; the refresh help text calls out VPA. Cgroup with system fallback.
Thanos Flag plus ratio. Cgroup with system fallback.
Grafana Tempo Config AutoMemLimitEnabled / AutoMemLimitRatio, refreshes every 15s, cgroup with system fallback.
Grafana Alloy Always on with library defaults (cgroup only, 0.9). Comment says it honors GOMEMLIMIT if set.
Kyverno Ratio flag, cgroup with system fallback.

Comment thread pkg/cmdutils/memlimit.go Outdated
// // Set the AUTOMEMLIMIT environment variable to a ratio in (0.0, 1.0], or "off".
// re-reads the limit every minute ( to support VPA in place resize).
func SetMemoryLimit(ctx context.Context) {
// Info level drops the "GOMEMLIMIT is unchanged" debug line logged on every refresh.

@igor-kupczynski igor-kupczynski Oct 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we drop "GOMEMLIMIT is unchanged" to debug? or maybe set the log level to warn? I'm worried about the log spam from 3x deployments x 32 shards every minute

@jbsouvestre jbsouvestre Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The "GOMEMLIMIT is unchanged" line is already gone: automemlimit logs it at debug, and the logger has been capped at info since a887fb2. So each pod logs one line at startup, plus one per actual limit change. I checked it in a 512Mi container resized to 1Gi: two lines across the refresh, nothing in between.

I would keep info rather than warn, so the startup line (which shows the effective limit) stays visible. 381727e rewords the code comment so this is clear from the code.

Comment thread pkg/cmdutils/memlimit.go Outdated
Comment on lines +18 to +22
// FROM: https://github.com/KimMachineGun/automemlimit#usage
// // Importing this package sets GOMEMLIMIT automatically from the cgroup memory limit.
// // By default, it sets GOMEMLIMIT to 90% of the cgroup memory limit.
// // Set the AUTOMEMLIMIT environment variable to a ratio in (0.0, 1.0], or "off".
// re-reads the limit every minute ( to support VPA in place resize).

@igor-kupczynski igor-kupczynski Oct 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can you reword what does this do in like one line/sentence, and then move the copy paste comment above the package import (as this is what triggers the side effect)

@jbsouvestre jbsouvestre Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reworded to a short godoc in 381727e. I did not move the copied text above the import: that side effect belongs to the root automemlimit package (its init()), while we import the memlimit subpackage and call memlimit.Set explicitly, so the import alone does nothing.

@jbsouvestre
document GOMEMLIMIT/AUTOMEMLIMIT and tidy SetMemoryLimit comments
381727e 
- Rewrite the SetMemoryLimit godoc as a short description that names the
  GOMEMLIMIT and AUTOMEMLIMIT overrides.
- Clarify why the logger is capped at info: automemlimit logs the
  per-refresh "unchanged" line at debug, so only startup and real changes
  are logged.
- Add a Memory Limit Configuration section to the self-hosting
  configuration options.
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Oct 12, 2026
@jbsouvestre

jbsouvestre commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

@igor-kupczynski re opt-out: the library's env vars already cover it. AUTOMEMLIMIT=off disables it, AUTOMEMLIMIT=<ratio> changes the ratio, and an explicit GOMEMLIMIT always wins (same approach as Caddy and Alloy in your table). I'd rather not add a SERVER_* flag on top. 381727e documents both in Memory Limit Configuration.

One difference from most projects in your table: we don't fall back to system memory when there is no cgroup limit, we keep Go's default. That mainly affects hatchet-lite on a bare VM, where 90% of host memory wouldn't protect much anyway, so I kept it conservative.

@jbsouvestre
jbsouvestre merged commit 1ba044d into main Oct 12, 2026
66 checks passed
@jbsouvestre
jbsouvestre deleted the feat/gomemlimit branch October 12, 2026 13:59

This branch was successfully deployed

1 active deployment
Preview — 381727e5 Deployed Oct 12, 2026 by vercel[bot]
View deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments
@igor-kupczynski igor-kupczynski igor-kupczynski approved these changes

Assignees

@jbsouvestre jbsouvestre

Labels

documentation Improvements or additions to documentation engine Related to the core Hatchet engine

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jbsouvestre @igor-kupczynski