Skip to content

test: capture invariant hardening - #1029

Open
eli-r-ph wants to merge 2 commits into
v1-capture-async-ai-lanefrom
v1-capture-invariant-tests
Open

eli-r-ph wants to merge 2 commits into
v1-capture-async-ai-lanefrom
v1-capture-invariant-tests

Conversation

@eli-r-ph

@eli-r-ph eli-r-ph commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

💡 Motivation and Context

posthog-go and posthog-rs found a set of capture invariants the hard way: a data race on enqueue during close, a pass-through hook that changed the wire, a size guard that refused events the endpoint accepts. This PR checks the Python stack against each one, and adds tests only where no existing test fails on the regression.

Changes

Tests only. No library code changes. Each invariant, and where it is covered:

Invariant Coverage
A pass-through before_send leaves the wire unchanged (null option falls back to its legacy property, empty containers stay empty, nested maps survive) New test_capture_invariants.py, sync and async, both lanes
Enqueue racing shutdown, including the first AI event starting the lazy lane test_shutdown_waits_for_racing_enqueue_before_draining now runs on both lanes and asserts delivery and no live consumers. The async cross-thread admission test adds a capture_ai case that must start no AI workers
An AI event whose properties sit at the endpoint ceiling is sent Was covered for the queued sync lane. Now also covered for sync_mode, capture_ai_immediate and the async queued lane, plus an over-guard case
No silent loss: one aggregate line per failed batch, naming the lane's endpoint, with no response text The sync sync_mode and async tests now cover all four capture methods and assert the endpoint
Fork Already covered: TestLaneForkRebuild (both lanes, sync_mode)
Config rejects batch < event Not applicable: Python's batch target is a fixed 5 MiB soft limit that one larger event may exceed alone. The AI event cap can only be lowered from 8 MiB plus headroom, well under the request limit. capture_ai_max_event_bytes validation is already tested

💚 How did you test it?

Break-on-purpose, one mutation per invariant, each restored afterwards:

  • The hook path turns empty or falsy options into null, on the sync client and on the async client: both lane cases fail on each.
  • Lane admission checks _closed under the lock and puts outside it: both racing cases fail.
  • The AI guard loses its envelope headroom: four ceiling cases fail.
  • Async AI workers start before the shutdown recheck: the capture_ai admission case fails.
  • The loss line names the analytics endpoint for AI failures, separately on the sync path, the async queued path and the async immediate path: the matching AI case fails each time.

One mutation was not caught: _Lane.close() setting _closed without the lock. Shutdown still waits on that lock through wait_for_sync_sends, so the race does not occur, and I did not add a test for it.

📝 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 sampo add to generate a changeset file

Tests only; no changeset.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Written with Cursor (Claude Opus) under the direction of the assignee. Skills used: writing-tests, writing-pr-descriptions.

Agent calls worth review:

  • Most coverage extends existing tests with a lane parameter. The only new file holds the hook wire test, which has no existing neighbor and spans both clients.
  • The batch-versus-event config check is marked not applicable, not ported.

@eli-r-ph eli-r-ph self-assigned this Oct 7, 2026
@eli-r-ph
eli-r-ph force-pushed the v1-capture-async-ai-lane branch from d5eb0bc to ae647de Compare October 7, 2026 03:56
@eli-r-ph
eli-r-ph force-pushed the v1-capture-invariant-tests branch 6 times, most recently from 44df57f to 2011a51 Compare October 7, 2026 17:43
@eli-r-ph
eli-r-ph force-pushed the v1-capture-async-ai-lane branch from 42c1d07 to 691c8bd Compare October 7, 2026 17:43
@eli-r-ph

eli-r-ph commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

@greptileai review

@eli-r-ph
eli-r-ph marked this pull request as ready for review October 7, 2026 20:44
@eli-r-ph
eli-r-ph requested a review from a team as a code owner October 7, 2026 20:44

@dustinbyrne dustinbyrne 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.

The test-only hardening looks sound, and existing CI passes for this snapshot. One non-blocking suggestion inline: assert exactly one aggregate loss record in the newly named single-loss-line test.

AI-assisted review.

Comment thread posthog/test/test_async_client.py Outdated
@eli-r-ph
eli-r-ph force-pushed the v1-capture-invariant-tests branch from aac8735 to fd36bd2 Compare October 8, 2026 16:25
@eli-r-ph
eli-r-ph force-pushed the v1-capture-async-ai-lane branch from 691c8bd to 1f784bb Compare October 8, 2026 16:25
@eli-r-ph
eli-r-ph force-pushed the v1-capture-invariant-tests branch from fd36bd2 to d969e0f Compare October 8, 2026 18:13
@eli-r-ph
eli-r-ph force-pushed the v1-capture-async-ai-lane branch from 1f784bb to 04507c0 Compare October 8, 2026 18:13
@eli-r-ph
eli-r-ph force-pushed the v1-capture-invariant-tests branch from d969e0f to 84e195d Compare October 8, 2026 21:12
@eli-r-ph
eli-r-ph force-pushed the v1-capture-async-ai-lane branch from 04507c0 to adf5b16 Compare October 8, 2026 21:12
@eli-r-ph
eli-r-ph force-pushed the v1-capture-invariant-tests branch from 84e195d to e542984 Compare October 8, 2026 22:54
@eli-r-ph
eli-r-ph force-pushed the v1-capture-async-ai-lane branch from adf5b16 to 0cfa23c Compare October 8, 2026 22:54
@eli-r-ph
eli-r-ph force-pushed the v1-capture-invariant-tests branch from e542984 to e34c122 Compare October 8, 2026 23:40

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.

2 participants