diff --git a/docs/browser-tab.md b/docs/browser-tab.md index 3bce68436..a72f53d19 100644 --- a/docs/browser-tab.md +++ b/docs/browser-tab.md @@ -65,7 +65,10 @@ A file the page downloads lands in the user's Downloads folder (`XDG_DOWNLOAD_DIR`, from the environment or `user-dirs.dirs`, on Linux; `~/Downloads` elsewhere) under the name the page suggests, made unique the way browsers do (`report (1).csv`): nothing already there is overwritten. Chrome -writes it under its download id and lich renames it once it is complete. A +writes it under its download id into a staging folder inside the session's own +profile, and lich moves it to Downloads once it is complete; across +filesystems the move is a copy. What Chrome downloads on its own, with no page +announcing it, stays in staging and is deleted with the profile. A click or key press that starts one answers with a `downloaded:` line, its path and size, after waiting up to ten seconds for it; one still running answers `downloading:`, and the next page read says when it ends. `browser_open` and diff --git a/docs/ceilings.md b/docs/ceilings.md index 2796af30b..7849647ce 100644 --- a/docs/ceilings.md +++ b/docs/ceilings.md @@ -993,11 +993,13 @@ work when nobody knows it and that the call site never shows. The mechanism and - **A session browser's downloads go straight to the user's Downloads folder** (`internal/browser/downloads.go`, `session_downloads.go`, `docs/browser-tab.md`): `XDG_DOWNLOAD_DIR` on Linux, `~/Downloads` on macOS and Windows, - even for a Windows user who moved that known folder. While one runs, a file named by Chrome's download id sits - there; it takes its real name once complete. The download behaviour is the whole browser's, so what Chrome - downloads on its own, with no page, lands there too under its id and is never renamed: the optimization guide's - model downloads are switched off (`disabledFeatures`, `chrome.go`), and a background download a later Chrome adds - would land. Chrome reloads the page 1.46 to 1.70 seconds after an `` begins, and the reload aborts a + even for a Windows user who moved that known folder. Chrome writes each one under its download id into a staging + folder in the session's profile, and only a download a page announced moves to Downloads, once complete. The + download behaviour is the whole browser's, so what Chrome downloads on its own, with no page (4 KB to 33 MB files + on the macOS and Windows runners), lands in staging, is never moved and goes with the profile when the browser + closes; until then it takes the profile's disk. The profile sits in the temp dir, so with Downloads on another + filesystem or volume the move is a copy, written under a hidden `.part` name and renamed once whole. A move that + fails reports the staged path, which goes with the profile too. Chrome reloads the page 1.46 to 1.70 seconds after an `` begins, and the reload aborts a navigation started before it (`net::ERR_ABORTED`; Chromium 155, with downloads on or off), so an agent's click, key press, open or navigate that downloaded holds its answer 2.5 seconds; a navigation the person starts in the pane is not held. A download still running after ten seconds is reported in progress and named again when it diff --git a/internal/browser/chrome.go b/internal/browser/chrome.go index ee76d1255..6ee91a4c4 100644 --- a/internal/browser/chrome.go +++ b/internal/browser/chrome.go @@ -8,6 +8,7 @@ import ( "log/slog" "os" "os/exec" + "path/filepath" "time" "github.com/chromedp/cdproto/browser" @@ -61,7 +62,7 @@ func chromeLaunch(dir string, headed bool) (Page, error) { return nil, err } p := &chromePage{ctx: ctx, cancel: cancel, allocCancel: allocCancel} - if err := p.allowDownloads(); err != nil { + if err := p.allowDownloads(dir); err != nil { slog.Warn("browser: downloads stay off", "err", err) } return p, nil @@ -70,9 +71,10 @@ func chromeLaunch(dir string, headed bool) (Page, error) { // disabledFeatures replaces chromedp's own disable-features list, which it // repeats, adding the optimization guide's model downloads. Those go through // the profile's download manager with no page and no download event, so with -// downloads allowed Chrome saved them into the user's Downloads as files -// named by a GUID, 4 KB to 33 MB each (Chrome on the Windows and macOS CI -// runners, 2026-10-27). +// downloads allowed Chrome saved them into the download folder as files named +// by a GUID, 4 KB to 33 MB each (Chrome on the Windows and macOS CI runners, +// 2026-10-27). They stay in the profile's staging folder now; these spare +// the bytes. const disabledFeatures = "site-per-process,Translate,BlinkGenPropertyTrees," + "OptimizationGuideModelDownloading,OptimizationTargetPrediction" @@ -90,9 +92,10 @@ type chromePage struct { var downloadFolder = downloadDir // allowDownloads saves what the page downloads into the user's Downloads -// folder, with events. A headless Chrome denies downloads by default, and a -// click on a download button did nothing. -func (p *chromePage) allowDownloads() error { +// folder, with events, by way of a staging folder in the profile dir. A +// headless Chrome denies downloads by default, and a click on a download +// button did nothing. +func (p *chromePage) allowDownloads(profile string) error { dir, err := downloadFolder() if err != nil { return err @@ -100,17 +103,21 @@ func (p *chromePage) allowDownloads() error { if err := os.MkdirAll(dir, 0o755); err != nil { return err } + staging := filepath.Join(profile, stagingDir) + if err := os.MkdirAll(staging, 0o700); err != nil { + return err + } c := chromedp.FromContext(p.ctx) err = p.run(nil, chromedp.ActionFunc(func(ctx context.Context) error { return browser.SetDownloadBehavior(browser.SetDownloadBehaviorBehaviorAllowAndName). - WithDownloadPath(dir). + WithDownloadPath(staging). WithEventsEnabled(true). Do(cdp.WithExecutor(ctx, c.Browser)) })) if err != nil { return err } - p.dl = newDownloads(dir) + p.dl = newDownloads(staging, dir) return nil } diff --git a/internal/browser/downloads.go b/internal/browser/downloads.go index d2953c364..4db86e50c 100644 --- a/internal/browser/downloads.go +++ b/internal/browser/downloads.go @@ -1,6 +1,7 @@ package browser import ( + "errors" "fmt" "os" "path/filepath" @@ -8,25 +9,37 @@ import ( "sync" "github.com/lichdotdev/lich/api" + "github.com/lichdotdev/lich/internal/profilestore" ) // fallbackName is what a download is called when the page suggests no usable // name. const fallbackName = "download" +// stagingDir is the folder inside the session's profile Chrome writes into. +const stagingDir = "lich-downloads" + +// partSuffix marks a download still being copied into Downloads. +const partSuffix = ".part" + // downloads names what a page saves. Chrome writes each file under its GUID -// (allowAndName) into dir; begin reserves the name the person will see, the -// suggested one made unique the way browsers do ("report (1).csv"), and the -// completed file is renamed to it, so nothing already there is overwritten. +// (allowAndName) into staging; begin reserves the name the person will see in +// dir, the suggested one made unique the way browsers do ("report (1).csv"), +// and the completed file is moved to it, so nothing already there is +// overwritten. The download behaviour is the whole browser's, so what Chrome +// downloads on its own, with no page, lands in staging too; no begin names it, +// nothing moves it, and it goes with the profile. type downloads struct { - dir string - mu sync.Mutex + staging, dir string + // rename is os.Rename, which the tests make cross filesystems. + rename func(from, to string) error + mu sync.Mutex // reserved are the paths in-flight downloads will take, by GUID. reserved map[string]string } -func newDownloads(dir string) *downloads { - return &downloads{dir: dir, reserved: make(map[string]string)} +func newDownloads(staging, dir string) *downloads { + return &downloads{staging: staging, dir: dir, rename: os.Rename, reserved: make(map[string]string)} } // begin reserves a path for the download guid announced. @@ -38,7 +51,7 @@ func (d *downloads) begin(guid, suggested string) api.BrowserDownload { return api.BrowserDownload{Path: path, State: api.DownloadInProgress} } -// progress applies one progress report, renaming the file into place once it +// progress applies one progress report, moving the file into place once it // is complete. ok is false for a GUID begin never saw. func (d *downloads) progress(guid string, received float64, state string) (api.BrowserDownload, bool, error) { d.mu.Lock() @@ -60,14 +73,43 @@ func (d *downloads) progress(guid string, received float64, state string) (api.B path = d.freeLocked(filepath.Base(path)) out.Path = path } - written := filepath.Join(d.dir, guid) - if err := os.Rename(written, path); err != nil { + written := filepath.Join(d.staging, guid) + if err := d.move(written, path); err != nil { out.Path = written return out, true, err } return out, true, nil } +// move renames from to to, and copies where a rename cannot go: the profile +// and Downloads may sit on different filesystems, or volumes on Windows. The +// copy is written under a temporary name beside to and renamed once whole, so +// a half-copied file never carries the download's name. +func (d *downloads) move(from, to string) error { + err := d.rename(from, to) + var linkErr *os.LinkError + if err == nil || !errors.As(err, &linkErr) || !profilestore.CrossDevice(linkErr.Err) { + return err + } + info, err := os.Stat(from) + if err != nil { + return err + } + part := filepath.Join(filepath.Dir(to), "."+filepath.Base(from)+partSuffix) + err = profilestore.CopyFile(from, part) + if err == nil { + err = os.Chmod(part, info.Mode().Perm()) + } + if err == nil { + err = os.Rename(part, to) + } + if err != nil { + _ = os.Remove(part) + return err + } + return os.Remove(from) +} + // freeLocked is name in dir, or its first " (n)" variant that neither exists // nor is reserved. func (d *downloads) freeLocked(name string) string { diff --git a/internal/browser/downloads_other_test.go b/internal/browser/downloads_other_test.go new file mode 100644 index 000000000..5faaa73e3 --- /dev/null +++ b/internal/browser/downloads_other_test.go @@ -0,0 +1,8 @@ +//go:build !windows + +package browser + +import "syscall" + +// errCrossDevice is what rename answers for a file moving across filesystems. +var errCrossDevice error = syscall.EXDEV diff --git a/internal/browser/downloads_test.go b/internal/browser/downloads_test.go index 63a0dbb2e..2496257aa 100644 --- a/internal/browser/downloads_test.go +++ b/internal/browser/downloads_test.go @@ -1,6 +1,8 @@ package browser import ( + "errors" + "io/fs" "os" "path/filepath" "testing" @@ -9,17 +11,17 @@ import ( ) func TestDownloadsNameTheFileWithoutOverwriting(t *testing.T) { - dir := t.TempDir() + d := testDownloadsAt(t) + dir := d.dir if err := os.WriteFile(filepath.Join(dir, "report.csv"), []byte("old"), 0o600); err != nil { t.Fatal(err) } - d := newDownloads(dir) a := d.begin("ga", "report.csv") b := d.begin("gb", "report.csv") if a.Path != filepath.Join(dir, "report (1).csv") || b.Path != filepath.Join(dir, "report (2).csv") { t.Fatalf("reserved %q and %q", a.Path, b.Path) } - if err := os.WriteFile(filepath.Join(dir, "ga"), []byte("new"), 0o600); err != nil { + if err := os.WriteFile(filepath.Join(d.staging, "ga"), []byte("new"), 0o600); err != nil { t.Fatal(err) } got, ok, err := d.progress("ga", 3, api.DownloadCompleted) @@ -32,14 +34,17 @@ func TestDownloadsNameTheFileWithoutOverwriting(t *testing.T) { if b, _ := os.ReadFile(filepath.Join(dir, "report.csv")); string(b) != "old" { t.Fatal("the file already there was overwritten") } + if _, err := os.Lstat(filepath.Join(d.staging, "ga")); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("the staged file is still there (%v)", err) + } } func TestDownloadsRenameAroundAFileThatTookTheName(t *testing.T) { - dir := t.TempDir() - d := newDownloads(dir) + d := testDownloadsAt(t) + dir := d.dir want := d.begin("g", "a.txt").Path - for _, name := range []string{"a.txt", "g"} { - if err := os.WriteFile(filepath.Join(dir, name), []byte(name), 0o600); err != nil { + for _, path := range []string{filepath.Join(dir, "a.txt"), filepath.Join(d.staging, "g")} { + if err := os.WriteFile(path, []byte("x"), 0o600); err != nil { t.Fatal(err) } } @@ -50,8 +55,7 @@ func TestDownloadsRenameAroundAFileThatTookTheName(t *testing.T) { } func TestDownloadsCanceledFreesTheNameAndUnknownIsIgnored(t *testing.T) { - dir := t.TempDir() - d := newDownloads(dir) + d := testDownloadsAt(t) first := d.begin("g", "x.bin").Path if got, ok, _ := d.progress("g", 0, api.DownloadCanceled); !ok || got.State != api.DownloadCanceled { t.Fatalf("canceled reported %+v", got) @@ -65,14 +69,74 @@ func TestDownloadsCanceledFreesTheNameAndUnknownIsIgnored(t *testing.T) { } func TestDownloadsReportTheWrittenFileWhenTheRenameFails(t *testing.T) { - d := newDownloads(t.TempDir()) + d := testDownloadsAt(t) d.begin("g", "a.txt") got, ok, err := d.progress("g", 1, api.DownloadCompleted) - if err == nil || !ok || got.Path != filepath.Join(d.dir, "g") { + if err == nil || !ok || got.Path != filepath.Join(d.staging, "g") { t.Fatalf("got %+v ok=%v err=%v, want the GUID file and the error", got, ok, err) } } +func TestDownloadsCopyAcrossFilesystems(t *testing.T) { + d := testDownloadsAt(t) + d.rename = func(from, to string) error { + return &os.LinkError{Op: "rename", Old: from, New: to, Err: errCrossDevice} + } + want := d.begin("g", "a.txt").Path + staged := filepath.Join(d.staging, "g") + if err := os.WriteFile(staged, []byte("data"), 0o640); err != nil { + t.Fatal(err) + } + got, _, err := d.progress("g", 4, api.DownloadCompleted) + if err != nil || got.Path != want { + t.Fatalf("got %+v (err %v), want %s", got, err, want) + } + if b, err := os.ReadFile(want); err != nil || string(b) != "data" { + t.Fatalf("%s holds %q (%v)", want, b, err) + } + if _, err := os.Lstat(staged); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("the staged file is still there (%v)", err) + } + if names := dirNames(t, d.dir); len(names) != 1 { + t.Fatalf("Downloads holds %v, want only the download", names) + } +} + +func TestDownloadsLeaveWhatNoPageAnnouncedInStaging(t *testing.T) { + d := testDownloadsAt(t) + stray := filepath.Join(d.staging, "stray") + if err := os.WriteFile(stray, []byte("model"), 0o600); err != nil { + t.Fatal(err) + } + if _, ok, err := d.progress("stray", 5, api.DownloadCompleted); ok || err != nil { + t.Fatalf("a file no page announced was handled (ok=%v err=%v)", ok, err) + } + if names := dirNames(t, d.dir); len(names) != 0 { + t.Fatalf("Downloads holds %v, want nothing", names) + } + if _, err := os.Lstat(stray); err != nil { + t.Fatalf("the stray file left staging: %v", err) + } +} + +// testDownloadsAt is a downloads staging in one temporary folder and saving +// into another. +func testDownloadsAt(t *testing.T) *downloads { + return newDownloads(t.TempDir(), t.TempDir()) +} + +func dirNames(t *testing.T, dir string) []string { + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + var names []string + for _, e := range entries { + names = append(names, e.Name()) + } + return names +} + func TestSafeNameKeepsTheFileInTheFolder(t *testing.T) { for in, want := range map[string]string{ "report.csv": "report.csv", diff --git a/internal/browser/downloads_windows_test.go b/internal/browser/downloads_windows_test.go new file mode 100644 index 000000000..062b7ffa6 --- /dev/null +++ b/internal/browser/downloads_windows_test.go @@ -0,0 +1,6 @@ +package browser + +import "golang.org/x/sys/windows" + +// errCrossDevice is what rename answers for a file moving across volumes. +var errCrossDevice error = windows.ERROR_NOT_SAME_DEVICE diff --git a/internal/profilestore/link_windows.go b/internal/profilestore/link_windows.go index 827734c5e..e2b99f8ab 100644 --- a/internal/profilestore/link_windows.go +++ b/internal/profilestore/link_windows.go @@ -42,7 +42,7 @@ func linkFile(target, path string) (copied bool, err error) { if !filepath.IsAbs(target) { target = filepath.Join(filepath.Dir(path), target) } - return true, copyFile(target, path) + return true, CopyFile(target, path) } // IsLink reports whether path is a link this package made: a symlink, or a diff --git a/internal/profilestore/move.go b/internal/profilestore/move.go index fb46b6d69..9e9208d5e 100644 --- a/internal/profilestore/move.go +++ b/internal/profilestore/move.go @@ -97,7 +97,7 @@ func move(from, to string) error { } err := os.Rename(from, to) var linkErr *os.LinkError - if err == nil || !errors.As(err, &linkErr) || !crossDevice(linkErr.Err) { + if err == nil || !errors.As(err, &linkErr) || !CrossDevice(linkErr.Err) { return err } if err := copyTree(from, to); err != nil { @@ -135,7 +135,7 @@ func copyTree(from, to string) error { dirs = append(dirs, dirMode{dst, info.Mode().Perm()}) return os.MkdirAll(dst, 0o700) } - if err := copyFile(path, dst); err != nil { + if err := CopyFile(path, dst); err != nil { return err } return os.Chmod(dst, info.Mode().Perm()) @@ -178,7 +178,8 @@ func copyLink(src, dst string) error { return err } -func copyFile(src, dst string) error { +// CopyFile copies src to dst, which it creates and never overwrites. +func CopyFile(src, dst string) error { in, err := os.Open(src) if err != nil { return err diff --git a/internal/profilestore/move_other.go b/internal/profilestore/move_other.go index f2d23a98c..1ac274d88 100644 --- a/internal/profilestore/move_other.go +++ b/internal/profilestore/move_other.go @@ -8,8 +8,8 @@ import ( "syscall" ) -// crossDevice reports a rename refused for crossing filesystems. -func crossDevice(err error) bool { return errors.Is(err, syscall.EXDEV) } +// CrossDevice reports a rename refused for crossing filesystems. +func CrossDevice(err error) bool { return errors.Is(err, syscall.EXDEV) } // replaceFile puts from in place of to; rename(2) replaces atomically. func replaceFile(from, to string) error { return os.Rename(from, to) } diff --git a/internal/profilestore/move_other_test.go b/internal/profilestore/move_other_test.go index 1b92e6a6e..0584aaf5e 100644 --- a/internal/profilestore/move_other_test.go +++ b/internal/profilestore/move_other_test.go @@ -10,10 +10,10 @@ import ( ) func TestCrossDeviceKnowsFilesystems(t *testing.T) { - if !crossDevice(syscall.EXDEV) { + if !CrossDevice(syscall.EXDEV) { t.Error("EXDEV was not read as crossing filesystems, so the copy never runs") } - if crossDevice(syscall.ENOENT) { + if CrossDevice(syscall.ENOENT) { t.Error("a missing file was read as crossing filesystems") } } diff --git a/internal/profilestore/move_windows.go b/internal/profilestore/move_windows.go index e4c64f059..80985c80f 100644 --- a/internal/profilestore/move_windows.go +++ b/internal/profilestore/move_windows.go @@ -8,11 +8,11 @@ import ( "golang.org/x/sys/windows" ) -// crossDevice reports a rename refused for crossing volumes. os.Rename is +// CrossDevice reports a rename refused for crossing volumes. os.Rename is // MoveFileEx without MOVEFILE_COPY_ALLOWED, which answers // ERROR_NOT_SAME_DEVICE there; syscall.EXDEV on Windows is an errno Go // invented that no call returns. -func crossDevice(err error) bool { return errors.Is(err, windows.ERROR_NOT_SAME_DEVICE) } +func CrossDevice(err error) bool { return errors.Is(err, windows.ERROR_NOT_SAME_DEVICE) } // replaceRetryFor bounds how long replaceFile retries, the 2s Go's own // cmd/go/internal/robustio gives the same errors. diff --git a/internal/profilestore/move_windows_test.go b/internal/profilestore/move_windows_test.go index 9b73e39b1..e52fcfe63 100644 --- a/internal/profilestore/move_windows_test.go +++ b/internal/profilestore/move_windows_test.go @@ -10,7 +10,7 @@ import ( func TestCrossDeviceKnowsWindowsVolumes(t *testing.T) { err := &os.LinkError{Op: "rename", Old: `C:\a`, New: `D:\b`, Err: windows.ERROR_NOT_SAME_DEVICE} - if !crossDevice(err.Err) { + if !CrossDevice(err.Err) { t.Error("ERROR_NOT_SAME_DEVICE was not read as crossing volumes, so the copy never runs") } }