feat(pageserver): more info on read errors

Signed-off-by: Alex Chi Z <chi@neon.tech>
2026-07-13 17:10:39 +00:00 · 2025-02-26 20:13:53 +01:00
16 changed files with 208 additions and 306 deletions
--- a/.github/workflows/neon_extra_builds.yml
+++ b/.github/workflows/neon_extra_builds.yml
@@ -71,7 +71,7 @@ jobs:
    uses: ./.github/workflows/build-macos.yml
    with:
      pg_versions: ${{ needs.files-changed.outputs.postgres_changes }}
-      rebuild_rust_code: ${{ fromJson(needs.files-changed.outputs.rebuild_rust_code) }}
+      rebuild_rust_code: ${{ needs.files-changed.outputs.rebuild_rust_code }}
      rebuild_everything: ${{ fromJson(needs.files-changed.outputs.rebuild_everything) }}

  gather-rust-build-stats:
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1342,9 +1342,7 @@ dependencies = [
 "tokio-util",
 "tower 0.5.2",
 "tower-http",
- "tower-otel",
 "tracing",
- "tracing-opentelemetry",
 "tracing-subscriber",
 "tracing-utils",
 "url",
@@ -4486,18 +4484,18 @@ dependencies = [

 [[package]]
 name = "pin-project"
-version = "1.1.9"
+version = "1.1.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "dfe2e71e1471fe07709406bf725f710b02927c9c54b2b5b2ec0e8087d97c327d"
+checksum = "c95a7476719eab1e366eaf73d0260af3021184f18177925b07f54b30089ceead"
 dependencies = [
 "pin-project-internal",
 ]

 [[package]]
 name = "pin-project-internal"
-version = "1.1.9"
+version = "1.1.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f6e859e6e5bd50440ab63c47e3ebabc90f26251f7c73c3d3e837b74a1cc3fa67"
+checksum = "39407670928234ebc5e6e580247dd567ad73a3578460c5990f9503df207e8f07"
 dependencies = [
 "proc-macro2",
 "quote",
@@ -7296,20 +7294,6 @@ version = "0.3.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "121c2a6cda46980bb0fcd1647ffaf6cd3fc79a013de288782836f6df9c48780e"

-[[package]]
-name = "tower-otel"
-version = "0.2.0"
-source = "git+https://github.com/mattiapenati/tower-otel?rev=56a7321053bcb72443888257b622ba0d43a11fcd#56a7321053bcb72443888257b622ba0d43a11fcd"
-dependencies = [
- "http 1.1.0",
- "opentelemetry",
- "pin-project",
- "tower-layer",
- "tower-service",
- "tracing",
- "tracing-opentelemetry",
-]
-
 [[package]]
 name = "tower-service"
 version = "0.3.3"
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -193,10 +193,6 @@ toml_edit = "0.22"
 tonic = {version = "0.12.3", default-features = false, features = ["channel", "tls", "tls-roots"]}
 tower = { version = "0.5.2", default-features = false }
 tower-http = { version = "0.6.2", features = ["request-id", "trace"] }
-
-# This revision uses opentelemetry 0.27. There's no tag for it.
-tower-otel = { git = "https://github.com/mattiapenati/tower-otel", rev = "56a7321053bcb72443888257b622ba0d43a11fcd" }
-
 tower-service = "0.3.3"
 tracing = "0.1"
 tracing-error = "0.2"
--- a/compute_tools/Cargo.toml
+++ b/compute_tools/Cargo.toml
@@ -46,9 +46,7 @@ tokio = { workspace = true, features = ["rt", "rt-multi-thread"] }
 tokio-postgres.workspace = true
 tokio-util.workspace = true
 tokio-stream.workspace = true
-tower-otel.workspace = true
 tracing.workspace = true
-tracing-opentelemetry.workspace = true
 tracing-subscriber.workspace = true
 tracing-utils.workspace = true
 thiserror.workspace = true
--- a/compute_tools/src/bin/compute_ctl.rs
+++ b/compute_tools/src/bin/compute_ctl.rs
@@ -406,21 +406,6 @@ fn start_postgres(
 ) -> Result<(Option<PostgresHandle>, StartPostgresResult)> {
    // We got all we need, update the state.
    let mut state = compute.state.lock().unwrap();
-
-    // Create a tracing span for the startup operation.
-    //
-    // We could otherwise just annotate the function with #[instrument], but if
-    // we're being configured from a /configure HTTP request, we want the
-    // startup to be considered part of the /configure request.
-    let _this_entered = {
-        // Temporarily enter the /configure request's span, so that the new span
-        // becomes its child.
-        let _parent_entered = state.startup_span.take().map(|p| p.entered());
-
-        tracing::info_span!("start_postgres")
-    }
-    .entered();
-
    state.set_status(ComputeStatus::Init, &compute.state_changed);

    info!(
--- a/compute_tools/src/compute.rs
+++ b/compute_tools/src/compute.rs
@@ -110,23 +110,7 @@ pub struct ComputeState {
    /// compute wasn't used since start.
    pub last_active: Option<DateTime<Utc>>,
    pub error: Option<String>,
-
-    /// Compute spec. This can be received from the CLI or - more likely -
-    /// passed by the control plane with a /configure HTTP request.
    pub pspec: Option<ParsedSpec>,
-
-    /// If the spec is passed by a /configure request, 'startup_span' is the
-    /// /configure request's tracing span. The main thread enters it when it
-    /// processes the compute startup, so that the compute startup is considered
-    /// to be part of the /configure request for tracing purposes.
-    ///
-    /// If the request handling thread/task called startup_compute() directly,
-    /// it would automatically be a child of the request handling span, and we
-    /// wouldn't need this. But because we use the main thread to perform the
-    /// startup, and the /configure task just waits for it to finish, we need to
-    /// set up the span relationship ourselves.
-    pub startup_span: Option<tracing::span::Span>,
-
    pub metrics: ComputeMetrics,
 }

@@ -138,7 +122,6 @@ impl ComputeState {
            last_active: None,
            error: None,
            pspec: None,
-            startup_span: None,
            metrics: ComputeMetrics::default(),
        }
    }
@@ -788,9 +771,8 @@ impl ComputeNode {
        Ok(())
    }

-    /// Start Postgres as a child process and wait for it to start accepting
-    /// connections.
-    ///
+    /// Start Postgres as a child process and manage DBs/roles.
+    /// After that this will hang waiting on the postmaster process to exit.
    /// Returns a handle to the child process and a handle to the logs thread.
    #[instrument(skip_all)]
    pub fn start_postgres(
--- a/compute_tools/src/http/routes/configure.rs
+++ b/compute_tools/src/http/routes/configure.rs
@@ -45,18 +45,13 @@ pub(in crate::http) async fn configure(
            return JsonResponse::invalid_status(state.status);
        }

-        // Pass the tracing span to the main thread that performs the startup,
-        // so that the start_compute operation is considered a child of this
-        // configure request for tracing purposes.
-        state.startup_span = Some(tracing::Span::current());
-
        state.pspec = Some(pspec);
        state.set_status(ComputeStatus::ConfigurationPending, &compute.state_changed);
        drop(state);
    }

    // Spawn a blocking thread to wait for compute to become Running. This is
-    // needed to not block the main pool of workers and to be able to serve
+    // needed to do not block the main pool of workers and be able to serve
    // other requests while some particular request is waiting for compute to
    // finish configuration.
    let c = compute.clone();
--- a/compute_tools/src/http/server.rs
+++ b/compute_tools/src/http/server.rs
@@ -121,7 +121,6 @@ impl From<Server> for Router<Arc<ComputeNode>> {
                )
                .layer(PropagateRequestIdLayer::x_request_id()),
        )
-            .layer(tower_otel::trace::HttpLayer::server(tracing::Level::INFO))
    }
 }

--- a/libs/safekeeper_api/src/membership.rs
+++ b/libs/safekeeper_api/src/membership.rs
@@ -85,12 +85,12 @@ impl MemberSet {
        Ok(MemberSet { m: members })
    }

-    pub fn contains(&self, sk: NodeId) -> bool {
-        self.m.iter().any(|m| m.id == sk)
+    pub fn contains(&self, sk: &SafekeeperId) -> bool {
+        self.m.iter().any(|m| m.id == sk.id)
    }

    pub fn add(&mut self, sk: SafekeeperId) -> anyhow::Result<()> {
-        if self.contains(sk.id) {
+        if self.contains(&sk) {
            bail!(format!(
                "sk {} is already member of the set {}",
                sk.id, self
@@ -130,11 +130,6 @@ impl Configuration {
            new_members: None,
        }
    }
-
-    /// Is `sk_id` member of the configuration?
-    pub fn contains(&self, sk_id: NodeId) -> bool {
-        self.members.contains(sk_id) || self.new_members.as_ref().is_some_and(|m| m.contains(sk_id))
-    }
 }

 impl Display for Configuration {
--- a/pageserver/src/page_service.rs
+++ b/pageserver/src/page_service.rs
@@ -82,6 +82,49 @@ const LOG_SLOW_GETPAGE_THRESHOLD: Duration = Duration::from_secs(30);

 ///////////////////////////////////////////////////////////////////////////////

+/// Information about a GetPage@LSN request.
+#[derive(Debug, Clone)]
+pub struct GetPageRequestTraceInfo {
+    request_lsn: Lsn,
+    gc_cutoff_lsn: Lsn,
+    start_time: SystemTime,
+    original_start_time: Option<SystemTime>,
+    gc_blocked_by_lsn_lease_deadline: bool,
+    covered_by_lsn_lease: bool,
+    merged: bool,
+}
+
+impl std::fmt::Display for GetPageRequestTraceInfo {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use chrono::{DateTime, Utc};
+        let start_time: DateTime<Utc> = self.start_time.into();
+        let original_start_time: Option<DateTime<Utc>> = self.original_start_time.map(|t| t.into());
+        write!(
+            f,
+            "request_lsn={}, start_time={}, gc_cutoff_lsn={}",
+            self.request_lsn, start_time, self.gc_cutoff_lsn
+        )?;
+        if let Some(original_start_time) = original_start_time {
+            write!(f, "original_start_time={},", original_start_time)?;
+        }
+        write!(
+            f,
+            "gc_blocked_by_lsn_lease_deadline={}, covered_by_lsn_lease={}, merged={}",
+            self.gc_blocked_by_lsn_lease_deadline, self.covered_by_lsn_lease, self.merged
+        )
+    }
+}
+
+impl GetPageRequestTraceInfo {
+    pub fn merge(&mut self, other: &GetPageRequestTraceInfo) {
+        if !self.merged {
+            self.original_start_time = Some(self.start_time);
+        }
+        self.start_time = std::cmp::min(self.start_time, other.start_time);
+        self.merged = true;
+    }
+}
+
 pub struct Listener {
    cancel: CancellationToken,
    /// Cancel the listener task through `listen_cancel` to shut down the listener
@@ -507,8 +550,11 @@ enum PageStreamError {
    Shutdown,

    /// Something went wrong reading a page: this likely indicates a pageserver bug
-    #[error("Read error")]
-    Read(#[source] PageReconstructError),
+    #[error("Read error: {0}")]
+    Read(
+        #[source] PageReconstructError,
+        Option<GetPageRequestTraceInfo>,
+    ),

    /// Ran out of time waiting for an LSN
    #[error("LSN timeout: {0}")]
@@ -528,7 +574,7 @@ impl From<PageReconstructError> for PageStreamError {
    fn from(value: PageReconstructError) -> Self {
        match value {
            PageReconstructError::Cancelled => Self::Shutdown,
-            e => Self::Read(e),
+            e => Self::Read(e, None),
        }
    }
 }
@@ -612,6 +658,7 @@ enum BatchedFeMessage {
        shard: timeline::handle::WeakHandle<TenantManagerTypes>,
        effective_request_lsn: Lsn,
        pages: smallvec::SmallVec<[BatchedGetPageRequest; 1]>,
+        trace_info: Option<GetPageRequestTraceInfo>,
    },
    DbSize {
        span: Span,
@@ -876,6 +923,16 @@ impl PageServerHandler {
                    }};
                }

+                let mut trace_info = GetPageRequestTraceInfo {
+                    request_lsn: req.hdr.request_lsn,
+                    gc_cutoff_lsn: Lsn::INVALID,
+                    start_time: SystemTime::now(),
+                    gc_blocked_by_lsn_lease_deadline: false,
+                    covered_by_lsn_lease: false,
+                    merged: false,
+                    original_start_time: None,
+                };
+
                let key = rel_block_to_key(req.rel, req.blkno);
                let shard = match timeline_handles
                    .get(tenant_id, timeline_id, ShardSelector::Page(key))
@@ -922,6 +979,7 @@ impl PageServerHandler {
                    req.hdr.request_lsn,
                    req.hdr.not_modified_since,
                    &shard.get_applied_gc_cutoff_lsn(),
+                    Some(&mut trace_info),
                    ctx,
                )
                // TODO: if we actually need to wait for lsn here, it delays the entire batch which doesn't need to wait
@@ -937,6 +995,7 @@ impl PageServerHandler {
                    shard: shard.downgrade(),
                    effective_request_lsn,
                    pages: smallvec::smallvec![BatchedGetPageRequest { req, timer }],
+                    trace_info: Some(trace_info),
                }
            }
            #[cfg(feature = "testing")]
@@ -981,12 +1040,14 @@ impl PageServerHandler {
                    shard: accum_shard,
                    pages: accum_pages,
                    effective_request_lsn: accum_lsn,
+                    trace_info: batched_trace_info,
                }),
                BatchedFeMessage::GetPage {
                    span: _,
                    shard: this_shard,
                    pages: this_pages,
                    effective_request_lsn: this_lsn,
+                    trace_info: trace_info_to_batch,
                },
            ) if (|| {
                assert_eq!(this_pages.len(), 1);
@@ -1011,6 +1072,11 @@ impl PageServerHandler {
            })() =>
            {
                // ok to batch
+                if let Some(trace_info_to_batch) = trace_info_to_batch {
+                    if let Some(batched_trace_info) = batched_trace_info {
+                        batched_trace_info.merge(&trace_info_to_batch);
+                    }
+                }
                accum_pages.extend(this_pages);
                Ok(())
            }
@@ -1143,7 +1209,21 @@ impl PageServerHandler {
                        span.in_scope(|| info!("handler requested reconnect: {reason}"));
                        return Err(QueryError::Reconnect);
                    }
-                    PageStreamError::Read(_)
+                    PageStreamError::Read(_, Some(trace_info)) => {
+                        // print the all details to the log with {:#}, but for the client the
+                        // error message is enough.  Do not log if shutting down, as the anyhow::Error
+                        // here includes cancellation which is not an error.
+                        let full = utils::error::report_compact_sources(&e.err);
+                        span.in_scope(|| {
+                            error!("error reading relation or page version: {full:#}, {trace_info}")
+                        });
+
+                        PagestreamBeMessage::Error(PagestreamErrorResponse {
+                            req: e.req,
+                            message: e.err.to_string(),
+                        })
+                    }
+                    PageStreamError::Read(_, None)
                    | PageStreamError::LsnTimeout(_)
                    | PageStreamError::NotFound(_)
                    | PageStreamError::BadRequest(_) => {
@@ -1262,6 +1342,7 @@ impl PageServerHandler {
                shard,
                effective_request_lsn,
                pages,
+                trace_info,
            } => {
                fail::fail_point!("ps::handle-pagerequest-message::getpage");
                (
@@ -1274,6 +1355,7 @@ impl PageServerHandler {
                                effective_request_lsn,
                                pages,
                                io_concurrency,
+                                trace_info,
                                ctx,
                            )
                            .instrument(span.clone())
@@ -1726,6 +1808,7 @@ impl PageServerHandler {
        request_lsn: Lsn,
        not_modified_since: Lsn,
        latest_gc_cutoff_lsn: &RcuReadGuard<Lsn>,
+        mut trace_info: Option<&mut GetPageRequestTraceInfo>,
        ctx: &RequestContext,
    ) -> Result<Lsn, PageStreamError> {
        let last_record_lsn = timeline.get_last_record_lsn();
@@ -1752,16 +1835,33 @@ impl PageServerHandler {
        //
        // We may have older data available, but we make a best effort to detect this case and return an error,
        // to distinguish a misbehaving client (asking for old LSN) from a storage issue (data missing at a legitimate LSN).
-        if request_lsn < **latest_gc_cutoff_lsn && !timeline.is_gc_blocked_by_lsn_lease_deadline() {
-            let gc_info = &timeline.gc_info.read().unwrap();
-            if !gc_info.lsn_covered_by_lease(request_lsn) {
-                return Err(
-                    PageStreamError::BadRequest(format!(
-                        "tried to request a page version that was garbage collected. requested at {} gc cutoff {}",
-                        request_lsn, **latest_gc_cutoff_lsn
-                    ).into())
-                );
+        let gc_cutoff_lsn = **latest_gc_cutoff_lsn;
+        if !timeline.is_gc_blocked_by_lsn_lease_deadline() {
+            if request_lsn < gc_cutoff_lsn {
+                let gc_info = &timeline.gc_info.read().unwrap();
+                if !gc_info.lsn_covered_by_lease(request_lsn) {
+                    return Err(
+                        PageStreamError::BadRequest(format!(
+                            "tried to request a page version that was garbage collected. requested at {} gc cutoff {}",
+                            request_lsn, gc_cutoff_lsn
+                        ).into())
+                    );
+                }
+                // The request is below the gc_cutoff, but it is covered by the lsn lease.
+                if let Some(trace_info) = trace_info.as_mut() {
+                    trace_info.covered_by_lsn_lease = true;
+                }
            }
+            // Otherwise, the request is above the gc_cutoff.
+        } else {
+            // gc is blocked by lsn lease deadline, we don't do any check against gc_cutoff.
+            if let Some(trace_info) = trace_info.as_mut() {
+                trace_info.gc_blocked_by_lsn_lease_deadline = true;
+            }
+        }
+
+        if let Some(trace_info) = trace_info.as_mut() {
+            trace_info.gc_cutoff_lsn = gc_cutoff_lsn;
        }

        // Wait for WAL up to 'not_modified_since' to arrive, if necessary
@@ -1829,12 +1929,6 @@ impl PageServerHandler {
                .to_string()
        });

-        info!(
-            "acquired lease for {} until {}",
-            lsn,
-            valid_until_str.as_deref().unwrap_or("<unknown>")
-        );
-
        let bytes = valid_until_str.as_ref().map(|x| x.as_bytes());

        pgb.write_message_noflush(&BeMessage::RowDescription(&[RowDescriptor::text_col(
@@ -1858,6 +1952,7 @@ impl PageServerHandler {
            req.hdr.request_lsn,
            req.hdr.not_modified_since,
            &latest_gc_cutoff_lsn,
+            None,
            ctx,
        )
        .await?;
@@ -1885,6 +1980,7 @@ impl PageServerHandler {
            req.hdr.request_lsn,
            req.hdr.not_modified_since,
            &latest_gc_cutoff_lsn,
+            None,
            ctx,
        )
        .await?;
@@ -1912,6 +2008,7 @@ impl PageServerHandler {
            req.hdr.request_lsn,
            req.hdr.not_modified_since,
            &latest_gc_cutoff_lsn,
+            None,
            ctx,
        )
        .await?;
@@ -1934,6 +2031,7 @@ impl PageServerHandler {
        effective_lsn: Lsn,
        requests: smallvec::SmallVec<[BatchedGetPageRequest; 1]>,
        io_concurrency: IoConcurrency,
+        trace_info: Option<GetPageRequestTraceInfo>,
        ctx: &RequestContext,
    ) -> Vec<Result<(PagestreamBeMessage, SmgrOpTimer), BatchedPageStreamError>> {
        debug_assert_current_span_has_tenant_and_timeline_id();
@@ -1982,7 +2080,13 @@ impl PageServerHandler {
                        )
                    })
                    .map_err(|e| BatchedPageStreamError {
-                        err: PageStreamError::from(e),
+                        err: {
+                            let mut err = PageStreamError::from(e);
+                            if let PageStreamError::Read(_, err_trace_info) = &mut err {
+                                *err_trace_info = trace_info.clone();
+                            }
+                            err
+                        },
                        req: req.req.hdr,
                    })
                }),
@@ -2002,6 +2106,7 @@ impl PageServerHandler {
            req.hdr.request_lsn,
            req.hdr.not_modified_since,
            &latest_gc_cutoff_lsn,
+            None,
            ctx,
        )
        .await?;
--- a/safekeeper/src/http/routes.rs
+++ b/safekeeper/src/http/routes.rs
@@ -19,7 +19,7 @@ use safekeeper_api::models::{
    AcceptorStateStatus, PullTimelineRequest, SafekeeperStatus, SkTimelineInfo, TermSwitchApiEntry,
    TimelineCopyRequest, TimelineCreateRequest, TimelineStatus, TimelineTermBumpRequest,
 };
-use safekeeper_api::{ServerInfo, membership, models};
+use safekeeper_api::{ServerInfo, models};
 use storage_broker::proto::{SafekeeperTimelineInfo, TenantTimelineId as ProtoTenantTimelineId};
 use tokio::sync::mpsc;
 use tokio::task;
@@ -32,7 +32,7 @@ use utils::lsn::Lsn;

 use crate::debug_dump::TimelineDigestRequest;
 use crate::safekeeper::TermLsn;
-use crate::timelines_global_map::{DeleteOrExclude, TimelineDeleteResult};
+use crate::timelines_global_map::TimelineDeleteForceResult;
 use crate::{
    GlobalTimelines, SafeKeeperConf, copy_timeline, debug_dump, patch_control_file, pull_timeline,
 };
@@ -73,13 +73,10 @@ async fn tenant_delete_handler(mut request: Request<Body>) -> Result<Response<Bo
    check_permission(&request, Some(tenant_id))?;
    ensure_no_body(&mut request).await?;
    let global_timelines = get_global_timelines(&request);
-    let action = if only_local {
-        DeleteOrExclude::DeleteLocal
-    } else {
-        DeleteOrExclude::Delete
-    };
+    // FIXME: `delete_force_all_for_tenant` can return an error for multiple different reasons;
+    // Using an `InternalServerError` should be fixed when the types support it
    let delete_info = global_timelines
-        .delete_all_for_tenant(&tenant_id, action)
+        .delete_force_all_for_tenant(&tenant_id, only_local)
        .await
        .map_err(ApiError::InternalServerError)?;
    json_response(
@@ -87,7 +84,7 @@ async fn tenant_delete_handler(mut request: Request<Body>) -> Result<Response<Bo
        delete_info
            .iter()
            .map(|(ttid, resp)| (format!("{}", ttid.timeline_id), *resp))
-            .collect::<HashMap<String, TimelineDeleteResult>>(),
+            .collect::<HashMap<String, TimelineDeleteForceResult>>(),
    )
 }

@@ -211,15 +208,12 @@ async fn timeline_delete_handler(mut request: Request<Body>) -> Result<Response<
    check_permission(&request, Some(ttid.tenant_id))?;
    ensure_no_body(&mut request).await?;
    let global_timelines = get_global_timelines(&request);
-    let action = if only_local {
-        DeleteOrExclude::DeleteLocal
-    } else {
-        DeleteOrExclude::Delete
-    };
+    // FIXME: `delete_force` can fail from both internal errors and bad requests. Add better
+    // error handling here when we're able to.
    let resp = global_timelines
-        .delete_or_exclude(&ttid, action)
+        .delete(&ttid, only_local)
        .await
-        .map_err(ApiError::from)?;
+        .map_err(ApiError::InternalServerError)?;
    json_response(StatusCode::OK, resp)
 }

@@ -273,64 +267,6 @@ async fn timeline_snapshot_handler(request: Request<Body>) -> Result<Response<Bo
    Ok(response)
 }

-/// Error type for delete_or_exclude: either generation conflict or something
-/// internal.
-#[derive(thiserror::Error, Debug)]
-pub enum DeleteOrExcludeError {
-    #[error("refused to switch into excluding mconf {requested}, current: {current}")]
-    Conflict {
-        requested: membership::Configuration,
-        current: membership::Configuration,
-    },
-    #[error(transparent)]
-    Other(#[from] anyhow::Error),
-}
-
-/// Convert DeleteOrExcludeError to ApiError.
-impl From<DeleteOrExcludeError> for ApiError {
-    fn from(de: DeleteOrExcludeError) -> ApiError {
-        match de {
-            DeleteOrExcludeError::Conflict {
-                requested: _,
-                current: _,
-            } => ApiError::Conflict(de.to_string()),
-            DeleteOrExcludeError::Other(e) => ApiError::InternalServerError(e),
-        }
-    }
-}
-
-/// Remove timeline locally after this node has been excluded from the
-/// membership configuration. The body is the same as in the membership endpoint
-/// -- conf where node is excluded -- and in principle single ep could be used
-/// for both actions, but since this is a data deletion op let's keep them
-/// separate.
-async fn timeline_exclude_handler(mut request: Request<Body>) -> Result<Response<Body>, ApiError> {
-    let ttid = TenantTimelineId::new(
-        parse_request_param(&request, "tenant_id")?,
-        parse_request_param(&request, "timeline_id")?,
-    );
-    check_permission(&request, Some(ttid.tenant_id))?;
-
-    let global_timelines = get_global_timelines(&request);
-    let data: models::TimelineMembershipSwitchRequest = json_request(&mut request).await?;
-    let my_id = get_conf(&request).my_id;
-    // If request doesn't exclude us, membership switch endpoint should be used
-    // instead.
-    if data.mconf.contains(my_id) {
-        return Err(ApiError::Forbidden(format!(
-            "refused to switch into {}, node {} is member of it",
-            data.mconf, my_id
-        )));
-    }
-    let action = DeleteOrExclude::Exclude(data.mconf);
-
-    let resp = global_timelines
-        .delete_or_exclude(&ttid, action)
-        .await
-        .map_err(ApiError::from)?;
-    json_response(StatusCode::OK, resp)
-}
-
 /// Consider switching timeline membership configuration to the provided one.
 async fn timeline_membership_handler(
    mut request: Request<Body>,
@@ -345,29 +281,12 @@ async fn timeline_membership_handler(
    let tli = global_timelines.get(ttid).map_err(ApiError::from)?;

    let data: models::TimelineMembershipSwitchRequest = json_request(&mut request).await?;
-    let my_id = get_conf(&request).my_id;
-    // If request excludes us, exclude endpoint should be used instead.
-    if !data.mconf.contains(my_id) {
-        return Err(ApiError::Forbidden(format!(
-            "refused to switch into {}, node {} is not a member of it",
-            data.mconf, my_id
-        )));
-    }
-    let req_gen = data.mconf.generation;
    let response = tli
        .membership_switch(data.mconf)
        .await
        .map_err(ApiError::InternalServerError)?;

-    // Return 409 if request was ignored.
-    if req_gen == response.current_conf.generation {
-        json_response(StatusCode::OK, response)
-    } else {
-        Err(ApiError::Conflict(format!(
-            "request to switch into {} ignored, current generation {}",
-            req_gen, response.current_conf.generation
-        )))
-    }
+    json_response(StatusCode::OK, response)
 }

 async fn timeline_copy_handler(mut request: Request<Body>) -> Result<Response<Body>, ApiError> {
@@ -718,14 +637,11 @@ pub fn make_router(
        .post("/v1/pull_timeline", |r| {
            request_span(r, timeline_pull_handler)
        })
-        .put("/v1/tenant/:tenant_id/timeline/:timeline_id/exclude", |r| {
-            request_span(r, timeline_exclude_handler)
-        })
        .get(
            "/v1/tenant/:tenant_id/timeline/:timeline_id/snapshot/:destination_id",
            |r| request_span(r, timeline_snapshot_handler),
        )
-        .put(
+        .post(
            "/v1/tenant/:tenant_id/timeline/:timeline_id/membership",
            |r| request_span(r, timeline_membership_handler),
        )
--- a/safekeeper/src/timeline.rs
+++ b/safekeeper/src/timeline.rs
@@ -558,18 +558,11 @@ impl Timeline {
        });
    }

-    /// Cancel the timeline, requesting background activity to stop. Closing
-    /// the `self.gate` waits for that.
-    pub async fn cancel(&self) {
+    /// Background timeline activities (which hold Timeline::gate) will no
+    /// longer run once this function completes.
+    pub async fn shutdown(&self) {
        info!("timeline {} shutting down", self.ttid);
        self.cancel.cancel();
-    }
-
-    /// Background timeline activities (which hold Timeline::gate) will no
-    /// longer run once this function completes. `Self::cancel` must have been
-    /// already called.
-    pub async fn close(&self) {
-        assert!(self.cancel.is_cancelled());

        // Wait for any concurrent tasks to stop using this timeline, to avoid e.g. attempts
        // to read deleted files.
@@ -581,13 +574,13 @@ impl Timeline {
    /// Also deletes WAL in s3. Might fail if e.g. s3 is unavailable, but
    /// deletion API endpoint is retriable.
    ///
-    /// Timeline must be in shut-down state (i.e. call [`Self::close`] first)
+    /// Timeline must be in shut-down state (i.e. call [`Self::shutdown`] first)
    pub async fn delete(
        &self,
        shared_state: &mut WriteGuardSharedState<'_>,
        only_local: bool,
    ) -> Result<bool> {
-        // Assert that [`Self::close`] was already called
+        // Assert that [`Self::shutdown`] was already called
        assert!(self.cancel.is_cancelled());
        assert!(self.gate.close_complete());

@@ -1113,7 +1106,7 @@ impl ManagerTimeline {
 }

 /// Deletes directory and it's contents. Returns false if directory does not exist.
-pub async fn delete_dir(path: &Utf8PathBuf) -> Result<bool> {
+async fn delete_dir(path: &Utf8PathBuf) -> Result<bool> {
    match fs::remove_dir_all(path).await {
        Ok(_) => Ok(true),
        Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(false),
--- a/safekeeper/src/timelines_global_map.rs
+++ b/safekeeper/src/timelines_global_map.rs
@@ -4,15 +4,16 @@

 use std::collections::HashMap;
 use std::str::FromStr;
+use std::sync::atomic::Ordering;
 use std::sync::{Arc, Mutex};
 use std::time::{Duration, Instant};

 use anyhow::{Context, Result, bail};
 use camino::Utf8PathBuf;
 use camino_tempfile::Utf8TempDir;
+use safekeeper_api::ServerInfo;
 use safekeeper_api::membership::Configuration;
 use safekeeper_api::models::SafekeeperUtilization;
-use safekeeper_api::{ServerInfo, membership};
 use serde::Serialize;
 use tokio::fs;
 use tracing::*;
@@ -21,10 +22,9 @@ use utils::id::{TenantId, TenantTimelineId, TimelineId};
 use utils::lsn::Lsn;

 use crate::defaults::DEFAULT_EVICTION_CONCURRENCY;
-use crate::http::routes::DeleteOrExcludeError;
 use crate::rate_limit::RateLimiter;
 use crate::state::TimelinePersistentState;
-use crate::timeline::{Timeline, TimelineError, delete_dir, get_tenant_dir, get_timeline_dir};
+use crate::timeline::{Timeline, TimelineError, get_tenant_dir, get_timeline_dir};
 use crate::timelines_set::TimelinesSet;
 use crate::wal_storage::Storage;
 use crate::{SafeKeeperConf, control_file, wal_storage};
@@ -448,20 +448,23 @@ impl GlobalTimelines {
            .collect()
    }

-    /// Delete timeline, only locally on this node or globally (also cleaning
-    /// remote storage WAL), depending on `action` value.
-    pub(crate) async fn delete_or_exclude(
+    /// Cancels timeline, then deletes the corresponding data directory.
+    /// If only_local, doesn't remove WAL segments in remote storage.
+    pub(crate) async fn delete(
        &self,
        ttid: &TenantTimelineId,
-        action: DeleteOrExclude,
-    ) -> Result<TimelineDeleteResult, DeleteOrExcludeError> {
+        only_local: bool,
+    ) -> Result<TimelineDeleteForceResult> {
        let tli_res = {
            let state = self.state.lock().unwrap();

            if state.tombstones.contains_key(ttid) {
                // Presence of a tombstone guarantees that a previous deletion has completed and there is no work to do.
                info!("Timeline {ttid} was already deleted");
-                return Ok(TimelineDeleteResult { dir_existed: false });
+                return Ok(TimelineDeleteForceResult {
+                    dir_existed: false,
+                    was_active: false,
+                });
            }

            state.get(ttid)
@@ -469,47 +472,32 @@ impl GlobalTimelines {

        let result = match tli_res {
            Ok(timeline) => {
-                info!("deleting timeline {}, action={:?}", ttid, action);
+                let was_active = timeline.broker_active.load(Ordering::Relaxed);

-                // If node is getting excluded, check the generation first.
-                // Then, while holding the lock cancel the timeline; it will be
-                // unusable after this point, and if node is added back first
-                // deletion must be completed and node seeded anew.
-                //
-                // We would like to avoid holding the lock while waiting for the
-                // gate to finish as this is deadlock prone, so for actual
-                // deletion will take it second time.
-                if let DeleteOrExclude::Exclude(ref mconf) = action {
-                    let shared_state = timeline.read_shared_state().await;
-                    if shared_state.sk.state().mconf.generation > mconf.generation {
-                        return Err(DeleteOrExcludeError::Conflict {
-                            requested: mconf.clone(),
-                            current: shared_state.sk.state().mconf.clone(),
-                        });
-                    }
-                    timeline.cancel().await;
-                } else {
-                    timeline.cancel().await;
-                }
-
-                timeline.close().await;
+                info!("deleting timeline {}, only_local={}", ttid, only_local);
+                timeline.shutdown().await;

                info!("timeline {ttid} shut down for deletion");

                // Take a lock and finish the deletion holding this mutex.
                let mut shared_state = timeline.write_shared_state().await;

-                let only_local = !matches!(action, DeleteOrExclude::Delete);
                let dir_existed = timeline.delete(&mut shared_state, only_local).await?;

-                Ok(TimelineDeleteResult { dir_existed })
+                Ok(TimelineDeleteForceResult {
+                    dir_existed,
+                    was_active, // TODO: we probably should remove this field
+                })
            }
            Err(_) => {
                // Timeline is not memory, but it may still exist on disk in broken state.
                let dir_path = get_timeline_dir(self.state.lock().unwrap().conf.as_ref(), ttid);
-                let dir_existed = delete_dir(&dir_path).await?;
+                let dir_existed = delete_dir(dir_path)?;

-                Ok(TimelineDeleteResult { dir_existed })
+                Ok(TimelineDeleteForceResult {
+                    dir_existed,
+                    was_active: false,
+                })
            }
        };

@@ -527,11 +515,11 @@ impl GlobalTimelines {
    /// retry tenant deletion again later.
    ///
    /// If only_local, doesn't remove WAL segments in remote storage.
-    pub async fn delete_all_for_tenant(
+    pub async fn delete_force_all_for_tenant(
        &self,
        tenant_id: &TenantId,
-        action: DeleteOrExclude,
-    ) -> Result<HashMap<TenantTimelineId, TimelineDeleteResult>> {
+        only_local: bool,
+    ) -> Result<HashMap<TenantTimelineId, TimelineDeleteForceResult>> {
        info!("deleting all timelines for tenant {}", tenant_id);
        let to_delete = self.get_all_for_tenant(*tenant_id);

@@ -539,7 +527,7 @@ impl GlobalTimelines {

        let mut deleted = HashMap::new();
        for tli in &to_delete {
-            match self.delete_or_exclude(&tli.ttid, action.clone()).await {
+            match self.delete(&tli.ttid, only_local).await {
                Ok(result) => {
                    deleted.insert(tli.ttid, result);
                }
@@ -553,15 +541,17 @@ impl GlobalTimelines {

        // If there was an error, return it.
        if let Some(e) = err {
-            return Err(anyhow::Error::from(e));
+            return Err(e);
        }

        // There may be broken timelines on disk, so delete the whole tenant dir as well.
        // Note that we could concurrently create new timelines while we were deleting them,
        // so the directory may be not empty. In this case timelines will have bad state
        // and timeline background jobs can panic.
-        let tenant_dir = get_tenant_dir(self.state.lock().unwrap().conf.as_ref(), tenant_id);
-        delete_dir(&tenant_dir).await?;
+        delete_dir(get_tenant_dir(
+            self.state.lock().unwrap().conf.as_ref(),
+            tenant_id,
+        ))?;

        Ok(deleted)
    }
@@ -580,20 +570,18 @@ impl GlobalTimelines {
 }

 #[derive(Clone, Copy, Serialize)]
-pub struct TimelineDeleteResult {
+pub struct TimelineDeleteForceResult {
    pub dir_existed: bool,
+    pub was_active: bool,
 }

-/// Action for delete_or_exclude.
-#[derive(Clone, Debug)]
-pub enum DeleteOrExclude {
-    /// Delete timeline globally.
-    Delete,
-    /// Legacy mode until we fully migrate to generations: like exclude deletes
-    /// timeline only locally, but ignores generation number.
-    DeleteLocal,
-    /// This node is getting excluded, delete timeline locally.
-    Exclude(membership::Configuration),
+/// Deletes directory and it's contents. Returns false if directory does not exist.
+fn delete_dir(path: Utf8PathBuf) -> Result<bool> {
+    match std::fs::remove_dir_all(path) {
+        Ok(_) => Ok(true),
+        Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(false),
+        Err(e) => Err(e.into()),
+    }
 }

 /// Create temp directory for a new timeline. It needs to be located on the same
--- a/test_runner/fixtures/safekeeper/http.py
+++ b/test_runner/fixtures/safekeeper/http.py
@@ -273,22 +273,10 @@ class SafekeeperHttpClient(requests.Session, MetricsGetter):
        assert isinstance(res_json, dict)
        return res_json

-    def timeline_exclude(
-        self, tenant_id: TenantId, timeline_id: TimelineId, to: Configuration
-    ) -> dict[str, Any]:
-        res = self.put(
-            f"http://localhost:{self.port}/v1/tenant/{tenant_id}/timeline/{timeline_id}/exclude",
-            data=to.to_json(),
-        )
-        res.raise_for_status()
-        res_json = res.json()
-        assert isinstance(res_json, dict)
-        return res_json
-
    def membership_switch(
        self, tenant_id: TenantId, timeline_id: TimelineId, to: Configuration
    ) -> TimelineMembershipSwitchResponse:
-        res = self.put(
+        res = self.post(
            f"http://localhost:{self.port}/v1/tenant/{tenant_id}/timeline/{timeline_id}/membership",
            data=to.to_json(),
        )
--- a/test_runner/regress/test_compaction.py
+++ b/test_runner/regress/test_compaction.py
@@ -319,12 +319,8 @@ def test_pageserver_gc_compaction_idempotent(
            },
        )
        wait_until(compaction_finished, timeout=60)
-        workload.validate(env.pageserver.id)
-        # Ensure all data are uploaded so that the duplicated layer gets into index_part.json
-        ps_http.timeline_checkpoint(tenant_id, timeline_id, wait_until_flushed=True)
    if compaction_mode == "after_restart":
        env.pageserver.restart(True)
-        workload.validate(env.pageserver.id)
        ps_http.timeline_gc(
            tenant_id, timeline_id, None
        )  # Force refresh gc info to have gc_cutoff generated
@@ -339,7 +335,6 @@ def test_pageserver_gc_compaction_idempotent(
                    "sub_compaction_max_job_size_mb": 16,
                },
            )
-            workload.validate(env.pageserver.id)
            wait_until(compaction_finished, timeout=60)

    # ensure gc_compaction is scheduled and it's actually running (instead of skipping due to no layers picked)
--- a/test_runner/regress/test_wal_acceptor.py
+++ b/test_runner/regress/test_wal_acceptor.py
@@ -1686,7 +1686,7 @@ def test_replace_safekeeper(neon_env_builder: NeonEnvBuilder):


@pytest.mark.parametrize("auth_enabled", [False, True])
-def test_delete(neon_env_builder: NeonEnvBuilder, auth_enabled: bool):
+def test_delete_force(neon_env_builder: NeonEnvBuilder, auth_enabled: bool):
    neon_env_builder.auth_enabled = auth_enabled
    env = neon_env_builder.init_start()

@@ -2215,21 +2215,13 @@ def test_membership_api(neon_env_builder: NeonEnvBuilder):
    neon_env_builder.num_safekeepers = 1
    env = neon_env_builder.init_start()

-    # These are expected after timeline deletion on safekeepers.
-    env.pageserver.allowed_errors.extend(
-        [
-            ".*Timeline .* was not found in global map.*",
-            ".*Timeline .* was cancelled and cannot be used anymore.*",
-        ]
-    )
-
    tenant_id = env.initial_tenant
    timeline_id = env.initial_timeline

    sk = env.safekeepers[0]
    http_cli = sk.http_client()

-    sk_id_1 = SafekeeperId(sk.id, "localhost", sk.port.pg_tenant_only)
+    sk_id_1 = SafekeeperId(env.safekeepers[0].id, "localhost", sk.port.pg_tenant_only)
    sk_id_2 = SafekeeperId(11, "localhost", 5434)  # just a mock

    # Request to switch before timeline creation should fail.
@@ -2257,28 +2249,19 @@ def test_membership_api(neon_env_builder: NeonEnvBuilder):
    log.info(f"conf after restart: {after_restart}")
    assert after_restart.generation == 4

-    # Switch into non joint conf of which sk is not a member, must fail.
-    non_joint_not_member = Configuration(generation=5, members=[sk_id_2], new_members=None)
-    with pytest.raises(requests.exceptions.HTTPError):
-        resp = http_cli.membership_switch(tenant_id, timeline_id, non_joint_not_member)
-
-    # Switch into good non joint conf.
-    non_joint = Configuration(generation=6, members=[sk_id_1], new_members=None)
+    # Switch into disjoint conf.
+    non_joint = Configuration(generation=5, members=[sk_id_2], new_members=None)
    resp = http_cli.membership_switch(tenant_id, timeline_id, non_joint)
    log.info(f"non joint switch resp: {resp}")
    assert resp.previous_conf.generation == 4
-    assert resp.current_conf.generation == 6
+    assert resp.current_conf.generation == 5

-    # Switch request to lower conf should be rejected.
-    lower_conf = Configuration(generation=3, members=[sk_id_1], new_members=None)
-    with pytest.raises(requests.exceptions.HTTPError):
-        http_cli.membership_switch(tenant_id, timeline_id, lower_conf)
-
-    # Now, exclude sk from the membership, timeline should be deleted.
-    excluded_conf = Configuration(generation=7, members=[sk_id_2], new_members=None)
-    http_cli.timeline_exclude(tenant_id, timeline_id, excluded_conf)
-    with pytest.raises(requests.exceptions.HTTPError):
-        http_cli.timeline_status(tenant_id, timeline_id)
+    # Switch request to lower conf should be ignored.
+    lower_conf = Configuration(generation=3, members=[], new_members=None)
+    resp = http_cli.membership_switch(tenant_id, timeline_id, lower_conf)
+    log.info(f"lower switch resp: {resp}")
+    assert resp.previous_conf.generation == 5
+    assert resp.current_conf.generation == 5


 # In this test we check for excessive START_REPLICATION and START_WAL_PUSH queries