5/5 🐛 Fix five defects the test suite uncovered - #22
Merged
Merged
Conversation
apache/thrift < 0.24.0 is affected by GHSA-8wv5-x4w7-5gww / CVE-2026-43871 — an infinite loop in the PHP, Python, Go and Java bindings, severity high — so the whole previous '>=0.11, <0.17' range is unusable and 0.24.0 is the only release that is not. 0.24.0 itself requires PHP ^8.1; this major goes to ^8.4 so the toolchain can land on current stable releases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three fixes, all enabled by the new baseline: * socket_create() returns a Socket object since PHP 8.0, so the is_resource($this->socket) guard in connect() was always false and a fresh socket was created on every flush(). Descriptors did not leak — the old socket was collected immediately — but each flush paid for a needless socket_create() plus socket_connect(), and close() was a no-op in practice. * Match the parameter types of the now strictly typed TTransport in 0.24: read(int $len): string and write(string $buf): void. * read() returned '' unconditionally, so the inherited readAll() spun forever in while (strlen($data) < $len). Unreachable for the emit-only agent client, but it is the same infinite-loop shape as CVE-2026-43871. It throws TTransportException now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tree was generated by compiler 0.15 while the runtime is now 0.24. Regenerating against jaeger-idl v0.12.0 aligns the two: * declare(strict_types=1) in every file (THRIFT-5986) * native property and constructor types (THRIFT-5991), so 'public $key' becomes 'public ?string $key' — which is what the IDL actually says * native return types on struct methods (THRIFT-5990) * PSR-12 layout (THRIFT-5959) bin/thrift-gen.sh cloned jaeger-idl from a moving 'main', so two runs a month apart could differ with no record of what changed. It now pins jaeger-idl v0.12.0, refuses to run unless the compiler is 0.24.x, generates into a mktemp directory cleaned up by a trap instead of leaving a clone in the working tree, and drops a dead 'rm -rf ../src/Jaeger/Thrift/' from an older layout. Running it twice is now a no-op. BREAKING: 24 classes are gone — AggregationValidator, BaggageRestrictionManager, Dependency and ThrottlingService with their DTOs. They came from IDL definitions Jaeger removed upstream long ago (already absent in jaeger-idl v0.11.4); they are server-side APIs this client never used. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
php-cs-fixer and rector, both configured to skip src/Thrift: that tree is generated by bin/thrift-gen.sh and has to stay byte-for-byte reproducible from the IDL, so reformatting it would be undone by the next regeneration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Break multi-argument constructors across lines and normalise the spacing between promoted parameter types and their names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Level 1 — the strictest — over src/, with src/Thrift excluded: its shape is the thrift compiler's contract and it calls thrift_protocol_* from the optional C extension. ClassMustBeFinal is suppressed because this is a library and consumers legitimately extend these classes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolve every issue psalm reports at level 1: * type the arrays that cross a public boundary (tags, logs, spans, baggage) so coercions are checked instead of inferred as mixed * handle the false branch of gethostname/getmypid/getmyuid/getmygid/json_encode instead of passing it straight into a string parameter * cast mixed/int and float operands explicitly * give the ArrayAccess and IteratorAggregate implementations their template parameters and real native signatures, dropping #[ReturnTypeWillChange] * align parameter names with the interfaces that declare them, so named arguments work One suppression remains, in Span\Batch\SpanBatch: SpanInterface cannot declare that it extends the generated Thrift\Span even though its only implementation does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds phpunit/phpunit ^13, a phpunit.xml.dist that keeps the generated src/Thrift tree out of coverage, and a Jaeger\Tests\ autoload-dev namespace. composer.json also loses "minimum-stability": "dev" — every dependency now resolves to a stable release — and gains the description, type and keywords that 'composer validate --strict' requires. ext-apcu is declared as a suggestion since only RateLimitingSampler needs it. The three linter configs are widened to cover tests/ as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
317 tests, 96.86% line coverage over src/ excluding the generated Thrift code. Table-driven throughout: each case yields a class name plus constructor arguments rather than a ready-made object, because code executed inside a data provider is not attributed to coverage. No mocks or stubs — tests/Fixture holds real implementations: a recording tracer, client and agent, a deterministic id generator and a bound UDP listener. tests/Thrift/SerializationTest.php exercises the generated structs against the apache/thrift runtime over both the binary and compact protocols and emits a real batch over UDP. Since the generated fields are honestly nullable, it narrows them explicitly — which makes the tests stricter, as a round trip must return every 'required' field non-null. Four existing defects are pinned by tests that document them rather than quietly encoding them as intended behaviour: * PeerAddressTag writes the key 'peer.adress' (typo) and PeerIpv4Tag writes 'peer.ip' where OpenTracing specifies 'peer.ipv4' * AdaptiveSampler copies flags and tags from the rate limiter that rejected the trace when the probabilistic sampler is the one that accepted it * StackSpanManager::remove() never removes anything: SplStack::valid() is false until rewind(), and it compares a Span's object hash against a SpanContext's * TextCodec::encode() writes the trace id as '%x%x' without padding, so a traceIdLow shorter than 16 hex digits cannot be split back out on decode Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three jobs: PHPUnit on PHP 8.4 and 8.5, psalm, and a coding-standards job that checks php-cs-fixer and rector in dry-run mode plus 'composer validate --strict'. APCu is enabled on the CLI so the RateLimitingSampler tests run instead of skipping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PeerAddressTag wrote 'peer.adress' — a typo — and PeerIpv4Tag wrote 'peer.ip' where the OpenTracing semantic conventions specify 'peer.ipv4'. BREAKING: spans now carry different tag keys. Jaeger queries, dashboards and alerts filtering on the old spellings must be updated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
When the rate limiter rejected a trace and the probabilistic sampler then accepted it, AdaptiveSampler still copied flags and tags from the rate limiter. The span came out marked sampled but with flags 0 and a sampler.decision of false — an agent would see a sampled span claiming it was not sampled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two independent bugs made it a no-op: * the loop was guarded by SplStack::valid(), an Iterator method that returns false until rewind() is called, so the body never ran; * it compared spl_object_hash() of a Span against spl_object_hash() of a SpanContext, which can never match. It now pops spans until it reaches the one owning the given context. Matching goes through the trace and span identifiers rather than object identity, because a span swaps its context for a copy whenever baggage changes. Tracer::remove() delegates here, so it was equally inert before this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
encode() wrote the trace id as '%x%x', so a traceIdLow shorter than 16 hex digits ran into the high half with no boundary: SpanContext(0xaa, 0xbb) encoded to 'aabb' and decoded back to a trace id of 0xaabb with a high half of 0. A 128-bit id now pads its low half to a full 16 digits. A 64-bit id is written with no leading zero, so '0' + '1a' becomes '1a' — which is what other Jaeger clients emit. BREAKING: the encoded trace context changes for 64-bit ids (no leading zero) and for 128-bit ids whose low half is short. decode() reads both spellings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
next() returned from a 'finally' block, which discards whatever 'try' produced. Every id cost two random_int() draws — the first computed and thrown away — and the 'catch (Exception)' was unreachable, since 'finally' runs and returns before any exception can propagate. Had the CSPRNG ever failed, the old code would have called it a second time from 'finally' and thrown from there anyway, so the catch bought nothing. A failure now surfaces directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Last of five stacked PRs for the 4.0 major. Stacked on #21.
Each fix follows a full TDD cycle: the pinned test from #21 is first flipped to the expected correct behaviour and observed failing, then the code is changed.
1. Tag keys
peer.adress→peer.address,peer.ip→peer.ipv4(OpenTracing semantic conventions).2. AdaptiveSampler
When the rate limiter rejected a trace and the probabilistic sampler then accepted it, the result still copied flags and tags from the rate limiter. The span came out marked sampled but with
flags = 0andsampler.decision = false— an agent would see a sampled span claiming it was not sampled.3. StackSpanManager::remove()
A no-op from two independent bugs: the loop was guarded by
SplStack::valid(), an Iterator method that returnsfalseuntilrewind(), so the body never ran; and it comparedspl_object_hash()of aSpanagainst that of aSpanContext, which can never match.Tracer::remove()delegates here, so it was equally inert.Matching now goes through trace and span identifiers rather than object identity, because a span swaps its context for a copy whenever baggage changes.
4. TextCodec::encode()
Wrote the trace id as
%x%x, soSpanContext(0xaa, 0xbb)encoded toaabband decoded back to a trace id of0xaabbwith a high half of 0. A 128-bit id now pads its low half to a full 16 digits; a 64-bit id is written with no leading zero, matching other Jaeger clients.decode()reads both spellings.5. RandomIntGenerator::next()
Returned from a
finallyblock, which discards whatevertryproduced — every id cost tworandom_int()draws, and thecatchwas unreachable.No TDD cycle here: the defect has no externally observable consequence, so there is no failing assert to write. Coverage is the evidence — the class was 66.7% because of the unreachable line, now 100%.
Result
326 tests, 97.70% coverage (up from 96.86%);
StackSpanManagerreaches 100%.