Skip to content

fix: guard against nil onAck in result-returning *Async methods#702

Open
lmars wants to merge 1 commit into
mainfrom
fix/append-message-async-nil-callback
Open

fix: guard against nil onAck in result-returning *Async methods#702
lmars wants to merge 1 commit into
mainfrom
fix/append-message-async-nil-callback

Conversation

@lmars
Copy link
Copy Markdown
Member

@lmars lmars commented May 15, 2026

Summary

  • AppendMessageAsync(msg, nil) (and the UpdateMessageAsync / DeleteMessageAsync siblings) segfaulted when the server ACK arrived, because performMessageOperationAsync invoked the callback unconditionally at realtime_channel.go:908.
  • The same latent bug existed in PublishWithResultAsync and PublishMultipleWithResultAsync.
  • Normalize a nil onAck to a no-op at the top of each — mirroring the nil-safety that msgAckCallback.call already provides at the lower layer.

Surfaced by ably/docs#3351, where a fire-and-forget append example using AppendMessageAsync(msg, nil) crashed at runtime.

Test plan

  • New integration sub-test TestRealtimeChannel_MessageUpdates/AppendMessageAsync/nil_onAck_is_treated_as_fire-and-forget — calls AppendMessageAsync(msg, nil), then round-trips a follow-up sync AppendMessage so we know the ACK for the nil-callback append landed without panicking the connection's read loop. Passes against nonprod:sandbox in ~0.6s.
  • go vet ./ably/... clean
  • go build ./... clean

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Async message publish operations now handle missing callbacks gracefully, enabling fire-and-forget usage patterns without errors or crashes. Improved stability for asynchronous publishing workflows.

Review Change Stack

Calling AppendMessageAsync (or the UpdateMessageAsync/DeleteMessageAsync
siblings) with a nil onAck caused a segfault when the server ACK arrived,
because performMessageOperationAsync invoked the callback unconditionally.
PublishWithResultAsync and PublishMultipleWithResultAsync had the same
latent bug. Normalize a nil onAck to a no-op at the top of each, mirroring
the nil-safety msgAckCallback.call already provides at the lower layer.

This makes fire-and-forget patterns like AppendMessageAsync(msg, nil)
valid, as the docs and AI-streaming use case assume.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ecd18c44-c355-460e-88f9-729356e43272

📥 Commits

Reviewing files that changed from the base of the PR and between 439d9f3 and b9e6eeb.

📒 Files selected for processing (2)
  • ably/message_updates_integration_test.go
  • ably/realtime_channel.go

Walkthrough

This pull request adds nil onAck callback tolerance to async message operations in RealtimeChannel. Three functions now install a default no-op handler when the callback is nil, enabling fire-and-forget usage without panic. A new integration test validates the safe behavior.

Changes

Nil acknowledgment callback handling

Layer / File(s) Summary
Nil callback guard in async message operations
ably/realtime_channel.go, ably/message_updates_integration_test.go
PublishWithResultAsync, PublishMultipleWithResultAsync, and performMessageOperationAsync now assign a default no-op onAck when the callback is nil, treating async publishes as fire-and-forget. A new subtest validates this behavior by calling AppendMessageAsync with nil callback and confirming the connection read loop processes the operation without panic.

🎯 2 (Simple) | ⏱️ ~8 minutes

🐰 A hop, skip, and fire away!
When callbacks are nil, no panic today
Async ops dance in a fire-and-forget way
The tests hop in to save the day
Safe and sound in every pathway! 🔥

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary change: fixing nil onAck handling in result-returning async methods to prevent segfaults.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/append-message-async-nil-callback

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant