Repository navigation
fix(cmd-tlm-api): reap cable subscriptions whose disconnect never arrives #3948
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ryanmelt
wants to merge
7
commits into
main
Choose a base branch
from
fix/orphaned-broadcasters
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
aa4383f
fix(cmd-tlm-api): reap cable subscriptions whose disconnect never arr…
ryanmelt 994463f
fix(review): apply Claude review fixes (turn 1)
github-actions[bot] f9edf69
Merge pull request #4036 from OpenC3/ai-review/pr-3948-38196203780
ryanmelt 22dc3e2
Merge branch 'main' into fix/orphaned-broadcasters
ryanmelt 3c26160
fix(cmd-tlm-api): clamp cable ttl and reap subscriptions before first…
ryanmelt d6566d9
Merge remote-tracking branch 'origin/main' into fix/orphaned-broadcas…
ryanmelt fc29ea1
test(python): time template protocol initial delay from connect
ryanmelt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
82 changes: 82 additions & 0 deletions
82
openc3-cosmos-cmd-tlm-api/app/channels/application_cable/broadcaster_channel.rb
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| # encoding: ascii-8bit | ||
|
|
||
| # Copyright 2026 OpenC3, Inc. | ||
| # All Rights Reserved. | ||
| # | ||
| # This program is distributed in the hope that it will be useful, | ||
| # but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| # See LICENSE.md for more details. | ||
| # | ||
| # This file may also be used under the terms of a commercial license | ||
| # if purchased from OpenC3, Inc. | ||
|
|
||
| module ApplicationCable | ||
| # Shared lifecycle for channels that stream from a per-subscription | ||
| # broadcaster object. Including channels declare a key prefix with | ||
| # `broadcaster_prefix` and implement a private `create_broadcaster`. | ||
| # Broadcasters are tracked in BroadcasterRegistry so a subscription whose | ||
| # Disconnect never arrives is eventually reaped. | ||
| module BroadcasterChannel | ||
| extend ActiveSupport::Concern | ||
|
|
||
| # Sent to a heartbeating client whose broadcaster no longer exists (reaped | ||
| # or lost), telling it to resubscribe. Handled inside the frontend Cable | ||
| # wrapper so it never reaches the channel's received callback. | ||
| RESUBSCRIBE_MESSAGE = { '__openc3_cable__' => 'resubscribe' }.freeze | ||
|
|
||
| included do | ||
| # Identifies the broadcaster this subscription registered. Kept in | ||
| # AnyCable channel state because channel instances don't survive between | ||
| # the subscribe and unsubscribe RPCs. | ||
| state_attr_accessor :broadcaster_token | ||
| end | ||
|
|
||
| class_methods do | ||
| def broadcaster_prefix(prefix = nil) | ||
| @broadcaster_prefix = prefix if prefix | ||
| @broadcaster_prefix | ||
| end | ||
| end | ||
|
|
||
| def subscribed | ||
| # Defensive: if the auth before_subscribe callback rejected us, skip work. | ||
| return if subscription_rejected? | ||
| stream_from subscription_key | ||
| self.broadcaster_token = SecureRandom.uuid | ||
| # Clients that heartbeat say so when they subscribe so they can be reaped | ||
| # even if they die before sending their first heartbeat | ||
| BroadcasterRegistry.register(subscription_key, create_broadcaster(), | ||
| heartbeat: [true, 'true'].include?(params[:heartbeat]), token: broadcaster_token) | ||
| end | ||
|
|
||
| def unsubscribed | ||
| # No token means subscribed was rejected and nothing was registered | ||
| token = broadcaster_token | ||
| return unless token | ||
| stop_stream_from subscription_key | ||
| # Only remove our own registration. A resubscribe on the same connection | ||
| # shares the key, and if its subscribe is handled before this unsubscribe | ||
| # the newer broadcaster must survive. | ||
| BroadcasterRegistry.unregister(subscription_key, token: token) | ||
| end | ||
|
|
||
| def heartbeat(_data = nil) | ||
| unless BroadcasterRegistry.touch(subscription_key, heartbeat: true) | ||
| transmit(RESUBSCRIBE_MESSAGE) | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def subscription_key | ||
| "#{self.class.broadcaster_prefix}_#{uuid}" | ||
| end | ||
|
|
||
| # Any perform from the client also counts as activity | ||
| def broadcaster | ||
| BroadcasterRegistry.touch(subscription_key) | ||
| BroadcasterRegistry.get(subscription_key) | ||
| end | ||
| end | ||
| end |
182 changes: 182 additions & 0 deletions
182
openc3-cosmos-cmd-tlm-api/app/channels/application_cable/broadcaster_registry.rb
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| # encoding: ascii-8bit | ||
|
|
||
| # Copyright 2026 OpenC3, Inc. | ||
| # All Rights Reserved. | ||
| # | ||
| # This program is distributed in the hope that it will be useful, | ||
| # but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| # See LICENSE.md for more details. | ||
| # | ||
| # This file may also be used under the terms of a commercial license | ||
| # if purchased from OpenC3, Inc. | ||
|
|
||
| module ApplicationCable | ||
| # Process-wide registry of the per-subscription broadcaster objects | ||
| # (StreamingApi, MessagesApi, *EventsApi) that the channels create. | ||
| # | ||
| # Entries are normally removed by the channel's `unsubscribed`. Under AnyCable | ||
| # that arrives as a separate Disconnect RPC from anycable-go, and if it is | ||
| # never delivered (anycable-go restarting or crashing, the gRPC stream being | ||
| # torn down) the broadcaster and its Redis-reading threads would live for the | ||
| # rest of the process. To bound that, clients send a periodic `heartbeat` | ||
| # perform and a background sweeper reaps any entry that has stopped | ||
| # heartbeating for longer than TTL. | ||
| # | ||
| # Only entries registered as heartbeating (the client said it heartbeats when | ||
| # it subscribed) or that have sent a heartbeat are reaped. Older clients (and | ||
| # the Ruby/Python scripting WebSocket APIs) never heartbeat, and a realtime | ||
| # stream legitimately goes a long time with no client-to-server traffic, so | ||
| # reaping them on idle time alone would kill healthy streams. | ||
| class BroadcasterRegistry | ||
| # Clients heartbeat every 60s, so this tolerates several missed heartbeats | ||
| # (e.g. background tab timer throttling) before reaping | ||
| DEFAULT_TTL = 300 | ||
| DEFAULT_SWEEP_INTERVAL = 60 | ||
| # A ttl shorter than the client heartbeat interval would reap healthy | ||
| # clients between heartbeats, so never go below two heartbeats | ||
| MIN_TTL = 120 | ||
| # Keeps a zero or invalid interval from turning the sweeper into a busy loop | ||
| MIN_SWEEP_INTERVAL = 1 | ||
|
|
||
| Entry = Struct.new(:broadcaster, :last_activity, :heartbeat, :token) | ||
|
|
||
| @entries = {} | ||
| @mutex = Mutex.new | ||
| @sweeper = nil | ||
| @clamp_warnings = {} | ||
|
|
||
| class << self | ||
| def ttl | ||
| env_seconds('OPENC3_CABLE_SUBSCRIPTION_TTL', DEFAULT_TTL, MIN_TTL) | ||
| end | ||
|
|
||
| def sweep_interval | ||
| env_seconds('OPENC3_CABLE_SWEEP_INTERVAL', DEFAULT_SWEEP_INTERVAL, MIN_SWEEP_INTERVAL) | ||
| end | ||
|
|
||
| # heartbeat: true makes the entry reapable immediately rather than only | ||
| # after its first heartbeat, so a client that dies right after | ||
| # subscribing is still reaped. token identifies this registration so a | ||
| # late unregister from an older subscription can't remove a newer one. | ||
| def register(key, broadcaster, heartbeat: false, token: nil) | ||
| previous = nil | ||
| @mutex.synchronize do | ||
| previous = @entries[key]&.broadcaster | ||
| @entries[key] = Entry.new(broadcaster, now(), heartbeat, token) | ||
| end | ||
| # A duplicate subscribe for the same key must not leak the old one | ||
| kill(key, previous) if previous and !previous.equal?(broadcaster) | ||
| start_sweeper() | ||
| broadcaster | ||
| end | ||
|
|
||
| def get(key) | ||
| @mutex.synchronize { @entries[key]&.broadcaster } | ||
| end | ||
|
|
||
| def registered?(key) | ||
| @mutex.synchronize { @entries.key?(key) } | ||
| end | ||
|
|
||
| # Record client activity. Returns false if the key is not registered, | ||
| # e.g. because it was already reaped. | ||
| def touch(key, heartbeat: false) | ||
| @mutex.synchronize do | ||
| entry = @entries[key] | ||
| return false unless entry | ||
| entry.last_activity = now() | ||
| entry.heartbeat = true if heartbeat | ||
| return true | ||
| end | ||
| end | ||
|
|
||
| # When token is given only the registration made with that token is | ||
| # removed. Returns whether an entry was removed. | ||
| def unregister(key, token: nil) | ||
| entry = @mutex.synchronize do | ||
| current = @entries[key] | ||
| next nil unless current | ||
| next nil if token and current.token != token | ||
| @entries.delete(key) | ||
| end | ||
| kill(key, entry.broadcaster) if entry and entry.broadcaster | ||
| !entry.nil? | ||
| end | ||
|
|
||
| # Remove and kill every heartbeating entry idle for longer than ttl. | ||
| # Returns the reaped keys. | ||
| def sweep(current_time = now()) | ||
| expired = [] | ||
| @mutex.synchronize do | ||
| cutoff = current_time - ttl() | ||
| @entries.each do |key, entry| | ||
| expired << [key, entry] if entry.heartbeat and entry.last_activity < cutoff | ||
| end | ||
| expired.each { |key, _| @entries.delete(key) } | ||
| end | ||
| # Kill outside the lock as it may wait on the broadcaster's threads | ||
| expired.each do |key, entry| | ||
| OpenC3::Logger.warn("Reaping cable subscription #{key}: no heartbeat in #{(current_time - entry.last_activity).round}s") | ||
| kill(key, entry.broadcaster) if entry.broadcaster | ||
| end | ||
| expired.map(&:first) | ||
| end | ||
|
|
||
| def size | ||
| @mutex.synchronize { @entries.length } | ||
| end | ||
|
|
||
| # For tests | ||
| def clear | ||
| @mutex.synchronize do | ||
| @entries.clear | ||
| @clamp_warnings.clear | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def now | ||
| Process.clock_gettime(Process::CLOCK_MONOTONIC) | ||
| end | ||
|
|
||
| def env_seconds(name, default, minimum) | ||
| raw = ENV.fetch(name, nil) | ||
| return default.to_f if raw.nil? or raw.to_s.strip.empty? | ||
| value = Float(raw, exception: false) | ||
| return value if value and value >= minimum | ||
| fallback = value ? minimum.to_f : default.to_f | ||
| # Only warn once per value, ttl is read on every sweep | ||
| unless @clamp_warnings[name] == raw | ||
| @clamp_warnings[name] = raw | ||
| OpenC3::Logger.warn("#{name}=#{raw} is invalid or below the minimum of #{minimum}s, using #{fallback}s") | ||
| end | ||
| fallback | ||
| end | ||
|
|
||
| def kill(key, broadcaster) | ||
| broadcaster.kill | ||
| rescue => e | ||
| OpenC3::Logger.error("Error killing cable subscription #{key}: #{e.formatted}") | ||
| end | ||
|
|
||
| def start_sweeper | ||
| return if @sweeper&.alive? | ||
| @mutex.synchronize do | ||
| return if @sweeper&.alive? | ||
| @sweeper = Thread.new do | ||
| loop do | ||
| sleep(sweep_interval()) | ||
| begin | ||
| sweep() | ||
| rescue => e | ||
| OpenC3::Logger.error("Cable subscription sweeper error: #{e.formatted}") | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like
entry.heartbeat andis here because non-JS clients don't send heartbeats, but this means that an entry where a JS client immediately died after opening the connection will never be cleaned up (which I don't think is a terribly uncommon edge case tbh). There are ways to handle that, but doing so elegantly and robustly is a hard problem. I'll let @ryanmelt decide whether we care about this bug. Might be able to just initializeheartbeatto true for those connections.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this is worth fixing. It also hits the main case this PR targets: when anycable-go restarts, any subscription opened in the last minute before the restart would never be reaped.
Fixed in 3c26160, along the lines you suggested. The server can't tell at subscribe time whether a client will heartbeat, so the client now says so.
Cableaddsheartbeat: trueto the subscribe params, only on the cmd-tlm-api cable where heartbeats are on.BroadcasterChannelregisters those subscriptions withheartbeat: true, so they can be reaped from the moment they subscribe. Older cached frontends and the Ruby/Python scripting APIs don't send the param, so they still never get reaped.The same commit also makes
unsubscribedremove only its own registration. It checks a per-subscription token kept in AnyCable channel state. Before this, a late unsubscribe from an earlier subscription on the same connection could kill the broadcaster of a resubscribe that shares its key.Generated by Claude Code