Do not store a donut template whose render threw

CHANGELOG.md

@@ -20,10 +20,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Requires `koriym/semantic-logger` `^0.11`. Written logs use its `log` document shape, and diagnostic `kind` is now `code`.
 - Requires `bear/resource` `^1.22.4`.
 - `KeepMutationsAndFailures` also keeps a session cut at the record limit (`semantic_logger_truncated`): a `command` past the limit leaves no other trace.
+- `ResourceDonut::refresh()`: a hole that throws while an already-cached `#[DonutCache]` page is recomposed (its own `onGet()` or its render) no longer lets that exception reach the caller directly from the refresh. It now degrades the same way a cache-store failure does and falls back to rendering the whole page fresh. A transient failure (the hole recovers on that fresh render) now answers 200 with the full body instead of an error - for example a corrupted legacy placeholder's `ParameterException`, previously an uncaught 500 straight from the refresh, now answers 200 on both this request and the next, since the fresh render re-stores the template with a correct `[le2:...]` placeholder. A hole that keeps throwing still fails the request - from the fresh render instead of from the refresh - exactly as the page would without the cache attribute at all.
 
 ### Fixed
 - `LogFileWriter`: two sessions flushed in the same microsecond no longer overwrite each other. The timestamped filename now carries a random suffix, so both survive (#221).
 - A `#[DonutCache]` page embedding an `#[Embed]` hole whose URI carried two or more query parameters leaked a raw `[le:...]` placeholder to the client and could throw `ParameterException` on the next request - fixed by moving new placeholders to a `[le2:...]` marker (#232).
+- An exception while rendering a `#[DonutCache]`/`#[CacheableResponse]` page - a hole's own resource or the page's own template - no longer stores an empty template that was served to every later visitor (since 1.17.0). The page's own template failure now surfaces exactly as it would for the same page without the cache attribute, instead of being swallowed into an empty page; a hole's failure now surfaces as a clean, catchable exception too - actually a better outcome than an `#[Embed]` of the same resource gets outside a `#[DonutCache]` page, where BEAR.Resource's own `AbstractRequest::__toString()` swallows it into a fatal `E_USER_ERROR` instead.
+- A degraded cache read (a down cache pool, or - see above - a hole failing while a cached page is refreshed) now actually renders its fresh-render fallback instead of returning the un-rendered result of running `onGet()` alone: previously such a request answered 200 with an unresolved `#[Embed]` hole still in its body and no view, rather than the composed page.
 - An embedded child of a `#[DonutCache]`/`#[CacheableResponse]` page is now reused until its query or links change, instead of running again when rendered as a hole after already being read in `onGet()`. Every such execution that was read but never rendered as a hole now tags the stored template and the live response's own Surrogate-Key/ETag, so purging any one of them refreshes the page instead of leaving it stale (#226).
 - `ResourceBodyEvaluator` no longer crashes materializing an embedded child whose `jsonSerialize()` does not answer the memoized `ResourceObject` (the shape `BEAR\Async\AsyncRequest` overrides it to): such a child is now left unmaterialized instead of asserting. Async support itself remains out of scope.
 

src/AbstractDonutCacheInterceptor.php

@@ -71,21 +71,23 @@ final public function invoke(MethodInvocation $invocation)
 
                 $this->triggerWarning($e);
 
-                return $invocation->proceed();
+                /** @var ResourceObject $fresh */
+                $fresh = $invocation->proceed();
+
+                // Same outcome a normal MISS reaches below: render (and, if the render and
+                // the store both succeed, re-cache) via the full donut path - a degraded
+                // read must not return $invocation->proceed()'s raw, un-rendered result
+                // (an un-executed #[Embed] request, `view` still null) directly to the
+                // caller, which would either show nothing or make a later `(string)` cast
+                // execute a hole untracked by UncacheableMark/SurrogateKeys, or - for a
+                // raw (non-donut-wrapped) AbstractRequest - trigger its own E_USER_ERROR.
+                return $this->handleProceedResult($fresh);
             }
 
             /** @var ResourceObject $ro */
             $ro = $invocation->proceed();
-            // donut created in ResourceObject
-            if (isset($ro->headers[Header::ETAG]) || $ro->code >= Code::BAD_REQUEST) {
-                // Record why this miss is not followed by a put; without it the log looks like a lost write.
-                $hasEtag = isset($ro->headers[Header::ETAG]);
-                $this->logger->event(new PutSkippedContext((string) $ro->uri, $hasEtag ? 'etag-present' : 'error-code', $hasEtag ? null : $ro->code));
 
-                return $ro;
-            }
-
-            return $this->putRecorded($ro);
+            return $this->handleProceedResult($ro);
         } finally {
             $durationMs = round((hrtime(true) - $start) / 1_000_000, 3);
             // Psalm mis-tracks the $hit flag mutated inside try when read from finally.
@@ -99,6 +101,26 @@ final public function invoke(MethodInvocation $invocation)
         }
     }
 
+    /**
+     * What runs on a fresh render, whether it is a normal MISS or a read-degrade fallback:
+     * an already-answered response (an ETag the resource set itself, an error code) is
+     * served as-is and the non-write recorded; anything else is rendered and stored through
+     * `putRecorded()`.
+     */
+    private function handleProceedResult(ResourceObject $ro): ResourceObject
+    {
+        // donut created in ResourceObject
+        if (isset($ro->headers[Header::ETAG]) || $ro->code >= Code::BAD_REQUEST) {
+            // Record why this miss is not followed by a put; without it the log looks like a lost write.
+            $hasEtag = isset($ro->headers[Header::ETAG]);
+            $this->logger->event(new PutSkippedContext((string) $ro->uri, $hasEtag ? 'etag-present' : 'error-code', $hasEtag ? null : $ro->code));
+
+            return $ro;
+        }
+
+        return $this->putRecorded($ro);
+    }
+
     /**
      * Run the donut write, degrading a store failure and recording either way
      *

src/DonutRequest.php

@@ -44,8 +44,16 @@ public function __toString(): string
             // AbstractRequest would double-execute a non-idempotent child, and for an
             // OnceRequest would still be correct, but there is no reason to prefer a
             // second call over the result already in hand).
+            //
+            // toString(), not (string) $executed: the magic __toString() swallows a
+            // Throwable from rendering into an E_USER_WARNING and '' - storing that empty
+            // placeholder content would make a #[CacheableResponse] view persist a hole
+            // that failed. toString() has no such catch, so the Throwable propagates
+            // through this __toString() (PHP 8 allows one to throw), through the
+            // template engine (Twig re-wraps it as a Twig\Error\RuntimeError), up to
+            // ResourceDonut::create()'s own non-swallowing render of the composing page.
             $executed = ($this->request)();
-            $view = (string) $executed;
+            $view = $executed->toString();
         } finally {
             $this->mark->leaveHole($uri);
         }

src/Exception/CacheStoreFailure.php

@@ -22,6 +22,14 @@
  * command or a direct call has no interceptor, so it reaches the caller and the write is not
  * reported as done. The automatic write inside a GET is caught and degraded - that request
  * changed nothing, so nothing at an edge became stale by it.
+ *
+ * `ResourceDonut::refresh()` also raises this type when a hole throws while an already-cached
+ * page is being recomposed - deliberately, not by accident: the read path degrades through
+ * the same catch that handles a storage outage, falling back to a full fresh render
+ * (`ResourceDonut::create()`), which then fails exactly as the hole would without the cache
+ * attribute at all if it is genuinely broken, or serves the full page if the failure was
+ * transient. `create()`'s own render does NOT use this type - a render failure there
+ * propagates unwrapped, matching a page with no cache attribute (see its docblock).
  */
 final class CacheStoreFailure extends RuntimeException
 {

src/ResourceDonut.php

@@ -4,9 +4,11 @@
 
 namespace BEAR\QueryRepository;
 
+use BEAR\QueryRepository\Exception\CacheStoreFailure;
 use BEAR\Resource\AbstractRequest;
 use BEAR\Resource\ResourceInterface;
 use BEAR\Resource\ResourceObject;
+use Throwable;
 
 use function array_key_exists;
 use function assert;
@@ -90,6 +92,13 @@ public function refresh(ResourceInterface $resource, ResourceObject $ro, Uncache
             try {
                 $ro = $resource->get($uri);
                 $ro->toString();
+            } catch (Throwable $e) {
+                // $ro->toString() does not swallow a Throwable (same non-swallowing render
+                // create() itself now uses) - converted here, deliberately, to
+                // CacheStoreFailure: unlike create()'s own render, which lets the Throwable
+                // travel unwrapped, a HIT's re-fetch degrades instead, so the interceptor's
+                // read path falls back to a full fresh render (reviewed design, kept as-is).
+                throw CacheStoreFailure::from($e);
             } finally {
                 $mark->leaveHole($uri);
             }
@@ -217,6 +226,21 @@ private function hashContent(string $view): string
         return hash('sha256', $view);
     }
 
+    /**
+     * `(string) $ro` here would call the page's own `ResourceObject::__toString()`, which
+     * swallows a render Throwable into an E_USER_WARNING and `''` - storing that would bake
+     * an empty page into the cache for every later visitor (since 1.17.0). `toString()` is
+     * the same render with no such catch (vendor/bear/resource), the same method
+     * `BEAR\Sunday\...\HttpResponder::getOutput()` already renders a response with - so a
+     * failure in the PAGE's own template propagates exactly as it would for a page with no
+     * cache attribute at all. A HOLE's failure travels too (DonutRequest wraps it with the
+     * same non-swallowing render, see DonutRequest::__toString()) - a clean, catchable
+     * exception, actually BETTER than the no-attribute baseline for an embedded hole: an
+     * un-donut-wrapped `#[Embed]` renders its child via the raw `AbstractRequest::__toString()`,
+     * which swallows into a fatal E_USER_ERROR instead (verified by execution, not assumed).
+     * Either way, the original Throwable - not a reconstruction - travels to the caller, and
+     * nothing past this call ever stores or returns a template.
+     */
     public static function create(ResourceObject $ro, DonutRendererInterface $storage, SurrogateKeys $etags, int|null $ttl, bool $isCacheble, UncacheableMarkInterface $mark): self
     {
         // A resource with nothing to return leaves its body null - a 204 typically does, while
@@ -234,7 +258,7 @@ public static function create(ResourceObject $ro, DonutRendererInterface $storag
             unset($maybeRequest);
         }
 
-        $donutTemplate = (string) $ro;
+        $donutTemplate = $ro->toString();
 
         return new self($donutTemplate, $ro->headers, $ttl, $isCacheble, code: $ro->code);
     }

tests/Fake/FakeWarningEmittingRenderer.php

@@ -0,0 +1,29 @@
+<?php
+
+declare(strict_types=1);
+
+namespace BEAR\QueryRepository;
+
+use BEAR\Resource\RenderInterface;
+use BEAR\Resource\ResourceObject;
+use Override;
+
+use function trigger_error;
+
+use const E_USER_WARNING;
+
+/**
+ * A renderer that emits an UNRELATED warning (not shaped like a swallowed-render warning,
+ * and not from a Throwable at all) and then renders successfully. Proves the guard never
+ * inspects warnings: there is nothing here for a false-positive match to trigger on.
+ */
+final class FakeWarningEmittingRenderer implements RenderInterface
+{
+    #[Override]
+    public function render(ResourceObject $ro): string
+    {
+        trigger_error('an unrelated notice during render', E_USER_WARNING);
+
+        return 'rendered-ok';
+    }
+}

tests/Fake/fake-app/src/Resource/App/AnonymousThrowingHole.php

@@ -0,0 +1,24 @@
+<?php
+
+declare(strict_types=1);
+
+namespace FakeVendor\HelloWorld\Resource\App;
+
+use BEAR\Resource\ResourceObject;
+use RuntimeException;
+
+/**
+ * A hole that throws an ANONYMOUS exception class - a design that identified a swallowed
+ * render by matching the exception class name in a reconstructed warning message would
+ * silently miss this (Kimi K3 review, MEDIUM: `RuntimeException@anonymous\0...` does not
+ * match a class-name regex) and still store the empty template. The current design never
+ * inspects the class at all: it lets `$ro->toString()` propagate the Throwable unexamined.
+ */
+class AnonymousThrowingHole extends ResourceObject
+{
+    public function onGet(): ResourceObject
+    {
+        throw new class ('anonymous hole blew up') extends RuntimeException {
+        };
+    }
+}

tests/Fake/fake-app/src/Resource/App/FlakyHole.php

@@ -0,0 +1,33 @@
+<?php
+
+declare(strict_types=1);
+
+namespace FakeVendor\HelloWorld\Resource\App;
+
+use BEAR\Resource\ResourceObject;
+use RuntimeException;
+
+/**
+ * A hole whose failure is controlled by a call counter, reset per test: fails exactly
+ * `$failureCount` more times, then succeeds. A transient failure decrements to 0 and
+ * recovers on the very next call (ResourceDonut::refresh()'s own re-fetch, then - if that
+ * also fails - AbstractDonutCacheInterceptor's fresh `proceed()` fallback); a persistent one
+ * is simulated by setting a count high enough that both attempts still fail.
+ */
+class FlakyHole extends ResourceObject
+{
+    public static int $failureCount = 0;
+
+    public function onGet(): static
+    {
+        if (self::$failureCount > 0) {
+            self::$failureCount--;
+
+            throw new RuntimeException('the hole is currently broken');
+        }
+
+        $this->body['value'] = 'healthy';
+
+        return $this;
+    }
+}

tests/Fake/fake-app/src/Resource/App/HoleWithNoTemplate.php

@@ -0,0 +1,23 @@
+<?php
+
+declare(strict_types=1);
+
+namespace FakeVendor\HelloWorld\Resource\App;
+
+use BEAR\Resource\ResourceObject;
+
+/**
+ * A hole whose `onGet()` succeeds - no template file exists for it, so its OWN render
+ * (not its `onGet()`) is what fails. Exercises `DonutRequest::__toString()`'s
+ * `$executed->toString()` call specifically: `ThrowingHole`'s failure happens earlier,
+ * inside `($this->request)()`, and never reaches that line at all.
+ */
+class HoleWithNoTemplate extends ResourceObject
+{
+    public function onGet(): static
+    {
+        $this->body['value'] = 'never rendered';
+
+        return $this;
+    }
+}

tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithFlakyHole.php

@@ -0,0 +1,24 @@
+<?php
+
+declare(strict_types=1);
+
+namespace FakeVendor\HelloWorld\Resource\Page\Html;
+
+use BEAR\RepositoryModule\Annotation\DonutCache;
+use BEAR\Resource\Annotation\Embed;
+use BEAR\Resource\ResourceObject;
+
+/**
+ * A #[DonutCache] page embedding a hole whose failure is controlled per test
+ * (`FlakyHole::$failureCount`) - used to exercise `ResourceDonut::refresh()`'s fallback to a
+ * fresh render on a HIT whose hole now fails.
+ */
+#[DonutCache]
+class DonutCacheWithFlakyHole extends ResourceObject
+{
+    #[Embed(rel: 'flaky', src: 'app://self/flaky-hole')]
+    public function onGet(): ResourceObject
+    {
+        return $this;
+    }
+}

tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithThrowingHole.php

@@ -0,0 +1,25 @@
+<?php
+
+declare(strict_types=1);
+
+namespace FakeVendor\HelloWorld\Resource\Page\Html;
+
+use BEAR\RepositoryModule\Annotation\DonutCache;
+use BEAR\Resource\Annotation\Embed;
+use BEAR\Resource\ResourceObject;
+
+/**
+ * A #[DonutCache] page whose embedded hole always throws mid-render - the template composed
+ * by `(string) $ro` in `ResourceDonut::create()` must never be stored: BEAR.Resource's
+ * `ResourceObject::__toString()` swallows that Throwable into an empty string, and storing it
+ * would serve an empty page to every later visitor (since 1.17.0).
+ */
+#[DonutCache]
+class DonutCacheWithThrowingHole extends ResourceObject
+{
+    #[Embed(rel: 'secret', src: 'app://self/throwing-hole')]
+    public function onGet(): ResourceObject
+    {
+        return $this;
+    }
+}

tests/Fake/fake-app/src/Resource/Page/Html/PlainNoCacheAttribute.php

@@ -0,0 +1,21 @@
+<?php
+
+declare(strict_types=1);
+
+namespace FakeVendor\HelloWorld\Resource\Page\Html;
+
+use BEAR\Resource\ResourceObject;
+
+/**
+ * A page with NO cache attribute at all - the actual baseline a `#[DonutCache]` render
+ * failure is compared against, not a `#[Cacheable]` page (a different interceptor).
+ */
+class PlainNoCacheAttribute extends ResourceObject
+{
+    public function onGet(): static
+    {
+        $this->body['value'] = 'plain';
+
+        return $this;
+    }
+}

tests/Fake/fake-app/var/templates/App/FlakyHole.html.twig

@@ -0,0 +1 @@
+{{ value }}

tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithFlakyHole.html.twig

@@ -0,0 +1 @@
+donut-cache-with-flaky-hole:<flaky>{{ flaky }}</flaky>

tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithThrowingHole.html.twig

@@ -0,0 +1 @@
+donut-cache-with-throwing-hole:<secret>{{ secret }}</secret>

tests/Fake/fake-app/var/templates/Page/Html/PlainNoCacheAttribute.html.twig

@@ -0,0 +1 @@
+plain-no-cache-attribute:{{ value }}