From adb84d97e316673fbb0ea40dec010dc2b4c76bed Mon Sep 17 00:00:00 2001 From: David Sheldrick Date: Tue, 2 Jul 2024 11:53:27 +0100 Subject: [PATCH] worker fixes (#4059) Describe what your pull request does. If you can, add GIFs or images showing the before and after of your change. ### Change type - [x] `bugfix` - [ ] `improvement` - [ ] `feature` - [ ] `api` - [ ] `other` ### Test plan 1. Create a shape... 2. - [ ] Unit tests - [ ] End to end tests ### Release notes - Fixed a bug with... --------- Co-authored-by: alex --- apps/bemo-worker/src/BemoDO.ts | 2 +- apps/bemo-worker/src/worker.ts | 2 +- apps/dotcom-asset-upload/package.json | 2 +- .../src/{index.ts => worker.ts} | 0 apps/dotcom-asset-upload/wrangler.toml | 4 +- apps/dotcom-worker/src/TLDrawDurableObject.ts | 2 +- apps/dotcom-worker/src/worker.ts | 2 +- packages/worker-shared/src/sentry.ts | 6 ++ scripts/package.json | 2 + scripts/workers/dev.ts | 63 ++++++++++++++++--- yarn.lock | 29 +++++++++ 11 files changed, 97 insertions(+), 17 deletions(-) rename apps/dotcom-asset-upload/src/{index.ts => worker.ts} (100%) diff --git a/apps/bemo-worker/src/BemoDO.ts b/apps/bemo-worker/src/BemoDO.ts index 604ea0f75..9147d22b1 100644 --- a/apps/bemo-worker/src/BemoDO.ts +++ b/apps/bemo-worker/src/BemoDO.ts @@ -8,7 +8,7 @@ export class BemoDO extends DurableObject { const sentry = createSentry(this.ctx, this.env, request) console.error(e) // eslint-disable-next-line deprecation/deprecation - sentry.captureException(e) + sentry?.captureException(e) } private readonly router = Router() diff --git a/apps/bemo-worker/src/worker.ts b/apps/bemo-worker/src/worker.ts index 411d8de0a..c32f048d3 100644 --- a/apps/bemo-worker/src/worker.ts +++ b/apps/bemo-worker/src/worker.ts @@ -27,7 +27,7 @@ export default class Worker extends WorkerEntrypoint { const sentry = createSentry(this.ctx, this.env, request) console.error(error) // eslint-disable-next-line deprecation/deprecation - sentry.captureException(error) + sentry?.captureException(error) return new Response('Something went wrong', { status: 500, statusText: 'Internal Server Error', diff --git a/apps/dotcom-asset-upload/package.json b/apps/dotcom-asset-upload/package.json index f8f7fd106..dbef280a3 100644 --- a/apps/dotcom-asset-upload/package.json +++ b/apps/dotcom-asset-upload/package.json @@ -9,7 +9,7 @@ }, "main": "src/index.ts", "scripts": { - "dev": "cross-env NODE_ENV=development wrangler dev --log-level info --persist-to tmp-assets", + "dev": "yarn run -T tsx ../../scripts/workers/dev.ts --persist-to tmp-assets", "test-ci": "lazy inherit --passWithNoTests", "test": "yarn run -T jest --passWithNoTests", "test-coverage": "lazy inherit --passWithNoTests", diff --git a/apps/dotcom-asset-upload/src/index.ts b/apps/dotcom-asset-upload/src/worker.ts similarity index 100% rename from apps/dotcom-asset-upload/src/index.ts rename to apps/dotcom-asset-upload/src/worker.ts diff --git a/apps/dotcom-asset-upload/wrangler.toml b/apps/dotcom-asset-upload/wrangler.toml index 3935c3b71..7a3c1d162 100644 --- a/apps/dotcom-asset-upload/wrangler.toml +++ b/apps/dotcom-asset-upload/wrangler.toml @@ -1,4 +1,4 @@ -main = "src/index.ts" +main = "src/worker.ts" compatibility_date = "2022-09-22" [dev] @@ -34,7 +34,7 @@ preview_bucket_name = 'uploads-preview' binding = 'UPLOADS' bucket_name = 'uploads-preview' -[[env.stating.r2_buckets]] +[[env.staging.r2_buckets]] binding = 'UPLOADS' bucket_name = 'uploads-preview' diff --git a/apps/dotcom-worker/src/TLDrawDurableObject.ts b/apps/dotcom-worker/src/TLDrawDurableObject.ts index a689750dc..15c14c37e 100644 --- a/apps/dotcom-worker/src/TLDrawDurableObject.ts +++ b/apps/dotcom-worker/src/TLDrawDurableObject.ts @@ -222,7 +222,7 @@ export class TLDrawDurableObject { } catch (err) { console.error(err) // eslint-disable-next-line deprecation/deprecation - sentry.captureException(err) + sentry?.captureException(err) return new Response('Something went wrong', { status: 500, statusText: 'Internal Server Error', diff --git a/apps/dotcom-worker/src/worker.ts b/apps/dotcom-worker/src/worker.ts index eb5878a08..11ef7c0c7 100644 --- a/apps/dotcom-worker/src/worker.ts +++ b/apps/dotcom-worker/src/worker.ts @@ -62,7 +62,7 @@ const Worker = { .catch((err) => { console.error(err) // eslint-disable-next-line deprecation/deprecation - sentry.captureException(err) + sentry?.captureException(err) return new Response('Something went wrong', { status: 500, diff --git a/packages/worker-shared/src/sentry.ts b/packages/worker-shared/src/sentry.ts index 058611c7c..6cfe63792 100644 --- a/packages/worker-shared/src/sentry.ts +++ b/packages/worker-shared/src/sentry.ts @@ -9,11 +9,17 @@ interface Context { interface SentryEnvironment { readonly SENTRY_DSN: string | undefined + readonly TLDRAW_ENV?: string | undefined readonly WORKER_NAME: string | undefined readonly CF_VERSION_METADATA: WorkerVersionMetadata } export function createSentry(ctx: Context, env: SentryEnvironment, request?: Request) { + // TLDRAW_ENV is undefined in dev + if (!env.SENTRY_DSN && !env.TLDRAW_ENV) { + return null + } + const { SENTRY_DSN, WORKER_NAME, CF_VERSION_METADATA } = requiredEnv(env, { SENTRY_DSN: true, WORKER_NAME: true, diff --git a/scripts/package.json b/scripts/package.json index 5ee9bb32b..e90dc208c 100644 --- a/scripts/package.json +++ b/scripts/package.json @@ -55,9 +55,11 @@ "dependencies": { "@octokit/rest": "^20.0.2", "@types/minimist": "^1.2.5", + "@types/proper-lockfile": "^4.1.4", "@types/tmp": "^0.2.6", "ignore": "^5.2.4", "minimist": "^1.2.8", + "proper-lockfile": "^4.1.2", "tar": "^7.0.1", "tmp": "^0.2.3", "toml": "^3.0.0" diff --git a/scripts/workers/dev.ts b/scripts/workers/dev.ts index 34b30659e..0d0ba91f1 100644 --- a/scripts/workers/dev.ts +++ b/scripts/workers/dev.ts @@ -1,11 +1,20 @@ import { ChildProcessWithoutNullStreams, spawn } from 'child_process' import kleur from 'kleur' +import { lock } from 'proper-lockfile' import stripAnsi from 'strip-ansi' -// at the time of writing, workerd will regularly crash with a segfault -// but the error is not caught by the process, so it will just hang -// this script wraps the process, tailing the logs and restarting the process -// if we encounter the string 'Segmentation fault' +const lockfileName = __dirname + +/** + * a long time ago, workerd would regularly crash with a segfault but the error is not caught by the + * process, so it will just hang. this script wraps the process, tailing the logs and restarting the + * process if we encounter the string 'Segmentation fault'. we think this error is probably fixed + * now. + * + * there's a separate issue where spawning multiple workerd instances at once would cause them to + * fight and crash. we use a lockfile to start our wrokerd instances one at a time, waiting for the + * previous one to be ready before we start the next. + */ class MiniflareMonitor { private process: ChildProcessWithoutNullStreams | null = null @@ -14,9 +23,10 @@ class MiniflareMonitor { private args: string[] = [] ) {} - public start(): void { - this.stop() // Ensure any existing process is stopped - console.log(`Starting wrangler...`) + public async start(): Promise { + await this.stop() // Ensure any existing process is stopped + await this.lock() + await console.log(`Starting wrangler...`) this.process = spawn(this.command, this.args, { env: { NODE_ENV: 'development', @@ -35,6 +45,11 @@ class MiniflareMonitor { private handleOutput(output: string, err = false): void { if (!output) return + + if (output.includes('Ready on') && this.isLocked()) { + this.release() + } + if (output.includes('Segmentation fault')) { console.error('Segmentation fault detected. Restarting Miniflare...') this.restart() @@ -43,18 +58,45 @@ class MiniflareMonitor { } } - private restart(): void { + private async restart(): Promise { console.log('Restarting wrangler...') - this.stop() + await this.stop() setTimeout(() => this.start(), 3000) // Restart after a short delay } - private stop(): void { + private async stop(): Promise { + if (this.isLocked()) await this.release() if (this.process) { this.process.kill() this.process = null } } + + private _lockPromise?: Promise<() => Promise> + private isLocked() { + return !!this._lockPromise + } + private async lock() { + if (this.isLocked()) throw new Error('Already locked') + console.log('Locking...') + this._lockPromise = lock(lockfileName, { + retries: { + minTimeout: 500, + retries: 10, + }, + }) + await this._lockPromise + console.log('Locked') + } + private async release() { + if (!this.isLocked()) throw new Error('Not locked') + console.log('Releasing...') + const lockPromise = this._lockPromise! + this._lockPromise = undefined + const release = await lockPromise + await release() + console.log('Released') + } } class SizeReporter { @@ -119,6 +161,7 @@ new MiniflareMonitor('wrangler', [ 'info', '--var', 'IS_LOCAL:true', + ...process.argv.slice(2), ]).start() new SizeReporter().start() diff --git a/yarn.lock b/yarn.lock index 1c9da7a72..dbb590a64 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6196,6 +6196,7 @@ __metadata: "@types/is-ci": "npm:^3.0.0" "@types/minimist": "npm:^1.2.5" "@types/node": "npm:~20.11" + "@types/proper-lockfile": "npm:^4.1.4" "@types/tmp": "npm:^0.2.6" "@typescript-eslint/utils": "npm:^5.59.0" ast-types: "npm:^0.14.2" @@ -6210,6 +6211,7 @@ __metadata: lazyrepo: "npm:0.0.0-alpha.27" minimist: "npm:^1.2.8" prettier: "npm:^3.0.3" + proper-lockfile: "npm:^4.1.2" recast: "npm:^0.22.0" rimraf: "npm:^4.4.0" semver: "npm:^7.3.8" @@ -6936,6 +6938,15 @@ __metadata: languageName: node linkType: hard +"@types/proper-lockfile@npm:^4.1.4": + version: 4.1.4 + resolution: "@types/proper-lockfile@npm:4.1.4" + dependencies: + "@types/retry": "npm:*" + checksum: b0d1b8e84a563b2c5f869f7ff7542b1d83dec03d1c9d980847cbb189865f44b4a854673cdde59767e41bcb8c31932e613ac43822d358a6f8eede6b79ccfceb1d + languageName: node + linkType: hard + "@types/qrcode@npm:^1.5.0": version: 1.5.5 resolution: "@types/qrcode@npm:1.5.5" @@ -7004,6 +7015,13 @@ __metadata: languageName: node linkType: hard +"@types/retry@npm:*": + version: 0.12.5 + resolution: "@types/retry@npm:0.12.5" + checksum: 3fb6bf91835ca0eb2987567d6977585235a7567f8aeb38b34a8bb7bbee57ac050ed6f04b9998cda29701b8c893f5dfe315869bc54ac17e536c9235637fe351a2 + languageName: node + linkType: hard + "@types/scheduler@npm:*": version: 0.16.8 resolution: "@types/scheduler@npm:0.16.8" @@ -19011,6 +19029,17 @@ __metadata: languageName: node linkType: hard +"proper-lockfile@npm:^4.1.2": + version: 4.1.2 + resolution: "proper-lockfile@npm:4.1.2" + dependencies: + graceful-fs: "npm:^4.2.4" + retry: "npm:^0.12.0" + signal-exit: "npm:^3.0.2" + checksum: 000a4875f543f591872b36ca94531af8a6463ddb0174f41c0b004d19e231d7445268b422ff1ea595e43d238655c702250cd3d27f408e7b9d97b56f1533ba26bf + languageName: node + linkType: hard + "property-information@npm:^6.0.0": version: 6.4.0 resolution: "property-information@npm:6.4.0"