Repository navigation
Replay DatabaseCache commit work as one pruned task per transaction - #8193
Merged
Merged
Conversation
A transaction touching many caches queued one commit task per cache and key, tripping the repeated commit task warning.
The repeated task warning is throttled per call site, so it can't explain what fills a large queue.
…it task warnings Container deletes queued a cache clear per sample type; the warning now names the registering call site, trims plumbing frames, and requires more distinct keys before flagging value-equal tasks.
labkey-jeckels
marked this pull request as ready for review
October 16, 2026 22:43
labkey-jeckels
requested review from
labkey-adam,
labkey-martyp and
labkey-tchad
October 16, 2026 22:43
Contributor
Author
|
@labkey-adam @labkey-martyp this is the change that tries to improve the cache reloading behavior within transactions and CommitTasks |
labkey-martyp
approved these changes
Oct 20, 2026
labkey-tchad
approved these changes
Oct 20, 2026
labkey-tchad
left a comment
labkey-tchad
left a comment
Member
There was a problem hiding this comment.
Looks good to me. Claude suggested some additional test cases: #8208
## Rationale
Filling some testing gaps found by Claude:
1. **No end-to-end test of pruning through DatabaseCache.** The whole
design depends on BlockingDatabaseCache.load() and TransactionCache
passing the same _databaseCache object as the Op.cache key. They do
today: TransactionCache
is built with this. But only the string-keyed unit tests exercise
pruning. If either side started passing getCache() or the wrapper
instead, every removal would stop dropping reloads, and no test would
fail. Suggested test
in DatabaseCache.TestCase, using a BlockingDatabaseCache with a counting
loader:
- Inside a transaction, call get("a") (miss), remove("a"), get("b"),
then clear(), then get("c"), and commit.
- Assert that the replay task holds only the clear and the reload of c.
- Assert that after commit the shared cache holds only c, and the loader
ran exactly once more after commit.
2. **Reloads are never shown to run after commit.**
testDatabaseCacheTransactionLimit commits but checks nothing afterwards.
Nothing proves the () -> get(key, argument, loader) lambda runs or fills
the shared cache. This was also
untested before the PR, but this PR rewrites that path. Two asserts
after transaction.commit() would cover it: the shared cache has maxSize
keys, and the loader call count is right.
3. **No automated rollback test.** The PR lists "Transaction rollback
doesn't update cache" as manually tested. A test that loads and removes
inside a transaction, closes it without committing, and asserts the
shared cache is
unchanged would lock that in.
4. **Remove → reload → remove of one key isn't tested.** The second
remove drops the reload in between, and its own add is a duplicate, so
the first REMOVE keeps its earlier position. The result is correct, but
this exact path in
remove() has no test. It would take three lines in
CacheReplayCommitTask.TestCase.
5. **Nothing tests registering an operation while the replay runs.** The
comment in run() says operations can be registered during replay when
commitAndKeepConnection() runs on an outer transaction. A unit test
where one operation
calls task.reload(...) on the same task would pin the copy that prevents
a ConcurrentModificationException. Without it, a later "simplification"
that drops the copy would pass every test.
3 tasks done
labkey-jeckels
added a commit
that referenced
this pull request
Oct 22, 2026
## Rationale Fix stale ObjectIds after ontology objects are deleted by SQL: recreating a deleted LSID reused its old ObjectId and failed the `objectproperty` foreign key. This path never invalidated the ObjectId cache; #8193's more accurate post-commit cache replay now leaves those ObjectIds cached, which broke `MixtureBatchUpgradeTest` when a batch edit removes and re-adds its material inputs ([TeamCity failure](https://teamcity.labkey.org/buildConfiguration/LabKey_Trunk_Internal_Upgrade_UpgradeSetupPostgres/4227772)). ## Related Pull Requests - #8193 ## Changes - Deleting ontology objects by SQL now invalidates the ObjectId and property-map caches, like the other delete paths - The SQL delete runs as one statement that reports exactly which objects it removed, so only those cache entries are evicted; very large deletes fall back to clearing the affected containers - All ontology delete paths share one cache-invalidation helper - New integration tests cover recreating objects after a SQL delete, inside and outside a transaction, across containers, and past the targeted-eviction limit Tested locally: the new tests fail without the fix and pass with it, and `OntologyManager`, `CacheReplayCommitTask`, and `DatabaseCache` tests pass. ## Tasks - [x] Claude Code Review - [x] Manual Testing - N/A - [x] Test Automation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale
Big transactions spend seconds after commit reloading cache entries they've already invalidated; this replays each transaction's DatabaseCache work as one task and skips what a later removal or clear undoes. A container delete measured 1,194 post-commit reloads taking 2-17 s, nearly all for ontology caches that the same delete clears. Also adds a DEBUG summary of large commit task queues, since the throttled repeated-task warning can't show what fills one.
Related Pull Requests
Changes
Tasks