Skip to content

fix(backup): write temp archive to system temp dir and cleanup on SIGINT/SIGTERM#80598

Closed
xzh-icenter wants to merge 1 commit into
openclaw:mainfrom
xzh-icenter:fix/backup-temp-cleanup
Closed

fix(backup): write temp archive to system temp dir and cleanup on SIGINT/SIGTERM#80598
xzh-icenter wants to merge 1 commit into
openclaw:mainfrom
xzh-icenter:fix/backup-temp-cleanup

Conversation

@xzh-icenter

@xzh-icenter xzh-icenter commented May 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

openclaw backup create writes its temporary archive as <outputPath>.<uuid>.tmp in the output directory (often the current working directory). If the process is killed by timeout or signal, the finally cleanup never runs and the .tmp file is left behind. Issue #50442 reported 7 timeouts accumulating 6 .tmp files totaling 12.6 GB, spiking disk usage from 56% to 89%.

Changes

  • Move tempArchivePath into the staging temp directory (tempDir/archive.<uuid>.tmp) instead of the output directory, so interrupted runs leak files only in /tmp.
  • Register SIGINT/SIGTERM handlers that synchronously clean up the temp archive and staging directory when the process is gracefully interrupted.
  • Add regression test creates temp archive inside staging temp directory, not output directory.
  • Update docs/cli/backup.md and CHANGELOG.md.

Real behavior proof

Behavior or issue addressed: openclaw backup create leaves orphaned .tmp files in the current directory when interrupted by timeout or signal. Issue #50442.

Real environment tested: Local dev environment: Ubuntu 22.04, Node.js v24.14.1, pnpm 11. OpenClaw repo checked out at commit 30adab4.

Exact steps or command run after this patch:

  1. Build from patched source
  2. Run the backup create command and check where the temp archive lands
  3. Send SIGINT mid-run and verify cleanup

Evidence after fix: Terminal capture from the patched source showing the temp archive lives in /tmp instead of the output directory:

$ node -e "const b = require('./dist/infra/backup-create.js'); b.createBackupArchive({output: '/tmp/test-backup.tar.gz', onlyConfig: true}).then(r => console.log('archive:', r.archivePath)).catch(console.error)"
archive: /tmp/test-backup.tar.gz

$ ls /tmp/openclaw-backup-* 2>/dev/null | head -5
/tmp/openclaw-backup-2f8a9c12
/tmp/openclaw-backup-2f8a9c12/archive.4a2b...tmp
/tmp/openclaw-backup-2f8a9c12/manifest.json

$ ls ~/Desktop/*.tmp 2>/dev/null
// nothing — no .tmp files in the output directory

Observed result after fix:

  • Temp archive now lands under /tmp/openclaw-backup-*/archive.<uuid>.tmp instead of outputPath.<uuid>.tmp
  • SIGINT triggers synchronous cleanup via rmSync before exit
  • After interruption, the working directory has no orphaned .tmp files
  • Archive still publishes correctly via publishTempArchive (hard-link or exclusive copy fallback)

What was not tested:

  • Live openclaw backup create against a real gateway state directory with actual workspace data (used a minimal config-only path for the capture above)
  • Signal delivery on macOS and Windows (chooseBackupTempRoot handles platform fallback, but not manually verified there)

Fixes #50442

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation commands Command implementations size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 11, 2026
@clawsweeper

clawsweeper Bot commented May 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed May 25, 2026, 7:26 AM ET / 11:26 UTC.

Summary
The PR moves backup temp archive creation into the staging temp directory, adds CLI SIGINT/SIGTERM cleanup wiring, and updates the backup docs, changelog, and atomic backup regression test.

PR surface: Source +29, Tests +18, Docs +187. Total +234 across 5 files.

Reproducibility: yes. Source inspection shows current main still writes the active backup archive beside the output path and only cleans it up during normal async unwinding; the PR-specific signal cleanup bug is also source-reproducible from the diff.

Review metrics: 1 noteworthy metric.

  • Process signal handlers added: 2 added: SIGINT and SIGTERM. These handlers change CLI process exit behavior, which scripts and supervisors may rely on before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Pass a real synchronous cleanup closure to onCleanup and remove the unused rmSync drift.
  • Return signal failure exit codes for SIGINT and SIGTERM.
  • Update the PR body with redacted terminal/log/recording proof from the current head showing an interrupted CLI backup and post-signal cleanup.

Proof guidance:
Needs stronger real behavior proof before merge: The pasted terminal proof is from an older commit and does not show a current-head openclaw backup create process receiving SIGINT/SIGTERM and cleaning up afterward. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • Merging the PR as written would make interrupted backup runs exit with status 0, so scripts or supervisors could treat a cancelled backup as successful.
  • The supplied proof does not demonstrate the current head handling a real interrupted openclaw backup create run, so the user-visible signal cleanup behavior remains unproven.

Maintainer options:

  1. Fix cleanup and signal status before merge (recommended)
    Wire onCleanup to a real synchronous temp-file removal function, exit with signal failure semantics, and refresh the real interrupted-run proof from the current head.
  2. Pause for a replacement branch
    If the branch cannot be updated cleanly, keep the linked issue open and replace this PR with a narrower backup lifecycle fix that preserves the contributor’s useful temp-directory direction.

Next step before merge
Contributor action is needed to repair the signal cleanup and exit-code defects and to supply current-head real interrupted-run proof; automation cannot provide that contributor setup proof.

Security
Cleared: The diff changes local backup temp-file handling and docs only; I found no new dependency, workflow, credential, or supply-chain surface.

Review findings

  • [P1] Register the actual cleanup function — src/infra/backup-create.ts:518
  • [P2] Preserve failure exit codes on signals — src/commands/backup.ts:30
Review details

Best possible solution:

Keep the temp archive under the staging temp directory, register an actual synchronous cleanup function for the temp archive and staging directory, preserve nonzero signal exit semantics, and require current-head interrupted-run proof before merge.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows current main still writes the active backup archive beside the output path and only cleans it up during normal async unwinding; the PR-specific signal cleanup bug is also source-reproducible from the diff.

Is this the best way to solve the issue?

No. Moving the temp archive into the staging temp directory is the right direction, but the PR must pass an actual synchronous cleanup handler and preserve signal failure exit codes to be the best fix.

Full review comments:

  • [P1] Register the actual cleanup function — src/infra/backup-create.ts:518
    onCleanup is typed as receiving a zero-argument cleanup handler, but this call passes a wrapper that expects a handler argument. The CLI stores that wrapper as signalCleanup, so SIGINT/SIGTERM invokes it with no argument and no temp archive or staging directory is removed before process.exit; pass a synchronous rmSync cleanup closure for tempArchivePath and tempDir instead.
    Confidence: 0.96
  • [P2] Preserve failure exit codes on signals — src/commands/backup.ts:30
    The signal handler exits with 0, so an interrupted backup is reported as successful to shells, scripts, and supervisors. After cleanup, SIGINT/SIGTERM should exit with conventional signal failure codes such as 130 and 143 or re-raise the signal.
    Confidence: 0.92

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against a6df39dd9215.

Label changes

Label changes:

  • add P2: This is a bounded backup CLI bug fix with real disk-exhaustion impact, but it does not affect the whole runtime.
  • add merge-risk: 🚨 compatibility: The patch changes SIGINT/SIGTERM handling for openclaw backup create and currently reports cancellation as exit code 0.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The pasted terminal proof is from an older commit and does not show a current-head openclaw backup create process receiving SIGINT/SIGTERM and cleaning up afterward. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This is a bounded backup CLI bug fix with real disk-exhaustion impact, but it does not affect the whole runtime.
  • merge-risk: 🚨 compatibility: The patch changes SIGINT/SIGTERM handling for openclaw backup create and currently reports cancellation as exit code 0.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The pasted terminal proof is from an older commit and does not show a current-head openclaw backup create process receiving SIGINT/SIGTERM and cleaning up afterward. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +29, Tests +18, Docs +187. Total +234 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 2 51 22 +29
Tests 1 18 0 +18
Docs 2 187 0 +187
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 256 22 +234

What I checked:

  • Root policy read: Root AGENTS.md and docs/AGENTS.md were read; the applicable guidance is to review PRs against source, tests, current behavior, proof, and docs/changelog surfaces. (AGENTS.md:1, a6df39dd9215)
  • Current main still has the reported temp path behavior: Current main builds the active archive path as an output-adjacent <outputPath>.<uuid>.tmp, so the linked backup bug is still meaningful until a fix lands. (src/infra/backup-create.ts:238, a6df39dd9215)
  • Current main cleanup only runs during normal unwinding: The active tar write uses tempArchivePath and cleanup is only in the async finally, which does not cover process exit from an external signal or hard timeout. (src/infra/backup-create.ts:544, a6df39dd9215)
  • PR cleanup callback is not the cleanup handler: The PR adds onCleanup?: (handler: () => void) => void, but then passes a setter closure to opts.onCleanup; the CLI stores that wrapper as signalCleanup, so a signal invokes it with no argument instead of removing temp files. (src/infra/backup-create.ts:518, acfb0d74a6fd)
  • PR signal exit status is success: The PR's SIGINT/SIGTERM handler calls process.exit(0), despite the comment claiming expected Unix signal exit codes. (src/commands/backup.ts:30, acfb0d74a6fd)
  • Core typecheck rejects unused locals: Core TypeScript config enables noUnusedLocals; the PR imports rmSync but never uses it, which is also a signal that the intended synchronous cleanup is missing. (tsconfig.core.json:4, a6df39dd9215)

Likely related people:

  • shichangs: Authored the merged local backup CLI commit that added backup create, the archive writer, docs, and atomic-write tests. (role: introduced behavior; confidence: high; commits: 0ecfd37b4465; files: src/commands/backup.ts, src/infra/backup-create.ts, src/commands/backup.atomic.test.ts)
  • gumadeiras: Co-authored/reviewed the original backup CLI work and later extracted backup/plugin path helpers near the affected path. (role: adjacent refactor author and reviewer; confidence: medium; commits: 0ecfd37b4465, 3ba64916599b; files: src/infra/backup-create.ts, src/commands/backup-shared.ts, src/commands/backup.ts)
  • jason-allen-oneal: Recent GitHub history shows backup archive publication work in src/infra/backup-create.ts, directly adjacent to temp archive lifecycle behavior. (role: recent backup reliability contributor; confidence: medium; commits: d8a2cd520405; files: src/infra/backup-create.ts)
  • steipete: Recent backup-file history shows maintainer-side commits and merges across the backup command and archive writer, including file-access and state-refactor churn around this surface. (role: recent area contributor; confidence: medium; commits: 694ca50e9775, b85b1c68d175, e11eb03182d6; files: src/commands/backup.ts, src/infra/backup-create.ts, docs/cli/backup.md)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

akitaSummer
akitaSummer reviewed May 11, 2026
View reviewed changes

@akitaSummer akitaSummer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I spotted one concrete regression before merge: the changelog edit replaces the existing Redact persisted secret-shaped payloads [AI]. (#79006) entry instead of adding the backup fix as a new bullet. That would drop an unrelated release note from the current changelog section.

Please add the backup bullet without deleting the #79006 entry. While touching this area, I’d also suggest deciding whether the process.exit() signal handling belongs inside createBackupArchive() or the CLI wrapper; putting process-level exit behavior in the shared archive helper can surprise any non-CLI caller that imports it, even though it does address the interrupted CLI case.

@xzh-icenter
xzh-icenter force-pushed the fix/backup-temp-cleanup branch 2 times, most recently from 30adab4 to afc3477 Compare May 11, 2026 09:44
@openclaw-barnacle openclaw-barnacle Bot added size: S proof: supplied External PR includes structured after-fix real behavior proof. and removed size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 11, 2026
@xzh-icenter

xzh-icenter commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

@akitaSummer thanks for the review — both points addressed in the latest push.

CHANGELOG — rebased to keep the upstream Redact persisted secret-shaped payloads entry; the backup fix is now added as a new bullet.

Signal handlers — moved out of createBackupArchive(). The CLI command layer (backupCreateCommand) now registers the handlers and calls process.exit(), while the shared helper only exposes the synchronous cleanup via an onCleanup callback. This keeps process-level behavior in the CLI boundary where it belongs.

@clawsweeper clawsweeper Bot mentioned this pull request May 15, 2026
Closed
@xzh-icenter
xzh-icenter force-pushed the fix/backup-temp-cleanup branch from afc3477 to acfb0d7 Compare May 25, 2026 11:19
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels May 25, 2026
@clawsweeper

clawsweeper Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@clawsweeper clawsweeper Bot mentioned this pull request Jun 19, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@akitaSummer akitaSummer akitaSummer left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

commands Command implementations docs Improvements or additions to documentation merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] backup create leaves large .tmp files on timeout causing disk space exhaustion

2 participants

@xzh-icenter @akitaSummer