fix(browser): move a finished download off the CDP listener

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

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

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 `<a download>` 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 `<a download>` 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

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)) }()
 		}
 	})
 }

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

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 {

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
 	}

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)