chore(model): slightly improve handling of pulling empty blocks (#10679)

In the common case (sparse files enabled, not reusing old data) we'd
optimise away pulling & writing zero blocks. However in the corner cases
we'd go through the whole processing of pulling the block over the
network which is of course entirely unnecessary.

Now, instead, always take an optimised path for all-zeroes blocks. In
the clean case we do nothing, otherwise we materialise a block of zeroes
and write it directly.

---------

Signed-off-by: Jakob Borg <jakob@kastelo.net>

.golangci.yml

@@ -76,6 +76,9 @@ linters:
       # Don't necessarily rewrite !(foo || bar) to !foo && !bar
       - linters: [staticcheck]
         text: QF1001
+      # Don't necessarily pass the context just for the sake of the logger
+      - linters: [contextcheck]
+        text: "->log`"
   settings:
     sloglint:
       context: "scope"

lib/model/folder_sendrecv.go

@@ -1558,10 +1558,21 @@ func (f *sendReceiveFolder) pullBlock(ctx context.Context, state pullBlockState,
 		return
 	}
 
-	if !f.DisableSparseFiles && state.reused == 0 && state.block.IsEmpty() {
+	if state.block.IsEmpty() {
 		// There is no need to request a block of all zeroes. Pretend we
 		// requested it and handled it correctly.
-		state.pullDone(state.block)
+		if state.reused != 0 || f.DisableSparseFiles {
+			// We are reusing a file (contents apparently weren't all-zeroes
+			// previously), or sparse files are disabled, so we need to
+			// actually write the block.
+			zeroes := make([]byte, state.block.Size)
+			err = f.limitedWriteAt(ctx, fd, zeroes, state.block.Offset)
+		}
+		if err != nil {
+			state.fail(fmt.Errorf("save: %w", err))
+		} else {
+			state.pullDone(state.block)
+		}
 		out <- state.sharedPullerState
 		return
 	}
@@ -1607,11 +1618,10 @@ loop:
 		}
 
 		// Verify that the received block matches the desired hash, if not
-		// try pulling it from another device.
+		// try pulling it from another device. For receive-encrypted
-		// For receive-only folders, the hash is not SHA256 as it's an
+		// folders, the hash is not SHA256 as it's an encrypted hash token.
-		// encrypted hash token. In that case we can't verify the block
+		// In that case we can't verify the block integrity so we'll take it
-		// integrity so we'll take it on trust. (The other side can and
+		// on trust. (The other side can and will verify.)
-		// will verify.)
 		if f.Type != config.FolderTypeReceiveEncrypted {
 			lastError = f.verifyBuffer(buf, state.block)
 		}

lib/model/folder_sendrecv_test.go

@@ -917,6 +917,88 @@ func TestPullCtxCancel(t *testing.T) {
 	}
 }
 
+// TestPullEmptyBlock exercises pullBlock for blocks of all zeroes.
+// pullBlock distinguishes three cases:
+//
+//   - Sparse files enabled, reused == 0: nothing to write, the offset
+//     reads as zero anyway because the temp file is sparse-allocated.
+//   - reused != 0: the temp file is being reused and may already hold
+//     non-zero data at this offset, so zeroes must be written explicitly.
+//   - DisableSparseFiles: cannot rely on a sparse hole reading as zero,
+//     so zeroes must be written explicitly.
+func TestPullEmptyBlock(t *testing.T) {
+	const blockSize = protocol.MinBlockSize
+	emptyBlock := protocol.BlockInfo{
+		Offset: 0,
+		Size:   blockSize,
+		Hash:   blocks[0].Hash, // sha256 of a 128 KiB all-zeroes block
+	}
+	if !emptyBlock.IsEmpty() {
+		t.Fatal("test setup: block not recognised as empty")
+	}
+
+	cases := []struct {
+		name            string
+		reused          []int
+		disableSparse   bool
+		preFillTempFile bool
+	}{
+		{name: "sparse-clean"},
+		{name: "reused-temp-file", reused: []int{1}, preFillTempFile: true},
+		{name: "disable-sparse-files", disableSparse: true},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			_, f := setupSendReceiveFolder(t)
+			f.folder.FolderConfiguration.DisableSparseFiles = tc.disableSparse
+
+			file := protocol.FileInfo{
+				Name:        "file",
+				Size:        int64(blockSize),
+				Permissions: 0o644,
+				Blocks:      []protocol.BlockInfo{emptyBlock},
+			}
+			tempName := fs.TempName(file.Name)
+
+			// Pre-fill the temp file with non-zero data so that a missing
+			// overwrite would be detectable.
+			if tc.preFillTempFile {
+				writeFile(t, f.Filesystem(), tempName, bytes.Repeat([]byte{0xff}, blockSize))
+			}
+
+			state := pullBlockState{
+				sharedPullerState: newSharedPullerState(file, f.Filesystem(), f.folderID, tempName, file.Blocks, tc.reused, false, false, protocol.FileInfo{}, !tc.disableSparse, false),
+				block:             emptyBlock,
+			}
+
+			out := make(chan *sharedPullerState, 1)
+			f.pullBlock(t.Context(), state, out)
+
+			s := <-out
+			if err := s.failed(); err != nil {
+				t.Fatalf("pullBlock failed: %v", err)
+			}
+			cleanupSharedPullerState(s)
+
+			fd, err := f.Filesystem().Open(tempName)
+			must(t, err)
+			defer fd.Close()
+			data, err := io.ReadAll(fd)
+			must(t, err)
+
+			if len(data) != blockSize {
+				t.Fatalf("temp file too short: got %d bytes, want at least %d", len(data), emptyBlock.Offset+int64(blockSize))
+			}
+			for i, b := range data {
+				if b != 0 {
+					t.Fatalf("byte %d: got %#x, want 0", i, b)
+				}
+			}
+		})
+	}
+}
+
 func TestPullDeleteUnscannedDir(t *testing.T) {
 	_, f := setupSendReceiveFolder(t)
 	ffs := f.Filesystem()