cmd/dist, cmd/go: Go tip leaving pkg/mod/golang.org/x/telemetry read-only files behind

[mvdan]

In the past week or so, since @matloob started working on telemetry support in cmd/go, I've started having issues with my source builds from master where read-only module cache files are left behind inside GOROOT:

$ git clean -dffx
Removing VERSION.cache
Removing bin/
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/config.json: Permission denied
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/doc.go: Permission denied
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/go.mod: Permission denied
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/LICENSE: Permission denied
Removing pkg/obj/gopath/pkg/mod/cache
Removing pkg/obj/gopath/pkg/sumdb
Removing pkg/tool
Removing pkg/include
Removing src/cmd/cgo/zdefaultcc.go
Removing src/cmd/go/internal/cfg/zdefaultcc.go
Removing src/cmd/internal/objabi/zbootstrap.go
Removing src/go/build/zcgo.go
Removing src/internal/buildcfg/zbootstrap.go
Removing src/runtime/internal/sys/zversion.go
Removing src/time/tzdata/zzipdata.go
$ git clean -dffx
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/config.json: Permission denied
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/doc.go: Permission denied
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/go.mod: Permission denied
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/LICENSE: Permission denied

That was a build from d05af62695. I just did a clean build of 2f6426834c150c37cdb1330b48e9903963d4329c and the read-only files weren't created right away, but I'm not sure if they will be created at some point as I use cmd/go.

[dmitshur]

Thanks for reporting. Some notes from when I briefly looked in case they're helpful.

cmd/dist sets GOPATH env var (which in turn sets the module cache location) to $GOROOT/pkg/obj/gopath during the bootstrap with the following comment:

// Set GOPATH to an internal directory. We shouldn't actually
// need to store files here, since the toolchain won't
// depend on modules outside of vendor directories, but if
// GOPATH points somewhere else (e.g., to GOROOT), the
// go tool may complain.
os.Setenv("GOPATH", pathf("%s/pkg/obj/gopath", goroot))

(source)

The golang.org/x/telemetry module is vendored, but the golang.org/x/telemetry/config nested module isn't:

https://cs.opensource.google/go/go/+/master:src/cmd/vendor/modules.txt;l=49-59;drc=d367b2a475e79cdd1f39ccf376098d0566b7dffa

It's probably intended for config not to be vendored, but it doesn't seem that placing it in the $GOROOT/pkg/obj/gopath/pkg module cache is quite ideal either. Maybe there's no better location for it? What would happen when the GOROOT directory is read-only?

[mvdan]

I've been running 2f6426834c150c37cdb1330b48e9903963d4329c for six hours today as I work and I haven't seen these files, or any other read-only files, being created yet. I'm not sure if this is a bug or issue that already got fixed in the last couple of days.

[dmitshur]

This doesn't reproduce each time for me, but I saw it happen again today after running these commands in my development GOROOT directory today:

src $ git-branches 
| Branch                | Base | Behind | Ahead |
|-----------------------|------|-------:|:------|
| **main**              | main |      0 | 0     |
| release-branch.go1.22 | main |   1139 | 70    |
| release-branch.go1.21 | main |   2827 | 161   |

| Branch                | Remote                       | Behind | Ahead |
|-----------------------|------------------------------|-------:|:------|
| **main**              | origin/master                |      4 | 1     |
| release-branch.go1.22 | origin/release-branch.go1.22 |      4 | 0     |
| release-branch.go1.21 | origin/release-branch.go1.21 |      4 | 0     |

(68 stale/trashed branches not shown.)
src $ git rebase origin/master 
Successfully rebased and updated refs/heads/main.
src $ cd ..
gotip $ git clean -dfx
Removing VERSION.cache
Removing bin/
Removing pkg/
Removing src/cmd/cgo/zdefaultcc.go
Removing src/cmd/go/internal/cfg/zdefaultcc.go
Removing src/cmd/internal/objabi/zbootstrap.go
Removing src/go/build/zcgo.go
Removing src/internal/buildcfg/zbootstrap.go
Removing src/runtime/internal/sys/zversion.go
Removing src/time/tzdata/zzipdata.go
gotip $ cd src
src $ ./make.bash
Building Go cmd/dist using /usr/local/go. (go1.22.3 darwin/arm64)
Building Go toolchain1 using /usr/local/go.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for darwin/arm64.
---
Installed Go for darwin/arm64 in /Users/dmitshur/gotip
Installed commands in /Users/dmitshur/gotip/bin
src $ ls ../pkg
include obj tool
src $ tree ../pkg/obj/gopath/pkg/mod/golang.org 
../pkg/obj/gopath/pkg/mod/golang.org
└── x
    └── telemetry
        └── config@v0.21.0
            ├── LICENSE
            ├── config.json
            ├── doc.go
            └── go.mod

4 directories, 4 files
src $ cd ..
gotip $ git clean -dfx
Removing VERSION.cache
Removing bin/
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/go.mod: Permission denied
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/LICENSE: Permission denied
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/config.json: Permission denied
warning: failed to remove pkg/obj/gopath/pkg/mod/golang.org/x/telemetry/config@v0.21.0/doc.go: Permission denied
Removing pkg/obj/gopath/pkg/mod/cache
Removing pkg/obj/gopath/pkg/sumdb
Removing pkg/include
Removing pkg/tool
Removing src/cmd/cgo/zdefaultcc.go
Removing src/cmd/go/internal/cfg/zdefaultcc.go
Removing src/cmd/internal/objabi/zbootstrap.go
Removing src/go/build/zcgo.go
Removing src/internal/buildcfg/zbootstrap.go
Removing src/runtime/internal/sys/zversion.go
Removing src/time/tzdata/zzipdata.go
gotip $ echo $?
1
gotip $ 

(I don't always do git clean -dfx to get a clean slate, only occasionally. I've been doing it more often recently to get more data points for this issue.)

It might be relevant that I have opted in to telemetry uploading (i.e., my gotelemetry env output includes mode: on), and I had that GOROOT directory opened in an editor which has an LSP plugin that uses gopls.

CC @golang/telemetry for awareness.

[findleyr]

Thanks for reporting.

The telemetry upload, which may be initiated from a subprocess of the go command, runs go mod download -json golang.org/x/telemetry/config@latest to get the latest upload configuration. I wonder if the go command subprocess somehow inherits GOMODCACHE=$GOROOT/pkg/mod due to some sort of intermediate state in the go command itself which isn't supposed to download module content. @matloob, does anything like that sound possible?

[findleyr]

Marking as a release blocker since I don't think we should cut the rc until we understand this.

[matloob]

We get the config as part of the upload process. The telemetry upload code downloads the latest config to the modcache using go mod download and then examines the config to see what it's allowed to upload.

@mvdan Talking to @rfindley, I understand that the reason you're not consistently observing this is that we only do the upload once a week, so we won't do the download of the config again until the next telemetry upload until a week has passed since the last telemetry upload. And if we understand what's going on correctly, you'd have to run make.bash when you're due for an upload or we'd just download the modules to the normal mod cache location.

@rfindley Yes that definitely sounds possible. I think we should try to skip the upload if GOMODCACHE is a subdirectory of GOROOT. It seems to me that this could happen whenever we download a module if GOMODCACHE is a subdirectory of GOROOT but we usually don't need to download anything to GOMODCACHE when running cmd/dist since everything is vendored.

@dmitshur I think we definitely don't want to use a GOMODCACHE that's in GOROOT to download the config.

[findleyr]

To close the loop here, @hyangah suggested that we use a temp GOMODCACHE for config download, and I think that makes sense. We already throttle the upload operation to once a day, and the config module is by design tiny, so the additional bandwidth of re-downloading the config should really not be a concern. (and we can do even better by only downloading the config when there is actually work to perform).

If we have consensus among @golang/telemetry, I can do this on Monday.

[rsc]

Using a temp GOMODCACHE will not solve the problem, because downloads will still write other files to the temporary GOPATH ($GOROOT/pkg/obj/gopath) such as the checksum database files. And we can't set a temporary GOPATH because then the checksum database lookups won't catch forking attacks.

Probably cmd/dist should set GOPROXY=off to disable any downloads while its custom GOPATH is set.

I don't believe the telemetry code should have special cases for GOPATH inside GOROOT or anything like that to try to detect running under cmd/dist. Instead, cmd/dist should just set things up the right way.

[findleyr]

Aha, @rsc makes some good points. Setting GOPROXY=off is indeed a simple solution -- the upload operation will be aborted.

@matloob assigning to you and I; if you can update cmd/dist, that would be great, and I can write some additional tests in x/telemetry.

[mvdan]

I admit I'm not sure I follow the details of the discussion :) The read-only files are still not present as of today. The earlier suggestion that this might only trigger when an infrequent telemetry upload happens makes sense. I'm happy with any solution that prevents read-only files, or ideally any cache files, from being created inside GOROOT.

[findleyr]

@mvdan to summarize:

• The readonly files would only be created when the upload runs as part of cmd/dist. That's pretty unlikely because the upload only runs once a day, from whichever process acquires the token, and would also be triggered by gopls or any other go command. In fact, we're very fortunate that you triggered this from cmd/dist, noticed it, and took the time to report it.
• The solution of failing the upload when it runs as part of cmd/dist is perfectly acceptable, provided setting GOPROXY=off doesn't have unintended side effects within cmd/dist. The upload will fail, but will then succeed the next day if initiated from anywhere else. We keep trying once a day for up to 4 weeks, so it seems extremely unlikely that this leads to significant data loss.

[mvdan]

Ah gotcha, that makes sense. It's perhaps not surprising that I ran into this within a week of telemetry being added to cmd/go in master, because pretty much the first thing I do when I start my work day is to pull master and re-build :) In most cases I wouldn't have run any other Go command since the afternoon or evening prior.

[matloob]

I'll send a CL to cmd/dist to turn GOPROXY=off in cmd/dist while GOPATH is set to be in GOROOT.

[gopherbot]

Change https://go.dev/cl/586078 mentions this issue: cmd/dist: set GOPROXY=off when GOPATH is set to be in GOROOT