startDevWorker Milestone 1 - Reboot #4175

RamIdeas · 2023-10-13T13:38:02Z

This is the PR to reintroduce #3960

TODO:

🐛 BUG: ProxyWorker request logs #4168
🐛 BUG: workerd cpp errors when reloading workerd#1366
~~🐛 BUG: EBUSY: resource busy or locked, rmdir #4167~~
~~🐛 BUG: dyld: Symbol not found: _MTLCommonCounterClipperInvocations #4166~~
🐛 BUG: wrangler dev --remote no longer logs? #4165
bug: dev registry contains UserWorker address, not ProxyWorker

What this PR solves / how to test:

Please use the prerelease builds in the bot comment below to test: wrangler dev, wrangler dev --remote, unstable_dev() in local and remote mode, on your setup and post your issues in comments below

Associated docs issue(s)/PR(s):

[insert associated docs issue(s)/PR(s)]

Author has included the following, where applicable:

Tests
Changeset (Changeset guidelines)

Reviewer is to perform the following, as applicable:

Checked for inclusion of relevant tests
Checked for inclusion of a relevant changeset
Checked for creation of associated docs updates
Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

changeset-bot · 2023-10-13T13:38:06Z

⚠️ No Changeset found

Latest commit: b55a13a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2023-10-13T13:40:10Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6720209255/npm-package-wrangler-4175

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6720209255/npm-package-wrangler-4175

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6720209255/npm-package-wrangler-4175 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6720209255/npm-package-cloudflare-pages-shared-4175

Note that these links will no longer work once the GitHub Actions artifact expires.

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	3.20231025.1	3.20231025.1
`workerd`	1.20231025.0	1.20231025.0
`workerd --version`	1.20231025.0	2023-10-25

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

codecov · 2023-10-13T13:44:59Z

Codecov Report

Merging #4175 (83af6d8) into main (e4aff81) will increase coverage by 0.26%.
Report is 265 commits behind head on main.
The diff coverage is 51.61%.

❗ Current head 83af6d8 differs from pull request most recent head b55a13a. Consider uploading reports for the commit b55a13a to get more accurate results

@@            Coverage Diff             @@
##             main    #4175      +/-   ##
==========================================
+ Coverage   75.38%   75.65%   +0.26%     
==========================================
  Files         223      237      +14     
  Lines       12327    12507     +180     
  Branches     3187     3198      +11     
==========================================
+ Hits         9293     9462     +169     
- Misses       3034     3045      +11

Files	Coverage Δ
packages/wrangler/src/api/index.ts	`100.00% <100.00%> (ø)`
.../wrangler/src/api/startDevWorker/BaseController.ts	`100.00% <100.00%> (ø)`
...gler/src/api/startDevWorker/NotImplementedError.ts	`100.00% <100.00%> (ø)`
...ckages/wrangler/src/api/startDevWorker/devtools.ts	`100.00% <100.00%> (ø)`
packages/wrangler/src/api/startDevWorker/types.ts	`100.00% <100.00%> (ø)`
packages/wrangler/src/dev.tsx	`83.53% <ø> (-0.21%)`	⬇️
packages/wrangler/src/dev/miniflare.ts	`66.86% <100.00%> (+6.02%)`	⬆️
packages/wrangler/src/https-options.ts	`100.00% <100.00%> (ø)`
packages/wrangler/src/api/dev.ts	`82.60% <75.00%> (ø)`
packages/wrangler/src/api/startDevWorker/index.ts	`50.00% <50.00%> (ø)`
... and 13 more

... and 25 files with indirect coverage changes

penalosa · 2023-10-17T11:09:18Z

@RamIdeas would it be possible to separate out the individual bug fixes into separate PRs?

RamIdeas · 2023-10-18T20:54:30Z

@RamIdeas would it be possible to separate out the individual bug fixes into separate PRs?

@penalosa the 2 commits to fix 2 of the bugs were quite tiny and encapsulated in individual commits. I think reviewing each commit will be easy enough

If there are bigger changes, I will absolutely open a separate PR 👍

Cyb3r-Jak3 · 2023-10-20T21:30:06Z

I tested this in my project and seeing warning released to Your worker created multiple branches of a single stream... with this version workflow (in the test stage) whereas the latest wrangler dev didn't have those errors workflow.

When testing locally on Windows 10 I also get the following additional messages to stderr

stderr | src/git.test.ts > Git Endpoints > Git Repos
workerd/jsg/util.c++:275: error: e = workerd/util/sqlite.c++:894: failed: SQLite failed; sqlite3_errmsg(db) = cannot commit transaction - SQL statements in progress;  sqlite3_step()
stack: 7ff60e46d2af 7ff60e802419 7ff60e8ada63 0; sentryErrorContext = jsgInternalError
workerd/jsg/util.c++:275: error: e = workerd/util/sqlite.c++:894: failed: SQLite failed; sqlite3_errmsg(db) = cannot commit transaction - SQL statements in progress;  sqlite3_step()
stack: 7ff60e46d2af 7ff60e802419 7ff60e8ada63 0; sentryErrorContext = jsgInternalError

stderr | src/git.test.ts > Git Endpoints > Git User
workerd/jsg/util.c++:275: error: e = kj/filesystem-disk-win32.c++:249: failed: DeleteFileW(path.begin()): cloudflare/workers-sdk#5 Access is denied.; dbgStr(path) = \\?\F:\git\workers\severless-api\.wrangler\state\v3\kv\a3030e4701ed406ba337ca8bd9108dda\blobs\3c82b7cd5bfbf427963c8519cc2107c742efd2080a295d5fee70ef81031353690000022f9a055c8c
stack: 7ff60e46d2af 7ff60e498d9a 7ff60e7748ef 0 0; sentryErrorContext = jsgInternalError

jadbox · 2023-10-24T20:48:21Z

Does this help with supporting wranger pages dev --remote?

This reverts commit 88f15f6.

unless log level is debug change ProxyControllerLogger info override to log override change WranglerLog info override to log override -- this worksaround an internal Miniflare change

remote inspector websocket upgrade request required auth headers so use `fetch` with `Upgrade: websocket` header instead of `new WebSocket`

instead of UserWorker

This reverts commit 1e6879b. This commit will be reapplied in the Milestone 2 PR.

for (Inspector)ProxyWorker

upon UserWorker restarts

previously, sequential cleanups fail to fully cleanup if earlier steps in the sequence fail

remove assertion proxyWorker miniflare instance is different

https://jira.cfdata.org/browse/DEVX-1016

across all packages/fixtures

to force different port across reloads to workaround workerd bug on Windows

RamIdeas · 2023-11-22T14:50:29Z

Hi @Cyb3r-Jak3! Thanks for your feedback and apologies for the delay. We used your test suite to analyse the stability of these changes. Github got a bit confused with this PR and so I closed this and opened a new PR #4413 – could you try the prerelease build from there and let us know how it goes please?

RamIdeas · 2023-11-22T14:51:59Z

@jadbox although we are not focusing on pages dev directly, wrangler pages dev uses unstable_dev under-the-hood and therefore will benefit from the changes and stability improvements indirectly

RamIdeas requested a review from a team as a code owner October 13, 2023 13:38

RamIdeas force-pushed the startDevWorker-milestone-1-reboot branch from bed083f to cdfb63c Compare October 17, 2023 12:59

Cherry mentioned this pull request Oct 17, 2023

🐛 BUG: EBUSY: resource busy or locked, rmdir #4167

Closed

This was referenced Oct 18, 2023

🐛 BUG: ProxyWorker request logs #4168

Closed

🐛 BUG: wrangler dev --remote no longer logs? #4165

Closed

RamIdeas force-pushed the startDevWorker-milestone-1-reboot branch from cdfb63c to 83af6d8 Compare October 18, 2023 21:36

Cyb3r-Jak3 added a commit to Cyb3r-Jak3/workers-serverless-api that referenced this pull request Oct 20, 2023

Use latest cloudflare/workers-sdk#4175

d41c87c

lrapoport-cf added the awaiting Cloudflare response Awaiting response from workers-sdk maintainer team label Oct 23, 2023

RamIdeas force-pushed the startDevWorker-milestone-1-reboot branch 2 times, most recently from 7ee52ca to f698350 Compare October 25, 2023 16:32

RamIdeas closed this Oct 26, 2023

RamIdeas reopened this Oct 26, 2023

RamIdeas added 11 commits October 30, 2023 12:38

Revert "Revert "startDevWorker - Milestone 1" (#4171)"

8f1590d

This reverts commit 88f15f6.

fix: don't show logs to ProxyWorker(s)

61b3cbb

unless log level is debug change ProxyControllerLogger info override to log override change WranglerLog info override to log override -- this worksaround an internal Miniflare change

fix: show console.log's in remote mode

b1b6544

remote inspector websocket upgrade request required auth headers so use `fetch` with `Upgrade: websocket` header instead of `new WebSocket`

use miniflare verbose mode only if debug log level

d34fc4c

register ProxyWorker with DevRegistry

cd9be39

instead of UserWorker

Revert "register ProxyWorker with DevRegistry"

e106aa6

This reverts commit 1e6879b. This commit will be reapplied in the Milestone 2 PR.

use single Miniflare instance

b272636

for (Inspector)ProxyWorker

port: clear remote runtime logs

6ce1f92

upon UserWorker restarts

default unstable_dev inspectorPort to 0

f2c8c97

parallelise cleanup to minimise chance of hanging

6749fba

previously, sequential cleanups fail to fully cleanup if earlier steps in the sequence fail

ensure InspectorProxyWorker unsafeDirectPort is set

8276138

RamIdeas force-pushed the startDevWorker-milestone-1-reboot branch from c1b209c to 2bf4d46 Compare October 31, 2023 15:35

RamIdeas added 2 commits October 31, 2023 15:45

temp: more test debugging

e8836d9

remove assertion proxyWorker miniflare instance is different

temp: more test debugging

7cf9069

RamIdeas force-pushed the startDevWorker-milestone-1-reboot branch from 55fc364 to 7cf9069 Compare October 31, 2023 16:04

RamIdeas added 3 commits October 31, 2023 16:08

temp: more test debugging

bc11f06

await setOptionsPromise before accessing mf.ready promise

10b73b7

workaround for Miniflare bug setOptions/ready

9cc5516

https://jira.cfdata.org/browse/DEVX-1016

RamIdeas force-pushed the startDevWorker-milestone-1-reboot branch from 436a320 to 9cc5516 Compare October 31, 2023 17:46

penalosa and others added 3 commits October 31, 2023 18:23

Add logging

e2544c6

remove miniflare workaround for parallel requests

db2cf68

ensure miniflare version is consistent

eecccd7

across all packages/fixtures

RamIdeas requested a review from a team as a code owner November 1, 2023 12:40

RamIdeas added 2 commits November 1, 2023 12:49

apply miniflare patch for Mutex#drained bug

c61bdb4

considerations for race between control messages and user fetches

2909265

RamIdeas closed this Nov 2, 2023

RamIdeas reopened this Nov 2, 2023

use port: undefined vs 0 for UserWorker

6b99cab

to force different port across reloads to workaround workerd bug on Windows

RamIdeas force-pushed the startDevWorker-milestone-1-reboot branch from eae8bec to 6b99cab Compare November 2, 2023 15:50

Don't try to parse node internals

42fcf89

RamIdeas closed this Nov 2, 2023

RamIdeas reopened this Nov 2, 2023

improve InspectorProxyWorker debug logs

b55a13a

RamIdeas closed this Nov 3, 2023

This was referenced Nov 3, 2023

[WIP] Implement dev registry as background daemon #4241

Closed

Rename unstable_dev() to dev() #2471

Closed

RamIdeas removed the awaiting Cloudflare response Awaiting response from workers-sdk maintainer team label Nov 22, 2023

lrapoport-cf linked an issue Jan 10, 2024 that may be closed by this pull request

🚀 Stabilizing unstable_dev and expanding Wrangler as an API #4730

Closed

lrapoport-cf added this to the Wrangler as a Library milestone Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

startDevWorker Milestone 1 - Reboot #4175

startDevWorker Milestone 1 - Reboot #4175

RamIdeas commented Oct 13, 2023 •

edited

Loading

changeset-bot bot commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading

penalosa commented Oct 17, 2023

RamIdeas commented Oct 18, 2023

Cyb3r-Jak3 commented Oct 20, 2023

jadbox commented Oct 24, 2023 •

edited

Loading

RamIdeas commented Nov 22, 2023

RamIdeas commented Nov 22, 2023

startDevWorker Milestone 1 - Reboot #4175

startDevWorker Milestone 1 - Reboot #4175

Conversation

RamIdeas commented Oct 13, 2023 • edited Loading

changeset-bot bot commented Oct 13, 2023 • edited Loading

⚠️ No Changeset found

github-actions bot commented Oct 13, 2023 • edited Loading

codecov bot commented Oct 13, 2023 • edited Loading

Codecov Report

penalosa commented Oct 17, 2023

RamIdeas commented Oct 18, 2023

Cyb3r-Jak3 commented Oct 20, 2023

jadbox commented Oct 24, 2023 • edited Loading

RamIdeas commented Nov 22, 2023

RamIdeas commented Nov 22, 2023

RamIdeas commented Oct 13, 2023 •

edited

Loading

changeset-bot bot commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading

jadbox commented Oct 24, 2023 •

edited

Loading