Skip to content

fix: normalize non-finite numbers in data-only event payloads - #276

Open
marandaneto wants to merge 16 commits into
mainfrom
fix/non-finite-event-properties-20261022
Open

marandaneto wants to merge 16 commits into
mainfrom
fix/non-finite-event-properties-20261022

Conversation

@marandaneto

@marandaneto marandaneto commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

💡 Motivation and Context

NaN and positive/negative infinity can make JSON encoding discard an event or an entire batch. Following the design checkpoint and explicit maintainer approval, this revision narrows the original proposal to bounded, data-only payloads instead of reproducing PHP object serialization.

  • Repair non-finite numbers to JSON null only when the whole payload contains scalar values/arrays, no objects/resources/retained array-element references, valid strings, at most 10,000 values, 64 array levels, and 1 MiB of raw string/key bytes. Counts include the complete envelope and SDK metadata.
  • Otherwise pass the original whole payload unchanged to native JSON. Exhausting an inspection budget is not a new rejection: finite payloads still encode natively. Objects, enums, serializer results and reference-cell updates are not normalized.
  • This deliberately does not isolate events in a mixed batch. An opaque value anywhere can leave a non-finite neighbor subject to native encoding failure. Ineligible File events retain existing failure handling, including empty-line output. The encoded batch-size guard is unchanged and separate from the repair budget.
  • Remove the custom sequential writer, object-handler emulation, property-layout reconstruction, active-reference tracking and synthetic JSON error restoration. Eligibility is read-only; copying starts only after full qualification. Native JSON remains authoritative for ordering, redaction, callback counts, recursion, exceptions and errors. There are no serializer retries, partial-output flags or token replacement.

The customer-facing encoding contract documents these limits and native fallback. The design checkpoint retains the complete review-reproduction inventory, rejected approaches and explicitly approved expectation changes. Existing SDK methods/options are unchanged; capture callers can use the existing before_send hook for their own JSON-safe conversion policy. The API snapshot records the internal static encoder, and a patch change intent is included. The capture spec has no NaN/infinity policy. Related Python work: PostHog/posthog-python#1081.

💚 How did you test it?

  • Retained every concrete reported reproduction. The five newest cases failed on 577a8b7 and now pass without changing their expectations: abstract-property redaction order, stateless/stateful active-array serializer re-entry, hooked DateInterval output/caller state, and a 12 MiB string under 32 MiB before normal batch-size rejection.
  • The new data-only policy had six expected failures against the previous writer before replacement. Boundary tests now cover exact value/byte/depth limits, successful finite fallback beyond each bound, opaque/reference whole-payload fallback, caller immutability and no serializer retry.
  • Original object/non-finite examples remain as native-failure cases under the approved narrowed contract. Added finite companions retain successful wire-format/privacy coverage rather than dropping the examples. Added supported data-only NaN/infinity integration cases for both batch and File consumers, plus 32 MiB dense-array and escaped-string repair probes against independent native-null fixtures.
  • Follow-up retained-capacity repros were red on d16a8b1: a million-slot pruned root, 16 repeated pruned children, and a sparse high key each exhausted 32 MiB while native-error/native-null baselines completed normally. Key-preserving compaction now passes all three under the same limit; a numeric-string symbol-table regression also protects object-vs-list JSON shape. Unchanged prefixes, sparse keys and caller values remain covered.
  • 164 focused tests / 1,216 assertions and the complete 1,594-test / 8,345-assertion suite with Xdebug 3.5.3 coverage pass locally on PHP 8.5.10. Existing notices remain 27 deprecations and 42 PHPUnit deprecations. All 16 adapter tests pass.
  • CI runs PHPUnit on PHP 8.2, 8.3, 8.4 and 8.5. Changed-file PHP_CodeSniffer, public API snapshot checks, strict Composer validation and whitespace checks pass. Tests use offline fakes/loopback servers, not live PostHog endpoints.

Compliance assertions remain advisory: lib_curl 46/47, socket 45/47 and fork_curl 36/47 on d16a8b1, matching the previous 577a8b7 reports exactly. Inventory-completeness checks pass; unrelated transport gaps are not changed here.

Design invariants were reviewed separately from test counts: no copying before full eligibility; no executable user values/reference cells in a repairable graph; original graphs preserved for native fallback; primitive string validation does not overwrite callback-visible JSON error state. The initial assumption that live entry counts bound copy-on-write capacity was disproved; normalization now compacts live entries before mapping values and reuses unchanged subtrees without writing into caller-seeded tables. Earlier autoreview covered an older revision, not this replacement; human review remains required.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran pnpm change to generate a change intent file

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Human DRI: @marandaneto. Implemented with Pi using bash and file tools. Earlier graph/writer approaches were rejected after compatibility and ownership regressions; the maintainer approved conservative whole-payload native delegation with narrowly bounded data-only repair. No new hook or SDK option was introduced. No public session link is available. Human review is required.

@marandaneto marandaneto self-assigned this Oct 22, 2026

@posthog posthog Bot left a comment

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.

PostHog Review · Standard · reviewed a3fd115: 1 Must fix, 1 Should fix.

This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.

Comment thread lib/JsonEncoder.php Outdated
Comment thread lib/JsonEncoder.php Outdated
@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

posthog-php-socket Compliance Report

Date: 2026-10-23T12:26:42.074177+00:00
Duration: 114616ms

⚠️ Some Tests Failed

45/47 tests passed, 2 failed


Capture Tests

⚠️ 28/30 tests passed, 2 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 25ms
Format Validation.Event Has Uuid ✅ 519ms
Format Validation.Event Has Lib Properties ✅ 522ms
Format Validation.Distinct Id Is String ✅ 520ms
Format Validation.Token Is Present ✅ 521ms
Format Validation.Custom Properties Preserved ✅ 520ms
Format Validation.Event Has Timestamp ✅ 520ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 521ms
Retry Behavior.Retries On 503 ✅ 5828ms
Retry Behavior.Does Not Retry On 400 ✅ 2524ms
Retry Behavior.Does Not Retry On 401 ✅ 2523ms
Retry Behavior.Respects Retry After Header ❌ 5628ms
Retry Behavior.Implements Backoff ✅ 16234ms
Retry Behavior.Retries On 500 ✅ 5137ms
Retry Behavior.Retries On 502 ✅ 5629ms
Retry Behavior.Retries On 504 ✅ 5630ms
Retry Behavior.Max Retries Respected ✅ 16243ms
Deduplication.Generates Unique Uuids ✅ 526ms
Deduplication.Preserves Uuid On Retry ✅ 5629ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 10837ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 5630ms
Deduplication.No Duplicate Events In Batch ✅ 526ms
Deduplication.Different Events Have Different Uuids ✅ 523ms
Compression.Sends Gzip When Enabled ✅ 520ms
Batch Format.Uses Proper Batch Structure ✅ 521ms
Batch Format.Flush With No Events Sends Nothing ✅ 516ms
Batch Format.Multiple Events Batched Together ✅ 511ms
Error Handling.Does Not Retry On 403 ✅ 2522ms
Error Handling.Does Not Retry On 413 ✅ 2524ms
Error Handling.Retries On 408 ❌ 5527ms

Failures

retry_behavior.respects_retry_after_header

Retry delay too short: 102ms < 2500ms

error_handling.retries_on_408

Expected at least 2 requests, got 1

Feature_Flags Tests

✅ 17/17 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id ✅ 521ms
Request Payload.Flags Request Uses V2 Query Param ✅ 519ms
Request Payload.Flags Request Hits Flags Path Not Decide ✅ 519ms
Request Payload.Flags Request Omits Authorization Header ✅ 518ms
Request Payload.Token In Flags Body Matches Init ✅ 520ms
Request Payload.Groups Round Trip ✅ 519ms
Request Payload.Groups Default To Empty Object ✅ 520ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False ✅ 519ms
Request Payload.Disable Geoip Omitted Defaults To False ✅ 519ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key ✅ 519ms
Request Lifecycle.No Flags Request On Init Alone ✅ 515ms
Request Lifecycle.No Flags Request On Normal Capture ✅ 507ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests ✅ 523ms
Request Lifecycle.Mock Response Value Is Returned To Caller ✅ 535ms
Retry Behavior.Retries Flags On 502 ✅ 622ms
Retry Behavior.Retries Flags On 504 ✅ 622ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event ✅ 521ms

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

posthog-php-fork_curl Compliance Report

Date: 2026-10-23T12:26:44.990216+00:00
Duration: 112398ms

⚠️ Some Tests Failed

36/47 tests passed, 11 failed


Capture Tests

⚠️ 19/30 tests passed, 11 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 35ms
Format Validation.Event Has Uuid ✅ 528ms
Format Validation.Event Has Lib Properties ✅ 530ms
Format Validation.Distinct Id Is String ✅ 530ms
Format Validation.Token Is Present ✅ 529ms
Format Validation.Custom Properties Preserved ✅ 531ms
Format Validation.Event Has Timestamp ✅ 530ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 530ms
Retry Behavior.Retries On 503 ❌ 5534ms
Retry Behavior.Does Not Retry On 400 ✅ 2532ms
Retry Behavior.Does Not Retry On 401 ✅ 2534ms
Retry Behavior.Respects Retry After Header ❌ 5531ms
Retry Behavior.Implements Backoff ❌ 15542ms
Retry Behavior.Retries On 500 ❌ 5045ms
Retry Behavior.Retries On 502 ❌ 5538ms
Retry Behavior.Retries On 504 ❌ 5531ms
Retry Behavior.Max Retries Respected ❌ 15551ms
Deduplication.Generates Unique Uuids ✅ 538ms
Deduplication.Preserves Uuid On Retry ❌ 5536ms
Deduplication.Preserves Uuid And Timestamp On Retry ❌ 10541ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ❌ 5540ms
Deduplication.No Duplicate Events In Batch ✅ 536ms
Deduplication.Different Events Have Different Uuids ✅ 531ms
Compression.Sends Gzip When Enabled ✅ 533ms
Batch Format.Uses Proper Batch Structure ✅ 530ms
Batch Format.Flush With No Events Sends Nothing ✅ 518ms
Batch Format.Multiple Events Batched Together ✅ 520ms
Error Handling.Does Not Retry On 403 ✅ 2532ms
Error Handling.Does Not Retry On 413 ✅ 2533ms
Error Handling.Retries On 408 ❌ 5533ms

Failures

retry_behavior.retries_on_503

Expected at least 3 requests, got 1

retry_behavior.respects_retry_after_header

Expected at least 2 requests, got 1

retry_behavior.implements_backoff

Expected at least 3 requests, got 1

retry_behavior.retries_on_500

Expected at least 2 requests, got 1

retry_behavior.retries_on_502

Expected at least 2 requests, got 1

retry_behavior.retries_on_504

Expected at least 2 requests, got 1

retry_behavior.max_retries_respected

Expected 4 requests, got 1

deduplication.preserves_uuid_on_retry

Need at least 2 requests to check retry

deduplication.preserves_uuid_and_timestamp_on_retry

Expected at least 3 requests, got 1

deduplication.preserves_uuid_and_timestamp_on_batch_retry

Expected at least 2 requests, got 1

error_handling.retries_on_408

Expected at least 2 requests, got 1

Feature_Flags Tests

✅ 17/17 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id ✅ 524ms
Request Payload.Flags Request Uses V2 Query Param ✅ 521ms
Request Payload.Flags Request Hits Flags Path Not Decide ✅ 522ms
Request Payload.Flags Request Omits Authorization Header ✅ 520ms
Request Payload.Token In Flags Body Matches Init ✅ 522ms
Request Payload.Groups Round Trip ✅ 522ms
Request Payload.Groups Default To Empty Object ✅ 522ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False ✅ 521ms
Request Payload.Disable Geoip Omitted Defaults To False ✅ 522ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key ✅ 520ms
Request Lifecycle.No Flags Request On Init Alone ✅ 517ms
Request Lifecycle.No Flags Request On Normal Capture ✅ 516ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests ✅ 525ms
Request Lifecycle.Mock Response Value Is Returned To Caller ✅ 523ms
Retry Behavior.Retries Flags On 502 ✅ 624ms
Retry Behavior.Retries Flags On 504 ✅ 623ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event ✅ 532ms

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

posthog-php-lib_curl Compliance Report

Date: 2026-10-23T12:26:34.093824+00:00
Duration: 116766ms

⚠️ Some Tests Failed

46/47 tests passed, 1 failed


Capture Tests

⚠️ 29/30 tests passed, 1 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 32ms
Format Validation.Event Has Uuid ✅ 524ms
Format Validation.Event Has Lib Properties ✅ 527ms
Format Validation.Distinct Id Is String ✅ 528ms
Format Validation.Token Is Present ✅ 525ms
Format Validation.Custom Properties Preserved ✅ 526ms
Format Validation.Event Has Timestamp ✅ 526ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 526ms
Retry Behavior.Retries On 503 ✅ 5836ms
Retry Behavior.Does Not Retry On 400 ✅ 2529ms
Retry Behavior.Does Not Retry On 401 ✅ 2528ms
Retry Behavior.Respects Retry After Header ❌ 6335ms
Retry Behavior.Implements Backoff ✅ 16251ms
Retry Behavior.Retries On 500 ✅ 5634ms
Retry Behavior.Retries On 502 ✅ 5634ms
Retry Behavior.Retries On 504 ✅ 5636ms
Retry Behavior.Max Retries Respected ✅ 17051ms
Deduplication.Generates Unique Uuids ✅ 234ms
Deduplication.Preserves Uuid On Retry ✅ 5633ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 10843ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 5640ms
Deduplication.No Duplicate Events In Batch ✅ 531ms
Deduplication.Different Events Have Different Uuids ✅ 527ms
Compression.Sends Gzip When Enabled ✅ 527ms
Batch Format.Uses Proper Batch Structure ✅ 525ms
Batch Format.Flush With No Events Sends Nothing ✅ 521ms
Batch Format.Multiple Events Batched Together ✅ 515ms
Error Handling.Does Not Retry On 403 ✅ 2528ms
Error Handling.Does Not Retry On 413 ✅ 2528ms
Error Handling.Retries On 408 ✅ 5634ms

Failures

retry_behavior.respects_retry_after_header

Retry delay too short: 804ms < 2500ms

Feature_Flags Tests

✅ 17/17 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id ✅ 525ms
Request Payload.Flags Request Uses V2 Query Param ✅ 521ms
Request Payload.Flags Request Hits Flags Path Not Decide ✅ 524ms
Request Payload.Flags Request Omits Authorization Header ✅ 522ms
Request Payload.Token In Flags Body Matches Init ✅ 524ms
Request Payload.Groups Round Trip ✅ 522ms
Request Payload.Groups Default To Empty Object ✅ 524ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False ✅ 522ms
Request Payload.Disable Geoip Omitted Defaults To False ✅ 523ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key ✅ 523ms
Request Lifecycle.No Flags Request On Init Alone ✅ 516ms
Request Lifecycle.No Flags Request On Normal Capture ✅ 511ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests ✅ 528ms
Request Lifecycle.Mock Response Value Is Returned To Caller ✅ 524ms
Retry Behavior.Retries Flags On 502 ✅ 626ms
Retry Behavior.Retries Flags On 504 ✅ 626ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event ✅ 526ms

@marandaneto
marandaneto marked this pull request as ready for review October 22, 2026 16:27
@marandaneto
marandaneto requested a review from a team as a code owner October 22, 2026 16:27
Comment thread lib/JsonEncoder.php Outdated
@veria-ai

veria-ai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 3 · PR risk: 0/10

@posthog posthog Bot left a comment

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.

PostHog Review · Standard · reviewed 2155f5a: 2 Should fix.

Comment thread lib/JsonEncoder.php Outdated
Comment thread lib/JsonEncoder.php Outdated

@posthog posthog Bot left a comment

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.

PostHog Review · Standard · reviewed e414589: 1 Must fix, 1 Should fix.

This is review round 3 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.

Comment thread lib/JsonEncoder.php Outdated
Comment thread lib/JsonEncoder.php Outdated

@posthog posthog Bot left a comment

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.

PostHog Review · Standard · reviewed 7e232ad: 1 Must fix.

This is review round 4 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.

Comment thread lib/JsonEncoder.php Outdated
Comment thread lib/JsonEncoder.php Outdated

@posthog posthog Bot left a comment

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.

PostHog Review · Standard · reviewed c5d55ce: 2 Should fix.

This is review round 5 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.

This Standard review started from the posthog-review label. Comment @posthog review --deep for a Deep review, or @posthog help to see all commands.

Comment thread lib/JsonEncoder.php Outdated
Comment thread lib/JsonEncoder.php Outdated
@posthog posthog Bot removed the reviewhog label Oct 23, 2026

@posthog posthog Bot left a comment

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.

PostHog Review · Standard · reviewed 5a4a540: 1 Must fix, 2 Should fix.

This is review round 6 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.

Comment thread lib/JsonEncoder.php Outdated
Comment thread lib/JsonEncoder.php Outdated
Comment thread lib/JsonEncoder.php Outdated

@posthog posthog Bot left a comment

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.

PostHog Review · Standard · reviewed 577a8b7: 1 Must fix, 3 Should fix.

This is review round 7 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.

Comment thread lib/JsonEncoder.php Outdated
Comment thread lib/JsonEncoder.php Outdated
Comment thread lib/JsonEncoder.php Outdated
Comment thread lib/JsonEncoder.php Outdated

@posthog posthog Bot left a comment

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.

PostHog Review · Standard · reviewed d16a8b1: 1 Must fix.

This is review round 8 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.

Comment thread lib/JsonEncoder.php Outdated
@marandaneto marandaneto changed the title fix: serialize non-finite event properties as null fix: normalize non-finite numbers in data-only event payloads Oct 23, 2026
Comment thread lib/JsonEncoder.php Outdated

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant