From c00a32dbbe2c1c1d1ee1719908ab0bfc04e40879 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Thu, 12 Nov 2026 11:26:46 +0100 Subject: [PATCH 1/2] fix(flags): treat failed remote records as unevaluated --- .changeset/fix-failed-flag-records.md | 5 + lib/posthog/client.rb | 9 +- lib/posthog/feature_flag_evaluations.rb | 18 +- lib/posthog/feature_flags.rb | 18 +- spec/posthog/failed_flag_records_spec.rb | 170 ++++++++++++++++++ spec/posthog/feature_flag_error_spec.rb | 11 +- spec/posthog/feature_flag_evaluations_spec.rb | 20 +-- 7 files changed, 221 insertions(+), 30 deletions(-) create mode 100644 .changeset/fix-failed-flag-records.md create mode 100644 spec/posthog/failed_flag_records_spec.rb diff --git a/.changeset/fix-failed-flag-records.md b/.changeset/fix-failed-flag-records.md new file mode 100644 index 00000000..259d3fa0 --- /dev/null +++ b/.changeset/fix-failed-flag-records.md @@ -0,0 +1,5 @@ +--- +"posthog-ruby": patch +--- + +Treat failed feature flag records as unevaluated, report evaluation failure diagnostics, and preserve successful local values and payloads when remote fallback fails. diff --git a/lib/posthog/client.rb b/lib/posthog/client.rb index fd3f9522..545470da 100644 --- a/lib/posthog/client.rb +++ b/lib/posthog/client.rb @@ -784,11 +784,14 @@ def evaluate_flags( metadata = ff.metadata reason = ff.reason + enabled = ff.enabled ? true : false + # nil marks no evaluated value; retain the record only for failure diagnostics. + enabled = nil if ff.failed == true records[key_str] = FeatureFlagEvaluations::EvaluatedFlagRecord.new( key: key_str, - enabled: ff.enabled ? true : false, - variant: ff.variant, - payload: FeatureFlagResult.parse_payload(ff.payload), + enabled: enabled, + variant: ff.failed == true ? nil : ff.variant, + payload: ff.failed == true ? nil : FeatureFlagResult.parse_payload(ff.payload), id: metadata&.id, version: metadata&.version, reason: reason ? (reason.description || reason.code) : nil, diff --git a/lib/posthog/feature_flag_evaluations.rb b/lib/posthog/feature_flag_evaluations.rb index 5129dcec..4efa27b8 100644 --- a/lib/posthog/feature_flag_evaluations.rb +++ b/lib/posthog/feature_flag_evaluations.rb @@ -76,7 +76,10 @@ def initialize( ) @host = host @distinct_id = distinct_id || '' - @flags = flags || {} + # Remote failures have no enabled value, unlike healthy disabled records. + failed, evaluated = (flags || {}).partition { |_, flag| flag.enabled.nil? } + @failed_flags = failed.to_h + @flags = evaluated.to_h @groups = groups @disable_geoip = disable_geoip @request_id = request_id @@ -98,9 +101,8 @@ def keys # @param default_value [Boolean, nil] Returned when the flag has no value in this # snapshot — it was never loaded, the `/flags` request failed, or no flag with that # key exists. A flag that does have a value, including `false` and variant strings, - # always wins over this default; so does a flag the server returned but marked as - # failed, which resolves to `false`. Defaults to `nil`, which keeps the historical - # `false` result for a missing flag. + # always wins over this default. A failed remote record has no value. Defaults to + # `nil`, which keeps the historical `false` result for a missing flag. # @return [Boolean] true when the flag is enabled, false when disabled, and the # caller-supplied default (or false when none was supplied) when the flag is missing. def enabled?(key, default_value: nil) @@ -177,7 +179,7 @@ def _get_event_properties # Variant string when present, else boolean enabled status; `nil` for # unknown flags. def _flag_value(flag) - return nil if flag.nil? + return nil if flag.nil? || flag.enabled.nil? return flag.variant if flag.variant flag.enabled ? true : false @@ -187,6 +189,8 @@ def _record_access(key, flag) @accessed.add(key) return if @distinct_id.nil? || @distinct_id.to_s.empty? + failed = @failed_flags.key?(key) + flag ||= @failed_flags[key] response = _flag_value(flag) properties = { '$feature_flag' => key, @@ -212,7 +216,9 @@ def _record_access(key, flag) errors = [] errors << 'errors_while_computing_flags' if @errors_while_computing errors << 'quota_limited' if @quota_limited - if flag.nil? && @unresolved_flags.key?(key) + if failed + errors << 'evaluation_failed' + elsif flag.nil? && @unresolved_flags.key?(key) errors << FeatureFlagError::LOCAL_EVALUATION_INCONCLUSIVE elsif flag.nil? errors << FeatureFlagError::FLAG_MISSING diff --git a/lib/posthog/feature_flags.rb b/lib/posthog/feature_flags.rb index 64b2cc2e..c0f730ce 100644 --- a/lib/posthog/feature_flags.rb +++ b/lib/posthog/feature_flags.rb @@ -302,14 +302,19 @@ def get_feature_flag( errors << FeatureFlagError.api_error(status) if status && status >= 400 errors << FeatureFlagError::ERRORS_WHILE_COMPUTING if flags_data[:errorsWhileComputingFlags] errors << FeatureFlagError::QUOTA_LIMITED if flags_data[:quotaLimited]&.include?('feature_flags') - errors << FeatureFlagError::FLAG_MISSING unless flags.key?(key.to_s) + flag_detail = flags_data[:flagDetails]&.[](key.to_sym) + failed = flag_detail&.failed == true && !flags.key?(key) + if failed + errors << 'evaluation_failed' + elsif !flags.key?(key) + errors << FeatureFlagError::FLAG_MISSING + end response = flags[key] - response = false if response.nil? + response = false if response.nil? && !failed payload = payloads[key] feature_flag_error = errors.join(',') unless errors.empty? - flag_detail = flags_data[:flagDetails]&.[](key.to_sym) has_experiment = flag_detail&.metadata&.has_experiment minimal_flag_called_events = flags_data[:minimalFlagCalledEvents] @@ -422,8 +427,11 @@ def get_all_flags_and_payloads( flags = stringify_keys(flags).merge(server_flags) payloads = stringify_keys(payloads).merge(server_payloads) else - flags = server_flags - payloads = server_payloads + # A failed record must preserve a successful same-context local pair, + # independently of the response-wide error bit. + failed_keys = (flag_details || {}).select { |_, flag| flag.failed == true }.keys.map(&:to_s) + flags = stringify_keys(flags).slice(*failed_keys).merge(server_flags) + payloads = stringify_keys(payloads).slice(*failed_keys).merge(server_payloads) end end rescue StandardError => e diff --git a/spec/posthog/failed_flag_records_spec.rb b/spec/posthog/failed_flag_records_spec.rb new file mode 100644 index 00000000..a29f4aeb --- /dev/null +++ b/spec/posthog/failed_flag_records_spec.rb @@ -0,0 +1,170 @@ +# frozen_string_literal: true + +require 'spec_helper' + +module PostHog + describe 'failed flag records (spec 211)' do + let(:flags_endpoint) { 'https://us.i.posthog.com/flags/?v=2' } + let(:definitions_endpoint) { 'https://us.i.posthog.com/flags/definitions?token=testsecret&send_cohorts=true' } + let(:client) { Client.new(api_key: API_KEY, test_mode: true) } + + def failed_record(key, variant = nil) + { + key: key, enabled: false, variant: variant, failed: true, + reason: { code: 'dependency_error', description: 'Evaluation failed' }, + metadata: { id: 1, version: 2, payload: '{"copy":"remote"}' } + } + end + + def local_flag(key, value) + { + key: key, active: value != false, + filters: { + groups: [{ properties: [], rollout_percentage: 100, variant: value.is_a?(String) ? value : nil }], + multivariate: value.is_a?(String) ? { variants: [{ key: 'blue', rollout_percentage: 100 }] } : nil, + payloads: { value.to_s => '{"copy":"local"}' } + } + } + end + + it 'does not label a retained successful fallback value as failed' do + poller = client.instance_variable_get(:@feature_flags_poller) + allow(poller).to receive(:get_all_flags_and_payloads).and_return( + featureFlags: { checkout: 'blue' }, featureFlagPayloads: { checkout: '{"copy":"local"}' }, + flagDetails: { checkout: FeatureFlag.new(failed_record('checkout')) }, errorsWhileComputingFlags: true + ) + result = client.get_feature_flag_result('checkout', 'user') + expect(result.value).to eq('blue') + expect(result.payload).to eq('copy' => 'local') + error = client.dequeue_last_message[:properties]['$feature_flag_error'] + expect(error).to eq('errors_while_computing_flags') + end + + it 'delivers failed diagnostics through the public getter and flush transport path' do + stub_request(:post, flags_endpoint).to_return( + status: 200, body: { flags: { checkout: failed_record('checkout', 'placeholder') }, + errorsWhileComputingFlags: true }.to_json + ) + delivered = [] + ingestion = stub_request(:post, 'https://us.i.posthog.com/batch/').to_return do |request| + delivered.concat(JSON.parse(request.body).fetch('batch')) + { status: 200, body: '{}' } + end + wire_client = Client.new(api_key: API_KEY, sync_mode: true, compress_request: false) + expect(wire_client.get_feature_flag_result('checkout', 'user')).to be_nil + expect(wire_client.flush).to be(true) + expect(ingestion).to have_been_requested.once + expect(delivered.size).to eq(1) + props = delivered.first.fetch('properties') + expect(props['$feature_flag_response']).to be_nil + expect(props['$feature_flag_error'].split(',')).to contain_exactly( + 'evaluation_failed', 'errors_while_computing_flags' + ) + ensure + wire_client&.shutdown + end + + [nil, false, true].each do |error_bit| + [nil, 'placeholder'].each do |variant| + context "with global errors #{error_bit.inspect} and failed variant #{variant.inspect}" do + let(:response) do + result = { + flags: { + checkout: failed_record('checkout', variant), + off: { key: 'off', enabled: false, failed: false } + }, + featureFlags: { checkout: 'mirror' }, featureFlagPayloads: { checkout: '"mirror"' } + } + result[:errorsWhileComputingFlags] = error_bit unless error_bit.nil? + result + end + + before do + stub_request(:post, flags_endpoint).to_return(status: 200, body: response.to_json) + end + + it 'returns no rich/scalar result and reports failure with the missing response, dedupe, and suppression' do + expect(client.get_feature_flag_result('checkout', 'user', send_feature_flag_events: false)).to be_nil + expect(client.queued_messages).to eq(0) + expect(client.get_feature_flag_result('checkout', 'user')).to be_nil + expect(client.get_feature_flag('checkout', 'user')).to be_nil + expect(client.is_feature_enabled('checkout', 'user')).to be_nil + expect(client.queued_messages).to eq(1) + event = client.dequeue_last_message + expect(event[:properties]['$feature_flag_response']).to be_nil + errors = event[:properties]['$feature_flag_error'].split(',') + expect(errors).to include('evaluation_failed') + expect(errors).not_to include('flag_missing') + expect(errors.include?('errors_while_computing_flags')).to eq(error_bit == true) + expect(client.get_feature_flag_result('off', 'user').enabled?).to be(false) + healthy_error = client.dequeue_last_message[:properties]['$feature_flag_error'].to_s + expect(healthy_error).not_to include('evaluation_failed') + combined = client.get_all_flags_and_payloads('user') + expect(combined[:featureFlags]).to eq('off' => false) + expect(combined[:featureFlagPayloads]).not_to have_key('checkout') + end + + it 'honors snapshot defaults, omits failed enrichment, and retains diagnostics' do + snapshot = client.evaluate_flags('user') + expect(snapshot.enabled?('checkout', default_value: true)).to be(true) + expect(snapshot.get_flag('checkout')).to be_nil + expect(snapshot.get_flag_payload('checkout')).to be_nil + event = client.dequeue_last_message + expect(event[:properties]['$feature_flag_response']).to be_nil + errors = event[:properties]['$feature_flag_error'].split(',') + expect(errors).to include('evaluation_failed') + expect(errors).not_to include('flag_missing') + expect(errors.include?('errors_while_computing_flags')).to eq(error_bit == true) + expect(snapshot.keys).to eq(['off']) + expect(snapshot.enabled?('off', default_value: true)).to be(false) + client.capture(distinct_id: 'user', event: 'enriched', flags: snapshot) + expect(client.dequeue_last_message[:properties]).to include('$feature/off' => false) + expect(snapshot._get_event_properties).not_to have_key('$feature/checkout') + end + + it 'reports evaluation_failed after inconclusive local evaluation' do + definition = local_flag('checkout', true).merge(ensure_experience_continuity: true) + stub_request(:get, definitions_endpoint).to_return(status: 200, body: { flags: [definition] }.to_json) + local_client = Client.new(api_key: API_KEY, personal_api_key: API_KEY, test_mode: true) + expect(local_client.get_feature_flag_result('checkout', 'user')).to be_nil + props = local_client.dequeue_last_message[:properties] + expect(props['$feature_flag_error']).to include('evaluation_failed') + expect(props['$feature_flag_error']).not_to include('flag_missing') + expect(props['$feature_flag_response']).to be_nil + ensure + local_client&.shutdown + end + end + end + + it "preserves local true/false/variant pairs and capture enrichment with global errors #{error_bit.inspect}" do + values = { 'on' => true, 'off' => false, 'variant' => 'blue', 'success' => true } + definitions = values.map { |key, value| local_flag(key, value) } + definitions << local_flag('checkout', true).merge(ensure_experience_continuity: true) + stub_request(:get, definitions_endpoint).to_return(status: 200, body: { flags: definitions }.to_json) + remote = { flags: (values.keys + ['checkout']).to_h { |key| [key, failed_record(key, 'placeholder')] } } + remote[:flags]['success'] = { + key: 'success', enabled: true, variant: 'remote', metadata: { payload: '{"copy":"success"}' } + } + remote[:errorsWhileComputingFlags] = error_bit unless error_bit.nil? + stub_request(:post, flags_endpoint).to_return(status: 200, body: remote.to_json) + local_client = Client.new(api_key: API_KEY, personal_api_key: API_KEY, test_mode: true) + result = local_client.get_all_flags_and_payloads('user') + expect(result[:featureFlags]).to eq(values.merge('success' => 'remote')) + expected_payloads = values.to_h do |key, _| + [key, key == 'success' ? '{"copy":"success"}' : '{"copy":"local"}'] + end + expect(result[:featureFlagPayloads]).to eq(expected_payloads) + local_client.capture(distinct_id: 'user', event: 'enriched', send_feature_flags: true) + props = local_client.dequeue_last_message[:properties] + expect(props).to include('$feature/on' => true, '$feature/off' => false, '$feature/variant' => 'blue') + expect(props).not_to have_key('$feature/checkout') + expect(local_client.get_feature_flag_result('on', 'user').enabled?).to be(true) + local_error = local_client.dequeue_last_message[:properties]['$feature_flag_error'].to_s + expect(local_error).not_to include('evaluation_failed') + ensure + local_client&.shutdown + end + end + end +end diff --git a/spec/posthog/feature_flag_error_spec.rb b/spec/posthog/feature_flag_error_spec.rb index 888a8f40..fed95a84 100644 --- a/spec/posthog/feature_flag_error_spec.rb +++ b/spec/posthog/feature_flag_error_spec.rb @@ -302,12 +302,11 @@ module PostHog stub_request(:post, flags_endpoint) .to_return(status: 200, body: flags_response.to_json) - # The failed flag has enabled=true, but should be filtered out and treated as missing. - # Without filtering, this would return true. + # The failed flag has no evaluated value, despite its enabled placeholder. result = client.get_feature_flag('my-flag', 'test-user') - expect(result).to eq(false) + expect(result).to be_nil captured_message = client.dequeue_last_message - expect(captured_message[:properties]['$feature_flag_error']).to include(FeatureFlagError::FLAG_MISSING) + expect(captured_message[:properties]['$feature_flag_error']).to include('evaluation_failed') # The non-failed flag should return its value normally good_result = client.get_feature_flag('good-flag', 'test-user') @@ -335,9 +334,9 @@ module PostHog # Without filtering, this would return 'test-variant'. result = client.get_feature_flag('variant-flag', 'test-user') - expect(result).to eq(false) + expect(result).to be_nil captured_message = client.dequeue_last_message - expect(captured_message[:properties]['$feature_flag_error']).to include(FeatureFlagError::FLAG_MISSING) + expect(captured_message[:properties]['$feature_flag_error']).to include('evaluation_failed') end it 'excludes failed flags from get_all_flags results' do diff --git a/spec/posthog/feature_flag_evaluations_spec.rb b/spec/posthog/feature_flag_evaluations_spec.rb index 8816c23e..f7286ae7 100644 --- a/spec/posthog/feature_flag_evaluations_spec.rb +++ b/spec/posthog/feature_flag_evaluations_spec.rb @@ -156,13 +156,13 @@ def capture_stderr expect(snapshot.enabled?('boolean-flag')).to be(false) end - it 'enabled? keeps a flag marked failed by /flags as false, ignoring the caller default' do + it 'enabled? honors the caller default for a flag marked failed by /flags' do stub_flags( flags: { 'failed-flag' => { key: 'failed-flag', enabled: false, variant: nil, failed: true } }, errorsWhileComputingFlags: true ) snapshot = client.evaluate_flags('user-1') - expect(snapshot.enabled?('failed-flag', default_value: true)).to be(false) + expect(snapshot.enabled?('failed-flag', default_value: true)).to be(true) end it 'enabled? still reports the evaluated response, not the default, on $feature_flag_called' do @@ -893,7 +893,7 @@ def flag_called_events(client) end end - it 'keeps a failed remote flag without a local definition as false' do + it 'omits a failed remote flag without a local definition' do c = local_client(beta_ui) stub_flags( flags: { 'remote-only' => { key: 'remote-only', enabled: false, variant: nil, failed: true } }, @@ -902,12 +902,12 @@ def flag_called_events(client) snapshot = c.evaluate_flags('user-123') - expect(snapshot.keys).to include('remote-only') - expect(snapshot.get_flag('remote-only')).to be(false) + expect(snapshot.keys).not_to include('remote-only') + expect(snapshot.get_flag('remote-only')).to be_nil expect(snapshot.unresolved_flags).to eq({}) end - it 'keeps a failed remote flag that local evaluation left unresolved as false' do + it 'omits a failed remote flag that local evaluation left unresolved' do c = local_client(checkout_with_continuity) stub_flags( flags: { 'checkout' => { key: 'checkout', enabled: false, variant: nil, failed: true } }, @@ -916,13 +916,13 @@ def flag_called_events(client) snapshot = c.evaluate_flags('user-123') - expect(snapshot.keys).to include('checkout') - expect(snapshot.get_flag('checkout')).to be(false) - expect(snapshot.enabled?('checkout', default_value: true)).to be(false) + expect(snapshot.keys).not_to include('checkout') + expect(snapshot.get_flag('checkout')).to be_nil + expect(snapshot.enabled?('checkout', default_value: true)).to be(true) expect(snapshot.unresolved_flags).to eq({}) c.capture(distinct_id: 'user-123', event: 'test-event', flags: snapshot) event = drain_messages(c).find { |m| m[:event] == 'test-event' } - expect(event[:properties]['$feature/checkout']).to be(false) + expect(event[:properties]).not_to have_key('$feature/checkout') end it 'reports a key without a local definition as missing, not unresolved' do From 9e642519cf69529338a8c6a39cf5b399e8a93d85 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Thu, 12 Nov 2026 14:14:48 +0100 Subject: [PATCH 2/2] fix(flags): retain unresolved reasons after failed fallback --- lib/posthog/client.rb | 2 +- spec/posthog/feature_flag_evaluations_spec.rb | 16 +++++++++------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/posthog/client.rb b/lib/posthog/client.rb index 545470da..091e5f74 100644 --- a/lib/posthog/client.rb +++ b/lib/posthog/client.rb @@ -816,7 +816,7 @@ def evaluate_flags( errors_while_computing: errors_while_computing, quota_limited: quota_limited, minimal_flag_called_events: minimal_flag_called_events, - unresolved_flags: unresolved_flags.reject { |key, _reason| records.key?(key) } + unresolved_flags: unresolved_flags.reject { |key, _reason| records.key?(key) && !records[key].enabled.nil? } ) end diff --git a/spec/posthog/feature_flag_evaluations_spec.rb b/spec/posthog/feature_flag_evaluations_spec.rb index f7286ae7..d9895e6d 100644 --- a/spec/posthog/feature_flag_evaluations_spec.rb +++ b/spec/posthog/feature_flag_evaluations_spec.rb @@ -863,14 +863,16 @@ def flag_called_events(client) expect(snapshot.unresolved_flags).to eq('beta-ui' => UnresolvedFlagReason::UNSUPPORTED_DEFINITION) end - it 'does not report a flag the remote fallback resolved' do - c = local_client(checkout_with_continuity) - stub_flags(flags: { 'checkout' => { key: 'checkout', enabled: true, variant: nil } }) + [true, false].each do |enabled| + it "does not report a flag the remote fallback resolved to #{enabled}" do + c = local_client(checkout_with_continuity) + stub_flags(flags: { 'checkout' => { key: 'checkout', enabled: enabled, variant: nil } }) - snapshot = c.evaluate_flags('user-123') + snapshot = c.evaluate_flags('user-123') - expect(snapshot.get_flag('checkout')).to be(true) - expect(snapshot.unresolved_flags).to eq({}) + expect(snapshot.get_flag('checkout')).to be(enabled) + expect(snapshot.unresolved_flags).to eq({}) + end end { @@ -919,7 +921,7 @@ def flag_called_events(client) expect(snapshot.keys).not_to include('checkout') expect(snapshot.get_flag('checkout')).to be_nil expect(snapshot.enabled?('checkout', default_value: true)).to be(true) - expect(snapshot.unresolved_flags).to eq({}) + expect(snapshot.unresolved_flags).to eq('checkout' => UnresolvedFlagReason::EXPERIENCE_CONTINUITY) c.capture(distinct_id: 'user-123', event: 'test-event', flags: snapshot) event = drain_messages(c).find { |m| m[:event] == 'test-event' } expect(event[:properties]).not_to have_key('$feature/checkout')