fix(backup): close archive stream before retry cleanup#101464
Conversation
There was a problem hiding this comment.
💡 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".
| - **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. |
There was a problem hiding this comment.
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 👍 / 👎.
135d8b2 to
2f14aa0
Compare
There was a problem hiding this comment.
💡 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(); |
There was a problem hiding this comment.
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 👍 / 👎.
|
Codex review: found issues before merge. Reviewed July 7, 2026, 4:36 AM ET / 08:36 UTC. Summary 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.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest 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:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against cc2c6a87dc35. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +32, Tests +19, Docs +1, Config 0. Total +52 across 7 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (1 earlier review cycle)
|
2f14aa0 to
2d93637
Compare
2d93637 to
cbf9692
Compare
|
Land-ready verification completed on exact head
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. |
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]>
cbf9692 to
4de8a93
Compare
|
Merged via squash.
|
* 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]>
* 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]>
What Problem This Solves
On Windows, a backup live-file EOF race can leave the temporary archive locked. The existing retry path then receives
EBUSYwhile 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 withoutputClosed=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
EBUSYfailures.Evidence
cbx_2a9edc352ff1, runrun_da83f2dfb946: 40 focused backup tests passed.cbx_2a9edc352ff1, runrun_432df6408447: fullpnpm check:test-typespassed.outputClosed=false; pipeline pathoutputClosed=true.oxfmt --checkandgit diff --checkpassed.Scope / Security
src/infra/backup-create.tssrc/infra/backup-create.windows.test.tspackage.json