Repository navigation
Memory Module - #173
Merged
Merged
Memory Module#173
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR relocates memory modules into a directory structure, adds locked-map and lock-free-cache implementations with comprehensive tests, and refactors storage-engine workers to use document caches and locked-map based namespace, identity, and primary-key caches. ChangesMemory and storage cache refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/storage_engine.zig (1)
26-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the storage specifications to remove or rename
PkSet.
PkSetis no longer exported fromsrc/storage_engine.zig, but both storage specifications still list it as an important type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage_engine.zig` around lines 26 - 27, Update both storage specifications to remove `PkSet` from their listed important types, or replace it with its current exported name if it was renamed. Ensure the specifications reference only types actually exported by `src/storage_engine.zig`.
🧹 Nitpick comments (2)
src/storage_engine.zig (1)
37-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse CamelCase for cache type aliases.
The same type-naming violation appears in both files. Rename the aliases and update their references consistently.
src/storage_engine.zig#L37-L39: Renamedocument_cache_type,namespace_cache_type, andidentity_cache_typeto CamelCase type aliases.src/storage_engine/read_worker_pool.zig#L24-L24: Renamedocument_cache_typeto the matching CamelCase alias.As per coding guidelines,
src/**/*.zigmust use CamelCase for types and snake_case for functions and variables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage_engine.zig` around lines 37 - 39, Rename the cache type aliases in src/storage_engine.zig lines 37-39 to CamelCase and update every reference consistently. Apply the corresponding document cache alias rename in src/storage_engine/read_worker_pool.zig line 24 and its references; preserve snake_case for functions and variables.Source: Coding guidelines
src/storage_engine/cache.zig (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse snake_case for all affected Zig identifiers.
- Rename the listed cache factories, helpers, aliases, APIs, and
readerThread, including all references.- Also rename
minActiveEpoch,cloneEntries,cowMutation,transformFn,releaseHandle,internalDefer,pushToDeferStack,reclaimLoop,readLock,readUnlock,writeLock, andwriteUnlock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage_engine/cache.zig` around lines 3 - 4, Rename all affected Zig identifiers to snake_case and update every reference, including cache factories, helpers, aliases, APIs, readerThread, minActiveEpoch, cloneEntries, cowMutation, transformFn, releaseHandle, internalDefer, pushToDeferStack, reclaimLoop, readLock, readUnlock, writeLock, and writeUnlock. Apply the changes in src/storage_engine/cache.zig at ranges 3-4, 40-42, and 60-61, plus corresponding references in src/memory/lock_free_cache_test.zig ranges 3-3 and 45-56 and src/memory/lock_free_cache_leak_test.zig range 3-3.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/memory/lock_free_cache_leak_test.zig`:
- Around line 70-79: Update internalDefer and the cache update path exercised by
lock_free_cache_leak_test so a full deferred-node pool never silently drops
retired maps or entries: retain them using allocator-backed overflow nodes or
return an explicit backpressure error, and do not force reclamation while a
reader pins an epoch. Revise the test to release the pinned handle and then
assert that all deferred resources are eventually reclaimed.
In `@src/storage_engine_test.zig`:
- Around line 1027-1030: The failure branch in the cache lookup test must
release the live handle before returning. Update the .hit arm of getCachedRecord
to call hit.handle.release() before returning error.TestUnexpectedResult, while
preserving the existing .miss behavior.
In `@src/storage_engine.zig`:
- Around line 360-365: Reset all primary-key sets before each bootstrap scan by
calling the existing reset_pk_sets() in the startup flow after setup SQL and
before the bootstrap loop. Ensure failed attempts cannot leave IDs in the sets
used by documentExists and write-path duplicate checks, while preserving the
existing cache reset behavior around document_cache, namespace_cache, and
identity_cache.
In `@src/storage_engine/write_worker.zig`:
- Around line 870-874: Update the post-COMMIT pk_sets handling in the write
worker and StorageEngine.documentExists so a failed pk_sets.put does not make
committed rows appear absent. On insertion failure, either fall back to SQLite
for existence checks or mark the set degraded and rebuild it before use; ensure
StoreService.applySet receives authoritative existence results for
create/update, required-field, and authorization decisions.
- Around line 856-860: Update the document_cache.update error path in
commitBatchAndApply so a failed post-commit write invalidates or removes the
existing cache entry before deinitializing r, guaranteeing subsequent reads
produce a cache miss rather than stale data. Add failure-injection coverage that
prepopulates the cache entry and verifies it is absent after the update failure.
---
Outside diff comments:
In `@src/storage_engine.zig`:
- Around line 26-27: Update both storage specifications to remove `PkSet` from
their listed important types, or replace it with its current exported name if it
was renamed. Ensure the specifications reference only types actually exported by
`src/storage_engine.zig`.
---
Nitpick comments:
In `@src/storage_engine.zig`:
- Around line 37-39: Rename the cache type aliases in src/storage_engine.zig
lines 37-39 to CamelCase and update every reference consistently. Apply the
corresponding document cache alias rename in
src/storage_engine/read_worker_pool.zig line 24 and its references; preserve
snake_case for functions and variables.
In `@src/storage_engine/cache.zig`:
- Around line 3-4: Rename all affected Zig identifiers to snake_case and update
every reference, including cache factories, helpers, aliases, APIs,
readerThread, minActiveEpoch, cloneEntries, cowMutation, transformFn,
releaseHandle, internalDefer, pushToDeferStack, reclaimLoop, readLock,
readUnlock, writeLock, and writeUnlock. Apply the changes in
src/storage_engine/cache.zig at ranges 3-4, 40-42, and 60-61, plus corresponding
references in src/memory/lock_free_cache_test.zig ranges 3-3 and 45-56 and
src/memory/lock_free_cache_leak_test.zig range 3-3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 77754089-3529-4974-b012-ad3969eed309
📒 Files selected for processing (34)
src/app_test_helpers.zigsrc/authorization/session_resolver.zigsrc/checkpoint_test_helpers.zigsrc/connection/manager.zigsrc/connection/send_queue.zigsrc/logging_property_test.zigsrc/memory/lock_free_cache.zigsrc/memory/lock_free_cache_leak_test.zigsrc/memory/lock_free_cache_test.zigsrc/memory/locked_map.zigsrc/memory/strategy.zigsrc/memory/strategy_test.zigsrc/memory_safety_property_test.zigsrc/message_handler.zigsrc/presence/worker.zigsrc/presence/worker_test.zigsrc/queues/mpsc_queue_test.zigsrc/queues/mpsc_queue_thread_safety_test.zigsrc/queues/spsc_queue_test.zigsrc/schema/test_helpers.zigsrc/server.zigsrc/storage_engine.zigsrc/storage_engine/cache.zigsrc/storage_engine/pk_set.zigsrc/storage_engine/read_worker_perf_test.zigsrc/storage_engine/read_worker_pool.zigsrc/storage_engine/write_queue.zigsrc/storage_engine/write_worker.zigsrc/storage_engine_test.zigsrc/storage_engine_test_helpers.zigsrc/subscription/worker_pool.zigsrc/subscription/worker_pool_perf_test.zigsrc/subscription/worker_pool_test.zigsrc/test_all.zig
💤 Files with no reviewable changes (1)
- src/storage_engine/pk_set.zig
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/storage_engine.zig (1)
26-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the storage specifications to remove or rename
PkSet.
PkSetis no longer exported fromsrc/storage_engine.zig, but both storage specifications still list it as an important type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage_engine.zig` around lines 26 - 27, Update both storage specifications to remove `PkSet` from their listed important types, or replace it with its current exported name if it was renamed. Ensure the specifications reference only types actually exported by `src/storage_engine.zig`.
🧹 Nitpick comments (2)
src/storage_engine.zig (1)
37-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse CamelCase for cache type aliases.
The same type-naming violation appears in both files. Rename the aliases and update their references consistently.
src/storage_engine.zig#L37-L39: Renamedocument_cache_type,namespace_cache_type, andidentity_cache_typeto CamelCase type aliases.src/storage_engine/read_worker_pool.zig#L24-L24: Renamedocument_cache_typeto the matching CamelCase alias.As per coding guidelines,
src/**/*.zigmust use CamelCase for types and snake_case for functions and variables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage_engine.zig` around lines 37 - 39, Rename the cache type aliases in src/storage_engine.zig lines 37-39 to CamelCase and update every reference consistently. Apply the corresponding document cache alias rename in src/storage_engine/read_worker_pool.zig line 24 and its references; preserve snake_case for functions and variables.Source: Coding guidelines
src/storage_engine/cache.zig (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse snake_case for all affected Zig identifiers.
- Rename the listed cache factories, helpers, aliases, APIs, and
readerThread, including all references.- Also rename
minActiveEpoch,cloneEntries,cowMutation,transformFn,releaseHandle,internalDefer,pushToDeferStack,reclaimLoop,readLock,readUnlock,writeLock, andwriteUnlock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage_engine/cache.zig` around lines 3 - 4, Rename all affected Zig identifiers to snake_case and update every reference, including cache factories, helpers, aliases, APIs, readerThread, minActiveEpoch, cloneEntries, cowMutation, transformFn, releaseHandle, internalDefer, pushToDeferStack, reclaimLoop, readLock, readUnlock, writeLock, and writeUnlock. Apply the changes in src/storage_engine/cache.zig at ranges 3-4, 40-42, and 60-61, plus corresponding references in src/memory/lock_free_cache_test.zig ranges 3-3 and 45-56 and src/memory/lock_free_cache_leak_test.zig range 3-3.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/memory/lock_free_cache_leak_test.zig`:
- Around line 70-79: Update internalDefer and the cache update path exercised by
lock_free_cache_leak_test so a full deferred-node pool never silently drops
retired maps or entries: retain them using allocator-backed overflow nodes or
return an explicit backpressure error, and do not force reclamation while a
reader pins an epoch. Revise the test to release the pinned handle and then
assert that all deferred resources are eventually reclaimed.
In `@src/storage_engine_test.zig`:
- Around line 1027-1030: The failure branch in the cache lookup test must
release the live handle before returning. Update the .hit arm of getCachedRecord
to call hit.handle.release() before returning error.TestUnexpectedResult, while
preserving the existing .miss behavior.
In `@src/storage_engine.zig`:
- Around line 360-365: Reset all primary-key sets before each bootstrap scan by
calling the existing reset_pk_sets() in the startup flow after setup SQL and
before the bootstrap loop. Ensure failed attempts cannot leave IDs in the sets
used by documentExists and write-path duplicate checks, while preserving the
existing cache reset behavior around document_cache, namespace_cache, and
identity_cache.
In `@src/storage_engine/write_worker.zig`:
- Around line 870-874: Update the post-COMMIT pk_sets handling in the write
worker and StorageEngine.documentExists so a failed pk_sets.put does not make
committed rows appear absent. On insertion failure, either fall back to SQLite
for existence checks or mark the set degraded and rebuild it before use; ensure
StoreService.applySet receives authoritative existence results for
create/update, required-field, and authorization decisions.
- Around line 856-860: Update the document_cache.update error path in
commitBatchAndApply so a failed post-commit write invalidates or removes the
existing cache entry before deinitializing r, guaranteeing subsequent reads
produce a cache miss rather than stale data. Add failure-injection coverage that
prepopulates the cache entry and verifies it is absent after the update failure.
---
Outside diff comments:
In `@src/storage_engine.zig`:
- Around line 26-27: Update both storage specifications to remove `PkSet` from
their listed important types, or replace it with its current exported name if it
was renamed. Ensure the specifications reference only types actually exported by
`src/storage_engine.zig`.
---
Nitpick comments:
In `@src/storage_engine.zig`:
- Around line 37-39: Rename the cache type aliases in src/storage_engine.zig
lines 37-39 to CamelCase and update every reference consistently. Apply the
corresponding document cache alias rename in
src/storage_engine/read_worker_pool.zig line 24 and its references; preserve
snake_case for functions and variables.
In `@src/storage_engine/cache.zig`:
- Around line 3-4: Rename all affected Zig identifiers to snake_case and update
every reference, including cache factories, helpers, aliases, APIs,
readerThread, minActiveEpoch, cloneEntries, cowMutation, transformFn,
releaseHandle, internalDefer, pushToDeferStack, reclaimLoop, readLock,
readUnlock, writeLock, and writeUnlock. Apply the changes in
src/storage_engine/cache.zig at ranges 3-4, 40-42, and 60-61, plus corresponding
references in src/memory/lock_free_cache_test.zig ranges 3-3 and 45-56 and
src/memory/lock_free_cache_leak_test.zig range 3-3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 77754089-3529-4974-b012-ad3969eed309
📒 Files selected for processing (34)
src/app_test_helpers.zigsrc/authorization/session_resolver.zigsrc/checkpoint_test_helpers.zigsrc/connection/manager.zigsrc/connection/send_queue.zigsrc/logging_property_test.zigsrc/memory/lock_free_cache.zigsrc/memory/lock_free_cache_leak_test.zigsrc/memory/lock_free_cache_test.zigsrc/memory/locked_map.zigsrc/memory/strategy.zigsrc/memory/strategy_test.zigsrc/memory_safety_property_test.zigsrc/message_handler.zigsrc/presence/worker.zigsrc/presence/worker_test.zigsrc/queues/mpsc_queue_test.zigsrc/queues/mpsc_queue_thread_safety_test.zigsrc/queues/spsc_queue_test.zigsrc/schema/test_helpers.zigsrc/server.zigsrc/storage_engine.zigsrc/storage_engine/cache.zigsrc/storage_engine/pk_set.zigsrc/storage_engine/read_worker_perf_test.zigsrc/storage_engine/read_worker_pool.zigsrc/storage_engine/write_queue.zigsrc/storage_engine/write_worker.zigsrc/storage_engine_test.zigsrc/storage_engine_test_helpers.zigsrc/subscription/worker_pool.zigsrc/subscription/worker_pool_perf_test.zigsrc/subscription/worker_pool_test.zigsrc/test_all.zig
💤 Files with no reviewable changes (1)
- src/storage_engine/pk_set.zig
🛑 Comments failed to post (1)
src/memory/lock_free_cache_leak_test.zig (1)
70-79: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not accept resource loss when the deferred-node pool is full.
A pinned reader prevents reclamation.
internalDeferthen returns when it cannot acquire a node. Each later update can lose a retired map or cache entry permanently.Retain the resource until safe reclamation is possible. Use allocator-backed overflow nodes, or return a backpressure error. Do not force reclamation while a reader holds an epoch. Update this test to verify eventual cleanup after the pinned handle is released.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/lock_free_cache_leak_test.zig` around lines 70 - 79, Update internalDefer and the cache update path exercised by lock_free_cache_leak_test so a full deferred-node pool never silently drops retired maps or entries: retain them using allocator-backed overflow nodes or return an explicit backpressure error, and do not force reclamation while a reader pins an epoch. Revise the test to release the pinned handle and then assert that all deferred resources are eventually reclaimed.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/storage_engine.zig (1)
367-368: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve a valid cache when reinitialization fails.
If
document_cache.initfails,execSetupSQLreturns whilestateremains.setupanddocument_cachehas already been deinitialized. A laterstart,deinit, or setup retry can use an invalid cache.Initialize a safe replacement before destroying the live cache. If the cache cannot support replacement, transition the engine to a state that blocks reuse after this failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage_engine.zig` around lines 367 - 368, Update the cache reset logic in execSetupSQL to initialize a safe replacement document_cache before deinitializing the currently valid cache; if replacement initialization fails, transition the engine out of reusable setup state so later start, deinit, or setup retries cannot access an invalid cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/storage_engine.zig`:
- Around line 367-368: Update the cache reset logic in execSetupSQL to
initialize a safe replacement document_cache before deinitializing the currently
valid cache; if replacement initialization fails, transition the engine out of
reusable setup state so later start, deinit, or setup retries cannot access an
invalid cache.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 91fc31f9-099c-4fa1-b5b3-341c3eed683d
📒 Files selected for processing (7)
specs/implementation/storage.mdsrc/failing_allocator_test_helper.zigsrc/memory/lock_free_cache.zigsrc/memory/lock_free_cache_leak_test.zigsrc/storage_engine.zigsrc/storage_engine/write_worker.zigsrc/storage_engine_test.zig
🚧 Files skipped from review as they are similar to previous changes (2)
- src/memory/lock_free_cache.zig
- src/storage_engine/write_worker.zig
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit