From d0fea56bff405c29a505f2669928eaf72ff93fae Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 29 Sep 2026 10:03:05 +0200 Subject: [PATCH 1/4] write: a finished file takes its name only while nobody holds it Every generated file, the manifest, the instructions beside it and a recipe written by "preset eject -o" are written under a temporary name and then given their own. That last step was a rename, and a rename replaces what it lands on - so a file another program or a person put under that name while the tool was writing was destroyed without a word. Measured with a second writer spinning on the same names: 2544 of 3000 lost on NTFS, around 1900 of 1950 on ext4, tmpfs and overlay, 999 of 1000 on an exFAT stick. core.Publish gives the name only while it is free: MoveFile on Windows (from syscall - golang.org/x/sys/windows imports net, which the command line may not link), renameat2 with RENAME_NOREPLACE on Linux and renamex_np with RENAME_EXCL on macOS through golang.org/x/sys/unix, promoted from indirect at the same version. It works on FAT and exFAT, where a hard link does not. Fallbacks for a filesystem without the call are taken only for an "unsupported" answer, and a guard walks them. A generated file whose name was taken during the run fails on its own (exit code 8) and the run goes on. The manifest's reservation is now its temporary name rather than an empty manifest.json, so a run that is killed leaves manifest.json.tfg-writing and the next run says a run is going or was killed instead of calling an empty file the record of an earlier run. The reservation is closed as soon as it is made and reopened by identity at the save. Clean-ups remove a file only while the name still holds it - file id, size and write time, because ext4 and overlay hand a freed inode number to the next file every time. CreateNew lost its non-exclusive second create, which answered O_EXCL misreporting a name through a junction: Go 1.27 no longer does. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 15 ++ THIRD-PARTY-NOTICES.md | 10 +- go.mod | 2 +- internal/audit/audit.go | 12 +- internal/cli/presetcmd.go | 31 +-- internal/core/createnew.go | 72 ++++-- internal/core/publish.go | 86 +++++++ internal/core/publish_darwin.go | 34 +++ internal/core/publish_linux.go | 36 +++ internal/core/publish_other.go | 12 + internal/core/publish_windows.go | 78 ++++++ internal/core/writenew.go | 122 +++++++++ internal/engine/engine.go | 45 +++- internal/engine/parallel.go | 11 +- internal/engine/preflight.go | 8 + internal/engine/record.go | 20 +- internal/guard/anotherrun_test.go | 8 +- internal/guard/concurrentruns_test.go | 86 +++++-- internal/guard/durability_test.go | 76 ++++-- internal/guard/generatorbytes_test.go | 5 +- internal/guard/manifestsafety_test.go | 6 +- internal/guard/orphanedfiles_test.go | 155 ++++++++---- internal/guard/publish_test.go | 322 ++++++++++++++++++++++++ internal/guard/runlock_test.go | 5 + internal/guard/safety_test.go | 9 +- internal/guard/writeescape_test.go | 15 +- internal/legal/modules.go | 11 +- internal/manifest/instructions.go | 32 +-- internal/manifest/manifest.go | 348 +++++++++++++------------- 29 files changed, 1291 insertions(+), 381 deletions(-) create mode 100644 internal/core/publish.go create mode 100644 internal/core/publish_darwin.go create mode 100644 internal/core/publish_linux.go create mode 100644 internal/core/publish_other.go create mode 100644 internal/core/publish_windows.go create mode 100644 internal/core/writenew.go create mode 100644 internal/guard/publish_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 85f52c86..2ece34cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,21 @@ because it turns other people's test suites red. A window that remembered one of these folders from an earlier run offers the home folder too. +- **A file put under a name while this tool is writing that name is no longer + written over.** Each generated file, the manifest, the instructions beside + it and a recipe written by `tfg preset eject -o` are written under a + temporary name first, and until now the last step replaced whatever had + appeared under the final name in the meantime - another program's file, or + a person's. Now that step refuses a name somebody holds: the other file + stays as it is, a generated file that could not take its name is reported + as a failed file (exit code 8), and a manifest or recipe that could not is + reported as a write failure (exit code 5). While a run goes, its manifest + is reserved as `manifest.json.tfg-writing` rather than as an empty + `manifest.json`, so a run that is killed leaves that name behind, and the + next run into the directory says a run is going or was killed and names + the file to remove, instead of calling an empty file the record of an + earlier run. + ## [0.4.0] - 2026-09-25 ### Changed diff --git a/THIRD-PARTY-NOTICES.md b/THIRD-PARTY-NOTICES.md index c4833c50..e2ab0c8d 100644 --- a/THIRD-PARTY-NOTICES.md +++ b/THIRD-PARTY-NOTICES.md @@ -17,14 +17,16 @@ first, so the difference is worth stating rather than leaving to be assumed. | binary | what it is | third party code in it | |---|---|---| -| `tfg` | the command line | the Go runtime, and **four** modules: `github.com/goccy/go-yaml`, `github.com/gen2brain/gav1d`, `github.com/gen2brain/jxl` and `golang.org/x/text` | +| `tfg` | the command line | the Go runtime, and **four** modules: `github.com/goccy/go-yaml`, `github.com/gen2brain/gav1d`, `github.com/gen2brain/jxl` and `golang.org/x/text` - plus `golang.org/x/sys` on Linux and macOS, whose notice is in the window's table below | | `tfg-gui` | the desktop window | the same, plus **27** more for the graphics toolkit, one of them on Linux only | The window is a separate binary because its toolkit needs a C compiler and OpenGL, neither of which the command line uses. A server or a build agent -running `tfg` therefore carries none of the 27, and that is checked rather than -asserted: a guard in the source compares what the command line binary actually -links against that list of four. +running `tfg` therefore carries none of the 27 but one - `golang.org/x/sys`, on +Linux and macOS only, for the one system call that gives a finished file its +name without replacing anything already there. What the command line binary +links is checked rather than asserted: a guard in the source compares it with +the reviewed list of modules, and another refuses a network package in it. --- diff --git a/go.mod b/go.mod index df597672..42f400c0 100644 --- a/go.mod +++ b/go.mod @@ -74,6 +74,7 @@ require ( github.com/gen2brain/jxl v0.2.0 github.com/goccy/go-yaml v1.19.2 github.com/nicksnyder/go-i18n/v2 v2.6.1 + golang.org/x/sys v0.48.0 golang.org/x/text v0.42.0 ) @@ -108,7 +109,6 @@ require ( github.com/yuin/goldmark v1.8.2 // indirect golang.org/x/image v0.46.0 // indirect golang.org/x/net v0.57.0 // indirect - golang.org/x/sys v0.48.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/internal/audit/audit.go b/internal/audit/audit.go index 6e5e8f03..85c46143 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -138,11 +138,17 @@ func (d Difference) String() string { // second is a RECORD that was being saved, so the useful thing to say // is that the directory may hold files nothing lists - which is the // one case where a person has to look rather than just delete. + // + // Since 2026-09-29 that second one is also how a run reserves its + // manifest's name, from before its first file to its save (O252). So + // it may belong to a run still going, like the lock above, and the + // sentence holds both endings open for the same reason. if core.IsWritingName(filepath.Base(d.Path)) { return fmt.Sprintf( - "leftover %s\n a run's record that was not finished being saved, from a run that was "+ - "stopped before it could tidy up. The directory may hold files that nothing lists, and cleanup "+ - "cannot remove those - check what is here against what you expected before deleting this by hand", + "leftover %s\n a run's record that is not saved yet. If a run is going on it will "+ + "replace this with its manifest when it ends. If none is, that run was stopped before it could "+ + "tidy up, and the directory may hold files that nothing lists, which cleanup cannot remove - "+ + "check what is here against what you expected before deleting this by hand", d.Path) } return fmt.Sprintf( diff --git a/internal/cli/presetcmd.go b/internal/cli/presetcmd.go index ea26478d..5b2d0c50 100644 --- a/internal/cli/presetcmd.go +++ b/internal/cli/presetcmd.go @@ -14,7 +14,7 @@ import ( "flag" "fmt" "io" - "os" + "io/fs" "strings" "github.com/donislawdev/TestingFilesGenerator/internal/core" @@ -323,30 +323,25 @@ func (f *fileFlag) Set(s string) error { // the console's code page, which on a stock console changes every letter // outside ASCII. Only the bytes the tool writes itself arrive as they are. // -// Claimed first and then replaced whole. The claim is exclusive and does not -// follow a link (core.CreateNew), which is what keeps an edited recipe from -// being written over. The replacement goes through a temporary name and a -// rename (core.ReplaceFile), so a run stopped part way leaves an empty file or -// none rather than a recipe cut short - and a YAML file cut short can still -// read as a smaller recipe. +// Written whole under a temporary name and given its own only while nobody +// holds it (core.WriteNew), so a run stopped part way leaves no recipe cut short +// - a YAML file cut short can still read as a smaller recipe - and a file +// already under that name, maybe a recipe somebody edited, is refused rather +// than written over. +// +// Until 2026-09-29 this claimed the name with an empty file first and renamed +// the recipe over the claim. A file put under the name between the two was +// destroyed by the rename, and a failure removed whatever held the name by +// then - two of the three windows of O252. func writeEjected(path string, source []byte, errOut io.Writer) int { - f, err := core.CreateNew(path, 0o644) - if err != nil { - var taken *core.NameTakenError - if errors.As(err, &taken) { + if _, err := core.WriteNew(path, source, 0o644); err != nil { + if errors.Is(err, fs.ErrExist) { fmt.Fprintf(errOut, "tfg: %s is already there, and -o does not write over a file - it may be a recipe somebody edited. Nothing was written. Choose another name, or remove that file first.\n", core.Shown(path)) return ExitIO } fmt.Fprintf(errOut, "tfg: cannot write the recipe to %s: %s\n", core.Shown(path), describeError(err)) return ExitIO } - _ = f.Close() - if err := core.ReplaceFile(path, source); err != nil { - // Only the empty claim this call made is there to take back. - _ = os.Remove(path) - fmt.Fprintf(errOut, "tfg: cannot write the recipe to %s: %s\n", core.Shown(path), describeError(err)) - return ExitIO - } fmt.Fprintf(errOut, "recipe: %s\n", core.Shown(path)) return ExitOK } diff --git a/internal/core/createnew.go b/internal/core/createnew.go index bd4b1e50..a1000b81 100644 --- a/internal/core/createnew.go +++ b/internal/core/createnew.go @@ -1,8 +1,6 @@ package core import ( - "errors" - "io/fs" "os" ) @@ -34,40 +32,62 @@ import ( // any privilege, which is measured rather than read. So the only answer that // holds is the one the operating system settles while it creates the file. // -// O_EXCL IS NOT RELIABLE EVERYWHERE, and that was measured too, on 2026-08-03 -// and again on 2026-08-25. On Windows, Go asks for the reparse point rather -// than for what it points at when O_EXCL is set, and the create then reports -// "the file exists" about a file that is not there whenever any part of the -// path is a symbolic link or a junction. A directory reached through a link is -// an ordinary setup - a redirected workspace, a mounted scratch disk - and this -// tool supports it on purpose. +// O_EXCL WAS NOT RELIABLE EVERYWHERE, measured on 2026-08-03 and again on +// 2026-08-25 (O47): on Windows the create reported "the file exists" about a +// file that was not there whenever any part of the path was a symbolic link or +// a junction. This function answered that with a second create, without +// O_EXCL, whenever os.Lstat found nothing - and that second create truncated +// whatever another process put under the name between the two calls, the +// first of the three windows in O252. // -// So a refusal is believed only when something really is there, and the -// question that settles it is os.Lstat rather than os.Stat: a link pointing at -// nothing is a name being taken, whatever it points at. Where O_EXCL works this -// is exactly O_EXCL. Where it lies, this is what the tool did before it, and -// what is left is the window between the two calls - narrow, on that one -// platform, and smaller than the whole of the door it replaces. +// It is gone since 2026-09-29, because the compiler moved underneath it. +// Measured that day on Go 1.27.0, the oldest compiler go.mod admits: O_EXCL +// through a junction creates the file and says nothing false. A symbolic link +// could not be measured on this machine, which grants no right to make one, +// and TestADirectoryReachedThroughALinkStillWorks asks exactly that question on +// the runners, which do. +// +// A refusal is still read with os.Lstat, and only to choose its words: a name +// that holds something - even a link pointing at nothing - gets the sentence +// about a name already in use, and any other failure is the create's own. func CreateNew(path string, perm os.FileMode) (*os.File, error) { f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, perm) if err == nil { return f, nil } - - _, lookErr := os.Lstat(path) - if lookErr == nil { - // Something is genuinely there. This is the refusal that matters, and - // it is the one the escapes above went round. + if _, lookErr := os.Lstat(path); lookErr == nil { return nil, &NameTakenError{Path: path, Err: err} } - if !errors.Is(lookErr, fs.ErrNotExist) { - // A name we cannot ask about is not a name we may write over. Reported - // as the create failed rather than as the look did, because the create - // is what the caller asked for. + return nil, err +} + +// OpenOwn opens for writing a file this tool made earlier and closed, and +// refuses when the name holds anything else by now. +// +// The question is asked of the opened file rather than of the name, and that +// order is the point. A name looked at and then opened can be swapped for a +// link in between, and the open follows the link wherever it points - so the +// look proves nothing. The open file is what the bytes would reach, so it is +// the one to ask. +// +// Here beside CreateNew rather than with the writers, because it is the other +// half of the same claim: CreateNew makes the file, and this is the only way +// back into it. +func OpenOwn(path string, own os.FileInfo) (*os.File, error) { + f, err := os.OpenFile(path, os.O_WRONLY, 0) + if err != nil { + return nil, err + } + now, err := f.Stat() + if err != nil { + _ = f.Close() return nil, err } - - return os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm) + if !sameWrite(own, now) { + _ = f.Close() + return nil, &NotOursError{Path: path} + } + return f, nil } // NameTakenError is refusing to write under a name something else is holding. diff --git a/internal/core/publish.go b/internal/core/publish.go new file mode 100644 index 00000000..9a456ba9 --- /dev/null +++ b/internal/core/publish.go @@ -0,0 +1,86 @@ +package core + +import ( + "errors" + "io/fs" + "os" +) + +// Publish puts a finished file under its final name, and refuses when anything +// already holds that name. +// +// Every file this tool finishes goes through here: each generated file, the +// manifest, the instructions beside it and a recipe written by "preset eject +// -o". They are all written whole under a temporary name first, so a run cut +// short never leaves half a file under a real one, and this is the step that +// gives the finished bytes their name. +// +// It replaced os.Rename, and the reason is O252, measured on 2026-09-29 with a +// second writer spinning on the same name (docs/O252-ANALIZA-2026-09-29.md). +// A rename REPLACES whatever is at the destination. So a file somebody else put +// under that name while we were writing - after our look, before our rename - +// was destroyed without a word. The measured loss was 2544 of 3000 names on +// NTFS, around 1900 of 1950 on ext4, tmpfs and overlay, and 999 of 1000 on an +// exFAT pendrive. The same writer through this function lost none on any of +// them, because the system settles "is the name free" and "take it" as one +// operation and says fs.ErrExist otherwise. +// +// The call differs by system, and the files beside this one each name theirs: +// MoveFileEx without MOVEFILE_REPLACE_EXISTING on Windows, renameat2 with +// RENAME_NOREPLACE on Linux, renamex_np with RENAME_EXCL on macOS. A hard link +// was the first idea and it is the fallback rather than the rule, because FAT +// and exFAT have none - measured on Linux, where link answers EPERM, and on +// Windows, where it answers "Incorrect function" even for a name that IS taken. +// +// Two fallbacks stand behind the call, for a filesystem that does not know it - +// some network shares, macOS on a FAT stick. Neither was reached on any disk +// measured, which is why each is taken only for an answer that means "this +// filesystem cannot do that" and never for any other failure: +// +// 1. a hard link to the final name, then the temporary name removed. It +// refuses a taken name the same way, where links exist at all. +// 2. the rename this replaced, after a look at the final name. That is the +// window O252 was about, and it is kept rather than refused because the +// alternative is a tool that cannot write to a stick at all. It is +// narrower than it was: it is only ever the last resort. +// +// The temporary name is ours in every case: on success it is gone, and on a +// refusal it is left for the caller, which created it and removes it. +func Publish(tmp, final string) error { + return PublishThrough(tmp, final, renameNoReplace, os.Link) +} + +// PublishThrough is Publish with its two system calls passed in. +// +// It exists for one reason: the fallbacks are reached only on a filesystem this +// project's runners do not have, and a fallback nothing can reach is a defence +// nothing can turn red. So a guard walks the chain here on an ordinary disk, +// handing it calls that answer "unsupported". Publish is the only caller in the +// program, and it passes the real ones. +func PublishThrough(tmp, final string, noReplace, link func(string, string) error) error { + err := noReplace(tmp, final) + if err == nil { + return nil + } + if !errors.Is(err, errors.ErrUnsupported) { + return &os.LinkError{Op: "publish", Old: tmp, New: final, Err: err} + } + + err = link(tmp, final) + if err == nil { + // The finished file has both names for a moment. The final one is + // what counts, so a failure to drop the other is not a failure of + // the publish - it leaves one of our temporary names behind, which + // verify already names as ours. + _ = os.Remove(tmp) + return nil + } + if !linkUnsupported(err) { + return &os.LinkError{Op: "publish", Old: tmp, New: final, Err: err} + } + + if _, lookErr := os.Lstat(final); lookErr == nil { + return &os.LinkError{Op: "publish", Old: tmp, New: final, Err: fs.ErrExist} + } + return os.Rename(tmp, final) +} diff --git a/internal/core/publish_darwin.go b/internal/core/publish_darwin.go new file mode 100644 index 00000000..ac3a9982 --- /dev/null +++ b/internal/core/publish_darwin.go @@ -0,0 +1,34 @@ +package core + +import ( + "errors" + "fmt" + + "golang.org/x/sys/unix" +) + +// renameNoReplace is renamex_np with RENAME_EXCL: the rename happens only when +// nothing is at the destination, and EEXIST otherwise. +// +// NOT MEASURED on a Mac - there is none to measure on (O153). The flag is +// documented for APFS and HFS+, and a filesystem that does not know it answers +// ENOTSUP, which sends Publish to its fallbacks rather than failing the write. +func renameNoReplace(from, to string) error { + err := unix.RenamexNp(from, to, unix.RENAME_EXCL) + if err == nil { + return nil + } + if errors.Is(err, unix.ENOTSUP) || errors.Is(err, unix.EOPNOTSUPP) || errors.Is(err, unix.EINVAL) { + return fmt.Errorf("%w: %w", errors.ErrUnsupported, err) + } + return err +} + +// linkUnsupported says whether a hard link failed because this filesystem has +// none. NOT MEASURED on a Mac. +func linkUnsupported(err error) bool { + return errors.Is(err, errors.ErrUnsupported) || + errors.Is(err, unix.EPERM) || + errors.Is(err, unix.ENOTSUP) || + errors.Is(err, unix.EOPNOTSUPP) +} diff --git a/internal/core/publish_linux.go b/internal/core/publish_linux.go new file mode 100644 index 00000000..ff67af91 --- /dev/null +++ b/internal/core/publish_linux.go @@ -0,0 +1,36 @@ +package core + +import ( + "errors" + "fmt" + + "golang.org/x/sys/unix" +) + +// renameNoReplace is renameat2 with RENAME_NOREPLACE: the rename happens only +// when nothing is at the destination, and EEXIST otherwise. +// +// Measured on 2026-09-29 on overlay, tmpfs, ext4, FAT (vfat) and exFAT through +// FUSE, all of them answering it correctly. A filesystem that does not know the +// flag answers EINVAL, and a kernel older than 3.15 has no such call at all. +// Both are "unsupported", which sends Publish to its fallbacks rather than +// failing the write. +func renameNoReplace(from, to string) error { + err := unix.Renameat2(unix.AT_FDCWD, from, unix.AT_FDCWD, to, unix.RENAME_NOREPLACE) + if err == nil { + return nil + } + if errors.Is(err, unix.EINVAL) || errors.Is(err, unix.ENOSYS) || errors.Is(err, unix.EOPNOTSUPP) { + return fmt.Errorf("%w: %w", errors.ErrUnsupported, err) + } + return err +} + +// linkUnsupported says whether a hard link failed because this filesystem has +// none. FAT and exFAT answer EPERM, measured on 2026-09-29. +func linkUnsupported(err error) bool { + return errors.Is(err, errors.ErrUnsupported) || + errors.Is(err, unix.EPERM) || + errors.Is(err, unix.EOPNOTSUPP) || + errors.Is(err, unix.ENOSYS) +} diff --git a/internal/core/publish_other.go b/internal/core/publish_other.go new file mode 100644 index 00000000..6208e9be --- /dev/null +++ b/internal/core/publish_other.go @@ -0,0 +1,12 @@ +//go:build !linux && !darwin && !windows + +package core + +import "errors" + +// renameNoReplace has no call behind it on a system this project does not +// build for, so Publish goes straight to its fallbacks there. +func renameNoReplace(from, to string) error { return errors.ErrUnsupported } + +// linkUnsupported knows only the answer Publish's own chain gives. +func linkUnsupported(err error) bool { return errors.Is(err, errors.ErrUnsupported) } diff --git a/internal/core/publish_windows.go b/internal/core/publish_windows.go new file mode 100644 index 00000000..7016fe4e --- /dev/null +++ b/internal/core/publish_windows.go @@ -0,0 +1,78 @@ +package core + +import ( + "errors" + "fmt" + "path/filepath" + "strings" + "syscall" +) + +// Two answers Windows gives for "this filesystem cannot do that", by number, +// because the syscall package names neither. +const ( + errInvalidFunction = syscall.Errno(1) // ERROR_INVALID_FUNCTION + errNotSupported = syscall.Errno(50) // ERROR_NOT_SUPPORTED +) + +// renameNoReplace is MoveFileW, which moves a file only when nothing is at the +// destination and answers ERROR_ALREADY_EXISTS otherwise - which Go reads as +// fs.ErrExist. os.Rename calls MoveFileEx with MOVEFILE_REPLACE_EXISTING, and +// that flag is the whole difference. +// +// From syscall rather than golang.org/x/sys/windows, and that is measured +// rather than taste: x/sys/windows imports net, so the command line binary +// would have linked a network stack for one call. The guard holding the command +// line away from the network went red on 2026-09-29 the moment it was tried +// (D16, untouchable rule 8). +// +// Measured on 2026-09-29 on NTFS, through a junction and on an exFAT pendrive, +// where a hard link answers "Incorrect function" and this does not. +func renameNoReplace(from, to string) error { + f, err := syscall.UTF16PtrFromString(extendedLength(from)) + if err != nil { + return err + } + t, err := syscall.UTF16PtrFromString(extendedLength(to)) + if err != nil { + return err + } + err = syscall.MoveFile(f, t) + if err == nil { + return nil + } + if errors.Is(err, errInvalidFunction) || errors.Is(err, errNotSupported) { + return fmt.Errorf("%w: %w", errors.ErrUnsupported, err) + } + return err +} + +// linkUnsupported says whether a hard link failed because this filesystem has +// none. exFAT answers ERROR_INVALID_FUNCTION, measured on 2026-09-29. +func linkUnsupported(err error) bool { + return errors.Is(err, errors.ErrUnsupported) || + errors.Is(err, errInvalidFunction) || + errors.Is(err, errNotSupported) +} + +// extendedLength writes a long path the way the system takes it past 260 +// characters. +// +// Every call in package os does this for us, and this is the one call that +// does not go through os. Without it a name at the length limit (O239) - which +// the tool writes on purpose, and whose temporary name is longer still - would +// be refused here while os.Rename took it. The same threshold Go uses, 248, +// because below it a directory plus an 8.3 name still fits. +func extendedLength(path string) string { + if len(path) < 248 || strings.HasPrefix(path, `\\?\`) { + return path + } + abs, err := filepath.Abs(path) + if err != nil { + return path + } + if strings.HasPrefix(abs, `\\`) { + return `\\?\UNC\` + abs[2:] + } + return `\\?\` + abs +} diff --git a/internal/core/writenew.go b/internal/core/writenew.go new file mode 100644 index 00000000..6bb1174f --- /dev/null +++ b/internal/core/writenew.go @@ -0,0 +1,122 @@ +package core + +import ( + "errors" + "os" +) + +// WriteNew writes a whole file under a name nobody holds, and hands back what +// the file is so the caller can later tell it from anything else put there. +// +// Written under a temporary name beside the final one, flushed to the device, +// and only then given its name by Publish - so a run cut short leaves at most a +// temporary name behind, never half a file under a real one, and a name +// somebody else took in the meantime is refused rather than written over. +// +// The flush is here because every caller is a file somebody keeps: the +// instructions beside a manifest and a recipe written by "preset eject -o". +// One of each per command, so the cost argument that keeps generated files +// unflushed does not reach them. +// +// The mode goes to the create, so the process umask applies to it the way it +// applies to every other file this tool writes. +func WriteNew(path string, content []byte, mode os.FileMode) (os.FileInfo, error) { + tmp := SiblingPath(path, WritingMarker) + f, err := CreateNew(tmp, mode) + if err != nil { + return nil, err + } + own, err := writeAndKeep(f, content) + if err != nil { + _ = RemoveOwn(tmp, own) + return nil, err + } + if err := Publish(tmp, path); err != nil { + _ = RemoveOwn(tmp, own) + return nil, err + } + return own, nil +} + +// writeAndKeep fills a file this call created, flushes it and closes it, and +// says what it is. The identity comes from the open handle rather than from the +// name, because the name is exactly what somebody else could have swapped - and +// it is asked after the write, because RemoveOwn compares the size and the time +// too, and those are what the write changes. +func writeAndKeep(f *os.File, content []byte) (os.FileInfo, error) { + if _, err := f.Write(content); err != nil { + return Finish(f, err) + } + if err := f.Sync(); err != nil { + return Finish(f, err) + } + return Finish(f, nil) +} + +// Finish asks an open file what it is, closes it, and reports the first +// failure - the one given, the question, or the close. +func Finish(f *os.File, err error) (os.FileInfo, error) { + own, statErr := f.Stat() + if closeErr := f.Close(); err == nil { + err = closeErr + } + if err == nil { + err = statErr + } + return own, err +} + +// RemoveOwn removes a name only while it still holds the file this tool made +// there. +// +// Every clean-up after a failure used to remove by name, and a name is not a +// file: between our write and our clean-up somebody could put their own file +// under it, and the clean-up then deleted theirs (O252, the third window). +// Asked by identity instead - the same file, however it got there. What stays +// open is the moment between this look and the removal, which is a different +// order of narrow from a whole write. +// +// Identity is the file id and more. On Linux and macOS os.SameFile compares the +// device and the inode number and nothing else, and a filesystem hands a freed +// inode number to the next file created. Measured on 2026-09-29: "ours +// removed, theirs created" got our inode number back 500 times in 500 on ext4 +// and on overlay (0 on tmpfs and NTFS), so os.SameFile alone would have called +// their file ours every time. With the size and the time of the last write +// asked as well it was fooled 0 times in 500 on all four, empty files +// included. The identity taken from the open file before the close agreed with +// the one asked by name afterwards 500 times in 500 on all four. +// +// A name that holds something else is left alone and reported, because +// untouchable rule 7 is that this tool removes only what it wrote. A name +// already gone is not an error: there is nothing of ours left to remove. +func RemoveOwn(path string, own os.FileInfo) error { + now, err := os.Lstat(path) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return err + } + if !sameWrite(own, now) { + return &NotOursError{Path: path} + } + return os.Remove(path) +} + +// sameWrite is the identity RemoveOwn asks for: one file, as it was left. +func sameWrite(own, now os.FileInfo) bool { + return own != nil && os.SameFile(own, now) && + own.Size() == now.Size() && own.ModTime().Equal(now.ModTime()) +} + +// NotOursError is a clean-up that found somebody else's file under a name it +// meant to remove, and left it. +type NotOursError struct { + Path string +} + +func (e *NotOursError) Error() string { + return "the name " + e.Path + " no longer holds the file this tool wrote there, so it was left as it is. " + + "Something else put a file under that name while this tool was working. " + + "Look at it before removing it yourself" +} diff --git a/internal/engine/engine.go b/internal/engine/engine.go index dffaa94f..117dd280 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -194,6 +194,11 @@ type Result struct { // which replaced the record of an earlier run and left that run's files // with nothing able to remove them. A refused run has nothing to record. Started bool + + // reservation is the run's hold on its manifest name, taken before the + // first file and handed to SaveRecord, which saves through it. Nil for a + // dry run and for a run refused before it reserved anything. + reservation *manifest.Reservation } // PlannedFile is one file worked out before anything is written. @@ -523,7 +528,8 @@ func Run(ctx context.Context, files []PlannedFile, opt Options) (*Result, error) // name the claim below already refused that, four times out of four, which // is why this is the same mechanism rather than a new one. lockPath := RunLockPath(opt.OutDir) - if err := claimRunLock(lockPath); err != nil { + lock, err := claimRunLock(lockPath) + if err != nil { if errors.Is(err, fs.ErrExist) { return res, &RunInProgressError{Path: lockPath, Dir: opt.OutDir} } @@ -533,7 +539,7 @@ func Run(ctx context.Context, files []PlannedFile, opt Options) (*Result, error) // signal cancels the context, Run returns, and this runs. What it cannot // cover is the process being killed outright, and that is why the refusal // above names the file to remove. - defer releaseRunLock(lockPath) + defer releaseRunLock(lockPath, lock) // The manifest name is taken before the first file, not after the last one. // @@ -544,7 +550,16 @@ func Run(ctx context.Context, files []PlannedFile, opt Options) (*Result, error) // 5, with sixteen files on the disk and eight of them in nobody's manifest. // Taking the name here turns that into a refusal before anything is written. manifestPath := ManifestPath(opt) - if err := manifest.Claim(manifestPath); err != nil { + reservation, err := manifest.Claim(manifestPath) + if err != nil { + // The reservation name held by somebody is a run going or a run + // killed, not a finished one - the same fault and the same remedy as + // the lock above, so the same words. Asked before the collision below, + // because that refusal is a fs.ErrExist too. + var held *core.NameTakenError + if errors.As(err, &held) { + return res, &RunInProgressError{Path: held.Path, Dir: filepath.Dir(manifestPath)} + } // Only a name that is genuinely taken is a collision. Reporting every // failure that way said "manifest.json already exists ... it is the // only record of what an earlier run wrote" about an empty directory @@ -556,6 +571,7 @@ func Run(ctx context.Context, files []PlannedFile, opt Options) (*Result, error) } return res, fmt.Errorf("cannot start a run in %s: %w", core.Shown(opt.OutDir), err) } + res.reservation = reservation // Past this point the run owns the name and may write. Started says so, and // it is what tells the caller a manifest is worth saving - set here rather @@ -569,7 +585,8 @@ func Run(ctx context.Context, files []PlannedFile, opt Options) (*Result, error) // taken by a run that never happened. defer func() { if !res.Manifest.Run.Complete && res.Failures == 0 && len(res.Manifest.Files) == 0 { - _ = manifest.Release(manifestPath) + _ = res.reservation.Release() + res.reservation = nil } }() @@ -632,24 +649,28 @@ func Run(ctx context.Context, files []PlannedFile, opt Options) (*Result, error) // core.CreateNew rather than os.Create, and that is the whole claim: it refuses // a name something already holds and it believes the refusal only when Lstat // finds something there, so a directory reached through a link still works. -// The file stays empty - the same shape as the manifest claim, and for the same -// reason. Nothing reads it, so there is nothing in it to be read half written. -func claimRunLock(path string) error { +// The file stays empty. Nothing reads it, so there is nothing in it to be read +// half written. What it is comes back with it, so the release removes this +// file rather than whatever holds the name by then. +func claimRunLock(path string) (os.FileInfo, error) { fh, err := core.CreateNew(path, 0o666) if err != nil { - return err + return nil, err } - return fh.Close() + return core.Finish(fh, nil) } -// releaseRunLock gives the name back. +// releaseRunLock gives the name back, if it still holds our lock. +// +// By identity rather than by name since 2026-09-29, the third window of O252: +// a name removed by name removes whatever somebody put there after us. // // The failure is dropped on purpose. A run that finished and could not remove // its own lock has nothing useful to say to the person - the files are written // and the manifest is saved - and the next run into that directory will name // the file and say what to do about it. -func releaseRunLock(path string) { - _ = os.Remove(path) +func releaseRunLock(path string, own os.FileInfo) { + _ = core.RemoveOwn(path, own) } func entryFor(f PlannedFile, sha string, materialized bool, failure error) manifest.File { diff --git a/internal/engine/parallel.go b/internal/engine/parallel.go index 2adf17a5..a621194f 100644 --- a/internal/engine/parallel.go +++ b/internal/engine/parallel.go @@ -396,8 +396,17 @@ func writeOne(ctx context.Context, f PlannedFile, outDir string, p *fileProgress f.Desc.ID, counter.n, f.Plan.Bytes) } - if err := os.Rename(tmp, final); err != nil { + // Given its name only while nobody holds it. Preflight refused every name + // that was taken when the run started, and this is the answer for one + // taken since: a rename used to replace it, so a file somebody put there + // during the run was destroyed without a word - 2544 of 3000 names on + // NTFS, measured on 2026-09-29 with a writer spinning on them (O252). Now + // this file fails in its own words and the run goes on with the rest. + if err := core.Publish(tmp, final); err != nil { _ = os.Remove(tmp) + if errors.Is(err, fs.ErrExist) { + return "", &CollisionError{Path: final} + } return "", err } return hex.EncodeToString(h.Sum(nil)), nil diff --git a/internal/engine/preflight.go b/internal/engine/preflight.go index 60a1d010..9ed04f76 100644 --- a/internal/engine/preflight.go +++ b/internal/engine/preflight.go @@ -70,6 +70,14 @@ func preflight(ctx context.Context, files []PlannedFile, opt Options) error { if path := RunLockPath(opt.OutDir); exists(path) { return &RunInProgressError{Path: path, Dir: opt.OutDir} } + // The manifest's reservation, which outlives the lock above: the lock is + // given back when the files are written, and the reservation when the + // manifest is. A run killed between the two leaves only this one, and a + // manifest pointed outside the output directory has its reservation there + // rather than beside the lock. Same fault, same remedy, same words. + if path := manifest.ReservationPath(ManifestPath(opt)); exists(path) { + return &RunInProgressError{Path: path, Dir: filepath.Dir(path)} + } // The manifest is checked with the files it would describe, and leaving it // out cost exactly what it protects. A second run into the same directory diff --git a/internal/engine/record.go b/internal/engine/record.go index b5a3c4b4..fc82a660 100644 --- a/internal/engine/record.go +++ b/internal/engine/record.go @@ -48,21 +48,35 @@ func (e *InstructionsError) Unwrap() error { return e.Err } func SaveRecord(res *Result, opt Options) (Record, error) { rec := Record{Manifest: ManifestPath(opt)} m := res.Manifest + var instructions os.FileInfo if text := m.Instructions(filepath.Base(rec.Manifest)); text != nil { path := InstructionsPath(opt) - if err := manifest.SaveInstructions(path, text); err != nil { + own, err := manifest.SaveInstructions(path, text) + if err != nil { rec.Missed = &InstructionsError{Path: path, Err: err} } else { + instructions = own rec.Instructions = path m.Run.Instructions = filepath.Base(path) } } - if err := m.Save(rec.Manifest); err != nil { + if err := saveManifest(res, m, rec.Manifest); err != nil { if rec.Instructions != "" { - _ = os.Remove(rec.Instructions) + // The file this call wrote, and nothing that took its name since. + _ = core.RemoveOwn(rec.Instructions, instructions) rec.Instructions, m.Run.Instructions = "", "" } return rec, err } return rec, nil } + +// saveManifest saves through the reservation the run took before its first +// file, or reserves and saves in one go for a result that has none. +func saveManifest(res *Result, m *manifest.Manifest, path string) error { + if r := res.reservation; r != nil { + res.reservation = nil + return r.Save(m) + } + return m.Save(path) +} diff --git a/internal/guard/anotherrun_test.go b/internal/guard/anotherrun_test.go index f8c927b3..49035d63 100644 --- a/internal/guard/anotherrun_test.go +++ b/internal/guard/anotherrun_test.go @@ -56,11 +56,13 @@ func twoRunsSharing(t *testing.T) (dir string, alpha *manifest.Manifest, alphaPa if err != nil { t.Fatalf("running %s: %v", id, err) } - path := engine.ManifestPath(opt) - if err := res.Manifest.Save(path); err != nil { + // Through SaveRecord, the one way both surfaces save, so the manifest + // lands through the reservation the run took before its first file. + rec, err := engine.SaveRecord(res, opt) + if err != nil { t.Fatalf("saving the manifest of %s: %v", id, err) } - return res.Manifest, path + return res.Manifest, rec.Manifest } alpha, alphaPath = run("alpha", "a_{index:04}.txt", "manifest-alpha.json", 3) diff --git a/internal/guard/concurrentruns_test.go b/internal/guard/concurrentruns_test.go index 32e5a38d..a85c4ba7 100644 --- a/internal/guard/concurrentruns_test.go +++ b/internal/guard/concurrentruns_test.go @@ -65,6 +65,11 @@ func TestAManifestIsNeverWrittenOverEvenWhenItAppearsMidRun(t *testing.T) { // 5, with sixteen files on the disk and eight of them in nobody's manifest. // After the claim moved to the front: eight files and one manifest. // +// Since 2026-09-29 the reservation is the temporary name the manifest is +// written under, not an empty file under the final name (O252). So while a run +// goes there is no manifest at all, and nothing can read an empty one as the +// record of a run that happened. +// // What is guarded here is the primitive, because the window it closes is // between two processes and this package keeps concurrency out on purpose. // The engine calling it is covered by the guards for a refused run writing @@ -73,50 +78,87 @@ func TestAClaimedNameCannotBeClaimedTwice(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "manifest.json") - if err := manifest.Claim(path); err != nil { + first, err := manifest.Claim(path) + if err != nil { t.Fatalf("claiming an unused name: %v", err) } - if err := manifest.Claim(path); err == nil { + defer func() { _ = first.Release() }() + if second, err := manifest.Claim(path); err == nil { + _ = second.Release() t.Error("the same name was claimed twice, so two runs could both believe it is theirs") } - // The claim is an empty file rather than a manifest, so nothing reads it as - // a record of a run that happened. - info, err := os.Stat(path) + if _, err := os.Lstat(path); err == nil { + t.Error("a manifest exists while the run that reserved it is still going - a reader would take it for a record") + } + info, err := os.Stat(manifest.ReservationPath(path)) if err != nil { - t.Fatalf("the claim is not there: %v", err) + t.Fatalf("the reservation is not there: %v", err) } if info.Size() != 0 { - t.Errorf("the claim carries %d B - it has to be empty, or a reader would take it for a manifest", info.Size()) + t.Errorf("the reservation carries %d B before the save - it has to be empty", info.Size()) } } -// And a run that claims the name and then cannot start gives it back, or the -// next run into that directory is refused for a file nobody ever wrote. -func TestAClaimIsGivenBackButAFilledInManifestIsNot(t *testing.T) { +// And a run that reserves the name and then cannot start gives it back, or the +// next run into that directory is told a run is going when none is. +func TestAClaimIsGivenBackAndLeavesNoManifest(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "manifest.json") - if err := manifest.Claim(path); err != nil { + r, err := manifest.Claim(path) + if err != nil { t.Fatalf("claiming: %v", err) } - if err := manifest.Release(path); err != nil { + if err := r.Release(); err != nil { t.Fatalf("releasing: %v", err) } - if _, err := os.Stat(path); err == nil { - t.Error("the name is still taken by a run that never wrote anything") + for _, name := range []string{path, manifest.ReservationPath(path)} { + if _, err := os.Lstat(name); err == nil { + t.Errorf("%s is still there after a run that wrote nothing gave its name back", filepath.Base(name)) + } + } + if again, err := manifest.Claim(path); err != nil { + t.Errorf("the name could not be claimed again after it was given back: %v", err) + } else { + _ = again.Release() } +} - // A manifest with content in it is somebody's record and is never removed - // by this path, whatever asks. - if err := os.WriteFile(path, []byte(`{"manifest_version":"1.0","files":[]}`), 0o644); err != nil { - t.Fatalf("writing: %v", err) +// A manifest that appears between the reservation and the save is left as it +// is, and the save says so. +// +// This is the second window of O252, measured on 2026-09-29: the save used to +// rename over the empty claim under the final name, and a rename replaces what +// it lands on. So anything put there during the run - by a person, by another +// tool - was destroyed without a word. Reproduced here without a second +// process, by putting the file there at the moment the race would. +func TestAManifestPutThereDuringTheRunIsNotWrittenOver(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "manifest.json") + + r, err := manifest.Claim(path) + if err != nil { + t.Fatalf("claiming: %v", err) + } + theirs := []byte(`{"manifest_version":"1.0","files":[]}`) + if err := os.WriteFile(path, theirs, 0o644); err != nil { + t.Fatalf("putting the other manifest there: %v", err) + } + + m := manifest.New("testing-files-generator", "0.0.0-dev", "run_x", "tfg generate", 1, "windows", "amd64") + if err := r.Save(m); err == nil { + t.Error("the save reported success over a manifest somebody else put there") + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatalf("the other manifest is gone: %v", err) } - if err := manifest.Release(path); err != nil { - t.Fatalf("releasing a filled in manifest reported an error: %v", err) + if string(after) != string(theirs) { + t.Errorf("the other manifest was written over:\nbefore %s\nafter %s", theirs, after) } - if _, err := os.Stat(path); err != nil { - t.Error("a manifest with content was removed as though it were an empty claim") + if _, err := os.Lstat(manifest.ReservationPath(path)); err == nil { + t.Error("the refused save left its reservation behind, so the next run would be told a run is going") } } diff --git a/internal/guard/durability_test.go b/internal/guard/durability_test.go index 2a8dc2a5..18c5a369 100644 --- a/internal/guard/durability_test.go +++ b/internal/guard/durability_test.go @@ -32,13 +32,19 @@ import ( // to make a check possible rather than checking the structure there is. The // same choice was made, for the same reason, in boundaryresolution_test.go. func TestWhatIsRenamedIntoPlaceIsOnTheDiskFirst(t *testing.T) { - // The manifest reaches the sequence below through writeOver. Asked rather - // than assumed: a writeOver that stopped calling writeClaimed would leave - // this guard reading, honestly and green, a function the manifest no - // longer passes through. - if !strings.Contains(functionSource(t, "internal/manifest/manifest.go", "writeOver"), "writeClaimed(") { - t.Fatal("writeOver in internal/manifest/manifest.go no longer calls writeClaimed, " + - "so the flush checked below is not the one the manifest is written through") + // The manifest reaches the sequence below through its reservation, and + // the instructions and an ejected recipe through WriteNew. Asked rather + // than assumed: a caller that stopped going through the function checked + // here would leave this guard reading, honestly and green, a function + // nothing is written through any more. + for _, via := range []struct{ file, caller, callee string }{ + {"internal/manifest/manifest.go", "Reservation.Save", "saveReserved("}, + {"internal/core/writenew.go", "WriteNew", "writeAndKeep("}, + } { + if !strings.Contains(functionSource(t, via.file, via.caller), via.callee) { + t.Fatalf("%s in %s no longer calls %s, so the flush checked below is not the one it writes through", + via.caller, via.file, via.callee) + } } for _, c := range []struct { file string @@ -48,19 +54,27 @@ func TestWhatIsRenamedIntoPlaceIsOnTheDiskFirst(t *testing.T) { }{ { file: "internal/manifest/manifest.go", - // writeOver rather than Save since 2026-08-27. Save used to hold - // this sequence itself and now wraps it, so that every way of - // failing gives the claimed name back in one place - review item - // S2. The sequence moved with the code and this guard moved with - // the sequence, which is the honest repair: the property being - // asked about is "the thing that renames flushes first", and the - // thing that renames is now called writeOver. - // - // writeClaimed since 2026-09-25, for the same reason: the - // instructions beside the manifest came to be written the same - // way, and the sequence moved into the one function both call. - function: "writeClaimed", - order: []string{"write(f)", "f.Sync()", "f.Close()", "os.Rename(tmp, path)"}, + // saveReserved since 2026-09-29 (O252). It was writeClaimed from + // 2026-09-25 and writeOver before that, and Save before that - the + // sequence moved with the code each time and this guard moved with + // the sequence, which is the honest repair: the property asked + // about is "the thing that names the file flushes first". The + // close is core.Finish, which asks the file what it is and then + // closes it, and the naming is core.Publish rather than a rename. + function: "saveReserved", + order: []string{"m.Encode(f)", "f.Sync()", "core.Finish(f, nil)", "core.Publish(tmp, final)"}, + }, + { + // The instructions beside the manifest and a recipe written by + // "preset eject -o", since 2026-09-29. + file: "internal/core/writenew.go", + function: "writeAndKeep", + order: []string{"f.Write(content)", "f.Sync()", "Finish(f, nil)"}, + }, + { + file: "internal/core/writenew.go", + function: "WriteNew", + order: []string{"writeAndKeep(f, content)", "Publish(tmp, path)"}, }, { file: "internal/core/replace.go", @@ -143,9 +157,21 @@ func function0f(t *testing.T, file, function string) (*ast.FuncDecl, *token.File t.Fatalf("parsing %s: %v", file, err) } source := readFile(t, path) + // "Type.Method" names a method, because two types in one file can each + // have a Save - and finding the first one by name would read the wrong one + // without a word. + receiver, name := "", function + if i := strings.IndexByte(function, '.'); i >= 0 { + receiver, name = function[:i], function[i+1:] + } for _, decl := range parsed.Decls { fn, ok := decl.(*ast.FuncDecl) - if !ok || fn.Name.Name != function || fn.Body == nil { + if !ok || fn.Name.Name != name || fn.Body == nil { + continue + } + // receiverOf lives in canvastold_test.go and reports the type a + // pointer receiver points at. + if _, on := receiverOf(fn); on != receiver { continue } return fn, fset, source @@ -180,10 +206,14 @@ func functionSource(t *testing.T, file, function string) string { // // Found by an outside review of the whole tree, docs/CODE-REVIEW-2026-08-23.md // section 3.7c. +// +// Asked of Claim since 2026-09-29, when the look moved there with the +// reservation (O252): Save now reserves and saves, and the reservation is where +// the name is looked at. func TestSavingAManifestTellsAnEmptySlotFromAnUnreadableOne(t *testing.T) { - body := functionSource(t, "internal/manifest/manifest.go", "Save") + body := functionSource(t, "internal/manifest/manifest.go", "Claim") if !strings.Contains(body, "errors.Is(err, fs.ErrNotExist)") { - t.Error("manifest.Save does not tell a missing file from a failure to look at one, " + + t.Error("manifest.Claim does not tell a missing file from a failure to look at one, " + "so a path it cannot examine is answered in words about a manifest that is already there") } } diff --git a/internal/guard/generatorbytes_test.go b/internal/guard/generatorbytes_test.go index a35e0b2c..2c001e53 100644 --- a/internal/guard/generatorbytes_test.go +++ b/internal/guard/generatorbytes_test.go @@ -10,6 +10,7 @@ import ( "sort" "testing" + "github.com/donislawdev/TestingFilesGenerator/internal/core" "github.com/donislawdev/TestingFilesGenerator/internal/engine" "github.com/donislawdev/TestingFilesGenerator/internal/format" _ "github.com/donislawdev/TestingFilesGenerator/internal/format/all" @@ -450,7 +451,9 @@ func generateOne(t *testing.T, target engine.Target) []byte { } var produced []string for _, e := range entries { - if !e.IsDir() && e.Name() != "manifest.json" { + // The run's manifest reservation stays behind, because this run is + // never saved - see manifest.Reservation. + if !e.IsDir() && e.Name() != "manifest.json" && !core.IsWritingName(e.Name()) { produced = append(produced, e.Name()) } } diff --git a/internal/guard/manifestsafety_test.go b/internal/guard/manifestsafety_test.go index d9ebfa8c..25477074 100644 --- a/internal/guard/manifestsafety_test.go +++ b/internal/guard/manifestsafety_test.go @@ -46,7 +46,9 @@ func seedDirectory(t *testing.T, dir string) []byte { if err != nil { t.Fatalf("the first run failed: %v", err) } - if err := res.Manifest.Save(manifestOf(dir)); err != nil { + // Through SaveRecord, the one way both surfaces save, so the manifest + // lands through the reservation the run took before its first file. + if _, err := engine.SaveRecord(res, opt); err != nil { t.Fatalf("saving the first manifest: %v", err) } body, err := os.ReadFile(manifestOf(dir)) @@ -208,7 +210,7 @@ func TestNoTargetCanTakeTheNameTheManifestNeeds(t *testing.T) { left, _ := os.ReadDir(dir) saveErr := error(nil) if runErr == nil { - saveErr = res.Manifest.Save(filepath.Join(dir, engine.DefaultManifestName)) + _, saveErr = engine.SaveRecord(res, opt) } t.Fatalf("planning allowed a target to take %q, the name the manifest needs. "+ "The run then left %d entries in the directory and saving the manifest said %v", diff --git a/internal/guard/orphanedfiles_test.go b/internal/guard/orphanedfiles_test.go index 0d74e937..3366c4c6 100644 --- a/internal/guard/orphanedfiles_test.go +++ b/internal/guard/orphanedfiles_test.go @@ -5,15 +5,18 @@ import ( "path/filepath" "strings" "testing" + "time" + "github.com/donislawdev/TestingFilesGenerator/internal/core" "github.com/donislawdev/TestingFilesGenerator/internal/manifest" ) // A manifest that could not be written leaves no manifest at all. // -// The run claims the manifest name before it writes its first file, and the -// claim is an empty file. So a save that fails after the files are on disk used -// to leave a nought byte manifest.json sitting beside a complete set of files, +// The run claims the manifest name before it writes its first file, and until +// 2026-09-29 the claim was an empty file under the final name. So a save that +// failed after the files were on disk used to leave a nought byte +// manifest.json sitting beside a complete set of files, // and that one empty file is worse than nothing three separate ways. Measured // on 2026-08-27 by putting a directory under the temporary name the writer uses // and running an ordinary generate: @@ -36,81 +39,93 @@ func TestAManifestThatCouldNotBeWrittenLeavesNoManifest(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "manifest.json") - // The claim, exactly as a run makes it before writing anything. - if err := manifest.Claim(path); err != nil { - t.Fatalf("claiming the name: %v", err) + // The reservation, exactly as a run makes it before writing anything. + r, err := manifest.Claim(path) + if err != nil { + t.Fatalf("reserving the name: %v", err) } - if _, err := os.Stat(path); err != nil { - t.Fatalf("the claim did not create the file: %v", err) + if _, err := os.Stat(manifest.ReservationPath(path)); err != nil { + t.Fatalf("the reservation did not create its file: %v", err) } - // Block the temporary name with a directory, which is how this was - // reproduced against the real binary. Any other way of making the write - // fail would do - this one needs no permissions and works on every host. - if err := os.Mkdir(path+".tfg-writing", 0o755); err != nil { - t.Fatalf("blocking the temporary name: %v", err) + // Block the final name with a directory, so the save fails at the very + // last step. Since 2026-09-29 the reservation is the temporary name + // itself, so blocking that one - how this was first reproduced - now + // refuses the reservation instead, before any file is written. + if err := os.Mkdir(path, 0o755); err != nil { + t.Fatalf("blocking the final name: %v", err) } m := manifest.New("testing-files-generator", "0.0.0-test", "run_x", "tfg generate", 1, "windows", "amd64") m.Add(manifest.File{ID: "files", Path: "files_0001.txt", Name: "files_0001.txt", Bytes: 1024}) - if err := m.Save(path); err == nil { - t.Fatal("saving over a blocked temporary name reported success, so this test is not reaching the failure it is about") + if err := r.Save(m); err == nil { + t.Fatal("saving over a blocked final name reported success, so this test is not reaching the failure it is about") } - if _, err := os.Stat(path); err == nil { - t.Error("a manifest is still there after the save failed. It is empty, so cleanup cannot read it and " + - "the next run into this directory is refused for a record that records nothing") + if info, err := os.Stat(path); err == nil && info.Mode().IsRegular() { + t.Error("a manifest file is there after the save failed, so the next run into this directory " + + "is refused for a record that may record nothing") + } + if _, err := os.Lstat(manifest.ReservationPath(path)); err == nil { + t.Error("the failed save left its reservation behind, so the next run is told a run is going when none is") } } -// Giving a claimed name back never takes away a manifest with something in it. +// A clean-up removes the file this tool wrote and nothing that took its name. // -// This is what makes the rule above safe to have. A failed save now removes the -// name it was writing to, and the one file this tool promises never to destroy -// is a manifest - so the removal has to be able to tell its own empty claim -// from somebody's record of a thousand files. +// This is what makes the rule above safe to have. A failed save removes the +// name it was writing to, and the one thing this tool promises never to +// destroy is a file somebody else put there - so the removal has to be able to +// tell its own file from one that took the name while it worked (O252, the +// third window: until 2026-09-29 every clean-up removed by name). // -// Asked of Release directly rather than through a failed run, and that is a -// correction rather than a shortcut. The first version of this drove a real -// save over a real manifest, passed, and could not be reddened by any single -// mutation - because in that path the file is protected TWICE: Save refuses at -// the size check before it ever writes, and Release refuses again afterwards. -// A guard nothing can break is not a guard, and this project has removed six -// pieces of code for exactly that reason. The end to end half of the question -// already has a guard of its own in manifestsafety_test.go. -func TestGivingAClaimedNameBackSparesAManifestWithContent(t *testing.T) { +// Asked of core.RemoveOwn directly, because every clean-up of this kind goes +// through it - the reservation, the run lock, the instructions after a failed +// manifest. The replacement is made the way the measurement made it: ours +// removed and theirs created at once, which on ext4 hands theirs our inode +// number every time. +func TestGivingAClaimedNameBackSparesWhatTookItsName(t *testing.T) { dir := t.TempDir() - path := filepath.Join(dir, "manifest.json") + path := filepath.Join(dir, "manifest.json.tfg-writing") + if err := os.WriteFile(path, nil, 0o644); err != nil { + t.Fatalf("writing our file: %v", err) + } + ours, err := os.Lstat(path) + if err != nil { + t.Fatalf("asking what our file is: %v", err) + } + if err := os.Remove(path); err != nil { + t.Fatalf("taking our file away: %v", err) + } const theirs = `{"manifest_version":"1.0","files":[]}` if err := os.WriteFile(path, []byte(theirs), 0o644); err != nil { - t.Fatalf("writing the earlier manifest: %v", err) + t.Fatalf("putting their file under the name: %v", err) } - if err := manifest.Release(path); err != nil { - t.Fatalf("releasing a name somebody else holds reported an error: %v", err) + if err := core.RemoveOwn(path, ours); err == nil { + t.Error("removing our file under a name that now holds theirs reported success") } - got, err := os.ReadFile(path) if err != nil { - t.Fatalf("the earlier manifest is gone: %v", err) + t.Fatalf("their file is gone: %v", err) } if string(got) != theirs { - t.Errorf("the earlier manifest changed.\n got: %s\nwant: %s", got, theirs) + t.Errorf("their file changed.\n got: %s\nwant: %s", got, theirs) } - // And the empty claim it IS meant to take away still goes, or the check - // above would pass against a Release that never removes anything. - claim := filepath.Join(dir, "claim.json") - if err := manifest.Claim(claim); err != nil { - t.Fatalf("claiming a name: %v", err) + // And our own file IS removed, or the check above would pass against a + // clean-up that never removes anything. + mine, err := os.Lstat(path) + if err != nil { + t.Fatalf("asking what the file is: %v", err) } - if err := manifest.Release(claim); err != nil { - t.Fatalf("releasing our own claim: %v", err) + if err := core.RemoveOwn(path, mine); err != nil { + t.Fatalf("removing a file by its own identity: %v", err) } - if _, err := os.Stat(claim); err == nil { - t.Error("an empty claim survived being given back, so a refused run leaves the name taken") + if _, err := os.Lstat(path); err == nil { + t.Error("a file survived being removed by its own identity, so a failed save would leave its name taken") } } @@ -125,6 +140,16 @@ func TestGivingAClaimedNameBackSparesAManifestWithContent(t *testing.T) { // them, and the number is checked at one as well as at several - a sentence // with a verb agreeing with the count reads wrong at exactly one of those, and // core.Count carries a paragraph about that mistake. +// +// How the save is made to fail changed on 2026-09-29 (O252). This used to put a +// directory under the temporary name before the run. That name is now the +// run's reservation, taken before the first file, so the same block refuses the +// run at the start and it writes nothing - and nothing else a person can do +// before a run makes the save fail at the end, which is the point of the +// change. So the block is put in place during the run: the moment the +// reservation appears, a directory goes under the manifest's final name. The +// files are large enough that the run is still writing them by then, and the +// guard asserts that it got there rather than assuming. func TestARunThatCannotSaveItsManifestSaysWhatItLeftBehind(t *testing.T) { for _, c := range []struct { count int @@ -135,14 +160,16 @@ func TestARunThatCannotSaveItsManifestSaysWhatItLeftBehind(t *testing.T) { } { dir := t.TempDir() out := filepath.Join(dir, "out") - if err := os.MkdirAll(filepath.Join(out, "manifest.json.tfg-writing"), 0o755); err != nil { - t.Fatalf("blocking the temporary name: %v", err) - } + final := filepath.Join(out, "manifest.json") + blocked := blockWhenReserved(final) code, _, errOut := run(t, "generate", - "--format", "txt", "--size", "1kb", + "--format", "txt", "--size", "16mb", "--count", itoa(c.count), "--out", out) + if !<-blocked { + t.Fatalf("count %d: the run finished before its manifest name could be blocked, so nothing here was tested", c.count) + } if code == 0 { t.Fatalf("count %d: the run ended with 0 although its manifest could not be written", c.count) } @@ -158,3 +185,27 @@ func TestARunThatCannotSaveItsManifestSaysWhatItLeftBehind(t *testing.T) { } } } + +// blockWhenReserved puts a directory under a manifest's final name the moment +// the run reserves it, and says on the channel whether it did so while the run +// still had the reservation - that is, before the save. +func blockWhenReserved(final string) <-chan bool { + done := make(chan bool, 1) + reservation := manifest.ReservationPath(final) + go func() { + deadline := time.Now().Add(30 * time.Second) + for time.Now().Before(deadline) { + if _, err := os.Lstat(reservation); err == nil { + if err := os.Mkdir(final, 0o755); err != nil { + done <- false + return + } + _, stillReserved := os.Lstat(reservation) + done <- stillReserved == nil + return + } + } + done <- false + }() + return done +} diff --git a/internal/guard/publish_test.go b/internal/guard/publish_test.go new file mode 100644 index 00000000..dc287973 --- /dev/null +++ b/internal/guard/publish_test.go @@ -0,0 +1,322 @@ +package guard + +import ( + "context" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" + "sync" + "sync/atomic" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/core" + "github.com/donislawdev/TestingFilesGenerator/internal/engine" + "github.com/donislawdev/TestingFilesGenerator/internal/manifest" +) + +// Every finished file gets its name through core.Publish, which never replaces +// a name somebody holds. The window it closes is O252, measured on 2026-09-29 +// with a second writer spinning on the same names: a rename lost that writer's +// file 2544 times in 3000 on NTFS and around 1900 in 1950 on ext4, tmpfs and +// overlay. docs/O252-ANALIZA-2026-09-29.md has the table. + +// The three answers the primitive gives, on an ordinary disk. +func TestPublishNeverReplacesANameSomebodyHolds(t *testing.T) { + cases := []struct { + what string + theirs []byte // nil: the name is free + }{ + {"a free name", nil}, + {"a name holding somebody's file", []byte("THEIRS")}, + {"a name holding an empty file, the shape a claim used to have", []byte{}}, + } + for _, c := range cases { + t.Run(c.what, func(t *testing.T) { + dir := t.TempDir() + tmp, final := filepath.Join(dir, "f.txt.tfg-writing"), filepath.Join(dir, "f.txt") + writeOrFail(t, tmp, []byte("OURS")) + if c.theirs != nil { + writeOrFail(t, final, c.theirs) + } + err := core.Publish(tmp, final) + assertPublished(t, err, tmp, final, c.theirs) + }) + } +} + +// The fallbacks refuse a taken name the same way the call does. +// +// They are reached only on a filesystem that does not know the call - some +// network shares, macOS on a FAT stick - and no runner has one. A fallback +// nothing can reach is a defence nothing can turn red, so the chain is walked +// here through core.PublishThrough, with the calls it would fall back from +// answering "unsupported". +func TestPublishFallbacksRefuseATakenNameToo(t *testing.T) { + unsupported := func(string, string) error { return errors.ErrUnsupported } + chains := []struct { + what string + link func(string, string) error + }{ + {"through a hard link", os.Link}, + {"through a look and a rename, the last resort", unsupported}, + } + for _, chain := range chains { + for _, theirs := range [][]byte{nil, []byte("THEIRS")} { + name := chain.what + ", free name" + if theirs != nil { + name = chain.what + ", taken name" + } + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + tmp, final := filepath.Join(dir, "f.txt.tfg-writing"), filepath.Join(dir, "f.txt") + writeOrFail(t, tmp, []byte("OURS")) + if theirs != nil { + writeOrFail(t, final, theirs) + } + err := core.PublishThrough(tmp, final, unsupported, chain.link) + assertPublished(t, err, tmp, final, theirs) + }) + } + } + + // And a failure that is not "unsupported" is the answer, not a reason to + // try the next way. A permission refused and then walked round by a + // rename would be the tool deciding it knows better than the system. + t.Run("a real failure stops the chain", func(t *testing.T) { + dir := t.TempDir() + tmp, final := filepath.Join(dir, "f.txt.tfg-writing"), filepath.Join(dir, "f.txt") + writeOrFail(t, tmp, []byte("OURS")) + linked := false + err := core.PublishThrough(tmp, final, + func(string, string) error { return fs.ErrPermission }, + func(string, string) error { linked = true; return nil }) + if !errors.Is(err, fs.ErrPermission) { + t.Errorf("a refused permission came back as %v", err) + } + if linked { + t.Error("the chain went on to a hard link after a failure that was not \"unsupported\"") + } + if _, err := os.Lstat(final); err == nil { + t.Error("the file got its name after the system refused it") + } + }) +} + +// A second writer spinning on the same names never loses its file. +// +// The race itself rather than its shape, because the shape is what the two +// guards above ask and a race is what O252 was about. The other writer takes a +// name with an exclusive create whenever it finds the name free, which is what +// another program - or a person - does. It is a goroutine rather than a +// process, and the window does not care: it lies between two calls to the +// system. +// +// Asserted both ways. The other writer has to have taken names at all, or this +// passes against a race nobody entered - measured, it takes nearly every one. +func TestAWriterSpinningOnTheNameNeverLosesItsFile(t *testing.T) { + const names = 300 + dir := t.TempDir() + nameOf := func(i int64) string { return filepath.Join(dir, fmt.Sprintf("f%04d.txt", i)) } + + var current atomic.Int64 + current.Store(-1) + took := make([]atomic.Bool, names) + stop := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + } + i := current.Load() + if i < 0 { + continue + } + f, err := os.OpenFile(nameOf(i), os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) + if err != nil { + continue + } + _, _ = f.WriteString("THEIRS") + _ = f.Close() + took[i].Store(true) + } + }() + + for i := int64(0); i < names; i++ { + tmp := nameOf(i) + ".tfg-writing" + writeOrFail(t, tmp, []byte("OURS")) + current.Store(i) + if err := core.Publish(tmp, nameOf(i)); err != nil { + _ = os.Remove(tmp) + } + } + close(stop) + wg.Wait() + + taken, lost := 0, 0 + for i := int64(0); i < names; i++ { + if !took[i].Load() { + continue + } + taken++ + if got, _ := os.ReadFile(nameOf(i)); string(got) != "THEIRS" { + lost++ + } + } + if taken == 0 { + t.Fatal("the other writer took no name at all, so nothing here was raced") + } + if lost != 0 { + t.Errorf("the other writer took %d names and lost its file under %d of them", taken, lost) + } + t.Logf("the other writer took %d of %d names and kept every file", taken, names) +} + +// A file somebody puts under a planned name while the run writes it is left as +// it is, and that one file fails in its own words. +// +// Through the engine rather than the primitive, because the primitive being +// right says nothing about the run calling it - a writer that went back to +// os.Rename would leave every guard above green. The file is put there from +// the progress report, which the run makes while that same file is still being +// written: after the preflight has looked, before the file gets its name. +func TestAFileTakenDuringTheRunIsNotWrittenOver(t *testing.T) { + dir := t.TempDir() + var planted string + opt := engine.Options{ + OutDir: dir, Seed: 7741, Command: "test", + ManifestName: engine.DefaultManifestName, + } + planned, err := engine.Plan([]engine.Target{txtTarget("files", 1, 4<<20)}, opt) + if err != nil { + t.Fatalf("planning: %v", err) + } + final := filepath.Join(dir, planned[0].Name) + opt.OnProgress = func(engine.Progress) { + if planted != "" { + return + } + planted = final + if err := os.WriteFile(final, []byte("THEIRS"), 0o644); err != nil { + t.Errorf("putting a file under the planned name: %v", err) + } + } + + res, _ := engine.Run(context.Background(), planned, opt) + if planted == "" { + t.Fatal("the run reported no progress while writing, so nothing was put in its way") + } + if got, err := os.ReadFile(final); err != nil || string(got) != "THEIRS" { + t.Errorf("the file put under %s during the run was written over: %q, %v", planned[0].Name, got, err) + } + if res == nil || res.Failures != 1 { + t.Errorf("the run should report the one file it could not name, and reported %v", res) + } +} + +// A reservation swapped for a link while the run goes is not written through. +// +// The reservation is closed as soon as it is made, so that a run which never +// saves leaves nothing open (a Windows directory with an open file in it +// cannot be removed). The save opens it again, and between the two somebody +// can put a hard link to their own file under its name - which needs no +// privilege on Windows. The save has to ask the file it opened, not the name. +func TestAReservationSwappedForALinkIsNotWrittenThrough(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "manifest.json") + victim := filepath.Join(t.TempDir(), "notes.txt") + writeOrFail(t, victim, []byte("ORIGINAL")) + + r, err := manifest.Claim(path) + if err != nil { + t.Fatalf("reserving: %v", err) + } + reservation := manifest.ReservationPath(path) + if err := os.Remove(reservation); err != nil { + t.Fatalf("taking the reservation away: %v", err) + } + if err := os.Link(victim, reservation); err != nil { + t.Fatalf("putting a hard link under the reservation's name: %v", err) + } + + m := manifest.New("testing-files-generator", "0.0.0-test", "run_x", "tfg generate", 1, "windows", "amd64") + if err := r.Save(m); err == nil { + t.Error("the manifest was saved through a reservation somebody swapped for a link") + } + if got, err := os.ReadFile(victim); err != nil || string(got) != "ORIGINAL" { + t.Errorf("the manifest was written into a file outside the directory: %q, %v", got, err) + } + if _, err := os.Lstat(path); err == nil { + t.Error("a manifest got its name after the save refused") + } +} + +// A reservation left by a killed run stops the next run, a dry run included, +// and says what it is. +// +// A real run meets it twice - in the preflight and again when it reserves the +// name - but a dry run stops before reserving anything, so for a preview the +// preflight is the whole answer. A preview that says a run would succeed in a +// directory where the run will be refused answers a question nobody asked. +func TestALeftReservationStopsEvenADryRun(t *testing.T) { + out := filepath.Join(t.TempDir(), "out") + if err := os.MkdirAll(out, 0o755); err != nil { + t.Fatalf("making the directory: %v", err) + } + left := manifest.ReservationPath(filepath.Join(out, engine.DefaultManifestName)) + writeOrFail(t, left, nil) + + code, _, errOut := run(t, "generate", "--format", "txt", "--size", "1kb", "--dry-run", "--out", out) + if code == 0 { + t.Fatal("a dry run said yes in a directory where the run itself will be refused") + } + if !strings.Contains(errOut, "another run is already writing") || !strings.Contains(errOut, filepath.Base(left)) { + t.Errorf("the refusal does not say a run is going or was killed, and name the file to remove:\n%s", errOut) + } +} + +func writeOrFail(t *testing.T, path string, content []byte) { + t.Helper() + if err := os.WriteFile(path, content, 0o644); err != nil { + t.Fatalf("writing %s: %v", filepath.Base(path), err) + } +} + +// assertPublished holds one publish to what it should have done: ours under +// the name when it was free, and theirs untouched with ours kept aside when it +// was not. +func assertPublished(t *testing.T, err error, tmp, final string, theirs []byte) { + t.Helper() + got, readErr := os.ReadFile(final) + if readErr != nil { + t.Fatalf("nothing under the final name afterwards: %v", readErr) + } + if theirs == nil { + if err != nil { + t.Fatalf("a free name was refused: %v", err) + } + if string(got) != "OURS" { + t.Errorf("the free name holds %q rather than what was written", got) + } + if _, err := os.Lstat(tmp); err == nil { + t.Error("the temporary name is still there after the file got its own") + } + return + } + if !errors.Is(err, fs.ErrExist) { + t.Errorf("a taken name did not come back as fs.ErrExist: %v", err) + } + if string(got) != string(theirs) { + t.Errorf("the file under a taken name was written over: %q", got) + } + if _, err := os.Lstat(tmp); err != nil { + t.Errorf("the refused file is gone from its temporary name, which is the caller's to remove: %v", err) + } +} diff --git a/internal/guard/runlock_test.go b/internal/guard/runlock_test.go index 5f643e59..7da1a935 100644 --- a/internal/guard/runlock_test.go +++ b/internal/guard/runlock_test.go @@ -185,6 +185,11 @@ func TestVerifyNamesTheRunLockAsOursRatherThanAsSomebodyElses(t *testing.T) { if err != nil { t.Fatalf("running: %v", err) } + // Saved the way both surfaces save, so the manifest's reservation is gone + // and the lock planted below is the one thing verify has to name. + if _, err := engine.SaveRecord(res, opt); err != nil { + t.Fatalf("saving: %v", err) + } if err := os.WriteFile(engine.RunLockPath(dir), nil, 0o644); err != nil { t.Fatalf("standing in for a run that was killed: %v", err) } diff --git a/internal/guard/safety_test.go b/internal/guard/safety_test.go index 335af627..f6ffb4a2 100644 --- a/internal/guard/safety_test.go +++ b/internal/guard/safety_test.go @@ -13,6 +13,7 @@ import ( "github.com/donislawdev/TestingFilesGenerator/internal/core" "github.com/donislawdev/TestingFilesGenerator/internal/engine" _ "github.com/donislawdev/TestingFilesGenerator/internal/format/all" + "github.com/donislawdev/TestingFilesGenerator/internal/manifest" ) // This tool writes large amounts of data and it runs in directories that @@ -285,9 +286,11 @@ func TestARunStoppedPartWayNamesEveryFileThatFinished(t *testing.T) { onDisk := map[string]bool{} for _, e := range entries { name := e.Name() - if name == engine.DefaultManifestName { - // The run takes this name before the first file and it holds no - // entry of its own. + if name == filepath.Base(manifest.ReservationPath(engine.DefaultManifestName)) { + // The run reserves its manifest's name before the first file, under + // the name the manifest is written through, and this run is never + // saved. It holds no entry of its own. Until 2026-09-29 the + // reservation was an empty file under the manifest's own name. continue } if strings.Contains(name, core.PartialMarker) { diff --git a/internal/guard/writeescape_test.go b/internal/guard/writeescape_test.go index 983dc452..f28e7460 100644 --- a/internal/guard/writeescape_test.go +++ b/internal/guard/writeescape_test.go @@ -142,13 +142,17 @@ func TestEveryFileThisToolWritesIsCreatedThroughOneClaim(t *testing.T) { // reports a hard link as an ordinary file, because that // is what it is. This is the shape that needs no // privilege on Windows -// a link to nothing O_EXCL says "it exists" and the fallback has to agree. +// a link to nothing O_EXCL says "it exists", and the refusal has to stand. // os.Stat follows the link and says the name is free, // which is exactly how the manifest escaped // -// The last one is why the fallback asks os.Lstat. The fallback exists because -// O_EXCL lies on Windows when the path runs through a reparse point - measured -// 2026-08-03 - so it cannot simply be taken away. +// The last one is why the refusal is read with os.Lstat. Until 2026-09-29 a +// refusal os.Lstat did not confirm was followed by a second create without +// O_EXCL, because O_EXCL lied on Windows when the path ran through a reparse +// point (measured 2026-08-03, O47). Go 1.27 no longer lies through a junction, +// measured that day, and that second create is gone - it was the first window +// of O252. TestADirectoryReachedThroughALinkStillWorks is what says whether a +// symbolic link agrees, on the runners that may make one. func TestCreateNewCreatesOnlyWhenTheNameIsFree(t *testing.T) { t.Run("a free name is created", func(t *testing.T) { dir := t.TempDir() @@ -265,7 +269,8 @@ func TestAHeldTemporaryNameStopsTheWriteRatherThanGoingThroughIt(t *testing.T) { skipIfLinksAreNotAllowed(t, err) } - if err := manifest.Claim(path); err == nil { + if r, err := manifest.Claim(path); err == nil { + _ = r.Release() t.Fatal("the name was claimed through a link pointing at nothing") } if _, err := os.Stat(target); err == nil { diff --git a/internal/legal/modules.go b/internal/legal/modules.go index a4470ce0..7b68bc1c 100644 --- a/internal/legal/modules.go +++ b/internal/legal/modules.go @@ -14,13 +14,13 @@ package legal // reads like an oversight. var modules = []Module{ {Path: "github.com/goccy/go-yaml", SPDX: "MIT", Copyright: "(c) 2019 Masaaki Goshima", - Note: "Reads the recipe file. One of the four modules in the command line binary."}, + Note: "Reads the recipe file. Linked into the command line binary."}, {Path: "github.com/gen2brain/gav1d", SPDX: "BSD-2-Clause", Copyright: "(c) 2018-2025 VideoLAN and dav1d authors, (c) 2016 Alliance for Open Media", - Note: "Codes and reads AV1, which is what an AVIF picture is made of. Pure Go with no module of its own behind it, so it brings nothing else along. It also ships an AOM Patent License 1.0, which travels with the notices. One of the four modules in the command line binary."}, + Note: "Codes and reads AV1, which is what an AVIF picture is made of. Pure Go with no module of its own behind it, so it brings nothing else along. It also ships an AOM Patent License 1.0, which travels with the notices. Linked into the command line binary."}, {Path: "github.com/gen2brain/jxl", SPDX: "BSD-3-Clause", Copyright: "(c) the JPEG XL Project Authors", - Note: "Codes JPEG XL. Pure Go with no module of its own behind it, so it brings nothing else along. It also ships a patent grant from Google covering this implementation of JPEG XL, which travels with the notices. One of the four modules in the command line binary."}, + Note: "Codes JPEG XL. Pure Go with no module of its own behind it, so it brings nothing else along. It also ships a patent grant from Google covering this implementation of JPEG XL, which travels with the notices. Linked into the command line binary."}, {Path: "golang.org/x/text", SPDX: "BSD-3-Clause", Copyright: "Copyright 2009 The Go Authors", - Note: "Unicode normalisation, used to decide whether two file names are one name spelled two ways. One of the four modules in the command line binary."}, + Note: "Unicode normalisation, used to decide whether two file names are one name spelled two ways. Linked into the command line binary."}, {Path: "std", SPDX: "BSD-3-Clause", Copyright: "Copyright 2009 The Go Authors", Note: "The Go runtime and standard library, linked into every binary here. Not a module, so `go list -deps` never names it - the one entry this list carries that no build can report."}, {Path: "fyne.io/fyne/v2", SPDX: "BSD-3-Clause", Copyright: "(C) 2018 Fyne.io developers (see AUTHORS)"}, @@ -50,5 +50,6 @@ var modules = []Module{ {Path: "github.com/yuin/goldmark", SPDX: "MIT", Copyright: "(c) 2019 Yusuke Inuzuka"}, {Path: "golang.org/x/image", SPDX: "BSD-3-Clause", Copyright: "2009 The Go Authors."}, {Path: "golang.org/x/net", SPDX: "BSD-3-Clause", Copyright: "2009 The Go Authors."}, - {Path: "golang.org/x/sys", SPDX: "BSD-3-Clause", Copyright: "2009 The Go Authors."}, + {Path: "golang.org/x/sys", SPDX: "BSD-3-Clause", Copyright: "2009 The Go Authors.", + Note: "Linked into the command line binary on Linux and macOS since 2026-09-29, for the one call that gives a finished file its name without replacing anything already there (renameat2 and renamex_np). The Windows build makes the same call through the standard library, because golang.org/x/sys/windows brings in the network package."}, } diff --git a/internal/manifest/instructions.go b/internal/manifest/instructions.go index abc99888..a8de5bc4 100644 --- a/internal/manifest/instructions.go +++ b/internal/manifest/instructions.go @@ -2,7 +2,7 @@ package manifest import ( "fmt" - "io" + "os" "path/filepath" "strings" @@ -244,24 +244,16 @@ func codeSpan(s string) string { return fence + s + fence } -// SaveInstructions writes the instructions under a name nobody holds. +// SaveInstructions writes the instructions under a name nobody holds, and says +// what the file is, so a caller taking them back after a failed manifest +// removes this file and nothing that took its name since. // -// Claimed at the moment of writing rather than before the first file, like the -// manifest is. The manifest's claim already keeps two runs of one record apart, -// and this name is the manifest's own with a different ending - so the only run -// that could reach it is one refused before it wrote anything. The claim is -// here for the case that leaves: a file put there by hand while the run went. -func SaveInstructions(path string, text []byte) error { - if err := claimName(path); err != nil { - return err - } - err := writeClaimed(path, 0o644, func(w io.Writer) error { - _, err := w.Write(text) - return err - }) - if err != nil { - _ = Release(path) - return err - } - return nil +// Not reserved before the first file, unlike the manifest. The manifest's +// reservation already keeps two runs of one record apart, and this name is the +// manifest's own with a different ending - so the only run that could reach it +// is one refused before it wrote anything. What is left is a file put there by +// hand while the run went, and core.WriteNew refuses that one rather than +// writing over it. +func SaveInstructions(path string, text []byte) (os.FileInfo, error) { + return core.WriteNew(path, text, 0o644) } diff --git a/internal/manifest/manifest.go b/internal/manifest/manifest.go index c782af2c..f6cb41eb 100644 --- a/internal/manifest/manifest.go +++ b/internal/manifest/manifest.go @@ -696,8 +696,8 @@ func major(v string) string { return v } -// Save writes the manifest, claiming the name before it writes and never -// writing over a manifest that is already there. +// Save writes the manifest under a name nothing holds, and never writes over a +// manifest that is already there. // // Two failures shaped this, and neither is hypothetical. // @@ -707,106 +707,190 @@ func major(v string) string { // wrote, and one manifest replaced the other. Measured on 2026-08-03 with two // runs of eight files under different ids - both ended with exit code 0, // sixteen files were on the disk, one manifest described eight of them, and the -// other eight could never be removed by this tool again. O_EXCL closes that, -// because creating the name and finding out whether it existed become one -// operation the operating system settles. +// other eight could never be removed by this tool again. // // The second is the process ending part way through the write. Every generated -// file goes through a temporary name and a rename for exactly this reason, -// while the manifest - the one file that can remove all the others - was -// written in place. A truncated manifest does not parse, so the record of a -// finished run would be lost to a Ctrl+C landing in the wrong second. -// -// So the name is claimed first, the content is written beside it, and the -// rename puts it in place in one step. -// Claim takes the manifest name before a run writes anything. -// -// Claiming at save time was already better than checking in advance - two runs -// could no longer both write - but it happened after the last file, so the -// second run wrote its whole set and only then found out it had nowhere to -// record them. Measured on 2026-08-03: two runs started together under -// different ids ended 0 and 5, with sixteen files on the disk and eight of them -// in nobody's manifest. That turned a silent loss into a loud partial run, -// which was the improvement, and this is the rest of it. -// -// The claim is an empty file under the final name. Save renames over it, so the -// window between them belongs to this run and nobody else can take the name. -func Claim(path string) error { - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { +// file goes through a temporary name for exactly this reason, while the +// manifest - the one file that can remove all the others - was written in +// place. A truncated manifest does not parse, so the record of a finished run +// would be lost to a Ctrl+C landing in the wrong second. +// +// So the manifest is written whole under a temporary name and given its own by +// core.Publish, which refuses a name somebody holds rather than replacing it. +// +// A run reserves the name before its first file and saves through that +// Reservation. This is for every other caller - the guards, and anything that +// writes a manifest it did not reserve - and it reserves and saves in one go. +func (m *Manifest) Save(path string) error { + r, err := Claim(path) + if err != nil { return err } - return claimName(path) -} - -// Release gives the name back, for a run that claimed it and then could not -// start. Without it a refused run would leave an empty manifest behind and the -// next run into that directory would be refused for a file nobody wrote. -func Release(path string) error { - if info, err := os.Stat(path); err != nil || info.Size() != 0 { - // Somebody filled it in, so it is not ours to remove. - return nil + return r.Save(m) +} + +// Reservation is a run's hold on the name its manifest will take. +// +// Taken before the first file, not after the last one. Claiming at save time +// already stopped two runs from both writing a manifest, but it happened at the +// end - so a second run wrote its whole set of files and only then found out it +// had nowhere to record them. Measured on 2026-08-03: two runs started together +// under different ids ended 0 and 5, with sixteen files on the disk and eight +// of them in nobody's manifest (O43). +// +// Until 2026-09-29 the hold was an empty file under the FINAL name, and the save +// renamed over it. A rename replaces whatever it lands on, so a file somebody +// else put there between the claim and the save was destroyed without a word - +// one of the three windows of O252, measured that day with a second writer +// spinning on the name. The hold is now the temporary name the manifest is +// written under, created exclusively when the run starts and written through +// at the save. A second run still cannot take it and is refused before its first +// file, and the final name is never written over. +// +// What lies on the disk while a run goes is therefore ".tfg-writing" +// and no manifest at all. A run killed outright leaves that name behind rather +// than an empty manifest, and that is better as well as safer: the empty +// manifest made the next run say "the only record of what an earlier run +// wrote" about a file that recorded nothing, and sent somebody looking for a +// run whose files it could not name. The leftover name makes the next run say +// that a run is going or was killed, and name the file to remove. +type Reservation struct { + final string + tmp string + // own is what the reservation file is. It is closed as soon as it is + // made - a file held open from the start of a run to its save keeps a + // Windows directory from being removed by anybody who ran the engine and + // never saved, measured on 2026-09-29 when a guard's own clean-up failed + // on it. So the save opens it again and asks the OPENED file whether it + // is still this one (core.OpenOwn), which is what stops a name swapped for + // a link in the hours a large run takes. A clean-up removes it only while + // it is still this one too. + own os.FileInfo + // used says the reservation was saved through or given back. + used bool +} + +// ReservationPath is the name a run holds while it goes, beside the manifest. +func ReservationPath(path string) string { + return core.SiblingPath(path, core.WritingMarker) +} + +// Claim reserves the name a manifest will take, before a run writes anything. +// +// A manifest already under that name is refused here rather than at the save, +// in the words that always named it, so a run does not write its files first. +// A reservation somebody already holds comes back as core.NameTakenError about +// the reservation's own name, which is how the engine tells a run in progress +// from a finished one. +func Claim(path string) (*Reservation, error) { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return nil, err + } + // "Nothing is there" and "I could not look" are two answers. Reading every + // failure of the look as an empty slot sent a path nobody could examine on + // to words about a manifest that already existed - a sentence about the + // wrong thing, and the one somebody would act on (review 2026-08-23, 3.7c). + switch _, err := os.Lstat(path); { + case err == nil: + return nil, &os.PathError{Op: "save", Path: path, Err: fs.ErrExist} + case !errors.Is(err, fs.ErrNotExist): + return nil, err + } + tmp := ReservationPath(path) + // Created exclusively, and core.CreateNew says why: this name sits in a + // directory the run does not own, and a create that is not exclusive + // follows whatever is at the name. Measured on 2026-09-06 - a link here + // put the manifest on a file outside the output directory and the run + // still exited 0. + f, err := core.CreateNew(tmp, 0o666) + if err != nil { + return nil, err + } + own, err := core.Finish(f, nil) + if err != nil { + _ = core.RemoveOwn(tmp, own) + return nil, err } - return os.Remove(path) + return &Reservation{final: path, tmp: tmp, own: own}, nil } -func (m *Manifest) Save(path string) error { - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return err +// Save writes the manifest through the reservation and gives it its name. +// +// Whatever fails, the reservation goes with it - but only while its name still +// holds the file this run created. A save can be asked once. +func (r *Reservation) Save(m *Manifest) error { + if r.used { + return fmt.Errorf("the reservation of %s was already used or given back", core.Shown(r.final)) } - - // A run that got this far claimed the name before it wrote a byte, and the - // claim is an empty file. Anything with content in it is somebody's - // manifest and is never written over - that is the whole point of the - // claim, and it is why "it exists" is not enough to go on here. - switch info, err := os.Stat(path); { - case errors.Is(err, fs.ErrNotExist): - // Nothing there. A caller that writes a manifest without claiming - // first - the guards do - claims it now. - if err := claimName(path); err != nil { - return err - } - case err != nil: - // Something is there and it cannot be looked at - a permission, a - // path whose parent is a file, a name the host will not take. Read as - // "nothing there" until 2026-08-25, which sent the run on to claim a - // name it had no answer about, and the claim then failed in words - // about the wrong thing. + r.used = true + f, err := core.OpenOwn(r.tmp, r.own) + if err != nil { return err - case info.Size() != 0: - return &os.PathError{Op: "save", Path: path, Err: fs.ErrExist} } - - // Past here the name is ours: either nothing was there and the claim above - // took it, or what was there is the empty claim this run made before its - // first file. So a failure from here on has to give the name back. - // - // Measured on 2026-08-27, review item S2, by putting a directory under the - // temporary name and running an ordinary generate: three files were written - // and exit was 5, which is right - and a nought byte manifest.json was left - // sitting beside them, which is not. What that costs was measured too, and - // it is three things rather than the one the review named: - // - // cleanup exit 5, "unexpected end of JSON input" - the files cannot be - // removed by the only thing allowed to remove them - // verify exit 5, the same - // generate refused, and the refusal SAYS the file "is the only record of - // what an earlier run wrote" about a file that records nothing - // - // The last of those is the worst, because it is a true sentence in every - // other case and a false one here, and it sends somebody looking for a run - // whose files it cannot name. Giving the name back turns all three into the - // honest situation: files nobody recorded, in a directory that says so by - // naming a FILE the next run collides with rather than a phantom manifest. - if err := m.writeOver(path); err != nil { - // Only ever removes a nought byte file - see Release. So a manifest - // somebody else wrote cannot be taken away by a failure of ours, which - // is the property that makes this safe to do on every error below. - _ = Release(path) + own, err := saveReserved(f, m, r.tmp, r.final) + if own != nil { + r.own = own + } + if err != nil { + _ = core.RemoveOwn(r.tmp, r.own) return err } return nil } +// Release gives the name back, for a run that reserved it and then had nothing +// to record. Without it a refused run would leave its reservation behind and the +// next run into that directory would be told a run is going. +func (r *Reservation) Release() error { + if r == nil || r.used { + return nil + } + r.used = true + return core.RemoveOwn(r.tmp, r.own) +} + +// saveReserved fills the reservation, flushes it, closes it and gives it the +// manifest's name, and says what the file is - asked after the write, because a +// clean-up compares the size and the time as well as the file +// (core.RemoveOwn). +// +// Written as one straight sequence on purpose. The guard that asks whether the +// bytes reach the disk before the name does reads the steps of this function +// in order, and a step hidden in the body of an if is a step it cannot see. +// +// The mode is set on the opened file, before anything is in it, because the +// publish moves the file and its mode with it. So this is where a manifest +// carrying a password stops being readable by every account on the machine. +// The ordinary mode is the one the create left, umask and all. +// +// On the device before the publish, because the publish is what turns this +// into the manifest and a rename can reach the disk before the bytes do. What +// survives that is an empty file under the name of the only record able to +// remove a run's files - the loss this whole function is shaped against, +// reached by pulling the plug rather than by killing the process. One call per +// run, so the cost argument that keeps generated files unsynced does not reach +// here. That one is written on engine.Run and it is about ten thousand +// flushes, not one. docs/CODE-REVIEW-2026-08-23.md section 3.4, owner's call on +// 2026-08-25. +func saveReserved(f *os.File, m *Manifest, tmp, final string) (os.FileInfo, error) { + if mode := m.mode(); mode != 0o666 { + if err := f.Chmod(mode); err != nil { + return core.Finish(f, err) + } + } + if err := m.Encode(f); err != nil { + return core.Finish(f, err) + } + if err := f.Sync(); err != nil { + return core.Finish(f, err) + } + own, err := core.Finish(f, nil) + if err != nil { + return own, err + } + return own, core.Publish(tmp, final) +} + // secretProperties are the property names whose value is a credential rather // than a description of a file. // @@ -870,96 +954,6 @@ func holdsACredential(props map[string]any) bool { return false } -// writeOver puts the manifest under a name this run already owns. -// -// Split out of Save so that every way of failing gives the name back, rather -// than the four early returns below each having to remember to. The rule is -// "the claim goes when the write does", and a rule spelled once cannot be -// half applied. -func (m *Manifest) writeOver(path string) error { - return writeClaimed(path, m.mode(), m.Encode) -} - -// writeClaimed writes a file over the empty claim this run holds on its name, -// through a temporary name and a rename. The manifest and the instructions -// beside it are written the same way, so the reasons below hold for both. -func writeClaimed(path string, mode os.FileMode, write func(io.Writer) error) error { - // The marker comes from core rather than being spelled here. It was a bare - // literal until 2026-09-06, which is how verify came to report our own half - // written manifest as "extra" - the reading side recognised the other - // marker and had never been told about this one. - // - // A sibling rather than a plain join, so a manifest named up to the length - // every system stores can be written under its temporary name (O239). - tmp := core.SiblingPath(path, core.WritingMarker) - // Claimed rather than created, and core.CreateNew says why: this name sits - // in a directory the run does not own, nothing else in the tool checks it, - // and a create that is not exclusive follows whatever is at the name. - // Measured on 2026-09-06 - a link here put the manifest on a file outside - // the output directory and the run still exited 0. - // - // The mode is the temporary file's, because the rename below moves the file - // and its mode with it. So this is where a manifest carrying a password - // stops being readable by every account on the machine. - f, err := core.CreateNew(tmp, mode) - if err != nil { - return err - } - if err := write(f); err != nil { - _ = f.Close() - _ = os.Remove(tmp) - return err - } - // On the device before the rename, because the rename is what turns this - // into the manifest and a rename can reach the disk before the bytes do. - // What survives that is an empty file under the name of the only record - // able to remove a run's files - the loss this whole function is shaped - // against, reached by pulling the plug rather than by killing the process. - // - // One call per run, so the cost argument that keeps generated files - // unsynced does not reach here. That one is written on engine.Run and it is - // about ten thousand flushes, not one. docs/CODE-REVIEW-2026-08-23.md - // section 3.4, owner's call on 2026-08-25. - if err := f.Sync(); err != nil { - _ = f.Close() - _ = os.Remove(tmp) - return err - } - if err := f.Close(); err != nil { - _ = os.Remove(tmp) - return err - } - // Over our own claim, which is why this rename is allowed to replace - // something. Nobody else can be holding that name. - if err := os.Rename(tmp, path); err != nil { - _ = os.Remove(tmp) - return err - } - return nil -} - -// claimName creates the file only if nobody else holds the name. -// -// How that question is settled, and why creating the file is the only way to -// ask it, is in core.CreateNew - together with the measurement of what Windows -// answers when the path runs through a reparse point. -// -// It moved there on 2026-09-06. This function had the better half of the answer -// and asked os.Stat, which follows a link: a link pointing at nothing answered -// "there is nothing here" and the create went through it. The temporary name -// beside this one had no half of the answer at all. One rule spelled in two -// places is one rule with a hole in it, so now there is one place. -// -// What was measured here stays true of the fallback: it leaves the narrow -// window two runs starting together could meet - see O43. -func claimName(path string) error { - f, err := core.CreateNew(path, 0o644) - if err != nil { - return err - } - return f.Close() -} - // readAtMost reads a manifest and refuses one that is over the ceiling. // // One rule in one place, and it used to be two. A look at the directory entry From 96a997e6920940f5d12c41795fc7ee7c344c3a0b Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 29 Sep 2026 10:08:20 +0200 Subject: [PATCH 2/4] ci: the command line links golang.org/x/sys on Linux and macOS The dependency step held the command line binary to exactly four modules on every runner. Since the previous commit it links a fifth on Linux and macOS - golang.org/x/sys/unix, for renameat2 and renamex_np - and still four on Windows, where the same call comes from syscall because golang.org/x/sys/windows imports net. The expected set now follows the runner, and the comment says why. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8d9a39f9..44cb5c01 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -172,6 +172,15 @@ jobs: # command line binary links exactly four external modules and the # toolkit is not among them, so a build for a server carries no # window, no OpenGL and - see internal/guard - no socket. + # + # Five on Linux and macOS since 2026-09-29 (O252): golang.org/x/sys, + # for the one call that gives a finished file its name without + # replacing anything there - renameat2 with RENAME_NOREPLACE and + # renamex_np with RENAME_EXCL. Already in the graph at the same + # version, BSD-3-Clause read from its LICENSE file. Not on Windows: + # golang.org/x/sys/windows imports net, which the command line may not + # link, so Windows makes the same call through syscall. The answer + # therefore depends on the runner, and the list says so. run: | set -euo pipefail # Built with printf rather than written across several lines. A @@ -266,6 +275,9 @@ jobs: linked=$(go list -deps -tags "$(cat .github/build-tags)" -f '{{if .Module}}{{.Module.Path}}{{end}}' ./cmd/tfg | LC_ALL=C sort -u | grep -v '^github.com/donislawdev/TestingFilesGenerator$' | grep .) wanted=$(printf '%s\n' github.com/gen2brain/gav1d github.com/gen2brain/jxl github.com/goccy/go-yaml golang.org/x/text) + if [ "$RUNNER_OS" != "Windows" ]; then + wanted=$(printf '%s\n' $wanted golang.org/x/sys | LC_ALL=C sort) + fi if [ "$linked" != "$wanted" ]; then echo "the command line binary links a different set of modules." echo "expected: $wanted" From 18c35dc82ee90c6375223174f596de53d0360ae1 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 29 Sep 2026 10:19:50 +0200 Subject: [PATCH 3/4] write: the manifest's reservation in a file of its own, and Run back under its ceilings CI measured what the previous commit grew: engine.Run at 79 lines and 23 decision points against 75 and 22, and manifest.go at 415 lines against 401. The reservation of the manifest's name moves to internal/manifest/reservation.go, and taking it - with the three refusals it turns into their own words - leaves Run for reserveManifest. The file ceiling follows the measurement down to 399, the longest file now being format/zip/zip.go. Co-Authored-By: Claude Opus 5.5 --- internal/engine/engine.go | 70 +++++++----- internal/guard/codeshape_test.go | 6 +- internal/guard/durability_test.go | 6 +- internal/manifest/manifest.go | 165 --------------------------- internal/manifest/reservation.go | 179 ++++++++++++++++++++++++++++++ 5 files changed, 228 insertions(+), 198 deletions(-) create mode 100644 internal/manifest/reservation.go diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 117dd280..d57d1d16 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -542,36 +542,10 @@ func Run(ctx context.Context, files []PlannedFile, opt Options) (*Result, error) defer releaseRunLock(lockPath, lock) // The manifest name is taken before the first file, not after the last one. - // - // Claiming it at save time already stopped two runs from both writing a - // manifest, but it happened at the end - so a second run wrote its whole set - // of files and only then found out it had nowhere to record them. Measured - // on 2026-08-03: two runs started together under different ids ended 0 and - // 5, with sixteen files on the disk and eight of them in nobody's manifest. - // Taking the name here turns that into a refusal before anything is written. - manifestPath := ManifestPath(opt) - reservation, err := manifest.Claim(manifestPath) - if err != nil { - // The reservation name held by somebody is a run going or a run - // killed, not a finished one - the same fault and the same remedy as - // the lock above, so the same words. Asked before the collision below, - // because that refusal is a fs.ErrExist too. - var held *core.NameTakenError - if errors.As(err, &held) { - return res, &RunInProgressError{Path: held.Path, Dir: filepath.Dir(manifestPath)} - } - // Only a name that is genuinely taken is a collision. Reporting every - // failure that way said "manifest.json already exists ... it is the - // only record of what an earlier run wrote" about an empty directory - // the user simply had no permission to write in - a sentence that is - // untrue and sends somebody looking for a run that never happened. - // Measured on 2026-08-04 with write denied on the output directory. - if errors.Is(err, fs.ErrExist) { - return res, &CollisionError{Path: manifestPath, Manifest: true} - } - return res, fmt.Errorf("cannot start a run in %s: %w", core.Shown(opt.OutDir), err) + // reserveManifest says why, and in which words a refusal comes back. + if res.reservation, err = reserveManifest(ManifestPath(opt), opt.OutDir); err != nil { + return res, err } - res.reservation = reservation // Past this point the run owns the name and may write. Started says so, and // it is what tells the caller a manifest is worth saving - set here rather @@ -644,6 +618,44 @@ func Run(ctx context.Context, files []PlannedFile, opt Options) (*Result, error) return res, nil } +// reserveManifest takes the name a run's manifest will have, before the first +// file is written, and turns a refusal into the words that fit it. +// +// Claiming it at save time already stopped two runs from both writing a +// manifest, but it happened at the end - so a second run wrote its whole set +// of files and only then found out it had nowhere to record them. Measured on +// 2026-08-03: two runs started together under different ids ended 0 and 5, +// with sixteen files on the disk and eight of them in nobody's manifest. +// Taking the name here turns that into a refusal before anything is written. +// +// Its own function since 2026-09-29, when the reservation moved to the +// manifest's temporary name (O252) and Run went past the size a person can +// follow. +func reserveManifest(manifestPath, outDir string) (*manifest.Reservation, error) { + reservation, err := manifest.Claim(manifestPath) + if err == nil { + return reservation, nil + } + // The reservation name held by somebody is a run going or a run killed, + // not a finished one - the same fault and the same remedy as the lock, + // so the same words. Asked before the collision below, because that + // refusal is a fs.ErrExist too. + var held *core.NameTakenError + if errors.As(err, &held) { + return nil, &RunInProgressError{Path: held.Path, Dir: filepath.Dir(manifestPath)} + } + // Only a name that is genuinely taken is a collision. Reporting every + // failure that way said "manifest.json already exists ... it is the only + // record of what an earlier run wrote" about an empty directory the user + // simply had no permission to write in - a sentence that is untrue and + // sends somebody looking for a run that never happened. Measured on + // 2026-08-04 with write denied on the output directory. + if errors.Is(err, fs.ErrExist) { + return nil, &CollisionError{Path: manifestPath, Manifest: true} + } + return nil, fmt.Errorf("cannot start a run in %s: %w", core.Shown(outDir), err) +} + // claimRunLock takes the name that says this directory has a run in it. // // core.CreateNew rather than os.Create, and that is the whole claim: it refuses diff --git a/internal/guard/codeshape_test.go b/internal/guard/codeshape_test.go index 4a52a91c..ba37535e 100644 --- a/internal/guard/codeshape_test.go +++ b/internal/guard/codeshape_test.go @@ -58,7 +58,11 @@ const ( // out of preset/uploadset.go into uploadfiles.go, when the instructions // beside the manifest took both past the ceiling. The longest file is // manifest/manifest.go now. - longestFile = 401 + // Lowered from 401 on 2026-09-29: a run's reservation of its manifest's + // name moved out of manifest/manifest.go into reservation.go when O252 + // took the file past the ceiling. The longest file is format/zip/zip.go + // now. + longestFile = 399 // Depth answers a different question than length, and it is the better // question of the two. A hundred line function that is flat reads top to diff --git a/internal/guard/durability_test.go b/internal/guard/durability_test.go index 18c5a369..8ecbeb89 100644 --- a/internal/guard/durability_test.go +++ b/internal/guard/durability_test.go @@ -38,7 +38,7 @@ func TestWhatIsRenamedIntoPlaceIsOnTheDiskFirst(t *testing.T) { // here would leave this guard reading, honestly and green, a function // nothing is written through any more. for _, via := range []struct{ file, caller, callee string }{ - {"internal/manifest/manifest.go", "Reservation.Save", "saveReserved("}, + {"internal/manifest/reservation.go", "Reservation.Save", "saveReserved("}, {"internal/core/writenew.go", "WriteNew", "writeAndKeep("}, } { if !strings.Contains(functionSource(t, via.file, via.caller), via.callee) { @@ -53,7 +53,7 @@ func TestWhatIsRenamedIntoPlaceIsOnTheDiskFirst(t *testing.T) { order []string }{ { - file: "internal/manifest/manifest.go", + file: "internal/manifest/reservation.go", // saveReserved since 2026-09-29 (O252). It was writeClaimed from // 2026-09-25 and writeOver before that, and Save before that - the // sequence moved with the code each time and this guard moved with @@ -211,7 +211,7 @@ func functionSource(t *testing.T, file, function string) string { // reservation (O252): Save now reserves and saves, and the reservation is where // the name is looked at. func TestSavingAManifestTellsAnEmptySlotFromAnUnreadableOne(t *testing.T) { - body := functionSource(t, "internal/manifest/manifest.go", "Claim") + body := functionSource(t, "internal/manifest/reservation.go", "Claim") if !strings.Contains(body, "errors.Is(err, fs.ErrNotExist)") { t.Error("manifest.Claim does not tell a missing file from a failure to look at one, " + "so a path it cannot examine is answered in words about a manifest that is already there") diff --git a/internal/manifest/manifest.go b/internal/manifest/manifest.go index f6cb41eb..40265d65 100644 --- a/internal/manifest/manifest.go +++ b/internal/manifest/manifest.go @@ -2,12 +2,9 @@ package manifest import ( "encoding/json" - "errors" "fmt" "io" - "io/fs" "os" - "path/filepath" "runtime" "slices" "sort" @@ -729,168 +726,6 @@ func (m *Manifest) Save(path string) error { return r.Save(m) } -// Reservation is a run's hold on the name its manifest will take. -// -// Taken before the first file, not after the last one. Claiming at save time -// already stopped two runs from both writing a manifest, but it happened at the -// end - so a second run wrote its whole set of files and only then found out it -// had nowhere to record them. Measured on 2026-08-03: two runs started together -// under different ids ended 0 and 5, with sixteen files on the disk and eight -// of them in nobody's manifest (O43). -// -// Until 2026-09-29 the hold was an empty file under the FINAL name, and the save -// renamed over it. A rename replaces whatever it lands on, so a file somebody -// else put there between the claim and the save was destroyed without a word - -// one of the three windows of O252, measured that day with a second writer -// spinning on the name. The hold is now the temporary name the manifest is -// written under, created exclusively when the run starts and written through -// at the save. A second run still cannot take it and is refused before its first -// file, and the final name is never written over. -// -// What lies on the disk while a run goes is therefore ".tfg-writing" -// and no manifest at all. A run killed outright leaves that name behind rather -// than an empty manifest, and that is better as well as safer: the empty -// manifest made the next run say "the only record of what an earlier run -// wrote" about a file that recorded nothing, and sent somebody looking for a -// run whose files it could not name. The leftover name makes the next run say -// that a run is going or was killed, and name the file to remove. -type Reservation struct { - final string - tmp string - // own is what the reservation file is. It is closed as soon as it is - // made - a file held open from the start of a run to its save keeps a - // Windows directory from being removed by anybody who ran the engine and - // never saved, measured on 2026-09-29 when a guard's own clean-up failed - // on it. So the save opens it again and asks the OPENED file whether it - // is still this one (core.OpenOwn), which is what stops a name swapped for - // a link in the hours a large run takes. A clean-up removes it only while - // it is still this one too. - own os.FileInfo - // used says the reservation was saved through or given back. - used bool -} - -// ReservationPath is the name a run holds while it goes, beside the manifest. -func ReservationPath(path string) string { - return core.SiblingPath(path, core.WritingMarker) -} - -// Claim reserves the name a manifest will take, before a run writes anything. -// -// A manifest already under that name is refused here rather than at the save, -// in the words that always named it, so a run does not write its files first. -// A reservation somebody already holds comes back as core.NameTakenError about -// the reservation's own name, which is how the engine tells a run in progress -// from a finished one. -func Claim(path string) (*Reservation, error) { - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return nil, err - } - // "Nothing is there" and "I could not look" are two answers. Reading every - // failure of the look as an empty slot sent a path nobody could examine on - // to words about a manifest that already existed - a sentence about the - // wrong thing, and the one somebody would act on (review 2026-08-23, 3.7c). - switch _, err := os.Lstat(path); { - case err == nil: - return nil, &os.PathError{Op: "save", Path: path, Err: fs.ErrExist} - case !errors.Is(err, fs.ErrNotExist): - return nil, err - } - tmp := ReservationPath(path) - // Created exclusively, and core.CreateNew says why: this name sits in a - // directory the run does not own, and a create that is not exclusive - // follows whatever is at the name. Measured on 2026-09-06 - a link here - // put the manifest on a file outside the output directory and the run - // still exited 0. - f, err := core.CreateNew(tmp, 0o666) - if err != nil { - return nil, err - } - own, err := core.Finish(f, nil) - if err != nil { - _ = core.RemoveOwn(tmp, own) - return nil, err - } - return &Reservation{final: path, tmp: tmp, own: own}, nil -} - -// Save writes the manifest through the reservation and gives it its name. -// -// Whatever fails, the reservation goes with it - but only while its name still -// holds the file this run created. A save can be asked once. -func (r *Reservation) Save(m *Manifest) error { - if r.used { - return fmt.Errorf("the reservation of %s was already used or given back", core.Shown(r.final)) - } - r.used = true - f, err := core.OpenOwn(r.tmp, r.own) - if err != nil { - return err - } - own, err := saveReserved(f, m, r.tmp, r.final) - if own != nil { - r.own = own - } - if err != nil { - _ = core.RemoveOwn(r.tmp, r.own) - return err - } - return nil -} - -// Release gives the name back, for a run that reserved it and then had nothing -// to record. Without it a refused run would leave its reservation behind and the -// next run into that directory would be told a run is going. -func (r *Reservation) Release() error { - if r == nil || r.used { - return nil - } - r.used = true - return core.RemoveOwn(r.tmp, r.own) -} - -// saveReserved fills the reservation, flushes it, closes it and gives it the -// manifest's name, and says what the file is - asked after the write, because a -// clean-up compares the size and the time as well as the file -// (core.RemoveOwn). -// -// Written as one straight sequence on purpose. The guard that asks whether the -// bytes reach the disk before the name does reads the steps of this function -// in order, and a step hidden in the body of an if is a step it cannot see. -// -// The mode is set on the opened file, before anything is in it, because the -// publish moves the file and its mode with it. So this is where a manifest -// carrying a password stops being readable by every account on the machine. -// The ordinary mode is the one the create left, umask and all. -// -// On the device before the publish, because the publish is what turns this -// into the manifest and a rename can reach the disk before the bytes do. What -// survives that is an empty file under the name of the only record able to -// remove a run's files - the loss this whole function is shaped against, -// reached by pulling the plug rather than by killing the process. One call per -// run, so the cost argument that keeps generated files unsynced does not reach -// here. That one is written on engine.Run and it is about ten thousand -// flushes, not one. docs/CODE-REVIEW-2026-08-23.md section 3.4, owner's call on -// 2026-08-25. -func saveReserved(f *os.File, m *Manifest, tmp, final string) (os.FileInfo, error) { - if mode := m.mode(); mode != 0o666 { - if err := f.Chmod(mode); err != nil { - return core.Finish(f, err) - } - } - if err := m.Encode(f); err != nil { - return core.Finish(f, err) - } - if err := f.Sync(); err != nil { - return core.Finish(f, err) - } - own, err := core.Finish(f, nil) - if err != nil { - return own, err - } - return own, core.Publish(tmp, final) -} - // secretProperties are the property names whose value is a credential rather // than a description of a file. // diff --git a/internal/manifest/reservation.go b/internal/manifest/reservation.go new file mode 100644 index 00000000..bd23e88e --- /dev/null +++ b/internal/manifest/reservation.go @@ -0,0 +1,179 @@ +// This file holds a run's reservation of its manifest's name - taken before +// the first file, written through at the end. Its own file since 2026-09-29, +// when the reservation moved from an empty file under the manifest's name to +// the temporary name the manifest is written under (O252), and manifest.go went +// past the size a person can follow. + +package manifest + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + + "github.com/donislawdev/TestingFilesGenerator/internal/core" +) + +// Reservation is a run's hold on the name its manifest will take. +// +// Taken before the first file, not after the last one. Claiming at save time +// already stopped two runs from both writing a manifest, but it happened at the +// end - so a second run wrote its whole set of files and only then found out it +// had nowhere to record them. Measured on 2026-08-03: two runs started together +// under different ids ended 0 and 5, with sixteen files on the disk and eight +// of them in nobody's manifest (O43). +// +// Until 2026-09-29 the hold was an empty file under the FINAL name, and the save +// renamed over it. A rename replaces whatever it lands on, so a file somebody +// else put there between the claim and the save was destroyed without a word - +// one of the three windows of O252, measured that day with a second writer +// spinning on the name. The hold is now the temporary name the manifest is +// written under, created exclusively when the run starts and written through +// at the save. A second run still cannot take it and is refused before its first +// file, and the final name is never written over. +// +// What lies on the disk while a run goes is therefore ".tfg-writing" +// and no manifest at all. A run killed outright leaves that name behind rather +// than an empty manifest, and that is better as well as safer: the empty +// manifest made the next run say "the only record of what an earlier run +// wrote" about a file that recorded nothing, and sent somebody looking for a +// run whose files it could not name. The leftover name makes the next run say +// that a run is going or was killed, and name the file to remove. +type Reservation struct { + final string + tmp string + // own is what the reservation file is. It is closed as soon as it is + // made - a file held open from the start of a run to its save keeps a + // Windows directory from being removed by anybody who ran the engine and + // never saved, measured on 2026-09-29 when a guard's own clean-up failed + // on it. So the save opens it again and asks the OPENED file whether it + // is still this one (core.OpenOwn), which is what stops a name swapped for + // a link in the hours a large run takes. A clean-up removes it only while + // it is still this one too. + own os.FileInfo + // used says the reservation was saved through or given back. + used bool +} + +// ReservationPath is the name a run holds while it goes, beside the manifest. +func ReservationPath(path string) string { + return core.SiblingPath(path, core.WritingMarker) +} + +// Claim reserves the name a manifest will take, before a run writes anything. +// +// A manifest already under that name is refused here rather than at the save, +// in the words that always named it, so a run does not write its files first. +// A reservation somebody already holds comes back as core.NameTakenError about +// the reservation's own name, which is how the engine tells a run in progress +// from a finished one. +func Claim(path string) (*Reservation, error) { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return nil, err + } + // "Nothing is there" and "I could not look" are two answers. Reading every + // failure of the look as an empty slot sent a path nobody could examine on + // to words about a manifest that already existed - a sentence about the + // wrong thing, and the one somebody would act on (review 2026-08-23, 3.7c). + switch _, err := os.Lstat(path); { + case err == nil: + return nil, &os.PathError{Op: "save", Path: path, Err: fs.ErrExist} + case !errors.Is(err, fs.ErrNotExist): + return nil, err + } + tmp := ReservationPath(path) + // Created exclusively, and core.CreateNew says why: this name sits in a + // directory the run does not own, and a create that is not exclusive + // follows whatever is at the name. Measured on 2026-09-06 - a link here + // put the manifest on a file outside the output directory and the run + // still exited 0. + f, err := core.CreateNew(tmp, 0o666) + if err != nil { + return nil, err + } + own, err := core.Finish(f, nil) + if err != nil { + _ = core.RemoveOwn(tmp, own) + return nil, err + } + return &Reservation{final: path, tmp: tmp, own: own}, nil +} + +// Save writes the manifest through the reservation and gives it its name. +// +// Whatever fails, the reservation goes with it - but only while its name still +// holds the file this run created. A save can be asked once. +func (r *Reservation) Save(m *Manifest) error { + if r.used { + return fmt.Errorf("the reservation of %s was already used or given back", core.Shown(r.final)) + } + r.used = true + f, err := core.OpenOwn(r.tmp, r.own) + if err != nil { + return err + } + own, err := saveReserved(f, m, r.tmp, r.final) + if own != nil { + r.own = own + } + if err != nil { + _ = core.RemoveOwn(r.tmp, r.own) + return err + } + return nil +} + +// Release gives the name back, for a run that reserved it and then had nothing +// to record. Without it a refused run would leave its reservation behind and the +// next run into that directory would be told a run is going. +func (r *Reservation) Release() error { + if r == nil || r.used { + return nil + } + r.used = true + return core.RemoveOwn(r.tmp, r.own) +} + +// saveReserved fills the reservation, flushes it, closes it and gives it the +// manifest's name, and says what the file is - asked after the write, because a +// clean-up compares the size and the time as well as the file +// (core.RemoveOwn). +// +// Written as one straight sequence on purpose. The guard that asks whether the +// bytes reach the disk before the name does reads the steps of this function +// in order, and a step hidden in the body of an if is a step it cannot see. +// +// The mode is set on the opened file, before anything is in it, because the +// publish moves the file and its mode with it. So this is where a manifest +// carrying a password stops being readable by every account on the machine. +// The ordinary mode is the one the create left, umask and all. +// +// On the device before the publish, because the publish is what turns this +// into the manifest and a rename can reach the disk before the bytes do. What +// survives that is an empty file under the name of the only record able to +// remove a run's files - the loss this whole function is shaped against, +// reached by pulling the plug rather than by killing the process. One call per +// run, so the cost argument that keeps generated files unsynced does not reach +// here. That one is written on engine.Run and it is about ten thousand +// flushes, not one. docs/CODE-REVIEW-2026-08-23.md section 3.4, owner's call on +// 2026-08-25. +func saveReserved(f *os.File, m *Manifest, tmp, final string) (os.FileInfo, error) { + if mode := m.mode(); mode != 0o666 { + if err := f.Chmod(mode); err != nil { + return core.Finish(f, err) + } + } + if err := m.Encode(f); err != nil { + return core.Finish(f, err) + } + if err := f.Sync(); err != nil { + return core.Finish(f, err) + } + own, err := core.Finish(f, nil) + if err != nil { + return own, err + } + return own, core.Publish(tmp, final) +} From 09829585948adcdc7cc89e3841ec747789cfe855 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 29 Sep 2026 10:24:57 +0200 Subject: [PATCH 4/4] write: the five findings of the review of #150 - verify: the sentence about a ".tfg-writing" leftover promised a manifest, and the instructions are written through the same marker - it now says the manifest or a file written beside it. - preset eject -o: a temporary name left by a stopped eject was reported as the recipe itself being there. It is now named for what it is, with the file to remove (guarded by TestEjectNamesItsOwnLeftoverRatherThanTheRecipe). - core.Publish's last resort renamed after any failed look at the final name. Only a look that found nothing lets the replacing rename through. - A run lock that failed to close after it was made is taken back, or every later run into the directory would be told a run is going. - A manifest reservation that could not be opened at the save is taken back by identity, for the same reason. Co-Authored-By: Claude Opus 5.5 --- internal/audit/audit.go | 9 ++++++--- internal/cli/presetcmd.go | 9 +++++++++ internal/core/publish.go | 8 +++++++- internal/engine/engine.go | 10 +++++++++- internal/guard/ejectfile_test.go | 27 +++++++++++++++++++++++++++ internal/manifest/reservation.go | 4 ++++ 6 files changed, 62 insertions(+), 5 deletions(-) diff --git a/internal/audit/audit.go b/internal/audit/audit.go index 85c46143..72b82ed5 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -142,11 +142,14 @@ func (d Difference) String() string { // Since 2026-09-29 that second one is also how a run reserves its // manifest's name, from before its first file to its save (O252). So // it may belong to a run still going, like the lock above, and the - // sentence holds both endings open for the same reason. + // sentence holds both endings open for the same reason. It names the + // manifest OR a file beside it, because the instructions are written + // through the same marker - a review of #150 caught the first wording + // promising a manifest about a file that would never become one. if core.IsWritingName(filepath.Base(d.Path)) { return fmt.Sprintf( - "leftover %s\n a run's record that is not saved yet. If a run is going on it will "+ - "replace this with its manifest when it ends. If none is, that run was stopped before it could "+ + "leftover %s\n a record of a run that is not saved yet - its manifest or a file written "+ + "beside it. If a run is going on it will give this its final name when it ends. If none is, that run was stopped before it could "+ "tidy up, and the directory may hold files that nothing lists, which cleanup cannot remove - "+ "check what is here against what you expected before deleting this by hand", d.Path) diff --git a/internal/cli/presetcmd.go b/internal/cli/presetcmd.go index 5b2d0c50..df5d51f9 100644 --- a/internal/cli/presetcmd.go +++ b/internal/cli/presetcmd.go @@ -335,6 +335,15 @@ func (f *fileFlag) Set(s string) error { // then - two of the three windows of O252. func writeEjected(path string, source []byte, errOut io.Writer) int { if _, err := core.WriteNew(path, source, 0o644); err != nil { + // The temporary name the recipe is written under first, held by a + // write that was stopped or by another program. Saying the recipe + // itself is there would send somebody looking for a file that is not + // (a review of #150). + var held *core.NameTakenError + if errors.As(err, &held) { + fmt.Fprintf(errOut, "tfg: %s is already there. It is the temporary name the recipe is written under before it becomes %s, left by a write that was stopped or put there by another program. Nothing was written. Remove it and try again.\n", core.Shown(held.Path), core.Shown(path)) + return ExitIO + } if errors.Is(err, fs.ErrExist) { fmt.Fprintf(errOut, "tfg: %s is already there, and -o does not write over a file - it may be a recipe somebody edited. Nothing was written. Choose another name, or remove that file first.\n", core.Shown(path)) return ExitIO diff --git a/internal/core/publish.go b/internal/core/publish.go index 9a456ba9..04bb4955 100644 --- a/internal/core/publish.go +++ b/internal/core/publish.go @@ -79,8 +79,14 @@ func PublishThrough(tmp, final string, noReplace, link func(string, string) erro return &os.LinkError{Op: "publish", Old: tmp, New: final, Err: err} } - if _, lookErr := os.Lstat(final); lookErr == nil { + // "I could not look" is not "nothing is there", and this is the one step + // that replaces what it lands on - so only a look that found nothing lets + // it through (the rule of review 2026-08-23, 3.7c, asked again on #150). + switch _, lookErr := os.Lstat(final); { + case lookErr == nil: return &os.LinkError{Op: "publish", Old: tmp, New: final, Err: fs.ErrExist} + case !errors.Is(lookErr, fs.ErrNotExist): + return &os.LinkError{Op: "publish", Old: tmp, New: final, Err: lookErr} } return os.Rename(tmp, final) } diff --git a/internal/engine/engine.go b/internal/engine/engine.go index d57d1d16..828b8443 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -669,7 +669,15 @@ func claimRunLock(path string) (os.FileInfo, error) { if err != nil { return nil, err } - return core.Finish(fh, nil) + // A lock made and then failing to close is still a lock, and nothing + // would ever give it back - every later run into the directory would be + // told a run is going. Taken back here, by what it is. + own, err := core.Finish(fh, nil) + if err != nil { + _ = core.RemoveOwn(path, own) + return nil, err + } + return own, nil } // releaseRunLock gives the name back, if it still holds our lock. diff --git a/internal/guard/ejectfile_test.go b/internal/guard/ejectfile_test.go index a07858a3..17250346 100644 --- a/internal/guard/ejectfile_test.go +++ b/internal/guard/ejectfile_test.go @@ -100,6 +100,33 @@ func TestEjectThatCannotWriteLeavesNoEmptyFile(t *testing.T) { } } +// A temporary name left by a stopped eject is named for what it is, rather than +// reported as the recipe itself being there. +// +// Since 2026-09-29 the recipe is written under ".tfg-writing" first and +// given its name after (O252). An eject stopped in between leaves that name, +// and the next eject refusing with "my.yaml is already there" sent somebody +// looking for a recipe that does not exist, with nothing naming the file to +// remove. A review of #150 caught it. +func TestEjectNamesItsOwnLeftoverRatherThanTheRecipe(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "my.yaml") + left := core.SiblingPath(path, core.WritingMarker) + if err := os.WriteFile(left, nil, 0o644); err != nil { + t.Fatal(err) + } + code, _, errOut := run(t, "preset", "eject", ejectedPreset, "-o", path) + if code != cli.ExitIO { + t.Errorf("an eject stopped by its own leftover ended %d rather than %d: %s", code, cli.ExitIO, errOut) + } + if !strings.Contains(errOut, filepath.Base(left)) || strings.Contains(errOut, "may be a recipe somebody edited") { + t.Errorf("the refusal does not name the leftover, or calls it the recipe:\n%s", errOut) + } + if _, err := os.Lstat(path); err == nil { + t.Error("a recipe was written although the temporary name was held") + } +} + // An empty name and "-" are refused as usage, and nothing is written - "-" is // not standard output here, leaving -o out is. func TestEjectRefusesAFileNameThatIsNotOne(t *testing.T) { diff --git a/internal/manifest/reservation.go b/internal/manifest/reservation.go index bd23e88e..e548ce0c 100644 --- a/internal/manifest/reservation.go +++ b/internal/manifest/reservation.go @@ -112,6 +112,10 @@ func (r *Reservation) Save(m *Manifest) error { r.used = true f, err := core.OpenOwn(r.tmp, r.own) if err != nil { + // A reservation that could not be opened is still this run's, unless + // the name holds something else now - and RemoveOwn asks exactly that. + // Left behind, it would tell the next run a run is going. + _ = core.RemoveOwn(r.tmp, r.own) return err } own, err := saveReserved(f, m, r.tmp, r.final)