Chris Carlon

This commit **fixes a persistent failure** in the `TestIntegration_WriterAppendEdgeCases` function, part of the **`storage` module's integration tests**. It **removes a skip directive** and **refines error handling and flush calls** within the test to accurately validate edge cases for writer appends. This **test fix** improves the reliability of the test suite, ensuring the `storage` component's write operations are thoroughly and correctly verified.

This commit **fixes a bug** in the **`storage`** module, specifically within the `grpc_writer.go` component, to correctly handle **redirection errors** during a **takeover connection**. Previously, the `connect` function only checked for redirects on successful stream responses, leaving error responses unhandled. The update ensures that the `connect` function now properly detects and manages redirection errors received from the stream, completing a prior fix that only addressed successful responses. This change significantly improves the **robustness and reliability** of storage operations by preventing connection failures in scenarios involving server-side redirects during a takeover.

This **chore** refactors the **storage** module's **retry conformance tests** to simplify the setup for appendable upload operations. Specifically, the tests for `insert` operations on appendable uploads in `storage/internal/test/conformance/retry_tests.json` now utilize a pre-existing bucket, rather than creating and deleting one for each test. This **maintenance** change streamlines the test environment by reusing `fs.bucket.Name`, making the test suite cleaner and potentially more efficient by reducing redundant bucket management. The primary impact is an improved and more robust **test infrastructure** for the storage client.

This commit delivers a **bug fix** for the **storage** subsystem, specifically addressing incorrect handling of **takeover responses** within the `gRPCAppendTakeoverBidiWriteBufferSender`. Previously, retries after a takeover might not present the necessary write handle, leading to attempts at new takeovers and errors due to repeated offsets. The fix ensures that the client correctly processes takeover responses, utilizing the provided write handle to allow the server to identify replayed bytes and avoid errors during retries. This significantly improves the **robustness of write operations** during storage takeovers, with a new conformance test added to verify the correct behavior of appendable uploads.

This commit introduces a **new feature** to the **`storage` module**, specifically implementing **takeover idempotence** for certain write operations. It **enhances the idempotence check** within the `openWriter` function to safely accommodate **append operations** that involve taking over a specific object generation. This crucial improvement ensures that these particular "takeover" operations can be **retried safely**, preventing potential data inconsistencies and significantly boosting the reliability of the storage system.

This commit implements a **bug fix** and **refactoring** to ensure the **`storage`** client's **`Writer`** component is fully **thread-safe**. It addresses a regression where concurrent calls to methods like `Write()`, `Flush()`, `Close()`, and `CloseWithError()` were not properly handled, and now supports repeated `Close()` operations. The `gRPCWriter` implementation was modified to start its sender goroutine earlier and utilize a command channel for operations. This change resolves critical concurrency issues, though it might introduce a minor performance overhead for workloads with many small writes.

This commit **fixes** an issue in the **Cloud Storage `storage` module** where **one-shot writes** were inadvertently reporting progress, contrary to their intended behavior. It **disables progress reporting** for these specific writes by setting the progress function to a no-op within `storage/grpc_writer.go` when `forceOneShot` is enabled, thereby restoring the original design. To prevent future regressions, a new **emulator test** has been added to `storage/client_test.go` to explicitly verify that one-shot writes do not report progress. This ensures that users performing one-shot uploads will no longer receive unexpected progress updates.

This commit introduces a **substantial refactoring** of the **`storage`** package's **gRPC writer** to pipeline data more efficiently. It modifies the `grpc_writer.go` implementation to send additional data while previous chunks are flushing, leveraging the `io.Writer` contract to refer to caller-provided byte slices in place rather than copying them. This **performance improvement** can lead to a significant **latency reduction** (e.g., ~35% for large uploads to remote regions) by optimizing how data is buffered and transmitted. The change also lays the groundwork for future optimizations, such as adaptive flushing strategies based on write slice sizes, further enhancing the **`storage`** subsystem's throughput.

This commit introduces a **new test** within the **storage client** to verify a critical behavior of the `Writer` component. Specifically, the `TestWriterChunkBufferReuseEmulated` test in `storage/client_test.go` ensures that `storage.Writer` implementations **do not retain user-provided buffers** after the `Write()` method returns. This **quality assurance** measure prevents potential issues like data corruption or unexpected side effects by confirming proper buffer lifecycle management within the **storage** package. The **test addition** enhances the robustness of the `storage` module by explicitly validating this important contract.

This commit **enhances testing** for the **storage client** by adding assertions to verify that a writer's `Attrs().Size` is accurately updated following a `flush` operation. Specifically, it modifies `storage/client_test.go` to ensure that the reported size of written data remains consistent after data is committed. This **test enhancement** confirms the correctness of size tracking within the storage subsystem, which is critical for reliable data management and accurate resource accounting. The work ensures that the `storage` module's writer attributes reflect the true state of written data post-flush.

This commit **updates the documentation** for the `storage` module, specifically enhancing `storage/TESTING.md` with detailed instructions for running **live service integration tests**. It outlines the prerequisites and the exact commands needed to execute these tests, improving the developer experience. This **maintenance task** makes it significantly easier for contributors to set up and run integration tests for the `storage` component, even though the instructions are noted as not yet fully complete.

This commit **fixes a segfault** occurring in the **`storage` module's integration tests** when the `JWT_PRIVATE_KEY_FILE` environment variable is unset. Previously, this omission caused a nil dereference during test execution, leading to crashes. The **bug fix** modifies `storage/integration_test.go` to now use `t.Fatal` instead of `t.Skip` if the required JWT private key file is missing, ensuring that tests fail immediately and explicitly rather than crashing or silently skipping. This **enhances the reliability and robustness** of the `storage` subsystem's test suite.

This commit introduces a **significant refactoring** within the **`storage` module** to abstract the underlying `io.PipeWriter` implementation. It defines a new `internalWriter` interface, which is now used by the `OpenWriter` methods in `storage/client.go`, `storage/grpc_writer.go`, and `storage/http_client.go`. Concrete implementations like `gRPCInternalWriter` and `httpInternalWriter` encapsulate the `io.PipeWriter` and its flush logic, effectively **hiding implementation details** from the public `Writer` interface. This **pure refactoring** enhances the **flexibility and maintainability** of the storage client's writing capabilities, paving the way for future performance improvements, such as reducing data copies, without altering the public API.

This commit **adds comprehensive testing instructions** for the **`storage` package**, introducing a new `TESTING.md` file. This **documentation update** details how to run both **unit and emulated integration tests**, significantly improving developer onboarding and understanding of the testing workflow for this critical component. As a **maintenance chore**, it clarifies the testing process for the `storage` subsystem and includes a placeholder for future documentation on live integration tests.

This commit introduces a **bug fix** to the **storage** module, specifically addressing an **inconsistency** in the **gRPC writer's** handling of **object generation takeover**. Previously, when a user specified `Generation(0)` for an appendable object writer, the system would open the writer but fail to perform the intended takeover internally, which was only triggered for generations greater than zero. The conditional logic in `storage/grpc_writer.go` has been corrected within functions like `OpenWriter` and `newGRPCWriter` to properly include `Generation(0)` in takeover operations. This ensures that **appendable object storage operations** now behave consistently and reliably when targeting the current object generation.

This commit **re-adds and enhances retry tests** for **appendable uploads** within the **storage client library**. It introduces new test cases in `storage/internal/test/conformance/retry_tests.json` to cover scenarios involving **redirect tokens** and significantly **increases the size of objects** used in existing append retry tests. The `storage/retry_conformance_test.go` file was updated to dynamically generate larger content and adjust chunk sizes, ensuring more comprehensive testing of the **storage client's retry mechanism** and **message flows** for large append operations. This **maintenance chore** improves the robustness and coverage of the `storage` module's retry logic.

This commit **optimizes test performance** within the **storage** module's **retry conformance tests**. It replaces inefficient `cmp.Diff` comparisons of large byte slices with faster MD5 hash comparisons in `storage/retry_conformance_test.go`. This **test optimization** specifically targets scenarios where the content is random and a full byte-by-byte comparison is unnecessary, significantly speeding up test execution. The change improves the efficiency of the testing suite without altering any production code behavior.

This commit **fixes a critical bug** in the **`storage` module's gRPC client** by correctly handling **redirection errors** during operations. Previously, inline retries for these errors could lead to resending unstable buffers; now, redirects are surfaced to the stream-level retry machinery, ensuring proper resend logic. This involved **refactoring** retry invocation in `storage/grpc_client.go` and `storage/grpc_reader.go`, and introducing specific retry logic for redirection errors within `storage/grpc_writer.go`'s `OpenWriter` method. A new `runShouldRetry` helper was also added to `storage/invoke.go` to **centralize retry decision logic**, significantly improving the **robustness and reliability** of storage operations by preventing data corruption and ensuring proper error recovery.

This commit **fixes an issue in the `storage` client tests** by enhancing their error handling for deadline expirations. It **modifies test assertions in `storage/client_test.go`** to correctly recognize `gRPC deadline exceeded errors` alongside existing `context deadline exceeded errors`. This **maintenance update** ensures that tests checking for `context.DeadlineExceeded` are robust against gRPC's internal deadline mechanisms, preventing potential false failures. The change improves the **reliability of the test suite** for the `storage` subsystem.

This commit **fixes a bug** within the **storage subsystem** that affected **resumable uploads**. Previously, when a resumable upload retried and data was trimmed from the `sendBuffer` because it had already been sent, the internal offset was not correctly updated. This correction ensures the offset is accurately adjusted after trimming, preventing **incorrect state** and improving the reliability of **resumable upload retries** within `storage/grpc_writer.go`.