diff --git a/api/generate.go b/api/generate.go index c8b02f95059..8d7a84b8ff6 100644 --- a/api/generate.go +++ b/api/generate.go @@ -2,10 +2,12 @@ package api import ( "fmt" + "go/parser" + "go/token" + "os" "path/filepath" "regexp" "strings" - "syscall" "golang.org/x/tools/imports" @@ -27,6 +29,32 @@ var ( ) // regex to grab the version number from a url ) +// maskGeneratedOutput hides an existing generated output file from the type +// loader for the duration of this generation run (see the comment at the top +// of generate). The mask is a package-clause-only stub — NOT empty bytes, +// which would be a Go parse error and break loading the rest of the package — +// so the package name is read from the real file's own package clause. A +// missing/unparseable file needs no mask (there is nothing stale to bind to; +// generation will (re)create it), matching how the old unlink was a no-op on +// a missing file. +func maskGeneratedOutput(cfg *config.Config, filename string) { + if filename == "" { + return + } + abs, err := filepath.Abs(filename) + if err != nil { + return + } + if _, err := os.Stat(abs); err != nil { + return // nothing on disk — nothing stale to mask + } + f, err := parser.ParseFile(token.NewFileSet(), abs, nil, parser.PackageClauseOnly) + if err != nil || f.Name == nil { + return // can't determine the package — leave it visible rather than corrupt the load + } + cfg.MaskGeneratedFile(abs, "package "+f.Name.Name+"\n") +} + // Generate generates GraphQL code based on the provided config. func Generate(cfg *config.Config, option ...Option) error { return generate(cfg, nil, option...) @@ -55,9 +83,22 @@ func generate( incrementalOpts *codegen.IncrementalOptions, option ...Option, ) error { - _ = syscall.Unlink(cfg.Exec.Filename) + // MASK gqlgen's own previous outputs from the type loader, WITHOUT deleting + // them from disk. If a stale generated model file is visible while the + // schema loads, autobind finds the previously-generated types in it and + // binds them as if they were user-written models — so modelgen skips + // (re)generating them, the freshly-written model file comes out (near-)empty, + // and the exec build then fails with "unable to find type" (every testserver + // config that autobinds its own model package hits this). Before this + // change, api.Generate handled that by syscall.Unlink-ing the outputs up + // front — but a deleted-then-interrupted generation left the user with NO + // generated file at all (#2345, #3505). An overlay gives the loader the + // same "these files don't exist yet" view with no destructive disk write: + // the real files stay intact until the atomic rename replaces them, and + // templates.write unmasks each file once its new contents are on disk. + maskGeneratedOutput(cfg, cfg.Exec.Filename) if cfg.Model.IsDefined() { - _ = syscall.Unlink(cfg.Model.Filename) + maskGeneratedOutput(cfg, cfg.Model.Filename) } plugins := []plugin.Plugin{} diff --git a/api/generate_test.go b/api/generate_test.go index 9ab18966f3c..378315b3b36 100644 --- a/api/generate_test.go +++ b/api/generate_test.go @@ -413,3 +413,50 @@ func TestBuildPattern(t *testing.T) { }) } } + +// TestGenerateAtomicWritePreservesOutputOnFailure reproduces issue #2345/#3505: when generation +// FAILS mid-run (after the output file would have been deleted, before it's rewritten), the +// PRE-EXISTING generated file must SURVIVE intact — not be left absent. Before the atomic-write +// fix, api/generate.go unlinked the exec + model outputs at the very start of Generate, so a +// failure anywhere in the long schema-load/plugin/render chain left the file ABSENT (invisible +// to `go build` until the next regen, and the root of the recovery-desyncs-sibling-golden class). +// Now the write is atomic (write-to-temp + os.Rename), and the upfront unlink is gone, so a +// mid-generation failure leaves the prior file untouched. +func TestGenerateAtomicWritePreservesOutputOnFailure(t *testing.T) { + wd, err := os.Getwd() + require.NoError(t, err) + + workDir := filepath.Join(wd, "testdata", "default") + t.Cleanup(func() { + cleanup(workDir) + t.Chdir(wd) + }) + t.Chdir(workDir) + + cfg, err := config.LoadConfigFromDefaultLocations() + require.NoError(t, err) + + // Pre-create the exec output with known content — the "previously generated" file that must + // survive a failed regen. + execPath := cfg.Exec.Filename + preExisting := []byte( + "// the previously-generated file — must survive a failed regen\npackage graph\n", + ) + require.NoError(t, os.MkdirAll(filepath.Dir(execPath), 0o755)) + require.NoError(t, os.WriteFile(execPath, preExisting, 0o644)) + + // Run Generate with an erroring schema mutator: it fails AFTER schema load but BEFORE the + // exec file is rewritten — the exact window where the old upfront-unlink left the file absent. + err = Generate(cfg, AddPlugin(&testSchemaMutator{name: "error-mutator", shouldError: true})) + require.Error(t, err) + require.Contains(t, err.Error(), "deliberate schema mutation error") + + // The pre-existing file must be INTACT — not absent, not truncated, not empty. + got, readErr := os.ReadFile(execPath) + require.NoError( + t, + readErr, + "exec output must be PRESENT after a failed regen (was deleted under the old unlink-first behavior)", + ) + require.Equal(t, preExisting, got, "exec output must be UNCHANGED after a failed regen") +} diff --git a/codegen/config/config.go b/codegen/config/config.go index 30717bc41f8..936553aebe5 100644 --- a/codegen/config/config.go +++ b/codegen/config/config.go @@ -78,6 +78,13 @@ type Config struct { Sources []*ast.Source `yaml:"-"` Packages *code.Packages `yaml:"-"` Schema *ast.Schema `yaml:"-"` + + // packagesOverlay is the loader overlay shared by every Packages instance + // this Config creates (LoadSchema recreates c.Packages, so the overlay must + // outlive any single instance). Entries mask gqlgen's own stale generated + // outputs from autobind during a generation run — see MaskGeneratedFile and + // api.Generate. Keyed by absolute file path. + packagesOverlay map[string][]byte } // boolOrFalse returns the value of a *bool pointer, or false if nil. @@ -337,12 +344,29 @@ func CompleteConfig(config *Config) error { return nil } +// MaskGeneratedFile registers a loader overlay masking absPath with the given +// stub contents for every Packages instance this Config creates — including +// the one LoadSchema recreates — so gqlgen's own stale outputs can be hidden +// from autobind for the whole generation run (see api.Generate). Held on the +// Config (not just the current Packages) because LoadSchema rebuilds +// c.Packages, which would otherwise silently drop masks set before it. +func (c *Config) MaskGeneratedFile(absPath, contents string) { + if c.packagesOverlay == nil { + c.packagesOverlay = map[string][]byte{} + } + c.packagesOverlay[absPath] = []byte(contents) + if c.Packages != nil { + c.Packages.MaskFile(absPath, contents) + } +} + func (c *Config) Init() error { if c.Packages == nil { c.Packages = code.NewPackages( code.WithBuildTags(c.GoBuildTags...), code.PackagePrefixToCache("github.com/99designs/gqlgen/graphql"), code.WithPreloadNames(templatePackageNames...), + code.WithOverlay(c.packagesOverlay), ) } @@ -1168,6 +1192,7 @@ func (c *Config) LoadSchema() error { code.WithBuildTags(c.GoBuildTags...), code.PackagePrefixToCache("github.com/99designs/gqlgen/graphql"), code.WithPreloadNames(templatePackageNames...), + code.WithOverlay(c.packagesOverlay), ) } diff --git a/codegen/templates/templates.go b/codegen/templates/templates.go index 027118da35b..27504a62d10 100644 --- a/codegen/templates/templates.go +++ b/codegen/templates/templates.go @@ -18,6 +18,7 @@ import ( "strings" "sync" "text/template" + "time" "unicode" "github.com/99designs/gqlgen/internal/code" @@ -736,10 +737,119 @@ func write(filename string, b []byte, packages *code.Packages, opts imports.Prun // Skip write if content is unchanged - preserves mtime for Go build cache existing, readErr := os.ReadFile(filename) if readErr == nil && bytes.Equal(existing, formatted) { + // The on-disk file IS the current output — later loads must see it. + unmask(packages, filename) return nil } - return os.WriteFile(filename, formatted, 0o644) + // Write atomically: render to a temp file in the SAME directory as the + // destination, fsync it, then rename it into place on success. Rename is + // atomic on the same filesystem, so a reader (or a build) never observes a + // half-written file, and an interruption/panic/OOM between formatting and the + // rename leaves the PRE-EXISTING file intact instead of an empty or absent + // one. This also removes the need for the upfront syscall.Unlink of the output + // in api/generate.go (the unlink deleted the file before generation began, so + // any interruption during generation left it absent — see issue #2345/#3505). + // + // This is the standard write-temp-then-rename pattern used by + // google/renameio, natefinch/atomic, tailscale/atomicfile and moby/sys (see + // https://github.com/99designs/gqlgen/pull/4262#issuecomment-5011193760 for + // the tailscale/atomicfile reference this follows most closely): + // - the temp lives next to the destination so the rename is same-filesystem; + // - the temp is fsync'd before the rename so a crash can't expose a + // zero-length file even though the rename itself succeeded (without fsync + // the directory entry can be durable before the data is, per ext4's Ts'o); + // - on non-Windows, the temp's permissions are set to match the existing + // destination file (preserving any user/repo mode and avoiding spurious + // mode churn across regens), defaulting to 0o644 for a brand-new file — + // the same mode os.WriteFile used here before this change. On Windows the + // permission bits are SKIPPED entirely (not attempted, not defaulted) — + // matching tailscale/atomicfile's own "perm argument is ignored on + // Windows" contract: Windows has no POSIX owner/group/other mode, only a + // coarse read-only attribute, so os.FileMode there doesn't mean the same + // thing and forcing a POSIX bit pattern onto it is a category error, not + // a portability nicety. + dir := filepath.Dir(filename) + tmp, err := os.CreateTemp(dir, filepath.Base(filename)+".*.tmp") + if err != nil { + return fmt.Errorf("failed to create temp file: %w", err) + } + tmpName := tmp.Name() + cleanup := func() { _ = os.Remove(tmpName) } + if _, err := tmp.Write(formatted); err != nil { + _ = tmp.Close() + cleanup() + return fmt.Errorf("failed to write temp file: %w", err) + } + if runtime.GOOS != "windows" { + perm := os.FileMode(0o644) + if fi, err := os.Stat(filename); err == nil && fi.Mode().IsRegular() { + perm = fi.Mode().Perm() + } + // Set permissions before fsync so the mode change is flushed with the + // data. Chmod sets the exact bits (umask is not applied), like the + // references. + if err := tmp.Chmod(perm); err != nil { + _ = tmp.Close() + cleanup() + return fmt.Errorf("failed to set temp file permissions: %w", err) + } + } + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + cleanup() + return fmt.Errorf("failed to sync temp file: %w", err) + } + if err := tmp.Close(); err != nil { + cleanup() + return fmt.Errorf("failed to close temp file: %w", err) + } + if err := renameWithRetry(tmpName, filename); err != nil { + cleanup() + return fmt.Errorf("failed to rename temp file: %w", err) + } + // The new contents are on disk — remove any loader mask api.Generate placed + // over this output (see maskGeneratedOutput), so later loads in this same + // run (e.g. the exec build reloading the model package after modelgen wrote + // it) see the just-generated types instead of the empty stub. + unmask(packages, filename) + return nil +} + +// unmask lifts api.Generate's loader mask (maskGeneratedOutput) for a just- +// written (or confirmed-current) output file — from here on, disk is truth. +func unmask(packages *code.Packages, filename string) { + if abs, err := filepath.Abs(filename); err == nil { + packages.UnmaskFile(abs) + } +} + +// renameWithRetry wraps os.Rename with a few short retries on Windows. Go's +// os.Rename on Windows already calls MoveFileEx with MOVEFILE_REPLACE_EXISTING +// — the same underlying API natefinch/atomic's ReplaceFile wraps — so it is +// not a weaker primitive; what it doesn't do is retry. MoveFileEx can fail +// TRANSIENTLY with "Access is denied" when something else (a virus scanner, a +// search indexer) briefly holds an open handle on the destination right after +// it was read (the unchanged-content check above just opened it) or on the +// freshly-written temp file. This is a well-known Windows quirk; a handful of +// short, bounded retries resolves it without materially slowing down the +// common (non-Windows, non-contended) case, where the first attempt always +// succeeds. +func renameWithRetry(oldpath, newpath string) error { + if runtime.GOOS != "windows" { + return os.Rename(oldpath, newpath) + } + var err error + for i := range 5 { + if err = os.Rename(oldpath, newpath); err == nil { + return nil + } + if !errors.Is(err, os.ErrPermission) { + return err + } + time.Sleep(time.Duration(i+1) * 10 * time.Millisecond) + } + return err } var pkgReplacer = strings.NewReplacer( diff --git a/codegen/templates/templates_test.go b/codegen/templates/templates_test.go index 4ba4577d96a..16fc4fc8aa9 100644 --- a/codegen/templates/templates_test.go +++ b/codegen/templates/templates_test.go @@ -5,12 +5,15 @@ import ( "fmt" "os" "path/filepath" + "runtime" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/99designs/gqlgen/internal/code" + "github.com/99designs/gqlgen/internal/imports" ) //go:embed *.gotpl @@ -401,8 +404,16 @@ func TestTemplateOverride(t *testing.T) { if err != nil { t.Fatal(err) } - defer f.Close() - err = Render(Options{Template: "hello", Filename: f.Name(), Packages: code.NewPackages()}) + name := f.Name() + // Close BEFORE Render: write()'s atomic rename replaces this file, and on + // Windows a rename/replace over a file with an open handle fails with + // "Access is denied" (mandatory file locking) even from the SAME process + // — unlike POSIX, where os.Rename over an open file is fine. Only the + // name is needed from here on. + if err := f.Close(); err != nil { + t.Fatal(err) + } + err = Render(Options{Template: "hello", Filename: name, Packages: code.NewPackages()}) if err != nil { t.Fatal(err) } @@ -419,14 +430,21 @@ func TestRenderFS(t *testing.T) { if err != nil { t.Fatal(err) } - defer f.Close() - err = Render(Options{TemplateFS: templateFS, Filename: f.Name(), Packages: code.NewPackages()}) + name := f.Name() + // Close BEFORE Render — see the comment in TestTemplateOverride: write()'s + // atomic rename can't replace a file this process still has open on + // Windows. Only the name is needed from here on (the content is read back + // fresh via os.ReadFile below, not through this handle). + if err := f.Close(); err != nil { + t.Fatal(err) + } + err = Render(Options{TemplateFS: templateFS, Filename: name, Packages: code.NewPackages()}) if err != nil { t.Fatal(err) } expectedString := "package \n\nimport (\n)\nthis is my test package" - actualContents, _ := os.ReadFile(f.Name()) + actualContents, _ := os.ReadFile(name) actualContentsStr := string(actualContents) // don't look at last character since it's \n on Linux and \r\n on Windows @@ -478,3 +496,106 @@ func TestDict(t *testing.T) { }) } } + +// writeContent is a small helper that renders a fixed, gofmt-clean Go file +// through write() so the atomic-write tests exercise the real code path +// (formatting, unchanged-content short-circuit, temp-then-rename). +func writeContent(t *testing.T, filename, content string) { + t.Helper() + packages := code.NewPackages() + require.NoError(t, write(filename, []byte(content), packages, imports.PruneOptions{})) +} + +// TestWriteIsAtomicPreservesPermissionsAndLeavesNoTemp covers the three +// properties the atomic write must hold, mirroring the reference +// implementations (google/renameio, natefinch/atomic, tailscale/atomicfile, +// moby/sys) that this change was compared against in #4262: +// +// 1. Existing destination mode is preserved across a regen (not silently +// dropped to the 0o600 that os.CreateTemp gives the temp file). +// 2. A brand-new file is created with 0o644 — the same mode os.WriteFile +// used here before this change — not 0o600. +// 3. No temp file is left behind in the directory after a successful write. +// +// Exact POSIX permission bits (cases 1 & 2) are meaningless on Windows, which +// has no POSIX mode — os.FileMode there is a coarse read-only/not emulation, +// not real owner/group/other bits (see renameio's own writefile.go, which is +// `//go:build !windows` for the same reason). Those two assertions are +// skipped on windows; case 3 (no leftover temp file) is platform-agnostic and +// always runs. +func TestWriteIsAtomicPreservesPermissionsAndLeavesNoTemp(t *testing.T) { + dir := t.TempDir() + + goFile := "package graph\n\n// Code generated by github.com/99designs/gqlgen, DO NOT EDIT.\n\nfunc a() {}\n" + + if runtime.GOOS != "windows" { + // Case 1: pre-existing file with a non-default mode must keep that mode. + existing := filepath.Join(dir, "existing.go") + require.NoError(t, os.WriteFile(existing, []byte("package graph\n\nfunc a() {}\n"), 0o600)) + before, err := os.Stat(existing) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), before.Mode().Perm()) + + writeContent(t, existing, goFile) + + after, err := os.Stat(existing) + require.NoError(t, err) + require.Equal(t, before.Mode().Perm(), after.Mode().Perm(), + "existing file mode must be preserved across a regen, not dropped to 0o600 or 0o644") + + // Case 2: brand-new file defaults to 0o644 (matches the old os.WriteFile mode). + fresh := filepath.Join(dir, "fresh.go") + writeContent(t, fresh, goFile) + freshInfo, err := os.Stat(fresh) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o644), freshInfo.Mode().Perm(), + "new file must be 0o644 (the pre-change os.WriteFile mode), not 0o600") + } else { + // Still exercise the write path on windows (case 3 below checks its + // leftover-temp-file property); just don't assert POSIX bits. + writeContent(t, filepath.Join(dir, "existing.go"), goFile) + writeContent(t, filepath.Join(dir, "fresh.go"), goFile) + } + + // Case 3: no temp file left behind. + entries, err := os.ReadDir(dir) + require.NoError(t, err) + for _, e := range entries { + require.False(t, strings.HasSuffix(e.Name(), ".tmp"), + "leftover temp file after successful write: %q", e.Name()) + } +} + +// TestWriteIsAtomicAndUnchangedShortCircuit confirms the unchanged-content +// short-circuit (which preserves mtime for the Go build cache) still fires and +// still writes atomically when content actually changes. +func TestWriteIsAtomicAndUnchangedShortCircuit(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "gen.go") + content := "package graph\n\nfunc a() {}\n" + + writeContent(t, target, content) + + first, err := os.Stat(target) + require.NoError(t, err) + + // Identical content → short-circuit: mtime should not move forward. + writeContent(t, target, content) + second, err := os.Stat(target) + require.NoError(t, err) + require.Equal(t, first.ModTime(), second.ModTime(), + "unchanged-content short-circuit must preserve mtime for the Go build cache") + + // Different content → rewritten atomically, content updates, mode unchanged. + writeContent(t, target, "package graph\n\nfunc b() {}\n") + got, err := os.ReadFile(target) + require.NoError(t, err) + require.Contains(t, string(got), "func b()") + // Exact POSIX mode is meaningless on windows (no real owner/group/other + // bits) — see the note on TestWriteIsAtomicPreservesPermissionsAndLeavesNoTemp. + if runtime.GOOS != "windows" { + mode, err := os.Stat(target) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o644), mode.Mode().Perm()) + } +} diff --git a/internal/code/packages.go b/internal/code/packages.go index 662ee364dde..7539f2a9add 100644 --- a/internal/code/packages.go +++ b/internal/code/packages.go @@ -29,6 +29,7 @@ type ( loadErrors []error buildFlags []string packagesToCachePrefix string + overlay map[string][]byte numLoadCalls int // stupid test steam. ignore. numNameCalls int // stupid test steam. ignore. @@ -58,6 +59,18 @@ func PackagePrefixToCache(prefixPath string) func(p *Packages) { } } +// WithOverlay option for NewPackages supplies the packages.Config.Overlay map +// used by every Load: files present in the map are read from it INSTEAD of +// the file system. The map is held by REFERENCE (not copied) so a caller — +// config.Config, which recreates its Packages instance during LoadSchema — +// can keep one overlay alive across recreations, and MaskFile/UnmaskFile +// mutations remain visible to whichever Packages instance currently holds it. +func WithOverlay(overlay map[string][]byte) func(p *Packages) { + return func(p *Packages) { + p.overlay = overlay + } +} + // NewPackages creates a new packages cache // It will load all packages in the current module, and any packages that are passed to Load or // LoadAll @@ -69,6 +82,35 @@ func NewPackages(opts ...Option) *Packages { return p } +// MaskFile makes every subsequent Load treat the file at absPath as if it +// contained only the given contents (a package-clause-only stub), WITHOUT +// touching the file on disk — a packages.Config.Overlay entry. gqlgen uses +// this to hide its OWN previously-generated outputs (the model file) from the +// type loader during generation: if a stale models_gen.go is visible while +// the schema loads, autobind finds the previously-generated types in it and +// binds them as if they were user-written models, so modelgen skips +// (re)generating them and the freshly-written model file comes out empty — +// the types vanish and the exec build fails with "unable to find type". +// Historically api.Generate prevented that by DELETING the outputs up front +// (syscall.Unlink), but that is exactly what left users with a missing +// generated.go when generation was interrupted (#2345, #3505): masking at the +// loader gives the same load semantics with no destructive disk write. +func (p *Packages) MaskFile(absPath, contents string) { + if p.overlay == nil { + p.overlay = map[string][]byte{} + } + p.overlay[absPath] = []byte(contents) +} + +// UnmaskFile removes a MaskFile entry so subsequent Loads read the real file +// from disk again — called right after gqlgen atomically writes that file +// (see codegen/templates.write): once the new contents are on disk, disk is +// the truth and later reloads (e.g. the exec build after modelgen runs) must +// see the just-generated types, not the mask. +func (p *Packages) UnmaskFile(absPath string) { + delete(p.overlay, absPath) +} + func dedupPackages(packages []string) []string { packageMap := make(map[string]struct{}) dedupedPackages := make([]string, 0, len(packageMap)) @@ -132,6 +174,7 @@ func (p *Packages) LoadAll(importPaths ...string) []*packages.Package { pkgs, err := packages.Load(&packages.Config{ Mode: mode, BuildFlags: p.buildFlags, + Overlay: p.overlay, }, missing...) if err != nil { p.loadErrors = append(p.loadErrors, err) @@ -208,6 +251,7 @@ func (p *Packages) LoadWithTypes(importPath string) *packages.Package { pkgs, err := packages.Load(&packages.Config{ Mode: mode, BuildFlags: p.buildFlags, + Overlay: p.overlay, }, importPath) if err != nil { p.loadErrors = append(p.loadErrors, err)