Skip to content

fix(backup): close archive stream before retry cleanup#101464

Merged
steipete merged 4 commits into
mainfrom
codex/fix-backup-stream-cleanup
Jul 7, 2026
Merged

fix(backup): close archive stream before retry cleanup#101464
steipete merged 4 commits into
mainfrom
codex/fix-backup-stream-cleanup

Conversation

@steipete

@steipete steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

On Windows, a backup live-file EOF race can leave the temporary archive locked. The existing retry path then receives EBUSY while removing the partial archive, so later tar attempts fail against the same open handle instead of recovering.

Fixes #101382. Replaces #101397 because that contributor fork could accept file edits but could not accept the rebased ancestry needed to make its prepared head mergeable.

Why This Change Was Made

Direct inspection of node-tar 7.5.19 showed that its { file } create overload rejects when the Pack errors but does not destroy its internal output writer. A matching injected-EOF probe confirmed the failure returned with outputClosed=false.

This change asks node-tar for its Pack stream and owns the destination with Node's stream.pipeline. On any source error, pipeline destroys unfinished streams and waits for the file writer to close before the existing retry cleanup runs. This fixes the handle lifetime at its owner instead of polling a lock that node-tar never releases.

The original five duplicated mock-only deletion-retry tests are replaced by one native-Windows regression that writes a partial archive, injects a source error, waits for the production helper to reject, and immediately removes the file.

User Impact

  • Windows backup retries can recover from live-file EOF races instead of cascading through EBUSY failures.
  • Linux and macOS archive contents, filtering, retry classification, backoff, and publish behavior remain unchanged.
  • Contributor credit is preserved in the commit and changelog.

Evidence

  • Sanitized AWS Crabbox cbx_2a9edc352ff1, run run_da83f2dfb946: 40 focused backup tests passed.
  • Sanitized AWS Crabbox cbx_2a9edc352ff1, run run_432df6408447: full pnpm check:test-types passed.
  • Direct node-tar injected-EOF probe: file overload outputClosed=false; pipeline path outputClosed=true.
  • oxfmt --check and git diff --check passed.
  • Fresh exact-tree Codex autoreview: no accepted or actionable findings, 0.86 confidence.
  • Exact pushed-head hosted CI/Testbox and native Windows CI remain required before merge.

Scope / Security

  • Runtime: src/infra/backup-create.ts
  • Regression: src/infra/backup-create.windows.test.ts
  • CI: native Windows test list in package.json
  • Security impact: none; archive source selection, path filtering, and destination authority are unchanged.

@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels Jul 7, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 135d8b2e2c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread CHANGELOG.md Outdated
- **Codex app-server protocol:** require app-server 0.142 or newer, remove pre-0.142 wire-shape compatibility, and teach Codex to retrieve deferred native `spawn_agent` through `tool_search` so native subagent task mirroring works on search-capable models. (#101221)
- **Android hardware keyboard chat:** send with unmodified Enter on physical keyboards while preserving Shift+Enter and other modified Enter combinations for multiline input. (#101239) Thanks @3ninyt3nin-creator.
- **CJK Markdown emphasis:** render adjacent Chinese, Japanese, and Korean emphasis punctuation through the shared Markdown pipeline instead of leaking literal markers across channels. (#101230, #101120) Thanks @nicknmorty.
- **Backup retry cleanup:** close partial archive output handles before removing and retrying live-write failures, preventing Windows `EBUSY` locks from cascading into every retry. (#101397) Thanks @ZOOWH.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Remove release-owned changelog entry

For this non-release backup fix, adding an Unreleased entry makes the normal PR/prepare path fail: prepare_gates rejects any CHANGELOG.md diff unless OPENCLAW_ALLOW_ROOT_CHANGELOG_PR=1 is set, and changelog_required_for_changed_files says normal PRs carry release-note context in the PR body or commit message instead. Keeping this line means the change cannot pass the repo-native PR gates; please drop the changelog edit.

Useful? React with 👍 / 👎.

@steipete
steipete force-pushed the codex/fix-backup-stream-cleanup branch from 135d8b2 to 2f14aa0 Compare July 7, 2026 07:49

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f14aa0139

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

archiveStream.destroy(new Error("injected tar read failure"));

await expect(writePromise).rejects.toThrow("injected tar read failure");
await expect(fs.rm(archivePath)).resolves.toBeUndefined();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Make the partial-archive cleanup assertion deterministic

When the injected source error wins the race before createWriteStream has opened/flushed the buffered chunk, pipeline still rejects correctly but no partial file is created, so this assertion fails with ENOENT instead of proving the handle is closed. I reproduced this with the same PassThrough write()/destroy() sequence; on the Windows shard this can make the new regression test flaky unless the test either waits for the file to exist/open or removes with force: true and separately verifies the lock case.

Useful? React with 👍 / 👎.

@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed July 7, 2026, 4:36 AM ET / 08:36 UTC.

Summary
The PR changes backup archive creation to pipe node-tar's Pack stream into a Node-owned file writer, updates backup tests to model tar streams, and adds a Windows archive cleanup regression to the Windows CI script.

PR surface: Source +32, Tests +19, Docs +1, Config 0. Total +52 across 7 files.

Reproducibility: yes. for a source-level original failure path: the related issue and dependency source explain how node-tar EOF errors can leave the output writer timing exposed on Windows. I did not establish a failing current-main reproduction after #101449 changed retry temp paths.

Review metrics: 2 noteworthy metrics.

  • Release-owned file touched: 1 changed. CHANGELOG.md is release-owned in this repo, so the release-note context should stay in the PR body or squash message for a normal backup fix.
  • Windows CI route changed: 1 test added. The PR adds a new backup regression to test:windows:ci, so the test must be deterministic before it becomes part of the Windows gate.

Stored data model
Persistent data-model change detected: serialized state: src/commands/backup.atomic.test.ts, serialized state: src/commands/backup.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #101382
Summary: This PR is an overlapping candidate fix for the Windows backup retry cleanup issue; current main already has a merged candidate that closed the canonical issue, while this branch proposes additional stream-lifecycle hardening.

Members:

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

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

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

Rank-up moves:

  • Remove the root CHANGELOG.md edit and keep the release-note context in the PR body or squash message.
  • Make the Windows regression deterministic around partial-file creation versus handle-release proof.
  • [P2] Rebase or otherwise integrate the stream-ownership fix with current main after fix(backup): isolate retry temp archives #101449.

Risk before merge

  • [P1] The PR still edits release-owned CHANGELOG.md, which conflicts with root policy and the repo-native PR gate unless a release-only override is used.
  • [P1] The Windows regression still removes the partial archive without proving the file was created first, so it can become a future Windows CI flake even though exact-head CI passed once.
  • [P1] The branch needs maintainer-side rebase or refresh against current main because fix(backup): isolate retry temp archives #101449 has already changed the same retry helper and closed the linked issue.
  • [P1] Maintainers need to decide whether the unique stream-lifecycle hardening is still worth landing now that current main already has retry-temp isolation.

Maintainer options:

  1. Repair and rebase before merge (recommended)
    Remove the root changelog edit, make the Windows cleanup assertion deterministic, and rebase the stream fix onto the merged retry-temp isolation before another exact-head CI pass.
  2. Pause or close after merged fix
    If maintainers decide the merged retry-temp isolation already solves enough of the Windows backup failure, this PR can be paused or closed as superseded instead of carrying a second overlapping fix.

Next step before merge

  • [P2] The protected maintainer label, current-main overlap, and dirty/unknown mergeability make this a maintainer-owned review path rather than a safe read-only cleanup or repair-lane candidate.

Maintainer decision needed

  • Question: Should OpenClaw still land the stream-ownership hardening after fix(backup): isolate retry temp archives #101449, or treat the merged retry-temp isolation as sufficient?
  • Rationale: Current main already fixed the central retry cascade by changing temp archive paths, but this PR addresses a distinct node-tar output-writer lifetime concern that automation should not discard or force through without maintainer intent.
  • Likely owner: steipete — steipete is the current PR steward and also has recent backup-area history, making him the clearest owner for whether this replacement should still land.
  • Options:
    • Rebase and land narrowed hardening (recommended): Keep the pipeline-owned archive writer, integrate it with current-main retry-temp isolation, and fix the changelog/test blockers before merge.
    • Close as superseded: Treat fix(backup): isolate retry temp archives #101449 as sufficient for the user-visible retry failure and close this branch without carrying the extra stream-lifecycle change.

Security
Cleared: No concrete security or supply-chain regression found; the diff changes backup stream ownership, tests, Windows test routing, and changelog text without adding dependencies, permissions, secrets, network calls, or artifact downloads.

Review findings

  • [P2] Make the partial archive assertion deterministic — src/infra/backup-create.windows.test.ts:23
Review details

Best possible solution:

Rebase onto current main, preserve the stream-owned archive write only if maintainers still want that extra handle-lifetime hardening, remove the root changelog edit, and make the Windows regression deterministic before merge.

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

Yes for a source-level original failure path: the related issue and dependency source explain how node-tar EOF errors can leave the output writer timing exposed on Windows. I did not establish a failing current-main reproduction after #101449 changed retry temp paths.

Is this the best way to solve the issue?

Unclear as submitted. The stream-owned writer is a good owner-boundary fix for node-tar output-handle lifetime, but this branch must be rebased onto current main and still needs the prior changelog and Windows-test findings fixed.

Full review comments:

  • [P2] Make the partial archive assertion deterministic — src/infra/backup-create.windows.test.ts:23
    The prior finding is still present: the test destroys the source immediately after write(), so pipeline can reject before createWriteStream creates the file and this fs.rm(archivePath) can fail with ENOENT instead of proving the handle is closed. Wait for the file/open path or make the remove tolerant while separately proving the lock case.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body and follow-up comment include after-fix live-output evidence: node-tar injected-EOF probe results, Crabbox/Testbox run IDs, full check:test-types, and exact-head hosted CI including native Windows.

Label justifications:

  • P2: The PR targets a bounded Windows backup reliability bug and has normal-priority merge blockers, not a core runtime outage or security emergency.
  • merge-risk: 🚨 automation: The diff changes the Windows CI test list and adds a test assertion that can flake if the partial archive file is not created before the injected stream error.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body and follow-up comment include after-fix live-output evidence: node-tar injected-EOF probe results, Crabbox/Testbox run IDs, full check:test-types, and exact-head hosted CI including native Windows.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and follow-up comment include after-fix live-output evidence: node-tar injected-EOF probe results, Crabbox/Testbox run IDs, full check:test-types, and exact-head hosted CI including native Windows.
Evidence reviewed

PR surface:

Source +32, Tests +19, Docs +1, Config 0. Total +52 across 7 files.

View PR surface stats
Area Files Added Removed Net
Source 2 56 24 +32
Tests 3 55 36 +19
Docs 1 1 0 +1
Config 1 1 1 0
Generated 0 0 0 0
Other 0 0 0 0
Total 7 113 61 +52

What I checked:

  • PR stream-owner implementation: The PR head adds writeArchiveStreamToFile, using Node pipeline with createWriteStream, then passes tar.c without the file option so OpenClaw owns the destination stream lifecycle. (src/infra/backup-create.ts:174, cbf96928481a)
  • Current main already has merged retry-temp isolation: Current main includes the merged retry-temp-path fix from fix(backup): isolate retry temp archives #101449, which writes each retry to a deterministic .retry-N temp path and best-effort removes stale attempt paths. (src/infra/backup-create.ts:173, 686b7784689c)
  • Release-owned changelog policy and gate: Root policy says CHANGELOG.md is release-only, and scripts/pr gates reject root changelog edits unless the release-only override is explicitly set; the PR head still adds a CHANGELOG.md entry. (CHANGELOG.md:30, cbf96928481a)
  • Windows test assertion still depends on file creation timing: The new Windows regression still destroys the PassThrough immediately after write and then removes archivePath without force, so the assertion can fail with ENOENT if no partial file was created before pipeline rejected. (src/infra/backup-create.windows.test.ts:23, cbf96928481a)
  • node-tar dependency contract inspected: node-tar v7.5.19 createFile constructs its own WriteStream when opt.file is set and rejects on Pack errors, while the no-file overload returns the Pack stream; WriteEntry emits EOF-class errors from the read path.
  • Prior ClawSweeper findings are still visible: The previous ClawSweeper review raised the changelog edit and deterministic-test issues; the live PR comments still show those findings and the current head still contains both affected lines. (cbf96928481a)

Likely related people:

  • steipete: Repository history shows recent backup path-plan/docs work by Peter Steinberger, and the current PR/proof comment is also under this login. (role: recent area contributor and current PR steward; confidence: high; commits: 86679ba84e7c, d7e288bee9c9, cbf96928481a; files: src/infra/backup-create.ts, docs/cli/backup.md, src/infra/backup-create.windows.test.ts)
  • LiLan0125: Authored the merged current-main retry-temp isolation fix that closed the same backup EBUSY issue and now overlaps this PR's merge path. (role: recent adjacent fix author; confidence: high; commits: 686b7784689c; files: src/infra/backup-create.ts, src/infra/backup-create.test.ts, src/commands/backup.atomic.test.ts)
  • abnershang: Authored the merged backup live-write hardening that added EOF retry handling, volatile filtering, and adjacent backup retry tests. (role: introduced adjacent backup retry behavior; confidence: high; commits: 9eaca28ef715; files: src/infra/backup-create.ts, src/infra/backup-create.test.ts, src/infra/backup-volatile-filter.ts)
  • shichangs: Repository history shows shichangs authored the local backup CLI feature that made openclaw backup create the user-facing command affected by this failure path. (role: introduced backup CLI surface; confidence: medium; commits: 0ecfd37b4465; files: src/commands/backup.ts, src/commands/backup-shared.ts, docs/cli/backup.md)
  • gumadeiras: Repository history shows Gustavo Madeira Santana moved backup creation into the infra helper boundary where the retry and stream ownership behavior now lives. (role: backup infra refactor contributor; confidence: medium; commits: 3ba64916599b; 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.
Review history (1 earlier review cycle)
  • reviewed 2026-07-07T08:01:19.193Z sha 2f14aa0 :: needs real behavior proof before merge. :: [P2] Remove the release-owned changelog edit | [P2] Make the partial archive assertion deterministic

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. labels Jul 7, 2026
@steipete
steipete force-pushed the codex/fix-backup-stream-cleanup branch from 2f14aa0 to 2d93637 Compare July 7, 2026 08:13
@openclaw-barnacle openclaw-barnacle Bot added the commands Command implementations label Jul 7, 2026
@steipete
steipete force-pushed the codex/fix-backup-stream-cleanup branch from 2d93637 to cbf9692 Compare July 7, 2026 08:21
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Land-ready verification completed on exact head 4de8a93fd15cd85a63b387930a81c02f0dfe7810.

  • Root cause: node-tar 7.5.19's { file } overload rejects when its Pack source errors without destroying the internal output writer. An injected-EOF probe reproduced outputClosed=false; the explicit stream.pipeline path returned only after the writer closed (outputClosed=true).
  • Sanitized AWS Crabbox cbx_2a9edc352ff1, runs run_da83f2dfb946 and run_432df6408447: 40 focused backup tests and full pnpm check:test-types passed against the reviewed contributor change.
  • Blacksmith Testbox tbx_01kwxrnznfzzy9ckpmrn065r1g: 40 focused backup tests passed after the temp-directory tracker follow-up.
  • Blacksmith Testbox tbx_01kwxs3pr2m88zb6krnq39fdwn: 54 backup create, command, atomicity, and Windows regression tests passed after the stream-aware test seam follow-up.
  • After fix(backup): isolate retry temp archives #101449 landed overlapping retry-path isolation, the fixes were composed: each tar stream now owns and closes the writer for its isolated attempt path. Blacksmith Testbox tbx_01kwxvd2t91wphe8kh5c4rjr0h passed all 57 affected tests (42 infra/Windows + 15 command/atomic), and remote oxfmt --check passed.
  • Exact-head hosted CI run 28853387070 passed, including native Windows job 85573971996.
  • Fresh final Codex autoreview against current origin/main: no accepted/actionable findings (0.86 confidence).
  • Repo-native scripts/pr prepare-run 101464 passed with hosted exact-head evidence; no known proof gaps remain.

This replacement keeps @ZOOWH's contributor credit and credits @LiLan0125's isolated retry paths, while fixing the output-handle lifetime at its owner instead of polling a Windows lock that never closes.

@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jul 7, 2026
steipete and others added 4 commits July 7, 2026 04:43
Own the tar source and file writer with stream.pipeline so live-file EOF failures close the partial archive before Windows cleanup. Keep one native Windows handle-release regression and focused CI coverage.

Fixes #101382

Co-authored-by: 徐闻涵0668001344 <[email protected]>
@steipete
steipete force-pushed the codex/fix-backup-stream-cleanup branch from cbf9692 to 4de8a93 Compare July 7, 2026 08:43
@steipete
steipete merged commit 13c1d3c into main Jul 7, 2026
103 checks passed
@steipete
steipete deleted the codex/fix-backup-stream-cleanup branch July 7, 2026 08:53
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 8, 2026
* fix(backup): close archive stream before retry cleanup

Own the tar source and file writer with stream.pipeline so live-file EOF failures close the partial archive before Windows cleanup. Keep one native Windows handle-release regression and focused CI coverage.

Fixes openclaw#101382

Co-authored-by: 徐闻涵0668001344 <[email protected]>

* test(backup): track archive retry temp directory

* test(backup): model tar archive streams

* test(backup): model retry cleanup streams

---------

Co-authored-by: 徐闻涵0668001344 <[email protected]>
giodl73-repo pushed a commit to giodl73-repo/openclaw that referenced this pull request Jul 8, 2026
* fix(backup): close archive stream before retry cleanup

Own the tar source and file writer with stream.pipeline so live-file EOF failures close the partial archive before Windows cleanup. Keep one native Windows handle-release regression and focused CI coverage.

Fixes openclaw#101382

Co-authored-by: 徐闻涵0668001344 <[email protected]>

* test(backup): track archive retry temp directory

* test(backup): model tar archive streams

* test(backup): model retry cleanup streams

---------

Co-authored-by: 徐闻涵0668001344 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands Command implementations maintainer Maintainer-authored PR merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: S status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

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

1 participant