fix: Register worker functions before system access control#26945
Merged
kevintang2022 merged 1 commit intoprestodb:masterfrom Jan 12, 2026
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideMoves registration of built-in sidecar worker functions earlier in PrestoServer startup so functions are available before system access control is loaded and the server starts taking traffic, eliminating a race where queries can arrive before function registration completes. Sequence diagram for updated PrestoServer startup function registrationsequenceDiagram
participant PrestoServer
participant Injector
participant FeaturesConfig
participant WorkerFunctionRegistryTool
participant FunctionAndTypeManager
participant AccessControlManager
PrestoServer->>Injector: getInstance FeaturesConfig
Injector-->>PrestoServer: FeaturesConfig
PrestoServer->>FeaturesConfig: isBuiltInSidecarFunctionsEnabled
alt builtInSidecarFunctionsEnabled
PrestoServer->>Injector: getInstance WorkerFunctionRegistryTool
Injector-->>PrestoServer: WorkerFunctionRegistryTool
PrestoServer->>WorkerFunctionRegistryTool: getWorkerFunctions
WorkerFunctionRegistryTool-->>PrestoServer: List SqlFunction
PrestoServer->>Injector: getInstance FunctionAndTypeManager
Injector-->>PrestoServer: FunctionAndTypeManager
PrestoServer->>FunctionAndTypeManager: registerWorkerFunctions functions
end
PrestoServer->>Injector: getInstance AccessControlManager
Injector-->>PrestoServer: AccessControlManager
PrestoServer->>AccessControlManager: loadSystemAccessControl
PrestoServer->>PrestoServer: continue startup and begin taking traffic
Flow diagram for updated PrestoServer startup ordergraph TD
A[PrestoServer run start] --> B[Create Injector and core services]
B --> C[Load type managers]
C --> D[Load session property configuration]
D --> E[Load resource group configuration]
E --> F{builtInSidecarFunctionsEnabled}
F -- Yes --> G[Get WorkerFunctionRegistryTool]
G --> H[Get worker functions List SqlFunction]
H --> I[Get FunctionAndTypeManager]
I --> J[Register worker functions]
F -- No --> K[Skip worker function registration]
J --> L{isResourceManager}
K --> L
L -- No --> M[Load system access control]
L -- Yes --> N[Skip system access control load]
M --> O[Load remaining managers and filters]
N --> O
O --> P[Server ready and begins taking traffic]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider adding a short comment around the new worker function registration block explaining why it must occur before
loadSystemAccessControl()to prevent future refactors from reintroducing the race condition. - You can slightly improve readability and avoid repeated injector calls by caching
FeaturesConfig,WorkerFunctionRegistryTool, andFunctionAndTypeManagerin local variables before using them in the new block.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a short comment around the new worker function registration block explaining why it must occur before `loadSystemAccessControl()` to prevent future refactors from reintroducing the race condition.
- You can slightly improve readability and avoid repeated injector calls by caching `FeaturesConfig`, `WorkerFunctionRegistryTool`, and `FunctionAndTypeManager` in local variables before using them in the new block.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
feilong-liu
approved these changes
Jan 12, 2026
This was referenced Mar 31, 2026
15 tasks
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.
Description
Register the functions earlier so that they are available before the Presto server begins taking traffic.
Motivation and Context
There is a race condition where a query is submitted before all functions are fully registered, so there might be function not registered errors.
Impact
No impact on functionality
Test Plan
No impact, so just keep existing behaviors
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.