Repository navigation
Collector: a client that goes away no longer cancels ingest - #164
Merged
Merged
Conversation
…could reach R2 and never be counted) ingest() registered its counting with waitUntil only after awaiting the R2 write. A client that reset its connection during that write (about 150 ms; a browser being shut down) cancelled the handler, so the batch stayed in R2 without reaching the dashboard's counters, the export index or forwarding. A reset before the write lost the batch altogether. waitUntil now holds ingest from its first line; the response is unchanged. Under local workerd with the real entry point, 10 batches per case, the client resetting 100 ms in, R2's timing modelled: write landed, ack pending: before 10 stored, 0 counted; after 10 stored, 10 counted write not started yet: before 0 stored, 0 counted; after 10 stored, 10 counted No change when the client waits or closes normally; half a body still stores nothing; a repeated batch still counts once. Production itself is not observed (logging is off, the error was swallowed): one app showed 13 of 198 page loads in R2 and not in D1, all solo visits by an automated browser. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Summary
ingest()registered its counting withwaitUntilonly after awaiting the R2 write. A client that reset its connection during that write (about 150 ms; for example a browser being shut down) cancelled the handler, so the batch stayed in R2 but never reached the dashboard's counters, the export index or forwarding. A reset before the write lost the batch altogether.handle()now passes the whole ofingest()toopts.waitUntilfrom its first line. The response is unchanged and other routes are not touched.Evidence
Local workerd with the real entry point (
src/index.ts). R2's timing is simulated in the two reset rows, where the client resets 100 ms in. Each cell is batches stored in R2 / counted on the dashboard:A batch sent twice still counts once. A plain close (FIN) does not cancel the handler under workerd, so reproducing this needs a connection reset (
socket.resetAndDestroy()in Node).In production the cancellation itself can't be observed, because logging is off and the error was swallowed. What points to it:
Test plan
cd telemetry-worker && npx vitest run: 363 pass. The two new tests intest/index.test.tsfailed before the fix.sessionsrows for new page loads. The gap should close.Deploying
Not deployed. The live worker runs
931b6f45, so deploying from main after this merges also shipse8767ebfand22590994, which change text inpages.tsandstudio.ts. There are no migrations. Counts already missed are not backfilled.🤖 Generated with Claude Code