Remove JetStream pointers from KV implementation #104
Conversation
Replace JetStream pointers with owned instances in KV, KVManager, and KVWatcher structs. This simplifies memory management since JetStream instances are cheap to create and don't require complex cleanup. Changes: - KV struct: js: *JetStream -> js: JetStream, removed separate allocator field - KVManager struct: js: *JetStream -> js: JetStream, removed separate allocator field - KVWatcher: updated to use kv.js.allocator - Updated all method signatures and calls to use owned instances - Fixed integration points and tests All 108 tests pass, including all KV functionality tests.
|
Warning Rate limit exceeded@lalinsky has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughRefactors JetStream and KV to remove per-instance allocators, using the Connection's allocator (nc.allocator) for all allocations. Many JetStream APIs and internal helpers changed from pointer receivers to by-value receivers. KV/KVManager constructors and fields updated to accept and store JetStream by value. Tests updated to use the new init signatures and removed explicit js.deinit() calls. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed Checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @lalinsky, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the internal implementation of the Key-Value (KV) store by transitioning from using pointers to owned instances of the JetStream context. This change streamlines memory management and simplifies the API by removing the need for explicit allocator parameters in many functions, as the JetStream instance itself provides access to the necessary allocator. The refactoring enhances code clarity and reduces potential memory-related complexities.
Highlights
- Refactor JetStream instances: Replaced JetStream pointers with owned instances within the KV, KVManager, and KVWatcher structs to simplify memory management.
- Improved API and Memory Management: The change leads to a cleaner API with fewer initialization parameters and eliminates potential issues related to pointer invalidation, leveraging the cheap-to-copy nature of JetStream instances.
- Allocator Handling Streamlined: The separate 'allocator' field was removed from KV and KVManager structs, with allocator operations now directly referencing 'js.allocator'.
- Comprehensive Test Coverage: All 108 existing tests, covering basic KV operations, history, watching, bucket management, and validation, continue to pass, ensuring full compatibility.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jetstream_kv.zig (1)
200-231: Watcher’s sub captures&kv.js; avoid pointer-to-field lifetime
kv.js.subscribeSync(...)creates a subscription that (today) stores*JetStream. Ifkvmoves,&kv.jsdangles. This is the practical fallout of switching to value semantics. Adopt the “subscriptions own JetStream by value” change proposed in src/jetstream.zig to break this aliasing.
🧹 Nitpick comments (2)
tests/jetstream_kv_test.zig (1)
236-244: Don’t passundefinedforncunless guaranteed unused
nats.JetStream.init(testing.allocator, undefined, .{})is fine here becauseKV.initerrors before touchingjs.nc, but it’s brittle. Prefer creating a realconnand usingconn.jetstream(.{})like other tests, or at least add a brief comment thatncis intentionally unused in this test. Also considerdefer js.deinit();for symmetry.src/jetstream_kv.zig (1)
658-683: PopulatecreatedfromNats-Time-Stampfor direct get; fix comment
parseMessage(used after direct get) setscreated = msg.time, butgetMsgDirectdoesn’t settime. The server providesNats-Time-Stamp; parse it for consistent metadata. Also the docstring mentions “JetStream message” but the parameter is*Message.Apply this diff:
- /// Parse a JetStream message into a KVEntry - /// Extracts Message pointer without calling js_msg.deinit() since we reference memory inside the message + /// Parse a Message into a KVEntry fn parseMessage(self: *KV, msg: *Message) !KVEntry { @@ - return KVEntry{ + var created_ts: u64 = 0; + if (msg.headerGet("Nats-Time-Stamp")) |ts| { + created_ts = timestamp.parseTimestamp(ts) catch 0; + } else { + created_ts = msg.time; + } + return KVEntry{ .bucket = self.bucket_name, .key = key, .value = msg.data, .operation = operation, - .created = msg.time, + .created = created_ts, .revision = msg.seq, .delta = 0, .msg = msg, };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/jetstream.zig(1 hunks)src/jetstream_kv.zig(16 hunks)tests/jetstream_kv_test.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
Format code using
zig fmtbefore commit
Files:
src/jetstream.zigtests/jetstream_kv_test.zigsrc/jetstream_kv.zig
🧠 Learnings (2)
📚 Learning: 2025-08-25T14:51:33.570Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#31
File: tests/jetstream_pull_test.zig:21-23
Timestamp: 2025-08-25T14:51:33.570Z
Learning: The nats.zig project uses a "tests:beforeEach" test in tests/all_tests.zig that runs before each test to clean up all JetStream streams and consumers, ensuring test isolation. This eliminates the need for unique test resource names as each test starts with a clean JetStream state.
Applied to files:
tests/jetstream_kv_test.zig
📚 Learning: 2025-09-06T20:49:10.762Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#98
File: src/result.zig:21-25
Timestamp: 2025-09-06T20:49:10.762Z
Learning: In Zig codebases including std.json.Parsed, ArenaAllocator is commonly heap-allocated (via allocator.create()) rather than stored by value when it needs to be moved between functions or returned from functions, to ensure proper lifetime management and avoid move semantics issues.
Applied to files:
src/jetstream_kv.zig
🔇 Additional comments (10)
src/jetstream_kv.zig (10)
357-369: Allocator consolidation looks goodDuping bucket/stream/subject strings via
js.allocatorwith correspondingdefererror paths is correct.
378-382: Correct teardownFreeing
bucket_name,stream_name, andsubject_prefixwithjs.allocatormatches the init path.
392-401: Reasonable error mapping for missing keysMapping
error.MessageNotFoundtoKVError.KeyNotFoundis consistent with ADR-8 semantics.
621-623: LGTMSubject construction for
watchAllvia{s}>and cleanup viadeferis correct.
690-696: Arena allocation pattern is solidHeap-allocating
ArenaAllocatorand returning it viaResultavoids move issues and centralizes cleanup. Matches the project learning.
736-739: Same here forkeys()Arena lifetime handling is correct; using
StringHashMapwith arena-backed keys is safe since the map doesn’t own key memory.
786-799: Filter subjects construction looks goodPre-sizing the
ArrayListand freeing each subject is clean.
805-808: Arena usage inkeysWithFilters()Consistent with
keys(). No leaks.
856-864: By-valueJetStreaminKVManagerhas the same pointer-to-field risk downstream
KVManagernow owns a copy ofJetStream. Any subs created through it will capture&self.jsunless subscriptions switch to owningJetStreamby value. Align with the subscription refactor suggested in src/jetstream.zig.
878-883: Manager paths LGTM
- Stream/subject strings allocated/freed via
js.allocator— OK.- Returning
KV.init(self.js, ...)matches the new API.- Error mapping in
openBucket()for missing streams reads sane.Also applies to: 918-919, 924-933, 939-941
The JetStream struct was duplicating allocator storage since it can access the allocator through the Connection pointer. This change: - Removes the allocator field from JetStream struct - Updates JetStream.init() to only take Connection and options - Replaces all self.allocator with self.nc.allocator - Updates Connection.jetstream() method accordingly - Fixes all related KV and test code All tests pass, confirming the refactoring maintains functionality while simplifying the struct.
Updates subscription objects to own their own copy of JetStream instead of holding a pointer reference. This improves lifetime management by eliminating potential dangling pointer issues. Changes: - PullSubscription.js: *JetStream -> JetStream - JetStreamSubscription.js: *JetStream -> JetStream - Updated subscription creation to pass JetStream value instead of pointer The JetStream struct is small (2 pointers) so copying is efficient. All tests pass confirming functionality is preserved.
Most JetStream methods now take `self: JetStream` (by value) instead of `self: *JetStream` (by pointer) since they only read from the struct and never modify it. This makes the API more ergonomic and prevents accidental mutations. Changes: - Most JetStream methods now take self by value - Removed unused JetStream.deinit() method entirely - Fixed maybeParseErrorResponse to not take JetStream parameter - Removed all defer js.deinit() calls from test files - Kept pointer signatures only where needed (subscription methods) Benefits: - More ergonomic API (no need for pointers in most cases) - Prevents accidental mutations (const by default) - Clearer semantics (read-only vs mutating operations) - Simpler usage patterns All 108 tests pass confirming functionality is preserved.
Similar to JetStream, KVManager methods don't modify the struct so they can take self by value instead of by pointer for better const correctness.
The JetStream.deinit() method was empty and unnecessary, so removing it along with all the defer js.deinit() calls from test files. Also updated one test to use const js since it's no longer modified.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jetstream_kv.zig (1)
865-917: Update to use KV.create instead of KV.init.Since
KV.initshould return a pointer (see previous comment), update these calls:- return try KV.init(self.js, config.bucket); + return try KV.create(self.js, config.bucket);
♻️ Duplicate comments (3)
src/jetstream.zig (1)
454-456: Fix potential lifetime issues with JetStream stored by value in subscriptions.The
PullSubscriptionstruct now storesjs: JetStreamby value, which is good. However, this creates a problem whenPullSubscriptionmethods accessself.js.nc.allocator- if the originalKVstruct that created this subscription moves or is deallocated, the nested references could become invalid.Since JetStream is just a thin wrapper around a
*Connectionpointer and options, storing it by value in subscriptions is actually safe - the Connection itself is heap-allocated and stable. The current implementation correctly copies the JetStream value into the subscription structs.src/jetstream_kv.zig (2)
920-931: Update to use KV.create instead of KV.init.- return try KV.init(self.js, bucket_name); + return try KV.create(self.js, bucket_name);
357-376: Critical: KV returns by value but owns heap resources - will cause memory issues.The
KVstruct allocates heap memory forbucket_name,stream_name, andsubject_prefixbut is returned by value. This violates Zig's ownership model and will lead to either:
- Double-frees if the struct is copied
- Memory leaks if
deinitisn't called- Use-after-free if the struct is moved
Change
initto return a heap-allocated pointer:- pub fn init(js: JetStream, bucket_name: []const u8) !KV { + pub fn create(js: JetStream, bucket_name: []const u8) !*KV { try validateBucketName(bucket_name); + const kv = try js.nc.allocator.create(KV); + errdefer js.nc.allocator.destroy(kv); + // Create owned copies of names const owned_bucket_name = try js.nc.allocator.dupe(u8, bucket_name); errdefer js.nc.allocator.free(owned_bucket_name); const stream_name = try std.fmt.allocPrint(js.nc.allocator, "KV_{s}", .{bucket_name}); errdefer js.nc.allocator.free(stream_name); const subject_prefix = try std.fmt.allocPrint(js.nc.allocator, "$KV.{s}.", .{bucket_name}); errdefer js.nc.allocator.free(subject_prefix); - return KV{ + kv.* = KV{ .js = js, .bucket_name = owned_bucket_name, .stream_name = stream_name, .subject_prefix = subject_prefix, }; + return kv; }Then add a corresponding
destroymethod:+ pub fn destroy(self: *KV) void { + self.deinit(); + self.js.nc.allocator.destroy(self); + }
🧹 Nitpick comments (1)
tests/jetstream_test.zig (1)
12-13: Remove the unused variable assignment.The
_ = js;line serves no purpose here. If you're not using the JetStream context in this test, just don't create it.- const js = conn.jetstream(.{}); - _ = js;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/connection.zig(1 hunks)src/jetstream.zig(35 hunks)src/jetstream_kv.zig(16 hunks)tests/jetstream_delete_msg_test.zig(0 hunks)tests/jetstream_duplicate_ack_test.zig(0 hunks)tests/jetstream_get_msg_direct_test.zig(0 hunks)tests/jetstream_get_msg_test.zig(0 hunks)tests/jetstream_kv_history_test.zig(0 hunks)tests/jetstream_kv_simple_test.zig(0 hunks)tests/jetstream_kv_test.zig(1 hunks)tests/jetstream_large_list_test.zig(0 hunks)tests/jetstream_nak_test.zig(0 hunks)tests/jetstream_pull_test.zig(0 hunks)tests/jetstream_push_test.zig(0 hunks)tests/jetstream_stream_purge_test.zig(0 hunks)tests/jetstream_stream_test.zig(0 hunks)tests/jetstream_sync_test.zig(0 hunks)tests/jetstream_test.zig(1 hunks)
💤 Files with no reviewable changes (13)
- tests/jetstream_kv_simple_test.zig
- tests/jetstream_kv_history_test.zig
- tests/jetstream_nak_test.zig
- tests/jetstream_delete_msg_test.zig
- tests/jetstream_sync_test.zig
- tests/jetstream_duplicate_ack_test.zig
- tests/jetstream_pull_test.zig
- tests/jetstream_push_test.zig
- tests/jetstream_get_msg_test.zig
- tests/jetstream_get_msg_direct_test.zig
- tests/jetstream_stream_test.zig
- tests/jetstream_large_list_test.zig
- tests/jetstream_stream_purge_test.zig
🧰 Additional context used
📓 Path-based instructions (1)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
Format code using
zig fmtbefore commit
Files:
tests/jetstream_kv_test.zigtests/jetstream_test.zigsrc/connection.zigsrc/jetstream_kv.zigsrc/jetstream.zig
🧠 Learnings (4)
📚 Learning: 2025-08-25T14:51:33.570Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#31
File: tests/jetstream_pull_test.zig:21-23
Timestamp: 2025-08-25T14:51:33.570Z
Learning: The nats.zig project uses a "tests:beforeEach" test in tests/all_tests.zig that runs before each test to clean up all JetStream streams and consumers, ensuring test isolation. This eliminates the need for unique test resource names as each test starts with a clean JetStream state.
Applied to files:
tests/jetstream_kv_test.zigtests/jetstream_test.zig
📚 Learning: 2025-09-06T20:49:10.762Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#98
File: src/result.zig:21-25
Timestamp: 2025-09-06T20:49:10.762Z
Learning: In Zig codebases including std.json.Parsed, ArenaAllocator is commonly heap-allocated (via allocator.create()) rather than stored by value when it needs to be moved between functions or returned from functions, to ensure proper lifetime management and avoid move semantics issues.
Applied to files:
src/jetstream_kv.zig
📚 Learning: 2025-08-25T02:49:59.119Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#28
File: src/response_manager.zig:89-105
Timestamp: 2025-08-25T02:49:59.119Z
Learning: In this NATS Zig codebase, subscriptions are reference counted, so calling deinit() on a subscription from multiple locations (like both Connection and ResponseManager) is safe and doesn't cause double-free issues.
Applied to files:
src/jetstream.zig
📚 Learning: 2025-09-06T20:28:53.149Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#98
File: src/jetstream_kv.zig:235-279
Timestamp: 2025-09-06T20:28:53.149Z
Learning: JetStreamMessage wrappers in nats.zig are allocated on the Message's owned arena, so they are automatically freed when Message.deinit() is called and don't require separate cleanup.
Applied to files:
src/jetstream.zig
🔇 Additional comments (2)
src/connection.zig (1)
1707-1708: LGTM!The change aligns perfectly with the PR's goal of removing JetStream pointers. The JetStream struct is lightweight (just a connection pointer and options), so returning it by value is appropriate.
src/jetstream.zig (1)
1243-1250: LGTM! Proper ownership transfer of JetStream value.The code correctly creates a heap-allocated
PullSubscriptionand copies the JetStream value into it. Since JetStream only contains a Connection pointer and options, this is a cheap copy operation that maintains proper ownership semantics.
Resolved conflicts by: - Accepting validation module changes from main - Keeping JetStream method signatures taking self by value - Keeping KVManager method signatures taking self by value - Adding subject validation to various methods
1 participant
Summary
Replace JetStream pointers with owned instances in KV, KVManager, and KVWatcher structs. This simplifies memory management since JetStream instances are cheap to create and don't require complex cleanup.
Changes
js: *JetStreamtojs: JetStream, removed separateallocatorfieldjs: *JetStreamtojs: JetStream, removed separateallocatorfieldkv.js.allocatorinstead of separate allocatorBenefits
Test Results
All 108 tests pass, including all KV functionality tests:
The changes maintain full compatibility with existing KV functionality while simplifying the internal implementation.