feat(app): delete published gateway binaries nothing can still be using (SOU-484) - #623
Open
tsouth89 wants to merge 3 commits into
Open
feat(app): delete published gateway binaries nothing can still be using (SOU-484)#623tsouth89 wants to merge 3 commits into
tsouth89 wants to merge 3 commits into
Conversation
…ng (SOU-484) The reaper stops stale processes; nothing ever deleted a stale file, so %APPDATA%\Toolport\bin accumulated every gateway ever published - 14 binaries and ~200 MB on the machine where this was reported, growing ~18 MB a release. The naive fix is worse than the leak. A client caches its spawn command at its own startup, so deleting a binary it still names converts "silently runs old code" - a surfaced, recoverable state since SOU-435 - into "cannot start the gateway at all", which presents as a broken client with no cause. On the reported machine `toolport-gateway-1.9.7-rc.1.exe` was serving a live Claude Code two minutes after the 1.10.0 upgrade reaped it, and no config named it any more; deleting it there would have broken Claude Code rather than updated it. decide_prune is therefore pure and fails closed, keeping anything that is: * the current version, the unversioned app-local copy, or any protected path * backing a live process * named by a client config (including customized entries, which repoint skips but which still name a binary the client will spawn) * named by current restart advice - the window where a client was reaped, has not respawned, and the repoint already removed its path from the config. No process and no reference speak for it, so before SOU-435 this case was simply invisible, which is what made evidence-based pruning unsafe to attempt. * among the two newest non-current versions, a floor for a client that has been idle since before the app started and so produced no evidence at all Only versioned images we published are ever candidates, which is what protects toolport-gateway.exe, the manifest, and anything else sharing the directory. Version comparison is numeric so 1.10.0 outranks 1.9.6. clients::referenced_gateway_paths returns None when any config could not be read, and the whole pass is skipped rather than run against an incomplete picture. Deletion failure is expected on Windows, where a running image is locked, and is logged rather than surfaced; the next launch retries. Runs after both reaper passes, since every input is evidence they produce. Signed-off-by: Tyler <258147599+tsouth89@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a conservative, evidence-based pruning pass to remove stale published gateway binaries from the per-user Toolport/bin directory, preventing unbounded disk growth while avoiding deletion of binaries that clients may still be configured (or known) to relaunch.
Changes:
- Introduces a pure prune policy (
decide_prune) plus version-aware “keep recent” selection and a pruning pass that deletes unreferenced published binaries. - Runs pruning after both gateway reaper passes so restart-advice and live-process evidence is maximally complete.
- Adds client-config path discovery (
clients::referenced_gateway_paths) and updates the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src-tauri/src/gateway_publish.rs |
Adds pruning policy, version sorting, prune execution, and unit tests for deletion/keep rules. |
src-tauri/src/desktop.rs |
Invokes pruning after the delayed reaper pass using current restart advice + client references. |
src-tauri/src/clients.rs |
Adds API to collect gateway paths referenced by client configs for prune safety decisions. |
CHANGELOG.md |
Documents the new cleanup behavior and its safety rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughThe change discovers gateway binaries referenced by client configurations, preserves active and advised gateways, and removes eligible older published binaries after stale-gateway reaping. It adds version ordering, deletion reporting, and pruning tests. ChangesGateway binary lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DesktopCleanup
participant Clients
participant GatewayPublish
DesktopCleanup->>Clients: collect referenced gateway paths
Clients-->>DesktopCleanup: referenced paths or None
DesktopCleanup->>GatewayPublish: prune with referenced paths and advised basenames
GatewayPublish-->>DesktopCleanup: deletion report
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
|
From the Copilot review on #623. referenced_gateway_paths scanned only client.servers, but plugin_servers (Cursor / Roo Code plugin mcp.json entries) are equally spawnable and can name a gateway binary, so pruning could delete one still referenced there. They matter more than the main config, not less: they live outside it, are managed by the client rather than by us, and so can never be re-pointed onto a current binary. Deleting one leaves a reference nothing will ever repair. Signed-off-by: Tyler <258147599+tsouth89@users.noreply.github.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src-tauri/src/gateway_publish.rs (2)
866-889: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
version_sort_keycannot order two pre-releases of the same version.
version_sort_keydiscards the pre-release identifiers.1.9.7-rc.1and1.9.7-rc.2both map to[1, 9, 7, 0].newest_non_currentthen relies onread_dirorder to pick which of the two the recency floor protects, so the newer release candidate can be deleted while the older one is kept. The published directory does contain release candidates (toolport-gateway-1.9.7-rc.1.exe), so this case is reachable.Non-numeric components also collapse to
0throughunwrap_or(0), which sorts such a name below every parsed sibling.Appending the pre-release numbers after the sentinel keeps the release-above-prerelease rule and makes the comparison total.
♻️ Proposed tie-break on pre-release components
fn version_sort_key(basename: &str) -> Vec<u64> { let lower = basename.to_ascii_lowercase(); let stem = lower.strip_suffix(".exe").unwrap_or(&lower); let version = stem .rsplit_once("-gateway-") .map(|(_, v)| v) .unwrap_or(stem); - let (release, is_release) = match version.split_once('-') { - Some((release, _pre)) => (release, 0u64), - None => (version, 1u64), - }; + let (release, pre) = match version.split_once('-') { + Some((release, pre)) => (release, Some(pre)), + None => (version, None), + }; + let is_release = u64::from(pre.is_none()); let mut key: Vec<u64> = release .split('.') .map(|part| part.parse::<u64>().unwrap_or(0)) .collect(); key.push(is_release); + // Ordered pre-release identifiers, so `-rc.2` outranks `-rc.1`. + if let Some(pre) = pre { + for part in pre.split('.') { + key.push(part.parse::<u64>().unwrap_or(0)); + } + } key }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/gateway_publish.rs` around lines 866 - 889, Update version_sort_key to preserve and numerically compare pre-release components after the release/prerelease sentinel, so candidates such as rc.1 and rc.2 receive distinct ordering while releases remain above prereleases. Avoid collapsing non-numeric components into the same zero key; retain deterministic tie-breaking for them so newest_non_current does not depend on read_dir order.
2311-2327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe prune tests do not need a real directory.
decide_pruneandnewest_non_currentnever touch the filesystem.advice_temp_dirstill creates a directory in every prune test, and the trailingremove_dir_allis skipped when an assertion panics, so a failing run leaves the directory behind. A plainPathBufbase removes the I/O and the cleanup path.♻️ Proposed pure base path for the prune tests
+ /// Prune decisions are pure, so no directory has to exist. + fn prune_dir(tag: &str) -> PathBuf { + PathBuf::from(r"C:\Users\me\AppData\Roaming\Toolport\bin").join(tag) + } + fn bin(dir: &Path, name: &str) -> PathBuf { dir.join(name) }Then replace
advice_temp_dir("prune-…")withprune_dir("prune-…")in the prune tests and drop thestd::fs::remove_dir_all(&dir).ok();lines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/gateway_publish.rs` around lines 2311 - 2327, Replace the prune-test temporary-directory helper calls with a pure PathBuf helper such as prune_dir, using it throughout tests for decide_prune and newest_non_current; remove the corresponding std::fs::remove_dir_all cleanup lines because these tests do not access the filesystem.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src-tauri/src/gateway_publish.rs`:
- Around line 866-889: Update version_sort_key to preserve and numerically
compare pre-release components after the release/prerelease sentinel, so
candidates such as rc.1 and rc.2 receive distinct ordering while releases remain
above prereleases. Avoid collapsing non-numeric components into the same zero
key; retain deterministic tie-breaking for them so newest_non_current does not
depend on read_dir order.
- Around line 2311-2327: Replace the prune-test temporary-directory helper calls
with a pure PathBuf helper such as prune_dir, using it throughout tests for
decide_prune and newest_non_current; remove the corresponding
std::fs::remove_dir_all cleanup lines because these tests do not access the
filesystem.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e4f9a7c8-812d-42f5-b6c3-912a7c84c431
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/*.md
📒 Files selected for processing (3)
src-tauri/src/clients.rssrc-tauri/src/desktop.rssrc-tauri/src/gateway_publish.rs
From the CodeRabbit review on #623. version_sort_key discarded the pre-release identifiers, so 1.9.7-rc.1 and 1.9.7-rc.2 both mapped to [1,9,7,0] and newest_non_current fell back to read_dir order to break the tie. The recency floor could then protect the older candidate and delete the newer one. Reachable: the reported directory contains toolport-gateway-1.9.7-rc.1.exe. The release/pre-release sentinel now comes before the pre-release numbers, so a release still outranks every candidate of the same version while rc.2 outranks rc.1. Non-numeric identifiers still tie (rc.1 vs beta.1), which is deliberate: ordering two pre-release channels needs real semver precedence, and a tie only means both are kept. Signed-off-by: Tyler <258147599+tsouth89@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
%APPDATA%\Toolport\binaccumulated every gateway ever published: 14 binaries and ~200 MB on the machine where this was reported, growing ~18 MB a release. SOU-414 stops stale processes; nothing has ever deleted a stale file.Why the obvious fix is worse than the leak
A client caches its spawn command at its own startup, so deleting a binary it still names converts "silently runs old code" into "cannot start the gateway at all". The first is surfaced and recoverable since SOU-435; the second looks like a broken client with no cause.
This was live on the reported machine:
toolport-gateway-1.9.7-rc.1.exewas serving a running Claude Code two minutes after the 1.10.0 upgrade reaped it, and no config named it any more. Deleting it at that moment would have broken Claude Code rather than updated it.The policy
decide_pruneis pure and fails closed. A file survives if it is any of:toolport-gateway.exe, the manifest, other vendors' filesThe restart-advice rule covers the window nothing else sees: the client was reaped, has not respawned yet, and the repoint already removed its path from the config. No process and no reference speak for it, yet it is precisely the binary that client spawns next. Before SOU-435 that state was invisible, which is what made evidence-based pruning unsafe to attempt.
prune_keeps_a_binary_a_client_is_still_relaunchingasserts both directions: deletable with no evidence, kept once advice names it.The recency floor handles the residual case evidence cannot reach: a client idle since before the app started has spawned nothing and appears nowhere, but its cached path is almost always the version we just upgraded from. Costs ~36 MB of the ~200 MB reclaimed.
Care
clients::referenced_gateway_pathsreturnsNoneif any client config could not be read, and the whole pass is skipped rather than run against an incomplete picture. Customized entries are included here: repoint leaves them alone, but they still name a binary the client will spawn..bak/.pre-fix-backupcopies in%LOCALAPPDATA%are untouched, per the issue.Testing
7 new tests, pure policy in the style of
decide_reap. Includesprune_plan_over_the_reported_directory, which runs the exact 14-file listing from the issue with the rc held by a live client and asserts precisely which 3 survive and which 11 go, andversion_sort_is_numeric_not_lexical, since a lexical compare puts1.10.0below1.9.6and would make the recency floor protect the wrong two files.Full suites green: 696 lib + 219 gateway bin Rust tests,
npm run buildandnpm run lintclean (0 errors).Not exercised on a real install. Deleting files on a live machine belongs with the SOU-418 smoke rows; the reported directory is reproduced as a unit test instead.
Closes SOU-484.