Conversation
| ], | ||
| }); | ||
|
|
||
| const resultServerlessFunction = await this.serverlessService.execute( | ||
| functionToExecute, | ||
| payload, | ||
| version, | ||
| ); | ||
| const resultServerlessFunction = await this.callWithTimeout({ | ||
| callback: () => | ||
| this.serverlessService.execute(functionToExecute, payload, version), | ||
| timeoutMs: DEFAULT_SERVERLESS_FUNCTION_TIMEOUT_MS, | ||
| }); | ||
|
|
||
| if (this.twentyConfigService.get('SERVERLESS_LOGS_ENABLED')) { | ||
| /* eslint-disable no-console */ |
There was a problem hiding this comment.
Bug: User-configured timeoutSeconds for serverless functions are ignored, always defaulting to 5 minutes.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The callWithTimeout function in serverless-function.service.ts uses a hardcoded DEFAULT_SERVERLESS_FUNCTION_TIMEOUT_MS (5 minutes) for all serverless function executions. This overrides and ignores the user-configured functionToExecute.timeoutSeconds value, which can range from 1 to 900 seconds. Consequently, functions configured with a timeout greater than 5 minutes will still be terminated after 5 minutes, and functions configured with a shorter timeout will run for the full 5 minutes before timing out.
💡 Suggested Fix
Modify serverless-function.service.ts to use functionToExecute.timeoutSeconds * 1_000 instead of DEFAULT_SERVERLESS_FUNCTION_TIMEOUT_MS in the callWithTimeout function.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-server/src/engine/metadata-modules/serverless-function/serverless-function.service.ts#L103-L113
Potential issue: The `callWithTimeout` function in `serverless-function.service.ts` uses
a hardcoded `DEFAULT_SERVERLESS_FUNCTION_TIMEOUT_MS` (5 minutes) for all serverless
function executions. This overrides and ignores the user-configured
`functionToExecute.timeoutSeconds` value, which can range from 1 to 900 seconds.
Consequently, functions configured with a timeout greater than 5 minutes will still be
terminated after 5 minutes, and functions configured with a shorter timeout will run for
the full 5 minutes before timing out.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR moves timeout enforcement from the Lambda driver to the service layer to ensure consistent timeout behavior.
Key Changes:
- Lambda timeout now hardcoded to 900s (AWS maximum) in
lambda.driver.ts - New
callWithTimeouthelper wraps serverless function execution with 5-minute timeout - New constant
DEFAULT_SERVERLESS_FUNCTION_TIMEOUT_MSdefines the service-level timeout
Issues Found:
- Memory leak in
callWithTimeout- timeout not cleared when callback completes successfully - Creates architectural inconsistency: local driver still uses
serverlessFunction.timeoutSecondsdirectly, while Lambda ignores it and relies on service-level timeout
Confidence Score: 2/5
- PR has a memory leak that will accumulate with each serverless function execution
- The
callWithTimeoutimplementation doesn't clear the timeout when the callback completes successfully, causing timer handles to accumulate in memory. This will lead to memory growth over time as functions are executed. - packages/twenty-server/src/engine/metadata-modules/serverless-function/serverless-function.service.ts needs immediate fix for timeout cleanup
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/twenty-server/src/engine/metadata-modules/serverless-function/serverless-function.service.ts | 2/5 | Added timeout wrapper with memory leak issue - timeout not cleared when callback completes |
| packages/twenty-server/src/engine/core-modules/serverless/drivers/lambda.driver.ts | 4/5 | Hardcoded Lambda timeout to 900s (max), moved timeout enforcement to service layer |
| packages/twenty-server/src/engine/metadata-modules/serverless-function/constants/default-serverless-function-timeout-ms.constant.ts | 5/5 | New constant file defining 5-minute default timeout |
Sequence Diagram
sequenceDiagram
participant Client
participant ServerlessFunctionService
participant ServerlessService
participant LambdaDriver
participant AWS Lambda
Client->>ServerlessFunctionService: executeOneServerlessFunction(id, payload)
ServerlessFunctionService->>ServerlessFunctionService: throttleExecution()
ServerlessFunctionService->>ServerlessFunctionService: callWithTimeout (5 min)
Note over ServerlessFunctionService: Starts 5-minute timeout timer
ServerlessFunctionService->>ServerlessService: execute(function, payload, version)
ServerlessService->>LambdaDriver: execute(serverlessFunction, payload, version)
LambdaDriver->>LambdaDriver: build & waitFunctionUpdates
LambdaDriver->>AWS Lambda: invoke (900s timeout)
AWS Lambda-->>LambdaDriver: result
LambdaDriver-->>ServerlessService: result
ServerlessService-->>ServerlessFunctionService: result
Note over ServerlessFunctionService: ⚠️ Timeout NOT cleared - memory leak
ServerlessFunctionService-->>Client: result
3 files reviewed, 1 comment
...twenty-server/src/engine/metadata-modules/serverless-function/serverless-function.service.ts
Outdated
Show resolved
Hide resolved
martmull
left a comment
There was a problem hiding this comment.
You need to update the local.driver also
Please add a test to verify the error message complies with what is already existing
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:45320 This environment will automatically shut down when the PR is closed or after 5 hours. |
packages/twenty-server/src/engine/core-modules/serverless/drivers/local.driver.ts
Outdated
Show resolved
Hide resolved
...twenty-server/src/engine/metadata-modules/serverless-function/serverless-function.service.ts
Outdated
Show resolved
Hide resolved
...ata-modules/serverless-function/constants/default-serverless-function-timeout-ms.constant.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const timeoutPromise = new Promise<never>((_, reject) => { | ||
| timeoutId = setTimeout( | ||
| () => reject(new Error(`Execution timeout: ${timeoutMs / 1000}s`)), |
There was a problem hiding this comment.
| () => reject(new Error(`Execution timeout: ${timeoutMs / 1000}s`)), | |
| () => reject(new ServerlessFunctionException(`Execution timeout: ${timeoutMs / 1000}s`), SERVERLESS_FUNCTION_TIMEOUT), |
28adf67 to
e2845e6
Compare
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
Some functions keep running without a timeout being thrown. Doing it in service directly.