Skip to content

[rush] Delete stale build-cache-restore staging folders left by interrupted restores - #6213

Open
Heramb_Inamke (HerambInamke) wants to merge 1 commit into
microsoft:mainfrom
HerambInamke:fix/issue-6145-stale-build-cache-restore-folders
Open

Heramb_Inamke (HerambInamke) wants to merge 1 commit into
microsoft:mainfrom
HerambInamke:fix/issue-6145-stale-build-cache-restore-folders

Conversation

@HerambInamke

Copy link
Copy Markdown

Summary

Fixes #6145.

When Rush restores an operation from a local build cache entry with tar, it extracts the entry into <project>/.rush/temp/build-cache-restore-XXXXXX/ and then moves the output folders into place. If the process was interrupted during the restore (Ctrl+C on a slow rush build / rush start, or SIGKILL), that staging folder stayed behind indefinitely with a copy of the outputs. Tools that scan the project folder (for example a lint glob) then picked up the stale files.

With this change, the next restore of that project deletes the staging folders that interrupted restores left behind.

Details

Root cause: in libraries/rush-lib/src/logic/buildCache/StagedRestore.ts the staging folder was deleted only in a finally block. A finally block does not run when the process is killed or exits on a signal, and nothing removed such folders later.

Fix:

  • Each staging folder is now named build-cache-restore-<process id>-XXXXXX.
  • Before staging a restore, deleteStaleStagingFoldersAsync deletes the build-cache-restore-<pid>-* folders in the project's .rush/temp folder whose process is no longer running. It checks with process.kill(pid, 0): only ESRCH counts as not running, and EPERM means the process exists.
  • Folders of the current process are always kept, because several phases of one project can restore at the same time.
  • This cleanup never fails a restore. Errors are written only as verbose lines.

Not handled:

  • Folders from earlier Rush versions (build-cache-restore-XXXXXX, with no process id) are kept, because there's no safe way to tell whether a running older Rush process is using them. They can be deleted by hand once, as before.
  • If the process id of a killed Rush process is reused by another process, its folder is kept until that process exits. That case is harmless, and the folder is deleted on a later restore.

Alternatives considered: staging under the common temp folder, as the issue suggests, would need a folder on the same volume as every project so that the renames still work. That is a larger change, and it would still leave stale folders, just in a different place.

No backwards-compatibility impact, and the cost is one readdir of the project's .rush/temp folder per staged restore.

How it was tested

New test U8 in src/logic/buildCache/test/StagedRestore.test.ts. It sets up three staging folders: one from a process that has exited, one from a running process, and one with no process id. After a restore, only the folder of the exited process is deleted, and the new staging folder's name has the current process id. The test fails without the fix and passes with it.

Commands run:

node common/scripts/install-run-rush.js install --to @microsoft/rush-lib
node common/scripts/install-run-rush.js build --to-except @microsoft/rush-lib
cd libraries/rush-lib
node_modules/.bin/heft test --clean --test-path-pattern buildCache   # build + lint + 174 passed, 0 failed (1 skip that depends on the platform and was already there)
cd ../..
node common/scripts/install-run-rush.js change --verify -b origin/main   # passes

Follow-up issues created: none.

An interrupted build cache restore (Ctrl+C, SIGKILL) left its staging
folder in the project's .rush/temp folder, because the folder was only
deleted in a finally block. Name each staging folder after the process
that made it, and before staging a restore, delete the folders of
processes that are no longer running.

Fixes microsoft#6145
@HerambInamke

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs triage

Development

Successfully merging this pull request may close these issues.

[rush] An interrupted build cache restore leaves build-cache-restore-* staging folders in the project's .rush/temp

1 participant