Allow multiplexed upstream servers to half close the stream before the downstream #34461

yanavlasov · 2024-05-31T17:43:02Z

Commit Message:
Allow HTTP/2 (and HTTP/3) upstream servers to half close the stream before the downstream. This enables bidirectional gRPC streams where server completes streaming before the client. Behavior of HTTP/1 or TCP proxy upstream servers is unchanged and the stream is reset if the upstream server completes response before the downstream. The stream is also reset if the upstream server responds with an error status before the downstream. This behavior is disabled by default and can be enabled by setting the envoy.reloadable_features.allow_multiplexed_upstream_half_close runtime key to true.

Change details:
Presently there are two places where the stream was reset when upstream half-closed.

In the router filter's Filter::onUpstreamComplete method, which covers HTTP upstreams.
In the filter manager's FilterManager::commonEncodePrefix which covers local reply's by filters and TCP upstreams.

When the envoy.reloadable_features.allow_multiplexed_upstream_half_close is enabled the router filter no longer forces reset in the Filter::onUpstreamComplete and allows fully independent half closes. To preserve existing half close behavior of HTTP/1 protocol the force reset is added in the H/1 codec's ClientConnectionImpl::onMessageCompleteBase() method.

When the envoy.reloadable_features.allow_multiplexed_upstream_half_close is enabled the filter manager closes stream after both decoder and encoder filter chain complete, except in two cases:

local reply - behaves the same.
error (non 1xx or 2xx) response from the server. This case is handled in the FilterManager::checkAndCloseStreamIfFullyClosed() method.

The state_.decoder_filter_chain_complete_ is added to track completion of the decoder filter chain.

To preserver behavior for TCP upstream the force reset is moved into the TcpUpstream::onUpstreamData method.

Risk Level: High (flag protected, disabled by default)
Testing: Unit Tests
Docs Changes: No
Release Notes: Yes
Platform Specific Features: N/A
Runtime guard: envoy.reloadable_features.allow_multiplexed_upstream_half_close
Fixes #30149

…e downstream. Signed-off-by: Yan Avlasov <[email protected]>

repokitteh-read-only · 2024-05-31T17:43:06Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #34461 was opened by yanavlasov.

see: more, trace.

repokitteh-read-only · 2024-05-31T17:43:09Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #34461 was opened by yanavlasov.

see: more, trace.

alyssawilk

great work! Complicated PR so a bunch of comments follow
/wait

alyssawilk · 2024-06-04T17:25:38Z

source/common/http/async_client_impl.cc

-  // being interpreted as a failure, when in fact no error has necessarily occurred. Gracefully
-  // closing seems most in-line with behavior elsewhere in Envoy for now.
+  // At present, the AsyncStream is always fully closed when the server half closes the stream.
+  // This is the case even when allow_multiplexed_upstream_half_close runtime flag is set, as there


suggestion: remove flag name so we don't have to remember to update the comment when we remove the flag.

alyssawilk · 2024-06-04T17:26:12Z

source/common/http/conn_manager_impl.h

@@ -283,7 +283,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
    OptRef<const Tracing::Config> tracingConfig() const override;
    const ScopeTrackedObject& scope() override;
    OptRef<DownstreamStreamFilterCallbacks> downstreamCallbacks() override { return *this; }
-    bool isHalfCloseEnabled() override { return false; }
+    bool isHalfCloseEnabled() override {
+      return filter_manager_.allowUpstreamHalfClose() ? true : false;


return filter_manager_.allowUpstreamHalfClose()?

alyssawilk · 2024-06-04T17:28:14Z

source/common/http/filter_manager.h

+    bool stream_closed_ : 1; // Set when both remote_decode_complete_ and remote_encode_complete_ is
+                             // true observed for the first time and prevents ending the stream
+                             // multiple times. Only set when allow_upstream_half_close is enabled.
+    bool force_close_stream_ : 1; // Set to indicate that stream should be closed due to either


should we stick with half close naming for consistency? support_half_close?

if not consider renaming to something like should_force_close

Renamed to should_force_close_stream_

alyssawilk · 2024-06-04T17:29:17Z

source/common/http/filter_manager.cc

+  ENVOY_STREAM_LOG(trace,
+                   "commonEncodePrefix end_stream: {}, isHalfCloseEnabled: {}, force_close: {}",
+                   *this, end_stream, filter_manager_callbacks_.isHalfCloseEnabled(),
+                   static_cast<bool>(state_.force_close_stream_));


add to dumpstate instead?

I don't have a good understanding of dumpstate but just quickly read #7300. if the dumpstate need to be invoked by human or at crash automatically, I hope we keep this log for stream level debugging (while agreed that this information should be added on the other place too, if needed). It will be super helpful to users who debug streams with trace log, why their stream got reset or why not.

Why not both? Added to both places.

alyssawilk · 2024-06-04T17:31:48Z

source/common/http/filter_manager.cc

-      state_.local_complete_ = true;
+    if (allow_upstream_half_close_) {
+      if (end_stream) {
+        state_.encoder_end_stream_ = true;


how is this different from remote_decode_complete? it's set when we see end stream. I think it's just badly named.

can we explicitly comment how this differs from local_complete as well?

It is a bit messy. I have added more comments to local_complete and encoder_end_stream and left a TODO to always use encoder_end_stream for tracking end_stream state in encoder filer iteration.

I will do this refactor when the runtime flag is removed.

alyssawilk · 2024-06-04T17:34:13Z

source/common/http/filter_manager.cc

@@ -1489,6 +1511,27 @@ void FilterManager::maybeEndEncode(bool end_stream) {
  if (end_stream) {
    ASSERT(!state_.remote_encode_complete_);
    state_.remote_encode_complete_ = true;
+    if (allow_upstream_half_close_) {
+      maybeCloseStream();


should this be half close? closeUpstream?

should this logic all be in maybeClose?

It closes downstream and upstream streams. I have renamed it checkAndCloseStreamIfFullyClosed to hopefully make it clearer.

alyssawilk · 2024-06-04T17:39:56Z

source/common/http/filter_manager.cc

+  // end stream is observed on the encode path in case upstream half closes before the
+  // downstream.
+  return parent_.allow_upstream_half_close_ ? parent_.state_.encoder_end_stream_
+                                            : parent_.state_.local_complete_;


I think we may need a rename / comment PR before we land this because I think this would be more clear if local_complete were more clear in the first place.

or after? (when we consider the urgency of this issue)

I have updated comments for local_complete_ and encoder_end_stream_ flags

alyssawilk · 2024-06-04T17:41:00Z

source/common/http/filter_manager.h

@@ -748,6 +750,9 @@ class FilterManager : public ScopeTrackedObject,
  void decodeHeaders(RequestHeaderMap& headers, bool end_stream) {
    state_.remote_decode_complete_ = end_stream;
    decodeHeaders(nullptr, headers, end_stream);
+    if (allow_upstream_half_close_) {


can this be cone in maybeendDecode so we don't have to do it in all headers/body/data cases? if maybeEndStream did checks on allow_upstream_half_close_ I think it'd simplify things?

Moved the flag check to checkAndCloseStreamIfFullyClosed

alyssawilk · 2024-06-04T17:47:45Z

source/common/http/filter_manager.cc

@@ -960,6 +972,8 @@ void DownstreamFilterManager::sendLocalReply(
  ASSERT(!state_.under_on_local_reply_);
  const bool is_head_request = state_.is_head_request_;
  const bool is_grpc_request = state_.is_grpc_request_;
+  // Local reply closes the stream even if downstream is not half closed.
+  state_.force_close_stream_ = true;


instead of setting a boolean here (which adds to the booleans we need to keep track of in all places, WDYT of adding a stopSending function which we call both on local reply and on receipt of error response, which stops downstream processing?

I think that'd then be reuseable for the "stream reset preceding data" bug as we could convert no_error stream resets into stop sending calls.

Makes sense. I have reworked this a bit and added the stopDecoding method.

alyssawilk · 2024-06-04T17:50:26Z

test/common/http/conn_manager_impl_test_2.cc

@@ -520,7 +571,7 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionDuration) {
  EXPECT_EQ(1U, stats_.named_.downstream_cx_max_duration_reached_.value());
 }

-TEST_F(HttpConnectionManagerImplTest, IntermediateBufferingEarlyResponse) {
+TEST_F(HttpConnectionManagerImplTest, DISABLED_IntermediateBufferingEarlyResponse) {


source/common/http/http1/codec_impl.cc

source/common/router/router.cc

envoy/http/stream_reset_handler.h

source/common/http/filter_manager.h

source/extensions/upstreams/http/tcp/upstream_request.cc

test/integration/protocol_integration_test.cc

KBaichoo · 2024-06-15T11:44:43Z

test/integration/protocol_integration_test.cc

+// H/1 upstream always causes the stream to be closed if it responds before downstream.
+// This test also causes downstream connection to run out of stream window (in H/2 and H/3 case)
+// when processing END_STREAM from the server to make sure the data is not lost in the case.
+TEST_P(ProtocolIntegrationTest, ServerHalfCloseBeforeClientWithBufferedResponseData) {


might be useful to dump the upstream timing and/or warn about it in release notes as I think before if we'd get an early response onLastUpstreamTxByte might not be set.

While now it will be and it might be > onLastUpstreamRxByteReceived which might cause some manipulations of timing data that assumed last byte tx to upstream < last upstream rx byte received to become negative

I've added test for duration values. %COMMON_DURATION% may now produce negative values in more cases but I'm not too concerned about it, since independent half close support is basically a new feature. It is useful for bidi streaming in gRPC and I think people will make sure to set monitoring correctly.

since independent half close support is basically a new feature. It is useful for bidi streaming in gRPC and I think people will make sure to set monitoring correctly.

It seems like our plan is to eventually enable it by default. In which case warning or noting it in either release notes or the docs for %COMMON_DURATION% will help less sophisticated users understand when this can be negative.

Signed-off-by: Yan Avlasov <[email protected]>

yanavlasov · 2024-06-24T22:32:22Z

Sanitizer failures look related to #34354

KBaichoo

Thank you for fixing this complicated edge case.

KBaichoo · 2024-06-26T14:12:34Z

test/integration/protocol_integration_test.cc

+// H/1 upstream always causes the stream to be closed if it responds before downstream.
+// This test also causes downstream connection to run out of stream window (in H/2 and H/3 case)
+// when processing END_STREAM from the server to make sure the data is not lost in the case.
+TEST_P(ProtocolIntegrationTest, ServerHalfCloseBeforeClientWithBufferedResponseData) {


since independent half close support is basically a new feature. It is useful for bidi streaming in gRPC and I think people will make sure to set monitoring correctly.

It seems like our plan is to eventually enable it by default. In which case warning or noting it in either release notes or the docs for %COMMON_DURATION% will help less sophisticated users understand when this can be negative.

source/common/router/router.cc

envoy/http/stream_reset_handler.h

alyssawilk

sorry to nitpick here but I'm really concerned about the number of extra bools and consistency problems. comments inline
/wait

alyssawilk · 2024-06-27T18:48:02Z

source/common/http/filter_manager.h

+                                  // end_stream flag when resuming encoder filter chain iteration.
+                                  // It is only used when the
+                                  // `allow_multiplexed_upstream_half_close` runtime flag is true.
+    bool local_complete_ : 1; // This indicates that request is complete prior to filter processing.


I don't want to nitpick but is this about request completion?
looks like ignoring half close, local_complete_ is set from setLocalComplete which is set from ConnectionManagerImpl::doEndStream which is called on [stream reset or ] HCM endStream which is called from filter manager maybeEndEncode.

Isn't it really a way to say the encode has started which is used (in the non-half-close case) to neuter the complete-or-not decode side of things?

it feels like prior to any half close logic, writes to this boolean are "have we started encoding" and reads are "do we want to stop decoding" and if we replaced read calls with shouldDecode() {finished_encoding_ && !half_close} we could avoid the extra boolean

thoughts?

alyssawilk · 2024-06-27T18:54:24Z

source/common/http/filter_manager.h

+    bool remote_encode_complete_ : 1; // Set when encoder filter chain iteration has completed. All
+                                      // decoder filters have completed and end_stream was observed.
+    // TODO(yanavlasov): always use encoder_end_stream_ for tracking end_stream state during encoder
+    // filter iteration.


I have concerns about having this as a follow-up. I believe you have good intent but I also have seen the number of times you've been yanked away from projects due to internal reprioritization and this code is complicated enough I'm pretty strongly averse to making it harder to understand and having a TODO to clean up :-/

Explicitly calling this out as a blocking issue.
Given the level of unresolved tech debt* I am not ok with plan "sort it out when we remove the flag"
*https://github.com/envoyproxy/envoy/issues?q=is%3Aopen+label%3A%22tech+debt%22+assignee%3Ayanavlasov+

alyssawilk · 2024-06-27T18:55:48Z

source/common/http/filter_manager.h

+    bool stream_closed_ : 1; // Set when both remote_decode_complete_ and remote_encode_complete_ is
+                             // true observed for the first time and prevents ending the stream
+                             // multiple times. Only set when allow_upstream_half_close is enabled.
+    bool should_stop_decoding_ : 1; // Set to indicate that decoding on the stream should be stopped


this is what local complete was used for previously. And it appears to me in most of the decode calls we abort if local complete and not based on should_stop_decoding which makes me think this is not doing what it says it's doing. I think we're adding complexity and not correctness here,

alyssawilk · 2024-06-27T18:57:55Z

source/common/http/filter_manager.h

@@ -893,6 +916,11 @@ class FilterManager : public ScopeTrackedObject,
    bool decoder_filter_chain_aborted_ : 1;
    bool encoder_filter_chain_aborted_ : 1;
    bool saw_downstream_reset_ : 1;
+    bool stream_closed_ : 1; // Set when both remote_decode_complete_ and remote_encode_complete_ is


why not just use a helper function which checks both booleans?

alyssawilk

/wait since I think that was just a main merge - lmk if incorrect

alyssawilk · 2024-07-03T14:51:56Z

(also please be aware I'm out for a week starting mid-week next week so there's a limited window for reviews)

alyssawilk · 2024-07-09T20:10:10Z

source/common/http/filter_manager.h

+    bool remote_encode_complete_ : 1; // Set when encoder filter chain iteration has completed. All
+                                      // decoder filters have completed and end_stream was observed.
+    // TODO(yanavlasov): always use encoder_end_stream_ for tracking end_stream state during encoder
+    // filter iteration.


Explicitly calling this out as a blocking issue.
Given the level of unresolved tech debt* I am not ok with plan "sort it out when we remove the flag"
*https://github.com/envoyproxy/envoy/issues?q=is%3Aopen+label%3A%22tech+debt%22+assignee%3Ayanavlasov+

Signed-off-by: Yan Avlasov <[email protected]>

This API was introduced by envoyproxy#32991 and it conflates support for half-close with TCP proxying mode in H/1 codec. This interferes with envoyproxy#34461 which enables support for server half-close in H/2 proxying. This PR refactors API to separate enabling of the half-close behavior from TCP proxy behavior. Prior to envoyproxy#34461 half-close semativs were only enabled for TCP proxy upstreams, but envoyproxy#34461 introduces other cases as well. This necessitates setting half-close and TCP proxy modes separately. TCP proxy mode is now enabled in the class dedicated to the TCP proxy upstreams. --------- Signed-off-by: Yan Avlasov <[email protected]> Signed-off-by: asingh-g <[email protected]>

adisuissa · 2024-08-20T20:43:12Z

@KBaichoo @RyanTheOptimist can you PTAL?

KBaichoo

Thank you again for working on such a complicated bug. Looked at the implementation again, and had a few questions. / comments

Great job naming and refactoring some of the naming / logic here.

docs/root/intro/arch_overview/http/http_connection_management.rst

source/common/http/filter_manager.h

source/common/http/filter_manager.cc

KBaichoo · 2024-08-20T21:47:06Z

source/common/router/router.cc

@@ -1754,6 +1755,10 @@ void Filter::onUpstreamMetadata(Http::MetadataMapPtr&& metadata_map) {

 void Filter::onUpstreamComplete(UpstreamRequest& upstream_request) {
  if (!downstream_end_stream_) {
+    if (allow_multiplexed_upstream_half_close_) {
+      // Continue request if downstream is not done yet.
+      return;


While we shouldn't be resetting the upstream, we do also skip over doing these various timing, outlier detection, etc. When will we end up updating them in this case when allow_multiplied_upstream_half_close is true?

Never. This code updates the response_time which is response_complete_time - request_complete_time . It is negative for the case where response completes before request and budget percentage computations will be off. Histograms record uint64_t so these will end up being some large values.

Since the response_time becomes nonsensical in this case, I decided to skip recording the stats.

Signed-off-by: Yan Avlasov <[email protected]>

yanavlasov · 2024-08-22T21:14:48Z

Blocked by the #35815 . Some tests with independent half-close enabled do not pass without it.

RyanTheOptimist · 2024-08-23T14:16:06Z

/wait

Signed-off-by: Yan Avlasov <[email protected]>

yanavlasov · 2024-08-23T23:42:45Z

Blocked by the #35815 . Some tests with independent half-close enabled do not pass without it.

Unblocked

RyanTheOptimist

LGMT, modulo one whitespace nit.
Awesome comments and docs!

RyanTheOptimist · 2024-08-28T15:44:43Z

docs/root/intro/life_of_a_request.rst

+
+For HTTP/1 protocol or if independent half-close is disabled the stream is destroyed when the response
+is complete and reaches its end-of-stream, i.e. when trailers or the response header/body with
+end-stream set are received, even if the request has not yet completed. This is handled in


Great docs!

RyanTheOptimist · 2024-08-28T15:49:21Z

source/common/http/filter_manager.cc

+  // disabled, i.e. proxying is stopped as soon as encoding has finished.
+  // TODO(yanavlasov): to support this case the codec needs to notify HCM that it has closed its low
+  // level stream
+  //                   to avoid use-after-free when HCM cleans-up its ActiveStream


nit: I think this line wrapping got confused somehow

All requested changes were completed.

- Update `ENVOY_COMMIT` to the latest Envoy's commit. - Update affected `ENVOY_LOG` and `ENVOY_LOG_MISC` function calls to accommodate the changes in envoyproxy/envoy#35869. - Add the `Http1PrematureUpstreamHalfClose` case to `StreamDecoder::streamResetReasonToResponseFlag` to accommodate envoyproxy/envoy#34461. Signed-off-by: jiajunye <[email protected]>

Allow multiplexed upstream servers to half close the stream before th…

7263c53

…e downstream. Signed-off-by: Yan Avlasov <[email protected]>

yanavlasov marked this pull request as ready for review June 3, 2024 14:57

yanavlasov assigned alyssawilk and KBaichoo Jun 3, 2024

alyssawilk reviewed Jun 4, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 4, 2024

KBaichoo reviewed Jun 14, 2024

View reviewed changes

source/common/http/http1/codec_impl.cc Outdated Show resolved Hide resolved

source/common/router/router.cc Show resolved Hide resolved

envoy/http/stream_reset_handler.h Show resolved Hide resolved

source/common/http/filter_manager.h Outdated Show resolved Hide resolved

KBaichoo reviewed Jun 15, 2024

View reviewed changes

sio4 mentioned this pull request Jun 19, 2024

RST_STREAM sent to downstream client instead of remaining DATA, HEADERS on end stream #30149

Closed

Address comments

fd17760

Signed-off-by: Yan Avlasov <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 21, 2024

yanavlasov added 5 commits June 21, 2024 18:14

Merge branch 'main' into allow-upstream-half-close

723ed65

Address comments

1d8172c

Signed-off-by: Yan Avlasov <[email protected]>

Clarify comment

3ba7cc4

Signed-off-by: Yan Avlasov <[email protected]>

Reuse isHalfCloseEnabled callback

6133efc

Signed-off-by: Yan Avlasov <[email protected]>

Add stopDecoding

fcf2a76

Signed-off-by: Yan Avlasov <[email protected]>

Merge branch 'main' into allow-upstream-half-close

f24bc34

KBaichoo previously approved these changes Jun 26, 2024

View reviewed changes

alyssawilk reviewed Jun 27, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 27, 2024

Merge branch 'main' into allow-upstream-half-close

0e881dc

yanavlasov dismissed KBaichoo’s stale review via 0e881dc June 28, 2024 14:57

repokitteh-read-only bot removed the waiting label Jun 28, 2024

alyssawilk added the waiting label Jul 2, 2024

alyssawilk reviewed Jul 3, 2024

View reviewed changes

alyssawilk previously requested changes Jul 9, 2024

View reviewed changes

yanavlasov added 2 commits August 15, 2024 17:31

Merge branch 'main' into allow-upstream-half-close

bd30f81

Post merge fix

b9948ab

Signed-off-by: Yan Avlasov <[email protected]>

repokitteh-read-only bot removed the waiting label Aug 15, 2024

yanavlasov added 5 commits August 15, 2024 18:28

Update comment

e224738

Signed-off-by: Yan Avlasov <[email protected]>

Merge branch 'main' into allow-upstream-half-close

e4ae6b4

Fix docs

d5320da

Signed-off-by: Yan Avlasov <[email protected]>

Disable upstream filter tests

b804cb5

Signed-off-by: Yan Avlasov <[email protected]>

Add coverage

4b305d9

Signed-off-by: Yan Avlasov <[email protected]>

alyssawilk removed their assignment Aug 20, 2024

KBaichoo reviewed Aug 20, 2024

View reviewed changes

Address comments

9155e49

Signed-off-by: Yan Avlasov <[email protected]>

repokitteh-read-only bot added the waiting label Aug 23, 2024

yanavlasov added 2 commits August 23, 2024 16:55

Merge branch 'main' into allow-upstream-half-close

49b6162

Add asserts

c02ac47

Signed-off-by: Yan Avlasov <[email protected]>

repokitteh-read-only bot removed the waiting label Aug 23, 2024

Merge branch 'main' into allow-upstream-half-close

d965ff6

KBaichoo approved these changes Aug 27, 2024

View reviewed changes

RyanTheOptimist approved these changes Aug 28, 2024

View reviewed changes

yanavlasov merged commit 779e69e into envoyproxy:main Aug 28, 2024
48 checks passed

yanavlasov deleted the allow-upstream-half-close branch August 28, 2024 17:17

DerekTBrown mentioned this pull request Aug 29, 2024

[Docs + Dev Infra] Shorten istio/proxy image build process istio/istio#52932

Open

ravenblackx mentioned this pull request Aug 30, 2024

Make http3 codec not encode empty trailers block #35907

Merged

jiajunye mentioned this pull request Sep 3, 2024

Envoy update 1725048099 envoyproxy/nighthawk#1217

Merged

Allow multiplexed upstream servers to half close the stream before the downstream #34461

Allow multiplexed upstream servers to half close the stream before the downstream #34461

Conversation

yanavlasov commented May 31, 2024 • edited Loading

repokitteh-read-only bot commented May 31, 2024

repokitteh-read-only bot commented May 31, 2024

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KBaichoo Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov commented Jun 24, 2024

KBaichoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk commented Jul 3, 2024

Choose a reason for hiding this comment

adisuissa commented Aug 20, 2024

KBaichoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov commented Aug 22, 2024

RyanTheOptimist commented Aug 23, 2024

yanavlasov commented Aug 23, 2024 • edited Loading

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov commented May 31, 2024 •

edited

Loading

KBaichoo Jun 15, 2024 •

edited

Loading

yanavlasov commented Aug 23, 2024 •

edited

Loading