From 3aa8bfc17fca98fe119d87f5971424c65e1bdf09 Mon Sep 17 00:00:00 2001 From: omartelo Date: Wed, 28 Oct 2026 10:15:30 -0300 Subject: [PATCH] fix(browser): move a finished download off the CDP listener A completed download was moved into Downloads under the downloads lock, inside Chrome's browser event listener. Across filesystems the move is a copy as long as the file, so every browser event, and every other download, waited behind it. progress now only keeps the name reserved; place moves the file outside the lock, on its own goroutine, and the agent hears completed once the file is in place. A move that fails is reported as a new failed state ("download failed: ") instead of the staged path, which goes with the profile. --- api/browser.go | 4 +- docs/browser-tab.md | 4 +- docs/ceilings.md | 5 +- internal/browser/chrome_view.go | 12 +++-- internal/browser/downloads.go | 41 +++++++++++------ internal/browser/downloads_test.go | 74 ++++++++++++++++++++++-------- internal/cli/browser_page.go | 2 + internal/cli/browser_page_test.go | 7 +++ 8 files changed, 109 insertions(+), 40 deletions(-) diff --git a/api/browser.go b/api/browser.go index 1ac0e13b..e64cc9a3 100644 --- a/api/browser.go +++ b/api/browser.go @@ -61,11 +61,13 @@ var ( BrowserDownloadEvent = defineEvent[BrowserDownload]("browser-download") ) -// Download states, Chrome's own (Browser.downloadProgress). +// Download states, Chrome's own (Browser.downloadProgress), and lich's +// DownloadFailed: Chrome completed it, and moving it into Downloads failed. const ( DownloadInProgress = "inProgress" DownloadCompleted = "completed" DownloadCanceled = "canceled" + DownloadFailed = "failed" ) // BrowserDownload is one file a session's page downloaded: Path is where it diff --git a/docs/browser-tab.md b/docs/browser-tab.md index a72f53d1..c740425a 100644 --- a/docs/browser-tab.md +++ b/docs/browser-tab.md @@ -71,7 +71,9 @@ 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 +`downloading:`, and the next page read says when it ends. A file Chrome +finished but lich could not move into Downloads answers `download failed:` +with the path it would have taken. `browser_open` and `browser_navigate` on a URL that answers with a file download it the same way: Chrome aborts that navigation and the page stays where it was, so the answer is the handle with `downloads` rather than an error. That answer also diff --git a/docs/ceilings.md b/docs/ceilings.md index 7849647c..a354e2f2 100644 --- a/docs/ceilings.md +++ b/docs/ceilings.md @@ -998,8 +998,9 @@ work when nobody knows it and that the call site never shows. The mechanism and 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 + filesystem or volume the move is a copy, written under a hidden `.part` name and renamed once whole; the agent + hears the download completed once it is in place. A move that fails is reported to the agent as a failed download + and the file goes with the profile; the window shows no toast for it. 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_view.go b/internal/browser/chrome_view.go index dc936d7c..0c1d7259 100644 --- a/internal/browser/chrome_view.go +++ b/internal/browser/chrome_view.go @@ -62,13 +62,17 @@ func (p *chromePage) watchDownloads(onDownload func(guid string, d api.BrowserDo case *browser.EventDownloadWillBegin: onDownload(e.GUID, p.dl.begin(e.GUID, e.SuggestedFilename)) case *browser.EventDownloadProgress: - d, ok, err := p.dl.progress(e.GUID, e.ReceivedBytes, string(e.State)) - if err != nil { - slog.Warn("browser: name a finished download", "path", d.Path, "err", err) + d, ok := p.dl.progress(e.GUID, e.ReceivedBytes, string(e.State)) + if !ok { + return } - if ok { + if d.State != api.DownloadCompleted { onDownload(e.GUID, d) + return } + // Every browser event waits behind this listener, and a move + // across disks takes as long as the file. + go func() { onDownload(e.GUID, p.dl.place(e.GUID, d)) }() } }) } diff --git a/internal/browser/downloads.go b/internal/browser/downloads.go index 4db86e50..ea3f5b80 100644 --- a/internal/browser/downloads.go +++ b/internal/browser/downloads.go @@ -3,6 +3,7 @@ package browser import ( "errors" "fmt" + "log/slog" "os" "path/filepath" "strings" @@ -51,34 +52,46 @@ func (d *downloads) begin(guid, suggested string) api.BrowserDownload { return api.BrowserDownload{Path: path, State: api.DownloadInProgress} } -// 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) { +// progress applies one progress report. A completed download is not in place +// yet: its name stays reserved until place moves it there. ok is false for a +// GUID begin never saw. +func (d *downloads) progress(guid string, received float64, state string) (api.BrowserDownload, bool) { d.mu.Lock() defer d.mu.Unlock() path, ok := d.reserved[guid] if !ok { - return api.BrowserDownload{}, false, nil + return api.BrowserDownload{}, false } out := api.BrowserDownload{Path: path, Bytes: int64(received), State: state} if state == api.DownloadInProgress { - return out, true, nil + return out, true } - delete(d.reserved, guid) if state != api.DownloadCompleted { - return out, true, nil + delete(d.reserved, guid) + return out, true } if _, err := os.Lstat(path); err == nil { // Something else took the name while the file downloaded. - path = d.freeLocked(filepath.Base(path)) - out.Path = path + out.Path = d.freeLocked(filepath.Base(path)) + d.reserved[guid] = out.Path } - written := filepath.Join(d.staging, guid) - if err := d.move(written, path); err != nil { - out.Path = written - return out, true, err + return out, true +} + +// place moves the completed download dl from staging to its reserved path, +// outside the lock: across filesystems the move is a copy as long as the file. +// A move that fails reports the download failed, since the staged file goes +// with the profile. +func (d *downloads) place(guid string, dl api.BrowserDownload) api.BrowserDownload { + err := d.move(filepath.Join(d.staging, guid), dl.Path) + d.mu.Lock() + delete(d.reserved, guid) + d.mu.Unlock() + if err != nil { + slog.Warn("browser: move a finished download", "path", dl.Path, "err", err) + dl.State = api.DownloadFailed } - return out, true, nil + return dl } // move renames from to to, and copies where a rename cannot go: the profile diff --git a/internal/browser/downloads_test.go b/internal/browser/downloads_test.go index 2496257a..5bf44907 100644 --- a/internal/browser/downloads_test.go +++ b/internal/browser/downloads_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/lichdotdev/lich/api" ) @@ -24,9 +25,9 @@ func TestDownloadsNameTheFileWithoutOverwriting(t *testing.T) { 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) - if err != nil || !ok || got.Path != a.Path || got.Bytes != 3 { - t.Fatalf("completed %+v ok=%v err=%v", got, ok, err) + got, ok := complete(d, "ga", 3) + if !ok || got.Path != a.Path || got.Bytes != 3 || got.State != api.DownloadCompleted { + t.Fatalf("completed %+v ok=%v", got, ok) } if b, _ := os.ReadFile(a.Path); string(b) != "new" { t.Fatalf("%s holds %q, want the download", a.Path, b) @@ -48,32 +49,60 @@ func TestDownloadsRenameAroundAFileThatTookTheName(t *testing.T) { t.Fatal(err) } } - got, _, err := d.progress("g", 1, api.DownloadCompleted) - if err != nil || got.Path != filepath.Join(dir, "a (1).txt") { - t.Fatalf("got %q (err %v) instead of a free name beside %q", got.Path, err, want) + got, _ := complete(d, "g", 1) + if got.State != api.DownloadCompleted || got.Path != filepath.Join(dir, "a (1).txt") { + t.Fatalf("got %+v instead of a free name beside %q", got, want) } } func TestDownloadsCanceledFreesTheNameAndUnknownIsIgnored(t *testing.T) { d := testDownloadsAt(t) first := d.begin("g", "x.bin").Path - if got, ok, _ := d.progress("g", 0, api.DownloadCanceled); !ok || got.State != api.DownloadCanceled { + if got, ok := d.progress("g", 0, api.DownloadCanceled); !ok || got.State != api.DownloadCanceled { t.Fatalf("canceled reported %+v", got) } if again := d.begin("h", "x.bin").Path; again != first { t.Fatalf("a canceled download kept %q reserved; got %q", first, again) } - if _, ok, _ := d.progress("nobody", 1, api.DownloadCompleted); ok { + if _, ok := d.progress("nobody", 1, api.DownloadCompleted); ok { t.Fatal("a GUID never begun was reported") } } -func TestDownloadsReportTheWrittenFileWhenTheRenameFails(t *testing.T) { +func TestDownloadsReportFailedWhenTheMoveFails(t *testing.T) { 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.staging, "g") { - t.Fatalf("got %+v ok=%v err=%v, want the GUID file and the error", got, ok, err) + want := d.begin("g", "a.txt").Path + got, ok := complete(d, "g", 1) + if !ok || got.State != api.DownloadFailed || got.Path != want { + t.Fatalf("got %+v ok=%v, want %s failed", got, ok, want) + } + if again := d.begin("h", "a.txt").Path; again != want { + t.Fatalf("a failed download kept %q reserved; got %q", want, again) + } +} + +func TestDownloadsKeepTheNameAndTheLockFreeWhileTheFileMoves(t *testing.T) { + d := testDownloadsAt(t) + want := d.begin("g", "a.txt").Path + if err := os.WriteFile(filepath.Join(d.staging, "g"), []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + var during string + d.rename = func(from, to string) error { + begun := make(chan string, 1) + go func() { begun <- d.begin("h", "a.txt").Path }() + select { + case during = <-begun: + case <-time.After(time.Second): + t.Error("a download began only after the move: the lock was held") + } + return os.Rename(from, to) + } + if got, _ := complete(d, "g", 1); got.State != api.DownloadCompleted || got.Path != want { + t.Fatalf("got %+v, want %s completed", got, want) + } + if during != filepath.Join(d.dir, "a (1).txt") { + t.Fatalf("a download begun during the move took %q", during) } } @@ -87,9 +116,8 @@ func TestDownloadsCopyAcrossFilesystems(t *testing.T) { 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 got, _ := complete(d, "g", 4); got.State != api.DownloadCompleted || got.Path != want { + t.Fatalf("got %+v, want %s completed", got, want) } if b, err := os.ReadFile(want); err != nil || string(b) != "data" { t.Fatalf("%s holds %q (%v)", want, b, err) @@ -108,8 +136,8 @@ func TestDownloadsLeaveWhatNoPageAnnouncedInStaging(t *testing.T) { 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 _, ok := d.progress("stray", 5, api.DownloadCompleted); ok { + t.Fatal("a file no page announced was handled") } if names := dirNames(t, d.dir); len(names) != 0 { t.Fatalf("Downloads holds %v, want nothing", names) @@ -119,6 +147,16 @@ func TestDownloadsLeaveWhatNoPageAnnouncedInStaging(t *testing.T) { } } +// complete reports guid completed and places it, the way the page's +// listener does. +func complete(d *downloads, guid string, received float64) (api.BrowserDownload, bool) { + dl, ok := d.progress(guid, received, api.DownloadCompleted) + if !ok { + return dl, false + } + return d.place(guid, dl), true +} + // testDownloadsAt is a downloads staging in one temporary folder and saving // into another. func testDownloadsAt(t *testing.T) *downloads { diff --git a/internal/cli/browser_page.go b/internal/cli/browser_page.go index 3027899a..da575259 100644 --- a/internal/cli/browser_page.go +++ b/internal/cli/browser_page.go @@ -88,6 +88,8 @@ func downloadLine(d api.BrowserDownload) string { return fmt.Sprintf("downloaded: %s (%s)", d.Path, sizeText(d.Bytes)) case api.DownloadInProgress: return fmt.Sprintf("downloading: %s (%s so far)", d.Path, sizeText(d.Bytes)) + case api.DownloadFailed: + return "download failed: " + d.Path default: return "download canceled: " + d.Path } diff --git a/internal/cli/browser_page_test.go b/internal/cli/browser_page_test.go index 7681d331..3d7d7320 100644 --- a/internal/cli/browser_page_test.go +++ b/internal/cli/browser_page_test.go @@ -44,6 +44,13 @@ func TestPageTextIsOneLinePerElement(t *testing.T) { } } +func TestDownloadLineSaysAFailedMove(t *testing.T) { + d := api.BrowserDownload{Path: "/home/u/Downloads/report.csv", Bytes: 5, State: api.DownloadFailed} + if got := downloadLine(d); got != "download failed: /home/u/Downloads/report.csv" { + t.Fatalf("got %q", got) + } +} + func TestPageTextOfAnEmptyPage(t *testing.T) { if got := pageText(api.BrowserPageInfo{URL: "about:blank"}); got != "url: about:blank\ntitle: " { t.Fatalf("got %q", got)