Fix: Transient s3 blips are being problematic

classes/local/object_manipulator/manipulator.php

@@ -115,6 +115,12 @@ public function execute(array $objectrecords) {
                 } else {
                     manager::update_object_by_hash($objectrecord->contenthash, $newlocation);
                 }
+            } catch (\Exception $e) {
+                // Transient storage errors (e.g. S3 connectivity blip) must not abort
+                // the entire batch — log and continue with the next object.
+                $this->logger->error_log(
+                    'Error processing object ' . $objectrecord->contenthash . ': ' . $e->getMessage()
+                );
             } finally {
                 $objectlock->release();
             }

classes/local/store/object_file_system.php

@@ -255,18 +255,26 @@ public function is_file_readable_externally_by_storedfile(stored_file $file) {
         }
 
         $path = $this->get_external_path_from_storedfile($file);
-        if (is_readable($path)) {
-            return true;
+        error_clear_last();
+        $readable = @is_readable($path);
+        if (!$readable) {
+            $error = error_get_last();
+            if ($error !== null) {
+                throw new \Exception(
+                    'Transient external storage error: ' . ($error['message'] ?? 'Unknown error')
+                );
+            }
         }
-
-        return false;
+        return $readable;
     }
 
     /**
      * is_file_readable_externally_by_hash
      * @param mixed $contenthash
      *
      * @return bool
+     * @throws \Exception On transient external storage errors (e.g. S3 connectivity failure).
+     *                    Returns false only when the object is confirmed absent.
      */
     public function is_file_readable_externally_by_hash($contenthash) {
         if ($contenthash === sha1('')) {
@@ -277,8 +285,22 @@ public function is_file_readable_externally_by_hash($contenthash) {
 
         $path = $this->get_external_path_from_hash($contenthash, false);
 
-        // Note - it is not possible to perform a content recovery safely from a hash alone.
-        return is_readable($path);
+        // Suppress the PHP Warning from the S3 StreamWrapper so we can inspect it ourselves.
+        // If is_readable returns false AND a PHP error was raised, the failure is transient
+        // (e.g. a connectivity blip) rather than the object genuinely being absent. In that
+        // case we throw so callers can retry rather than silently recording OBJECT_LOCATION_ERROR.
+        error_clear_last();
+        $readable = @is_readable($path);
+        if (!$readable) {
+            $error = error_get_last();
+            if ($error !== null) {
+                throw new \Exception(
+                    'Transient external storage error for ' . $contenthash . ': ' .
+                    ($error['message'] ?? 'Unknown error')
+                );
+            }
+        }
+        return $readable;
     }
 
     /**
@@ -520,20 +542,28 @@ public function xsendfile_file(stored_file $file): bool {
         }
 
         $contenthash = $file->get_contenthash();
-        if (
-            $this->presigned_url_configured() &&
-            $this->presigned_url_should_redirect_file($file) &&
-            $this->is_file_stored_externally_by_hash($contenthash)
-        ) {
-            return $this->redirect_to_presigned_url($contenthash, headers_list());
-        }
+        try {
+            if (
+                $this->presigned_url_configured() &&
+                $this->presigned_url_should_redirect_file($file) &&
+                $this->is_file_readable_externally_by_hash($contenthash)
+            ) {
+                return $this->redirect_to_presigned_url($contenthash, headers_list());
+            }
 
-        $ranges = $this->get_valid_http_ranges($file->get_filesize());
-        if (
-            $this->externalclient->support_presigned_urls() && !empty($ranges) &&
-            $this->is_file_stored_externally_by_hash($contenthash)
-        ) {
-            return $this->externalclient->proxy_range_request($file, $ranges);
+            $ranges = $this->get_valid_http_ranges($file->get_filesize());
+            if (
+                $this->externalclient->support_presigned_urls() && !empty($ranges) &&
+                $this->is_file_readable_externally_by_hash($contenthash)
+            ) {
+                return $this->externalclient->proxy_range_request($file, $ranges);
+            }
+        } catch (\Exception $e) {
+            // Transient external storage error - fall back to standard file serving.
+            debugging(
+                'objectfs: transient error in xsendfile_file for ' . $contenthash . ': ' . $e->getMessage(),
+                DEBUG_DEVELOPER
+            );
         }
 
         return false;
@@ -555,12 +585,20 @@ public function xsendfile($contenthash) {
             return parent::xsendfile($contenthash);
         }
         $headers = headers_list();
-        if (
-            $this->presigned_url_configured() &&
-            $this->is_file_stored_externally_by_hash($contenthash) &&
-            $this->presigned_url_should_redirect($contenthash, $headers)
-        ) {
-            return $this->redirect_to_presigned_url($contenthash, $headers);
+        try {
+            if (
+                $this->presigned_url_configured() &&
+                $this->is_file_readable_externally_by_hash($contenthash) &&
+                $this->presigned_url_should_redirect($contenthash, $headers)
+            ) {
+                return $this->redirect_to_presigned_url($contenthash, $headers);
+            }
+        } catch (\Exception $e) {
+            // Transient external storage error - fall back to standard file serving.
+            debugging(
+                'objectfs: transient error in xsendfile for ' . $contenthash . ': ' . $e->getMessage(),
+                DEBUG_DEVELOPER
+            );
         }
         return false;
     }
@@ -609,7 +647,13 @@ protected function get_object_handle_for_path($path, $type = \stored_file::FILE_
         switch ($type) {
             case \stored_file::FILE_HANDLE_FOPEN:
                 $context = $this->externalclient->get_seekable_stream_context();
-                return fopen($path, 'rb', false, $context);
+                $handle = @fopen($path, 'rb', false, $context);
+                if ($handle === false) {
+                    $error = error_get_last();
+                    $message = $error['message'] ?? 'Unknown filesystem error occurred.';
+                    throw new \Exception('Failed to open object file handle: ' . $message);
+                }
+                return $handle;
             case \stored_file::FILE_HANDLE_GZOPEN:
                 // Binary reading of file in gz format.
                 return gzopen($path, 'rb');

lang/en/tool_objectfs.php

@@ -255,12 +255,12 @@
 $string['settings:presignedurl:coresupport'] = 'Feature is not supported by core, you need to cherry pick: <a href="https://github.com/catalyst/moodle-tool_objectfs#allow-support-for-xsendfile-in-alternative-file-system">xsendfile support</a>';
 $string['settings:presignedurl:deletedsuccess'] = 'Files deleted successfully.';
 $string['settings:presignedurl:deletefiles'] = 'Delete test files.';
+$string['settings:presignedurl:disallowfileareas'] = 'Disallow file areas for Pre-Signed URL redirects';
+$string['settings:presignedurl:disallowfileareas_help'] = 'One entry per line in the format component|filearea. Matching files will never redirect to Pre-Signed URLs.';
 $string['settings:presignedurl:enablepresigneds3urls'] = 'S3 Pre-Signed URLs';
 $string['settings:presignedurl:enablepresigneds3urls_help'] = 'Enable Pre-Signed S3 URLs to request content directly from external storage.';
 $string['settings:presignedurl:enablepresignedurls'] = 'Enable Pre-Signed URLs';
 $string['settings:presignedurl:enablepresignedurls_help'] = 'Enable Pre-Signed URLs to request content directly from external storage.';
-$string['settings:presignedurl:disallowfileareas'] = 'Disallow file areas for Pre-Signed URL redirects';
-$string['settings:presignedurl:disallowfileareas_help'] = 'One entry per line in the format component|filearea. Matching files will never redirect to Pre-Signed URLs.';
 $string['settings:presignedurl:enablepresignedurlschoice'] = 'Signing method';
 $string['settings:presignedurl:expirationtime'] = 'Pre-Signed URL expiration time';
 $string['settings:presignedurl:expirationtime_help'] = 'All expirations are inherited from the Expires header sent by Moodle. If no headers are sent the Expiration defaults to this setting.';

tests/object_file_system_test.php

@@ -366,18 +366,12 @@ public function error_surpressor() {
         // normally because phpunit will still fail.
     }
 
-    public function test_xsendfile_updates_object_with_error_location_on_fail(): void {
-        global $DB;
+    public function test_xsendfile_returns_false_on_transient_fail(): void {
         $fakefile = $this->create_error_file();
 
-        // Phpunit will fail if PHP warning is thrown (which we want)
-        // so we surpress here.
-        set_error_handler([$this, 'error_surpressor']);
-        $this->filesystem->xsendfile($fakefile->get_contenthash());
-        restore_error_handler();
+        $result = $this->filesystem->xsendfile($fakefile->get_contenthash());
 
-        $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $fakefile->get_contenthash()]);
-        $this->assertEquals(OBJECT_LOCATION_ERROR, $location);
+        $this->assertFalse($result);
     }
 
     public function test_get_content_file_handle_if_object_is_local(): void {
@@ -406,18 +400,34 @@ public function test_get_content_file_handle_will_pull_remote_object_if_gzopen()
         $this->assertTrue(is_readable($localpath));
     }
 
-    public function test_get_content_file_handle_updates_object_with_error_location_on_fail(): void {
-        global $DB;
+    public function test_get_content_file_handle_throws_on_fail(): void {
         $fakefile = $this->create_error_file();
 
-        // Phpunit will fail if PHP warning is thrown (which we want)
-        // so we surpress here.
-        set_error_handler([$this, 'error_surpressor']);
-        $filehandle = $this->filesystem->get_content_file_handle($fakefile);
-        restore_error_handler();
+        $this->expectException(\Exception::class);
+        $this->expectExceptionMessageMatches('/Failed to open object file handle/');
 
-        $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $fakefile->get_contenthash()]);
-        $this->assertEquals(OBJECT_LOCATION_ERROR, $location);
+        $this->filesystem->get_content_file_handle($fakefile);
+    }
+
+    public function test_get_content_file_handle_does_not_set_error_location_on_transient_fail(): void {
+        global $DB;
+
+        // Create a file that physically doesn't exist so the file handle open will fail,
+        // but whose DB location is LOCAL (not already -1) so we can verify it stays that way.
+        $file = $this->create_local_file('transient fail test content');
+        $path = $this->get_local_path_from_storedfile($file);
+        unlink($path);
+        manager::update_object_by_hash($file->get_contenthash(), OBJECT_LOCATION_LOCAL);
+
+        try {
+            $this->filesystem->get_content_file_handle($file);
+        } catch (\Exception $e) {
+            // Expected — transient failure should throw, not silently record OBJECT_LOCATION_ERROR.
+            $this->assertStringContainsString('Failed to open', $e->getMessage());
+        }
+
+        $location = $DB->get_field('tool_objectfs_objects', 'location', ['contenthash' => $file->get_contenthash()]);
+        $this->assertNotEquals(OBJECT_LOCATION_ERROR, $location);
     }
 
     public function test_remove_file_will_remove_local_file(): void {

version.php

@@ -25,8 +25,8 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$plugin->version   = 2026041005;      // The current plugin version (Date: YYYYMMDDXX).
-$plugin->release   = 2026041005;      // Same as version.
+$plugin->version   = 2026041006;      // The current plugin version (Date: YYYYMMDDXX).
+$plugin->release   = 2026041006;      // Same as version.
 $plugin->requires  = 2024042200;      // Requires 4.4.
 $plugin->component = "tool_objectfs";
 $plugin->maturity  = MATURITY_STABLE;