feat: add tracking lifecycle events - #13
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a lifecycle event system for SDK observability, including an in-memory event bus, event definitions, and integration across the context and tracking modules. The feedback identifies a potential infinite loop in the tracking queue if the batch size is not properly constrained and highlights a race condition where concurrent flushes could lead to duplicate event delivery, suggesting the implementation of a dedicated release lock.
| with self._lock: | ||
| if not self._pending: | ||
| break | ||
| batch = tuple(self._pending[: self._tracking_config.batch_size]) |
There was a problem hiding this comment.
If batch_size is configured to 0 or a negative value, this loop will become infinite because self._pending will never be reduced. Ensure a minimum batch size of 1 to prevent hanging the process.
| batch = tuple(self._pending[: self._tracking_config.batch_size]) | |
| batch_size = max(1, self._tracking_config.batch_size) | |
| batch = tuple(self._pending[:batch_size]) |
| self._event_bus = event_bus | ||
| self._pending: list[ConversionEvent] = [] | ||
| self._triggered_goals: set[tuple[str, str]] = set() | ||
| self._lock = Lock() |
There was a problem hiding this comment.
Add a dedicated lock to prevent concurrent flushes. While _lock protects the queue's integrity, it is released during network transport, which allows multiple release calls to process and potentially duplicate the same events.
| self._lock = Lock() | |
| self._lock = Lock() | |
| self._release_lock = Lock() |
| while True: | ||
| with self._lock: | ||
| if not self._pending: | ||
| break | ||
| batch = tuple(self._pending[: self._tracking_config.batch_size]) |
There was a problem hiding this comment.
This loop is susceptible to a race condition if multiple threads call release() concurrently. Since _lock is released during the network call (send_tracking), another thread can enter the loop, take the same batch of events, and send them again. Use the _release_lock suggested in __init__ to wrap the release method body to ensure mutual exclusion during flushes.
All eight open Dependabot alerts on this repo sit in `yarn.lock` — the dev-only semantic-release tooling. None of them reach the published wheel/sdist (httpx is still the only runtime dependency). Three of the four highs were already reachable inside the existing semver ranges; the fourth (sigstore GHSA-52v5-jr5w-gjxr, `certificateOIDs` verification constraints silently dropped) needed sigstore >= 4.1.1, which only arrives through a major bump: semantic-release 24.2.9 -> 25.0.9 @semantic-release/npm 12.0.2 -> 13.1.5 npm 10.9.8 -> 11.19.0 libnpmpublish 10 -> 11.2.0 }-> sigstore ^3 -> ^4 (4.1.1) pacote 19 -> 21.5.1 } @sigstore/core 2.0.0 -> 3.2.1 tar 7.5.16 -> 7.5.22 `@semantic-release/github` moves to ^12 to match what semantic-release 25 depends on — leaving it at ^11 would hoist the older copy to the project root and shadow the one core resolves. Resolved (4 high, 4 medium — no criticals were open): #14 high ip-address 10.2.0 -> 10.4.0 (needs >= 10.3.1) #9 high brace-expansion 2.1.1 -> 5.0.9 (needs >= 2.1.2) #7 high js-yaml 4.2.0 -> 4.3.1 (needs >= 4.3.0) #2 high sigstore 3.1.0 -> 4.1.1 (needs >= 4.1.1) #13 medium ip-address (same bump as #14) #12 medium ip-address (same bump as #14) #4 medium tar 7.5.16 -> 7.5.22 (needs >= 7.5.18) #1 medium @sigstore/core 2.0.0 -> 3.2.1 (needs >= 3.2.1) Node: semantic-release 25 requires ^22.14.0 || >= 24.10.0. release.yml installs `lts/*`, currently Node 24.19.0 — satisfied, and every future LTS line stays above the floor. Verified: `yarn install --immutable` (what release.yml runs) passes against the regenerated lockfile with the lockfile format unchanged (__metadata version 10), and `yarn npm audit --all --recursive` reports no suggestions. A `semantic-release --dry-run` against this branch loads all four configured plugins, passes verifyConditions for both exec and github (GitHub authentication + push permission), and analyzes commits to "no release" — correct, since `chore` is a non-releasing type in release.config.mjs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Adds tracking lifecycle observability for the Convert Python SDK.
This PR completes the tracking MVP by adding:
BMAD scope covered:
What Changed
LifecycleEventLifecycleEventPayloadCore:Core.on(...)Core.off(...)Context.release_queues(...)Why
The tracking queue from Story 2.3 made delivery explicit and efficient, but it still lacked observability for integrators and maintainers.
This PR makes the tracking pipeline diagnosable without changing the control model:
Validation
Ran in
../python-sdk:uv sync --group devuv run pytest -p no:cacheprovideruv buildResults:
41/41tests passedNotes
This PR completes the tracking MVP boundary for Epic 2.
Not included here: