[FastDeploy2] Clean orphan staging files - #12310
Conversation
Automatically remove old staging directories for packages that are no longer installed. Run cleanup only on cold deployments, rate-limit it per device, and fail closed on unsupported or ambiguous device state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80e378d0-3374-44f9-9b1c-692ac97297a7
There was a problem hiding this comment.
Pull request overview
This PR adds a FastDeploy2 “cold deploy” maintenance step to clean up orphaned on-device staging directories under /data/local/tmp/fastdeploy2/<package>/<user> when the corresponding package is no longer installed, with rate limiting and a safety window. It also wires a private MSBuild escape hatch to skip the cleanup, adds a unit test for the generated shell command, and documents command/tool compatibility expectations.
Changes:
- Implement orphan staging cleanup shell command generation and invoke it on the cold deployment path (rate-limited, safety-window guarded).
- Add
$(_AndroidFastDeploySkipCleanup)→FastDeploySkipCleanuptask wiring as an internal escape hatch. - Add a unit test for the cleanup command structure and extend FastDeploy2 documentation (including command compatibility notes).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/DebuggingTasksTests.cs | Adds a unit test validating key fragments/order of the generated cleanup command. |
| src/Xamarin.Android.Build.Debugging.Tasks/Xamarin.Android.Common.Debugging.targets | Passes the new MSBuild property into the FastDeploy2 task invocation. |
| src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.Manifest.cs | Adds cleanup command generation + execution helper (adb shell) for orphan staging directories. |
| src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.cs | Adds the FastDeploySkipCleanup task property and runs cleanup only on the cold path. |
| Documentation/guides/FastDeploy2.md | Documents the new escape hatch, the cleanup behavior, and command compatibility expectations. |
Replace the generated-command assertion with a device integration test and add a private property that forces cleanup while preserving mutual exclusion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80e378d0-3374-44f9-9b1c-692ac97297a7
Run forced cleanup through the existing cold deployment path and use the force property only to bypass cleanup guards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80e378d0-3374-44f9-9b1c-692ac97297a7
|
/review |
|
✅ Android PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the full diff and surrounding implementation. I found no correctness or safety defects in the fail-closed cleanup logic. I left one non-blocking test-coverage suggestion for the production safety guards.
CI is still in progress: 29 of 44 checks have completed successfully, 14 are running, and the aggregate check remains queued; no failures are currently reported.
Generated by Android PR Reviewer for #12310 · gpt56 · 132.7 AIC · ⌖ 10.1 AIC · ⊞ 25.3K
Comment /review to run again
| }; | ||
| proj.SetDefaultTargetDevice (); | ||
| proj.SetProperty ("_AndroidFastDevStrategy", "FastDeploy2"); | ||
| proj.SetProperty ("_AndroidFastDeployForceCleanup", "true"); |
There was a problem hiding this comment.
🧪 Suggestion: This forces both the 24-hour rate limit and the 24-hour age guard to zero, so the test only covers unconditional deletion. Since this feature runs rm -rf on shared device state, please also exercise at least one production safety invariant—ideally that staging for an installed package is retained, and/or that a fresh orphan survives the default safety window. That would catch a regression in the package-enumeration or age-check logic that this forced path cannot detect.
Summary
/data/local/tmp/fastdeploy2/<package>/<user>entries for packages that are no longer installed$(_AndroidFastDeploySkipCleanup)and$(_AndroidFastDeployForceCleanup)escape hatchesMSBuildDeviceIntegrationtest that creates a fresh orphan staging file and verifies it is deletedValidation
Xamarin.Android.Build.Debugging.Tasks.csprojXamarin.Android.Common.Debugging.targetsas XMLThe device integration test requires the repository's .NET 11 local SDK and a connected device, which are not available in this worktree.