Simplify LinkSafeFileSystem Enumerate and fix alias-of-link shadowing - #1763
Merged
Merged
Conversation
EnumerateFiles needs two rules: remember which physical directories were scanned (links make the tree a graph, and .NET's own recursion follows them forever on a cycle), and follow links only after the real tree is done, so an alias like Swarm's diffusion_models never shadows the real DiffusionModels folder. Replace the three visited maps and two stacks with a recursive walk, one HashSet keyed by real path under the platform comparer, and a queue of links walked afterwards. A single set makes the cross-map check that the previous revision missed impossible to forget. Add an a_alias row to the shadowing test: every existing alias sorted after DiffusionModels, so following links inline still passed all tests. Co-Authored-By: Lykos (Opus 5.5) <noreply@lykos.ai>
Real-folders-first is one case short: when a folder is itself a link (a user moved DiffusionModels to another drive and linked it back), an alias of that link ties with it, and sibling order picks the winner. Swarm's diffusion_models happens to sort after on NTFS; a_alias does not, and on Linux sibling order is unordered. Generalize the rule: links are walked in order of how many links their path goes through, and the first walk to reach a physical directory owns it. Real directories are the zero-link case. GetRealPath now follows one link at a time (returnFinalTarget: false) and counts hops along the way. Following one link at a time also keeps link targets spelled the way they were stored instead of resolving through subst drives, which yielded every file twice when the root was given on a subst letter. Tests: alias of a link (both sort orders), dangling link, root spelled in a different case, a link to an ancestor of the root, a link inside a link target, and two links looping into each other. Plain FIFO link order fails the alias-of-link row. Co-Authored-By: Lykos (Opus 5.5) <noreply@lykos.ai>
mohnjiles
previously approved these changes
Oct 1, 2026
ionite34
dismissed
mohnjiles’s stale review
October 1, 2026 01:30
The merge-base changed after approval.
mohnjiles
approved these changes
Oct 1, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Stacked on #1743 (NeuralFault's fix for
diffusion_modelsshadowingDiffusionModels).SM creates
Models/diffusion_models -> Models/DiffusionModelsfor every SwarmUI install, and the walk claimed folders in reversed push order, so on NTFS the alias won and the real folder got skipped. Every diffusion model ended up indexed under the alias asUnknown.The diagnosis in #1743 is right, this reworks the implementation to be simpler and more robust, closing one more bug case.
Why shaped like this
We can't just use
Directory.EnumerateFiles(..., AllDirectories). That's what the indexer did before #1385, and .NET's recursion follows links, so a link back up the tree loops forever. If we follow links, the walk has to remember where it's been. That part doesn't go away.#1743 split "where it's been" into three maps with two different comparers, and the regression @mohnjiles caught came from that: one branch forgot to check one of the maps. With one set there's nothing to forget.
The other half is deciding who owns a folder when two paths reach it. "Real folders before links" fixes the Swarm case, but it's one case short. Say a user moves
DiffusionModelsto another drive and links it back, and Swarm's alias then points at that link. Both are links now, so "real first" has nothing to prefer, and whichever sorts first wins.diffusion_modelshappens to sort after on NTFS, buta_aliasdoesn't, and on Linux sibling order is effectively random. Same bug, one level up.So the rule is: walk paths in order of how many links they go through, and the first walk to reach a folder owns it. Real folders are just the zero-link case.
What changed
EnumerateFilesis a recursiveWalkwith oneHashSetand aPriorityQueueof links, keyed by how many links each path goes through.GetRealPathfollows one link at a time and counts as it goes. That also stops it resolving throughsubstdrives, which listed every file twice when the root was on a subst letter (Windows, there since Send Current SM Workflow to Current ComfyUI Instance - To More Easily Visualize #1385).Debug, since the Swarm alias trips one on every index.Tests
Funny thing: before this, you could follow links inline with no queue at all and every test still passed, because every alias in the suite sorted after
DiffusionModels. So the newa_aliasrows are the ones that matter. We broke each rule on purpose to make sure something catches it:RealFolderShadowedByLink("a_alias"), alias-of-link("a_alias")("a_alias")returnFinalTarget: true("a_alias")Also new: dangling link, root spelled in a different case, link to an ancestor of the root, link inside a link target, two links looping into each other.
LinkSafeFileSystemTestsis 28/28 on Windows. We haven't run it on Linux or macOS yet, how relative symlinks resolve there is read from .NET's source.What it doesn't cover
If you hand-make a link that stores its target through a different drive alias (
T:\...while the root is reached viaC:\...), files can still show up twice. SM's own links are always spelled fromModelsDirectory, so they can't hit this. Fixing it properly means comparing folders by file ID instead of path text, which felt like its own PR.🐺 Generated with Lykos (Opus 5.5, Reviewed by Fable 5.1)