Skip to content

fix: make layout observation lifecycle thread-safe - #814

Merged
marandaneto merged 7 commits into
mainfrom
investigate/issue-806-layout-lifecycle
Sep 11, 2026
Merged

marandaneto merged 7 commits into
mainfrom
investigate/issue-806-layout-lifecycle

Conversation

@marandaneto

@marandaneto marandaneto commented Sep 10, 2026 •

Copy link
Copy Markdown
Member

💡 Motivation and Context

Converts this reproduction-only PR into a production lifecycle fix. The reproductions are now passing regression tests; no intentionally failing tests remain.

Related to #806, but these confirmed lifecycle defects do not establish the cause of the customer's production NSISEngine crash. The diagnostic warning from merged #815 is preserved in the new hook. The run-loop proposal in #813 remains separate work. This preserves synchronous UIKit forwarding on its original calling thread; it does not suppress background layout.

Changes

  • Eagerly initialize the callback publisher instead of racing Swift's lazy initialization.
  • Serialize and coalesce subscriber-count observer delivery, reconciling the latest count without holding callback-state locks during observer execution. Concurrent and reentrant observers cannot overlap or apply an older count after the final current count.
  • Bound subscriber-count delivery to 32 observer calls per batch. Queue pending reconciliation on a private serial queue while retaining notification ownership, so sustained churn cannot keep the initiating thread in an unbounded drain. This bounds call count, not the duration of arbitrary observer code.
  • Replace exchange-based forwarding with hooks that capture immutable original IMPs. A layout dispatch that retained a hook before unsubscribe can still forward safely after uninstall, without recursive alias lookup.
  • Cache hooks per original IMP and retain their block implementations for process lifetime, since in-flight dispatches and other swizzlers may retain those function pointers. Ordinary restarts reuse the same hook.
  • Preserve an observed newer swizzler during teardown, but retire our ownership when the last subscriber leaves. On positive subscriber-count changes, check the actual current IMP and recover if another library removed or bypassed our hook. This also works when another subscriber, such as Surveys, remains active.
  • Reactivate a restored cached PostHog IMP rather than wrapping ourselves. An opaque external hook may retain our old hook, so recovery can add one cached outer wrapper. The active token prevents duplicate notifications, and repeated restarts reuse a stable chain.
  • Use dedicated test view classes so runtime regression fixtures do not interfere with the shared publisher or other suites.

An older library restoring UIKit can remove PostHog from the chain. Recovery now handles that sequence on the next subscriber-count notification rather than trusting stale installation state. It does not continuously monitor runtime replacement or coordinate arbitrary concurrent third-party writes. Forwarding implementations retained by other libraries must remain valid.

💚 How did you test it?

Reproduction-first validation confirmed stale count delivery, a hasSwizzled TSan race, multiple callback objects from concurrent first access, stack-overflowing forwarding after unsubscribe, and clobbering a newer swizzler before the fixes. A retained-old-hook regression also caught duplicate notifications during implementation.

Validation Result
Focused lifecycle suite with Thread Sanitizer Pass, three repetitions/relaunches; includes 5,000 rounds of eight concurrent retained subscriptions
Reentrant and bounded-delivery regressions Pass: 32 synchronous calls, deferred reconciliation to zero/nonzero counts, 257 calls across multiple batches, no overlapping observers
make test Pass on 6f31b314c: 799 Swift Testing tests, plus XCTest
make testOniOSSimulator Pass on 6f31b314c: 193 XCTest tests and 926 Swift Testing tests
Detached-hook recovery Pass: older uninstall and newer bypass, each with/without another retained subscriber; 100 restart cycles verify exactly-once forwarding/notifications and stable cached IMP reuse
SDK targets via make build Pass: iOS, macOS, Mac Catalyst, tvOS, watchOS, visionOS
make format, make lint, git diff --check Pass
make build SDK and platform examples passed; external-client example blocked by checkout/package identity mismatch described below

Full build stops at ExternalSDK-iOS: the example expects package identity posthog-ios, while the isolated checkout is named posthog-ios-issue-806-lifecycle. Subsequent CocoaPods examples were not reached. No unrelated build configuration was changed.

Autoreview command: $HOME/.pi/agent/skills/autoreview/scripts/autoreview --mode branch --base origin/main --prompt-file autoreview-scope.md. One test-ordering finding was rejected after checking the actual code: this test uses only zero-interval trailing subscriptions, whose invokeTrailing directly enqueues on main before the continuation barrier. It does not traverse the background leading-throttle queue. The bundle-only reviewer could not see that unchanged implementation. No accepted review blockers remain.

The bounded-delivery follow-up (5826de1b0) passed isolated autoreview with no findings: $HOME/.pi/agent/skills/autoreview/scripts/autoreview --mode commit --commit HEAD --prompt-file autoreview-bounded-scope.md. Its tests first failed against the unbounded loop, then passed along with the existing lifecycle tests under Thread Sanitizer across three repetitions/relaunches.

Latest main merge validation (a4be3e52c)

All 47 CI checks passed for this exact commit, including SDK/example builds, tests, replay masking snapshots, and compliance. The combined warning/lifecycle tests passed locally under TSan across three repetitions, and the full iOS suite passed (190 XCTest tests and 921 Swift Testing tests). Format and lint passed. Local macOS full-suite attempts had varying failures in unchanged feature-flag/event/queue timing tests; the queue test passed in isolation, and the full macOS CI job passed. No tests were weakened or disabled. The existing local external-client package-identity build limitation remains; its CI build passed.

Current head (6f31b314c)

Recovery fix 7e043c190 addresses the clarified review sequence. Both detach sequences first failed with zero notifications. The retained-subscriber variants also failed on the partial zero-only fix before runtime reconciliation was added. No intentionally failing tests remain.

Merged latest main (05bbc8d6e, including v3.73.0) without conflicts in 6f31b314c. The table above reflects this head: full SPM/iOS suites passed, combined layout and bounded/reentrant tests passed under TSan across three repetitions/relaunches, and format/lint passed. The local external-client build limitation is unchanged. CI is running for this exact head.

Final autoreview: $HOME/.pi/agent/skills/autoreview/scripts/autoreview --mode branch --base origin/main --prompt-file autoreview-final-lifecycle-scope.md. Clean, no actionable findings.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed. Internal lifecycle behavior is documented above and in code.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Added a patch changeset: .changeset/stable-layout-lifecycle.md.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Manoel requested implementing the lifecycle fixes in this existing PR worktree and pushing to this PR. Pi used reproduction-first tests, file edits, make-based validation, Thread Sanitizer, GitHub CLI, and isolated Pi autoreview. Changes remain within layout-hook lifecycle and throttled subscriber-count delivery; no run-loop architecture or public API changes are included. Human review is required.

Keep this as a reproduction-only draft. The stale-count test intentionally fails, and Thread Sanitizer detects the existing hasSwizzled race. No production fix is included.
@marandaneto marandaneto changed the title test: reproduce layout publisher lifecycle races fix: make layout observation lifecycle thread-safe Sep 10, 2026
@github-actions

github-actions Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

posthog-ios Compliance Report

Date: 2026-09-11 07:26:45 UTC
Duration: 209162ms

⚠️ Some Tests Failed

44/45 tests passed, 1 failed


Capture Tests

⚠️ 28/29 tests passed, 1 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 2832ms
Format Validation.Event Has Uuid ✅ 2795ms
Format Validation.Event Has Lib Properties ✅ 391ms
Format Validation.Distinct Id Is String ✅ 299ms
Format Validation.Token Is Present ✅ 2754ms
Format Validation.Custom Properties Preserved ✅ 2746ms
Format Validation.Event Has Timestamp ✅ 2710ms
Retry Behavior.Retries On 503 ✅ 9167ms
Retry Behavior.Does Not Retry On 400 ✅ 2284ms
Retry Behavior.Does Not Retry On 401 ✅ 4650ms
Retry Behavior.Respects Retry After Header ✅ 7778ms
Retry Behavior.Implements Backoff ✅ 15403ms
Retry Behavior.Retries On 500 ✅ 9362ms
Retry Behavior.Retries On 502 ✅ 9235ms
Retry Behavior.Retries On 504 ✅ 9179ms
Retry Behavior.Max Retries Respected ❌ 15294ms
Deduplication.Generates Unique Uuids ✅ 2974ms
Deduplication.Preserves Uuid On Retry ✅ 8818ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 16712ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 7844ms
Deduplication.No Duplicate Events In Batch ✅ 2901ms
Deduplication.Different Events Have Different Uuids ✅ 2833ms
Compression.Sends Gzip When Enabled ✅ 2733ms
Batch Format.Uses Proper Batch Structure ✅ 2666ms
Batch Format.Flush With No Events Sends Nothing ✅ 230ms
Batch Format.Multiple Events Batched Together ✅ 2854ms
Error Handling.Does Not Retry On 403 ✅ 4800ms
Error Handling.Does Not Retry On 413 ✅ 4820ms
Error Handling.Retries On 408 ✅ 8853ms

Failures

retry_behavior.max_retries_respected

Expected 4 requests, got 5

Feature_Flags Tests

✅ 16/16 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id ✅ 2745ms
Request Payload.Flags Request Uses V2 Query Param ✅ 2831ms
Request Payload.Flags Request Hits Flags Path Not Decide ✅ 2833ms
Request Payload.Flags Request Omits Authorization Header ✅ 2792ms
Request Payload.Token In Flags Body Matches Init ✅ 2837ms
Request Payload.Groups Round Trip ✅ 2713ms
Request Payload.Groups Default To Empty Object ✅ 2691ms
Request Payload.Person Properties Distinct Id Auto Populated When Caller Omits It ✅ 2721ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False ✅ 2787ms
Request Payload.Disable Geoip Omitted Defaults To False ✅ 2873ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key ✅ 2836ms
Request Lifecycle.No Flags Request On Init Alone ✅ 86ms
Request Lifecycle.No Flags Request On Normal Capture ✅ 457ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests ✅ 5467ms
Request Lifecycle.Mock Response Value Is Returned To Caller ✅ 2766ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event ✅ 3139ms

@marandaneto
marandaneto marked this pull request as ready for review September 10, 2026 09:16
@marandaneto
marandaneto requested a review from a team as a code owner September 10, 2026 09:16
@marandaneto

Copy link
Copy Markdown
Member Author

@ioannisj or @turnipdabeets can you help me testing this out? its very specific and we dont want to break anything, burnt quite some time figuring this out already

@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Reviews (1): Last reviewed commit: "fix: bound subscriber count notification..." | Re-trigger Greptile

@marandaneto

Copy link
Copy Markdown
Member Author

Live session replay smoke test passed on an iPhone 17 Pro simulator (iOS 26.5), using commit f07a39f32 against US Cloud.

  • Exercised stop/resume, 10 rapid stop/resume cycles, new sessions, background/foreground transitions, navigation, sheets, and synthetic text input.
  • Tested with Surveys enabled and disabled.
  • Observed 49 successful snapshot upload requests and inspected actual encoded replay frames to confirm masking.
  • No crashes or hangs observed. One upload timeout around background/foreground recovered; subsequent uploads succeeded.

Replay: https://us.posthog.com/shared/ivb2F5GoUwu1-l4WxOuE4a4E1jVsLg?t=11

This validates the tested simulator flows, not every replay scenario or the production cause of #806. Web-player playback was not independently verified during the smoke test. Temporary example configuration/instrumentation was removed, the baseline app restored, and the worktree left clean. No project token was committed.

…-layout-lifecycle

# Conflicts:
#	PostHog/ApplicationViewLayoutPublisher.swift
#	PostHogTests/ApplicationViewLayoutPublisherTest.swift
@ioannisj

Copy link
Copy Markdown
Contributor

Did a pre-pr and PR repro as well and could not find anything that this changes could break. Extra locking was marked as insignificant and nothing above simulator noise. I'll run through the code as well but lgtm

Comment thread PostHog/ApplicationViewLayoutPublisher.swift Outdated
@marandaneto
marandaneto enabled auto-merge (squash) September 11, 2026 07:29
@marandaneto
marandaneto merged commit 20992b2 into main Sep 11, 2026
48 checks passed
@marandaneto
marandaneto deleted the investigate/issue-806-layout-lifecycle branch September 11, 2026 07:32
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.

2 participants