Stop caching datasource pages that cannot say when they go stale

A guests page was being saved as a static, which is wrong twice over: the live
state of the machines is the entire point of looking at it, and nothing could
ever invalidate the copy -- a guest starting is not a post being saved, and
libvirt is not going to tell us about it either.  So the cached listing would not
merely be stale, it would have no way of ever becoming right again.

Datasources now say which they are with a CACHEABLE constant.  Saying nothing
means no, the same way round as EDITABLE, and for the same reason: a source whose
author has not thought about invalidation should get the answer that is merely
slow rather than the one that is wrong.  Virt says no and explains why; DirIndex
says yes, which is only defensible because of the inotify watch it registers, and
its POD says that too -- take the watch away and it has to become false.

Doing that turned up why nobody had noticed.  TCMS::build_routes puts a route's
nocache flag on the query, but Trog::Renderer only ever looked for it in the
render *options*, and exactly one caller in the codebase (login) has ever
forwarded it there.  Every other nocache route was relying on being auth-gated,
so that the separate "is somebody logged in" test caught it -- which says nothing
whatsoever about a route that is nocache and public.  There was no such route
before; an uncacheable datasource page is one.  The renderer now reads nocache
off the data as well, so the flag works for the routes that already set it.

The lifecycle test asserts both halves through the real route on both data
models: a guests page is never handed to tPSGI to save, and a directory listing
still is.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T6Q1gRJ2Rr85S8FQ47EjnQ

Readme.md

@@ -182,11 +182,23 @@ A source has to provide posts($series, $query).  It may also provide:
 * EDITABLE - whether the wizard should generate an editor for the type.  A source
   building its posts from somewhere else has nothing to edit, and saying nothing
   means no.
-
-Datasource pages are cached as statics exactly like any other page, which raises
-the question of what throws that cache away -- saving a post will not, because no
-post was saved when somebody dropped a file in a directory.  A source which knows
-what it depends on should say so with tPSGI's watch facility, as DirIndex does:
+* CACHEABLE - whether its pages may be saved as static renders.  Saying nothing
+  means no; see below.
+
+Datasource pages would be cached as statics exactly like any other page, which
+raises the question of what throws that cache away -- saving a post will not,
+because no post was saved when somebody dropped a file in a directory.  So a
+source has to say which of the two it is:
+
+* CACHEABLE - it knows what it depends on and says when that changes, so its
+  pages may be cached.  Saying nothing means no, and that is the safe way round:
+  a source whose author has not thought about invalidation gets the answer that
+  is merely slow rather than the one that is wrong.  DirIndex says yes because it
+  watches the directory; Virt says no, because a guest's state is the point of
+  the page and nothing could ever tell us it changed.
+
+A source declaring CACHEABLE should say when it changes, using tPSGI's watch
+facility, as DirIndex does:
 
     $tpsgi->add_watch( $directory, sub {
         my ( $watcher, $change ) = @_;

design.md

@@ -133,6 +133,11 @@ variants and editing chrome, and caching those would serve one user's view to
 another. Anonymous readers get files off disk; editors get live renders. Saving a
 post invalidates the cache.
 
+`nocache` is read off both the render options and the page data, so a route's
+`nocache` flag -- which `TCMS::build_routes` puts on the query -- reaches it
+whether or not the route thought to forward it. That is also how a datasource
+which cannot say when its posts go stale keeps its pages out of the cache.
+
 Post types
 ==========
 
@@ -263,12 +268,16 @@ both are places the obvious implementation is wrong:
   along with the posts it filtered. `filter()` is how the search reaches your
   posts, and you should implement it if your posts carry anything worth
   searching beyond a title.
-- **Nothing else will invalidate your page's static render.** Datasource pages
-  are cached like any other, and a post being saved does not invalidate them,
-  because no post was saved -- the thing you depend on changed instead. If you
-  can name what you depend on, register a `$tpsgi->add_watch` for it with a
-  stable `key` and invalidate through the tPSGI object your callback is *handed*,
-  not the one it closed over. `DirIndex::_watch` is four lines of this.
+- **Nothing else will invalidate your page's static render.** A post being saved
+  does not invalidate it, because no post was saved -- the thing you depend on
+  changed instead. So you must answer one of two ways. Either you can name what
+  you depend on, in which case register a `$tpsgi->add_watch` for it with a
+  stable `key`, invalidate through the tPSGI object your callback is *handed*
+  rather than the one it closed over, and declare `CACHEABLE`
+  (`DirIndex::_watch` is four lines of this). Or you cannot, in which case say
+  nothing and your pages are never cached -- which is the default, because a
+  page that is merely slow beats one that is wrong with no way of becoming
+  right.
 - **Your posts probably all share a timestamp**, because you stamped them with
   the time you built them. Stored posts paginate by a `created` cursor, which on
   such a page cannot move -- every cursor is the same instant. Datasource pages

lib/Trog/DataSource.pm

@@ -32,6 +32,11 @@ Whether the wizard should generate an editor for a type drawn from this source.
 A source which builds its posts from somewhere else has nothing to edit, and
 saying nothing means no.
 
+    CACHEABLE                             optional constant
+
+Whether a page drawn from this source may be saved as a static render.  Saying
+nothing means no -- see cacheable() for why that is the safe way round.
+
     filter($query, @posts) = @posts       optional
 
 Apply the viewer's search to the synthesized posts.  Defaulted below; implement
@@ -180,6 +185,32 @@ Where to read about it.
 
 sub help { 'https://en.wikipedia.org/wiki/Substring' }
 
+=head2 cacheable($source) = BOOL
+
+Whether pages drawn from $source may be cached as static renders.
+
+A datasource page is cached like any other unless something says not to, and
+nothing else ever invalidates it: saving a post does not, because no post was
+saved -- whatever the source draws on changed instead.  So a source has to
+answer one of two questions, and it has to answer deliberately.
+
+Either it knows what it depends on and can say when that changes, in which case
+it should register a watch (Trog::DataSource::DirIndex::_watch is the worked
+example) and declare CACHEABLE.  Or it doesn't, in which case its pages must not
+be cached at all, because the cached copy has no way of ever becoming right
+again.
+
+Absent means not cacheable, which is the same way round as EDITABLE: a source
+whose author has not thought about invalidation gets the answer that is merely
+slow rather than the one that is wrong.
+
+=cut
+
+sub cacheable ($source) {
+    return 0 unless $source && $source->can('CACHEABLE');
+    return $source->CACHEABLE ? 1 : 0;
+}
+
 =head2 for_type($form) = STRING or undef
 
 The datasource a post type names, or nothing when it names none.

lib/Trog/DataSource/DirIndex.pm

@@ -40,6 +40,20 @@ collide with, the synthesized ones.  Put files in the directory instead.
 
 use constant EDITABLE => 0;
 
+=head2 CACHEABLE
+
+True, which is only defensible because of the watch _watch() registers.  A
+listing is cheap to build but there is no reason to build it per request, and the
+one thing that would make a cached copy wrong -- the directory changing -- is
+exactly the thing inotify tells us about.
+
+Take that watch away and this has to become false, or the page is stale with no
+way of ever becoming right again.
+
+=cut
+
+use constant CACHEABLE => 1;
+
 =head1 WHAT MAY BE INDEXED
 
 Only directories under C<www/>, and never C<www/assets/private>.

lib/Trog/DataSource/Virt.pm

@@ -36,6 +36,21 @@ would then sit alongside, and collide with, the synthesized ones.
 
 use constant EDITABLE => 0;
 
+=head2 CACHEABLE
+
+False.  A guest's state is the whole point of the page: whether it is running,
+how much memory it has, what its console looks like right now.  A static render
+of that is a photograph of a machine, and the reader wants the machine.
+
+There is also nothing that could invalidate it.  A guest starting or stopping is
+not a post being saved, and libvirt is not going to tell us about it -- so a
+cached listing would not merely be stale, it would have no way of ever becoming
+right again.
+
+=cut
+
+use constant CACHEABLE => 0;
+
 =head2 A NOTE ON SAFETY
 
 libvirt forbids virDomainScreenshot on a read-only connection, so the read path

lib/Trog/Renderer.pm

@@ -55,8 +55,23 @@ sub render ( $class, %options ) {
     return _yeet( $renderer, "Template not provided",                    %options ) unless $options{template};
 
     #TODO future - save the components too and then compose them?
-    my $data      = $options{data};
-    my $skip_save = $options{component} || $options{nocache} || !$data->{route} || $data->{has_query} || $data->{user} || ( $options{code} // 0 ) != 200 || Trog::Log::is_debug();
+    my $data = $options{data};
+
+    # $data->{nocache} as well as $options{nocache}: TCMS::build_routes puts a
+    # route's nocache flag on the query, and exactly one caller (login) has ever
+    # forwarded it here as an option.  Every other nocache route was relying on
+    # being auth-gated, so that the $data->{user} test below caught it -- which
+    # says nothing about a route that is nocache and public, and there is now
+    # one: a datasource whose posts go stale asks for it this way.
+    my $skip_save =
+         $options{component}
+      || $options{nocache}
+      || $data->{nocache}
+      || !$data->{route}
+      || $data->{has_query}
+      || $data->{user}
+      || ( $options{code} // 0 ) != 200
+      || Trog::Log::is_debug();
 
     my $ret;
     local $@;

lib/Trog/Routes/HTML.pm

@@ -1654,6 +1654,11 @@ sub _datasource_posts ( $query, $posts ) {
     # the created cursor stored posts use.
     $query->{is_datasource} = 1;
 
+    # A source which cannot say when its posts go stale must not have its pages
+    # saved as statics: nothing else would ever invalidate them, so the cached
+    # copy could never become right again.  See Trog::DataSource::cacheable.
+    $query->{nocache} = 1 unless Trog::DataSource::cacheable($source);
+
     return @ordered;
 }
 

t/Trog-DataSource.t

@@ -118,6 +118,81 @@ subtest 'for_type' => sub {
     is( Trog::DataSource::for_type('blog.tx'), undef, 'a type with no datasource names none' );
 };
 
+subtest 'cacheable' => sub {
+
+    # A datasource page is cached like any other unless something says not to,
+    # and nothing else ever invalidates it: saving a post does not, because no
+    # post was saved.  A source that cannot say when its posts go stale would
+    # therefore be cached forever wrong.
+    is( Trog::DataSource::cacheable('Trog::DataSource::TestPlain'), 0, 'a source which says nothing is not cacheable' );
+    is( Trog::DataSource::cacheable(undef),                         0, 'and neither is no source at all' );
+
+    require Trog::DataSource::Virt;
+    is( Trog::DataSource::cacheable('Trog::DataSource::Virt'), 0, 'Virt is not: a guest\'s state is the point of the page' );
+
+    require Trog::DataSource::DirIndex;
+    is( Trog::DataSource::cacheable('Trog::DataSource::DirIndex'), 1, 'DirIndex is, because it watches the directory' );
+};
+
+subtest 'the route marks an uncacheable page nocache' => sub {
+    require Trog::Routes::HTML;
+    require Test::MockModule;
+
+    my $meta = Test::MockModule->new('Trog::DataModule');
+    my $source;
+    $meta->redefine( type_meta_for => sub { return $source ? { 'x-tcms-datasource' => $source } : {} } );
+
+    my $query = sub { return { primary_post => { child_form => 'fake.tx' } } };
+
+    # An ordinary page is left alone.
+    $source = undef;
+    my $ordinary = $query->();
+    Trog::Routes::HTML::_datasource_posts( $ordinary, [ guest('stored') ] );
+    ok( !$ordinary->{nocache}, 'an ordinary page is not marked' );
+
+    $source = 'Trog::DataSource::TestPlain';
+    my $uncacheable = $query->();
+    Trog::Routes::HTML::_datasource_posts( $uncacheable, [] );
+    is( $uncacheable->{nocache}, 1, 'a source which has not said is marked nocache' );
+
+    $source = 'Trog::DataSource::TestCached';
+    my $cacheable = $query->();
+    Trog::Routes::HTML::_datasource_posts( $cacheable, [] );
+    ok( !$cacheable->{nocache}, 'and one which has said so is not' );
+};
+
+subtest 'nocache on the query reaches the renderer' => sub {
+    require Trog::Renderer;
+
+    # TCMS::build_routes puts a route's nocache flag on the query, and exactly
+    # one caller has ever forwarded it to render() as an option.  Everything
+    # else was relying on being auth-gated, which says nothing about a route
+    # that is nocache and public -- which is what an uncacheable datasource
+    # page is.
+    my @saved;
+    my $tpsgi = bless {}, 'FakeTPSGI2';
+    {
+        no warnings qw{once};
+        *FakeTPSGI2::add_post_close_callback = sub { $_[1]->();             return 1 };
+        *FakeTPSGI2::save_render             = sub { push( @saved, $_[1] ); return 1 };
+    }
+
+    # The dispatch table holds a code ref taken at load time, so redefining the
+    # sub it came from leaves the table pointing at the original.  Localise the
+    # entry instead.
+    no warnings qw{once};
+    local $Trog::Renderer::renderers{html} = sub { return [ 200, [], ['a page'] ] };
+
+    my %common = ( contenttype => 'text/html', template => 'whatever.tx', code => 200 );
+
+    Trog::Renderer->render( %common, data => { route => '/cacheable', tpsgi => $tpsgi } );
+    is_deeply( \@saved, ['/cacheable'], 'an ordinary page is saved as a static' );
+
+    @saved = ();
+    Trog::Renderer->render( %common, data => { route => '/live', tpsgi => $tpsgi, nocache => 1 } );
+    is_deeply( \@saved, [], 'one whose query says nocache is not' );
+};
+
 subtest 'the defaults are advertised' => sub {
     ok( length Trog::DataSource::lang(), 'there is a language to advertise' );
     like( Trog::DataSource::help(), qr{^https?://}, 'and somewhere to read about it' );
@@ -149,6 +224,7 @@ BEGIN {
     $INC{'Trog/DataSource/TestPlain.pm'}  = 1;
     $INC{'Trog/DataSource/TestPicky.pm'}  = 1;
     $INC{'Trog/DataSource/TestBroken.pm'} = 1;
+    $INC{'Trog/DataSource/TestCached.pm'} = 1;
 }
 
 {
@@ -156,6 +232,11 @@ BEGIN {
     package Trog::DataSource::TestPlain;
     sub posts { return ( main::guest('alpha'), main::guest('beta') ) }
 
+    # One which can say when its posts go stale, and so may be cached.
+    package Trog::DataSource::TestCached;
+    use constant CACHEABLE => 1;
+    sub posts { return ( main::guest('cached') ) }
+
     # One with opinions, including its own search language.
     package Trog::DataSource::TestPicky;
     our @FILTERED;

t/Trog-Routes-HTML-series-lifecycle.t

@@ -626,6 +626,13 @@ subtest 'a datasource-backed series searches and paginates' => sub {
     is_deeply( $titles->($third), [qw{guest11 guest12}], 'the last page holds the remainder' );
     unlike( $third, qr/rel="next"/, 'and offers nothing after it' );
 
+    # A guest's state is the point of the page, and nothing could invalidate a
+    # cached copy: a guest starting is not a post being saved, and libvirt is
+    # not going to tell us about it.
+    my $vtpsgi = Test::TPSGI->new( run_callbacks => 1 );
+    _render( _anon( route => '/specguests', tpsgi => $vtpsgi ), \&Trog::Routes::HTML::series );
+    is_deeply( [ keys %{ $vtpsgi->{renders} } ], [], 'a guests page is never saved as a static' );
+
     # The cursor paginator would be worse than useless here: every guest shares
     # a created, so Prev would ask for everything older than that one instant.
     unlike( $first, qr/older=$built/, 'no cursor is offered on a page that cannot use one' );
@@ -749,7 +756,8 @@ subtest 'a directory index series' => sub {
 
     # And it really is cached, rather than merely being cacheable in principle:
     # the renderer hands anonymous, unparameterised, successful renders to tPSGI
-    # to save, and a directory listing is one.
+    # to save, and a directory listing is one.  Which it may be, unlike the
+    # guests page above, precisely because it registers the watch below.
     ok( scalar( keys %{ $tpsgi->{renders} } ),                    'the listing was handed to tPSGI to save as a static' );
     ok( ( grep { m{^/specfiles:} } keys %{ $tpsgi->{renders} } ), 'under its own route' );