Conversation
Contributor
size-limit report 📦
|
JPeer264
force-pushed
the
jp/cf-int-tests-bindings-streaming
branch
from
September 19, 2026 15:46
bfc18d9 to
4b27b89
Compare
JPeer264
marked this pull request as ready for review
September 21, 2026 06:45
JPeer264
requested review from
isaacs,
msonnb and
mydea
and removed request for
a team
September 21, 2026 06:45
Contributor
JPeer264
force-pushed
the
jp/cf-int-tests-bindings-streaming
branch
from
September 27, 2026 09:54
4b27b89 to
31da3d8
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
9
to
14
| export default Sentry.withSentry( | ||
| (env: Env) => ({ | ||
| dsn: env.SENTRY_DSN, | ||
| traceLifecycle: 'static', | ||
| tracesSampleRate: 1, | ||
| }), | ||
| { |
Contributor
There was a problem hiding this comment.
Bug: The D1 batch test can fail due to a race condition where parent and child spans arrive in separate envelopes, but the test asserts they are in the same one.
Severity: MEDIUM
Suggested Fix
The test should be updated to handle spans arriving in separate envelopes. Use collectStreamedSpans() to wait for all spans before making assertions, which is the pattern used in the Prisma and Vercel AI tests. Alternatively, restructure the assertions so they do not depend on finding parent and child spans within the same envelope.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: dev-packages/cloudflare-integration-tests/suites/d1/index.ts#L9-L14
Potential issue: The D1 batch integration test is susceptible to a race condition. The
test expects a parent segment span and its child 'D1 batch' span to be present in the
same envelope when using `.unordered()` mode. However, due to the test runner's
streaming nature and timed buffer flushes, these spans can arrive in separate envelopes.
When this occurs, the logic attempting to find the child span fails because it's not in
the same envelope as the parent. Consequently, a later assertion,
`expect(directBatchSpan).toBeDefined()`, fails, leading to flaky test outcomes in CI
environments.
Also affects:
dev-packages/cloudflare-integration-tests/suites/d1/test.ts:85~110
Did we get this right? 👍 / 👎 to inform future reviews.
JPeer264
force-pushed
the
jp/cf-int-tests-bindings-streaming
branch
from
September 27, 2026 13:28
31da3d8 to
6c87954
Compare
JPeer264
force-pushed
the
jp/cf-int-tests-bindings-streaming
branch
from
September 29, 2026 08:19
6c87954 to
8f0e653
Compare
Contributor
nicohrubec
approved these changes
Sep 29, 2026
Removes the `traceLifecycle: 'static'` pin from `suites/d1`, `suites/r2`,
`suites/queue`, `suites/prisma`, `suites/durableobject/error`,
`suites/workflows/step-context` and `suites/vite/diagnostics-channel/vercelai-6`,
and rewrites the assertions from transaction envelopes to span v2.
`durableobject/error` and `workflows/step-context` only assert on error events,
so the pin is all that goes. `cache-client` and `durableobject-scope` were
already unpinned; they drop `transaction` from their ignore list, which is now an
envelope type nothing emits.
The binding spans keep the shape they had, and only the encoding changes: a span
carries its name in `name` rather than `description`, its op and origin as
attributes, and every attribute as a `{ type, value }` pair. The r2 and queue
suites therefore keep one envelope expectation per request.
Two things do change.
D1 and Prisma spans are `db.query`, so streaming names them after
`db.query.summary`. `SELECT * FROM users WHERE id = ?` becomes `SELECT users`.
The Prisma suite loses the description-based split between its two `SELECT`
spans, which now share the name `SELECT main.User`, so the D1 one is picked by its
op and the traceparent comment is asserted on `db.query.text` instead.
`prisma` and `vercelai-6` collect until the whole trace is in hand, seventeen spans
and three. Both assert on the complete child set of one request, and the span
buffer flushes on a timer, so reading a single envelope would be a race. Waiting
for the segment span would be one too: it ends last, but each envelope is its own
request to the mock server, so it can arrive before its children do.
`vercelai-6` also drops the separate span container it used to read next to the
transaction item: a streamed gen_ai span is an ordinary item of the one span
envelope.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
JPeer264
force-pushed
the
jp/cf-int-tests-bindings-streaming
branch
from
September 29, 2026 13:34
8f0e653 to
849c4dc
Compare
Comment on lines
9
to
14
| export default Sentry.withSentry( | ||
| (env: Env) => ({ | ||
| dsn: env.SENTRY_DSN, | ||
| traceLifecycle: 'static', | ||
| tracesSampleRate: 1, | ||
| }), | ||
| { |
Contributor
There was a problem hiding this comment.
Bug: Removing traceLifecycle: 'static' may cause the R2 integration test to become flaky. The test expects r2_put and r2_get spans in one envelope, which is not guaranteed with streaming.
Severity: MEDIUM
Suggested Fix
Update the R2 test to handle streamed spans robustly. Instead of assuming both r2_put and r2_get spans are in the same envelope, modify the test to either accumulate spans from multiple envelopes before asserting or use separate, unordered expectations for each span type, similar to how other integration tests are structured.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: dev-packages/cloudflare-integration-tests/suites/r2/index.ts#L9-L14
Potential issue: By removing `traceLifecycle: 'static'`, the span collection behavior
changes from batching all spans into a single envelope to streaming them, potentially
across multiple envelopes. The R2 integration test is not prepared for this change. It
uses a single `.expect()` callback that asserts both `r2_put` and `r2_get` spans are
present in the same envelope. If these spans are flushed at different times and arrive
in separate envelopes, the test will fail because one of the assertions will find an
empty array of spans. This will lead to flaky test failures in the CI pipeline.
Also affects:
dev-packages/cloudflare-integration-tests/suites/r2/test.ts:104~154
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
2 participants
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.
Ports the binding suites off the
traceLifecycle: 'static'pin:d1,r2,queue,prisma,durableobject/error,workflows/step-contextandvite/diagnostics-channel/vercelai-6.durableobject/errorandworkflows/step-contextonly assert on error events, so the pin is all that goes.cache-clientanddurableobject-scopewere already unpinned and keep ignoring spans, so they are untouched.Most binding spans keep the shape they had and only the encoding changes, so the r2 and queue suites keep one envelope expectation per request. Two things do change.
D1 and Prisma spans carry the
db.queryop, so streaming names them afterdb.query.summary:SELECT * FROM users WHERE id = ?becomesSELECT users. The Prisma suite loses the description-based split between its twoSELECTspans, which now share the nameSELECT main.User. The D1 one is picked by its op instead, and the traceparent comment that used to be matched on the description is asserted ondb.query.text.prismaandvercelai-6switch tocollectStreamedSpansUntilSegment. Both assert on the complete child set of one request and the span buffer flushes on a timer, so reading a single envelope would be a race.vercelai-6also drops the separate span container it used to read next to the transaction item: a streamed gen_ai span is an ordinary item of the one span envelope./keeps itsGET /name under streaming (the source isroute, noturl), sovercelai-6waits on that rather than the bare method the raw-URL suites see.Part of #24148
🤖 Generated with Claude Code