Repository navigation
fix: mint the instance id on every client, not only under CLI SAPI (centinel coverage hunt) - #278
Closed
JackDanger wants to merge 1 commit into
Closed
JackDanger wants to merge 1 commit into
JackDanger wants to merge 1 commit into
Conversation
…entinel coverage hunt) The instance id came from apcu when available, otherwise only under php_sapi_name() === 'cli'. The SDK's own documented test path is PHP's built-in web server (make start-contract-test-service runs 'php -S'), whose SAPI name is 'cli-server': every client created there sent requests without X-LaunchDarkly-Instance-Id, and the official instance-id rows failed with 'Should NOT be empty' (poll requests + event posts). Mint the UUID whenever no instance_id option is set, for every runtime; explicit instance_id options keep their precedence. Co-Authored-By: Kimi (LD Research via termag) Generated-by: termag
keelerm84
reviewed
Oct 30, 2026
| $uuid = Uuid::uuid4()->toString(); | ||
| return $uuid; | ||
| }); | ||
| } else { |
Member
There was a problem hiding this comment.
This change will explode the cardinality of that data, which has effects on relay behavior around usage metrics and o11y.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Coverage-hunt findings from the centinel verifier + the official sdk-test-harness (v2.41.1, the version
make contract-testsprescribes) ride against php-server-sdk 6.11.0 at main.Repro one-liner: official harness v2.41.1 with the SDK's own documented test path (
make start-contract-test-service, i.e.php -S, SAPIcli-server):Root cause:
LDClient.__constructsetoptions['instance_id']from apcu when available, otherwise only underphp_sapi_name() === 'cli'. PHP's built-in web server reports SAPIcli-server, so under the SDK's own test path every client sent requests with noX-LaunchDarkly-Instance-Idheader (the other server SDKs always send it).Fix (this PR): mint the instance id (apcu-shared where available, else a fresh UUID per client) whenever no explicit
instance_idoption is set — every SAPI. Explicit options keep their precedence.Post-fix evidence: same official harness v2 ride on the patched tree:
instance id/poll requests+instance id/event postsran and passed (junit receiptphp-fix-inst-h2in the centinel verifier run-dirs; 10 ids, 0 failures).These reviewers were automatically added by ld-research.
via LD Research 🤖