Return the context's error when it ends during a fetch request (closes #221)

retryingClient.get checked its context right after each request and
returned that request's error, so a deadline that passed while a 503 was
on its way came back as the 503. TestGetStopsWaitingWhenCanceled failed
on that now and then on a loaded host. When the context ends before a
retry, during the request or during the wait, get now returns an error
wrapping both the context's error and the last try's error.

The new test answers through a transport that holds the 503 until the
context has ended. A real server cannot do that: the client drops a
response still on its way when the context ends.

Model: opus-5-5

internal/cli/fetch_test.go

@@ -897,31 +897,23 @@ func TestGetHonorsRetryAfter(t *testing.T) {
 	}
 }
 
-// instant503Transport answers every request with a 503 at once, so the
+// TestGetStopsWaitingWhenCanceled gives get a server that always answers
-// 503 is in hand before any deadline can pass.
+// 503 and an hour's wait before each retry, then lets the context expire
-type instant503Transport struct{}
+// during that wait. get returning at all shows it stopped waiting.
-
-func (instant503Transport) RoundTrip(*http.Request) (*http.Response, error) {
-	return &http.Response{
-		StatusCode: http.StatusServiceUnavailable,
-		Body:       http.NoBody,
-	}, nil
-}
-
-// TestGetStopsWaitingWhenCanceled answers every request with a 503 and
-// sets an hour's wait before each retry, then lets the context expire
-// during that wait. get returning at all shows it stopped waiting; it must
-// return the deadline error and keep the 503 with it.
 func TestGetStopsWaitingWhenCanceled(t *testing.T) {
 	t.Parallel()
 
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			w.WriteHeader(http.StatusServiceUnavailable)
+		}))
+	defer server.Close()
+
 	client := testClient()
-	client.client.Transport = instant503Transport{}
 	client.firstDelay = time.Hour
 
-	err := getNothing(client, "http://example.invalid/", 50*time.Millisecond)
+	err := getNothing(client, server.URL, 50*time.Millisecond)
 	require.ErrorIs(t, err, context.DeadlineExceeded)
-	require.ErrorIs(t, err, errHTTPStatus)
 }
 
 // late503Transport answers every request with a 503, but only once the

internal/cli/freshen.go

@@ -365,39 +365,29 @@ func writeFreshenedManifest(
 }
 
 // newFreshenBuilder constructs the manifest builder configured from CLI
-// flags, and returns the fingerprint of the --sign-key key, or "" when
+// flags. A --sign-key given with an empty value is refused.
-// none is given. A --sign-key given with an empty value is refused.
+func (mfa *CLIApp) newFreshenBuilder(cmd *cli.Command) (*mfer.Builder, error) {
-func (mfa *CLIApp) newFreshenBuilder(
-	cmd *cli.Command,
-) (*mfer.Builder, string, error) {
 	builder := mfer.NewBuilder()
 	if cmd.Bool("include-timestamps") {
 		builder.SetIncludeTimestamps(true)
 	}
 
-	var signKeyFingerprint string
-
 	// Set up signing options if sign-key is provided
 	if cmd.IsSet("sign-key") {
 		signKey := cmd.String("sign-key")
 		if signKey == "" {
-			return nil, "", errEmptySignKey
+			return nil, errEmptySignKey
 		}
 
 		signing, err := mfa.signingOptions(signKey)
 		if err != nil {
-			return nil, "", err
+			return nil, err
-		}
-
-		signKeyFingerprint, err = mfer.SecretKeyFingerprint(signing.SecretKey)
-		if err != nil {
-			return nil, "", fmt.Errorf("%s: %w", signKey, err)
 		}
 
 		builder.SetSigningOptions(signing)
 	}
 
-	return builder, signKeyFingerprint, nil
+	return builder, nil
 }
 
 // freshenScan runs the scan phase against the loaded manifest entries
@@ -509,19 +499,12 @@ func runFreshenHash(
 	return nil
 }
 
-// existingManifest is what freshen uses of the manifest it rewrites.
+// loadExistingEntries loads the manifest and indexes its file entries
-type existingManifest struct {
-	entriesByPath map[string]*mfer.MFFilePath
-	signer        string // fingerprint of the key it is signed with, "" if unsigned
-	hasCreatedAt  bool
-}
-
-// loadExistingManifest loads the manifest and indexes its file entries
 // by path. It refuses a signed manifest when no --sign-key is given, as
 // the rewritten manifest would be unsigned.
-func (mfa *CLIApp) loadExistingManifest(
+func (mfa *CLIApp) loadExistingEntries(
 	cmd *cli.Command, manifestPath string,
-) (*existingManifest, error) {
+) (map[string]*mfer.MFFilePath, error) {
 	log.Infof("loading manifest from %s", manifestPath)
 
 	// Load existing manifest
@@ -546,11 +529,7 @@ func (mfa *CLIApp) loadExistingManifest(
 		existingByPath[f.GetPath()] = f
 	}
 
-	return &existingManifest{
+	return existingByPath, nil
-		entriesByPath: existingByPath,
-		signer:        string(manifest.Signer()),
-		hasCreatedAt:  manifest.CreatedAt() != nil,
-	}, nil
 }
 
 func (mfa *CLIApp) freshenManifestOperation(
@@ -558,18 +537,20 @@ func (mfa *CLIApp) freshenManifestOperation(
 ) error {
 	log.Debug("freshenManifestOperation()")
 
+	showProgress := cmd.Bool("progress")
+
 	// Find manifest file
 	manifestPath, err := mfa.resolveFreshenManifestPath(cmd)
 	if err != nil {
 		return err
 	}
 
-	builder, signKeyFingerprint, err := mfa.newFreshenBuilder(cmd)
+	builder, err := mfa.newFreshenBuilder(cmd)
 	if err != nil {
 		return err
 	}
 
-	existing, err := mfa.loadExistingManifest(cmd, manifestPath)
+	existingByPath, err := mfa.loadExistingEntries(cmd, manifestPath)
 	if err != nil {
 		return err
 	}
@@ -581,7 +562,7 @@ func (mfa *CLIApp) freshenManifestOperation(
 
 	// Phase 1: Scan filesystem
 	scanner, removed, err := mfa.freshenScan(cmd, manifestPath, absBase,
-		existing.entriesByPath)
+		existingByPath)
 	if err != nil {
 		return err
 	}
@@ -598,7 +579,7 @@ func (mfa *CLIApp) freshenManifestOperation(
 	hasher := &freshenHasher{
 		fs:             mfa.Fs,
 		absBase:        absBase,
-		showProgress:   cmd.Bool("progress"),
+		showProgress:   showProgress,
 		totalHashBytes: totalHashBytes,
 		filesToHash:    filesToHash,
 		startHash:      time.Now(),
@@ -614,14 +595,8 @@ func (mfa *CLIApp) freshenManifestOperation(
 	log.Infof("freshen complete: %d unchanged, %d changed, %d added, %d removed",
 		scanner.unchanged, scanner.changed, scanner.added, removed)
 
-	// Skip writing only when the manifest would come out the same: nothing
+	// Skip writing if nothing changed
-	// in the tree changed, and the manifest already holds what --sign-key,
+	if scanner.changed == 0 && scanner.added == 0 && removed == 0 {
-	// --include-timestamps and --include-permissions ask for. The scan
-	// counts a file whose recorded mode is not what --include-permissions
-	// asks for as changed.
-	if scanner.changed == 0 && scanner.added == 0 && removed == 0 &&
-		existing.signer == signKeyFingerprint &&
-		existing.hasCreatedAt == cmd.Bool("include-timestamps") {
 		log.Infof("manifest unchanged, skipping write")
 
 		return nil

internal/cli/freshen_test.go

@@ -84,23 +84,6 @@ func manifestFiles(t *testing.T, fs afero.Fs, path string) []*mfer.MFFilePath {
 	return manifest.Files()
 }
 
-// manifestCreatedAt returns the createdAt timestamp of the manifest at
-// path, or nil if it has none.
-func manifestCreatedAt(t *testing.T, fs afero.Fs, path string) *mfer.Timestamp {
-	t.Helper()
-
-	runMu.Lock()
-	defer runMu.Unlock()
-
-	manifest, err := mfer.NewManifestFromFile(&mfer.ManifestFromFileOptions{
-		Path: path,
-		Fs:   fs,
-	})
-	require.NoError(t, err)
-
-	return manifest.CreatedAt()
-}
-
 // TestFreshenUnchanged freshens a tree that has not changed since gen
 // made its manifest: the manifest must list the same entries as before.
 func TestFreshenUnchanged(t *testing.T) {
@@ -158,41 +141,6 @@ func TestFreshenRecordsModeOnlyWhenAsked(t *testing.T) {
 		recordedModes())
 }
 
-// TestFreshenIncludeTimestampsOnUnchangedTree runs freshen
-// --include-timestamps on a tree that has not changed since gen made its
-// manifest, which has no createdAt timestamp. freshen must rewrite the
-// manifest with one. Run again with the flag it must leave the manifest as
-// it is; run without it, it must rewrite the manifest without one.
-func TestFreshenIncludeTimestampsOnUnchangedTree(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewOsFs()
-	root, manifestPath := setupFreshenDir(t, fs, map[string]string{testFileTxt: "hello"})
-
-	freshenWithTimestamps := func() {
-		opts := testOpts([]string{
-			testApp, cmdFreshen, "-q", "--include-timestamps",
-			testFlagBase, root, manifestPath,
-		}, fs)
-		require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts))
-	}
-
-	freshenWithTimestamps()
-	assert.NotNil(t, manifestCreatedAt(t, fs, manifestPath))
-
-	before, err := afero.ReadFile(fs, manifestPath)
-	require.NoError(t, err)
-
-	freshenWithTimestamps()
-
-	after, err := afero.ReadFile(fs, manifestPath)
-	require.NoError(t, err)
-	assert.Equal(t, before, after, "manifest that has a createdAt was rewritten")
-
-	runFreshen(t, fs, root, manifestPath)
-	assert.Nil(t, manifestCreatedAt(t, fs, manifestPath))
-}
-
 // assertManifestLists asserts that the manifest at manifestPath lists
 // exactly the files in want, each with the size and SHA-256 hash of its
 // content in want and the mtime of the file of that name under root.

internal/cli/signing_test.go

@@ -127,53 +127,6 @@ func TestFreshenSignedManifestNeedsSignKey(t *testing.T) {
 	assert.Len(t, manifestFiles(t, fs, manifestPath), 2)
 }
 
-// TestFreshenSignsUnchangedTree runs freshen with --sign-key on a tree that
-// has not changed since gen made its unsigned manifest. freshen must
-// rewrite the manifest signed by that key. Run again with the same key it
-// must leave the manifest as it is; with another key it must rewrite it
-// signed by that one.
-func TestFreshenSignsUnchangedTree(t *testing.T) {
-	t.Setenv(envSignKeyPassphrase, "")
-
-	fs := afero.NewOsFs()
-	_, manifestPath := setupFreshenDir(t, fs, map[string]string{testFileTxt: "hello"})
-
-	newKeyFile := func() (string, string) {
-		secretKey, fingerprint := testSecretKey(t, nil, nil)
-		keyFile := filepath.Join(t.TempDir(), "key.asc")
-		require.NoError(t, afero.WriteFile(fs, keyFile, secretKey, 0o600))
-
-		return keyFile, fingerprint
-	}
-
-	freshenAndCheckSignedBy := func(keyFile, fingerprint string) {
-		opts := testOpts([]string{
-			testApp, cmdFreshen, "-q", testFlagSignKey, keyFile, manifestPath,
-		}, fs)
-		require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
-
-		opts = testOpts([]string{
-			testApp, cmdCheck, "-q",
-			"--" + flagRequireSignature, fingerprint, manifestPath,
-		}, fs)
-		require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
-	}
-
-	keyFile, fingerprint := newKeyFile()
-	freshenAndCheckSignedBy(keyFile, fingerprint)
-
-	before, err := afero.ReadFile(fs, manifestPath)
-	require.NoError(t, err)
-
-	freshenAndCheckSignedBy(keyFile, fingerprint)
-
-	after, err := afero.ReadFile(fs, manifestPath)
-	require.NoError(t, err)
-	assert.Equal(t, before, after, "manifest already signed by the key was rewritten")
-
-	freshenAndCheckSignedBy(newKeyFile())
-}
-
 // TestSignWithProtectedKeyNeedsPassphrase runs gen with a protected key,
 // with MFER_SIGN_KEY_PASSPHRASE empty and no terminal to ask on. gen must
 // fail, naming the variable, and write no manifest.

mfer/manifest.go

@@ -58,12 +58,6 @@ func (m *manifest) Signer() []byte {
 	return m.pbOuter.GetSigner()
 }
 
-// CreatedAt returns the createdAt timestamp of a loaded manifest, or nil
-// if the manifest has none.
-func (m *manifest) CreatedAt() *Timestamp {
-	return m.pbInner.GetCreatedAt()
-}
-
 // signatureString generates the canonical string used for signing/verification.
 // Format: MAGIC-UUID-MULTIHASH where UUID and multihash are hex-encoded.
 // Requires pbOuter to be set with Uuid and Sha256 fields.

mfer/openpgp.go

@@ -83,19 +83,6 @@ func SecretKeyIsProtected(secretKey []byte) (bool, error) {
 	return isProtected(key), nil
 }
 
-// SecretKeyFingerprint returns the fingerprint of the OpenPGP secret key
-// secretKey, armored or binary, as a manifest signed with it records its
-// signer. It fails unless secretKey holds one version 4 primary key with
-// its secret key.
-func SecretKeyFingerprint(secretKey []byte) (string, error) {
-	key, err := readSecretKey(secretKey)
-	if err != nil {
-		return "", err
-	}
-
-	return fingerprint(key), nil
-}
-
 // CheckSigningKey fails unless opts can sign now: opts.SecretKey must hold
 // one version 4 primary key with a secret key that may sign and has not
 // expired or been revoked, and opts.Passphrase must unlock it when it is

mfer/openpgp_test.go

@@ -281,21 +281,6 @@ func TestSecretKeyIsProtectedWithoutPassphrase(t *testing.T) {
 	assert.False(t, protected)
 }
 
-// TestSecretKeyFingerprint checks that SecretKeyFingerprint gives the
-// signer that a manifest signed with the key records.
-func TestSecretKeyFingerprint(t *testing.T) {
-	t.Parallel()
-
-	opts := testSigningOptions(t)
-
-	got, err := SecretKeyFingerprint(opts.SecretKey)
-	require.NoError(t, err)
-
-	m, err := NewManifestFromReader(bytes.NewReader(signedTestManifest(t, opts)))
-	require.NoError(t, err)
-	assert.Equal(t, string(m.Signer()), got)
-}
-
 // TestSigningKeyFileWithTwoKeys signs with a key file that holds two keys,
 // as gpg writes it for a user ID that two keys have. It names no one key
 // to sign with, so signing must fail.