Skip to content

Commit 1830b0c

Browse files
donislawdevclaude
andcommitted
core: resolve the output directory once a pass, not once a file
Checking that a manifest entry stays inside the output directory resolved the whole directory from the root down, and then resolved the file from the root down, for every entry. A run of 3000 entries walked the same ancestors 6000 times. On Windows the cost of resolving a path grows with its depth, about 0.24 ms per component here, so that was most of what verify did. Measured on 3000 files of 1 kB, same machine, same moment: working out paths 12475 ms opening and hashing all 626 ms the walk and the stats 257 ms ------- the whole of verify 13358 ms verify now takes 0.90-1.09 s where it took 16.9-17.3 s from a ten component path, and 0.50-0.59 s where it took 2.40-2.56 s from a three component one. cleanup goes through the same code and gets the same saving. Linux was already fast and is unchanged. No concurrency was added. What was rejected, and why it matters. The recorded plan was to hash in parallel, on the reading that the loop already did the minimum so the cost had to be the operating system opening files. A probe that timed the real verify beside its own loop showed 869 ms against 17295 ms on the same files at the same moment, which is how the missing 95% was found. Hashing in parallel would have addressed 4.7% of the run. It scales 4.75x at eight threads, so the number exists if the reason ever does. The first version of this was wrong in a way worth recording. Answering from the walk below the boundary alone is quick and it is not the rule: a link inside the output directory pointing at another file inside it has left nothing, and this tool accepted it before. Refusing it would have been a change to what the tool accepts, smuggled in as a change to how fast it answers. The quick reading now only ever answers "inside" - it cannot wrongly refuse, only wrongly accept - and anything else falls through to the original reading. Wrongly accepting is what the junction guards already catch. Nothing about what verify and cleanup accept or refuse has changed. Only the boundary is settled once. Every entry still gets its own walk below it, asked of the filesystem at that moment, so a link planted mid-run is still caught. Swapping the named directory itself was never caught, before or after: the old reading resolved both ends through the new link and found them agreeing. Four new guards, all proven by mutation. Two of them read the source, because there is no behaviour to assert on: putting the resolution back inside the loop, or comparing against the directory as typed rather than its absolute spelling, both leave every verdict correct and quietly give the cost back. filepath.Rel refuses a relative path against an absolute one, so the second mistake would have been slow in the field and green in the suite, for exactly the people who type "./out" - every other containment guard here builds its directory with t.TempDir, which is absolute. The README carried the same wrong reading as advice - it told people the cost was the antivirus opening each file, and quoted 22 seconds. It is corrected here rather than left standing, because it was the one place the mistake was being handed to somebody else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent b3dce86 commit 1830b0c

6 files changed

Lines changed: 443 additions & 22 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,23 @@ because it turns other people's test suites red.
1414

1515
## [Unreleased]
1616

17+
### Changed
18+
19+
- `verify` and `cleanup` are much faster over large runs on Windows. Checking
20+
that an entry stays inside the output directory used to work the directory
21+
out again for every single file, and working it out is expensive on Windows -
22+
more so the deeper the directory sits. It is now worked out once per command.
23+
Measured on 3000 files of 1 kB: `verify` went from about 17 seconds to about
24+
0.9 seconds from a deep path, and from about 2.4 seconds to about 0.5 seconds
25+
from a short one. Linux was already fast and is unchanged.
26+
27+
Nothing about what the two commands accept or refuse has changed. A file that
28+
leaves the directory through a link or a junction is still refused, and a
29+
directory reached through a link still works.
30+
31+
- The answer in the README about slow runs on Windows said the cost was the
32+
antivirus opening each file. That was wrong, and it is corrected.
33+
1734
## [0.1.0] - 2026-08-20
1835

1936
Initial release.

‎README.md‎

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -580,17 +580,24 @@ The one link in the whole program is the Donate button in the window, and
580580
pressing it hands the address to your browser. Your browser makes that
581581
connection, if you ask for it. The program never opens one.
582582

583-
### Why is a run over thousands of files slow on Windows?
584-
585-
Because something is looking at every file as it is opened, and on Windows that
586-
is usually the antivirus.
587-
588-
Measured on the same build, same machine, 3000 files of 1 kB: `verify` takes
589-
about 22 seconds on Windows and about 0.2 seconds on Linux in a container. The
590-
work is identical, so what differs is the cost of opening a file. If your
591-
fixtures live somewhere you can add an antivirus exclusion for, that is the
592-
lever - the files this tool writes are synthesised from a seed and contain
593-
nothing to scan.
583+
### Why is a run over thousands of files slower on Windows?
584+
585+
Because Windows charges more for every path it looks at, and a command that
586+
goes over thousands of files looks at thousands of paths.
587+
588+
Measured on the same machine, 3000 files of 1 kB: `verify` takes about
589+
0.9 seconds on Windows and about 0.2 seconds on Linux in a container. How deep
590+
your output directory sits changes the Windows figure - the same 3000 files
591+
verify in about half that from a short path like `C:\fixtures`, because every
592+
folder above them is part of what gets looked at.
593+
594+
An earlier version of this answer said the cost was the antivirus scanning
595+
each file as it was opened, and that `verify` took about 22 seconds. Measuring
596+
it properly showed the reading was wrong: opening and reading all 3000 files
597+
was a small part of that time and working out paths was most of it. That has
598+
been fixed, and the numbers above are what it costs now. If a scanner does
599+
watch the folder you generate into, an exclusion still helps - the files this
600+
tool writes are made up from a seed and contain nothing to find.
594601

595602
### What if I ask for something impossible?
596603

‎internal/audit/audit.go‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -108,10 +108,16 @@ func (e *EscapeError) Error() string {
108108
// the second half of the rule docs/SECURITY.md section 2.4 states: a name from
109109
// somebody else's file is a name, and where it lands is settled after the links
110110
// have been followed rather than by reading the string.
111-
func resolved(dir string, f manifest.File) (string, error) {
112-
full := filepath.Join(dir, filepath.FromSlash(f.Path))
113-
if core.EscapesAfterResolving(dir, full) {
114-
return "", &EscapeError{Dir: dir, Path: f.Path}
111+
//
112+
// The boundary is worked out once by the caller and handed in, rather than
113+
// worked out here from the directory name. Passing it as a value is what makes
114+
// resolving it per entry impossible to write by accident: there is no directory
115+
// string in scope to resolve. See core.Boundary for what that is worth in
116+
// seconds, and observation O117 for the measurement.
117+
func resolved(b core.Boundary, f manifest.File) (string, error) {
118+
full := filepath.Join(b.Dir(), filepath.FromSlash(f.Path))
119+
if b.Escapes(full) {
120+
return "", &EscapeError{Dir: b.Dir(), Path: f.Path}
115121
}
116122
return full, nil
117123
}
@@ -149,6 +155,7 @@ func Claimed(m *manifest.Manifest) []manifest.File {
149155
// directory is the one answer this must never give.
150156
func Verify(ctx context.Context, dir string, m *manifest.Manifest, skip string) ([]Difference, error) {
151157
claimed := Claimed(m)
158+
boundary := core.NewBoundary(dir)
152159

153160
present, err := walk(dir)
154161
if err != nil {
@@ -164,7 +171,7 @@ func Verify(ctx context.Context, dir string, m *manifest.Manifest, skip string)
164171
}
165172
seen[f.Path] = true
166173

167-
full, err := resolved(dir, f)
174+
full, err := resolved(boundary, f)
168175
if err != nil {
169176
return nil, err
170177
}

‎internal/audit/cleanup.go‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"io/fs"
88
"os"
99

10+
"github.com/donislawdev/TestingFilesGenerator/internal/core"
1011
"github.com/donislawdev/TestingFilesGenerator/internal/manifest"
1112
)
1213

@@ -59,12 +60,14 @@ func (c Candidate) Removable(force bool) bool {
5960
// prints this list. A person gets to read what is about to disappear before it
6061
// does, and CLI.md section 9 rules out asking them interactively.
6162
func Inspect(ctx context.Context, dir string, m *manifest.Manifest) ([]Candidate, error) {
63+
boundary := core.NewBoundary(dir)
64+
6265
var out []Candidate
6366
for _, f := range Claimed(m) {
6467
if err := ctx.Err(); err != nil {
6568
return out, err
6669
}
67-
full, err := resolved(dir, f)
70+
full, err := resolved(boundary, f)
6871
if err != nil {
6972
// Nothing is inspected and nothing is offered. A list that points
7073
// outside the directory is not a list this tool acts on, and the
@@ -131,6 +134,23 @@ type Outcome struct {
131134
// was removed stays removed, and the caller reports both halves - there is no
132135
// undo, so the report is the only record.
133136
func Remove(ctx context.Context, dir string, cands []Candidate, force bool) ([]Outcome, error) {
137+
// This pass works the boundary out for itself rather than taking Inspect's,
138+
// which is the same rule as the one below: the two passes are separated by
139+
// however long somebody spends reading the preview, and nothing learned
140+
// before that pause is carried across it.
141+
//
142+
// Inside this pass the boundary is settled once. What that does and does not
143+
// cover is worth being exact about, because this is the one operation here
144+
// that destroys data. Every entry still gets its own walk below the
145+
// boundary, asked of the filesystem at the moment that entry is removed - so
146+
// a link planted under the directory mid-run is still caught. What is not
147+
// re-asked is the directory the person named. Swapping that for a link
148+
// halfway through was never caught: the old reading resolved both ends
149+
// through the new link and found them agreeing, so it reported "inside" too.
150+
// Redirections above the boundary are the caller's own, as the comment on
151+
// crossesUnresolvedLink says.
152+
boundary := core.NewBoundary(dir)
153+
134154
var out []Outcome
135155
for _, c := range cands {
136156
if err := ctx.Err(); err != nil {
@@ -148,7 +168,7 @@ func Remove(ctx context.Context, dir string, cands []Candidate, force bool) ([]O
148168
// separated by however long a person spends reading the preview, and
149169
// this is the one operation in the tool that destroys data - so the
150170
// question is put to the filesystem in the state it is in now.
151-
full, err := resolved(dir, manifest.File{Path: c.Path})
171+
full, err := resolved(boundary, manifest.File{Path: c.Path})
152172
if err != nil {
153173
return out, err
154174
}

‎internal/core/filename.go‎

Lines changed: 105 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,27 +96,128 @@ func hasVolumeName(p string) bool {
9696
// paths on purpose, and a workspace that is itself a link has to keep working.
9797
// The question is only where the path ends up.
9898
func EscapesAfterResolving(dir, full string) bool {
99-
base, err := resolveAsFarAsItExists(dir)
99+
return NewBoundary(dir).Escapes(full)
100+
}
101+
102+
// Boundary is the directory a run was pointed at, with the links on the way
103+
// followed once instead of once per entry.
104+
//
105+
// It exists for speed, and the speed is not a detail. Judging one entry used to
106+
// resolve the whole directory from the root down, and a run judges thousands of
107+
// them. Measured on 2026-08-20 with 3000 entries, verify on Windows:
108+
// resolving the directory again for every entry was 5665 ms of a 13.4 s run,
109+
// and resolving each file from the root down was another 8703 ms - against
110+
// 626 ms to actually open and hash all 3000 files. On Windows the cost of
111+
// filepath.EvalSymlinks grows with the depth of the path, about 0.24 ms per
112+
// component here, so both numbers are really the same mistake: walking the
113+
// same ancestors over and over. The full measurement is observation O117.
114+
//
115+
// What it does NOT cache is the part that does the work. Every entry still gets
116+
// its own walk below the boundary, asking the filesystem in the state it is in
117+
// at that moment - see crossesUnresolvedLink. Only the boundary itself is
118+
// settled once, and that is the directory the person named, whose own
119+
// redirections are their business rather than ours.
120+
type Boundary struct {
121+
named string
122+
abs string
123+
resolved string
124+
known bool
125+
}
126+
127+
// NewBoundary follows the links on the way to dir, once.
128+
//
129+
// The absolute spelling is kept beside the name because filepath.Rel refuses to
130+
// compare a relative path against an absolute one, and the directory arrives
131+
// however the person typed it - "tfg verify ./tfg-out/manifest.json" is the
132+
// ordinary way to run this. Without it the cheap comparison in Escapes would
133+
// fail for exactly the people who type the shorter thing, fall back to the
134+
// expensive reading, and be slow in the field while every guard here stayed
135+
// green: they all build their directories with t.TempDir, which is absolute.
136+
func NewBoundary(dir string) Boundary {
137+
b := Boundary{named: dir, abs: dir}
138+
if abs, err := filepath.Abs(dir); err == nil {
139+
b.abs = abs
140+
}
141+
r, err := resolveAsFarAsItExists(dir)
100142
if err != nil {
101143
// Nothing could be resolved, so nothing can be judged. Saying "it
102144
// escapes" would refuse an ordinary run on a path we simply could not
103145
// examine, and the caller checks the text separately.
146+
return b
147+
}
148+
b.resolved, b.known = r, true
149+
return b
150+
}
151+
152+
// Dir is the directory as the caller named it, before any link was followed.
153+
func (b Boundary) Dir() string { return b.named }
154+
155+
// Escapes reports whether full lands outside this boundary once the links on
156+
// the way have been followed.
157+
//
158+
// It gives the same answer as resolving both ends, in every case. The saving is
159+
// not a different rule, it is the same rule reached without walking the
160+
// ancestors again, and it only applies to the case where nothing redirects at
161+
// all - which is every ordinary run.
162+
//
163+
// The reasoning, because getting it wrong here is a containment bug. When a
164+
// path is written inside the directory we were given and no step below that
165+
// directory redirects anywhere, the path as written IS the real path: the
166+
// ancestors resolve to the boundary we already resolved, and the steps below it
167+
// resolve to themselves. So it is inside, and resolving it could not have said
168+
// otherwise.
169+
//
170+
// The moment a step below the boundary does redirect, that shortcut stops
171+
// holding and the thorough reading decides. It has to, and this was nearly got
172+
// wrong: a link inside the directory that points at another file inside the
173+
// same directory is NOT an escape, and the old reading allows it - it follows
174+
// the link, lands inside, and says so. Refusing it here because a link was
175+
// seen would turn a contained directory into a hard refusal, which is a change
176+
// to what this tool accepts rather than a change to how fast it answers.
177+
func (b Boundary) Escapes(full string) bool {
178+
if !b.known {
179+
return false
180+
}
181+
target, err := filepath.Abs(full)
182+
if err != nil {
104183
return false
105184
}
185+
// Written inside the directory we were given, and nothing below that
186+
// directory redirects anywhere. Then the path as written is the real path,
187+
// and resolving it from the root down could not have said otherwise.
188+
if rel, err := filepath.Rel(b.abs, target); err == nil && !climbsOut(rel) && !crossesUnresolvedLink(b.resolved, rel) {
189+
return false
190+
}
191+
// Anything else is a question rather than an answer, and the thorough
192+
// reading is what answers it. A path written outside the name can still be
193+
// inside once the links are followed, and a redirection below the boundary
194+
// can point either way.
195+
return b.escapesTheThoroughWay(full)
196+
}
197+
198+
// escapesTheThoroughWay is the original reading: resolve both ends and compare
199+
// them. Kept for a full path that was not written inside the directory, where
200+
// the cheap comparison above has no shared prefix to work from.
201+
func (b Boundary) escapesTheThoroughWay(full string) bool {
106202
target, err := resolveAsFarAsItExists(full)
107203
if err != nil {
108204
return false
109205
}
110-
rel, err := filepath.Rel(base, target)
206+
rel, err := filepath.Rel(b.resolved, target)
111207
if err != nil {
112208
// Different volumes have no relative path between them, which is as
113209
// far outside as it gets.
114210
return true
115211
}
116-
if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
212+
if climbsOut(rel) {
117213
return true
118214
}
119-
return crossesUnresolvedLink(base, rel)
215+
return crossesUnresolvedLink(b.resolved, rel)
216+
}
217+
218+
// climbsOut reports whether a relative path steps above what it is relative to.
219+
func climbsOut(rel string) bool {
220+
return rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator))
120221
}
121222

122223
// crossesUnresolvedLink reports whether any step from base down to rel is a

0 commit comments

Comments
 (0)