Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(lambda): lambda functions that use triggers error when invoked #23728

Merged
merged 14 commits into from Jan 20, 2023

Conversation

comcalvi
Copy link
Contributor

@comcalvi comcalvi commented Jan 18, 2023

Reverts #23062.

#23062 introduced #23407, which causes lambda functions that use triggers to fail to invoke with either this error

submit response to cloudformation {
  Status: 'FAILED',
  Reason: `TypeError [ERR_INVALID_ARG_TYPE]: The "msecs" argument must be of type number. Received type string ('120000')\n` +
    '    at new NodeError (internal/errors.js:322:7)\n' +
    '    at validateNumber (internal/validators.js:129:11)\n' +
    '    at getTimerDuration (internal/timers.js:384:3)\n' +
    '    at ClientRequest.setTimeout (_http_client.js:865:11)\n' +
    '    at features.constructor.handleRequest (/var/runtime/node_modules/aws-sdk/lib/http/node.js:82:12)\n' +
    '    at executeSend (/var/runtime/node_modules/aws-sdk/lib/event_listeners.js:370:29)\n' +
    '    at Request.SEND (/var/runtime/node_modules/aws-sdk/lib/event_listeners.js:384:9)\n' +
    '    at Request.callListeners (/var/runtime/node_modules/aws-sdk/lib/sequential_executor.js:102:18)\n' +
    '    at Request.emit (/var/runtime/node_modules/aws-sdk/lib/sequential_executor.js:78:10)\n' +
    '    at Request.emit (/var/runtime/node_modules/aws-sdk/lib/request.js:686:14)',
  StackId: 'arn:aws:cloudformation:us-east-1:***:stack/TestStack/83f77790-806c-11ed-8956-0a55d38b49ed',
  RequestId: '86a74312-347d-40c4-873a-09eed5b8eddd',
  PhysicalResourceId: 'AWSCDK::CustomResourceProviderFramework::CREATE_FAILED',
  LogicalResourceId: 'Trigger',
  NoEcho: undefined,
  Data: undefined
}

or

Error: Trigger handler failed with status code 202 at handler (/var/task/index.js:53:15) at processTicksAndRejections (internal/process/task_queues.js:95:5) at async Runtime.handler (/var/task/__entrypoint__.js:32:24) (RequestId: ab252a7a-e06b-4fcf-b2b4-f59b2dd88734)

Reverting for now, since people are unable to upgrade.

To unblock this revert, I'm disabling the integration test dependencies-pnpm, because when running it locally (and in this PR build) the logical ID of the version changes every run. This has been reproduced by others locally, so it's being disabled until we can resolve that issue.

Fixes #23407

@gitpod-io
Copy link

gitpod-io bot commented Jan 18, 2023

@github-actions github-actions bot added the p2 label Jan 18, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team January 18, 2023 02:58
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 18, 2023
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 18, 2023

@aws-cdk/aws-lambda-nodejs:   CHANGED    integ.dependencies-pnpm 47.892s
@aws-cdk/aws-lambda-nodejs:       Resources
@aws-cdk/aws-lambda-nodejs: [~] Custom::Trigger Trigger 
@aws-cdk/aws-lambda-nodejs:  ├─ [-] InvocationType
@aws-cdk/aws-lambda-nodejs:  │   └─ RequestResponse
@aws-cdk/aws-lambda-nodejs:  └─ [-] Timeout
@aws-cdk/aws-lambda-nodejs:      └─ 120000

@comcalvi comcalvi force-pushed the revert-23062-feature/notriggertimeout branch from 392a78a to b789217 Compare January 19, 2023 19:13
@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort labels Jan 20, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 20, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: f77e142
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 37974ed into main Jan 20, 2023
@mergify mergify bot deleted the revert-23062-feature/notriggertimeout branch January 20, 2023 01:56
@mergify
Copy link
Contributor

mergify bot commented Jan 20, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

comcalvi added a commit that referenced this pull request Jan 20, 2023
…23728)

Reverts #23062.

#23062 introduced #23407, which causes lambda functions that use triggers to fail to invoke with either this error

```
submit response to cloudformation {
  Status: 'FAILED',
  Reason: `TypeError [ERR_INVALID_ARG_TYPE]: The "msecs" argument must be of type number. Received type string ('120000')\n` +
    '    at new NodeError (internal/errors.js:322:7)\n' +
    '    at validateNumber (internal/validators.js:129:11)\n' +
    '    at getTimerDuration (internal/timers.js:384:3)\n' +
    '    at ClientRequest.setTimeout (_http_client.js:865:11)\n' +
    '    at features.constructor.handleRequest (/var/runtime/node_modules/aws-sdk/lib/http/node.js:82:12)\n' +
    '    at executeSend (/var/runtime/node_modules/aws-sdk/lib/event_listeners.js:370:29)\n' +
    '    at Request.SEND (/var/runtime/node_modules/aws-sdk/lib/event_listeners.js:384:9)\n' +
    '    at Request.callListeners (/var/runtime/node_modules/aws-sdk/lib/sequential_executor.js:102:18)\n' +
    '    at Request.emit (/var/runtime/node_modules/aws-sdk/lib/sequential_executor.js:78:10)\n' +
    '    at Request.emit (/var/runtime/node_modules/aws-sdk/lib/request.js:686:14)',
  StackId: 'arn:aws:cloudformation:us-east-1:***:stack/TestStack/83f77790-806c-11ed-8956-0a55d38b49ed',
  RequestId: '86a74312-347d-40c4-873a-09eed5b8eddd',
  PhysicalResourceId: 'AWSCDK::CustomResourceProviderFramework::CREATE_FAILED',
  LogicalResourceId: 'Trigger',
  NoEcho: undefined,
  Data: undefined
}
```

or

```
Error: Trigger handler failed with status code 202 at handler (/var/task/index.js:53:15) at processTicksAndRejections (internal/process/task_queues.js:95:5) at async Runtime.handler (/var/task/__entrypoint__.js:32:24) (RequestId: ab252a7a-e06b-4fcf-b2b4-f59b2dd88734)
```

Reverting for now, since people are unable to upgrade. 

To unblock this revert, I'm disabling the integration test `dependencies-pnpm`, because when running it locally (and in this PR build) the logical ID of the version changes every run. This has been reproduced by others locally, so it's being disabled until we can resolve that issue.

Fixes #23407
mergify bot pushed a commit that referenced this pull request Jan 20, 2023
The integ-runner obtains information on how to run a test from the `integ.json` manifest file in the cloud assembly. In order to get this information for a new version of the test it first synthesizes the new cloud assembly _only for the purpose of loading the integ manifest_. It will then run the actual test which synthesizes again into the same cloud assembly directory.

We need to cleanup that first temporary cloud assembly _before_ running the actual test to make sure we remove any leftover files. Here is one example of what might happen.

1. Do the temporary synthesis and create the cloud assembly at `cdk-integ.out.my-test.js/`. This assembly has an asset with some hash `asset.abcdefg`.
2. Do the synthesis while executing the test. The cloud assembly at `cdk-integ.out.my-test.js` is updated. This time the asset hash is different `asset.123456`. Now we have two assets in the cloud assembly and one `asset.abcdefg` is not being used!
3. Test completes and copies the temporary snapshot to the final snapshot directory, including the old asset

I've also added back the `lambda-nodejs/integ.dependencies-pnpm.ts` integration test that was removed in #23728. With this fix the test will no longer check in the asset file. I also removed the Trigger from the test since that is what introduced the always changing diff.


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p2
Projects
None yet
4 participants