fix: eliminate three thread-safety bugs in telemetry delta reporting#90
Conversation
- active_duration_s was incremented outside any lock in _activity_detector, letting _post_telemetry_delta read it twice with a torn value and permanently lose increments; move the increment inside _counter_lock and snapshot the field once under that lock before computing the delta - dict(rows_written) could raise RuntimeError when concurrent stream-flush threads added new keys mid-iteration; add _rows_written_lock to WriteManager and hold it at both the mutation site and every call site that copies the dict (telemetry delta + manifest snapshot) - skip post_telemetry network call when both duration_delta and events_delta are zero; defer advancing _last_posted_* until we actually post so no interval is silently dropped from the accumulator Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces thread-safety improvements to IOTracer and WriterManager by adding synchronization locks around shared state variables, such as active_duration_s and rows_written. It also fixes an initialization bug where self.automatic_upload was not correctly passed to WriteManager. The review feedback highlights two key areas for improvement: first, protecting the write_dropped dictionary under the same _rows_written_lock to prevent potential concurrent modification errors during manifest generation; second, improving encapsulation by exposing a thread-safe snapshot method on WriteManager rather than directly accessing its internal lock and dictionary from IOTracer.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
write_dropped was mutated on error paths without any lock, meaning two streams failing simultaneously could concurrently add new keys while _write_manifest iterated the dict via dict(), raising RuntimeError. Reuse the existing _rows_written_lock at the mutation site in _write_buffer_to_file and expand the snapshot block in _write_manifest to capture write_dropped alongside rows_written under the same lock. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nager IOTracer was reaching directly into _rows_written_lock and rows_written to build thread-safe snapshots, duplicating the lock pattern in two places. Add get_rows_written_snapshot() and get_counters_snapshot() to WriteManager so callers never touch the internal lock directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
Race on
active_duration_s:_activity_detectorwas incrementingactive_duration_soutside any lock, while_post_telemetry_deltaread it twice inside_telemetry_lock(which didn't guard the write). If the detector fired between the two reads,_last_posted_active_swould advance past what was actually reported, permanently losing that second from every future delta. Fix: move the increment inside the existing_counter_lockblock and snapshot the field once under that same lock in_post_telemetry_delta.RuntimeErrorfromdict(rows_written): Multiple stream-flush threads can add new keys torows_writtenconcurrently (each under its own per-stream lock, but with no shared guard on the dict). Iterating it viadict()while another thread inserts a key raisesRuntimeError: dictionary changed size during iteration. Fix: add_rows_written_locktoWriteManager, hold it at the single mutation site in_write_buffer_to_file, and hold it at everydict(rows_written)call site (_post_telemetry_deltaand_write_manifest).Redundant network calls:
post_telemetrywas called even when bothduration_deltaandevents_deltawere zero. Fix: early-return before advancing_last_posted_active_s/_last_posted_rowsso no interval is silently dropped from the accumulator on a skipped call.Test plan
pytest tests/and confirm no regressions--uploadenabled and verify telemetry still fires after each file uploadmanifest.jsonrows_writtencounts match expected stream output🤖 Generated with Claude Code