Repository navigation
fix(trace): replace overlapping cache fetch results atomically - #913
Gareth.Y (linjiaye929) wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d11e988111
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| # append the same spans twice. A failed or cancelled fetch leaves the | ||
| # previous cache intact, and a full fetch replaces every prior type. | ||
| if span_type is None: | ||
| self._span_cache = fetched |
There was a problem hiding this comment.
When two concurrent full reads observe different snapshots—for example, request A is issued first and its nonempty response is delayed while request B sees an additional newly visible span—B can cache the newer response first, after which this unconditional assignment lets A replace it with the older subset. A's caller then sets _all_fetched to true, so subsequent get_spans calls never refetch and omit the newer span for the lifetime of the fetcher; coalescing full reads or tracking request generations would prevent an older in-flight response from superseding a newer one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6eed80c.
Confirmed with differently ordered full responses, and extended coverage to full/typed and typed/typed overlaps. The cache now records request generations for committed full responses and type groups, so an older response cannot overwrite a group from a later-started successful request. Full reads still replace stale groups, including types absent from their response, while preserving later-started committed typed groups.
Failures and cancellation leave committed generations unchanged. Empty full responses stay retryable, including when retaining a newer typed group; empty typed responses and advanced-filter/scorer bypass behavior are preserved. This orders overlapping requests, not server revisions, so it does not promise snapshot freshness.
The new regression set gives 13 failures and 35 passing controls on the prior PR head; the final trace suite passes all 101 tests. Targeted Ruff, Pylint, codespell and diff checks pass. The final full core rerun remains unverified due to an environment restriction on possible outbound test traffic.
AI-assisted follow-up.
This branch has not been deployed
1 participant
Concurrent
trace.get_spans()calls can append overlapping results into the same per-type cache. One caller receives duplicate spans, and later cached reads keep returning them. This can affect scorers sharing aLocalTracewhen reads fall back to the backend. It is the concurrent counterpart of the sequential issue fixed in #546.Build each response's type groups locally, then replace the full cache or update the returned groups in one synchronous step. Failed or cancelled requests leave successful cached data intact. An overlapping empty full response remains retriable. Rows repeated within a single backend response are preserved.
Tests cover full/full, typed/typed, partially overlapping types, full/typed in both completion orders, failure/cancellation, empty full responses, and stale-type replacement. Existing filtering and empty-result retry tests still pass. Concurrent calls may still issue overlapping backend reads.
Validation on Python 3.12.14:
test_trace.py: 53 passed, including existing cassette-backed backend parity.test_corenox session: 990 passed, 73 skipped, 12 xfailed. Reused the existing core environment with--no-install; verified imports came from this worktree and dependency compatibility checks passed.pylint --errors-only, andgit diff --checkpassed.make fixuprequires unavailablemise, so itspre-commit run --all-filescommand was run directly. Other Python versions, full provider suites, whole-SDK pylint, and live backend calls were not tested.AI assistance was used to implement and test this change.