Implement comprehensive JetStream error handling #91
Conversation
WalkthroughIntroduces a script to generate JetStream error mappings from NATS errors.json, adds the generated Zig module with error set and code-to-error mapping, updates jetstream.zig to use centralized mapping and removes JetStreamPublishError, exports JetStreamError via root, and adjusts tests to assert specific JetStreamError variants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
- Replace custom JetStreamPublishError with comprehensive error mapping - Add jetstream_errors.zig generated from NATS server error definitions - Create scripts/generate_jetstream_errors.py for error code generation - Update all 196 JetStream error codes with proper Zig error types - Fix expected_* option error handling to use specific error types - Update tests to expect specific errors (StreamWrongLastSequence, NoMessageFound, etc.) - Expose PublishOptions and JetStreamError in public API The implementation now provides precise error discrimination for all JetStream operations, making it easier for users to handle specific error conditions. All tests pass with the new comprehensive error handling.
lalinsky
force-pushed
the
feature/jetstream-comprehensive-error-handling
branch
from
dfed3d2 to
bdd750a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
src/jetstream_errors.zig (2)
4-5: Drop unused std import (adjust generator).std isn’t used here. It’s harmless in practice, but we can avoid “unused import” churn and keep the generated file lean. Since this file is generated, please update the generator to omit this line.
Apply (for current file only; see script fix in a separate comment):
-const std = @import("std"); -
404-605: Optional: add reverse mapping helper (error -> code).Having a generated helper like codeFor(err: JetStreamError) u32 can simplify telemetry and assertions without duplicating switches in callers.
Example addition the generator could emit:
pub fn codeFor(err: JetStreamError) u32 { return switch (err) { // mirror of mapErrorCode, e.g.: .StreamWrongLastSequence => 10071, // ... else => 0, }; }scripts/generate_jetstream_errors.py (4)
1-1: Make the script executable or drop the shebang.Either set the executable bit in the repo (preferred) or remove the shebang to silence EXE001.
To make it executable in git:
- git update-index --chmod=+x scripts/generate_jetstream_errors.py
50-58: Don’t emit an unused std import into the generated Zig.The generated file doesn’t use std; drop it from zig_content to keep output minimal and avoid potential “unused constant” warnings on some Zig versions.
- zig_content = [ - f"// Generated from {errors_file}", - "// DO NOT EDIT MANUALLY - regenerate using scripts/generate_jetstream_errors.py", - "", - "const std = @import(\"std\");", - "", - "/// JetStream error codes and their corresponding Zig error types", - "pub const JetStreamError = error{", - ] + zig_content = [ + f"// Generated from {errors_file}", + "// DO NOT EDIT MANUALLY - regenerate using scripts/generate_jetstream_errors.py", + "", + "/// JetStream error codes and their corresponding Zig error types", + "pub const JetStreamError = error{", + ]
7-10: Optional: auto-format the generated Zig via zig fmt (if available).Keeps us aligned with “zig fmt before commit” without relying on manual steps.
-import json -import sys -from pathlib import Path +import json +import sys +import shutil +import subprocess +from pathlib import Pathwith open(output_file, 'w') as f: f.write('\n'.join(zig_content)) print(f"Generated {output_file} with {len(js_errors)} JetStream error codes") + + # Format with zig fmt if available + zig_path = shutil.which("zig") + if zig_path: + try: + subprocess.run([zig_path, "fmt", str(output_file)], check=True) + except Exception as e: + print(f"Warning: zig fmt failed: {e}")
122-122: Fix Ruff F541: remove unnecessary f-string.-print(f"Error breakdown by HTTP status code:") +print("Error breakdown by HTTP status code:")tests/jetstream_test.zig (1)
763-798: Add coverage for expected_last_msg_id → StreamWrongLastMsgID (10070).You’re covering 10071 well. A quick test using .{ .expected_last_msg_id = "some-id" } with a mismatched last ID would validate 10070 end-to-end.
Sketch:
test "JetStream publish with expected last msg id mismatch" { const conn = try utils.createDefaultConnection(); defer utils.closeConnection(conn); var js = conn.jetstream(.{}); defer js.deinit(); const stream = try utils.generateUniqueStreamName(testing.allocator); defer testing.allocator.free(stream); const subject = try utils.generateSubjectFromStreamName(testing.allocator, stream); defer testing.allocator.free(subject); var _ = try js.addStream(.{ .name = stream, .subjects = &.{subject} }); // Publish one message with a known msg_id _ = try js.publish(subject, "first", .{ .msg_id = "id-1" }); // Mismatch expected_last_msg_id const res = js.publish(subject, "second", .{ .expected_last_msg_id = "id-other" }); try testing.expectError(nats.JetStreamError.StreamWrongLastMsgID, res); }src/jetstream.zig (2)
1196-1197: Publish: map NoResponders → NoStreamResponse — good; consider centralizing in sendRequest().This is the right UX for publish. For consistency across all JetStream API calls, consider doing the same mapping in sendRequest() so INFO/STREAM/CONSUMER calls get the same error when JetStream is unavailable:
fn sendRequest(self: *JetStream, subject: []const u8, payload: []const u8) !*Message { const full_subject = try std.fmt.allocPrint(self.allocator, "{s}{s}", .{ default_api_prefix, subject }); defer self.allocator.free(full_subject); return self.nc.request(full_subject, payload, self.opts.request_timeout_ms) catch |e| { if (e == error.NoResponders) return error.NoStreamResponse; return e; }; }
602-604: Map server err_code to concrete errors—verify fallback and consider larger parse buffer.
- Verify that
mapErrorCode(info.err_code)covers unknown or zero codes, falling back to a genericJetStreamErrorif needed.- For long error descriptions, bump the 1024-byte parse buffer (near Line 590) to at least 2048 bytes (or 4096) to avoid silent parse failures.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
scripts/generate_jetstream_errors.py(1 hunks)src/jetstream.zig(3 hunks)src/jetstream_errors.zig(1 hunks)src/root.zig(1 hunks)tests/jetstream_msg_test.zig(4 hunks)tests/jetstream_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/root.zigtests/jetstream_test.zigsrc/jetstream_errors.zigtests/jetstream_msg_test.zigsrc/jetstream.zig
🧠 Learnings (2)
📓 Common learnings
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.
📚 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_test.zigtests/jetstream_msg_test.zig
🪛 Ruff (0.12.2)
scripts/generate_jetstream_errors.py
1-1: Shebang is present but file is not executable
(EXE001)
122-122: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (6)
src/jetstream_errors.zig (1)
145-148: Good: precise mapping for 10070, 10071, 10164.StreamWrongLastMsgID (10070), StreamWrongLastSequence (10071), and StreamWrongLastSequenceConstant (10164) are distinctly represented and mapped. This matches the server behavior tests care about.
Also applies to: 333-334, 475-477, 569-569
src/root.zig (1)
35-36: Re-exporting JetStreamError and PublishOptions looks right.This matches the new centralized error mapping and keeps the public API coherent.
tests/jetstream_msg_test.zig (1)
123-124: Good: assert specific JetStreamError variants.Asserting NoMessageFound, StreamNotFound, and StreamMsgDeleteFailed tightens the contract and validates the mapping end-to-end. Given your beforeEach cleanup, the fixed stream names are fine.
Also applies to: 167-168, 227-228, 239-240, 243-244, 247-248
tests/jetstream_test.zig (1)
758-761: Good: expect StreamWrongLastSequence for bad expected_last_seq.This nails the 10071 path.
src/jetstream.zig (2)
41-41: Import of generated JetStream error mapping — good.This correctly wires the file to the centralized, generated error set.
1204-1205: Publish: invalid PubAck JSON → InvalidJSAck — good.Matches the new error model and keeps publish errors specific.
1 participant
Summary
Replaces custom JetStream error handling with comprehensive error mapping based on official NATS server error definitions. This provides precise error discrimination for all JetStream operations.
Changes
jetstream_errors.zigwith all 196 JetStream error codes from NATS server definitionsexpected_*Error Handling: Now returns specific errors likeStreamWrongLastSequence,StreamWrongLastMsgIDinstead of genericJetStreamErrorscripts/generate_jetstream_errors.pyto maintain error mappings from official NATS server sourcesPublishOptionsandJetStreamErrorin public APITechnical Details
JetStreamPublishErrorenum in favor of comprehensiveJetStreamError_refs/nats-server/server/errors.jsonError Code Mappings
StreamWrongLastMsgID- forexpected_last_msg_idvalidationStreamWrongLastSequence- forexpected_last_seqandexpected_last_subject_seqvalidationStreamWrongLastSequenceConstant- future compatibilityTest Plan
[✅] All existing tests pass with updated error expectations
[✅] Error handling works correctly for
expected_*publish options[✅] Comprehensive error mapping covers all JetStream operations