[boubaker]

Symptom 1: after the built-in sites (global, dw, myworkspace) are restored with their layout, a displayed site's pages can render empty: the meta site was left with an empty layout.

Symptom 2: an overwrite restore of a site defined by several configurations (global has six: multifactor-authentication, agenda, documents, processes, sites, digital-workplace) logs, for every second configuration (reproduced on a local server):

NewPortalConfig error: Error while committing the transaction [org.hibernate.TransientPropertyValueException: Persistent instance of 'org.exoplatform.portal.jdbc.entity.NodeEntity' references an unsaved transient instance of 'org.exoplatform.portal.jdbc.entity.PageEntity' (persist the transient instance before flushing) [org.exoplatform.portal.jdbc.entity.NodeEntity.page -> org.exoplatform.portal.jdbc.entity.PageEntity]]
Can't find Window with ids: ...

and leaves pages missing, pages whose layout names deleted windows, and the dw site's People and Spaces nodes without a page.

Cause 1: SiteStorageImpl.save replaced a site's layout one kernel DAO call at a time, each @ExoTransactional call committing on its own. A concurrent reader could read the site row while its root container was already deleted and get, and cache, an empty layout. UserPortalConfigService.restoreSite then read the site again after the import and saved that empty layout back over the restored one.

Cause 2: a restore imported each configuration of the site in turn with the requested mode. In overwrite mode, each configuration deleted every page of the site, the pages the previous configurations had just imported included, and re-created its own pages with new identifiers: the nodes of other sites that reference them lost their page (FK_PORTAL_NODE_PAGE is ON DELETE SET NULL). Its page deletion was refused at commit while the page windows were already deleted in their own transactions. In overwrite and restore-defaults mode, each configuration also deleted the nodes the previous ones had imported under the nodes it defines. The startup import did the same for a configuration in overwrite mode, at every start.

Fix:

• @ContainerTransactional(atomic = true) runs the method in one JPA transaction, committed when it returns and rolled back when it throws; when a failed call marked it rollback-only, a method that returned throws RollbackException. Kernel EntityManager writes join it. When the method begins it, Spring transactional methods and Spring Data repositories join it too, and their callbacks run with its outcome. SiteStorageImpl create(PortalData), save(PortalData) and remove(SiteKey) use it.
• The site, layout, page, navigation and description caches evict again once the transaction completes (TransactionCallbacks.afterCompletion), after invalidating the loads in flight; FutureCache.clearFutures invalidates every load it removes.
• A site read whose root container is missing is read again once (SiteDAO#refresh), then flagged layoutUnresolved with an empty layout; a save of it keeps the stored layout.
• restoreSite applies the previous site's settings to the imported config before its save, instead of saving the site a second time.
• A site restore (UserPortalConfigService#restoreSite), and the startup import of a site that one of its configurations overwrites, import the site from all its configurations (NewPortalConfigListener#importSitePages, #importSiteNavigation): an overwrite deletes the pages that no configuration defines, then the pages of every configuration are imported in place; the navigation of every configuration is imported without deleting any node, then NavigationOrphanRemover deletes the nodes that none of them defines, as the mode does (every such node in overwrite mode, the nodes nested in a default node in restore-defaults mode, a node only in the path of a fragment attachment point keeping its children). Nothing is deleted when the pages or the navigation file of a configuration can't be parsed; a file a configuration can't open counts as defining none. All pages are imported before any navigation.
• A restore imports the pages of every configuration of the site, whatever was imported before: the check of NewPortalConfigListener#initPageDB (createdOwners, filled by the startup import and by a site-layout restore) no longer applies to it.
• Each orphan page is removed on its own: a page that can't be removed, as one removed since the orphans were selected, is logged (WARN) and the next ones are removed.
• PageStorageImpl deletes a page, and replaces a page layout, in one transaction, and broadcasts the page event once that write returns.
• A page delete, a page layout save, a page clone, a site save and a site removal read their row again (PageDAO#refresh, SiteDAO#refresh), and work from the layout that row holds. Before, they worked from the page or site as this request first read it, for instance through a node load, which reads the pages its nodes reference; with fix: Refuse to hand on the layout of a site that couldn't be read, and a site restore refreshes its pages' search index EXO-91183 layout#581, a site restore reads the site's navigation before it imports the pages. A layout another request saved since is now removed with the page or site, replaced by a layout save, and copied by a clone.
• PageEntity is @DynamicUpdate. A page row has two writers, its properties save and its layout save, and no version: an update now writes only the columns it changed, so a properties save no longer writes back the page body it read. It reads its row again too, so each property it saves is compared with the row as stored.
• A failing atomic call that joined an active kernel transaction marks it rollback-only and logs a WARN naming the method.
• ExoTransactionalAspect rolls back and throws RollbackException when the method it began a transaction for returns over a rollback-only transaction, as ContainerTransactionalAspect already did for its own transactions. A method that throws keeps its error, unless ending the transaction fails, as before. The EntityManager it started is closed whatever the method's outcome.
• At startup, a configuration whose site type or import mode is unknown is left out of the overwritten sites; on a site no other configuration overwrites, its own import logs the error as before.
• Pinned by ContainerTransactionalAspectTest, ExoTransactionalAspectTest, ContainerTransactionalRepositoryTest (a real Spring Data repository), TransactionCallbacksTest, FutureCacheTest, TestDataStorage (a layout write and a page delete failing midway through the woven advice, a node update evicting the node cache after the commit), SiteDAOTest, TestUserPortalConfigService, TestRestoreSiteConfigurations in the new InitContainer10TestSuite (a restore and a startup import of sites defined by several configurations, a node of another site keeping its page, a configuration of an unknown site type, a pages restore of a site this startup didn't import, an orphan page the request has read and another request saves a layout on during the restore), TestDataStorage (a page layout save, a page properties save, a page clone, a site save and a site removal after another request saved a layout on the page or site the request had read, an update of the page properties that leaves the layout column another request wrote, and a properties save that writes back a property another request changed since the request read the page), NewPortalConfigListenerTest (an orphan page that can't be removed), TestNavigationFragmentImporterRestoreDefaults (now run, in InitContainer1TestSuite), TestSiteDataImportOverwrite and TestImportNavigationOverwrite.

Upgrade impact:

• A page delete, a page layout or properties save, a page clone, and a site save or removal read their row once more (a SELECT by id).
• A site whose stored body names a missing root container is kept, read with an empty layout flagged unresolved (WARN) and cached so; a save of it keeps the stored layout (WARN). A restore with layout, or a save that sets a layout, repairs it.
• restoreSite makes one save per configuration that declares the site: PORTAL_CONFIG_UPDATED, or PORTAL_CONFIG_CREATED for a site the import creates, and none when the import skips the site.
• A write failure now rolls back the whole site create, save or remove. Inside an enclosing kernel transaction, a failed site create, save or remove, page delete or page layout save marks that transaction rollback-only and logs a WARN naming the method.
• An @ExoTransactional method that began the transaction and returns over it marked rollback-only now throws jakarta.persistence.RollbackException after the rollback, instead of returning with none of its writes committed. A method that joined an active transaction doesn't throw: the method that began it does. The transaction is marked by an atomic call above, or by a failing JPA operation (every converted PersistenceException but NoResultException, NonUniqueResultException, LockTimeoutException and QueryTimeoutException). So any such method that catches the failure and returns now fails visibly. The callers checked:
  • JPASettingServiceImpl#set catches every exception of its body: a SettingService#set call made outside a transaction now throws where it logged an ERROR.
  • social LayoutUpgradePlugin#updateLayout: the exception leaves processUpgrade, the upgrades after it don't run, and UpgradeProductService doesn't record the plugin as done. The whole plugin runs again at every start, re-applying the upgrades before the failing one, until that one passes. Before, the failing upgrade's writes were rolled back and the plugin was recorded as done.
  • social MetadataServiceImpl#start: a failing metadata init plugin would fail the start of the portal container; fix: Index no page of a site users don't navigate, and create each init plugin's metadata on its own EXO-91183 social#6245 creates each plugin's metadata in its own transaction.
  • social SocialUserEventListenerImpl#postDelete throws, UserDAOImpl logs it (WARN), and the user is deleted as before.
  • gamification ProgramSpaceListener and commons JPAQueueMessageImpl#send keep their outcome: the exception is dropped by an unread future, or logged by the job (ERROR), and the writes are rolled back as before.
  • A kernel component whose start() makes such a call outside a transaction fails its start (ContainerException), and the portal container is left partly started.
• An overwrite restore with pages keeps the pages and nodes of every configuration of the site; the pages it imports keep their identifier, so the nodes of other sites keep their page. The pages and nodes that no configuration defines are deleted once, after every import for the nodes.
• A restore imports the site layout of every configuration, then their pages, then their navigation: a node that references a page of a later configuration of the site is linked to it.
• A restore-defaults restore with navigation keeps the nodes of every configuration, whatever their order; a node nested in one configuration that another one attaches a fragment to keeps its custom children.
• The search index of a restored site's pages is refreshed by fix: Refuse to hand on the layout of a site that couldn't be read, and a site restore refreshes its pages' search index EXO-91183 layout#581. With this PR alone, an overwrite restore updates the pages in place with the portal's page update event only: their indexed blocks keep their content and permissions until the page is next saved through Layout, or the next full reindex, as the restore-defaults restore already did.
• When the navigation, or the pages, file of one configuration can't be parsed, a restore deletes no node, or no page, of the site. A file a configuration can't open counts as defining none: the pages and nodes only it defines are deleted as orphans, as the previous whole-site removal deleted every page.
• A pages restore without the site layout ("Restore pages") restores the pages of a site that neither this start nor a site-layout restore since then imported (after a restart, a site no configuration overrides); before, it restored only its navigation. A REST restore with layout and pages whose site layout import fails now still imports the pages.
• Startup contract change: when one configuration of a site is in overwrite mode (an extension, or a property such as exo.agenda.portalConfig.metadata.importmode=overwrite), every start that runs that configuration (its override true, or a first start, or a reimport) imports the pages and the navigation of all the configurations of the site, whatever their own mode and override, then deletes the pages and nodes that none of them defines. The pages created in the UI in that site are among them, as before. So are the nodes created in the UI under a node that one of its configurations defines or attaches a fragment to, the root included when one attaches there, as one configuration of every shipped site does; before, a start deleted only those under the attachment points of the overwriting configuration's own fragments. Each configuration's pages are saved from its files in place (layout, state and permissions replaced), and its navigation merged. An extension in overwrite mode no longer deletes the pages and nodes the other configurations of the site define: they are kept and the extension's own are merged over them. On a first start, or a reimport, with an overwrite configuration, the result is the union of the configurations, where the last overwrite used to win.
  • The shipped configurations (meeds sites, digital-workplace, agenda, documents, processes, multifactor-authentication) default to insert or conserve: the change applies to a deployment that sets an overwrite mode.
  • A configuration of that site whose pages.xml or navigation.xml can't be parsed: no page, or no node, is deleted.
  • A configuration of that site with an unknown import mode: its pages and navigation are imported in merge mode; its site layout import still logs the mode.
  • The site layout import, and the shipped defaults, are unchanged.
• A page delete or a page layout save that fails midway rolls back its windows and permissions; before, the page kept a layout naming deleted windows (Can't find Window with ids).

Known limits:

• PAGE_DESTROYED is broadcast before the commit when the page removal joins a site removal (SiteStorageImpl#remove); no production listener.
• A node write drops every navigation and node load in flight, for all sites, before evicting; the after-completion repeat is pinned, the in-flight invalidation is not.
• A page delete, layout save or clone, and a site save or removal read their row without a lock: a layout another request commits between that read and the write is missed.
• PAGE_REMOVED carries the page as LayoutService#remove(PageKey) gets it, from the page cache or from the request's first read of the page: it may lack a layout saved since, which the delete removes.
• A user site's pages are never deleted by the orphan removal, as the page import never did (not reachable today: no user-type configuration ships).
• The template clone (savePortalFromTemplate) keeps the unresolved flag, and create ignores it.
• The in-flight load pin covers the site cache; the clearFutures race fix is not pinned.
• The banner write of the PortalConfig overloads runs before, outside the transaction.
• A reader that straddles the commit still misses containers: a missing root is re-read once, then flagged; a missing nested container is skipped.
• Spring: a Spring write inside an atomic method joined to an outer non-atomic kernel transaction keeps its own transaction. With a Spring synchronization already active and no EntityManager of its own, the atomic transaction collects none of the Spring callbacks. A Spring @CacheEvict evicts at once, before the commit. A Spring call made while the commit flushes (an entity listener) does not join.

Follow-ups: the per-session UIPortal cache is not refreshed on a meta-site layout change; global is displayed on community-dev but not in the packaged configuration; nested container misses; an optional guard in the QA restore hook (meeds-qa-ui#339); a Spring join inside a non-atomic kernel transaction; the web-conferencing pins of task 91299 (exoplatform/web-conferencing#421).

Release order: Meeds-io/social#6245 before this PR, or with it: without it, a failing metadata init plugin fails the start. This PR before the Meeds-io/layout PR of EXO-91183, which refuses to hand on an unresolved site's layout and compiles against PortalConfig#isLayoutUnresolved.

Tests: at the head 65f8e9a240, mvn -B -ntp -o -Dcheckstyle.skip=true -pl component/portal -Dtest='TestDataStorage,TestModelStorage,TestPageService,TestUserPortalConfigService,TestRestoreSiteConfigurations,TestSiteDataImport*,TestImportNavigation*,TestImportFragment*,TestNavigationFragmentImporterRestoreDefaults,NewPortalConfigListenerTest' -Dsurefire.failIfNoSpecifiedTests=false -Djacoco.skip=true test: 18 classes, 148 tests, 0 failures, 0 errors. The full suite (mvn -B -ntp clean verify javadoc:javadoc -Pcoverage) wasn't run at this head.

Knowledge: Meeds-io/eng-standards#409, #560, #586, #587

This change is classified N1 — Kernel ↔ Spring bridge (the atomic @ContainerTransactional transaction that Spring transactional methods and Spring Data repositories join), the outcome of every @ExoTransactional method, the JPA transaction boundaries over the site and page storage writes, the access and edit permissions a restored site keeps, and the irreversible deletion of pages and navigation nodes by a restore and by the startup import. Its approver must be an Archi/Dev who knows it is N1, not an approval on AI review alone; author ≠ approver.

🤖 Generated with Claude Code

[boubaker]

Self-review close-out (AI-assisted loop) — head cf3c9b5

Three reviewer rounds (the last a fresh reviewer: CLEAN — nothing to fix) and twelve delivery-checks passes (the last: 0 entries). cf3c9b5 is the squash of the reviewed head 5f7c162370, same tree.

Reviewer findings

Round	#	Sev	Finding	Status
1	1	🟡	An atomic method whose transaction was marked rollback-only returned normally with nothing committed	✅ Developer ruling "throw after rollback": rolled back, then RollbackException; pinned, mutant killed
1	2	🟡	entityManager.clear() after the rollback was redundant (Hibernate clears on an unsuccessful completion)	✅ Refute pass confirmed; removed
1	3	🟡	atomic() promised one transaction, but Spring Data repositories did not join the kernel EntityManager	✅ Developer ruling "make repositories join now": the thread's EntityManager is bound in Spring's synchronization when the method begins the transaction; pinned on a real Spring Data repository (ContainerTransactionalRepositoryTest) and by the aspect's unit tests, mutants killed
1	4	🟡	New unit tests on JUnit 4	✅ Jupiter + MockitoExtension
1	5	🟢	TransactionCallbacks looks EntityManagerService up in the current container only	✅ Refuted: ExoContainerContext.getService falls back to the portal container
1	6	🟢	SiteDAO.refresh default body did not do what its Javadoc said	✅ Abstract; the in-memory DAO implements it
1	7	🟢	CacheSiteStorage did not override remove(SiteKey)	✅ Developer ruling "fix it here"; pinned, mutant killed
2	1	🟡	SpringTransactionScope branches unpinned (synchronization already active; close before afterCompletion on rollback)	✅ Pinned, mutants killed
2	2	🟢	A transaction callback calling a Spring transactional method failed inside an atomic transaction (holder still bound during commit/rollback)	✅ Developer ruling "release before commit": the scope is released and closed after the before-callbacks, before the commit and both rollbacks; pinned, mutants killed. Accepted consequence: a Spring call made while the commit flushes does not join
2	3	🟢	A Spring @CacheEvict inside an atomic method evicts at once (the platform's Spring cache manager is not transaction-aware)	✅ Developer ruling "fix wording + limit": stated in ContainerTransactional#atomic and in the PR's limits
3	—	—	Closing round	CLEAN — nothing to fix

Delivery checks (passes 1–12)

• Fixed: evictions before the commit → after-completion eviction in the five mop caches; in-flight loads → removeFuture/clearFutures; clearFutures race; remove(SiteKey) made atomic (developer ruling); joined Spring callbacks firing before the commit, an EntityManager still bound during afterCommit, a failed commit reported UNKNOWN → Spring synchronization scope.
• Ruled by the developer: an unresolved site is cached after its one re-read; the template clone keeps the flag (left as is).
• Accepted, stated in the PR body: PAGE_DESTROYED before the commit; the in-flight pin covers the site cache only, and the clearFutures interleaving is not deterministically reachable; the limits of an already-active Spring synchronization.

Verified conform

• The aspect against Spring 7.0.9: JpaTransactionManager sees the bound holder as an existing transaction, a REQUIRED call participates, REQUIRES_NEW suspends and resumes.
• A failed participating call marks the kernel transaction rollback-only.
• A Spring @Transactional call inside an atomic method neither closes the kernel EntityManager nor commits: a following kernel write commits or rolls back with it, pinned on the real engine.
• Atomicity of the site writes is pinned through the woven advice on a kernel container: a layout write failing midway leaves the stored site unchanged (TestDataStorage).
• ACL, REST and schema are untouched.

Evidence

• mvn -B -ntp clean verify javadoc:javadoc -Pcoverage at cf3c9b5 → BUILD SUCCESS, 1688 tests, 0 failures, 0 errors, coverage met (14 modules).

Classification

This change is classified N1 at cf3c9b5 (the Kernel ↔ Spring bridge: ContainerTransactionalAspect / ContainerTransactional#atomic, woven into every addon; and the SiteStorageImpl DAO writes). Its approver must be an Archi/Dev who knows it is N1, not an approval on AI review alone, and not its author.

[MayTekayaa]

This PR also fixes a page upgraded by a LayoutUpgradePlugin being served stale until the next restart (found on Meeds-io/meeds#4293, EXO-91419, which now depends on this PR).

Mechanism: CachePageStorage.save evicts inside the upgrade's @ExoTransactional, before the commit; social's @Asynchronous PageSavedIndexingListener reacts to LayoutService.PAGE_UPDATED with layoutService.getPage(pageKey) on another thread and caches the pre-upgrade page.

Checked on a local 7.3.x-maintenance server, database restored to the same pre-upgrade checkpoint, one restart each:

• base jars: the upgrade stores the new global/news page, GET /layout/rest/pages/layout?pageRef=portal::global::news still returns the old addonContainers;
• this PR's portal.component.{api,common,portal} jars built at d9a953a: the same call returns the upgraded page (content/ContentListView).

[ahamdi]

AI review — Round #1

First independent round, at 2ea8e4201f — a rebase of the self-reviewed cf3c9b53a3 onto 15e587bb1b (identical patch-id). Whole diff read (24 files), Hibernate 7.4.5 / Spring 7.0.9 claims checked against their sources. Two 🟡, two 🟢 inline, plus the body-level items below.

Body-level

• 🟢 Test evidence is on the pre-rebase SHA. The close-out's result line (mvn -B -ntp clean verify javadoc:javadoc -Pcoverage → 1688 tests) ran at cf3c9b53a3; the head now sits on a newer base (EXO-91484). Same patch-id, different base: please post the result line at 2ea8e4201f (the TestDataStorage runs in InitContainer5 and, through TestModelStorage, InitContainer8 included).
• 🟢 The classification line cites a SHA feature/maintenance will never contain. "classified N1 (at cf3c9b5: …)" lands in the squash commit message (squash_merge_commit_message: PR_BODY). Fix: drop "at cf3c9b5" and keep the surfaces.

Verified conform

• Spring bridge (spring-orm/spring-tx 7.0.9): JpaTransactionManager.doGetTransaction picks the bound ThreadEntityManagerHolder, hasTransaction() reads the overridden isTransactionActive(), so a REQUIRED call participates; JpaTransactionObject.setRollbackOnly marks the kernel EntityTransaction; synchronizedWithTransaction = true keeps EntityManagerFactoryUtils from joining/closing the EM. The resource key is the same object on both sides (PersistenceUnitIntegration.entityManagerFactory() returns entityManagerService.getEntityManagerFactory()).
• Aspect paths: every exit of proceedInNewTransaction (begin failure, throw, rollback-only, failed commit, throwing after-commit) reaches end(status), which unbinds and clears; close() is idempotent. Nesting: atomic-in-atomic and atomic-in-@ExoTransactional join; atomic inside a Spring transaction holding its own EM gets NONE.
• Hibernate 7.4.5: local synchronizations fire on commit, failed commit and rollback, and the registry is cleared after each notification (no pile-up on a request-long EM); isActive() is false after a failed commit, so the RollbackException branch reports ROLLED_BACK correctly.
• Cache evictions: all five caches still evict at once and only add the after-completion repeat; TransactionCallbacks returns false with no active transaction, so nothing previously immediate is deferred or lost. The CacheSiteStorage overrides run their eviction after super's advised execution has committed.
• Site read/save: SiteDAOImpl.refresh (detach + find) reads the database (no second-level cache); the layoutUnresolved flag is not copied by restoreSite's keepSiteSettings, so a restore repairs an unresolved site; remove(PortalData) dispatches to the new CacheSiteStorage.remove(SiteKey) override.
• restoreSite: one save per declaring config, PORTAL_CONFIG_UPDATED/CREATED as the Upgrade impact says, none on a skipped import.
• Accepted limits, stated in the body and not re-raised: the template clone ignoring the flag on create, PAGE_DESTROYED broadcast before the commit (no production listener on EventType.PAGE_*), the clearFutures interleaving not pinned.
• Tests: TestDataStorage pins atomicity through the woven advice on a real kernel container; ContainerTransactionalRepositoryTest uses a real Spring Data proxy over HSQLDB; new unit tests on Jupiter + MockitoExtension, matched by surefire's default includes.
• Hygiene: new-file headers only carry 2026; no change history in comments; no i18n change. Knowledge: — Meeds-io/eng-standards#405 merged, #409 a draft carrying this delivery's norm/pitfall update, as expected for code not yet on develop.

What the PR does well: the Spring join is built on the engine's own contracts (isTransactionActive override, synchronizedWithTransaction, scope closed before the commit) rather than around them; every trade-off is either pinned or written in Known limits with precise wording; the in-flight-load race is pinned deterministically with latches; and the fix also closes the stale-page-after-LayoutUpgradePlugin case reported in the thread.

Classification: this change is classified N1 (the Kernel ↔ Spring bridge — ContainerTransactionalAspect / ContainerTransactional#atomic, woven into every addon — and the SiteStorageImpl DAO writes). It must be validated by an Architect/Developer who knows it is N1, not auto-merged on AI review alone, and not approved by its author.

🤖 Generated with Claude Code

[boubaker]

Rebased onto feature/maintenance @ faf570360f (#1344's merge), from 15e587bb1b. The rebase was clean, with no conflict, and every commit keeps its patch-id.

Before	After	Commit
2ea8e4201f (was pushed)	739cbf8cd9	Restoring the built-in sites can leave the main sites with empty pages
d043f2a111	189a82a087	A site restore keeps the pages and navigation nodes of every configuration of the site
6786a226a7	03a4c52eb2	A site that a configuration overwrites at startup is imported from all its configurations
be9f632c1d	927b301845	A failing atomic call logs that the transaction it joined is rolled back
4ced7189db	8a42ffb983	Pin the node cache eviction after the commit and run the fragment restore test
30a81ed517	1fc83a6b45	A method whose transaction was marked rollback-only fails instead of returning
3f4ab7e405	5df08c5600	A site configuration of an unknown type doesn't stop the startup import of the others

Only 2ea8e4201f had been pushed; the six others are new on this push. After the rebase, the transaction tests of component/common, #1344's EntityManagerHolderTest included, pass: 43 tests, 0 failures.

[boubaker]

On review 5572003775's body items, at 5df08c5600:

• Result line: the body's Tests line now gives the full module close-out (1720 tests) and the transaction tests run after the rebase (43).
• Classification line: it no longer cites a SHA. The level, N1, was recomputed on the final diff; its triggers are in the line.

@MayTekayaa, for EXO-91419: the eviction your upgrade case relies on is unchanged. CachePageStorage evicts again once the transaction completes (TransactionCallbacks.afterCompletion), pinned by TestDataStorage. One change since your check at d9a953a touches the same path: an @ExoTransactional method that returns over a transaction a failed call marked rollback-only now throws RollbackException (1fc83a6). So if a site or page write fails inside LayoutUpgradePlugin#updateLayout, the upgrade now fails visibly (processUpgrade stops at that entry) instead of ending with that entry's writes rolled back. The body's Upgrade impact has the details.

[ahamdi]

AI review — Round #2 (follow-up)

Reviewed at 5df08c5600. The branch was rebased onto faf570360f (#1344's merge); 739cbf8cd9 is patch-identical to the Round #1 head and was not re-read. The six new commits (189a82a087 … 5df08c5600, 37 files) and the live PR body were read in full.

Status of Round #1

#	Finding	Status
1	🟡 A joined atomic call dooms the caller's transaction silently	➖ The marking is accepted, per the thread reply. The WARN is in (ContainerTransactionalAspect.java:110-115) and Upgrade impact lists the marking. The outcome change 1fc83a6b45 adds on top of it is new finding R2-1 below.
2	🟡 Nested types SpringTransactionScope / ThreadEntityManagerHolder	✅ Fixed in 927b301845, verified: both are top-level package-private classes in io.meeds.common.
3	🟢 Spring scope not asserted closed after the call; begin() failure unpinned	✅ Fixed in 927b301845, verified: post-call isSpringScopeClosed() in the commit and rollback tests, plus testCloseTheSpringScopeWhenTheTransactionCantBegin.
4	🟢 Node-write evictions unpinned	✅ Fixed in 8a42ffb983, verified: TestDataStorage#testUpdatedNodeCacheIsClearedAgainOnceTheTransactionCompletes. The in-flight invalidation stays a stated Known limit.
5	🟢 Test evidence not at head	❌ Still open, see below.
6	🟢 Classification line cites cf3c9b53a3	✅ Fixed: the line names no SHA.

Body-level

• 🟡 Knowledge: pending — EXO-91183: not approvable until replaced (pr-conventions.md §3). The line must name the corpus PR(s) this delivery owes (the new restore/startup contract, the ExoTransactional outcome change), or none — issue-first|doc-neutral. That update has to amend the pending pitfall an-entitymanager-exception-caught-inside-an-exotransactional-transaction-rolls-it-back-without-a-word (Meeds-io/eng-standards#560, draft). Its title and Check ("rolls the whole transaction back at its end, and nothing is thrown") turn false with 1fc83a6b45.

• 🟡 The Fix bullet on unknown import modes contradicts the code. The bullet says "At startup, a configuration whose site type or import mode is unknown is left out of the sites imported from all their configurations, and imported on its own"; commit 5df08c5600 says the same. That holds for an unknown type only. For an unknown mode:

  • getOverwrittenSites skips only that configuration, so its site stays overwritten;
  • withoutSites removes the site from the configuration's own initPageDB / initPageNavigationDB (NewPortalConfigListener.java:233-248);
  • getPortalConfigs still returns it, so importSitePages / importSiteNavigation import its pages and navigation in MERGE, with no error logged.

  The Startup contract change sub-bullet states this correctly, so the two bullets disagree. Fix: scope the Fix bullet (and the commit message) to an unknown site type.

• 🟢 R1-5, still open: test evidence is not at head. The full run (1720 tests) "ran on the tree of the head before the rebase onto #1344's merge" and names no SHA. The 43-test line after the rebase covers only component/common: TestRestoreSiteConfigurations, TestDataStorage and the import tests have no result at 5df08c5600, which now sits on #1344's change to the DAO proxy's exceptions. Fix: post the result line of the component/portal suites (InitContainer1, 5, 8, 10 at least) at 5df08c5600.

• 🟢 Follow-ups describe a state the base has already ended. "web-conferencing task 91299 (kernel DAO JPA exceptions arrive wrapped, so its storage error handling never runs)": #1344 fixed that, and its merge faf570360f is this PR's base. Fix: drop the item, or reword it to what is left on the web-conferencing side.

• 🟢 The Startup contract change should say outright that admin-created content goes. "then deletes the pages and nodes that none of them defines" is accurate. On an irreversible-deletion surface, say plainly that this includes the pages and nodes administrators created in the UI in that site, at every start. That behaviour is unchanged from the previous PageImporter OVERWRITE removePages and navigation destroyOrphan. Fix: add "pages and nodes created in the UI in that site are deleted at every start, as before".

Verified conform

• ExoTransactionalAspect, every path (ExoTransactionalAspectTest pins all six through the package-private proceed seam):
  • a method that throws keeps its own exception, and a commit failure still replaces it;
  • only a method that returned over a rollback-only transaction it began throws RollbackException; a joined call never does;
  • the EntityManager the advice started is closed in finally on every outcome.
• ContainerTransactionalAspect after the extraction: behaviour-identical, plus the WARN.
• PageStorageImpl: deletePage / savePageLayout are atomic with their former bodies, and the PAGE_DESTROYED / PAGE_UPDATED broadcast comes after the atomic call returns. TestDataStorage pins a page delete and a layout save failing midway through the woven advice.
• Restore model:
  • every configuration is imported without deleting, then what none defines is deleted once;
  • NavigationOrphanRemover / DefinedNavigationNode reproduce ImportMode's destroyOrphan / destroyNestedOrphan and the fragment importer's top-level depth rule;
  • user sites are skipped by the page-orphan removal;
  • all pages are imported before any navigation;
  • a configuration whose pages.xml / navigation.xml fails to parse aborts the deletion: getConfig throws and the outer catch deletes nothing.
• Startup: an unknown site type no longer aborts the startup import (fixture contribution9).
• Tests run: InitContainer10TestSuite matches surefire's **/*TestSuite.java include. TestNavigationFragmentImporterRestoreDefaults now runs in InitContainer1TestSuite. TestRestoreSiteConfigurations asserts what the body claims: a later configuration's page is linked, another site's node keeps its page across an overwrite and a startup, nested custom nodes are removed or kept per mode, and nothing is deleted on an unparsable configuration.

What the push does well: the restore is rebuilt around one rule: import everything without deleting, then delete once what no configuration defines. That removes the order dependence behind Symptom 2 rather than patching each mode. The fixtures are precise (a cross-site page reference, a node pointing at a later configuration's page, a malformed file, an unknown-type configuration). The ExoTransactionalAspect change is small and keeps every prior outcome except the one it targets, pinned path by path.

Classification: this change is classified N1, as the body states: the Kernel ↔ Spring bridge, the outcome of every @ExoTransactional method, the JPA transaction boundaries of the site and page writes, and the irreversible deletion of pages and navigation nodes by a restore and by the startup import. It must be validated by an Architect/Developer who knows it is N1, not auto-merged on AI review alone, and not approved by its author.

🤖 Generated with Claude Code

[boubaker]

Rebased onto feature/maintenance @ 7b35dfa4c2, from faf570360f. The base was force-pushed on 2026-10-19 with the same commits and the same tree (git diff faf570360f 7b35dfa4c2 is empty), and git range-diff maps every commit =:

• 739cbf8cd9 → 67c8f20eb8
• 189a82a087 → 83cfd0a499
• 03a4c52eb2 → 3f9bae219d
• 927b301845 → 377f166866
• 8a42ffb983 → edbf69361f
• 1fc83a6b45 → 1432792e05
• 5df08c5600 → 6ac620b18a

Then four new commits:

• e793a92f50: a restore imports the pages of every configuration of the site, whatever was imported before. "Restore pages" after a restart restored only the navigation of a site no configuration overrides.
• dca02911cd: the Javadoc of @ExoTransactional and atomic says that a method that returned throws a RollbackException, and that a method that threw keeps its exception, unless ending the transaction fails.
• 982bcbb527: "can't be parsed" (R2-3).
• 5a4f5ef206: each orphan page is removed on its own (R2-2).

Meeds-io/social#6245 goes with this PR: without it, a failing metadata init plugin fails the start.

[boubaker]

A correction to my comment 6271760414 for EXO-91419: the outcome "a page write fails, so the upgrade fails visibly" needs two commits, not one.

• 83cfd0a499 makes the page layout save atomic. Inside updateLayout's transaction, any failure of that save marks the transaction rollback-only. Before it, only an exception that Hibernate converted did.
• 1432792e05 makes the method that began that transaction throw.

A site write needs 1432792e05 only: the site save was already atomic when you checked.

[boubaker]

Self-review close-out — rounds 4 to 10

Rounds 1 to 3 were closed out in an earlier comment. Rounds 4 to 9 ended NOT CLEAN; round 10, a fresh reviewer continued by message to verify its own items, ended CLEAN. The last rounds found no defect in the code paths, only wordings of this body. DC rows come from the delivery-checks pass run before each round, ext rows from the external review. The SHAs below are those of the pushed head 5a4f5ef206; the rebase map is in comments 6271750741 and 6285362306.

Status table

Round	#	Sev	Finding	Status
4	1	🟡	TestRestoreSiteConfigurations was in no test suite, so the build never ran the restore pins	✅ Fixed 83cfd0a499 (InitContainer10TestSuite, its own container configuration)
4	2	🟡	The PR body described the first commit only, and one stated limit (PAGE_DESTROYED before the commit) was no longer true	✅ Fixed (body)
4	3	🟡	An OVERWRITE restore deleted and re-created pages, so nodes of other sites lost their link to them (SET NULL on the foreign key)	✅ Fixed 83cfd0a499: pages no configuration defines are removed, the others are merged in place and keep their id
4	4	🟢	The restore pins compared names, not that a restored node points at its page	✅ Fixed 83cfd0a499 (page reference asserted)
4	DC1	🟡	A page layout update was not atomic	✅ Fixed 83cfd0a499 (savePageLayout, pinned)
4	DC1	🟡	RESTORE_DEFAULTS deleted nested nodes of earlier configurations, depending on their order	✅ Fixed 83cfd0a499: nodes are merged for every configuration, then one sweep against their union
4	DC1	🟢	getConfig outside the try, page event inside the transaction, unlabelled claim in the commit message	✅ Fixed 83cfd0a499
4	DC2	🟡	The path ancestors of an attachment point were swept	✅ Fixed 83cfd0a499 (DefinedNavigationNode#childrenDefined, pinned)
4	DC2	🟢	Wrong administration example and a false "after that transaction" in the commit message	✅ Fixed 83cfd0a499
4	DC3	🟢	Symptom sentence limited to one navigation input	✅ Fixed 83cfd0a499
5	1	🟡	An in-place pages restore left the search index stale (removed blocks searchable, old permissions kept)	✅ Fixed in Meeds-io/layout#581 01d1b2bd; the effect is stated in the body
5	2	🟡	Body: Upgrade impact for each restore and startup outcome, joined failure, limits, pins, result line	✅ Fixed (body)
5	3	🟢	One misdeclared ownerType stopped the page and navigation import of every site at startup	✅ Fixed 6ac620b18a (the declaration is skipped, pinned with an unknown-type configuration)
5	4	🟢	Three corpus notes contradicted at head	✅ Corpus PRs opened (Knowledge: line)
5	ext 4314179373	🟡	A failing joined atomic call marks the enclosing transaction rollback-only: Upgrade impact and a question	⏳ Waits on its reviewer. The rollback-only marking is kept, since a joined call can't commit apart from the transaction it joined (argued in reply 4322529815); a WARN names the method (377f166866), and the body lists the outcome
5	ext 4314179388	🟡	Nested types in the aspect	✅ Fixed 377f166866 (two package-private top-level classes)
5	ext 4314179403	🟢	Spring scope not asserted closed; begin-failure path untested	✅ Fixed 377f166866 (ContainerTransactionalAspectTest)
5	ext 4314179417	🟢	Node-write eviction unpinned	✅ Fixed edbf69361f; the in-flight load limit is in Known limits (body)
5	ext body	🟢	Result line at head; classification line cited an old SHA	✅ Fixed (body)
5	DC4	🟢	Startup left the other declarations of an overwritten site to import on their own	✅ Fixed 3f9bae219d: every declaration of the site is imported in place (pages first, then navigation, one sweep)
5	DC4	🟢	Concurrently deleted orphan page: NPE stops the removal of the others	✅ Fixed 5a4f5ef206 (per-page catch, pinned; see round 8 R2-2)
5	DC5	🟡	Startup and restore order change unpinned	✅ Fixed 83cfd0a499 (node23 pin)
5	DC5	🟡	Body: SHAs, result lines, Upgrade impact	✅ Fixed (body)
5	DC5	🟢	Commit message overstated importSiteLayout; one fixture not in insert mode	✅ Fixed 3f9bae219d
6	2	🟢	@ExoTransactional Javadoc did not state the new outcome	✅ Fixed 1432792e05
6	2b	🟢	Body: Upgrade impact of the throw input by input, "closed on every outcome", result line	✅ Fixed (body)
6	2b	🟢	Corpus notes to amend for the throw	✅ Corpus PRs opened (Knowledge: line)
6	DC6/7	🟢	Claims reworded in messages (cause, "keeps its own error", log unchanged) and a stale comment in the aspect	✅ Fixed 1432792e05
7	1	🟡	A kernel component whose @ExoTransactional start() swallows a failed JPA call would now fail the container start	✅ Fixed in Meeds-io/social#6245 c2106c8844 (the caller loses the annotation); the body names the startup consequence
7	2	🟡	A reply named JPASettingServiceImpl#set as an example the body did not carry	✅ Fixed (body)
7	3	🟢	Four inaccurate body sentences (EntityManager closing, which method throws, unknown import mode, stale follow-up)	✅ Fixed (body)
7	4	🟢	Javadoc said a method that threw also throws RollbackException	✅ Fixed dca02911cd
7	5	🟢	A comment credited one commit for an outcome that comes from two	✅ Fixed (correction, comment 6285363054)
7	6	🟢	Result lines named no SHA	✅ Fixed (body: head and its tree)
7	DC	🟡	A pages-only restore imported no page for a site the startup had not imported (startup-only gate)	✅ Fixed e793a92f50, pinned
8	1	🟢	Body described the removed gate as startup-only, and missed the REST layout-plus-pages case	✅ Fixed (body)
8	2	🟢	Admin-created content deleted at start was not said	✅ Fixed (body)
8	R2-1 4317410794	🟡	Callers of the new throw (external round 2)	⏳ Waits on its reviewer. Documented in the body with each caller's outcome (options argued in reply 4322530136). The upgrade plugin's re-run is kept: a failed plugin is retried at the next start, where its rolled-back upgrade used to be recorded as done
8	R2-2 4317410817	🟡	A page deleted concurrently during orphan removal	✅ Fixed 5a4f5ef206 (per-page catch, pinned)
8	R2-3 4317410827	🟢	"can't be read" where the guard is "can't be parsed"	✅ Fixed 982bcbb527 and body
8	DC	🟢	Body wording ("can't be read", scope of the startup contract, refresh timing)	✅ Fixed (body)
9	1	🟡	Body claimed nodes created in the UI are deleted "as before"; not true when the overwriting fragment has a parent-uri	✅ Fixed (body)
9	2	🟡	Tests line named a SHA absent from GitHub and told the rebase history	✅ Fixed (body: head and tree c1852b61fa)
10	2	🟢	Startup contract: the root counts only when a fragment attaches there	✅ Fixed (body)

Verified conform

The last two rounds checked the body's every new sentence against the code at the pushed head. They found these hold: the startup and restore contract (pages in place, one orphan removal, nothing removed when a configuration can't be parsed), the root rule of the navigation sweep (DefinedNavigationNode, NavigationOrphanRemover), the joined-failure outcomes and the cross-addon callers of the throw, the head's tree being the one the Tests line names, and the per-page catch against LayoutServiceImpl#remove(PageKey). No product text cites a role or a ruling as a reason. Every item of round 8 is closed by what is live.

Tests

mvn -B -ntp clean verify javadoc:javadoc -Pcoverage on the tree of the head 5a4f5ef206 (c1852b61fa): BUILD SUCCESS, 1727 tests, 0 failures, 0 errors.

Classification

This change is classified N1 at 5a4f5ef206 (Kernel ↔ Spring bridge, outcome of every @ExoTransactional method, JPA boundaries over the site and page storage writes, irreversible deletion of pages and nodes at restore and at startup) — its approver must be an Archi/Dev who knows it is N1, not an approval on AI review alone; author ≠ approver.

Open items for a human

• Threads 4314179373 and 4317410794 wait on their reviewer.
• Merge order stated in the bodies: fix: Index no page of a site users don't navigate, and create each init plugin's metadata on its own EXO-91183 social#6245 before or with this PR, this PR before fix: Refuse to hand on the layout of a site that couldn't be read, and a site restore refreshes its pages' search index EXO-91183 layout#581, and fix: The administration declaration's comment describes the overwrite reload EXO-91183 exoplatform/digital-workplace#232 (the comment this PR made stale) with or after this PR.

🤖 Generated with Claude Code

[ahamdi]

AI review — Round #3 (follow-up)

Reviewed at 5a4f5ef206.

• The rebase changed nothing. The branch was rebased onto 7b35dfa4c2, the force-pushed feature/maintenance: git diff faf570360f 7b35dfa4c2 is empty, and git range-diff maps the seven reviewed commits =.
• Read in full: the four new commits (e793a92f50, dca02911cd, 982bcbb527, 5a4f5ef206) and the live PR body.

Status of previous findings

#	Finding	Status
R1-1	🟡 A joined atomic call marks the caller's transaction rollback-only	➖ Architects Lead decision; the author's reasons are in the thread. The thread is left for a human to close.
R1-2 – R1-4, R1-6	Nested types, Spring-scope pins, node-eviction pin, classification SHA	✅ (Round #2)
R1-5	🟢 Test evidence not at head	✅ The Tests line runs on the tree of 5a4f5ef206 (c1852b61fa, matched against HEAD^{tree}): 1727 tests, 0 failures.
R2-1	🟡 Callers of the new RollbackException missing from Upgrade impact	✅ for the body, ➖ for the shipping. The body now names the class of callers and each caller's outcome. I verified the outcomes in their repos on feature/maintenance. Social's UserDAOImpl runs each listener in an organization-service request, not a JPA transaction, so the listener's own transaction throws and its exception is logged as a WARN. Gamification's ProgramSpaceListener runs in runAsync, whose future nobody reads. Commons' SendEmailNotificationJob catches it and logs an ERROR. Architect decision: the throw ships for every caller, as argued in the thread. MetadataServiceImpl#start is handled by Meeds-io/social#6245, which the Release order puts before or with this PR.
R2 orphan race	🟡 A concurrently removed orphan page stopped the removal of the others	✅ Fixed in 5a4f5ef206, verified: per-page try with a WARN, pinned by NewPortalConfigListenerTest#testOverwriteRemovesTheOtherOrphanPagesWhenOneCantBeRemoved.
R2-2	🟢 "can't be read" vs "can't be parsed"	✅ Fixed in 982bcbb527, verified: both Javadocs and the body say "can't be parsed", and that an unopenable file defines nothing.
R2 body	🟡 Fix bullet on unknown import modes	✅ Now scoped: "left out of the overwritten sites; on a site no other configuration overwrites, its own import logs the error". It matches the Startup contract change sub-bullet.
R2 body	🟢 Stale web-conferencing follow-up	✅ Now points at exoplatform/web-conferencing#421.
R2 body	🟢 Admin-created content deleted at start	✅ The Startup contract change says it plainly, including the broader node sweep, which is a change from before.
R2 body	🟡 Knowledge: pending	❌ Partly: see below.

Body-level

• 🟡 The Knowledge: line misses two corpus updates this PR owes. The line now reads Meeds-io/eng-standards#586, #587 (drafts: the restore and startup pitfalls, and the portal, layout and digital-workplace domain docs). Two things are missing:

  • Meeds-io/eng-standards#409 is no longer named. It is still open, and it carries ADR-0010 and the backend-spring.md §4 / container-transactional-opens-no-database-transaction rewrite for @ContainerTransactional(atomic = true), which this PR introduces.
  • No open corpus PR amends the pitfall this PR makes false. an-entitymanager-exception-caught-inside-an-exotransactional-transaction-rolls-it-back-without-a-word (Meeds-io/eng-standards#560, draft, at de71ed7e) is still titled "…rolls the whole transaction back at its end, and nothing is thrown". Since 1432792e05, the method that began the transaction throws RollbackException. Neither #586, #587 nor #409 touches that file, and #409's norm and ADR say nothing about the @ExoTransactional throw.

  Fix: name #409 again, and amend that pitfall (in #560, or in one of this delivery's PRs) to state the new outcome; then list the PR that carries it in the line.

Verified conform

• importSitePages now calls createPage per configuration (e793a92f50), without initPageDB's createdOwners gate:
  • on a restore, that is the fix: a site no configuration overrides is no longer skipped after a restart;
  • at startup, the gate still holds, because getOverwrittenSites only selects owners in createdOwners;
  • createPage opens its own RequestLifeCycle, and a configuration that fails is logged per configuration and doesn't stop the others;
  • pinned by TestRestoreSiteConfigurations#testRestorePagesImportsThePagesOfASiteThatTheStartupDidntImport.
• @ExoTransactional and @ContainerTransactional#atomic Javadoc (dca02911cd) match the aspects. A method that returned throws RollbackException. A method that threw keeps its exception, unless ending the transaction fails. A joined method never throws it.
• The caller list in the Upgrade impact, as above.

What this push does well: every Round #2 item is answered in the code or the body, and the replies give each decision's reasons rather than its outcome alone. The restore-pages gap found by the author's own loop (e793a92f50) is fixed with a pin that reproduces the restart condition.

Classification: this change is classified N1: the Kernel ↔ Spring bridge, the outcome of every @ExoTransactional method, the JPA transaction boundaries of the site and page writes, and the irreversible deletion of pages and navigation nodes by a restore and at startup. It must be validated by an Architect/Developer who knows it is N1, not auto-merged on AI review alone, and not approved by its author.

🤖 Generated with Claude Code

[boubaker]

Self-review close-out — round 11

Round 11 answers the external nit 4323707015. It has no reviewer round. Instead, two delivery-checks passes and a refuter covered the change, and their entries are fixed or listed below. The head is 2d20d07490.

The branch was rebased onto feature/maintenance at 3fd575ffd7 (#1345) before this push. git range-diff shows every commit unchanged (=). Old → new:

• 67c8f20eb8 → 2433e66295
• 83cfd0a499 → 32c86aae4b
• 3f9bae219d → 0364c31806
• 377f166866 → f985294c10
• edbf69361f → 223ed9abb5
• 1432792e05 → 53bfd23e18
• 6ac620b18a → 942212174c
• e793a92f50 → 809c5a0471
• dca02911cd → 96d2d0becc
• 982bcbb527 → 4997786116
• 5a4f5ef206 → 4a86fed897

Status table

Round	#	Sev	Finding	Status
11	ext 4323707015	🟢	The Javadoc of the orphan removal doesn't state the window between the selection and each removal	✅ Fixed 2d20d07490. The sentence is adopted with three precisions, argued in reply 4328347768, and pinned on a real engine
11	DC1	🟠	An orphan page the request had already read was deleted using the body Hibernate kept for it. Two reads load it: the findPages selection, and a REST restore's navigation read (getReachablePages → buildNodeData → page.getProfiles()). A layout another request saved on it meanwhile stayed in storage, unreferenced. The refuter CONFIRMED it	✅ Fixed 2d20d07490: PageStorageImpl#deletePage reads the row again (PageDAO#refresh). Pinned by TestRestoreSiteConfigurations#testOverwriteRemovesTheLayoutSavedOnAnOrphanPageSinceTheRequestReadIt; dropping the refresh fails it
11	DC1	🟢	LayoutServiceImpl#removePages loads every page, then removes each	✅ Fixed 2d20d07490: every page delete walks the stored row
11	DC2	🟡	A Known limit stated a defect the code doesn't have: the page body nests every container's children	✅ Fixed (body)
11	DC2	🟡	A page layout replace and the site writes walk the body the request first read: an editor saving during a restore, or two concurrent saves	⏳ Known limits and Follow-ups (body). A re-read on every page save is a hot-path change and needs its own review
11	DC2	🟢	PAGE_REMOVED payload age; the commit's engine mechanism; the window left between the re-read and the delete	✅ Fixed (body, commit message, deletePage Javadoc)

Tests

• Targeted: InitContainer10TestSuite 8/8, NewPortalConfigListenerTest 2/2, TestDataStorage 29/29.
• Full: mvn -B -ntp clean verify javadoc:javadoc -Pcoverage on the tree c32ee4f22f, which is the head without fix: A draft site no longer opens to a user whose name ends its owner's EXO-91427 #1345's UserACL change: BUILD SUCCESS, 1728 tests, 0 failures, 0 errors.

Classification

This change is classified N1 at 2d20d07490 (the Kernel ↔ Spring bridge, the JPA boundaries and DAO reads over the site and page storage writes, and the irreversible deletion of pages and nodes at restore and at startup). Its approver must be an Archi/Dev who knows it is N1, not an approval on AI review alone; author ≠ approver.

Open items for a human

• Thread 4323707015 waits on its reviewer.
• Follow-up: the page layout replace and the site writes should read their row again, as a page delete does.

🤖 Generated with Claude Code

[ahamdi]

AI review — Round #4 (final)

Reviewed at 2d20d07490.

• The rebase changed nothing. The branch was rebased onto feature/maintenance @ 3fd575ffd7 (#1345's merge), and git range-diff maps the eleven reviewed commits =.
• Read in full: the one new commit 2d20d07490 and the live PR body.

Status of previous findings

#	Finding	Status
R1-1	Joined atomic call marks the caller's transaction rollback-only	➖ Architects Lead decision; the thread is left for a human to close
R1-2 – R1-6	Nested types, Spring-scope and node-eviction pins, test evidence, classification SHA	✅ (Rounds #2–#3)
R2-1	Callers of the new RollbackException	➖ Architect decision (ships for every caller); the body lists each caller's outcome. The thread is left for a human to close
R2 orphan race, R2-2, R2 body items	Per-page removal, "can't be parsed", unknown import modes, follow-ups, admin-created content	✅ (Round #3)
R3	🟡 Knowledge: line	✅ Now Meeds-io/eng-standards#409, #560, #586, #587 (all drafts, as expected for code not yet on develop). #409 is named again. The #560 pitfall is retitled "…and the method that began it throws RollbackException", matching 1432792e05.
R3	🟢 Javadoc of the orphan selection window	✅ Fixed in 2d20d07490, verified: the removePageOrphans Javadoc states the single selection, the per-page re-read, and that a page created since is kept unless it is named after an orphan

Verified conform: 2d20d07490

• PageStorageImpl#deletePage re-reads the page row. It calls PageDAO#refresh, built like SiteDAO#refresh: detach, then find. So it walks the layout as stored, not the state the request's persistence context kept from an earlier read (the findPages selection, or a restore's navigation read).
  • It runs inside the atomic transaction, and null when the page is already gone returns false, as before.
  • The in-memory test DAO implements refresh.
• The pin is TestRestoreSiteConfigurations#testOverwriteRemovesTheLayoutSavedOnAnOrphanPageSinceTheRequestReadIt, in InitContainer10TestSuite, on a real container. A PAGE_REMOVED listener saves a container on the other orphan from another request between the two removals, and the test asserts that container is gone after the restore.
• The windows that remain are stated in Known limits: the re-read takes no lock, a page layout replace and the site writes still walk the first-read body, and the PAGE_REMOVED payload can be older than the row the delete removes. The layout replace and site writes are also listed as a follow-up. This is accepted as stated, given the hot-path cost the body names.

Note (non-blocking)

• The full-suite result line names tree c32ee4f22f, described as "the head without the UserACL change of #1345". That object can't be found locally or on GitHub, and the head's tree is 9303ebe813. Each part is evidenced at its own head (this PR's suite, and #1345's suite at de4674777c), but the combined tree isn't. A result line at 2d20d07490 would close it.

What the push does well: pinning the Javadoc nit on a real engine turned up a real defect (the stale-body delete), which the author fixed at its root, deletePage, so every page delete benefits, not only the restore. Every window that remains is written down with its reason.

All findings from previous rounds are resolved or recorded as owner decisions; nothing is outstanding from the AI review side.

Classification: this change is classified N1: the Kernel ↔ Spring bridge, the outcome of every @ExoTransactional method, the JPA transaction boundaries and DAO reads of the site and page writes, and the irreversible deletion of pages and navigation nodes by a restore and at startup. It must be validated by an Architect/Developer who knows it is N1, not auto-merged on AI review alone, and not approved by its author.

🤖 Generated with Claude Code

[boubaker]

Self-review close-out — round 12

Head a2991682af, which adds one commit after the approved 2d20d07490. That delta re-enters classification and needs its approval again.

The commit extends deletePage's re-read to the five other page and site writes that walked a body the request had read earlier: the page layout save, the page properties save, the page clone, the site save and the site removal. Before, they kept Known limits and a follow-up; they are now fixed in this PR. One read-only delivery-checks pass covered the commit, and no reviewer agent ran.

Status table

Round	#	Sev	Finding	Status
12	—	🟡	PageStorageImpl#savePageLayout, #savePage(PageContext) and #clone, and SiteStorageImpl#save(PortalData) and #remove(SiteKey), worked from the entity as the request first read it. The layout save and the site writes left another request's rows unreferenced. The properties save wrote the earlier page body back, since it writes every column. The clone copied the earlier layout	✅ Fixed a2991682af: each write re-reads its row (PageDAO#refresh, SiteDAO#refresh). Five TestDataStorage pins; dropping the five re-reads fails exactly those five
12	DC	🟢	The no-lock limit understated the properties save, which overwrites rather than misses	✅ Fixed (body Known limits, commit message)
12	DC	🟢	The navigation read before the page import comes from Meeds-io/layout#581, not from this PR	✅ Fixed (body)
12	DC	🟢	Three pins didn't prove the other request's save landed	✅ Fixed a2991682af: both helpers assert the stored layout grew by one
12	DC	🟢	savePage and clone re-read outside a transaction when the caller has none	➖ Kept. The re-read takes no lock inside or outside a transaction, so the window is the one Known limits states
12	DC	🟢	The targeted result line gave no command	✅ Fixed (body Tests)
ext R4	note	—	The full-suite line named a tree that isn't the head	✅ The Tests line now gives the targeted run at a2991682af with its command, and says the full suite wasn't run at this head

Tests

At a2991682af, 18 classes of the page and site storage, the imports and the site configuration ran: 144 tests, 0 failures, 0 errors. The command is in the body. TestDataStorage ran 34 tests, 5 of them new.

Classification

This change is classified N1 at a2991682af: the Kernel ↔ Spring bridge, the JPA boundaries and DAO reads of the site and page storage writes, and the irreversible deletion of pages and nodes at restore and at startup. Its approver must be an Archi/Dev who knows it is N1, not an approval on AI review alone; author ≠ approver.

🤖 Generated with Claude Code

[boubaker]

Self-review close-out — round 13

Head 3b43334f47, which adds one commit after a2991682af. Its approval is owed with round 12's.

A page row has two writers, its properties save and its layout save, and no version. That is backend-spring §4's multi-writer case, so PageEntity is now @DynamicUpdate: a properties save writes no PAGE_BODY, even between its re-read and its update. SiteEntity has a single writer, SiteStorageImpl#save, and stays as it is. One read-only delivery-checks pass covered the commit, and no reviewer agent ran.

Status table

Round	#	Sev	Finding	Status
13	—	🟡	PageEntity has two writers and no @Version, so each update wrote every column. In the window between its re-read and its update, a properties save wrote the page body it had read over a layout committed meanwhile	✅ Fixed 3b43334f47: @DynamicUpdate, pinned by TestDataStorage#testPageRowUpdateWritesOnlyTheColumnsItChanged (real SQL; without the annotation, the stored body loses the other request's container)
13	DC	🟡	The comment on savePage's re-read still said that the update writes every column	✅ Fixed 3b43334f47: the re-read now compares each property with the stored row
13	DC	🟢	The no-lock limit still counted the properties save, and ended in a stray ,.	✅ Fixed (body Known limits)
13	DC	—	Sweep: does any path rely on a full-column UPDATE of PageEntity?	✅ None. The two updates (:159, :367) both re-read; the other writes are inserts, deletes and the bulk JPQL delete; data-upgrade's native PROFILES update bypasses the entity

Tests

The mutation ran on the tree before a comment-only amend. At the head 3b43334f47, the targeted run of the body's Tests line passed: 18 classes, 146 tests, 0 failures, 0 errors (TestModelStorage extends TestDataStorage, so it runs the new pin too).

Classification

This change is classified N1 at 3b43334f47: the Kernel ↔ Spring bridge, the JPA entity and the DAO reads of the site and page storage writes, and the irreversible deletion of pages and nodes at restore and at startup. Its approver must be an Archi/Dev who knows it is N1, not an approval on AI review alone; author ≠ approver.

🤖 Generated with Claude Code

[boubaker]

Self-review close-out — round 14

Head 65f8e9a240, which adds one test-only commit after 3b43334f47.

Round	#	Sev	Finding	Status
14	corpus verify	🟡	With @DynamicUpdate, testSavePagePropertiesKeepsTheLayoutSavedSinceTheRequestReadThePage no longer pins the properties save's re-read: dropping that re-read leaves it passing. What the re-read still guards, comparing each property with the stored row, had no pin	✅ Fixed 65f8e9a240: TestDataStorage#testSavePagePropertiesWritesBackAPropertyChangedSinceTheRequestReadThePage. Another request changes the display name, and the request saves back the value it first read. Dropping only that re-read fails this test alone (Other stored instead of Original)

Tests at 65f8e9a240: the body's targeted command, 18 classes, 148 tests, 0 failures, 0 errors.

This change is classified N1 at 65f8e9a240, unchanged by a test-only commit. Its approver must be an Archi/Dev who knows it is N1, not an approval on AI review alone; author ≠ approver.

🤖 Generated with Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[ahamdi]

AI review — Round #5 (final)

Reviewed at 65f8e9a240. The branch was not rebased: 2d20d07490 (the Round #4 head) is an ancestor of the head. Read in full: the three new commits (a2991682af, 3b43334f47 and 65f8e9a240), PageDAOImpl#refresh and SiteDAOImpl#refresh, the mappings of PageEntity and NodeEntity, and the live PR body. A delivery-checks pass over 2d20d07490..65f8e9a240 returned no entry.

Status of previous findings

#	Finding	Status
R1-1	A joined atomic call marks the caller's transaction rollback-only	➖ Architects Lead decision
R1-2 – R1-6	Nested types, Spring-scope and node-eviction pins, test evidence, classification SHA	✅ (Rounds #2–#3)
R2-1	Callers of the new RollbackException	➖ Architect decision. The body lists each caller's outcome
R2 orphan race, R2-2, R2 body items	Per-page removal, "can't be parsed", unknown import modes, follow-ups, admin-created content	✅ (Round #3)
R3	Knowledge: line; Javadoc of the orphan selection window	✅ (Round #4)
R4 note	The full-suite result line named a tree that isn't the head	✅ The Tests line now gives the targeted run at 65f8e9a240 (18 classes, 148 tests, 0 failures) with its command, and says the full suite wasn't run at this head

Verified conform: 2d20d07490..65f8e9a240

• The five writes re-read their row the way deletePage does. These are PageStorageImpl#savePageLayout, #savePage(PageContext) and #clone, and SiteStorageImpl#save(PortalData) and #remove(SiteKey). Each one calls refresh (detach, then find) after findByKey.
  • Detaching drops no pending write. findByKey is a JPQL named query on PORTAL_PAGES or PORTAL_SITES, so under AUTO flush it flushes the dirty entities of that table before the detach.
  • Detaching doesn't cascade. PageEntity#owner declares no cascade, and the managed NodeEntity rows that point at the old instance keep a non-transient reference with an id.
  • SiteStorageImpl#remove now throws NoSuchDataException when the row is gone between getPortalConfig and the write. Before, it threw an NPE.
• @DynamicUpdate on PageEntity follows backend-spring.md §4: two writers, no @Version. It has the real-SQL column-set pin that the norm asks for, testPageRowUpdateWritesOnlyTheColumnsItChanged. That pin parses the stored PAGE_BODY after a stale-instance PageDAO#update.
• Each re-read has its own pin, and each pin asserts that the other request's save landed first (addPageContainerInAnotherRequest and addSiteContainerInAnotherRequest). Without that, the pins would prove nothing. The @DynamicUpdate annotation makes the layout-keeping pin pass even without the properties save's re-read. testSavePagePropertiesWritesBackAPropertyChangedSinceTheRequestReadThePage closes that gap: without the re-read, the stale managed instance already holds Original, Hibernate sees no dirty column, and the row keeps Other. The test asserts that outcome.
• The body matches the code. Fix and Upgrade impact list the same five re-reads and the extra SELECT by id. Known limits no longer counts the properties save among the no-lock writes, and the stale-body follow-up of Round #4 is gone because this push fixes it. The commit messages state the remaining no-lock window.
• Knowledge: Meeds-io/eng-standards#409, #560, #586, #587 are all open drafts, as expected for code not yet on develop. #586's pending ledger file covers all six re-reads, the @DynamicUpdate annotation and the four new pins.

What the push does well: it closes, in this PR, the follow-up that Round #4 accepted as a Known limit. It also caught that @DynamicUpdate had silently made one of its own pins vacuous, and wrote the pin that tells the re-read and the annotation apart.

All findings from previous rounds are resolved or recorded as owner decisions; nothing is outstanding from the AI review side.

Classification: this change is classified N1: the Kernel ↔ Spring bridge, the outcome of every @ExoTransactional method, the JPA entity and the JPA transaction boundaries and DAO reads of the site and page writes, and the irreversible deletion of pages and navigation nodes by a restore and at startup. It must be validated by an Architect/Developer who knows it is N1, not auto-merged on AI review alone, and not approved by its author. The approval given at 2d20d07490 was dismissed by the new commits, so it is owed again at 65f8e9a240.

🤖 Generated with Claude Code