Repository navigation
feat: disk-backed /tmp and a configurable guest swapfile - #5
Conversation
The Ubuntu guest mounts /tmp as a tmpfs sized at half of RAM and ships without swap. Under a Gradle-plus-Testcontainers burst, with agent scratch piling up in /tmp, the guest went straight to the OOM killer, which took dockerd out repeatedly and left the VM unresponsive. Provisioning now masks tmp.mount and unmounts the tmpfs on the boot it first runs (deferred to the next boot only if /tmp is busy), and creates /swapfile sized by a new `swap` config key (default 4GiB, 0B disables). The swapfile is resized or removed to follow the config, fstab mirrors what is on disk, and a swap setup failure is logged rather than failing the boot. With swap present the agent slice's MemoryMax swaps instead of OOM-killing. The integration suite asserts /tmp is on the root filesystem without a deferral, and that the swapfile is active and in fstab. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Swap conversion can abort provisioning, masking failures are hidden, and resize/disable transitions lack integration coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds disk-backed /tmp and configurable guest swap to reduce OOM failures.
Changes:
- Adds and validates the
swapconfiguration. - Provisions, resizes, disables, and activates
/swapfile. - Masks
tmp.mountand adds integration checks.
File summaries
| File | Description |
|---|---|
README.md |
Documents swap and disk-backed /tmp. |
test-vm-sandbox.sh |
Verifies /tmp and default swap setup. |
internal/config/config.go |
Adds swap configuration and validation. |
internal/config/config_test.go |
Tests swap defaults and syntax. |
internal/lima/template.go |
Exports swap size to provisioning. |
internal/lima/template_test.go |
Tests swap-size rendering. |
internal/lima/testdata/golden-template.yaml |
Updates expected rendered environment. |
internal/guest/files/scripts/provision-system.sh |
Implements /tmp unmounting and swap management. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address the review on the swap/tmp change. - Validate the swap size host-side: it must parse into 64 bits and be smaller than the disk. Previously a value like 999999999999999999999TiB passed the shape check and made numfmt exit 2 in the guest, which under set -e aborted provisioning instead of degrading to no swap. The guest now also guards that conversion and leaves existing swap untouched on a parse failure rather than treating it as a request for 0B. - Check that tmp.mount actually reports as masked and warn otherwise: the same-boot unmount would make the boot look right even if the mask had silently failed. The suite asserts the masked state directly. - Exercise the swap transitions in the suite by setting swap to 0B before the first restart-hygiene reboot and to 1GiB before the second, asserting the file, active swap list and fstab after each. No extra reboots. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Failed swap activation leaves a persistent fstab entry that causes repeated failing swap units on later boots.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/guest/files/scripts/provision-system.sh:233
- The fstab entry is added before activation is known to work. If
swaponfails (the explicitly handled failure path), the entry remains, so every subsequent boot generates and retries a failing swap unit even though provisioning reports that the guest is running without swap. Add the persistent entry only after the file is active, and remove any managed entry on activation failure.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The fstab entry was written before swapon ran. Had swapon failed, the entry stayed, giving every later boot a failing swap unit, and since the file was already the right size provisioning never rebuilt it either. Activate first, persist second. On activation failure the swapfile is removed along with any fstab line, so the next boot starts clean and retries creation instead of re-activating a file known not to work. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Re the second Copilot pass (fstab entry written before |
There was a problem hiding this comment.
🔵 Needs a closer look
Validation accepts swap sizes that the guest cannot process or format successfully.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/config/config.go:166
parseSizeaccepts unsigned 64-bit values, but the rendered value is later compared with Bash[integer operators, which reject values aboveMaxInt64. For example,swap: 8388609TiBwith a larger valid disk passes this validation, then provisioning printsinteger expression expectedand skips reconciliation, leaving swap absent or stale. Bound the swap value to the guest shell's signed range and add a boundary test.
This issue also appears on line 171 of the same file.
internal/config/config.go:173
- The validation has no lower bound for enabled swap, so values such as
swap: 1Bare accepted even thoughmkswapcannot format such a file; provisioning then deterministically removes it and runs without the configured swap. Reject nonzero sizes below a documented usable minimum (while continuing to allow zero as the disable value).
if swap >= disk {
return fmt.Errorf("swap (%s) must be smaller than disk (%s)", c.Swap, c.Disk)
}
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Two edge values passed validation but could never work in the guest: - Above MaxInt64: numfmt converts it, but bash's `[` rejects it with "integer expected", so every swap step was silently skipped. - Below mkswap's minimum area: formatting failed deterministically and the guest ran without the configured swap. Validate now caps swap at MaxInt64 and requires nonzero sizes to be at least 1MiB, above mkswap's ten-page minimum on both 4 KiB and 64 KiB page kernels. 0B stays the disable value. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Re the third Copilot pass (config.go:166 and :173): both fixed in 271635e. |
The Ubuntu guest mounts /tmp as a tmpfs sized at half of RAM and ships without swap. Under a Gradle-plus-Testcontainers burst, with agent scratch piling up in /tmp, the guest went straight to the OOM killer, which took dockerd out repeatedly and left the VM unresponsive.
Provisioning now masks tmp.mount and unmounts the tmpfs on the boot it first runs (deferred to the next boot only if /tmp is busy), and creates /swapfile sized by a new
swapconfig key (default 4GiB, 0B disables). The swapfile is resized or removed to follow the config, fstab mirrors what is on disk, and a swap setup failure is logged rather than failing the boot. With swap present the agent slice's MemoryMax swaps instead of OOM-killing.The integration suite asserts /tmp is on the root filesystem without a deferral, and that the swapfile is active and in fstab.