Don't erase elements if multiple subscriptions use them by kilativ-dotcom · Pull Request #425 · ostis-ai/sc-machine

kilativ-dotcom · 2024-10-15T15:20:07Z

Read PR documentation
Update changelog
Update documentation

Summary by Gitar

Memory/Core:
- Optimized sc_storage_element_erase by preventing premature element deletion if active subscriptions are present.
- Implemented _sc_storage_element_erase_with_incoming_outgoing_connectors_and_hanging_nodes to recursively manage element erasure.
Event Handling:
- Added emitted_erase_events tracking to sc_event_emission_manager to prevent race conditions during concurrent deletions.
- Refactored sc_event_emission_manager to safely wait for pending events during shutdown.
Testing:
- Added multiple stress tests in test_sc_event.cpp to verify stability of concurrent subscriptions during erasure.
- Improved test reliability in test_sc_specified_agents.cpp and test_sc_event.cpp using ScMemoryContextEventsPendingGuard.

_{This will update automatically on new commits.}

codecov · 2024-10-16T05:43:05Z

Codecov Report

Attention: Patch coverage is 97.63948% with 11 lines in your changes missing coverage. Please review.

Project coverage is 95.76%. Comparing base (1424e78) to head (b375f96).
Report is 179 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   95.70%   95.76%   +0.05%     
==========================================
  Files         237      237              
  Lines       26965    27315     +350     
  Branches     1863     1880      +17     
==========================================
+ Hits        25808    26157     +349     
- Misses       1157     1158       +1

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

NikitaZotov · 2024-10-17T13:32:16Z

+    sc_uint32 * count = (sc_uint32 *)sc_hash_table_get(
+        manager->emitted_erase_events, GUINT_TO_POINTER(SC_ADDR_LOCAL_TO_INT(connector_addr)));
+    if (count == null_ptr)
+    {
+      count = sc_mem_new(sc_uint32, 1);
+      *count = 0;
+    }
+    ++(*count);
+    sc_hash_table_insert(manager->emitted_erase_events, GUINT_TO_POINTER(SC_ADDR_LOCAL_TO_INT(connector_addr)), count);


Suggested change

sc_uint32 * count = (sc_uint32 *)sc_hash_table_get(

manager->emitted_erase_events, GUINT_TO_POINTER(SC_ADDR_LOCAL_TO_INT(connector_addr)));

if (count == null_ptr)

{

count = sc_mem_new(sc_uint32, 1);

*count = 0;

}

++(*count);

sc_hash_table_insert(manager->emitted_erase_events, GUINT_TO_POINTER(SC_ADDR_LOCAL_TO_INT(connector_addr)), count);

sc_uint32 count = (sc_uint32)(sc_uint64)sc_hash_table_get(

manager->emitted_erase_events, GUINT_TO_POINTER(SC_ADDR_LOCAL_TO_INT(connector_addr)));

++count;

sc_hash_table_insert(manager->emitted_erase_events, GUINT_TO_POINTER(SC_ADDR_LOCAL_TO_INT(connector_addr)), count);

NikitaZotov · 2024-11-25T16:14:27Z

+
+sc_result _sc_storage_element_erase_with_incoming_outgoing_connectors(
+    sc_memory_context const * ctx,
+    sc_addr connector_chain_begin_addr,


What is chain here?

chain is like sequence of connectors next connector of which cannot exist without previous(previous element is source/target of next connector)

NikitaZotov

Tests are failed

… event linked to subscription

…a state of race

…stency for agents responding to sc-events of erasing sc-elements

gitar-bot · 2026-07-19T11:37:05Z

  sc_event_emission_manager * emission_manager = sc_storage_get_event_emission_manager();
+  if (subscription_manager == null_ptr)
+    return SC_RESULT_NO;


⚠️ Bug: Wrong variable in null check leaves emission_manager unchecked

In sc_event_subscription_destroy, after fetching emission_manager, the guard at line 211 mistakenly re-checks subscription_manager == null_ptr instead of emission_manager == null_ptr (copy-paste error). If emission_manager is null while subscription_manager and storage are non-null, the code reaches line 236 and dereferences emission_manager->pool_monitor, causing a null-pointer crash. Change the check to test emission_manager.

Check emission_manager instead of re-checking subscription_manager.:

sc_event_emission_manager * emission_manager = sc_storage_get_event_emission_manager(); if (emission_manager == null_ptr) return SC_RESULT_NO;

Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

gitar-bot · 2026-07-19T11:37:06Z

+      sc_monitor_acquire_read(&emission_manager->pool_monitor);
+      sc_uint32 count = (sc_uint32)(sc_uint64)sc_hash_table_get(
+          emission_manager->emitted_erase_events, GUINT_TO_POINTER(SC_ADDR_LOCAL_TO_INT(addr)));
+      if (count == 0)
+        sc_hash_table_insert(incident_elements_under_erasure, key, element);
+      sc_monitor_release_read(&emission_manager->pool_monitor);


⚠️ Bug: emitted_erase_events accessed under two different monitors (data race)

The emitted_erase_events hash table is written by the worker and by _sc_event_emission_manager_add while holding emitted_erase_events_monitor, but sc_storage.c reads it while holding only pool_monitor (read). Because readers and the worker never share a common lock, concurrent erase + event processing produces unsynchronized concurrent access to a GHashTable, which is undefined behavior and can corrupt the table or crash. The storage reads should acquire emitted_erase_events_monitor (not pool_monitor) to match the writers.

Guard reads of emitted_erase_events with emitted_erase_events_monitor at both sc_storage.c sites.:

sc_monitor_acquire_read(&emission_manager->emitted_erase_events_monitor); sc_uint32 count = (sc_uint32)(sc_uint64)sc_hash_table_get( emission_manager->emitted_erase_events, GUINT_TO_POINTER(SC_ADDR_LOCAL_TO_INT(addr))); ... sc_monitor_release_read(&emission_manager->emitted_erase_events_monitor);

Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

gitar-bot · 2026-07-19T11:37:07Z


-  if (queue->running == SC_FALSE)
+  if (manager->running == SC_FALSE)
    goto end;


💡 Bug: running flag read/written without synchronization

sc_event_emission_manager_stop now reads and writes manager->running with no lock, while the worker reads manager->running under destroy_monitor. This is an unsynchronized concurrent access to running; previously it was protected by destroy_monitor. In practice this is a benign torn-read of a bool, but it is a data race per the memory model and could delay a worker observing the shutdown. Re-acquire destroy_monitor around the update or make the flag atomic.

_{Was this helpful? React with 👍 / 👎}

gitar-bot · 2026-07-19T11:37:09Z

Code Review ⚠️ Changes requested 0 resolved / 3 findings

Prevents premature element erasure for active subscriptions and adds stress tests for concurrent event handling, but contains critical synchronization and pointer validation errors.

⚠️

Bug: Wrong variable in null check leaves emission_manager unchecked

📄 sc-memory/sc-core/src/sc-store/sc_event_subscription.c:210-212 📄 sc-memory/sc-core/src/sc-store/sc_event_subscription.c:236

In sc_event_subscription_destroy, after fetching emission_manager, the guard at line 211 mistakenly re-checks subscription_manager == null_ptr instead of emission_manager == null_ptr (copy-paste error). If emission_manager is null while subscription_manager and storage are non-null, the code reaches line 236 and dereferences emission_manager->pool_monitor, causing a null-pointer crash. Change the check to test emission_manager.

Check emission_manager instead of re-checking subscription_manager.

sc_event_emission_manager * emission_manager = sc_storage_get_event_emission_manager();
if (emission_manager == null_ptr)
  return SC_RESULT_NO;

⚠️

Bug: emitted_erase_events accessed under two different monitors (data race)

📄 sc-memory/sc-core/src/sc-store/sc_storage.c:693-698 📄 sc-memory/sc-core/src/sc-store/sc_storage.c:830-835 📄 sc-memory/sc-core/src/sc-store/sc-event/sc_event_queue.c:107-121 📄 sc-memory/sc-core/src/sc-store/sc-event/sc_event_queue.c:265-274

The emitted_erase_events hash table is written by the worker and by _sc_event_emission_manager_add while holding emitted_erase_events_monitor, but sc_storage.c reads it while holding only pool_monitor (read). Because readers and the worker never share a common lock, concurrent erase + event processing produces unsynchronized concurrent access to a GHashTable, which is undefined behavior and can corrupt the table or crash. The storage reads should acquire emitted_erase_events_monitor (not pool_monitor) to match the writers.

Guard reads of emitted_erase_events with emitted_erase_events_monitor at both sc_storage.c sites.

sc_monitor_acquire_read(&emission_manager->emitted_erase_events_monitor);
sc_uint32 count = (sc_uint32)(sc_uint64)sc_hash_table_get(
    emission_manager->emitted_erase_events, GUINT_TO_POINTER(SC_ADDR_LOCAL_TO_INT(addr)));
...
sc_monitor_release_read(&emission_manager->emitted_erase_events_monitor);

💡 Bug: running flag read/written without synchronization

📄 sc-memory/sc-core/src/sc-store/sc-event/sc_event_queue.c:189-196 📄 sc-memory/sc-core/src/sc-store/sc-event/sc_event_queue.c:77-79

sc_event_emission_manager_stop now reads and writes manager->running with no lock, while the worker reads manager->running under destroy_monitor. This is an unsynchronized concurrent access to running; previously it was protected by destroy_monitor. In practice this is a benign torn-read of a bool, but it is a data race per the memory model and could delay a worker observing the shutdown. Re-acquire destroy_monitor around the update or make the flag atomic.

🤖 Prompt for agents

Code Review: Prevents premature element erasure for active subscriptions and adds stress tests for concurrent event handling, but contains critical synchronization and pointer validation errors.

1. ⚠️ Bug: Wrong variable in null check leaves emission_manager unchecked
   Files: sc-memory/sc-core/src/sc-store/sc_event_subscription.c:210-212, sc-memory/sc-core/src/sc-store/sc_event_subscription.c:236

   In `sc_event_subscription_destroy`, after fetching `emission_manager`, the guard at line 211 mistakenly re-checks `subscription_manager == null_ptr` instead of `emission_manager == null_ptr` (copy-paste error). If `emission_manager` is null while `subscription_manager` and `storage` are non-null, the code reaches line 236 and dereferences `emission_manager->pool_monitor`, causing a null-pointer crash. Change the check to test `emission_manager`.

   Fix (Check emission_manager instead of re-checking subscription_manager.):
   sc_event_emission_manager * emission_manager = sc_storage_get_event_emission_manager();
   if (emission_manager == null_ptr)
     return SC_RESULT_NO;

2. ⚠️ Bug: emitted_erase_events accessed under two different monitors (data race)
   Files: sc-memory/sc-core/src/sc-store/sc_storage.c:693-698, sc-memory/sc-core/src/sc-store/sc_storage.c:830-835, sc-memory/sc-core/src/sc-store/sc-event/sc_event_queue.c:107-121, sc-memory/sc-core/src/sc-store/sc-event/sc_event_queue.c:265-274

   The `emitted_erase_events` hash table is written by the worker and by `_sc_event_emission_manager_add` while holding `emitted_erase_events_monitor`, but `sc_storage.c` reads it while holding only `pool_monitor` (read). Because readers and the worker never share a common lock, concurrent erase + event processing produces unsynchronized concurrent access to a GHashTable, which is undefined behavior and can corrupt the table or crash. The storage reads should acquire `emitted_erase_events_monitor` (not `pool_monitor`) to match the writers.

   Fix (Guard reads of emitted_erase_events with emitted_erase_events_monitor at both sc_storage.c sites.):
   sc_monitor_acquire_read(&emission_manager->emitted_erase_events_monitor);
   sc_uint32 count = (sc_uint32)(sc_uint64)sc_hash_table_get(
       emission_manager->emitted_erase_events, GUINT_TO_POINTER(SC_ADDR_LOCAL_TO_INT(addr)));
   ...
   sc_monitor_release_read(&emission_manager->emitted_erase_events_monitor);

3. 💡 Bug: running flag read/written without synchronization
   Files: sc-memory/sc-core/src/sc-store/sc-event/sc_event_queue.c:189-196, sc-memory/sc-core/src/sc-store/sc-event/sc_event_queue.c:77-79

   `sc_event_emission_manager_stop` now reads and writes `manager->running` with no lock, while the worker reads `manager->running` under `destroy_monitor`. This is an unsynchronized concurrent access to `running`; previously it was protected by `destroy_monitor`. In practice this is a benign torn-read of a bool, but it is a data race per the memory model and could delay a worker observing the shutdown. Re-acquire `destroy_monitor` around the update or make the flag atomic.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}

kilativ-dotcom added the 0.10.0 label Oct 15, 2024

kilativ-dotcom requested a review from NikitaZotov October 15, 2024 15:20

kilativ-dotcom self-assigned this Oct 15, 2024

NikitaZotov requested changes Oct 17, 2024

View reviewed changes

kilativ-dotcom requested a review from NikitaZotov October 21, 2024 09:47

NikitaZotov changed the title ~~Don't erase elements if multiple subsciptions use them~~ Don't erase elements if multiple subscriptions use them Nov 4, 2024

NikitaZotov requested changes Nov 4, 2024

View reviewed changes

kilativ-dotcom requested a review from NikitaZotov November 15, 2024 13:02

NikitaZotov requested changes Nov 25, 2024

View reviewed changes

NikitaZotov added this to the 0.10.0 milestone Nov 25, 2024

NikitaZotov added bug Something isn't working sc-memory Changes in sc-memory module labels Nov 25, 2024

kilativ-dotcom requested a review from NikitaZotov December 6, 2024 10:25

NikitaZotov requested changes Dec 6, 2024

View reviewed changes

kilativ-dotcom and others added 15 commits January 8, 2025 10:54

[memory][event] Don't erase elements if multiple subsciptions use them

a989d5f

[memory][event] Replace list with bool flag

28247a4

[memory][event] Reuse bool flag during recursion

3ceb3bd

[memory][event] Add check for emission_manager absence

57b1f6a

[memory][event][refactor] Remove space after &

5c4804e

[memory][event][refactor] Remove space after &

dcb5483

[tests] Add pending for initiation condition generation

e6c443d

[tests] Rename for more clear understanding

ea5b29b

[tests][event] Use pending for arc assignment

7a8f4c4

[tests] Don't shadow local variable

c5d8743

[events][tests] Erase incident nodes without event callbacks

9a97df2

[event][tests] Don't erase elements that were erased

bacd431

[event] Release read monitor before source and target processing

969e37d

[tests][event] Reorder monitor acquire

b6c6c04

[memory][core][events] Fix syncing removing subscription and emmiting…

a069701

… event linked to subscription

NikitaZotov added 2 commits January 11, 2025 15:01

[memory][core][events][tests] Fix checking timeouts for processes in …

172ddc8

…a state of race

[memory][core][events][tests] Leave TODO about providing causal consi…

ad6d130

…stency for agents responding to sc-events of erasing sc-elements

NikitaZotov force-pushed the fix/erase_events branch from c8eebff to ad6d130 Compare January 11, 2025 12:16

NikitaZotov self-requested a review January 11, 2025 12:18

[memory][core][events][tests] Duplicate TODO in tests

b375f96

NikitaZotov modified the milestones: 0.10.0, 0.10.1 Jan 28, 2025

NikitaZotov added 0.10.1 and removed 0.10.0 labels Jan 28, 2025

NikitaZotov added 0.10.2 and removed 0.10.1 labels Mar 15, 2025

NikitaZotov added 0.10.4 and removed 0.10.2 labels May 1, 2025

NikitaZotov modified the milestones: 0.10.1, 0.10.4 May 1, 2025

gitar-bot Bot reviewed Jul 19, 2026

View reviewed changes

Uh oh!

Conversation

kilativ-dotcom commented Oct 15, 2024 • edited by gitar-bot Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Gitar

Uh oh!

codecov Bot commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

NikitaZotov Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NikitaZotov Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

kilativ-dotcom Nov 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NikitaZotov left a comment

Choose a reason for hiding this comment

Uh oh!

gitar-bot Bot Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot Bot Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot Bot Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot Bot commented Jul 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kilativ-dotcom commented Oct 15, 2024 •

edited by gitar-bot Bot

Loading

codecov Bot commented Oct 16, 2024 •

edited

Loading