Bug 38454

Summary: Memory (L1) cache is not flushed before API requests
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: REST APIAssignee: Julian Maurice <julian.maurice>
Status: RESOLVED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: normal    
Priority: P5 - low CC: alexbuckley, dcook, didier.gautheron, fridolin.somers, jesse, jonathan.druart, katrin.fischer, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, paul.derscheid, philippe.blouin, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38630
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00,24.11.02,24.05.07,23.11.12,22.11.24
Circulation function:
Attachments: Bug 38454: [DO NOT PUSH] Add API route to showcase the bug
Bug 38454: Flush memory cache before every API request
Bug 38454: Flush memory cache before every API request
Bug 38454: Tidy
Bug 38454: Flush memory cache before every API request

Description Julian Maurice 2024-11-15 10:06:25 UTC
Not sure if it belongs to "security" but problems with cache can cause really weird behaviors sometimes, so I prefer to be safe.

The memory cache (Koha::Cache::Memory::Lite, and also probably - but not tested atm - the L1 cache of Koha::Cache) flushed before every OPAC and Staff request, but not before every API request.
So every request will start with the memory cache of the previous request (whether it was API, OPAC or Staff) handled by the same worker process.
Comment 1 Julian Maurice 2024-11-15 10:06:56 UTC
Created attachment 174554 [details] [review]
Bug 38454: [DO NOT PUSH] Add API route to showcase the bug

This route fetches a value from the memory cache, increment it, store it
in memory cache and return the incremented value.
If the memory cache was flushed before every request, the route should
always return "1". Instead it returns "1" at the first request, "2" at
the second request, and so on

Test plan:
1. Start the REST API app:
    `api/v1/app.pl daemon -l http://*:3000`
2. Use curl to request the app:
    `curl http://localhost:3000/memlite-counter`
Comment 2 Julian Maurice 2024-11-15 10:24:55 UTC
Created attachment 174557 [details] [review]
Bug 38454: Flush memory cache before every API request
Comment 3 Julian Maurice 2024-11-15 10:28:09 UTC
Test plan:
1. Apply first patch only, follow the test plan in comment 1, witness the bug
2. Apply 2nd patch, follow the test plan in comment 1 again, see that the bug is gone.
Comment 4 David Cook 2024-11-17 22:46:21 UTC
This is an interesting one!

I was going to say it shouldn't be a problem since plack.psgi redefines CGI::new and flushes the cache there, but that should only affect the Staff Interface and OPAC which still use CGI objects. 

But yeah the API shouldn't be using CGI objects. 

--

I think the test plan looks a bit alien to Koha devs/users, so I think that could be improved, but yeah... interesting. I don't know how this hasn't come up before. 

--

I have some other morning tasks to do, but I'll keep this one in my inbox and take another look later...
Comment 5 David Cook 2024-11-18 05:52:36 UTC
Got sidetracked by other security things...

Will have to come back to this one.
Comment 6 David Cook 2024-11-18 06:03:11 UTC
Test plan:
0. Apply the first patch "Bug 38454: [DO NOT PUSH] Add API route to showcase the bug"

1. Change '/memlite-counter' in Koha/REST/V1.pm to '/api/v1/memlite-counter'

2. sudo koha-plack --reload kohadev

3. Go to http://localhost:8081/api/v1/memlite-counter a few times
4. Note that the counter keeps increasing

--

I don't love that this test patch doesn't follow our usual methods, so it's not a perfect test. 

But... it looks good enough I think, and I can't see anything specific to the REST API that would cause any flushing...
Comment 7 David Cook 2024-11-18 06:07:55 UTC
We already have a "before_dispatch" hook. Why not add the cache clearing there rather than int he "around_action" hook?
Comment 8 Julian Maurice 2024-11-18 08:21:15 UTC
(In reply to David Cook from comment #7)
> We already have a "before_dispatch" hook. Why not add the cache clearing
> there rather than int he "around_action" hook?
before_dispatch is called even when no action is run, for instance, before a static file request (it will never happen in the context of the REST api but there is probably other cases). around_action is called right before an action so it seems like the best place.
Also it's what is already used in Koha::App::{Intranet,Opac}
Comment 9 Julian Maurice 2024-11-18 08:24:30 UTC
(In reply to David Cook from comment #6)
> I don't love that this test patch doesn't follow our usual methods, so it's
> not a perfect test. 
What do you mean ?
Comment 10 David Cook 2024-11-18 23:55:49 UTC
(In reply to Julian Maurice from comment #9)
> (In reply to David Cook from comment #6)
> > I don't love that this test patch doesn't follow our usual methods, so it's
> > not a perfect test. 
> What do you mean ?

That is, you didn't add the route via the OpenAPI plugin, so it's not a perfect test of a route Koha would have. 

But it's not like we're doing anything special with the "openapi_routes_added" hook for our routes. So it shouldn't matter.
Comment 11 David Cook 2024-11-19 00:23:32 UTC
Created attachment 174760 [details] [review]
Bug 38454: Flush memory cache before every API request

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 12 David Cook 2024-11-19 00:23:35 UTC
Created attachment 174761 [details] [review]
Bug 38454: Tidy

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 13 David Cook 2024-11-19 00:24:29 UTC
In any case, it works (more or less) as described, and I think it makes sense as a change overall.
Comment 14 Julian Maurice 2024-11-19 07:42:47 UTC
(In reply to David Cook from comment #10)
> That is, you didn't add the route via the OpenAPI plugin, so it's not a
> perfect test of a route Koha would have. 
Thanks for explaining. I agree it would have been better, but I was in a rush and I always struggle with the openapi schema... I didn't want to postpone this again
Comment 15 Julian Maurice 2024-12-05 11:26:02 UTC
This patch highlights a bug in how language is handled in the REST API: bug 38630.
It would be nice to have them pushed at the same time, but not mandatory.
Comment 16 David Cook 2025-01-07 04:14:43 UTC
I'm planning to use caching in a Plack Middleware, and I realise that the L1 cache won't be automatically flushed there either... 

And any script that uses CGI instead of Starman also won't have L1 cache flushed. At the moment that would include:

/cgi-bin/koha/tools/export.pl
/cgi-bin/koha/tools/inventory.pl
/cgi-bin/koha/tools/upload-cover-image.pl
/cgi-bin/koha/svc/cataloguing/metasearch
Comment 17 David Cook 2025-01-07 04:28:48 UTC
(In reply to David Cook from comment #16)
> I'm planning to use caching in a Plack Middleware, and I realise that the L1
> cache won't be automatically flushed there either... 

Actually, since Koha::Cache doesn't implement Memcached's increment method, I'll just bypass the Koha::Cache methods anyway, and work directly with the Cache::Memcached::Fast::Safe object. At least for my proof of concept...
Comment 18 Julian Maurice 2025-01-07 07:21:19 UTC
(In reply to David Cook from comment #16)
> And any script that uses CGI instead of Starman also won't have L1 cache
> flushed. At the moment that would include:
But scripts in CGI context always start with an empty L1 cache, right ?
Comment 19 Jonathan Druart 2025-01-07 08:29:10 UTC
(In reply to Julian Maurice from comment #18)
> (In reply to David Cook from comment #16)
> > And any script that uses CGI instead of Starman also won't have L1 cache
> > flushed. At the moment that would include:
> But scripts in CGI context always start with an empty L1 cache, right ?

Yes.
Comment 20 David Cook 2025-01-07 22:35:06 UTC
(In reply to Julian Maurice from comment #18)
> (In reply to David Cook from comment #16)
> > And any script that uses CGI instead of Starman also won't have L1 cache
> > flushed. At the moment that would include:
> But scripts in CGI context always start with an empty L1 cache, right ?

Ah right. Phew.
Comment 21 David Cook 2025-01-07 22:38:16 UTC
Anyway, I don't want to distract from these patches in any case. It would be good to get this fixed.
Comment 22 Jonathan Druart 2025-01-16 15:14:27 UTC
Created attachment 176673 [details] [review]
Bug 38454: Flush memory cache before every API request

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2025-01-16 15:14:37 UTC
Would be nice to have a test but I don't have the time right now to write it.

Also dropped the tidy patch.
Comment 24 Alex Buckley 2025-02-05 01:14:10 UTC
Hi all, 

We have applied the second patch ('Bug 38454: Flush memory cache before every API request') on this bug report to 24.05.x-security branch

Note: We ran through the test plan in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38454#c6 before and after applying the 'Bug 38454: Flush memory cache before every API request' patch.

After applying this patch the http://localhost:8081/api/v1/memlite-counter page did not keep incrementing when you refreshed it.

We also tried running through the test plan in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38454#c3 however the curl command output does not look expected or match what we see for this page http://localhost:8081/api/v1/memlite-counter.

Please let us know if we should remove the 'Bug 38454: Flush memory cache before every API request' patch from the 24.05.x-security branch
Comment 25 Julian Maurice 2025-02-05 07:22:52 UTC
(In reply to Alex Buckley from comment #24)
> We also tried running through the test plan in
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38454#c3 however
> the curl command output does not look expected or match what we see for this
> page http://localhost:8081/api/v1/memlite-counter.
Are you accessing Koha through a reverse proxy ? If so, then it is expected that the curl command in comment 3 does not work

> Please let us know if we should remove the 'Bug 38454: Flush memory cache
> before every API request' patch from the 24.05.x-security branch
If the test plan in comment 6 works, you can keep the patch.
Comment 26 Tomás Cohen Arazi (tcohen) 2025-02-05 13:24:47 UTC
This looks great to me.
Comment 27 Alex Buckley 2025-02-10 02:12:22 UTC
(In reply to Julian Maurice from comment #25)
> If the test plan in comment 6 works, you can keep the patch.

Great, thanks for your help!
Comment 28 Fridolin Somers 2025-02-14 14:10:47 UTC
Applied to 23.11.x-security
Comment 29 Jesse Maseto 2025-02-19 16:08:34 UTC
Applied to 22.11.x-security branch for next release.
Comment 30 Paul Derscheid 2025-02-22 18:18:32 UTC
Applied to 24.11.x-security
Comment 31 Katrin Fischer 2025-03-06 12:56:16 UTC
Pushed for 25.05!

Well done everyone, thank you!