Evan Zou

This commit provides a **bug fix** for two critical **use-after-free** issues within the **Thrift Rocket BiDi transport server-side**'s `RocketBiDiClientCallback` component. It prevents a double-call crash in `handleConnectionClose` by nulling out the `serverCallback_` pointer before making subsequent calls, avoiding a dangling pointer. More significantly, it resolves a **production SIGSEGV crash** by ensuring that `serverCallback_` is properly nulled and the stream is freed when the BiDi stream enters a terminal state after a server callback. This improves **lifetime management** and **resource freeing** for BiDi streams, preventing dangling pointers and un-freed resources.

This commit introduces a **new benchmark** to **ServiceLab** specifically for **Thrift server streams** over the Rocket transport, addressing a gap in existing performance coverage. It measures end-to-end streaming RPC performance across varying stream element counts, payload sizes, and server-side computational loads, utilizing `ScopedServerInterfaceThread` and `AsyncGenerator`. This **new capability** provides crucial performance insights for the **Thrift streaming subsystem**, complementing existing request-response and serialization benchmarks. The benchmark is configured as an **active landblock guard** for changes within `thrift/lib/cpp2`, preventing performance regressions in streaming RPCs.

This commit **fixes inconsistent error propagation** for **BiDi channels** within the **Rocket transport**, ensuring proper delivery of serialized SR error headers. By adopting a noop server callback pattern, similar to stream and sink, BiDi now correctly propagates errors like `THROTTLING_PENDING` instead of the generic `PROXY_CLIENT_APP`. This **refactoring** introduces `BiDiServerCallbackNoop` and updates the BiDi error path to use `onFirstResponse`. Furthermore, a **bug fix** in `RocketBiDiClientCallback::onFirstResponse` resolves a null pointer dereference during early client cancellations, significantly improving the **robustness of BiDi error handling**.

This commit **enhances error handling** within the **ThriftServer module** by introducing a new exception type, `DuplicateModuleNameError`. This **new capability** allows callers of `ThriftServer::addModule` to specifically catch errors when attempting to add a module with a name that already exists, rather than a generic `std::invalid_argument`. The change improves the clarity of **module registration error reporting**, enabling more precise handling of duplicate module name detection. Existing code that catches `std::invalid_argument` will continue to function correctly due to `DuplicateModuleNameError` inheriting from it, ensuring backward compatibility.

This commit **fixes a bug** in **Thrift server-side metrics reporting** for **stream and sink methods**. Previously, `HandlerCallbackBase` incorrectly used unqualified method names when calling `setMethodName` on `ServerStreamFactory` and `ServerSinkFactory`, leading to **FB303 counters** being emitted with incorrect keys. By switching to `methodNameInfo_.qualifiedMethodName`, this change ensures that **metrics are reported correctly** with qualified method names, making them visible and usable in tests and dashboards. Additionally, a killswitch test is updated to clarify that the `disable_streaming_metrics` flag only affects the old metric event handler path.

This commit introduces **byte-level metrics** to **Thrift stream and sink Scuba logging**, providing `totalBytes`, `minChunkSize`, and `maxChunkSize` for each operation. It enhances the `IThriftRequestLogging` interface and modifies `ThriftStreamLog` and `ThriftSinkLog` to accumulate these statistics from individual `StreamNextEvent` and `SinkNextEvent` payloads, with payload sizes captured in `ServerGeneratorStreamBridge` and `ServerSinkBridge`. This **new capability** allows downstream consumers to gain deeper insights into the payload characteristics and data transfer patterns of Thrift stream and sink operations, improving **observability**. The changes are confined to Scuba logging and do not affect `IThriftServerCounters` or `Fb303ThriftServerCounters`.

This commit performs a significant **refactoring** of the **Rocket transport server-side connection handling** by replacing `std::variant` based dispatch with **virtual dispatch** through the `IConnectionStreamHandler` interface. This change primarily affects `RocketServerConnection` and `RefactoredRocketServerConnection`, streamlining how different stream types (Stream, Sink, BiDi) process frames and connection events like `handleFrame()`, `handleConnectionClose()`, and `handleStreamHeadersPush()`. The refactoring removes approximately 540 lines of dead code and eliminates the shared `bufferedFragments_` map, improving code clarity and maintainability within the **Thrift Rocket server implementation**. This simplifies future extensions and reduces complexity in the core connection logic.

This commit introduces a **stream credit starvation timeout** mechanism to the `RocketBiDiClientCallback`, mirroring existing protection in `RocketStreamClientCallback`. This **new capability** prevents hangs and resource exhaustion in **bidirectional Thrift streams** by tearing down the stream if the client fails to send `REQUEST_N` credits after the server exhausts its tokens. The change involves tracking `streamTokens_`, scheduling timeouts when tokens reach zero, and cancelling both stream and sink halves with a `CREDIT_TIMEOUT` error upon expiry, significantly improving **server stability** and **resource management** for the **Rocket transport** layer. New **unit and end-to-end tests** verify this crucial protection.

This commit introduces a **sink chunk timeout mechanism** to the **Thrift C++2 Rocket transport's bi-directional streams**, specifically within `RocketBiDiClientCallback`. This **new capability** prevents server resources from being held indefinitely by clients that stop sending data after receiving credits, mirroring existing protection for sink-only streams. It involves adding a `chunkTimeout` field to the user-facing `StreamTransformation` API, threading it through the server-side stream setup via `ServerBiDiStreamFactory`, and implementing a `SinkTimeoutCallback` within `RocketBiDiClientCallback` to manage the timeout lifecycle. This enhancement significantly **improves server stability** and **resource management** by automatically tearing down unresponsive bi-directional streams and notifying the client of the `CHUNK_TIMEOUT` error.

This commit implements a **bug fix** within the **Thrift Rocket transport server** by correcting the return value of `RocketBiDiClientCallback::onStreamRequestN`. Previously, this method was erroneously negating the result of the underlying `serverCallback_->onStreamRequestN(n)`, which inverted the meaning of the stream callback contract (where `true` indicates an active stream). This **bug fix** ensures that `onStreamRequestN` now accurately reflects the stream's liveness status, preventing potential future incorrect behavior if other parts of the system were to rely on its return value.

This commit introduces a **new capability** to the **Thrift logging subsystem** by adding `ThriftSinkLog` and `ThriftConnectionLog`. `ThriftSinkLog` provides detailed server-side logging for sink operations, tracking metrics such as chunk receive/consume counts, receive intervals, consume latency, and approximating client pause duration via credit exhaustion detection. `ThriftConnectionLog` acts as a per-connection factory, creating correctly-wired `ThriftStreamLog` and `ThriftSinkLog` instances with injected backends. This enhancement significantly improves the observability and monitoring of **Thrift server-side sink operations**, enabling better performance analysis and debugging. Unit tests are included for both new classes, verifying their functionality and integration.

This commit introduces the **new capability** of `ThriftStreamLog` within the **Thrift C++2 logging subsystem**, a concrete class designed to accumulate state for a single **server-side stream**. It tracks critical stream lifecycle events, credit management, pause/resume states, and timing metrics like chunk generation and send delays. The class dispatches real-time metrics to `IThriftServerCounters` and performs structured logging via `IThriftRequestLogging` upon stream completion. Comprehensive **unit tests** are included to validate state accumulation, counter dispatch, and terminal event handling, significantly enhancing observability for Thrift streams.

This commit performs an **atomic switchover** of the metric backend for **Thrift stream and sink RPCs**, ensuring no double-counting or missing metrics during the transition. It **installs `Fb303ThriftServerCounters`** in `ThriftStatsModuleLight` and **removes `StreamMetricEventHandler`**, establishing a unified logging path for fb303 stats. The `StreamMetricHandler` is also updated to focus solely on interaction tracking and BiDi metrics, no longer allocating `StreamState` for stream/sink RPCs. This **refactoring** guarantees that stream/sink counter emission now flows exclusively through `lifecycle events → ThriftStreamLog/ThriftSinkLog → IThriftServerCounters → Fb303ThriftServerCounters → fb303 stats`. Additionally, the **Rust Thrift compiler and runtime** are updated to support tracking streaming metrics by setting the stream method name in generated server code and adding a `set_stream_method_name` method to the `ReplyState` trait.

This commit **integrates a unified logging infrastructure** into the **Thrift server stack**, establishing the foundational components for comprehensive server-side metrics and request logging. It introduces new virtual methods like `getThriftServerCounters` and `getThriftRequestLogging` to `ServerConfigs`, adds corresponding members and override getters in `ThriftServer`, and setters in `ThriftServerInternals` for module installation. Furthermore, a `ThriftConnectionLog` is now stored on `Cpp2ConnContext` and initialized within the `ThriftRocketServerHandler` constructor. This **new capability** wires the logging path throughout the server, though it currently utilizes no-op backends, meaning no metrics are emitted yet and there is no overlap with existing `StreamMetricEventHandler` paths.

This commit **integrates the unified logging layer** for **Thrift streams** and **Thrift sinks** directly into their runtime lifecycles. It wires `ThriftStreamLog` and `ThriftSinkLog` instances into core components like `ServerGeneratorStreamBridge`, `ServerSinkBridge`, `ServerStreamFactory`, `ServerSinkFactory`, and `RocketThriftRequests`, ensuring that various **stream and sink events** (e.g., subscribe, next, complete, error, cancel) are logged. This **new capability** significantly enhances **observability** for asynchronous Thrift operations by providing detailed, event-driven logging. Although the default logging backend is currently a no-op, this change establishes the necessary infrastructure for future detailed monitoring of stream and sink activity without immediate performance overhead or fb303 stats.

This commit introduces a **unified metric logging infrastructure** for **Thrift C++ streams and sinks**, providing a cleaner, typed API for event handling. It adds three core components: `ThriftEvent` for typed lifecycle events, `IThriftServerCounters` for real-time counter updates, and `IThriftRequestLogging` for structured logging at request completion. This **new capability** replaces fragmented `StreamEventHandler` dispatch paths, enabling more robust and standardized metric collection and structured logging for Thrift services. Additionally, it includes minor **refactoring** to update stream pause event handling across various `thrift/lib/cpp` and `thrift/lib/cpp2/async` components to use a new `StreamPauseReason` type.

This commit introduces a **new configuration capability** for **Thrift C++2 bi-directional streams**, allowing users to customize the **sink-direction initial credits** (client-to-server buffer size). A `bufferSize` field is added to the `StreamTransformation` struct, providing a configurable option that defaults to 100. The `ServerBiDiStreamFactory` is updated to read this value from the transformation, replacing a previously hardcoded credit limit. This enhancement provides greater control over **bi-directional stream flow control** and resource management. An end-to-end test has been added to verify the functionality of custom sink buffer size configuration.

This commit provides a **bug fix** for **BiDi test utility classes** (`BiDiConfigurableServer`, `BiDiEchoServer`, `BiDiFiniteClient`) that were causing tests to hang indefinitely in optimized builds. Previously, callback invocations with side effects were incorrectly wrapped inside `DCHECK()` macros, which are stripped in opt builds, preventing these callbacks from executing. The fix involves extracting these critical callback calls, such as `clientCallback_->onStreamComplete()`, outside of `DCHECK` to ensure they always execute, while keeping the assertion on their return value within the macro. This change prevents **test hangs** and ensures reliable execution of BiDi functionality tests across all build configurations.

This commit introduces **new end-to-end tests** for **Thrift's oneway RPC services** within the `thrift/lib/cpp2` library. These tests validate core oneway semantics, including basic fire-and-forget delivery, proper handling of server-side exceptions without client propagation, and concurrent invocation scenarios. By adding `OnewayServiceE2ETest.cpp` and defining `TestOnewayService.thrift`, this work significantly enhances the **testing coverage and reliability** of the **Thrift C++2 oneway RPC implementation**, ensuring its behavior aligns with expectations.

This commit establishes a new **test infrastructure** directory, `fbcode/thrift/lib/cpp2/test/e2e/`, to house a consistent **end-to-end test suite** for all five **Thrift RPC types**. It introduces the `E2ETestFixture` class, providing shared utilities such as `testConfig()` for spinning up in-process servers and `makeClient()` for creating typed clients over `RocketClientChannel`. This **new capability** standardizes and simplifies the development of modern E2E tests across request-response, oneway, stream, sink, and bidi RPC patterns within the **Thrift C++2 library**. The fixture ensures a uniform approach to testing, leveraging patterns from existing modern E2E tests.