benbjohnson / benbjohnson/litestream

GCS WriteLTXFile may finalize a partial object when the source reader fails

Open Beginner friendly
#1,263 2 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Go
Stars
14.4k
Forks
414
Avg merge
7d 1h
Merged PRs (30d)
21

Description

## Bug Description

The GCS replica client appears to finalize the object writer even when `io.Copy()` fails after writing partial data.

In `gs/replica_client.go`, `WriteLTXFile()` creates a GCS writer and immediately defers `w.Close()`:

```go
w := c.bkt.Object(key).NewWriter(ctx)
defer w.Close()

// Store timestamp in GCS metadata for accurate timestamp retrieval
w.Metadata = map[string]string{
MetadataKeyTimestamp: timestamp.Format(time.RFC3339Nano),
}

n, err := io.Copy(w, fullReader)
if err != nil {
return info, err
} else if err := w.Close(); err != nil {
return info, err
}
```

If the source reader fails after some bytes have already been written to the GCS writer, `io.Copy()` returns an error, but the deferred `w.Close()` still runs while unwinding. For the GCS storage writer, `Close()` completes and flushes the write operation. The current GCS documentation also states that an object is not visible until `Close()` is called, and that callers should cancel the writer context to stop writing without saving the data.

As a result, the current code can convert a failed source read into a normally closed GCS write, leaving a partially written object committed under the final LTX object name.

This is especially risky in the compaction path. `Compactor.Compact()` writes the compacted output through an `io.Pipe`, and the compactor goroutine reports source/compaction failures with `CloseWithError()`:

```go
pr, pw := io.Pipe()
go func() {
comp, err := ltx.NewCompactor(pw, rdrs)
if err != nil {
pw.CloseWithError(fmt.Errorf("new ltx compactor: %w", err))
return
}
comp.HeaderFlags = ltx.HeaderFlagNoChecksum
_ = pw.CloseWithError(comp.Compact(ctx))
}()

info, err := c.client.WriteLTXFile(ctx, dstLevel, minTXID, maxTXID, pr)
```

If source LTX reading or compaction fails after partial output has already been copied to the GCS writer, the current GCS implementation can return an error while still finalizing the partial object via the deferred `Close()`.

The expected behavior is that a failed upload is aborted and no completed object is created.

This issue is separate from #1262. That issue is about GCS `LTXFiles(seek)` listing behavior. This issue is about failed `WriteLTXFile()` calls leaving behind partial objects.

## Environment

**Litestream version:**

```text
v0.5.11
```

I also checked the current `main` branch:

```text
v0.5.11-3-gfc050c9
```

Current `main` commit checked:

```text
fc050c9
```

**Operating system & version:**

The production environment is Linux in Kubernetes. The source inspection was done on macOS.

**Installation method:**

Docker image in production:

```text
litestream/litestream:0.5.11
```

Source inspected from a fresh clone of `main`.

**Storage backend:**

Google Cloud Storage (GCS)

## Steps to Reproduce

This was found by source inspection. A minimal regression test should be possible against the existing GCS fake server test setup:

1. Create a valid LTX byte stream large enough for `ltx.PeekHeader()` to succeed.
2. Wrap it in a reader that returns some bytes and then returns a non-EOF error.
3. Call `gs.ReplicaClient.WriteLTXFile(ctx, level, minTXID, maxTXID, failingReader)`.
4. Assert that `WriteLTXFile()` returns an error.
5. Assert that the corresponding GCS object does not exist.

Pseudo-code for the failing reader:

```go
type errAfterReader struct {
r io.Reader
}

func (r *errAfterReader) Read(p []byte) (int, error) {
n, err := r.r.Read(p)
if n > 0 {
return n, nil
}
if errors.Is(err, io.EOF) {
return 0, errors.New("injected source failure")
}
return n, err
}
```

The important part is that `ltx.PeekHeader()` must succeed before the GCS writer is created, and the injected failure must happen during the later `io.Copy(w, fullReader)` call.

**Expected behavior:**

If `io.Copy(w, fullReader)` returns an error, `WriteLTXFile()` should abort the GCS writer and return the error. No completed object should be visible at the final LTX object key.

For example:

```go
writeCtx, cancel := context.WithCancel(ctx)
defer cancel()

w := c.bkt.Object(key).NewWriter(writeCtx)

n, err := io.Copy(w, fullReader)
if err != nil {
cancel()
return info, err
}
if err := w.Close(); err != nil {
return info, err
}
```

For the currently used `cloud.google.com/go/storage v1.36.0`, `w.CloseWithError(err)` is another possible abort path. However, the latest GCS Go client documentation marks `CloseWithError()` as deprecated and recommends canceling the context passed to `NewWriter()` instead.

The main requirement is that failed source reads do not call normal `Close()` and do not finalize the object.

**Actual behavior:**

The current code returns the `io.Copy()` error, but the deferred `w.Close()` still executes:

```go
n, err := io.Copy(w, fullReader)
if err != nil {
return info, err
}
```

Because `w.Close()` completes the GCS write operation, a partial object may be committed even though `WriteLTXFile()` returned an error.

In normal operation, a partial LTX object committed under the final object key can later be listed and opened as if it were a valid LTX file. That can cause follow-up compaction or restore operations to fail while reading or validating the LTX file.

## Configuration

litestream.yml

```yaml
dbs:
- path: /pb_data/data.db
replica:
url: gs://REDACTED_BUCKET/REDACTED_PREFIX
```

## Logs

Log output

```text
No production log has been isolated for this specific failure mode yet.

This report is based on the GCS WriteLTXFile() source path and the documented behavior
of cloud.google.com/go/storage.Writer.Close() and context cancellation.
```

## Additional Context

The project currently uses:

```text
cloud.google.com/go/storage v1.36.0
```

In that version, `Writer.Close()` is documented as completing the write operation and flushing buffered data. The same writer type also has `CloseWithError(err)`, documented as aborting the write operation.

The latest GCS Go client documentation keeps the same important write semantics:

- `ObjectHandle.NewWriter(ctx)` returns a writer for the object. The object is not available until `Close()` has been called.
- To stop writing without saving the data, the caller should cancel the context passed to `NewWriter()`.
- `Writer.Close()` completes the write operation and flushes buffered data.
- `Writer.CloseWithError(err)` aborts the write operation, but is now deprecated in favor of canceling the writer context.

References:

- https://pkg.go.dev/cloud.google.com/go/storage#ObjectHandle.NewWriter
- https://pkg.go.dev/cloud.google.com/go/storage#Writer.Close
- https://pkg.go.dev/cloud.google.com/go/storage#Writer.CloseWithError

This appears to be GCS-specific in the current codebase. The S3 replica client passes the source reader to the AWS SDK uploader:

```go
out, err := c.uploader.Upload(ctx, input)
if err != nil {
return nil, fmt.Errorf("s3: upload to %s: %w", key, err)
}
```

The AWS SDK uploader owns the upload lifecycle. For multipart uploads, its default `LeavePartsOnError` is `false`, so failed multipart uploads are aborted instead of being completed as final objects. Orphan multipart parts may still exist if abort itself fails, but that is different from committing a visible partial `.ltx` object at the final key.

Suggested scope for a fix:

1. Remove the unconditional `defer w.Close()` from `gs.ReplicaClient.WriteLTXFile()`.
2. Create a derived writer context with `context.WithCancel(ctx)` and pass it to `NewWriter()`.
3. On `io.Copy()` error, cancel the writer context and return the error without calling normal `Close()`.
4. Only call `w.Close()` on the success path.
5. Optionally use `w.CloseWithError(err)` for compatibility with the currently pinned GCS client version, but context cancellation matches the latest GCS guidance.
6. Add a GCS unit test that verifies a failing source reader does not leave a completed object behind.

Suggested test commands:

```bash
go test ./gs
go test ./...
```

This should be a small, focused GCS backend bug fix. It does not require changing the LTX file format, object naming, compaction policy, or other storage backends.

Contributor guide

Open the contributing guide

Research direction

Start in gs/replica_client.go at ReplicaClient.WriteLTXFile and inspect the existing GCS fake server tests. Run go test ./gs, then add a regression case using a reader that fails after returning data. Done means a failed copy returns an error without leaving a completed object at the final LTX key, while successful writes still close normally.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, google-cloud
Domain
backend, cloud
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
82/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.