Uh oh!
There was an error while loading. Please reload this page.
feat(cli): inject function slug into served fns - #6345
Conversation
e264980 to
2989c69CompareThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:e264980dc6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@abc0a94e32199dde2ba3f608a1fb92d941fdbe5fPreview package for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:961e6ac204
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
961e6ac to
c1103deCompareThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:c1103de0cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
c1103de to
954c972CompareThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:c24a039af9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bddf73e to
cbc9aecCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
## I have read the [CONTRIBUTING.md](https://github.com/supabase/supabase/blob/master/CONTRIBUTING.md) file. YES ## What kind of change does this PR introduce? Feature (self-hosted Edge Functions) ## What is the current behavior? The self-hosted Edge Functions router (`docker/volumes/functions/main/index.ts`) doesn't tell a function which slug a request resolved to. As a result, `@supabase/server`'s `withOAuthProtectedResource` can't derive its canonical resource URL and falls back to reconstructing it from the request path against the internal `api-gw` origin, so the advertised OAuth Protected Resource is /wrong for self-hosted deployments. ## What is the new behavior? `main/index.ts` now injects `SUPABASE_FUNCTION_SLUG: service_name` per request (after the `Deno.env.toObject()` snapshot, so nothing in the container env can shadow it). Combined with the operator's `SUPABASE_PUBLIC_URL`, the advertised resource is the correct external `{SUPABASE_PUBLIC_URL}/functions/v1/{slug}`, not the internal `http://api-gw:8000`. Verified on the docker stack: the slug is injected per-function, the resource origin resolves to `SUPABASE_PUBLIC_URL`, and the `401` `www-authenticate` carries the right `resource_metadata`. ## Additional context Fixes AI-1128 Companion to `@supabase/server` [PR #117](supabase/server#117) and the [CLI slug injection](supabase/cli#6345) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Edge workers now receive the correct function slug in their runtime environment, improving per-function request handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
43b08e3 to
91f7dfeCompareThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:91f7dfed2c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
91f7dfe to
91e1cd3Compare91e1cd3 to
abc0a94CompareThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:abc0a94e32
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } finally { | ||
| releaseSharedWorkerSlot?.(); |
There was a problem hiding this comment.
Hold the worker slot until the request completes
When requests for two function names sharing a servicePath overlap, this releases the slot immediately after worker creation, before worker.fetch() completes. The second request can therefore acquire the slot and force-replace the sole cached worker while the first request is still executing, potentially cancelling the first request or producing a worker error; the duplicate flow in packages/stack/src/services/edge-runtime-main.ts has the same gap. Keep the critical section through request completion or give each slug an independent worker identity. The fresh evidence beyond the earlier creation-race comment is this new finally explicitly releasing the slot before the fetch.
AGENTS.md reference: AGENTS.md:L142-L148
Useful? React with 👍 / 👎.
| await previousSlot; | ||
| releaseSharedWorkerSlot = releaseSlot!; | ||
| forceCreate = sharedWorkerOwners.get(servicePath) !== functionName; | ||
| sharedWorkerOwners.set(servicePath, functionName); |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
A caller can select a function whose worker fails during creation. This records the URL-selected function as the owner before the cache is replaced; a later request for that function skips forceCreate and may receive the previous function's worker, exposing its code, state, or environment under the wrong endpoint.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: The root cause is that sharedWorkerOwners.set(servicePath, functionName) is called at line 238 beforeEdgeRuntime.userWorkers.create() is awaited. If worker creation fails, the owner map is already poisoned: a subsequent request for that same functionName sees forceCreate = false and reuses a stale/wrong worker.
The fix requires two coordinated changes:
Remove line 238 (
sharedWorkerOwners.set(servicePath, functionName);) from its current location before worker creation.Move the
sharedWorkerOwners.set()call to immediately afteruserWorkers.create()succeeds (after line 260, the closing});of thecreatecall), but before thefinallyblock at line 261 that releases the queue slot. This ensures the owner map is only updated when the worker is actually live, while still holding the serialization lock so no concurrent request can race on the owner decision:
worker=awaitEdgeRuntime.userWorkers.create({ ... });// Only record ownership after the worker is confirmed running:sharedWorkerOwners.set(servicePath,functionName);}finally{releaseSharedWorkerSlot?.();}With this change, a failed create() leaves sharedWorkerOwners pointing at the previous owner (or unset), so the next request will correctly set forceCreate = true and attempt a fresh worker instead of reusing a cached one belonging to a different endpoint.
| ); | ||
| await previousSlot; | ||
| releaseSharedWorkerSlot = releaseSlot!; | ||
| forceCreate = sharedWorkerOwners.get(servicePath) !== functionName; |
There was a problem hiding this comment.
[P1] The owner map does not identify the worker Edge Runtime will reuse
forceCreate: true only skips the active-worker lookup; it does not retire workers already registered for this servicePath. After A creates worker A and B force-creates worker B, both remain in the same registry. A later B request reaches this line with forceCreate: false, and Edge Runtime can round-robin back to worker A, exposing A’s code/state/environment and SUPABASE_FUNCTION_SLUG under B. See maybe_active_worker, add_user_worker, and the registry’s round-robin selection. Alternating A/B requests also keep forcing new workers, so accumulation remains reachable. A last-owner map cannot provide a stable (servicePath, functionName) cache identity; that identity must exist in the worker pool/caller-owned worker cache, or shared paths need to be rejected until it does.
| await previousSlot; | ||
| releaseSharedWorkerSlot = releaseSlot!; | ||
| forceCreate = sharedWorkerOwners.get(servicePath) !== functionName; | ||
| sharedWorkerOwners.set(servicePath, functionName); |
There was a problem hiding this comment.
[P2] Do not record ownership before worker creation succeeds
If B’s forced userWorkers.create() fails, this map still records B while the only reusable worker may belong to A. The next B request therefore computes forceCreate: false and can reuse A’s worker. Ownership must only change after successful creation while coordination is still held. This fixes the failure-state corruption, although it does not address the separate P1 that multiple same-path workers remain registered and can be selected later.
| ); | ||
| await previousSlot; | ||
| releaseSharedWorkerSlot = releaseSlot!; | ||
| forceCreate = sharedWorkerOwners.get(servicePath) !== functionName; |
There was a problem hiding this comment.
[P1] Apply the stable worker-identity fix in the stack path too
This owner comparison duplicates the CLI serve-path bug: after multiple forced creations, Edge Runtime retains multiple active workers for this servicePath, so a later forceCreate: false call may round-robin to a worker with another function’s slug. See the full lifecycle trace in the corresponding CLI finding. Please keep both runtime paths aligned, with integration coverage for A → B → repeated B and alternating A/B requests.
| if (sharedServicePaths.has(servicePath)) { | ||
| const previousSlot = sharedWorkerQueues.get(servicePath) ?? Promise.resolve(); | ||
| let releaseSlot: () => void; | ||
| const currentSlot = new Promise<void>((resolve) => { |
There was a problem hiding this comment.
Blocking: this manual Promise gate currently fails the required quality check
@supabase/root#lint:effect:check rejects this new Promise(...) construction, which is why the PR’s Check code quality job is red. The stack runtime is Effect-scoped and its coordination must use the applicable Effect primitive rather than a Promise gate. Given the worker-identity P1 above, the coordination design should be corrected first instead of mechanically wrapping this gate just to satisfy lint.
Summary
Injects
SUPABASE_FUNCTION_SLUG(the function's own name, resolved per-request) into every locally served Edge Function, in both serve paths: legacy and next stacks.Context
@supabase/server'swithOAuthProtectedResourceuses this to advertise the canonical/functions/v1/{slug}OAuth Protected Resource URL instead of reconstructing it from the request path. See this pull-request.Other changes
Since Edge Runtime keeps one worker per folder rather than per function, two functions sharing the same folder ended up sharing a worker too, and that worker only had one slug, so the second function got the wrong one.
I fixed this without adding any new files or folders: the code now tracks which function owns each shared folder, so a request for a different function gets a fresh worker while the same function keeps reusing its old one. This keeps the fix small and leaves normal worker reuse untouched for every other function.
Linked issue
Fixes AI-1129
open-for-contributionlabel (or I'm a Supabase maintainer).Checklist
fix(cli): …).pnpm check:allpasses; relevant package tests pass for every touched workspace, andpnpm types:checkpasses for each touched TypeScript workspace (or workspace declaring it).