Skip to content

Fix Docker volume leak on SIGTERM, SIGHUP, and concurrent cleanup#1787

Merged
firecow merged 4 commits intofirecow:masterfrom
cegofrhs:fix/docker-volume-cleanup-on-signals
Mar 2, 2026
Merged

Fix Docker volume leak on SIGTERM, SIGHUP, and concurrent cleanup#1787
firecow merged 4 commits intofirecow:masterfrom
cegofrhs:fix/docker-volume-cleanup-on-signals

Conversation

@cegofrhs
Copy link
Copy Markdown
Contributor

@cegofrhs cegofrhs commented Mar 2, 2026

Summary

  • Add SIGTERM and SIGHUP signal handlers — closing a terminal (SIGHUP) or kill <pid> (SIGTERM) previously terminated the process without running cleanup, leaving orphaned gcl-*-build and gcl-*-tmp Docker volumes
  • Consolidate signal/error cleanup into a guarded cleanupAndExit() — eliminates the race condition where Ctrl+C triggered cleanup from both the SIGINT handler and the error catch block concurrently, with process.exit() from the winner killing the loser mid-flight before docker volume rm
  • Add re-entry guard on Job.cleanupResources() — concurrent callers now share the same cleanup promise instead of racing on docker rm vs docker volume rm ordering

Test plan

  • tsc --noEmit passes (no new errors in src/)
  • bun run lint passes
  • bun run build passes
  • bun test tests/test-cases/artifacts-docker/ passes (exercises Docker volume creation/cleanup)
  • bun test tests/test-cases/list-case/ passes
  • bun test tests/test-cases/preview/ passes
  • Manual: run a Docker job, press Ctrl+C, verify no orphaned gcl-* volumes remain
  • Manual: run a Docker job, close the terminal, verify no orphaned gcl-* volumes remain

Summary by cubic

Stops leaking Docker volumes by running a single, guarded cleanup on SIGINT, SIGTERM, SIGHUP, and error exits. Prevents orphaned gcl-* volumes and avoids hangs from concurrent cleanup or cleanup failures.

  • Bug Fixes
    • Route SIGINT, SIGTERM, and SIGHUP to one cleanupAndExit with exit codes 130, 143, 129; first caller's exit code wins.
    • Share a cleanup promise in Job.cleanupResources to prevent concurrent cleanup and docker rm vs docker volume rm races.
    • Use the same cleanup for error exits; keep SIGUSR2 for nodemon-only cleanup and no-op if termination cleanup is in flight.
    • Always call process.exit via .finally() so signals don't hang if cleanup throws.

Written for commit b4a527f. Summary will update on new commits.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/index.ts">

<violation number="1" location="src/index.ts:21">
P2: cleanupAndExit caches a rejected promise and never calls process.exit when cleanupJobResources fails, so a failed cleanup can leave the process running and future signals become no-ops.</violation>
</file>

Since this is your first cubic review, here's how it works:

  • cubic automatically reviews your code and comments on bugs and improvements
  • Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
  • Add one-off context when rerunning by tagging @cubic-dev-ai with guidance or docs links (including llms.txt)
  • Ask questions if you need clarification on any suggestion

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

If cleanupJobResources throws, .then() never fires, the rejected
promise gets cached, and all future signals become no-ops — leaving
the process hanging. Using .finally() ensures process.exit() always
runs regardless of whether cleanup succeeds or fails.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner

@firecow firecow left a comment

Choose a reason for hiding this comment

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

Nice fix — the core change is solid and well-scoped. A few minor suggestions below.

(Correction from earlier: point 3 about clearTimeout only running on the first call was wrong — it's before the re-entry guard and runs every time. Point 5 about a cleanup timeout is out of scope for this PR.)

firecow added 2 commits March 2, 2026 19:23
- Remove space before ( in cleanupAndExit declaration
- Add comment explaining first-caller-wins exit code behavior
- Guard SIGUSR2 handler to skip cleanup if termination cleanup is in flight
The @stylistic/space-before-function-paren rule requires it.
Copy link
Copy Markdown
Owner

@firecow firecow left a comment

Choose a reason for hiding this comment

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

Tested locally — all CI checks pass, both manual signal tests (SIGINT and SIGHUP) confirmed no orphaned Docker volumes.

@firecow firecow merged commit 5d95fff into firecow:master Mar 2, 2026
10 checks passed
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Mar 24, 2026
This MR contains the following updates:

| Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Adoption](https://docs.renovatebot.com/merge-confidence/) | [Passing](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) |
|---|---|---|---|---|---|
| [npm:gitlab-ci-local](https://github.com/firecow/gitlab-ci-local) | `4.67.2` → `4.70.0` | ![age](https://developer.mend.io/api/mc/badges/age/npm/gitlab-ci-local/4.70.0?slim=true) | ![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/gitlab-ci-local/4.70.0?slim=true) | ![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/gitlab-ci-local/4.67.2/4.70.0?slim=true) | ![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/gitlab-ci-local/4.67.2/4.70.0?slim=true) |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>firecow/gitlab-ci-local (npm:gitlab-ci-local)</summary>

### [`v4.70.0`](https://github.com/firecow/gitlab-ci-local/releases/tag/4.70.0)

[Compare Source](firecow/gitlab-ci-local@4.69.0...4.70.0)

#### What's Changed

- Add `GCL_VARIABLE_<name>` env var support, replace `yargs .env("GCL")` by [@&#8203;firecow](https://github.com/firecow) in [#&#8203;1805](firecow/gitlab-ci-local#1805)
- fix: prefix rsync paths with ./ by [@&#8203;ednxzu](https://github.com/ednxzu) in [#&#8203;1801](firecow/gitlab-ci-local#1801)
- chore(deps): update all non-major by [@&#8203;renovate](https://github.com/renovate)\[bot] in [#&#8203;1802](firecow/gitlab-ci-local#1802)
- chore(deps): lock file maintenance by [@&#8203;renovate](https://github.com/renovate)\[bot] in [#&#8203;1798](firecow/gitlab-ci-local#1798)

#### New Contributors

- [@&#8203;ednxzu](https://github.com/ednxzu) made their first contribution in [#&#8203;1801](firecow/gitlab-ci-local#1801)

**Full Changelog**: <firecow/gitlab-ci-local@4.69.0...4.70.0>

### [`v4.69.0`](https://github.com/firecow/gitlab-ci-local/releases/tag/4.69.0)

[Compare Source](firecow/gitlab-ci-local@4.68.1...4.69.0)

#### What's Changed

- fix: disable .env autoload in compiled binaries by [@&#8203;firecow](https://github.com/firecow) in [#&#8203;1799](firecow/gitlab-ci-local#1799)
- Add `--wait-for-services-timeout` option to CLI by [@&#8203;dernilz](https://github.com/dernilz) in [#&#8203;1796](firecow/gitlab-ci-local#1796)
- ci: add Node.js and Bun version matrix to smoke test by [@&#8203;firecow](https://github.com/firecow) in [#&#8203;1797](firecow/gitlab-ci-local#1797)
- chore(deps): update github/codeql-action action to v4.32.6 by [@&#8203;renovate](https://github.com/renovate)\[bot] in [#&#8203;1793](firecow/gitlab-ci-local#1793)

**Full Changelog**: <firecow/gitlab-ci-local@4.68.1...4.68.2>

### [`v4.68.1`](https://github.com/firecow/gitlab-ci-local/releases/tag/4.68.1)

[Compare Source](firecow/gitlab-ci-local@4.68.0...4.68.1)

#### What's Changed

- chore(deps): update all non-major by [@&#8203;renovate](https://github.com/renovate)\[bot] in [#&#8203;1792](firecow/gitlab-ci-local#1792)
- fix: disable Bun automatic .env loading by [@&#8203;firecow](https://github.com/firecow) in [#&#8203;1794](firecow/gitlab-ci-local#1794)

**Full Changelog**: <firecow/gitlab-ci-local@4.68.0...4.68.1>

### [`v4.68.0`](https://github.com/firecow/gitlab-ci-local/releases/tag/4.68.0)

[Compare Source](firecow/gitlab-ci-local@4.67.2...4.68.0)

#### What's Changed

- chore(deps): lock file maintenance by [@&#8203;renovate](https://github.com/renovate)\[bot] in [#&#8203;1785](firecow/gitlab-ci-local#1785)
- fix: docker cp fails on read-only directories by [@&#8203;firecow](https://github.com/firecow) in [#&#8203;1773](firecow/gitlab-ci-local#1773)
- make services run as container user instead of root by [@&#8203;peterbbeu](https://github.com/peterbbeu) in [#&#8203;1781](firecow/gitlab-ci-local#1781)
- chore(deps): update github/codeql-action action to v4.32.5 by [@&#8203;renovate](https://github.com/renovate)\[bot] in [#&#8203;1786](firecow/gitlab-ci-local#1786)
- Migrate test runner from bun:test to vitest by [@&#8203;firecow](https://github.com/firecow) in [#&#8203;1784](firecow/gitlab-ci-local#1784)
- Route parser warnings through WriteStreams instead of console.log by [@&#8203;firecow](https://github.com/firecow) in [#&#8203;1788](firecow/gitlab-ci-local#1788)
- Use test.concurrent for independent integration tests by [@&#8203;firecow](https://github.com/firecow) in [#&#8203;1769](firecow/gitlab-ci-local#1769)
- Fix Docker volume leak on SIGTERM, SIGHUP, and concurrent cleanup by [@&#8203;cegofrhs](https://github.com/cegofrhs) in [#&#8203;1787](firecow/gitlab-ci-local#1787)
- Add TypeScript type checking to CI by [@&#8203;firecow](https://github.com/firecow) in [#&#8203;1789](firecow/gitlab-ci-local#1789)
- Convert remaining tests to concurrent and tune vitest by [@&#8203;firecow](https://github.com/firecow) in [#&#8203;1790](firecow/gitlab-ci-local#1790)
- fix: flaky log-padding test assertions by [@&#8203;firecow](https://github.com/firecow) in [#&#8203;1791](firecow/gitlab-ci-local#1791)

#### New Contributors

- [@&#8203;cegofrhs](https://github.com/cegofrhs) made their first contribution in [#&#8203;1787](firecow/gitlab-ci-local#1787)

**Full Changelog**: <firecow/gitlab-ci-local@4.67.2...4.68.0>

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My42MS43IiwidXBkYXRlZEluVmVyIjoiNDMuODYuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6Om1pbm9yIl19-->
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.

2 participants