A real release download finishes: the 3-second check clock no longer covers the asset

cmd/codeaf/chatv3_surface.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"log"
 	"os"
-	"time"
 
 	"github.com/Agent-Field/codeaf/internal/buildinfo"
 	internalenv "github.com/Agent-Field/codeaf/internal/env"
@@ -44,7 +43,7 @@ var (
 // reads this package's sources rather than trusting the next door to remember.
 func runSurface(ctx context.Context, options tui3.Options) error {
 	revision := surfaceRevision()
-	client := surfaceUpdateClient(revision, 3*time.Second)
+	client := surfaceUpdateClient(revision, codeupdate.CheckTimeout)
 	restart := options.Restart
 	if restart == nil {
 		restart = &codeupdate.Plan{}

cmd/codeaf/update.go

@@ -7,7 +7,6 @@ import (
 	"os"
 	"os/exec"
 	"strings"
-	"time"
 
 	"github.com/Agent-Field/codeaf/internal/buildinfo"
 	codeupdate "github.com/Agent-Field/codeaf/internal/update"
@@ -72,8 +71,14 @@ func runUpdate(args []string) error {
 			return exitStatus(2)
 		}
 	}
-	client := updateClient(running, 3*time.Second)
-	release, err := client.Select(context.Background(), choice)
+	client := updateClient(running, codeupdate.CheckTimeout)
+	var release codeupdate.Release
+	var err error
+	if *check {
+		release, err = client.Check(context.Background(), choice)
+	} else {
+		release, err = client.Select(context.Background(), choice)
+	}
 	if err != nil {
 		if *check {
 			fmt.Fprintln(updateErr, "codeaf: could not check for an update:", err)

cmd/codeaf/update_test.go

@@ -77,6 +77,68 @@ func TestC9UpdateCheckUsesTheRealDoorAndItsThreeExitCodes(t *testing.T) {
 	}
 }
 
+// TestTimeoutContractC2HangingTerminalCheckExitsInsideItsBudget proves C2 for
+// the real `codeaf update --check` door and its exit-1 failure road.
+func TestTimeoutContractC2HangingTerminalCheckExitsInsideItsBudget(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, request *http.Request) {
+		<-request.Context().Done()
+	}))
+	defer server.Close()
+	client := &codeupdate.Client{HTTP: server.Client(), APIBase: server.URL, DownloadBase: server.URL}
+	stdout, stderr := withUpdateDoor(t, "v0.1.0", client, filepath.Join(t.TempDir(), "codeaf"))
+	started := time.Now()
+	if got := updateExit(runUpdate([]string{"--check"})); got != 1 {
+		t.Fatalf("exit = %d, want 1; stdout %q stderr %q", got, stdout.String(), stderr.String())
+	}
+	if elapsed := time.Since(started); elapsed > 6*time.Second {
+		t.Fatalf("hanging terminal check took %s, want at most 6s", elapsed)
+	}
+	if stdout.Len() != 0 || !strings.Contains(stderr.String(), "codeaf: could not check for an update: release API did not answer within 3 s") {
+		t.Fatalf("stdout %q stderr %q", stdout.String(), stderr.String())
+	}
+	if strings.Contains(stderr.String(), "context deadline exceeded") || strings.Contains(stderr.String(), "Client.Timeout") {
+		t.Fatalf("terminal check exposed Go's timeout: %q", stderr.String())
+	}
+}
+
+// TestTimeoutContractC4HangingInstallSelectionNamesTheAPI proves C4 through
+// the terminal install door with a short injected API window.
+func TestTimeoutContractC4HangingInstallSelectionNamesTheAPI(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, request *http.Request) {
+		<-request.Context().Done()
+	}))
+	defer server.Close()
+	dir := t.TempDir()
+	target := filepath.Join(dir, "codeaf")
+	original := []byte("original codeaf")
+	if err := os.WriteFile(target, original, 0o700); err != nil {
+		t.Fatal(err)
+	}
+	client := &codeupdate.Client{
+		HTTP: server.Client(), APIBase: server.URL, DownloadBase: server.URL,
+		APIWindow: 60 * time.Millisecond,
+	}
+	_, _ = withUpdateDoor(t, "v0.1.0", client, target)
+	started := time.Now()
+	err := runUpdate(nil)
+	if err == nil || !strings.Contains(err.Error(), "could not select a release: release API did not answer within 60 ms") {
+		t.Fatalf("error = %v", err)
+	}
+	if elapsed := time.Since(started); elapsed > time.Second {
+		t.Fatalf("hanging install selection took %s, want at most 1s", elapsed)
+	}
+	if strings.Contains(err.Error(), "codeaf-") || strings.Contains(err.Error(), "context deadline exceeded") {
+		t.Fatalf("API failure named an asset or Go's deadline: %q", err)
+	}
+	installed, readErr := os.ReadFile(target)
+	if readErr != nil || string(installed) != string(original) {
+		t.Fatalf("target = %q, error %v; want untouched original", installed, readErr)
+	}
+	if matches, _ := filepath.Glob(filepath.Join(dir, ".codeaf.tmp.*")); len(matches) != 0 {
+		t.Fatalf("temporary files remain: %v", matches)
+	}
+}
+
 // TestC9bUpdateCheckNamesTheSelectedTagAndUsesItsChannelRules proves C9b.
 func TestC9bUpdateCheckNamesTheSelectedTagAndUsesItsChannelRules(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

docs/changes/unreleased/1110-update-download-timeout.md

@@ -0,0 +1,17 @@
+---
+kind: fixed
+title: a release download finishes over a real network, on its own clock
+pr: 1110
+surface: [chat, build]
+invalidates:
+  - "#1060's `/update` and `codeaf update` were believed to install a release over any working network. They could not finish one at all: both doors built the release client with the three-second budget meant for the launch check, and Go's `http.Client.Timeout` covers the whole exchange including every body byte, so the 57,532,578-byte `codeaf-linux-amd64` — about eight seconds on a healthy link — was cut off three seconds in with `read the release response: context deadline exceeded`. `--check` passed everywhere because its answer is a few KB, and every hand test served the asset from a local mirror in milliseconds."
+  - "The release client no longer sets `http.Client.Timeout` at all, and there is no longer one clock over the whole road. Its transport bounds only what has made no progress — a five-second dial, a five-second TLS handshake, a ten-second wait for response headers — and every call takes its deadline from its context: the launch check and `codeaf update --check` keep a three-second whole-exchange deadline from the single `update.CheckTimeout` constant that both doors read, the release metadata an install reads gets ten seconds per request, and the asset download gets a thirty-second no-byte stall window plus a fifteen-minute ceiling. A dead link still fails in seconds; a slow one now succeeds."
+  - "A failed update could hand a person Go's own deadline text. It cannot now: a stall says `downloading codeaf-linux-amd64 stalled — no bytes for 30 s`, the ceiling says `downloading codeaf-linux-amd64 took longer than 15 minutes`, late headers say `codeaf-linux-amd64 did not start arriving within 10 s`, and a release request that does not answer names the release API and its window. The terminal road still ends with the curl line and the chat still offers it."
+---
+
+A contract item about time has to be proved with a slow server. Every hand test
+behind #1060 downloaded the asset from a local mirror, which serves 57 MB in
+milliseconds, so the one clock that mattered was the one nothing ever ran. The
+tests here inject the stall window and the ceiling and then use real slow,
+silent and trickling servers, and the two roads were walked again against the
+published v0.2.0 release.

internal/update/check.go

@@ -80,7 +80,7 @@ func CheckLaunch(ctx context.Context, options CheckOptions) (Available, bool) {
 	if options.Client == nil {
 		return Available{}, false
 	}
-	release, err := options.Client.Select(ctx, Choice{Channel: "stable"})
+	release, err := options.Client.Check(ctx, Choice{Channel: "stable"})
 	if err != nil {
 		return Available{}, false
 	}

internal/update/client.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/url"
 	"strings"
@@ -28,20 +29,37 @@ const (
 	GitHubDownloadEnv = "CODEAF_GITHUB_DOWNLOAD"
 )
 
+const (
+	// CheckTimeout is the whole-exchange budget for a launch check and for
+	// `codeaf update --check`. THE CHECK CLOCK NEVER COVERS AN INSTALL: a
+	// release asset is allowed to make steady progress for much longer.
+	CheckTimeout = 3 * time.Second
+
+	networkSetupTimeout = 5 * time.Second
+	apiRequestTimeout   = 10 * time.Second
+	downloadStallWindow = 30 * time.Second
+	downloadCeiling     = 15 * time.Minute
+)
+
 const primaryRepository = "Agent-Field/codeaf"
 const legacyRepository = "Agent-Field/aforge-v2" // legacy-name
 
 // Client talks to the release API and release asset host.
 type Client struct {
-	HTTP         *http.Client
-	APIBase      string
-	DownloadBase string
-	Token        string
-	Revision     string
+	HTTP            *http.Client
+	APIBase         string
+	DownloadBase    string
+	Token           string
+	Revision        string
+	CheckWindow     time.Duration
+	APIWindow       time.Duration
+	StallWindow     time.Duration
+	DownloadCeiling time.Duration
 }
 
 // NewClient builds the release client from the same mirror and token inputs as
-// the shell installer. The timeout covers the whole request, including bodies.
+// the shell installer. The timeout belongs only to the small release check;
+// every install request carries the clock for the particular thing it reads.
 func NewClient(revision string, timeout time.Duration) *Client {
 	apiBase := strings.TrimRight(internalenv.Get(GitHubAPIEnv), "/")
 	if apiBase == "" {
@@ -56,19 +74,34 @@ func NewClient(revision string, timeout time.Duration) *Client {
 		token = internalenv.Value("GH_TOKEN")
 	}
 	return &Client{
-		HTTP:         &http.Client{Timeout: timeout},
-		APIBase:      apiBase,
-		DownloadBase: downloadBase,
-		Token:        token,
-		Revision:     revision,
+		HTTP:            defaultHTTP(),
+		APIBase:         apiBase,
+		DownloadBase:    downloadBase,
+		Token:           token,
+		Revision:        revision,
+		CheckWindow:     timeout,
+		APIWindow:       apiRequestTimeout,
+		StallWindow:     downloadStallWindow,
+		DownloadCeiling: downloadCeiling,
 	}
 }
 
+// defaultHTTP bounds only the parts of a request that have made no progress.
+// In particular, Client.Timeout stays zero: that field includes every body
+// byte and was the three-second clock that cut a healthy 57 MB download off.
+func defaultHTTP() *http.Client {
+	transport := http.DefaultTransport.(*http.Transport).Clone()
+	transport.DialContext = (&net.Dialer{Timeout: networkSetupTimeout}).DialContext
+	transport.TLSHandshakeTimeout = networkSetupTimeout
+	transport.ResponseHeaderTimeout = apiRequestTimeout
+	return &http.Client{Transport: transport}
+}
+
 func (c *Client) httpClient() *http.Client {
 	if c != nil && c.HTTP != nil {
 		return c.HTTP
 	}
-	return &http.Client{Timeout: 3 * time.Second}
+	return defaultHTTP()
 }
 
 // requestClient keeps the caller's redirect decisions while enforcing the
@@ -78,6 +111,9 @@ func (c *Client) httpClient() *http.Client {
 func (c *Client) requestClient() *http.Client {
 	base := c.httpClient()
 	client := *base
+	// EVERY CALL GETS ITS DEADLINE FROM ITS CONTEXT. An injected or reused
+	// client must not smuggle a whole-body Client.Timeout back into downloads.
+	client.Timeout = 0
 	checkRedirect := base.CheckRedirect
 	client.CheckRedirect = func(request *http.Request, via []*http.Request) error {
 		if checkRedirect != nil {
@@ -112,30 +148,57 @@ func (e *statusError) Error() string {
 	return fmt.Sprintf("%s answered HTTP %d", e.resource, e.code)
 }
 
-type requestError struct{ cause error }
-
-func (e *requestError) Error() string { return "release request failed: " + e.cause.Error() }
-func (e *requestError) Unwrap() error { return e.cause }
-
 func isStatus(err error, code int) bool {
 	var status *statusError
 	return errors.As(err, &status) && status.code == code
 }
 
-func nameReleaseResource(err error, resource string) error {
-	var status *statusError
-	if errors.As(err, &status) {
-		return &statusError{code: status.code, resource: resource}
+func (c *Client) checkWindow() time.Duration {
+	if c != nil && c.CheckWindow > 0 {
+		return c.CheckWindow
+	}
+	return CheckTimeout
+}
+
+func (c *Client) apiWindow() time.Duration {
+	if c != nil && c.APIWindow > 0 {
+		return c.APIWindow
+	}
+	return apiRequestTimeout
+}
+
+func (c *Client) stallWindow() time.Duration {
+	if c != nil && c.StallWindow > 0 {
+		return c.StallWindow
+	}
+	return downloadStallWindow
+}
+
+func (c *Client) downloadCeiling() time.Duration {
+	if c != nil && c.DownloadCeiling > 0 {
+		return c.DownloadCeiling
 	}
-	var request *requestError
-	if errors.As(err, &request) {
-		return fmt.Errorf("%s request failed: %w", resource, request.cause)
+	return downloadCeiling
+}
+
+// Check selects one release inside the short whole-exchange window shared by
+// the launch notice and the terminal's explicit check. Installation calls
+// Select directly, where each metadata request receives its own API window.
+func (c *Client) Check(ctx context.Context, choice Choice) (Release, error) {
+	window := c.checkWindow()
+	check, cancel := context.WithTimeout(ctx, window)
+	defer cancel()
+	release, err := c.Select(check, choice)
+	if err != nil && ctx.Err() == nil && errors.Is(check.Err(), context.DeadlineExceeded) {
+		return Release{}, fmt.Errorf("release API did not answer within %s", spellDuration(window))
 	}
-	return err
+	return release, err
 }
 
-func (c *Client) get(ctx context.Context, rawURL, accept string, api bool) ([]byte, error) {
-	req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil)
+func (c *Client) get(ctx context.Context, rawURL, accept string, api bool, resource string) ([]byte, error) {
+	requestContext, cancel := context.WithTimeout(ctx, c.apiWindow())
+	defer cancel()
+	req, err := http.NewRequestWithContext(requestContext, http.MethodGet, rawURL, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -148,24 +211,56 @@ func (c *Client) get(ctx context.Context, rawURL, accept string, api bool) ([]by
 	}
 	resp, err := c.requestClient().Do(req)
 	if err != nil {
-		var requestURL *url.Error
-		if errors.As(err, &requestURL) {
-			err = requestURL.Err
-		}
-		return nil, &requestError{cause: err}
+		return nil, metadataRequestFailure(ctx, requestContext, err, resource, c.apiWindow())
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
 		_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096))
-		return nil, &statusError{code: resp.StatusCode, resource: "release API"}
+		return nil, &statusError{code: resp.StatusCode, resource: resource}
 	}
 	body, err := io.ReadAll(resp.Body)
 	if err != nil {
-		return nil, fmt.Errorf("read the release response: %w", err)
+		return nil, metadataRequestFailure(ctx, requestContext, err, resource, c.apiWindow())
 	}
 	return body, nil
 }
 
+func metadataRequestFailure(parent, request context.Context, err error, resource string, window time.Duration) error {
+	if parent.Err() != nil {
+		if errors.Is(parent.Err(), context.DeadlineExceeded) {
+			return fmt.Errorf("%s did not answer before this update stopped", resource)
+		}
+		return fmt.Errorf("the %s request was stopped", resource)
+	}
+	if errors.Is(request.Err(), context.DeadlineExceeded) || networkTimedOut(err) {
+		return fmt.Errorf("%s did not answer within %s", resource, spellDuration(window))
+	}
+	return fmt.Errorf("%s could not be reached", resource)
+}
+
+func networkTimedOut(err error) bool {
+	var networkError net.Error
+	return errors.As(err, &networkError) && networkError.Timeout()
+}
+
+func spellDuration(duration time.Duration) string {
+	switch {
+	case duration%time.Minute == 0:
+		minutes := duration / time.Minute
+		unit := "minutes"
+		if minutes == 1 {
+			unit = "minute"
+		}
+		return fmt.Sprintf("%d %s", minutes, unit)
+	case duration%time.Second == 0:
+		return fmt.Sprintf("%d s", duration/time.Second)
+	case duration%time.Millisecond == 0:
+		return fmt.Sprintf("%d ms", duration/time.Millisecond)
+	default:
+		return duration.String()
+	}
+}
+
 // Choice selects one release channel or one exact tag.
 type Choice struct {
 	Channel string
@@ -224,7 +319,7 @@ func (c *Client) selectRepository(ctx context.Context, repository, channel strin
 		suffix = "releases?per_page=100"
 	}
 	rawURL := strings.TrimRight(c.APIBase, "/") + "/repos/" + repository + "/" + suffix
-	body, err := c.get(ctx, rawURL, "application/vnd.github+json", true)
+	body, err := c.get(ctx, rawURL, "application/vnd.github+json", true, "release API")
 	if err != nil {
 		return Release{}, err
 	}