Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions .github/workflows/sdk-compliance.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
name: SDK Compliance Tests

permissions:
contents: read
packages: read
pull-requests: write

on:
pull_request:
push:
branches:
- main

jobs:
compliance:
name: PostHog SDK compliance tests
uses: PostHog/posthog-sdk-test-harness/.github/workflows/test-sdk-action.yml@be8b8d5a3f94a249659844e94832e874f049c1e4
with:
adapter-dockerfile: "sdk_compliance_adapter/Dockerfile"
adapter-context: "."
test-harness-version: "0.8.0"
2 changes: 1 addition & 1 deletion lib/posthog/backoff_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def next_interval

@attempts += 1

[interval, @max_timeout_ms].min
interval.clamp(@min_timeout_ms, @max_timeout_ms)
end

private
Expand Down
6 changes: 5 additions & 1 deletion lib/posthog/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ def _decrement_instance_count(api_key)
# in seconds. Defaults to 30.
# @option opts [Integer] :feature_flag_request_timeout_seconds How long to wait for feature flag evaluation,
# in seconds. Defaults to 3.
# @option opts [Integer] :max_retries How many times to retry batch uploads after the first send attempt.
# Defaults to the transport default. Set to 0 to disable retrying.
# @option opts [Boolean] :enable_compression +true+ to gzip batch upload request bodies.
# @option opts [Integer] :feature_flag_request_max_retries How many times to retry a flag request after a
# transient network error. Each retry sleeps on the calling thread before retrying, so this adds to
# worst-case latency. Defaults to 1. Set to 0 to disable retrying.
Expand Down Expand Up @@ -109,7 +112,8 @@ def initialize(opts = {})
@transport = Transport.new(
api_host: opts[:host],
skip_ssl_verification: opts[:skip_ssl_verification],
retries: 3
retries: opts.key?(:max_retries) ? opts[:max_retries].to_i + 1 : 3,
gzip: opts[:enable_compression] == true
)
@sync_lock = Mutex.new
end
Expand Down
2 changes: 1 addition & 1 deletion lib/posthog/feature_flags.rb
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def get_flags(distinct_id, groups = {}, person_properties = {}, group_properties
group_properties: group_properties
}
request_data[:flag_keys_to_evaluate] = flag_keys if flag_keys && !flag_keys.empty?
request_data[:geoip_disable] = true if disable_geoip
request_data[:geoip_disable] = disable_geoip unless disable_geoip.nil?

flags_response = _request_feature_flag_evaluation(request_data)

Expand Down
2 changes: 2 additions & 0 deletions lib/posthog/send_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ def initialize(queue, api_key, options = {})
@shutdown = false
@pid = Process.pid
@transport_options = { api_host: options[:host], skip_ssl_verification: options[:skip_ssl_verification] }
@transport_options[:retries] = options[:max_retries].to_i + 1 if options.key?(:max_retries)
@transport_options[:gzip] = true if options[:enable_compression] == true
@transport = Transport.new(@transport_options)
end

Expand Down
47 changes: 41 additions & 6 deletions lib/posthog/transport.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
require 'net/http'
require 'net/https'
require 'json'
require 'stringio'
require 'time'
require 'zlib'

module PostHog
# HTTP transport used by the SDK workers.
Expand Down Expand Up @@ -40,10 +43,12 @@ def initialize(options = {})
options[:port] = options[:port].nil? ? PORT : options[:port]
options[:ssl] = options[:ssl].nil? ? SSL : options[:ssl]

@headers = options[:headers] || HEADERS
@headers = (options[:headers] || HEADERS).dup
@path = options[:path] || PATH
@retries = options[:retries] || RETRIES
@backoff_policy = options[:backoff_policy] || PostHog::BackoffPolicy.new
@gzip = options[:gzip] == true
@last_retry_after = nil

http = Net::HTTP.new(options[:host], options[:port])
http.use_ssl = options[:ssl]
Expand Down Expand Up @@ -100,10 +105,8 @@ def shutdown
private

def should_retry_request?(status_code, body)
if status_code >= 500
true # Server error
elsif status_code == 429 # rubocop:disable Lint/DuplicateBranch
true # Rate limited
if status_code >= 500 || [408, 429].include?(status_code)
true # Server error, request timeout, or rate limited
elsif status_code >= 400
logger.error(body)
false # Client error. Do not retry, but log
Expand Down Expand Up @@ -133,18 +136,49 @@ def retry_with_backoff(retries_remaining, &block)

if should_retry && (retries_remaining > 1)
logger.debug("Retrying request, #{retries_remaining} retries left")
sleep(@backoff_policy.next_interval.to_f / 1000)
sleep(retry_delay_seconds)
retry_with_backoff(retries_remaining - 1, &block)
else
[result, caught_exception]
end
end

def retry_delay_seconds
retry_after = parse_retry_after(@last_retry_after)
@last_retry_after = nil
return retry_after if retry_after

@backoff_policy.next_interval.to_f / 1000
end

def parse_retry_after(value)
return nil if value.nil? || value.empty?

seconds = Float(value, exception: false)
return seconds if seconds&.positive?
Comment on lines +157 to +158

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Retry-After: 0 means "retry immediately", but 0.0.positive? returns false, so the method falls through to the HTTP-date branch, fails to parse "0", and returns nil. That causes the caller to use the backoff policy instead of obeying the header. Using seconds&.>=(0) matches the semantics of the header spec (any non-negative number is a valid delay, including zero).

Suggested change
seconds = Float(value, exception: false)
return seconds if seconds&.positive?
seconds = Float(value, exception: false)
return seconds if seconds&.>=(0)


parsed_time = Time.httpdate(value)
delay = parsed_time - Time.now
delay.positive? ? delay : nil
rescue ArgumentError
nil
end

def gzip(payload)
io = StringIO.new
Zlib::GzipWriter.wrap(io) { |gzip| gzip.write(payload) }
io.string
end

# Sends a request for the batch, returns [status_code, body]
def send_request(api_key, batch)
payload = JSON.generate(api_key: api_key, batch: batch)
Comment on lines 173 to 175

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Stale @last_retry_after can leak into the next batch's first retry delay. When retries are exhausted after a 429 response that carries a Retry-After header, retry_delay_seconds is never called (because retries_remaining > 1 is false) and @last_retry_after retains the header value. On the next call to send, if @http.request raises an exception before the mutex block can overwrite the field, retry_delay_seconds reads the stale header and sleeps for that many seconds — potentially thousands of seconds — instead of using the backoff policy. Clearing the field unconditionally at the start of send_request closes the gap.

Suggested change
# Sends a request for the batch, returns [status_code, body]
def send_request(api_key, batch)
payload = JSON.generate(api_key: api_key, batch: batch)
# Sends a request for the batch, returns [status_code, body]
def send_request(api_key, batch)
@last_retry_after = nil
payload = JSON.generate(api_key: api_key, batch: batch)


request = Net::HTTP::Post.new(@path, @headers)
if @gzip
payload = gzip(payload)
request['Content-Encoding'] = 'gzip'
end

if self.class.stub
logger.debug "stubbed request to #{@path}: " \
Expand All @@ -155,6 +189,7 @@ def send_request(api_key, batch)
@http_mutex.synchronize do
@http.start unless @http.started? # Maintain a persistent connection
response = @http.request(request, payload)
@last_retry_after = response['Retry-After']
[response.code.to_i, response.body]
end
end
Expand Down
14 changes: 14 additions & 0 deletions sdk_compliance_adapter/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
FROM ruby:3.3-slim

WORKDIR /app

RUN gem install concurrent-ruby --no-document

COPY lib/ /app/lib/
COPY sdk_compliance_adapter/adapter.rb /app/adapter.rb

ENV RUBYLIB=/app/lib

EXPOSE 8080

CMD ["ruby", "/app/adapter.rb"]
Loading
Loading