Repository navigation
Dedupe repeated installs to the same physical directory - #2156
wakqasahmed wants to merge 2 commits into
Conversation
|
The situation is a bit strange, and I'm not sure exactly what happened. Initially, when I tested using this PR's branch, I still encountered issue #2098. After I deleted I then repointed However, when I resumed Syncthing synchronization for this folder, the issue did not recur. I also tried running I'm not really sure what happened exactly, and I can no longer reproduce the issue on my end now. |
|
Thanks for digging into that, even though it's inconclusive. That actually lines up with what I flagged in the PR description: the regression test I wrote passes on this Linux sandbox even without the fix applied, so I can't independently confirm it against the Windows-specific |
|
Hi @quuu — not sure who owns review for this one — it's been a little while, green and mergeable. Could you take a look or redirect me? |
|
I found the exact reproduction method for #2098, though it may not have much to do with this PR. |
When multiple selected targets resolve to the same physical directory (e.g. several universal agents all writing to ~/.agents/skills, or two agent-specific dirs that both alias to the canonical dir via a symlink), the install loop previously wiped and rewrote that directory once per target. Cache the install result per resolved write path and reuse it for every subsequent target that maps to the same directory. This also reduces the number of separate rm+mkdir+write cycles issued against a single physical directory in quick succession, which matters when that directory is reached through a symlink to another location (e.g. a Syncthing-synced drive) rather than a plain local path.
2fbba80 to
501abfd
Compare
|
@TheWhiteDog9487 thanks for tracking that down, that's a much cleaner root cause than either of us had. I went back through this with your finding in mind, and I don't think the dedup fix in this PR is actually the same bug. The So this PR is still a real fix for a real thing (fewer redundant rm+mkdir+write cycles against one physical directory per Given that, I'd treat #2098 as closed by your finding rather than by this PR, and this PR as a separate, smaller improvement that can land on its own merits. Rebased onto current main, no code changes beyond the rebase itself. |
Summary
When multiple selected targets resolve to the same physical directory — several universal agents all writing to
~/.agents/skills, or an agent-specific directory that aliases to the canonical directory through a symlink — the install loop previously wiped and rewrote that directory once per target. This adds a cache keyed by the resolved write path, so a directory is only installed to once and every subsequent target that maps to it reuses that result.Relation to #2098
This addresses one real contributing factor named in #2098: when
~/.agentsitself is a symlink to another location (e.g. a Syncthing-synced drive), repeated rm+mkdir+write cycles against that target in quick succession are more likely to race with whatever else is watching or locking that path. Deduping to a single write per physical directory cuts that down substantially.I want to be upfront about what this does and doesn't verify: I could not reproduce the exact reported
EPERM: operation not permitted, stat '...\.agents'in a Linux sandbox — that error is Windows-specific and, per the issue, tied to a particular symlink + Syncthing interaction I don't have a way to trigger here. The included regression test confirms the dedup behavior itself (installing to 5 agents that all resolve to one symlinked universal directory succeeds and writes exactly once), not a reproduction of the original EPERM. If this doesn't fully resolve #2098 on Windows, it should at least reduce how often it happens — @TheWhiteDog9487, if you're able to test against this branch that would help confirm.Testing
~/.agents, confirms the install succeeds and lands at the real (resolved) location.detect-agent.test.tsunrelated to this change (that suite asserts on which agent is actually running the test process — fails in any non-Cursor sandbox regardless of this diff, confirmed by running it on a clean checkout).tsc --noEmitclean,prettier --checkclean.Implemented with AI assistance (Claude Code).