fix: Guard EventParser.lastEventId against a data race#108
Merged
Conversation
getLastEventId() reads EventParser.lastEventId from arbitrary caller threads (via EventSource.getLastEventId()), while it is written on the serialized delegate queue during event dispatch. Reading a String concurrently with a write is undefined behavior (a torn copy-on-write buffer), not merely a stale value; the @unchecked Sendable delegate suppressed the compiler check. Guard the field with an NSLock: lastEventId is now a computed property whose get and set lock a private backing store, so the cross-thread read is safe regardless of the writing thread. parse()/reset()/dispatch remain serialized on the delegate queue and are unchanged. Mark EventParser @unchecked Sendable to document that split (serialized mutation + lock-guarded cross-thread read). Adds a concurrency test that hammers getLastEventId() from one task while another drives events, asserting correctness under load. Run under `--sanitize=thread` to catch a regression (TSan is unavailable on the Linux toolchain here; the macOS CI leg is TSan-capable).
3845aa0 to
b8cd872
Compare
Runs the suite under `swift test --sanitize=thread` on macOS so data races (like the getLastEventId one this fixes) are caught in CI. Sets TSAN_OPTIONS=halt_on_error=1 so a detected race aborts with a non-zero exit and fails the job — without it TSan only prints a warning and the step stays green. The Linux toolchain doesn't support -sanitize=thread, so this macOS job is where the getLastEventIdIsSafeDuringConcurrentParsing test's race coverage actually runs.
b8cd872 to
efa6daf
Compare
tanderson-ld
approved these changes
Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
getLastEventId() reads EventParser.lastEventId from arbitrary caller threads (via
EventSource.getLastEventId()), while it is written on the serialized delegate
queue during event dispatch. Reading a String concurrently with a write is
undefined behavior (a torn copy-on-write buffer), not merely a stale value; the
@unchecked Sendable delegate suppressed the compiler check.
Guard the field with an NSLock: lastEventId is now a computed property whose get
and set lock a private backing store, so the cross-thread read is safe regardless
of the writing thread. parse()/reset()/dispatch remain serialized on the delegate
queue and are unchanged. Mark EventParser @unchecked Sendable to document that
split (serialized mutation + lock-guarded cross-thread read).
Adds a concurrency test that hammers getLastEventId() from one task while another
drives events, asserting correctness under load. Run under
--sanitize=threadtocatch a regression (TSan is unavailable on the Linux toolchain here; the macOS CI
leg is TSan-capable).
Note
Medium Risk
Touches hot-path SSE state read from arbitrary threads; the lock is narrow but incorrect locking could still affect reconnect
Last-Event-Idbehavior under load.Overview
Fixes a data race on
EventParser.lastEventId:getLastEventId()can run on caller threads (viaEventSource) while dispatch updates the field on the serialized delegate queue. ConcurrentStringread/write is undefined behavior, not just stale values.lastEventIdis now backed by_lastEventIdbehind anNSLockvia a computed property; parse/reset/dispatch stay on the delegate queue.EventParseris@unchecked Sendablewith comments describing that split.Adds
getLastEventIdIsSafeDuringConcurrentParsing(writer parses events, reader hammersgetLastEventId()). CI gainstest-thread-sanitizeron Ubuntu (swift:6.1) runningswift test --sanitize=threadwithTSAN_OPTIONS=halt_on_error=1so races fail the job.Reviewed by Cursor Bugbot for commit efa6daf. Bugbot is set up for automated code reviews on this repo. Configure here.