Repository navigation
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
8bda8ce to
f78b14b
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
f78b14b to
6d3e886
Compare
6d3e886 to
a91cd13
Compare
a91cd13 to
d03300a
Compare
d03300a to
3e79081
Compare
3e79081 to
adbf97e
Compare
adbf97e to
8ddd34d
Compare
8ddd34d to
adac5f9
Compare
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Just some minor questions
| "db.namespace": "appdb", | ||
| "server.address": "localhost", | ||
| "server.port": 5432, | ||
| "server.port": float64(5432), |
There was a problem hiding this comment.
l: This kinda looks wrong; why do we now need to use floats for server ports?
There was a problem hiding this comment.
This is unfortunately somewhat of a hack. The mock transport encodes to the wire format and we decode back so we don't really know the type. This affects only tests though.
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
My clanker identified some issues that actually seem to be somewhat meaningful regressions. I had it create a reproduction here: #1450.
I commented the precise issues inline. Would appreciate if you could take a look and say whether these are expected behavior
| return client.telemetryProcessor.FlushWithContext(ctx) | ||
| } | ||
| return client.Transport.FlushWithContext(ctx) | ||
| return client.telemetryProcessor.FlushWithContext(ctx) |
There was a problem hiding this comment.
h: Clanker thinks that this change breaks the flushing timeout for the sync transport
Routing the synchronous transport through the processor appears to break the flush deadline for buffered logs/metrics. With one buffered log and a fake HTTP request that stalls for 30 seconds, client.Flush(time.Second) takes 30 seconds and returns true; the equivalent stack-base run returns after one second.
Scheduler.FlushWithContext calls flushBuffers() synchronously before reaching the transport flush. That drain invokes blocking SyncTransport.SendEnvelope, which uses a background context rather than the flush context. Can we make the deadline cover buffer draining/submission as well, not just the final transport flush?
Both versions already return true in this scenario; the newly introduced regression is the caller waiting beyond its deadline.
There was a problem hiding this comment.
l: Clanker says timing regression is fixed, but that this still can report flush success, when failure should be reported instead:
l: The timing regression is fixed, but a pre-existing return-value issue remains: with only a pending client report and a stalled synchronous request,
Flush(time.Second)returns after one second but reportstrue.The report send reaches the deadline, then
SyncTransport.FlushWithContextunconditionally returnstrue. Could this path checkctx.Err()before reporting success?This also occurred at the previously reviewed head; it is not a regression introduced by these fixes.
| client.setupTransport() | ||
| client.setupTelemetryProcessor() |
There was a problem hiding this comment.
m: Clanker thinks there is a deadlock; it claims to have observed it on a make race-test run also. Not sure how relevant this is in practice though.
Starting the processor for mock/custom transports exposes an existing scheduler shutdown race. Repeatedly creating a client with
MockTransportand immediately closing it insidetesting/synctestpasses on the stack base, but fails on this head with a deadlock: the scheduler is left blocked insync.Cond.WaitafterClosereturns.
Scheduler.Stopcancels and broadcasts without holdings.mu. The worker can check cancellation, then miss the broadcast before enteringWait; the periodic notifier also exits on cancellation, so no later notification is guaranteed. Can we synchronize cancellation/notification with the waiter before enabling this path for every client?This is scheduling-dependent, but the focused 1,000-iteration reproduction has failed repeatedly. The same deadlock also appeared in a full workspace race-test run.
| b.scheduler.recorder.RecordItem(report.ReasonInternalError, item) | ||
| return false | ||
| } | ||
| return b.scheduler.sendItem(convertible) |
There was a problem hiding this comment.
m: If I understand correctly, this skips buffering. Clanker thinks this is sensible for the SyncTransport, but it might not be for the AsyncTransport, although this change would apply to both.
Did I get this right, and if so, what would need to be changed?
There was a problem hiding this comment.
Yeah that's true. The problem here is that we want the sync transport to be truly sync for non batchable payloads, but we can't really do this cleanly.
4fd76f1 to
808380d
Compare
808380d to
f09b1eb
Compare
f09b1eb to
ee77ba1
Compare
1e9eec8 to
6aca134
Compare
5674423 to
84c46c8
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 91d5ea9. Configure here.
| Sdk: t.resolveSdkInfo(), | ||
| } | ||
| envelope := protocol.NewEnvelope(header) | ||
| envelope.AddItem(item) |
There was a problem hiding this comment.
Close drops accepted async envelopes
Medium Severity
The worker now exits as soon as t.ctx is canceled, but SendEnvelope still treats the transport as open until t.done is closed under closeMu. Close cancels t.ctx before it can take that lock, so a concurrent SendEnvelope can enqueue after the worker has already stopped. Those envelopes are accepted with a nil error and then never sent or recorded.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 91d5ea9. Configure here.
| } | ||
| } | ||
|
|
||
| if client.telemetryProcessor != nil { | ||
| if !client.telemetryProcessor.Add(event) { | ||
| debuglog.Println("Event dropped: telemetry buffer full or unavailable") | ||
| return nil | ||
| } | ||
| } else { | ||
| client.Transport.SendEvent(event) | ||
| if !client.telemetryProcessor.Add(ctx, event) { | ||
| debuglog.Println("Event dropped: telemetry buffer full or unavailable") | ||
| return nil |
There was a problem hiding this comment.
Bug: The capture() function unconditionally accesses client.telemetryProcessor without a nil check, risking a panic if called directly with a noopClient.
Severity: MEDIUM
Suggested Fix
Add a defensive check inside the capture function before accessing telemetryProcessor. This can be achieved by adding an !client.IsEnabled() check, consistent with the pattern used in captureLog and captureMetric, to prevent the method call when the client is disabled and telemetryProcessor is nil.
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: client.go#L887-L892
Potential issue: The `capture()` function at line 890 unconditionally calls
`client.telemetryProcessor.Add(ctx, event)`. A `noopClient`, which is a documented and
expected state, has a `nil` `telemetryProcessor`. While the current public API callers
like `CaptureEvent` prevent this issue with an `IsEnabled()` check, the `capture()`
function itself is not self-defensive. This creates a dependency on caller discipline.
Any future code path that calls `capture()` directly on a `noopClient` without a prior
`IsEnabled()` check will cause a nil pointer dereference panic. This pattern is also
inconsistent with `captureLog()` and `captureMetric()`, which do perform this check
internally.
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Approving, but there are still some l items 🚀
| } | ||
|
|
||
| // Add adds a TelemetryItem to the appropriate buffer based on its category. | ||
| // Add buffers item, or submits single-item categories. |
There was a problem hiding this comment.
l: typos
| // Add buffers item, or submits single-item categories. | |
| // Adds buffered items, or submits single-item categories. |
| @@ -303,21 +342,25 @@ func (s *Scheduler) sendItem(item EnvelopeConvertible) { | |||
| if err != nil { | |||
| debuglog.Printf("error while converting to envelope: %v", err) | |||
| s.recorder.RecordItem(report.ReasonInternalError, item) | |||
| return | |||
| return false | |||
| } | |||
| s.sendEnvelope(envelope) | |||
| return s.sendEnvelope(ctx, envelope, wait) | |||
| } | |||
|
|
|||
| func (s *Scheduler) sendEnvelope(envelope *protocol.Envelope) bool { | |||
| func (s *Scheduler) sendEnvelope(ctx context.Context, envelope *protocol.Envelope, wait bool) bool { | |||
| if len(envelope.Items) == 0 { | |||
| return false | |||
| } | |||
| s.provider.AttachToEnvelope(envelope) | |||
| return s.sendPreparedEnvelope(envelope) | |||
| return s.sendPreparedEnvelope(ctx, envelope, wait) | |||
| } | |||
|
|
|||
| func (s *Scheduler) sendPreparedEnvelope(envelope *protocol.Envelope) bool { | |||
| if err := s.transport.SendEnvelope(envelope); err != nil { | |||
| func (s *Scheduler) sendPreparedEnvelope(ctx context.Context, envelope *protocol.Envelope, wait bool) bool { | |||
| err := s.transport.SendEnvelope(ctx, envelope) | |||
| if wait && errors.Is(err, ErrQueueFull) && s.transport.FlushWithContext(ctx) { | |||
| err = s.transport.SendEnvelope(ctx, envelope) | |||
| } | |||
| if err != nil { | |||
| debuglog.Printf("error sending envelope: %v", err) | |||
| reason := report.ReasonSendError | |||
| if errors.Is(err, ErrQueueFull) { | |||
| @@ -330,26 +373,19 @@ func (s *Scheduler) sendPreparedEnvelope(envelope *protocol.Envelope) bool { | |||
| } | |||
There was a problem hiding this comment.
l: Given the similar names of these functions, I think some short documentation comments explaining each could be helpful. Especially, it would be helpful to document the meaning of the wait parameter and the boolean return value, as it is not immediately clear what these are indicating.
| Sdk: t.resolveSdkInfo(), | ||
| } | ||
| envelope := protocol.NewEnvelope(header) | ||
| envelope.AddItem(item) |
| return client.telemetryProcessor.FlushWithContext(ctx) | ||
| } | ||
| return client.Transport.FlushWithContext(ctx) | ||
| return client.telemetryProcessor.FlushWithContext(ctx) |
There was a problem hiding this comment.
l: Clanker says timing regression is fixed, but that this still can report flush success, when failure should be reported instead:
l: The timing regression is fixed, but a pre-existing return-value issue remains: with only a pending client report and a stalled synchronous request,
Flush(time.Second)returns after one second but reportstrue.The report send reaches the deadline, then
SyncTransport.FlushWithContextunconditionally returnstrue. Could this path checkctx.Err()before reporting success?This also occurred at the previously reviewed head; it is not a regression introduced by these fixes.
2 participants
Description
Replace the legacy transport path with envelope-based delivery, supporting built-in and custom transports consistently.
Issues
Changelog Entry Instructions
To add a custom changelog entry, uncomment the section above. Supports:
For more details: custom changelog entries
Reminders
feat:,fix:,ref:,meta:)Stack created with GitHub Stacks CLI • Give Feedback 💬