[incubator-kie#2412] Release dynamic facts when they are deleted or e…

[Thebas]

Issue:

• BUG createTypeDeclarationForBean should honour @PropertyChangeSupport the way it honours @PropertyReactive. incubator-kie-issues#2412

Bug 1: delete never released facts inserted with insert(obj, true) (NamedEntryPoint.java)

• I added a helper, removeDynamicPropertyChangeListener. It removes the listener when the type is dynamic or the handle is in dynamicFacts, which is the condition the issue suggested.
• It is called from two places:
  • deleteStated, which replaces the old check that only looked at the type.
  • removeFromObjectStore, the path that expired events go through. Before this, expiry never removed the listener at all, so an expired event of a dynamic type leaked its listener the same way.
• Logical (TMS) retraction doesn't need the change. Logical inserts go straight to the internal ep.insert(handle, …), past the point where listeners are added, so a logical fact never has one.

Bug 2: class-level @PropertyChangeSupport was ignored (TypeDeclaration.java)

• processTypeAnnotations now calls a new configurePropertyChangeSupport, which only ever sets the type to dynamic. It can't undo a DRL declare … @propertyChangeSupport, because the DRL path runs this same method afterwards.
• This one change applies to all three createTypeDeclarationForBean callers: the compiler, the runtime KnowledgeBaseImpl, and the executable model.

Tests (in PropertyChangeSupportTest). Each one checks that the bean's own listener count drops to 0:

• testDeleteUnregistersFactInsertedAsDynamic: your reproducer, inserting with insert(x, true) from a rule and deleting from another, over 10 cycles.
• testClassLevelAnnotationMakesTypeDynamic: a class with only the annotation, no DRL declare.
• testExpiryUnregistersDynamicFact: an @expires("1s") event, using a pseudo clock.

Results

• On main without the fix, all 3 new tests fail. With the fix they pass, along with the existing test in that class.
• These also pass with the fix:
  • the full drools-base and drools-kiesession test suites
  • DrlSpecificFeaturesTest, Misc2Test, JittingTest, CepEspTest, StatefulSessionTest and TruthMaintenanceTest
  • PropertyListenerTest in test-suite
• Checkstyle passes on the 3 modules I changed.

[Thebas]

Also PASS: DynamicFactMarshallingTest

[Copilot]

🟢 Approval recommended

The focused implementation addresses both reported leaks with appropriate regression coverage.

0 open findings

What changed in this PR

Fixes listener leaks for dynamic facts and honors class-level property-change annotations.

Changes:

• Unregisters listeners when dynamic facts are deleted or expire.
• Enables dynamic behavior for class-level @PropertyChangeSupport.
• Adds regression tests for deletion, annotation handling, and expiry.

File	Description
PropertyChangeSupportTest.java	Adds listener lifecycle regression tests.
NamedEntryPoint.java	Centralizes listener cleanup across deletion paths.
TypeDeclaration.java	Processes class-level property-change annotations.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[tkobayas]

GHA CI :: Build / ubuntu-latest, Java 17 failure is not related to this PR. Tracked by #7179

[tkobayas]

@Thebas Thanks for the fix — the listener leak on delete and expiry is clearly the right thing to address, and the tests are solid.

I found one concurrency issue with the expiry path that I think should be fixed before merging.

removeFromObjectStore accesses dynamicFacts without the entry-point lock

insert() holds the entry-point ReentrantLock when it adds to dynamicFacts. But removeFromObjectStore() is called from DefaultAgenda.doRetract() during expiration, which does not hold that lock. The new removeDynamicPropertyChangeListener() call therefore performs contains() and remove() on the backing HashSet concurrently with insertion on another thread.

A minimal fix would be to wrap the call in removeFromObjectStore with the existing lock:

public void removeFromObjectStore(InternalFactHandle handle) {
    this.objectStore.removeHandle( handle );
    ObjectTypeConf typeConf = getObjectTypeConfigurationRegistry().getObjectTypeConf( handle.getObject() );
    lock();
    try {
        removeDynamicPropertyChangeListener( handle, typeConf );
    } finally {
        unlock();
    }
    deleteFromTMS( handle, handle.getEqualityKey(), typeConf, null );
}

The deleteStated path already runs under the lock, so only removeFromObjectStore needs the change.

[Thebas]

Dear @tkobayas . Fixed it. Thank you.

[Rikkola]

I can not take credit for this. Bob picked it up for me ( AI ).

But it looks like if the DRL definition does not have the annotation, then the annotation gets ignored.

The PR should have a fix and a test for this.

Thebas#1

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

1 open finding

• KieSession#getSessionClock() typically returns SessionClock, so assigning directly to… · New

🧠 Review effort: Lite