Skip to content

Synchronize attribute key cache refresh#898

Open
Symmetricity wants to merge 1 commit into
systemed:masterfrom
Symmetricity:fix/attribute-key-cache-race
Open

Synchronize attribute key cache refresh#898
Symmetricity wants to merge 1 commit into
systemed:masterfrom
Symmetricity:fix/attribute-key-cache-race

Conversation

@Symmetricity
Copy link
Copy Markdown

This PR is AI generated.

Summary

This synchronizes AttributeKeyStore's thread-local key cache refresh with the
shared key store mutation path.

AttributeKeyStore::key2index() is called while PBF blocks are processed in
parallel. The previous code allowed worker threads to refresh their
thread-local keys2index cache from the shared keys deque without holding
keys2indexMutex. It also advanced keys2indexSize before appending the
corresponding key to keys.

That left a window where another worker could observe a newly published key
count before the matching keys[] entry was safely available, or while the
deque/map was being mutated by another thread.

Evidence

While investigating an intermittent Windows --store crash, a full ProcDump
capture showed an access violation in the attribute key cache path:

Exception: C0000005.ACCESS_VIOLATION
Failure: INVALID_POINTER_READ
Stack:
  AttributeKeyStore::key2index
  AttributeStore::addAttribute
  OsmLuaProcessing::Attribute
  OsmLuaProcessing::setNode
  PbfProcessor::ReadNodes
  PbfProcessor::ReadBlock
  boost::asio worker thread

The failing instruction was inside an MSVC std::string comparison used by the
std::map<const std::string*, ...> comparator in AttributeKeyStore. One side
of the comparison was a null/invalid string pointer, consistent with a worker
thread caching an invalid &keys[index] pointer.

Reproduction signal from the Windows VM:

  • before this fix, the low-disk --store repro produced a ProcDump crash dump
    on run 15 with 0xC0000005
  • after applying this fix, the same ProcDump-wrapped repro completed 30 runs
    without producing an access-violation dump

This is also a plausible source of non-crashing downstream issues: if the stale
or invalid cache path does not crash immediately, it can return the wrong
attribute key index or corrupt the key lookup path used later when serializing
feature attributes.

Implementation

The patch keeps the existing data structures and synchronization model, but
closes the unsafe window:

  • take keys2indexMutex before refreshing the thread-local cache from keys
  • append the new key and update the shared keys2index map before publishing
    the new keys2indexSize

This means worker-local caches only observe key indexes that have already been
fully installed in the shared store.

Testing

The branch adds test_attribute_key_store_threaded, which exercises concurrent
lookup and insertion into a single AttributeKeyStore from multiple threads.

The test checks that:

  • no worker receives index 0
  • no worker receives an index outside the expected key range
  • repeated lookup of the same key within a worker returns the same index

Recommended local checks:

git diff --check origin/master..fix/attribute-key-cache-race
cmake -S . -B local-builds/attribute-key-cache-race -DCMAKE_BUILD_TYPE=RelWithDebInfo
cmake --build local-builds/attribute-key-cache-race --target attribute_store.test -j2
./local-builds/attribute-key-cache-race/tests/attribute_store.test

The original crash is scheduling-sensitive, so the unit test is a focused
concurrency regression rather than a guaranteed pre-fix crash reproducer.

Performance

I also compared the submitted PR stack with and without this patch on a warmed
Austria Geofabrik extract using the OpenMapTiles profile, PMTiles output, and
--threads 4. The comparison used three alternating runs for each build.

submitted stack:            average wall 81.20s, average RSS 2,927,728 KiB, max RSS 2,993,736 KiB
submitted stack + this fix: average wall 75.40s, average RSS 2,932,616 KiB, max RSS 2,998,324 KiB

All six runs exited successfully. The timings are close enough that I would not
claim a speedup from this change, but the test did not show a meaningful memory
or runtime regression from synchronizing the cache refresh path.

Possible Regressions

This adds a mutex hold around the thread-local cache refresh path after a cache
miss. The hot path remains unchanged when the key is already present in the
thread-local cache.

Expected behavior changes:

  • attribute key indexes remain stable under concurrent PBF processing
  • no change to serialized attribute semantics except preventing race-induced
    corruption or inconsistent key lookup
  • a small amount of extra locking is possible when a worker first observes
    newly added keys

AttributeKeyStore publishes new keys to worker-local caches while PBF reading runs in parallel. The previous code advanced keys2indexSize before appending the matching key and refreshed the thread-local cache without holding keys2indexMutex, so another worker could read the deque while it was being mutated.

Take the existing mutex before refreshing the thread-local cache and publish the new key only after it is appended to the shared store. Add a threaded AttributeKeyStore test for the concurrent lookup/insert path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant