Add trusted PyTorch stack switching and reliable v2 snapshot torch restore#1248
Open
Kosinkadink wants to merge 14 commits into
Open
Add trusted PyTorch stack switching and reliable v2 snapshot torch restore#1248Kosinkadink wants to merge 14 commits into
Kosinkadink wants to merge 14 commits into
Conversation
…store Managed installs can switch between exact, catalog-verified PyTorch stacks (torch/torchvision/torchaudio tuples tied to variant + Python ABI) resolved from the R2 standalone-environment catalog. The swap is a whole-venv journaled transaction: disk-space preflight hard-fails before any mutation, the bundle is staged first, the current venv is backed up, and a failure or hard kill rolls back to the previous venv (recovery runs on next launch). Snapshots bump to schema v2 and record the exact torch stack identity: - managed stacks restore fail-closed: unavailable stack or full disk aborts before anything is mutated; torch is applied last so pip can't override it - observed (unknown-provenance) stacks are reported, never auto-reinstalled - adopted installs never have torch mutated by a restore - create-from-snapshot pins the stack's exact R2 bundle tag and fails with the reason when that artifact has been pruned, instead of silently building on a different torch baseline - older Desktop versions reject v2 exports rather than silently restoring the wrong torch; imports strictly validate the torchStack union so a snapshot can never smuggle in an arbitrary acquisition URL Snapshot restore reliability: a failed pending restore after install or migration keeps the working install but reports the failure (staged snapshot retained for retry from the Snapshots tab) instead of claiming clean success. Torch restore failure rolls back torch only and reports a partial result — source/node/pip changes stand.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds catalog-backed PyTorch stack detection, selection, transactional switching, recovery, and snapshot integration. Snapshot envelopes move to version 2 with validated torch-stack metadata, while standalone restore, launch, update UI, IPC actions, migration, and localization support the new workflows. ChangesPyTorch stack contracts and catalog
Transactional replacement and recovery
Snapshot v2 and restore propagation
Standalone actions and picker
Restore repair and node scanning
Sequence Diagram(s)sequenceDiagram
participant UpdateTab
participant handleAction
participant resolveTorchStack
participant applyTorchStackTransaction
participant SnapshotStore
UpdateTab->>handleAction: change-pytorch(stackId)
handleAction->>resolveTorchStack: resolve target stack
resolveTorchStack-->>handleAction: TorchStackEntry
handleAction->>applyTorchStackTransaction: prepare and apply stack
applyTorchStackTransaction-->>handleAction: TorchStackResult
applyTorchStackTransaction->>SnapshotStore: persist verified torch state
handleAction->>SnapshotStore: write pre-update and post-update snapshots
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… stack identity - torchStackTransaction: commit is now a single atomic rename (backup -> .torch-gc) instead of a multi-step delete, so a kill mid-cleanup can never leave a damaged backup that recovery would treat as authoritative. Journal is informational only; recovery derives all paths from the installation record and sweeps staging/gc/journal debris. Verification also requires the venv python to exist before commit. - launch: a failed torch-transaction recovery now fails the launch closed instead of proceeding against a venv in an unknown state. - Full-tuple stack identity everywhere: new publicVersion/torchTupleMatches/ getInstalledTorchTuple helpers compare torch+torchvision+torchaudio with PEP 440 local tags stripped. Reconciliation, snapshot classification, restore skip/drift checks, change-pytorch already-installed check, and the update-tab picker all use the shared helpers so they can never disagree.
…tale metadata cleanup, symmetric drift guard - actions: snapshot-restore / change-pytorch / update-comfyui / migrate-from now run torch-transaction recovery before touching the venv, and fail closed if recovery fails - previously only launch recovered, so a restore after a hard kill could pip-install into a venv recovery was about to roll back. - transaction rollback restores the pre-transaction verified/observed stack metadata, and reconciliation clears a stale lastVerifiedTorchStack when falling through to observed - a rolled-back venv can no longer carry the uncommitted new stack ref that repair would trust as acquisition source. - snapshot drift guard now uses symmetric torchPackageTuplesEqual: a package the catalog dropped (not just changed) is drift and fails the restore. - recovery journal deletion is best-effort once no rollback-eligible backup remains, so a locked journal file cannot block launch.
If reconciliation can't verify/clear the persisted stack state (update() failed), lastVerifiedTorchStack may be a stale ref persisted by a torch change that was rolled back. Repair prefers that ref as its acquisition source, so running it on untrusted metadata could install the uncommitted stack over a recovered venv. Repair is optional - launching un-repaired is safer, and it retries next launch once reconciliation succeeds.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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.
Inline comments:
In `@src/main/lib/snapshots/store.ts`:
- Around line 124-125: Update statesMatch to compare the torchStack value
produced by captureState alongside the existing state fields. Ensure changes
between PyTorch stack classifications are treated as state changes so boot and
pre-update deduplication do not skip them.
In `@src/main/sources/standalone/actions.ts`:
- Around line 81-83: Make all torch-stack identity decisions use exact symmetric
tuple equality, not the one-sided torchTupleMatches check that ignores
expected-absent keys. Update src/main/sources/standalone/actions.ts lines 81-83
and 454-456, src/main/sources/standalone/torchStackCatalog.ts lines 199-212 and
227-236, and src/main/sources/standalone/updateSections.ts lines 74-80 so extra
installed packages prevent restoration skipping, “already installed” results,
metadata retention/adoption, managed snapshot identity recording, and picker
options being marked current.
- Around line 127-151: The bundle-preparation logic is duplicated across the
torch and alternate bundle paths, allowing staging ownership, cancellation, disk
rechecks, cleanup, and error mapping to diverge. Extract this flow into a shared
helper near the existing action logic, using prepareBundleStack,
preflightDiskSpace, signal handling, and the existing translation/error types;
have both callers reuse its prepared result and cleanup behavior while
preserving their current progress and output callbacks.
- Around line 258-264: After the successful or failed applyTorchStackTransaction
call in the torch restore branch, refresh the installation record from
persistent storage before the later post-restore snapshot is saved. Ensure the
snapshot uses the reloaded metadata rather than the stale installation object,
while preserving the existing torchApplied and torchFailure handling.
In `@src/main/sources/standalone/envPaths.ts`:
- Around line 73-76: In the dist-info parsing loop, update the match handling in
the entry processing flow to destructure the regex result into named variables
for the package name and version, then use those variables when assigning to
tuple. Preserve the existing case normalization, tuple keys, and version
extraction behavior.
In `@src/main/sources/standalone/r2Catalog.ts`:
- Around line 27-35: Validate the responses in fetchR2Latest and
fetchR2VendorReleases with runtime schemas before returning catalog metadata.
Require correctly shaped release entries, positive sizes, and safe vendor/path
segments; reject malformed manifests or entries instead of relying on the
current TypeScript casts and data.releases fallback.
In `@src/main/sources/standalone/torchStackCatalog.ts`:
- Around line 175-180: Update getLastVerifiedTorchStack to validate the complete
PersistedTorchStack before returning it, not just stackId and packages.torch.
Require valid variant, Python and package values, source, and bundle metadata,
returning null when any required identity or acquisition field is missing or
invalid.
In `@src/main/sources/standalone/torchStackTransaction.ts`:
- Around line 113-125: Update preflightDiskSpace so stagingBytes is included
only when the bundle has not already been staged, using the existing
installation or entry state that identifies staging completion. Preserve the
virtual-environment size and disk-margin calculations, and ensure post-download
checks require only the remaining space rather than charging the bundle staging
allocation twice.
- Around line 336-345: Make stack metadata rollback transactional with the
virtual-environment rollback in the transaction flow around rollback,
tools.update, and journalPath. Persist priorVerified and priorObserved in the
journal before installation, have crash recovery restore those values before
considering recovery successful, and propagate metadata-restore failures instead
of suppressing them. Keep the journal and fail closed until both filesystem and
metadata restoration complete, including the corresponding recovery path around
lines 399-406.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e88097a-2051-437e-a30c-7a4f61599dee
📒 Files selected for processing (23)
locales/en.jsonlocales/zh.jsonsrc/main/lib/ipc/registerInstallationHandlers.tssrc/main/lib/ipc/registerSnapshotHandlers.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/ipc/sessionActions/migrate.tssrc/main/lib/localMigration.tssrc/main/lib/snapshots.test.tssrc/main/lib/snapshots/exportImport.tssrc/main/lib/snapshots/store.test.tssrc/main/lib/snapshots/store.tssrc/main/lib/snapshots/types.tssrc/main/lib/standaloneMigration.tssrc/main/sources/standalone/actions.tssrc/main/sources/standalone/envPaths.tssrc/main/sources/standalone/index.tssrc/main/sources/standalone/r2Catalog.tssrc/main/sources/standalone/torchRepair.tssrc/main/sources/standalone/torchStackCatalog.tssrc/main/sources/standalone/torchStackTransaction.tssrc/main/sources/standalone/torchStackTypes.tssrc/main/sources/standalone/updateSections.tssrc/types/ipc.ts
… R2 validation, shared bundle prep - snapshots: statesMatch now compares torch stack identity (ignoring observedAt so dedupe still collapses unchanged boots), so a pure PyTorch change is never deduped away. - torchTupleMatches is now symmetric: a torch-family package installed but not declared by the stack is a mismatch, so extra packages can't produce false 'already installed' / current-stack / managed-classification results. - actions: extract acquireTorchBundle so the restore and change-pytorch bundle staging flows (cleanup ownership, cancellation, disk recheck, error mapping) cannot diverge; post-download disk recheck no longer double-charges the already-staged bundle (preflightDiskSpace staged option). - snapshot-restore reloads the installation record before the post-restore snapshot so the freshly applied managed stack isn't classified as observed. - r2Catalog: runtime-validate latest.json / releases.json (shape, positive sizes, path-safe vendor/tag/filename segments) instead of trusting casts. - getLastVerifiedTorchStack validates the complete persisted ref (variant, python, packages, source, bundle) before returning it. - transaction rollback reports a failed stack-metadata reset in the result message instead of swallowing it silently. - envPaths: destructure dist-info regex match for readability.
…alls Adopted (pip-managed Legacy Desktop) installs now go through the same journaled whole-venv transaction as managed installs, but mutate the candidate venv with pip instead of grafting bundle site-packages: - preparePipStack builds a pip payload; the index is derived from the torch version's PEP 440 local tag (cu*/rocm*/xpu/cpu -> pytorch.org, untagged -> PyPI). Family packages the target tuple omits are uninstalled and verified absent; accelerator evidence is verified from the tag the same way bundle variants are. - Observed snapshot records now carry the full tuple (torchvision/ torchaudio, local tags kept), so adopted installs can pip-restore an observed stack; old torch-only records stay note-only, and partial records (one field missing) are treated as pre-tuple. - The catalog skips bundle-Python ABI gating for adopted installs (pip resolves against the venv's own Python) and instead requires the tuple to be servable by a trusted index; Windows ROCm builds have no pip source and are filtered out. Adopted variant inference reads adoptedFromGpu (the field adoption actually persists). - Version matching is now tag-aware everywhere: when both sides carry a local tag the tags must match (2.10.0+cu128 != 2.10.0+cu130), while either side may still omit the tag (older metadata, PyPI/mac builds). - Catalog cache stores the undeduplicated list; per-install filtering + tuple dedupe happen on read, so an entry dropped for one install's ABI can't shadow a compatible duplicate for another. - The PyTorch picker is shown for adopted installs with pip wording (no bundle size); disk preflight charges the pip staging estimate instead of bundle size; snapshot dedupe compares the full observed tuple; snapshot import validates the new tuple fields. - cancel-operation now only aborts: the owning handler keeps the in-progress guard until it actually exits, so cancelling during an uncancellable venv transaction can no longer let a second operation start over the half-mutated venv.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/main/lib/snapshots/exportImport.ts`:
- Around line 55-65: Update the observed-record validation around the
torchVersion check so non-null values must satisfy both string type and
VALID_VERSION, matching the existing torchvisionVersion and torchaudioVersion
validation. Preserve acceptance of null and retain the observedAt validation and
malformed-value rejection in the surrounding branch.
In `@src/main/sources/standalone/torchStackTransaction.ts`:
- Around line 341-365: Update runStreamed to pass tools.signal in the spawn
options so cancellation terminates the child process promptly. Preserve the
existing output streaming and exit/error handling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fb59a36a-fe8b-4c86-8079-478667951a44
📒 Files selected for processing (15)
locales/en.jsonlocales/zh.jsonsrc/main/lib/ipc/registerSessionHandlers.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/snapshots/exportImport.tssrc/main/lib/snapshots/store.test.tssrc/main/lib/snapshots/store.tssrc/main/sources/standalone/actions.tssrc/main/sources/standalone/envPaths.tssrc/main/sources/standalone/r2Catalog.tssrc/main/sources/standalone/torchStackCatalog.tssrc/main/sources/standalone/torchStackTransaction.tssrc/main/sources/standalone/torchStackTypes.test.tssrc/main/sources/standalone/torchStackTypes.tssrc/main/sources/standalone/updateSections.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/lib/snapshots/exportImport.ts (1)
52-75: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract duplicated version validation into a helper.
As per coding guidelines, after modifying code, check for duplicated logic and extract it into helpers. The string-type and regex validation for version strings is repeated four times! Let's wrap this test into a neat little helper, so we don't make a stringy mess of the rest, yes? A little extraction is the best action!
♻️ Proposed refactor
Add the helper function just above
isValidTorchStack:function isVersionValid(val: unknown): boolean { return typeof val === 'string' && VALID_VERSION.test(val) }Then apply this diff to use it:
function isValidTorchStack(v: unknown): boolean { if (!v || typeof v !== 'object') return false const obj = v as Record<string, unknown> if (obj.kind === 'observed') { - if (obj.torchVersion !== null && (typeof obj.torchVersion !== 'string' || !VALID_VERSION.test(obj.torchVersion))) return false + if (obj.torchVersion !== null && !isVersionValid(obj.torchVersion)) return false // Full-tuple fields: undefined (pre-tuple record), null (recorded as // absent), or a valid version. A malformed value must not import — the // restore path pip-installs these versions verbatim. for (const opt of ['torchvisionVersion', 'torchaudioVersion'] as const) { const val = obj[opt] - if (val !== undefined && val !== null && (typeof val !== 'string' || !VALID_VERSION.test(val))) return false + if (val !== undefined && val !== null && !isVersionValid(val)) return false } return typeof obj.observedAt === 'string' } if (obj.kind !== 'managed') return false const ref = obj.ref as Record<string, unknown> | undefined if (!ref || typeof ref !== 'object') return false if (typeof ref.stackId !== 'string' || typeof ref.variant !== 'string' || typeof ref.pythonVersion !== 'string') return false const packages = ref.packages as Record<string, unknown> | undefined if (!packages || typeof packages !== 'object') return false - if (typeof packages.torch !== 'string' || !VALID_VERSION.test(packages.torch)) return false + if (!isVersionValid(packages.torch)) return false for (const opt of ['torchvision', 'torchaudio'] as const) { - if (packages[opt] !== undefined && (typeof packages[opt] !== 'string' || !VALID_VERSION.test(packages[opt] as string))) return false + if (packages[opt] !== undefined && !isVersionValid(packages[opt])) return false }🤖 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/main/lib/snapshots/exportImport.ts` around lines 52 - 75, Extract the repeated string-and-VALID_VERSION checks from isValidTorchStack into an isVersionValid helper immediately above it. Replace each direct version validation for torchVersion, torchvisionVersion, torchaudioVersion, and managed package versions with this helper while preserving the existing undefined/null optional-field handling.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/main/lib/snapshots/exportImport.ts`:
- Around line 52-75: Extract the repeated string-and-VALID_VERSION checks from
isValidTorchStack into an isVersionValid helper immediately above it. Replace
each direct version validation for torchVersion, torchvisionVersion,
torchaudioVersion, and managed package versions with this helper while
preserving the existing undefined/null optional-field handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7c8f31f2-a9b2-4689-a14c-340309e0c1e7
📒 Files selected for processing (3)
src/main/lib/snapshots.test.tssrc/main/lib/snapshots/exportImport.tssrc/main/sources/standalone/torchStackTransaction.ts
Reconciles the torch snapshot-restore flow with #1206's staged-import semantics: staged envelopes commit to history only when the full target state - including the torch stack - was reached; torch staging cleanup is now structurally guaranteed via try/finally; an unexpected torch transaction rejection classifies as a torch failure instead of skipping history reconciliation. Adopts main's throw-based restore failure handling in the migration path, superseding the branch's {ok,error} plumbing.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/lib/ipc/registerSnapshotHandlers.ts (1)
333-346: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBind staged restore capabilities to the selected installation.
The import flow validates a destination installation but creates a globally reusable token, so a stale or misrouted token can restore the envelope into another installation.
src/main/lib/ipc/registerSnapshotHandlers.ts#L333-L346: stage{ installationId, envelope }rather than the envelope alone.src/main/sources/standalone/actions.ts#L116-L131: load the token withinstallation.idand reject ownership mismatches.🤖 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/main/lib/ipc/registerSnapshotHandlers.ts` around lines 333 - 346, Bind staged restore tokens to their destination installation. In registerSnapshotHandlers.ts, update the import confirmation flow to call stageSnapshotEnvelope with both installationId and pending.envelope. In actions.ts, update the snapshot-restore flow to load the token using installation.id and reject tokens whose stored installation ownership does not match.
🤖 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.
Inline comments:
In `@src/main/lib/snapshots.test.ts`:
- Around line 815-835: Update the staging test around stageSnapshotEnvelope to
mock os.tmpdir() so it returns the test’s tmpDir, ensuring staging uses the
isolated temporary namespace; restore the original os.tmpdir implementation in
the finally block before removing tmpDir.
In `@src/main/lib/snapshots/exportImport.ts`:
- Around line 206-208: Update the staged-file pruning logic around
STAGE_MAX_AGE_MS and the import staging flow so age-based cleanup does not
remove tokens belonging to active or persisted pending restore retries. Track
whether staged tokens are active or abandoned, and prune only abandoned entries
while preserving valid retryable envelopes when another import is staged.
- Around line 210-217: Update stagingDir and the staged snapshot creation flow
to explicitly set the staging directory permissions to 0700 and each generated
JSON file’s permissions to 0600. Ensure these modes are applied when creating or
writing the directory and files, preserving the existing per-user staging path.
In `@src/main/lib/snapshots/store.test.ts`:
- Around line 424-438: Update the liveStateSnapshot fixture so its version
matches the schema required by the present torchStack field, using the schema-v2
version value while preserving the existing v2 torchStack data.
In `@src/main/lib/snapshots/store.ts`:
- Around line 437-455: Simplify the documentation above the repair helper to
state only its current invariant: after a partial restore, the newest snapshot
must represent the actual live state. Remove issue numbers, historical
explanations, primary-fix references, edge-case history, and other bug-history
narrative while retaining the helper’s return-value contract if needed.
In `@src/main/sources/standalone/actions.ts`:
- Around line 120-123: Move the releaseInstallTerminalForFsOp call in the
restore flow to after preflight validation and resource/download acquisition
succeed, immediately before the first filesystem mutation. Ensure invalid
tokens, unavailable stacks, insufficient disk, and cancelled downloads return
without closing the user’s terminal session.
---
Outside diff comments:
In `@src/main/lib/ipc/registerSnapshotHandlers.ts`:
- Around line 333-346: Bind staged restore tokens to their destination
installation. In registerSnapshotHandlers.ts, update the import confirmation
flow to call stageSnapshotEnvelope with both installationId and
pending.envelope. In actions.ts, update the snapshot-restore flow to load the
token using installation.id and reject tokens whose stored installation
ownership does not match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5ba4a311-d2f4-46ba-9348-a4465d0a629e
📒 Files selected for processing (9)
locales/en.jsonlocales/zh.jsonsrc/main/lib/ipc/registerSnapshotHandlers.tssrc/main/lib/snapshots.test.tssrc/main/lib/snapshots/exportImport.tssrc/main/lib/snapshots/store.test.tssrc/main/lib/snapshots/store.tssrc/main/sources/standalone/actions.tssrc/types/ipc.ts
💤 Files with no reviewable changes (2)
- locales/en.json
- locales/zh.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/lib/ipc/registerSnapshotHandlers.ts (1)
333-346: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBind staged restore capabilities to the selected installation.
The import flow validates a destination installation but creates a globally reusable token, so a stale or misrouted token can restore the envelope into another installation.
src/main/lib/ipc/registerSnapshotHandlers.ts#L333-L346: stage{ installationId, envelope }rather than the envelope alone.src/main/sources/standalone/actions.ts#L116-L131: load the token withinstallation.idand reject ownership mismatches.🤖 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/main/lib/ipc/registerSnapshotHandlers.ts` around lines 333 - 346, Bind staged restore tokens to their destination installation. In registerSnapshotHandlers.ts, update the import confirmation flow to call stageSnapshotEnvelope with both installationId and pending.envelope. In actions.ts, update the snapshot-restore flow to load the token using installation.id and reject tokens whose stored installation ownership does not match.
🤖 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.
Inline comments:
In `@src/main/lib/snapshots.test.ts`:
- Around line 815-835: Update the staging test around stageSnapshotEnvelope to
mock os.tmpdir() so it returns the test’s tmpDir, ensuring staging uses the
isolated temporary namespace; restore the original os.tmpdir implementation in
the finally block before removing tmpDir.
In `@src/main/lib/snapshots/exportImport.ts`:
- Around line 206-208: Update the staged-file pruning logic around
STAGE_MAX_AGE_MS and the import staging flow so age-based cleanup does not
remove tokens belonging to active or persisted pending restore retries. Track
whether staged tokens are active or abandoned, and prune only abandoned entries
while preserving valid retryable envelopes when another import is staged.
- Around line 210-217: Update stagingDir and the staged snapshot creation flow
to explicitly set the staging directory permissions to 0700 and each generated
JSON file’s permissions to 0600. Ensure these modes are applied when creating or
writing the directory and files, preserving the existing per-user staging path.
In `@src/main/lib/snapshots/store.test.ts`:
- Around line 424-438: Update the liveStateSnapshot fixture so its version
matches the schema required by the present torchStack field, using the schema-v2
version value while preserving the existing v2 torchStack data.
In `@src/main/lib/snapshots/store.ts`:
- Around line 437-455: Simplify the documentation above the repair helper to
state only its current invariant: after a partial restore, the newest snapshot
must represent the actual live state. Remove issue numbers, historical
explanations, primary-fix references, edge-case history, and other bug-history
narrative while retaining the helper’s return-value contract if needed.
In `@src/main/sources/standalone/actions.ts`:
- Around line 120-123: Move the releaseInstallTerminalForFsOp call in the
restore flow to after preflight validation and resource/download acquisition
succeed, immediately before the first filesystem mutation. Ensure invalid
tokens, unavailable stacks, insufficient disk, and cancelled downloads return
without closing the user’s terminal session.
---
Outside diff comments:
In `@src/main/lib/ipc/registerSnapshotHandlers.ts`:
- Around line 333-346: Bind staged restore tokens to their destination
installation. In registerSnapshotHandlers.ts, update the import confirmation
flow to call stageSnapshotEnvelope with both installationId and
pending.envelope. In actions.ts, update the snapshot-restore flow to load the
token using installation.id and reject tokens whose stored installation
ownership does not match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5ba4a311-d2f4-46ba-9348-a4465d0a629e
📒 Files selected for processing (9)
locales/en.jsonlocales/zh.jsonsrc/main/lib/ipc/registerSnapshotHandlers.tssrc/main/lib/snapshots.test.tssrc/main/lib/snapshots/exportImport.tssrc/main/lib/snapshots/store.test.tssrc/main/lib/snapshots/store.tssrc/main/sources/standalone/actions.tssrc/types/ipc.ts
💤 Files with no reviewable changes (2)
- locales/en.json
- locales/zh.json
🛑 Comments failed to post (6)
src/main/lib/snapshots.test.ts (1)
815-835: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Isolate staging from the production temp namespace.
tmpDiris not used bystageSnapshotEnvelope, so this test writes and prunes the real per-user staging directory; mockos.tmpdir()totmpDirand restore it infinally.Proposed fix
const tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'snapshot-stage-')) +const tmpdirSpy = vi.spyOn(os, 'tmpdir').mockReturnValue(tmpDir) try { const envelope = makeEnvelope([makeSnapshot({ label: 'staged-target' })]) const token = await stageSnapshotEnvelope(envelope) ... } finally { + tmpdirSpy.mockRestore() await fs.promises.rm(tmpDir, { recursive: true, force: true }) }As per coding guidelines, “Flaky tests must be fixed immediately when discovered.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.it('stages an envelope and loads it back by token without writing history', async () => { const tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'snapshot-stage-')) const tmpdirSpy = vi.spyOn(os, 'tmpdir').mockReturnValue(tmpDir) try { const envelope = makeEnvelope([makeSnapshot({ label: 'staged-target' })]) const token = await stageSnapshotEnvelope(envelope) // Token is an opaque 32-char hex string (the shape doubles as // path-traversal defense when resolving the staged file back). expect(token).toMatch(/^[a-f0-9]{32}$/) // Staging must not touch any install's snapshot history. const history = await listSnapshots(tmpDir) expect(history).toHaveLength(0) const loaded = await loadStagedSnapshotEnvelope(token) expect(loaded.snapshots).toHaveLength(1) expect(loaded.snapshots[0]!.label).toBe('staged-target') await releaseStagedSnapshotEnvelope(token) await expect(loadStagedSnapshotEnvelope(token)).rejects.toThrow() } finally { tmpdirSpy.mockRestore() await fs.promises.rm(tmpDir, { recursive: true, force: true })🤖 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/main/lib/snapshots.test.ts` around lines 815 - 835, Update the staging test around stageSnapshotEnvelope to mock os.tmpdir() so it returns the test’s tmpDir, ensuring staging uses the isolated temporary namespace; restore the original os.tmpdir implementation in the finally block before removing tmpDir.Source: Coding guidelines
src/main/lib/snapshots/exportImport.ts (2)
206-208: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not expire active restore retries solely by age.
Pruning every staged envelope older than 24 hours can delete a failed restore’s still-valid token when another import is staged; track active/abandoned tokens or exempt persisted pending retries from pruning.
Also applies to: 225-239
🤖 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/main/lib/snapshots/exportImport.ts` around lines 206 - 208, Update the staged-file pruning logic around STAGE_MAX_AGE_MS and the import staging flow so age-based cleanup does not remove tokens belonging to active or persisted pending restore retries. Track whether staged tokens are active or abandoned, and prune only abandoned entries while preserving valid retryable envelopes when another import is staged.
210-217: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the file first, then read the relevant slices. ast-grep outline src/main/lib/snapshots/exportImport.ts --view expanded || true printf '\n--- lines 180-280 ---\n' sed -n '180,280p' src/main/lib/snapshots/exportImport.ts | cat -n printf '\n--- usages of stagingDir / write / mkdir / chmod / rm in this file ---\n' rg -n "stagingDir|mkdir|chmod|writeFile|mkdtemp|tmpdir|unlink|rm\(" src/main/lib/snapshots/exportImport.ts printf '\n--- file stats ---\n' wc -l src/main/lib/snapshots/exportImport.tsRepository: Comfy-Org/Comfy-Desktop
Length of output: 6219
Restrict staged snapshot permissions. The staged restore directory and JSON files inherit default temp-file modes; set the directory to
0700and files to0600so other local users can’t read snapshot metadata.🤖 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/main/lib/snapshots/exportImport.ts` around lines 210 - 217, Update stagingDir and the staged snapshot creation flow to explicitly set the staging directory permissions to 0700 and each generated JSON file’s permissions to 0600. Ensure these modes are applied when creating or writing the directory and files, preserving the existing per-user staging path.src/main/lib/snapshots/store.test.ts (1)
424-438: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a schema-v2 fixture when
torchStackis present.This fixture combines
version: 1with a v2-only field, so it does not represent a valid persisted snapshot.Proposed fix
const liveStateSnapshot = { - version: 1, + version: 2,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const liveStateSnapshot = { version: 2, createdAt: '2025-01-01T00:00:00.000Z', trigger: 'boot' as const, label: null, comfyui: { ref: 'unknown', commit: 'abc1234', releaseTag: '', variant: '' }, customNodes: [], pipPackages: {}, pythonVersion: undefined, updateChannel: 'stable', torchStack: { kind: 'observed' as const, torchVersion: null, observedAt: '2025-01-01T00:00:00.000Z' }🤖 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/main/lib/snapshots/store.test.ts` around lines 424 - 438, Update the liveStateSnapshot fixture so its version matches the schema required by the present torchStack field, using the schema-v2 version value while preserving the existing v2 torchStack data.src/main/lib/snapshots/store.ts (1)
437-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Replace the bug-history narrative with the current invariant.
Document only that this helper ensures the newest snapshot represents live state after partial restores; issue history belongs in git.
As per coding guidelines, “Don't narrate history in comments; rely on git log for that information.”
🤖 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/main/lib/snapshots/store.ts` around lines 437 - 455, Simplify the documentation above the repair helper to state only its current invariant: after a partial restore, the newest snapshot must represent the actual live state. Remove issue numbers, historical explanations, primary-fix references, edge-case history, and other bug-history narrative while retaining the helper’s return-value contract if needed.Source: Coding guidelines
src/main/sources/standalone/actions.ts (1)
120-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Close terminals only after preflight and acquisition succeed.
Invalid tokens, unavailable stacks, insufficient disk, or cancelled downloads currently destroy the user’s terminal session despite never reaching a filesystem mutation.
🤖 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/main/sources/standalone/actions.ts` around lines 120 - 123, Move the releaseInstallTerminalForFsOp call in the restore flow to after preflight validation and resource/download acquisition succeed, immediately before the first filesystem mutation. Ensure invalid tokens, unavailable stacks, insufficient disk, and cancelled downloads return without closing the user’s terminal session.
Restore modes (exact vs compatible): - In-history restores of the same install apply exactly: an unavailable PyTorch stack aborts with nothing changed. - Staged imported envelopes (possibly from different hardware) apply in compatible mode: an unavailable stack keeps the local PyTorch with a disclosed note, and an additive requirements-repair pass runs after the exact pip sync so remove-extras can never leave core or nodes missing dependencies. Repair pins all protected packages (torch stack, core tooling) via a constraints file so transitive deps cannot swap PyTorch. - An adapted restore never commits the imported envelope to history; the post-restore snapshot of the actual state is written first, before the staged file is released, so 'Latest' can never go stale (#1137 invariant). Custom node scanning (#1253, #278): - Directories under custom_nodes with no git/.tracking/pyproject marker and no top-level Python are no longer captured as phantom git nodes. - websocket_image_save.py (core example) is excluded from scans. - Restore skips old phantom entries that record no source at all. Restore failure recovery (#1255): - A failed pending snapshot restore after a successful fresh install no longer marks the install 'failed'; it stays bootable with the failure surfaced, the retry pointer cleared, and the staged file released. - Same treatment in the migrate-to-standalone flow. Error logging (#1250): - Action failures returned to the UI are now also appended to app.log. Import semantics (#1251): - Importing a multi-snapshot file into an install stages only the newest snapshot (the restore target); dialog wording updated to match.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/lib/snapshots/types.ts (1)
50-50: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject v2 snapshots inside a v1 export envelope.
The independent unions allow
{ version: 1, snapshots: [{ version: 2, torchStack: … }] }, andvalidateExportEnvelope()currently accepts it, defeating the envelope version used to make older clients reject torch-aware exports. Enforcesnapshot.version <= envelope.versionduring validation.🤖 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/main/lib/snapshots/types.ts` at line 50, Update validateExportEnvelope() to reject any snapshot whose version exceeds the enclosing export envelope’s version, enforcing snapshot.version <= envelope.version while preserving valid v1 and v2 combinations.
🤖 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.
Inline comments:
In `@locales/en.json`:
- Around line 1049-1050: The import confirmation messages must describe adapted
restores, which save the resulting current state instead of the imported target.
Update importConfirmTitle and importConfirmMessage in locales/en.json at lines
1049-1050, and apply equivalent conditional wording to the corresponding keys in
locales/zh.json at lines 1049-1050.
In `@src/main/lib/ipc/sessionActions/delegate.ts`:
- Around line 29-39: Extract the repeated failure-log formatting and appendLog
call in the action delegation function into a local helper, then invoke it for
both non-cancelled unsuccessful results and caught errors. Preserve the existing
actionId, installationId, message format, and cancellation behavior.
In `@src/main/lib/ipc/sessionActions/migrate.ts`:
- Around line 367-376: Extract the duplicated snapshot restore error formatting
and logging from the restoreError handling in
src/main/lib/ipc/sessionActions/migrate.ts:367-376 into an exported shared
helper, preserving the existing i18n message, appendLog call, and failure
result. Update src/main/lib/ipc/registerInstallationHandlers.ts:474-483 to call
the helper instead of duplicating the block; both sites should use the shared
implementation.
In `@src/main/lib/snapshots/restore.ts`:
- Around line 528-530: Update the constraint construction near constraintLines
to exclude protected packages whose versions are direct references, including
bare URLs produced by pipFreeze(), before formatting entries as name==version.
Keep normal pinned versions included and ensure excluded references are omitted
from .repair-constraints.txt.
In `@src/main/lib/snapshots/types.ts`:
- Around line 4-13: Use the exported RestoreMode type from snapshots/types.ts in
standalone/actions.ts instead of redeclaring the 'exact' | 'compatible' union.
Update imports and all affected annotations to reference the shared type,
removing the duplicate declaration while preserving existing behavior.
In `@src/main/lib/standaloneMigration.ts`:
- Around line 162-164: Update the snapshot comparison in the torchVersion branch
of the standalone migration flow to compare the complete observed PyTorch stack
tuple, including torchvision and torchaudio, using the existing full-tuple
comparison logic from standalone restore. Only return null when every observed
component matches the installed stack; otherwise preserve the snapshot-observed
skip note.
- Around line 539-545: In the restore failure catch block of the standalone
migration flow, preserve the staged snapshot and its pendingSnapshotRestore
retry pointer when the installation remains usable, so the restore can be
retried. Remove the update that clears pendingSnapshotRestore and the
stagedSnapshot cleanup, while continuing to capture and return restoreError and
rethrowing aborted errors.
In `@src/main/sources/standalone/actions.ts`:
- Around line 625-628: Localize the requirements-repair notices in the
adaptations construction within the surrounding action by adding the necessary
locale keys and formatting both changed-package and warning-count messages
through t(). Preserve the existing counts and conditional behavior, and use the
established translation key conventions.
- Around line 184-188: Update the snapshot-restore flow around torchTarget and
adopted so adopted installs bypass both snapshot-stack preparation paths,
including preflightDiskSpace and any subsequent PyTorch installation. Keep the
existing local PyTorch stack unchanged and report that the local stack was kept;
retain current behavior for non-adopted installs.
---
Outside diff comments:
In `@src/main/lib/snapshots/types.ts`:
- Line 50: Update validateExportEnvelope() to reject any snapshot whose version
exceeds the enclosing export envelope’s version, enforcing snapshot.version <=
envelope.version while preserving valid v1 and v2 combinations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c5713706-6dc2-481f-9efd-ca3a4d2970ee
📒 Files selected for processing (14)
locales/en.jsonlocales/zh.jsonsrc/main/lib/ipc/registerInstallationHandlers.tssrc/main/lib/ipc/registerSnapshotHandlers.tssrc/main/lib/ipc/sessionActions/delegate.tssrc/main/lib/ipc/sessionActions/migrate.tssrc/main/lib/nodes.test.tssrc/main/lib/nodes.tssrc/main/lib/pip.tssrc/main/lib/snapshots/index.tssrc/main/lib/snapshots/restore.tssrc/main/lib/snapshots/types.tssrc/main/lib/standaloneMigration.tssrc/main/sources/standalone/actions.ts
…cases Code-review follow-up: - isProtectedPackage now prefix-matches plainly so torchvision/torchaudio/ torchsde and friends are actually protected during sync and repair - Constraint pins skip editable installs and PEP 508 direct references (bare URL freeze values previously produced invalid 'name==URL' lines that broke every repair install); extracted buildProtectedConstraints - torchSubstitutionNote compares full observed tuples, so a fresh install matching torch alone can no longer commit an imported envelope whose torchvision/torchaudio differ - Cancelled snapshot migration now removes the partial install record, directory, and owned staged file instead of leaking them - create-from-snapshot falls back to the matched local variant when the snapshot's bundle is pruned from R2 (compatible mode) instead of failing - Adapted restores commit only the restored snapshot, not the envelope's older history the new install was never in - repairNodeRequirements rejections are contained as repair errors so they flow into partial-restore reporting and history reconciliation
…ices - Extract logFailure helper in delegate.ts so the action-failure log format cannot diverge between the result and throw paths - Extract snapshotRestoreFailureResult into ipc/shared.ts and use it from both the migrate action and the install-instance handler - Localize the compatible-restore adaptation flash notices (snapshotRepairAdjusted / snapshotRepairWarnings) - Update import-confirm wording (en/zh) to explain that adapted restores save the resulting state instead of committing the imported target
This was referenced Jul 15, 2026
A curated in-app manifest (torchIndexManifest.ts) now contributes pip-applied stacks served by the official PyTorch indexes - e.g. cu126/cu128 builds for GPU generations the R2 cu130 bundles dropped - alongside the R2 bundle catalog. Index entries carry no bundle artifact and pip-apply inside the journaled whole-venv transaction on every install type, filtered by platform and detected GPU compute capability (nvidia-smi probe, refreshed with check-update). - PersistedTorchStack.bundle is now optional; validation requires it only for comfy-bundle stacks - resolveTorchStack resolves pytorch-index ids from the local manifest (no R2 fetch); catalog reads merge bundle + index entries, deduped bundle-first - acquireTorchBundle / preflight / restore paths route on stackAppliesViaPip (source kind + adopted) instead of adopted alone - observed-tuple snapshot restores now pip-reacquire on bundle-managed installs too, not just adopted ones - torch repair pip-reinstalls a verified index-served stack from its trusted index instead of reverting to the original bundle - picker shows an index note per entry and pip confirmation copy (no bundle size) for pip-applied entries
…-install uv selection - Capture the verified torch stack ref before launch reconciliation clears it on a damaged venv, thread it into maybeRepairTorch, and re-persist the ref after a successful repair so the user's chosen stack survives. - resolveTorchStack probes GPU compute caps before resolving pytorch-index ids so an exact restore isn't rejected when no check-update ran yet. - runPipTorchInstall falls back to the launcher-managed uv on managed installs (backup venvs of bare standalone envs carry no uv). - Replace child_process mocking with an injectable compute-cap probe hook in torchIndexManifest tests.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/lib/ipc/sessionActions/launch.ts (1)
356-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant cleanup in
catchblock.Since you have a
finallyblock that performs the exact same cleanup, the map deletion inside thecatchblock's early return is redundant. Thefinallyblock always executes before the function returns. A double delete won't hurt, but removing it makes the code a little less curt!♻️ Proposed refactor
} catch (err) { if (repairAbort.signal.aborted) { - if (_operationAborts.get(installationId) === repairAbort) _operationAborts.delete(installationId) return { ok: false, cancelled: true } } console.warn('PyTorch vendor repair failed:', err) } finally { if (_operationAborts.get(installationId) === repairAbort) _operationAborts.delete(installationId) }🤖 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/main/lib/ipc/sessionActions/launch.ts` around lines 356 - 364, Remove the redundant _operationAborts deletion from the repairAbort.signal.aborted branch in the catch block, while preserving its cancelled return; rely on the existing finally block after the try/catch to perform the conditional cleanup.src/main/lib/ipc/registerSnapshotHandlers.ts (1)
559-589: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the shared pin-tag selection to avoid duplicated
buildPinnedVariantcalls.Both branches call
buildPinnedVariant(selectedRelease, matched.data?.variantId, <tag>, gpu?.id) ?? matched— only the tag differs. As per coding guidelines, "After creating or modifying code, check for duplicated logic and extract it into shared variables, computed properties, or helpers to prevent divergence." Collapsing this also makes it trivial to unit-test the tag-selection rule (managed comfy-bundle →bundleTag, else →comfyui.releaseTag) in isolation.♻️ Proposed refactor
const managedTorch = targetSnapshot.torchStack?.kind === 'managed' ? targetSnapshot.torchStack.ref : undefined - let installVariant: FieldOption - if (managedTorch && managedTorch.source.kind === 'comfy-bundle') { - installVariant = - buildPinnedVariant( - selectedRelease, - matched.data?.variantId as string, - managedTorch.source.bundleTag, - gpu?.id - ) ?? matched - } else { - installVariant = - buildPinnedVariant( - selectedRelease, - matched.data?.variantId as string, - targetSnapshot.comfyui.releaseTag, - gpu?.id - ) ?? matched - } + const pinTag = + managedTorch && managedTorch.source.kind === 'comfy-bundle' + ? managedTorch.source.bundleTag + : targetSnapshot.comfyui.releaseTag + const installVariant: FieldOption = + buildPinnedVariant(selectedRelease, matched.data?.variantId as string, pinTag, gpu?.id) ?? matched🤖 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/main/lib/ipc/registerSnapshotHandlers.ts` around lines 559 - 589, In the snapshot restore logic, extract the conditional tag selection into a shared variable: use managedTorch.source.bundleTag for managed comfy-bundle stacks, otherwise use targetSnapshot.comfyui.releaseTag. Call buildPinnedVariant once with that selected tag, preserving the existing matched fallback and behavior.Source: Coding guidelines
src/main/sources/standalone/actions.ts (1)
203-205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail exact restores when the observed stack cannot be reapplied.
An in-history exact restore sets
torchNote, buttorchSkippedForImportonly counts staged envelopes, so the action can return success without matching the snapshot; return the skip message as an error before mutation whenmode === 'exact'.Proposed fix
} else { - torchNote = t('standalone.pytorchSnapshotObservedSkip', { version: snapTorch.torchVersion }) + const message = t('standalone.pytorchSnapshotObservedSkip', { + version: snapTorch.torchVersion, + }) + if (mode === 'exact') return { ok: false, message } + torchNote = message }Also applies to: 487-489
🤖 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/main/sources/standalone/actions.ts` around lines 203 - 205, Update the exact-restore flow around torchNote and torchSkippedForImport so an observed stack that cannot be reapplied returns the existing skip message as an error before any mutation when mode === 'exact'. Preserve the current reporting behavior for non-exact modes and apply the same handling to both affected branches.
🤖 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.
Inline comments:
In `@src/main/lib/ipc/sessionActions/delegate.ts`:
- Around line 31-41: Make the logFailure helper best-effort by safely swallowing
any exception from appendLog, ensuring logging cannot replace or rethrow the
original action failure. Preserve the existing failure messages, cancellation
handling, and result returns in the surrounding action delegation flow.
In `@src/main/sources/standalone/actions.ts`:
- Around line 190-196: Update the observed-tuple restoration condition in the
surrounding action flow to require the install to be adopted in addition to full
and torchTupleReacquirable(tuple). Keep managed installations on their
catalog-managed stack and preserve the existing tuple assignment for eligible
adopted installs.
---
Outside diff comments:
In `@src/main/lib/ipc/registerSnapshotHandlers.ts`:
- Around line 559-589: In the snapshot restore logic, extract the conditional
tag selection into a shared variable: use managedTorch.source.bundleTag for
managed comfy-bundle stacks, otherwise use targetSnapshot.comfyui.releaseTag.
Call buildPinnedVariant once with that selected tag, preserving the existing
matched fallback and behavior.
In `@src/main/lib/ipc/sessionActions/launch.ts`:
- Around line 356-364: Remove the redundant _operationAborts deletion from the
repairAbort.signal.aborted branch in the catch block, while preserving its
cancelled return; rely on the existing finally block after the try/catch to
perform the conditional cleanup.
In `@src/main/sources/standalone/actions.ts`:
- Around line 203-205: Update the exact-restore flow around torchNote and
torchSkippedForImport so an observed stack that cannot be reapplied returns the
existing skip message as an error before any mutation when mode === 'exact'.
Preserve the current reporting behavior for non-exact modes and apply the same
handling to both affected branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa819b31-cb92-4609-9220-141e0b54d01d
📒 Files selected for processing (27)
locales/en.jsonlocales/zh.jsonsrc/main/lib/ipc/registerInstallationHandlers.tssrc/main/lib/ipc/registerSnapshotHandlers.tssrc/main/lib/ipc/sessionActions/delegate.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/ipc/sessionActions/migrate.tssrc/main/lib/ipc/shared.tssrc/main/lib/nodes.test.tssrc/main/lib/nodes.tssrc/main/lib/pip.tssrc/main/lib/snapshots/index.tssrc/main/lib/snapshots/restore.test.tssrc/main/lib/snapshots/restore.tssrc/main/lib/snapshots/types.tssrc/main/lib/standaloneMigration.tssrc/main/sources/standalone/actions.tssrc/main/sources/standalone/torchIndexManifest.test.tssrc/main/sources/standalone/torchIndexManifest.tssrc/main/sources/standalone/torchRepair.test.tssrc/main/sources/standalone/torchRepair.tssrc/main/sources/standalone/torchStackCatalog.tssrc/main/sources/standalone/torchStackTransaction.tssrc/main/sources/standalone/torchStackTypes.test.tssrc/main/sources/standalone/torchStackTypes.tssrc/main/sources/standalone/updateSections.test.tssrc/main/sources/standalone/updateSections.ts
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
1 participant
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.
PyTorch stack switching + reliable v2 snapshot torch restore
Implements the consolidated plan for trusted PyTorch stack switching and honest snapshot restore semantics.
PyTorch stack switching (managed installs)
torchStackTypes.tsseparates what a stack is (ManagedTorchStackRef: stackId, variant, pythonVersion, packages) from where it can be re-acquired (TorchStackSource, currentlycomfy-bundleonly;pytorch-index/pypireserved).stackId; main re-resolves it against a fresh catalog fetch before acting (torchStackCatalog.ts).torchStackTransaction.ts) - the correctness boundary:recoverTorchStackTransactionfinishes interrupted swaps on next launchupdateSections.ts), driven by the cached catalog;change-pytorchruns stopped + in-place-relaunch like other environment mutations.Snapshot schema v2
Snapshots now record the exact PyTorch stack identity at capture time (
torchStack:managedref orobservedinfo-only record).torchStackunion (allowlisted sources, PEP-440-ish version strings) so a snapshot can never smuggle in an arbitrary acquisition URL.Restore reliability
pendingSnapshotRestoremarker are retained so the user can retry from the Snapshots tab.torchRepair.ts) now prefers the persistedlastVerifiedTorchStackand no longer latches "done" prematurely.Tests
torchStackvalidation (source smuggling, injection strings, local build tags like+rocm7.1)lastVerifiedTorchStackpreferenceFixes
Related: #1254 (exact/compatible restore modes land here; mode-selection UI tracked in #1262). Follow-ups: #1261, #1262, #1263, #1264, #1265.