Gemma2 delete experiments fix - #1682
Merged
Merged
Conversation
…ackport AbstractCLI imported org.ocpsoft.prettytime.shade.edu.emory.mathcs.backport.java.util.Collections -- a JDK 1.4-era backport that prettytime-nlp carries inside its uber-jar, almost certainly an IDE auto-import picking the wrong candidate. Its emptyList() returns a raw List, which is why the call site needed a //noinspection unchecked; with java.util.Collections the inference works and the suppression goes away.
removeAllSingleCellDataVectors streamed ee.getSingleCellExpressionDataVectors() to collect the distinct quantitation types, which forced the lazy collection to initialize. That selects every vector's DATA and DATA_INDICES blob along with its eager join graph. On GSE277430 -- 25,050 vectors spanning 333,570 cells -- it exhausted a 30 GB heap. The OutOfMemoryError then aborted mid-resultset and left the connection's protocol stream desynced, so the subsequent rollback parsed a reply against a stale buffer and threw ArrayIndexOutOfBoundsException, which Connector/J wrapped as SQLState S1000. Spring reported that as "Application exception overridden by rollback exception", discarding the OOM, and the retry advice then re-ran the delete on the broken connection. The visible symptom was therefore an unexplained "arraycopy: last source index 16 out of bounds for byte[14]" with no trace of the real cause. Resolve the QTs with a projection query when the collection is not already loaded, exactly as removeAllRawDataVectors has always done. Forcing initialization also made the Hibernate.isInitialized() check further down unconditionally true, defeating the eviction guard it fronts. The collection is still initialized once, by super.remove(ee)'s cascade walk, but that happens after the bulk delete so it selects zero rows and pulls no blobs. Test: removeWithUninitializedSingleCellDataVectors covers the branch that runs in production, which no existing test reached -- every other fixture here builds its experiment in-session, so only the already-initialized branch ran and the new HQL would have gone unvalidated. It asserts through Hibernate statistics that no vector entity is loaded; an emptiness assertion cannot distinguish the two paths, since the broken one loads the collection and then clears it. Verified to fail (1 vector loaded, expected 0) with the fix reverted.
…entity AclObjectIdentity is @immutable on this branch, so Hibernate silently discards aoi.setParentObject(...) -- no exception, no UPDATE. All four child-parent lint fixes recorded fixed=true before any flush, so the linter reported repairs it had not made. A run against prod on 2026-08-06 reported 548 parent assignments and wrote none of them: the 16 subsets blocking the deletion of GSE273690 came back with parent_object still NULL, and the deletion stayed denied. Route the writes through aclService.readAclById + setParent + updateAcl, the same sequence BaseAclAdvice uses when it wires a child's parent. That reaches JdbcMutableAclService, which writes by JDBC and evicts the ACL cache. The two SecuredNotChild sites were converted earlier for exactly this reason and carry a javadoc describing the trap; these four were missed. hotfix-1.32.8 is not affected. It builds against gsec 0.0.22, whose AclObjectIdentity.hbm.xml declares no mutable="false", so the entity mutation persists there. The defect arrived with the canonical-schema migration, which replaced gsec's mutable ACL entity with a JPA one annotated @immutable while carrying the linter over unchanged. Not yet verified end to end, and deliberately shipped without a test rather than with a misleading one. BaseDatabaseTest5 wires AclServiceImpl/AclDaoImpl -- the Hibernate-backed stack GemmaAclConfiguration describes as "no longer wired in" -- so a test written against that context exercises a different implementation than the one this fixes, and fails for reasons unrelated to the change. The existing AclDaoImplParentPersistenceTest covers precisely this invariant and is @disabled for a wiring reason, which is how the regression reached production. Verification is a lintAcls --apply-fixes run against the real wiring plus a parent_object check in acl_object_identity; a meaningful regression test needs the test context to wire JdbcMutableAclService first.
…n audit Follow-up measurements on the deletion remediation plan. The canonical-store drift is not a fixed backlog: re-measured after deleting GSE306819 and cleaning its ACLs, legacy orphans were 0 while canonical orphans went 44 -> 46 in ~14 hours, neither of them ours. A phase2 deletion leaks both stores, so a deletion that cleaned legacy but not canonical can only have come from 1.32.x. Reconciliation therefore has to handle the continuous case. New section 4.1 answers the question of whether single-cell components are more loosely attached than the rest. They are, but the weak edge is backstopped: everything below a dimension is FK-enforced RESTRICT, and no dimension is orphaned database-wide. The genuinely weak edge is dimension -> experiment, which has no FK at all; a surviving dimension is caught by the bio-assay FK and turns into a loud failure rather than a silent orphan. That surfaced defect 8: phase2 resolves dimensions through the SINGLE_CELL_DIMENSION_EXPERIMENT cache that 1.32.x does not maintain, so 17 named experiments currently cannot be deleted on this branch, and hotfix-1.32.8's vector-derived query does not have that gap.
…CL check Defect 9 and a new section 4.2. Deleting GSE273690 was denied before any data was touched: ExpressionExperimentWriteServiceImpl removes subsets one at a time through an ACL-secured call, and phase2 had no ACL for the experiment's 16 subsets. That is defect 6 escalating from orphan rows to blocked operations, and it identifies the risk class -- aggregated or preprocessed datasets, 56 of them today. Missing analysis ACLs do not block deletion, since analyses are removed by experiment-scoped calls. The repair tool then reported 548 parent assignments and wrote none of them, leaving identities with entries_inheriting=1, no parent and no ACEs, which still denies. Fixed in cc5b515 and verified: all 16 subsets now carry their parent. Also recorded why this was invisible to the suite -- every enabled linter test runs applyFixes(false), the one that applies fixes asserts nothing, and the test targeting this exact invariant is @disabled -- and that ACL tests exercise AclServiceImpl/AclDaoImpl while runtime uses JdbcMutableAclService, so a passing ACL test says little about deployed behaviour. New section 6.1 gives the pre-deletion check for experiments with subsets, so the next one does not fail the same way.
AclEventListenerConfig and AuditTrailEventListenerConfig are @configuration classes that exist only for the side effect in afterPropertiesSet(): registering Hibernate event listeners. Nothing injects them. LazyInitByDefaultPostProcessor, registered in CliComponentScanConfig at HIGHEST_PRECEDENCE, marks every non-infrastructure bean definition lazy-init. Both configs were therefore defined but never instantiated in CLI contexts, and their listeners never registered -- with no error and no warning. ACL maintenance has been off since 21e4fc4 (2026-05-18), which disabled the AOP advice and handed the job to AclEventListener. The listener covers both POST_INSERT and POST_DELETE, so both directions were lost: - deletes left the whole ACL tree behind (115/342/281 rows removed by hand for GSE277430, GSE306819, GSE273690) - inserts created no ACL at all: 76 experiments, IDs 92410-93196, contiguous, nothing older The insert side is worse. With no acl_object_identity row hasPermission fails for every user including admins, and AFTER_ACL_READ_QUIET turns that into a silent null, so the experiment is indistinguishable from one that does not exist. That is why viewChangelog could not locate EE 92838, which is present and populated. AuditTrailEventListenerConfig was dead the same way, and its AuditAdvice create/delete advices were deleted in the same C-2 cutover to avoid double emission, so CLI-run work emitted no CREATE/DELETE audit events either. Scope is CLI-only: the post-processor is registered nowhere else, so the webapp was always correct. Fix is @lazy(false) on both -- the opt-out the post-processor is built around, already covered by LazyInitByDefaultPostProcessorTest.Bean5. SideEffectConfigurationEagerTest scans both packages for any @configuration implementing InitializingBean that lacks it; verified to fail when the annotation is removed from either config. Backfilling the 76 experiments with lintAcls is still outstanding. The missing audit events cannot be reconstructed.
…irement 271 objects repaired: 46 dangling identities removed, 76 created, 152 base ACEs added. All post-conditions verified read-only -- no experiment lacks an ACL, no dangling EE-class identity or orphaned ACE remains, and none of the 76 was made public. Records the trap that -lintPermissions is opt-in while every other check is hardcoded true, so omitting it creates ACL identities with no entries: access is not restored, but the identity check reports clean.
During dual-version operation, 1.32.x has no mapping for phase2's canonical
acl_* tables, so everything prod creates, deletes or re-permissions leaves the
canonical store stale. The canonical store was populated once by the schema
migration (~2026-05-15) and has received nothing since.
lintAcls cannot close this. It enforces existence and default ACEs and grants
anonymous READ only for shouldBePublic = {ExternalDatabase, Protocol}, so it
cannot replicate per-dataset public/private status. Measured on 2026-08-08: 186
experiments public on prod but private in canonical, and 78,643 identities
missing across all classes.
Writes are confined to acl_class / acl_sid / acl_object_identity / acl_entry.
The legacy store and all entity tables are read-only to this script. Verified
that hotfix-1.32.8 has no runtime reference to the canonical tables -- the only
hits are comments and a historical migration that dropped their predecessors.
Design notes:
- ACE comparison is set-based over distinct (sid, principal, mask, granting).
A count-based diff would flag the ~104 identities where legacy carries
duplicate ACE rows (EE 1 has IS_AUTHENTICATED_ANONYMOUSLY twice, both at
ACE_ORDER 2) which the migration correctly collapsed; copying them back would
also violate UNIQUE(acl_object_identity, ace_order).
- ACEs are replaced wholesale, deduplicated and renumbered, since ace_order must
stay a contiguous 0-based run. Renumbering is order-safe only because both
stores hold zero deny ACEs; the script aborts if one ever appears.
- ace_order is computed with a correlated COUNT: no window functions on MySQL
5.7, and session-variable ordering is not contractual.
- INSERT..SELECT statements carry explicit ORDER BY because prod runs
binlog_format=STATEMENT and the targets are AUTO_INCREMENT.
- Credentials come from MySQL's login-path store; --apply refuses a '-ro' path.
Dry run by default. Delete this script at cutover.
…ntext AclAdviceTest.testExpressionExperimentAcls already deletes an experiment and asserts its ACLs are gone, and it passed for the entire three months the listener was dead in production. It could not have failed: no test context registers LazyInitByDefaultPostProcessor, so every test context is eager, the listener configs always instantiate there, and the listeners always register. The defect was never in the deletion logic. It was in whether the listener was wired at all, which depends on the shape of the context rather than on any ACL code. So the gap was not another deletion assertion but the absence of any test that exercises a lazy-by-default context -- the only shape in which the bug is observable. This builds a context the way CliComponentScanConfig does (lazy PP active, nothing injecting the config) and asserts both listeners still register on their event types. Verified to fail with "Wanted but not invoked" on all five registrations when @lazy(false) is removed from either config. Complements SideEffectConfigurationEagerTest, which guards the annotation generically for future side-effect configs; this pins the behaviour of the two that actually broke.
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
2 participants
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.
No description provided.