From 84ad91215211fcc5c10b7da98f59538a3818c47e Mon Sep 17 00:00:00 2001 From: Gabor Gevay Date: Wed, 10 Jun 2026 19:15:49 +0200 Subject: [PATCH] adapter: make duplicate end_statement_execution a soft panic Ending the same statement execution twice indicates a bug, but panicking on it aborts the whole environmentd process, turning a statement-logging bookkeeping error into an outage. Report it with soft_panic_or_log instead and keep the first end: still a hard panic in CI and debug builds, an error log in production. Co-Authored-By: Claude Fable 5 --- src/adapter/src/coord/statement_logging.rs | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/adapter/src/coord/statement_logging.rs b/src/adapter/src/coord/statement_logging.rs index 6e67b44248780..16b2525b5c589 100644 --- a/src/adapter/src/coord/statement_logging.rs +++ b/src/adapter/src/coord/statement_logging.rs @@ -16,7 +16,7 @@ use mz_compute_client::controller::error::CollectionLookupError; use mz_controller_types::ClusterId; use mz_ore::now::{EpochMillis, NowFn, epoch_to_uuid_v7, to_datetime}; use mz_ore::task::spawn; -use mz_ore::{cast::CastFrom, cast::CastInto}; +use mz_ore::{cast::CastFrom, cast::CastInto, soft_panic_or_log}; use mz_repr::adt::timestamp::TimestampLike; use mz_repr::{Datum, Diff, GlobalId, Row, Timestamp}; use mz_sql::plan::Params; @@ -395,6 +395,9 @@ impl Coordinator { /// (because it was not sampled). Requiring the opaque `StatementLoggingId` type, /// which is only instantiated by `begin_statement_execution` if the statement is actually logged, /// should prevent this. + /// + /// It is also an error to end the same execution twice; the duplicate end is + /// reported and ignored, keeping the first end. pub(crate) fn end_statement_execution( &mut self, id: StatementLoggingId, @@ -408,13 +411,17 @@ impl Coordinator { ended_at: now, }; - let began_record = self - .statement_logging - .executions_begun - .remove(&uuid) - .expect( - "matched `begin_statement_execution` and `end_statement_execution` invocations", + let Some(began_record) = self.statement_logging.executions_begun.remove(&uuid) else { + // A `StatementLoggingId` is only minted when a begin is logged, so + // a missing entry means this execution was already ended: some bug + // ended it twice. That's worth a loud report, but statement + // logging must never abort environmentd in production. + soft_panic_or_log!( + "duplicate end_statement_execution for statement {uuid}, reason: {:?}", + ended_record.reason ); + return; + }; for (row, diff) in Self::pack_statement_ended_execution_updates(&began_record, &ended_record) {