notes

This reverts commit d359d88f51.

pageserver/src/page_service.rs

@@ -317,6 +317,9 @@ struct PageServerHandler {
     /// to see is the number of shards divided by the number of pageservers (typically < 2),
     /// or the ratio used when splitting shards (i.e. how many children created from one)
     /// parent shard, where a "large" number might be ~8.
+    ///
+    /// Hmm, we never remove timelines from this map, even not on remove_tenant_from_memory.
+    /// Shouldn't matter though.
     shard_timelines: HashMap<ShardIndex, HandlerTimeline>,
 }
 
@@ -585,7 +588,7 @@ impl PageServerHandler {
             let msg = tokio::select! {
                 biased;
 
-                _ = self.await_connection_cancelled() => {
+                _ = self.await_connection_cancelled() => { // we're sensitive here, and we see prompt sensitivy in the logs
                     // We were requested to shut down.
                     info!("shutdown request received in page handler");
                     return Err(QueryError::Shutdown)
@@ -1176,6 +1179,7 @@ impl PageServerHandler {
         ctx: &RequestContext,
     ) -> Result<PagestreamBeMessage, PageStreamError> {
         let timeline = match self.get_cached_timeline_for_page(req) {
+            // on cache hit, we don't check if timeline is active
             Ok(tl) => {
                 set_tracing_field_shard_id(tl);
                 tl
@@ -1210,6 +1214,7 @@ impl PageServerHandler {
 
         let latest_gc_cutoff_lsn = timeline.get_latest_gc_cutoff_lsn();
         let lsn = Self::wait_or_get_last_lsn(
+            // if `timeline` is not active, but not_modified_since is < last_record_lsn, this doesn't wait
             timeline,
             req.request_lsn,
             req.not_modified_since,
@@ -1219,6 +1224,7 @@ impl PageServerHandler {
         .await?;
 
         let page = timeline
+            // but if tl is cancelled, then we'd quickly return from here
             .get_rel_page_at_lsn(req.rel, req.blkno, Version::Lsn(lsn), ctx)
             .await?;
 

pageserver/src/tenant/remote_timeline_client.rs

@@ -519,7 +519,7 @@ impl RemoteTimelineClient {
         local_path: &Utf8Path,
         cancel: &CancellationToken,
         ctx: &RequestContext,
-    ) -> anyhow::Result<u64> {
+    ) -> Result<u64, DownloadError> {
         let downloaded_size = {
             let _unfinished_gauge_guard = self.metrics.call_begin(
                 &RemoteOpFileKind::Layer,

pageserver/src/tenant/storage_layer/layer.rs

@@ -1096,19 +1096,10 @@ impl LayerInner {
 
         match rx.await {
             Ok(Ok(res)) => Ok(res),
-            Ok(Err(e)) => {
-                // sleep already happened in the spawned task, if it was not cancelled
-                match e.downcast_ref::<remote_storage::DownloadError>() {
-                    // If the download failed due to its cancellation token,
-                    // propagate the cancellation error upstream.
-                    Some(remote_storage::DownloadError::Cancelled) => {
-                        Err(DownloadError::DownloadCancelled)
-                    }
-                    // FIXME: this is not embedding the error because historically it would had
-                    // been output to compute, however that is no longer the case.
-                    _ => Err(DownloadError::DownloadFailed),
-                }
-            }
+            Ok(Err(e)) => match e {
+                remote_storage::DownloadError::Cancelled => Err(DownloadError::DownloadCancelled),
+                _ => Err(DownloadError::DownloadFailed),
+            },
             Err(_gone) => Err(DownloadError::DownloadCancelled),
         }
     }
@@ -1118,7 +1109,7 @@ impl LayerInner {
         timeline: Arc<Timeline>,
         permit: heavier_once_cell::InitPermit,
         ctx: &RequestContext,
-    ) -> anyhow::Result<Arc<DownloadedLayer>> {
+    ) -> Result<Arc<DownloadedLayer>, remote_storage::DownloadError> {
         let result = timeline
             .remote_client
             .download_layer_file(

pageserver/src/tenant/timeline.rs

@@ -883,7 +883,7 @@ impl Timeline {
         // page_service.
         debug_assert!(!self.shard_identity.is_key_disposable(&key));
 
-        self.timeline_get_throttle.throttle(ctx, 1).await;
+        self.timeline_get_throttle.throttle(ctx, 1).await; // throttling should prob be sensitive to self cancellation, but really, caller should tokio::select! us
 
         match self.conf.get_impl {
             GetImpl::Legacy => {

pageserver/src/tenant/timeline/walreceiver/walreceiver_connection.rs

@@ -121,7 +121,7 @@ pub(super) async fn handle_walreceiver_connection(
 
     // prevent timeline shutdown from finishing until we have exited
     let _guard = timeline.gate.enter().map_err(|e| match e {
-        GateError::GateClosed => WalReceiverError::ClosedGate,
+        GateError::GateClosed => WalReceiverError::ClosedGate, // this one might be candidate
     })?;
     // This function spawns a side-car task (WalReceiverConnectionPoller).
     // Get its gate guard now as well.