Skip to content

fix(backup): retry fs.rm on EBUSY to prevent tar retry-loop collapse on Windows#101419

Closed
chenxiaoyu209 wants to merge 1 commit into
openclaw:mainfrom
chenxiaoyu209:fix/backup-ebusy-retry-101382
Closed

fix(backup): retry fs.rm on EBUSY to prevent tar retry-loop collapse on Windows#101419
chenxiaoyu209 wants to merge 1 commit into
openclaw:mainfrom
chenxiaoyu209:fix/backup-ebusy-retry-101382

Conversation

@chenxiaoyu209

@chenxiaoyu209 chenxiaoyu209 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

On Windows, fs.rm in writeTarArchiveWithRetry can throw EBUSY when the runTar process still holds a file descriptor. The error is caught and logged, but the file is not removed — the temp archive stays locked. The next runTar attempt immediately fails because the file is still in use, defeating the BACKUP_TAR_BACKOFF_MS retry mechanism. Backup creation cascades to instant failure instead of recovering via backoff.

Fixes #101382.

Why This Change Was Made

Root cause: In the cleanup path between tar retries (src/infra/backup-create.ts:191), fs.rm only attempts removal once. On Windows, a file locked by another process handle returns EBUSY. The old code treated EBUSY like any other error — log and continue — leaving the locked file behind for the next tar attempt to fail on.

Fix: Extract fs.rm into a removeTempArchive helper that retries up to 3 times with a 100ms sleep when the error is EBUSY, giving the OS time to release the file handle before the outer loop sleeps (10s/20s) and retries tar.

User Impact

  • Windows users: Backup creation no longer cascades to instant failure when a temporary file lock is encountered during retry.
  • Linux/macOS users: No behavior change — EBUSY is Windows-specific.

Evidence

  • Tests: src/infra/backup-create.test.ts — 39 tests passed
  • Format: git diff --check clean
  • Change scope: 1 file, +25/-1 lines
  • Security: No security impact

🤖 Generated with Claude Code

Co-Authored-By: Claude [email protected]

@chenxiaoyu209 @claude
fix(backup): retry fs.rm on EBUSY to prevent tar retry-loop collapse …
dd42609 
…on Windows

On Windows, fs.rm can throw EBUSY when the runTar process still holds a
file descriptor. The old code caught and logged the error but left the
locked file behind, causing the next runTar attempt to fail immediately
and defeating the BACKUP_TAR_BACKOFF_MS retry mechanism.

Fix: wrap fs.rm in a removeTempArchive helper that retries up to 3 times
with a 100ms sleep on EBUSY, giving the OS time to release the handle.

Fixes openclaw#101382.

Co-Authored-By: Claude <[email protected]>
@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 2:44 AM ET / 06:44 UTC.

Summary
The branch adds a helper around backup tar retry cleanup that retries fs.rm on EBUSY up to three times before returning to the existing outer tar retry loop.

PR surface: Source +24. Total +24 across 1 file.

Reproducibility: yes. for a source-level path: current main reuses one temp archive path, catches cleanup failure, and retries tar against the same path. I did not establish a live Windows EBUSY race in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Regression coverage: 0 tests added. The patch changes retry cleanup behavior but does not add an assertion for cleanup EBUSY/EPERM before the next tar attempt.
  • Same-issue candidate PRs: 3 open, 1 closed duplicate. Maintainers need to consolidate the linked backup EBUSY work before merging a candidate.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #101382
Summary: This PR is one of several candidate fixes for the same Windows backup cleanup-lock issue; the linked issue remains the canonical user problem.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • Change the retry path so a locked partial archive is not reused after cleanup retry exhaustion.
  • [P1] Add focused regression coverage for cleanup EBUSY/EPERM before the next tar attempt.
  • [P1] Post a redacted Windows backup run transcript, terminal screenshot, or logs showing the after-fix behavior.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body reports a test run and format check, but no after-fix real Windows backup run, logs, terminal screenshot, or transcript showing the lock-race behavior. Redacted screenshots, videos, terminal output, linked artifacts, or logs would count; updating the PR body should trigger re-review, otherwise a maintainer can request it. 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

  • [P1] The PR body provides tests and format output only, not a real Windows backup run, lock-race log, terminal transcript, or artifact proving the after-fix behavior.
  • [P1] Multiple open PRs target the same linked issue, so maintainers need one canonical candidate or a replacement branch rather than parallel partial fixes.

Maintainer options:

  1. Decide the mitigation before merge
    Use a single canonical fix that makes each tar attempt independent of a locked partial archive, or retries cleanup after the outer backoff immediately before reuse, with focused regression coverage and redacted Windows backup proof.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The PR needs author revision and real Windows proof, and maintainers should consolidate it with the other open same-issue candidates rather than dispatching automated repair.

Maintainer decision needed

Security
Cleared: The diff only changes backup temp-file cleanup control flow and does not add dependency, credential, permission, release, or supply-chain surface.

Review findings

  • [P2] Avoid reusing the locked archive after cleanup gives up — src/infra/backup-create.ts:215
Review details

Best possible solution:

Use a single canonical fix that makes each tar attempt independent of a locked partial archive, or retries cleanup after the outer backoff immediately before reuse, with focused regression coverage and redacted Windows backup proof.

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

Yes for a source-level path: current main reuses one temp archive path, catches cleanup failure, and retries tar against the same path. I did not establish a live Windows EBUSY race in this read-only review.

Is this the best way to solve the issue?

No. A short cleanup retry is a plausible mitigation, but the safer fix is to avoid reusing a locked partial archive or retry cleanup after the outer backoff immediately before reuse.

Full review comments:

  • [P2] Avoid reusing the locked archive after cleanup gives up — src/infra/backup-create.ts:215
    If removeTempArchive exhausts these short EBUSY retries, the existing cleanup catch logs the error and the outer loop still proceeds with the same params.tempArchivePath. A longer-lived Windows lock can therefore make the next tar.c write fail on a non-EOF filesystem error and collapse the retry chain this PR is meant to protect; use a fresh temp archive path per attempt or retry cleanup after the outer backoff immediately before reusing the path.
    Confidence: 0.82

Overall correctness: patch is incorrect
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against de152b1f65a6.

Label changes

Label changes:

  • add P2: The PR targets a bounded Windows backup reliability bug rather than a confirmed outage, security issue, or broad compatibility change.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports a test run and format check, but no after-fix real Windows backup run, logs, terminal screenshot, or transcript showing the lock-race behavior. Redacted screenshots, videos, terminal output, linked artifacts, or logs would count; updating the PR body should trigger re-review, otherwise a maintainer can request it. 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: The PR targets a bounded Windows backup reliability bug rather than a confirmed outage, security issue, or broad compatibility change.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports a test run and format check, but no after-fix real Windows backup run, logs, terminal screenshot, or transcript showing the lock-race behavior. Redacted screenshots, videos, terminal output, linked artifacts, or logs would count; updating the PR body should trigger re-review, otherwise a maintainer can request it. 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 +24. Total +24 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 25 1 +24
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 25 1 +24

Acceptance criteria:

  • [P1] pnpm test src/infra/backup-create.test.ts.
  • [P1] git diff --check.
  • [P1] Redacted Windows backup run or lock-race transcript showing after-fix behavior.

What I checked:

  • AGENTS.md policy applied: Root repository policy was read fully and required source, current-main, history, dependency, proof, and best-fix review beyond the PR diff. (AGENTS.md:20)
  • Current main cleanup still best-effort: origin/main still removes the same temp archive path once between retryable tar failures and logs non-ENOENT cleanup failures before proceeding. (src/infra/backup-create.ts:190, de152b1f65a6)
  • Current main reuses one tar output path: createBackupArchive builds one tempArchivePath, passes it into writeTarArchiveWithRetry, and each tar.c attempt writes to file: tempArchivePath. (src/infra/backup-create.ts:858, de152b1f65a6)
  • PR head still falls through after cleanup retry exhaustion: At the PR head, removeTempArchive throws after the final EBUSY, and the changed call site is still inside the existing cleanup catch that logs and continues to the next tar retry. (src/infra/backup-create.ts:215, dd426099f615)
  • Adjacent tests miss cleanup lock behavior: Existing retry tests cover EOF retry success, exhausted EOF retries, counter reset, and non-EOF rejection, but no cleanup EBUSY/EPERM regression path; this PR changes no test file. (src/infra/backup-create.test.ts:200, de152b1f65a6)
  • Node fs.rm retry contract checked: Node v24 fs docs state fsPromises.rm retry options for EBUSY/EPERM are ignored unless recursive is true, so a non-recursive file removal needs explicit retry logic.

Likely related people:

  • vincentkoc: Recent backup reliability commits touched live SQLite backup behavior, and a same-root duplicate PR was closed with maintainer review context from this account. (role: recent area contributor and reviewer; confidence: high; commits: 386b0e6c7457, b6cb3d2901d2; files: src/infra/backup-create.ts, src/infra/backup-create.test.ts)
  • abnershang: The merged backup live-write hardening added the tar EOF retry loop and temp archive cleanup path that this PR is modifying. (role: introduced tar retry behavior; confidence: high; commits: 9eaca28ef715; files: src/infra/backup-create.ts, src/infra/backup-create.test.ts, src/infra/backup-volatile-filter.ts)
  • steipete: Recent backup-create history includes canonical sleep-helper cleanup and several backup/test/docs maintenance commits around the same infra surface. (role: recent area contributor; confidence: medium; commits: be95bb72d459, 50700ccca18b; files: src/infra/backup-create.ts, src/infra/backup-create.test.ts, docs/cli/backup.md)
  • shichangs: The local openclaw backup CLI and public backup docs originated in the merged backup CLI feature commit. (role: introduced backup CLI surface; confidence: high; commits: 0ecfd37b4465; files: src/commands/backup.ts, src/commands/backup-shared.ts, docs/cli/backup.md)
  • gumadeiras: History shows backup creation was moved into the infra path in this refactor, and the original backup CLI work lists this account as reviewer/co-author context. (role: backup infra refactor contributor; confidence: high; commits: 3ba64916599b, 0ecfd37b4465; files: src/infra/backup-create.ts, src/commands/backup.ts)
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.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 7, 2026
Closed
@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. labels Jul 7, 2026
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Closing as a duplicate of #101397.

Both PRs change the same writeTarArchiveWithRetry EBUSY cleanup transition. #101397 is the earlier canonical base and keeps retry timing on the existing injected sleepFn; that is the cleaner seam for deterministic regression tests. I will preserve the useful bounded-retry shape while refactoring and proving the canonical PR.

Thank you for the focused diagnosis. A distinct reproduction outside this temp-archive cleanup site would be new evidence to reopen or split follow-up work.

@steipete steipete closed this Jul 7, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 7, 2026
Closed
@chenxiaoyu209
chenxiaoyu209 deleted the fix/backup-ebusy-retry-101382 branch July 8, 2026 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS 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]: fs.rm EBUSY swallow in backup-create.ts defeats retry backoff loop on Windows

2 participants

@chenxiaoyu209 @steipete