save() does not populate save_cache when creating an instance, leaving the eventual-consistency window unguarded

[apdavison]

Summary

exists() documents save_cache as being populated on two paths (kgobject.py:580-583):

# Because the KnowledgeGraph is only eventually consistent, an instance
# that has just been written to the KG may not appear in the query.
# Therefore we cache the query when creating an instance and
# where exists() returns True

Only the second clause is true. save_cache is written in exactly one place — kgobject.py:628, inside exists(), when the existence query found an instance. The create branch of save() (kgobject.py:817-848) sets self.id, _raw_remote_data and remote_data, but never adds a cache entry.

So the mitigation the comment describes is not in place, and the eventual-consistency race it was written to guard against is live.

Failure scenario

Two separately-constructed objects with the same existence properties, saved in one session, where the first is created rather than found:

person_a = omcore.Person(given_name="Bilbo", family_name="Baggins")
person_a.save(client, space="myspace")   # not in the KG -> created; no cache entry written

person_b = omcore.Person(given_name="Bilbo", family_name="Baggins")
person_b.save(client, space="myspace")   # exists(): no id, save_cache miss -> queries the KG
                                         # within the consistency window the new instance
                                         # is not yet visible -> returns False -> duplicate

Note the contrast with #134: there the first object is found by query, so kgobject.py:628 populates the cache and the second save takes the cache branch. Created-then-constructed-again is the half that is unguarded. Both arise from the same workflow described in #134 — harvesting scripts that build a fresh Person for each role someone holds — with the difference that this path bites the first time a person is added rather than on subsequent runs.

Origin

The write existed in the KG v2 implementation, in the create branch of save() (base_v2.py:450-452 at 8564c75^):

instance = client.create_new_instance(self.__class__.path, data)
self.id = instance.data["@id"]
self.instance = instance
existence_query = self._build_existence_query(api="nexus")
# make the cache key api-independent?
KGObject.save_cache[self.__class__][generate_cache_key(existence_query)] = self.id

It was removed in 8564c75 (2022-12-23, "Remove code relating to KG v2") along with the whole module, and not carried over into the v3 kgobject.py. git log -S over fairgraph/ confirms it has not been present since.

Why no test catches it

test/test_base.py covers the save-cache read path, but populates the cache by hand (test_base.py:643-645):

def _register_in_save_cache(self, obj):
    """Mimic the caching that exists() and save() perform for an object in the KG."""
    save_cache[MockKGObject][generate_cache_key(obj._build_existence_query())] = obj.id

The helper's docstring states the intended behaviour that save() does not implement.

Suggested fix

Write the cache entry in the create branch, after self.id is set (kgobject.py:844), using self._build_existence_query() and skipping when it returns None or raises CannotBuildExistenceQuery — objects with no existence query are allowed to duplicate by design. Worth confirming the key generated at create time matches the one exists() computes for a later equivalent object. A test that saves two separately-constructed equivalent objects against a mock client which does not report the first one as visible would cover the gap.

Related to #80, of which this is a concrete instance, and to #134, which concerns the other path through the same cache.