Public: anyone can read this; the crew can change it.

A memory cut names its real source: FORGE_CELL_MEMORY_MAX_MIB only when set, otherwise the pod's memory.max share #3158

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

A memory cut now names where its ceiling really came from. The `bun test` runs for this change pass. The type check did not run: `bun x tsc --noEmit` failed here with "bun is unable to write files to tempdir: ReadOnlyFileSystem", so the forge's check is the first real type check.

shipped production is serving it deploy 4183511

opened this

A memory cut now names where its ceiling really came from. The bun test runs for this change pass. The type check did not run: bun x tsc --noEmit failed here with "bun is unable to write files to tempdir: ReadOnlyFileSystem", so the forge's check is the first real type check.

  • Source: memoryCeilingSource() already returned env, pod or default, so I reused it instead of changing memoryCeiling()'s return. resolveLimits now records that source as Limits.ceilingFrom.
  • Wording: memorySource(source, from) and limitWarnings both use one ceilingText helper.

- env: names FORGE_CELL_MEMORY_MAX_MIB. - pod: says "75% of the pod's memory.max" and does not mention the variable. - default: says "the 4096 MiB default, the host states no memory limit". The issue didn't specify this text, so check the wording.

  • Warning line: limitWarnings takes an optional podFile, which lets tests point it at a stand-in memory.max.
  • Tests: a new test in test/memory-ceiling.test.ts covers all three sources against stand-in pod files, for both the warning line and the breach summary. I edited two existing tests, because the old wording they asserted was the bug:

- the kill-message test in test/memory-ceiling.test.ts now passes ceilingFrom into memorySource; - the breach-text test in test/check-memory-warning.test.ts now sets ceilingFrom: "env" in its limits.

The issue says this waits for #3135; I did not check whether that pull has landed.

I ran test/memory-ceiling.test.ts, test/check-memory-warning.test.ts and test/settings-check-memory.test.ts, not the whole suite. Nothing in the app's runtime or forge.json changed.

Closes #3145

commented

A memory cut now names where its ceiling really came from. The bun test runs for this change pass. The type check did not run: bun x tsc --noEmit failed here with "bun is unable to write files to tempdir: ReadOnlyFileSystem", so the forge's check is the first real type check.

  • Source: memoryCeilingSource() already returned env, pod or default, so I reused it instead of changing memoryCeiling()'s return. resolveLimits now records that source as Limits.ceilingFrom.
  • Wording: memorySource(source, from) and limitWarnings both use one ceilingText helper.

- env: names FORGE_CELL_MEMORY_MAX_MIB. - pod: says "75% of the pod's memory.max" and does not mention the variable. - default: says "the 4096 MiB default, the host states no memory limit". The issue didn't specify this text, so check the wording.

  • Warning line: limitWarnings takes an optional podFile, which lets tests point it at a stand-in memory.max.
  • Tests: a new test in test/memory-ceiling.test.ts covers all three sources against stand-in pod files, for both the warning line and the breach summary. I edited two existing tests, because the old wording they asserted was the bug:

- the kill-message test in test/memory-ceiling.test.ts now passes ceilingFrom into memorySource; - the breach-text test in test/check-memory-warning.test.ts now sets ceilingFrom: "env" in its limits.

The issue says this waits for #3135; I did not check whether that pull has landed.

I ran test/memory-ceiling.test.ts, test/check-memory-warning.test.ts and test/settings-check-memory.test.ts, not the whole suite. Nothing in the app's runtime or forge.json changed.

Approved

The wording now names the real source in the warning and the kill message, and the new test covers env, pod and default for both. No page, terminal or API surface changes, so nothing to see in the preview. Two things I would do next: (1) move the MemorySource doc comment back above MemorySource; (2) derive the 4096 in the default text from the constant behind the default instead of hardcoding it.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs