Conversation
|
PR template validation failedPlease fix the following issues by editing your PR description:
See CONTRIBUTING.md for the full contribution policy. |
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
Scope checkThis PR changes 509 lines across 1 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed PR #3870 as an RFC-only change. I checked AGENTS.md, rfcs/README.md, and the relevant implementation files (packages/core/src/plugins/sandbox/types.ts, packages/core/src/plugins/sandbox/runner-options.ts, packages/cloudflare/src/sandbox/runner.ts, packages/core/src/astro/middleware.ts, packages/cloudflare/src/worker.ts, packages/core/src/plugins/scheduler/node.ts, packages/plugin-types/src/routes.ts, and packages/plugin-types/src/manifest-schema.ts).
Headline: the RFC is in the wrong place and contains one inaccurate technical reference. The resource-limit defaults it quotes (50 ms CPU, 10 subrequests, 30 s wall time) do match the current ResourceLimits defaults in packages/core/src/plugins/sandbox/types.ts. The route request: { body: "text", maxBytes: ... } example matches the plugin route request schema. The fallback scheduler references (NodeCronScheduler on Node, scheduled() on Cloudflare) exist. The proposed table name _emdash_plugin_jobs follows the plural system-table convention used elsewhere.
Two things need correction before this should land:
- The file must be moved from the repo root to
rfcs/0003-plugin-jobs.mdperrfcs/README.md. - The helper named as the source of default limits is misidentified; it lives in a different file and does not itself apply defaults.
I’m leaving a comment rather than requesting changes only because the PR introduces no code/build regression, but both findings are real process/accuracy issues that should be resolved.
| @@ -0,0 +1,509 @@ | |||
| --- | |||
There was a problem hiding this comment.
[needs fixing] The RFC is added at the repository root (0003-plugin-jobs.md), but rfcs/README.md requires every RFC to be a new file in rfcs/ named NNNN-short-slug.md. Merging it here would leave the RFC outside the tracked RFC directory and break the numbering/project conventions the repo enforces.
| --- | |
| Move/add the file as `rfcs/0003-plugin-jobs.md` instead of `0003-plugin-jobs.md` at the repo root. |
| | Subrequests | 10 | Worker Loader, documented in `WorkerLoaderLimits` as "fetch/service-binding calls" | | ||
| | Wall time | 30 s | runner `Promise.race` | | ||
|
|
||
| The host constructs the runner without passing limits (`createSandboxRunnerOptions` in `emdash-runtime.ts`), so every sandboxed plugin gets these defaults. Each `ctx.storage`, `ctx.kv` and settings call crosses the `PluginBridge` loopback service binding. Cloudflare's Service Bindings documentation states that "each request to a Worker via a Service binding counts toward your subrequest limit", and that "a single request has a maximum of 32 Worker invocations, and each call to a Service binding counts towards this limit." |
There was a problem hiding this comment.
[needs fixing] This line says the host calls createSandboxRunnerOptions in emdash-runtime.ts to construct the runner and that this is where the default limits originate. In the current codebase, createSandboxRunnerOptions is defined in packages/core/src/plugins/sandbox/runner-options.ts (it is only imported and called from packages/core/src/emdash-runtime.ts). Moreover, that helper only normalizes siteInfo; it does not supply the default ResourceLimits. The defaults are applied by the platform-specific runner, e.g. DEFAULT_LIMITS in packages/cloudflare/src/sandbox/runner.ts.
| The host constructs the runner without passing limits (`createSandboxRunnerOptions` in `emdash-runtime.ts`), so every sandboxed plugin gets these defaults. Each `ctx.storage`, `ctx.kv` and settings call crosses the `PluginBridge` loopback service binding. Cloudflare's Service Bindings documentation states that "each request to a Worker via a Service binding counts toward your subrequest limit", and that "a single request has a maximum of 32 Worker invocations, and each call to a Service binding counts towards this limit." | |
| The host constructs the runner without passing limits (`createSandboxRunnerOptions` in `packages/core/src/plugins/sandbox/runner-options.ts`), so every sandboxed plugin gets the defaults supplied by the platform runner (e.g. `DEFAULT_LIMITS` in `packages/cloudflare/src/sandbox/runner.ts`). |
1 participant
What does this PR do?
Closes #
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output