Skip to content

Commit 82a1557

Browse files
committed
Fix bg fill teardown crash in update_size_and_time_stats
Production crash logs show ink_assert(0) firing in HttpTransact::update_size_and_time_stats from HttpSM::update_stats via HttpSM::kill_this, triggered by an Http2Stream event re-entering the state machine while background_fill was still in the STARTED state. The background_fill state is normally driven to a terminal value (COMPLETED or ABORTED) by tunnel_handler_server, but when the SM is torn down before that handler runs the state stays STARTED and the unreachable default branch in update_size_and_time_stats asserts. The same path also leaks proxy.process.http.background_fill_current_count because tunnel_handler_server is the only site that decrements the gauge after tunnel_handler_ua incremented it. This normalizes STARTED to ABORTED inside HttpSM::kill_this, before the enable_http_stats gate that guards update_stats, and decrements the gauge there. Doing the normalization in kill_this rather than in update_stats keeps the gauge accounting balanced regardless of whether http stats are enabled, and ensures the downstream stats helper sees a terminal state. As a defensive backstop this also folds the STARTED case into the ABORTED branch of update_size_and_time_stats so a future caller that reaches this point mid-fill records the bytes against background_fill_bytes_aborted instead of crashing the server.
1 parent 450d3f3 commit 82a1557

2 files changed

Lines changed: 23 additions & 1 deletion

File tree

src/proxy/http/HttpSM.cc

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7706,6 +7706,23 @@ HttpSM::kill_this()
77067706
// we must check it again
77077707
if (kill_this_async_done == true) {
77087708
pending_action = nullptr;
7709+
7710+
// The background fill state is normally driven to a terminal value
7711+
// (COMPLETED or ABORTED) by tunnel_handler_server when the server-side
7712+
// tunnel finishes. If the SM is torn down before that handler runs (for
7713+
// example, when an Http2Stream event re-enters the SM and drives
7714+
// kill_this while the bg fill is still in flight), the state can remain
7715+
// STARTED. In that case the background_fill_current_count gauge would
7716+
// also leak, because tunnel_handler_server is the only place that
7717+
// decrements it after tunnel_handler_ua incremented it. Normalize the
7718+
// state here, before the enable_http_stats gate, so the accounting
7719+
// balances regardless of whether http stats are enabled and so
7720+
// update_size_and_time_stats does not see an unexpected value.
7721+
if (background_fill == BackgroundFill_t::STARTED) {
7722+
background_fill = BackgroundFill_t::ABORTED;
7723+
Metrics::Gauge::decrement(http_rsb.background_fill_current_count);
7724+
}
7725+
77097726
if (t_state.http_config_param->enable_http_stats) {
77107727
update_stats();
77117728
}

src/proxy/http/HttpTransact.cc

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8957,6 +8957,12 @@ HttpTransact::update_size_and_time_stats(State *s, ink_hrtime total_time, ink_hr
89578957
Metrics::Counter::increment(http_rsb.background_fill_bytes_completed, bg_size);
89588958
break;
89598959
}
8960+
// STARTED is normally transitioned to COMPLETED or ABORTED by
8961+
// tunnel_handler_server, and HttpSM::kill_this() normalizes any leftover
8962+
// STARTED state to ABORTED before reaching here. Treat STARTED the same as
8963+
// ABORTED defensively in case a future code path reaches this point with
8964+
// the bg fill still in flight, so we record the bytes rather than abort.
8965+
case BackgroundFill_t::STARTED:
89608966
case BackgroundFill_t::ABORTED: {
89618967
int64_t bg_size = origin_server_response_body_size - user_agent_response_body_size;
89628968

@@ -8968,7 +8974,6 @@ HttpTransact::update_size_and_time_stats(State *s, ink_hrtime total_time, ink_hr
89688974
}
89698975
case BackgroundFill_t::NONE:
89708976
break;
8971-
case BackgroundFill_t::STARTED:
89728977
default:
89738978
ink_assert(0);
89748979
}

0 commit comments

Comments
 (0)