fix: update @alicloud/fc20230330 from 4.6.8 - 4.7.4、add session pau…#153
Open
liuzewen99 wants to merge 1 commit intomasterfrom
Open
fix: update @alicloud/fc20230330 from 4.6.8 - 4.7.4、add session pau…#153liuzewen99 wants to merge 1 commit intomasterfrom
liuzewen99 wants to merge 1 commit intomasterfrom
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds ChangesSession Lifecycle and Configuration Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/subCommands/session/index.ts (1)
244-257:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix validation logic that incorrectly rejects undefined timeout values.
The validation will throw an error even when
sessionTTLInSecondsorsessionIdleTimeoutInSecondsare intentionally omitted (set toundefinedat lines 229-234). The test at lines 614-629 expects this to work, but!_.isNumber(undefined)evaluates totrue, triggering the error.🐛 Proposed fix to validate only when values are provided
if ( + sessionTTLInSeconds !== undefined && ( !_.isNumber(sessionTTLInSeconds) || sessionTTLInSeconds < 0 || - sessionTTLInSeconds > 21600 + sessionTTLInSeconds > 21600) ) { throw new Error('sessionTTLInSeconds must be a number between 0 and 21600'); } if ( + sessionIdleTimeoutInSeconds !== undefined && ( !_.isNumber(sessionIdleTimeoutInSeconds) || sessionIdleTimeoutInSeconds < 0 || - sessionIdleTimeoutInSeconds > 21600 + sessionIdleTimeoutInSeconds > 21600) ) { throw new Error('sessionIdleTimeoutInSeconds must be a number between 0 and 21600'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/subCommands/session/index.ts` around lines 244 - 257, The current validation for sessionTTLInSeconds and sessionIdleTimeoutInSeconds rejects undefined values; update the checks in the session command to only run when a value is provided. Specifically, change the validation around sessionTTLInSeconds and sessionIdleTimeoutInSeconds so they first check for undefined (e.g., sessionTTLInSeconds !== undefined) and then assert _.isNumber(...) and range 0–21600; apply the same pattern for both sessionTTLInSeconds and sessionIdleTimeoutInSeconds in the function handling argument parsing/validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/resources/fc/index.ts`:
- Around line 1057-1106: Extract the duplicated filesystem construction logic
into a shared private helper (e.g., buildFilesystemConfigs or
constructFsConfigs) and call it from both createFunctionSession and
updateFunctionSession: move the mapping code that creates
NASMountConfig/NASConfig, OSSMountPoint/OSSMountConfig, and
PolarFsMountConfig/PolarFsConfig into that helper, accept the raw config object
(with mountPoints) and return the constructed typed config objects (nasConfig,
ossMountConfig, polarFsConfig) or undefined when empty; update both
createFunctionSession and updateFunctionSession to use this helper, preserving
types and the same field mappings (enableTLS, mountDir, serverAddr, bucketName,
bucketPath, endpoint, readOnly, instanceId, remoteDir, groupId, userId).
---
Outside diff comments:
In `@src/subCommands/session/index.ts`:
- Around line 244-257: The current validation for sessionTTLInSeconds and
sessionIdleTimeoutInSeconds rejects undefined values; update the checks in the
session command to only run when a value is provided. Specifically, change the
validation around sessionTTLInSeconds and sessionIdleTimeoutInSeconds so they
first check for undefined (e.g., sessionTTLInSeconds !== undefined) and then
assert _.isNumber(...) and range 0–21600; apply the same pattern for both
sessionTTLInSeconds and sessionIdleTimeoutInSeconds in the function handling
argument parsing/validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73a44f1e-954b-4448-bcd8-6f45823d6b45
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
__tests__/ut/commands/session_test.tspackage.jsonsrc/commands-help/session.tssrc/resources/fc/index.tssrc/subCommands/session/index.ts
Comment on lines
+1057
to
+1106
| let nasConfig: NASConfig | undefined; | ||
| if (config.nasConfig && !_.isEmpty(config.nasConfig.mountPoints)) { | ||
| const mountPoints = config.nasConfig.mountPoints.map( | ||
| (mountPoint: any) => | ||
| new NASMountConfig({ | ||
| enableTLS: mountPoint.enableTLS, | ||
| mountDir: mountPoint.mountDir, | ||
| serverAddr: mountPoint.serverAddr, | ||
| }), | ||
| ); | ||
| nasConfig = new NASConfig({ | ||
| groupId: config.nasConfig.groupId, | ||
| mountPoints, | ||
| userId: config.nasConfig.userId, | ||
| }); | ||
| } | ||
|
|
||
| let ossMountConfig: OSSMountConfig | undefined; | ||
| if (config.ossMountConfig && !_.isEmpty(config.ossMountConfig.mountPoints)) { | ||
| const mountPoints = config.ossMountConfig.mountPoints.map( | ||
| (mountPoint: any) => | ||
| new OSSMountPoint({ | ||
| bucketName: mountPoint.bucketName, | ||
| bucketPath: mountPoint.bucketPath, | ||
| endpoint: mountPoint.endpoint, | ||
| mountDir: mountPoint.mountDir, | ||
| readOnly: mountPoint.readOnly, | ||
| }), | ||
| ); | ||
| ossMountConfig = new OSSMountConfig({ | ||
| mountPoints, | ||
| }); | ||
| } | ||
|
|
||
| let polarFsConfig: PolarFsConfig | undefined; | ||
| if (config.polarFsConfig && !_.isEmpty(config.polarFsConfig.mountPoints)) { | ||
| const mountPoints = config.polarFsConfig.mountPoints.map( | ||
| (mountPoint: any) => | ||
| new PolarFsMountConfig({ | ||
| instanceId: mountPoint.instanceId, | ||
| mountDir: mountPoint.mountDir, | ||
| remoteDir: mountPoint.remoteDir, | ||
| }), | ||
| ); | ||
| polarFsConfig = new PolarFsConfig({ | ||
| groupId: config.polarFsConfig.groupId, | ||
| mountPoints, | ||
| userId: config.polarFsConfig.userId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract duplicated filesystem config construction logic.
The filesystem config construction for nasConfig, ossMountConfig, and polarFsConfig (lines 1057-1106) duplicates the same logic from createFunctionSession (lines 975-1024). This violates DRY and creates a maintenance burden—any changes to the mapping logic must be applied in both places.
♻️ Proposed refactor to extract a helper function
Add a private helper method to construct filesystem configurations:
+ private buildFileSystemConfigs(config: any): {
+ nasConfig?: NASConfig;
+ ossMountConfig?: OSSMountConfig;
+ polarFsConfig?: PolarFsConfig;
+ } {
+ let nasConfig: NASConfig | undefined;
+ if (config.nasConfig && !_.isEmpty(config.nasConfig.mountPoints)) {
+ const mountPoints = config.nasConfig.mountPoints.map(
+ (mountPoint: any) =>
+ new NASMountConfig({
+ enableTLS: mountPoint.enableTLS,
+ mountDir: mountPoint.mountDir,
+ serverAddr: mountPoint.serverAddr,
+ }),
+ );
+ nasConfig = new NASConfig({
+ groupId: config.nasConfig.groupId,
+ mountPoints,
+ userId: config.nasConfig.userId,
+ });
+ }
+
+ let ossMountConfig: OSSMountConfig | undefined;
+ if (config.ossMountConfig && !_.isEmpty(config.ossMountConfig.mountPoints)) {
+ const mountPoints = config.ossMountConfig.mountPoints.map(
+ (mountPoint: any) =>
+ new OSSMountPoint({
+ bucketName: mountPoint.bucketName,
+ bucketPath: mountPoint.bucketPath,
+ endpoint: mountPoint.endpoint,
+ mountDir: mountPoint.mountDir,
+ readOnly: mountPoint.readOnly,
+ }),
+ );
+ ossMountConfig = new OSSMountConfig({
+ mountPoints,
+ });
+ }
+
+ let polarFsConfig: PolarFsConfig | undefined;
+ if (config.polarFsConfig && !_.isEmpty(config.polarFsConfig.mountPoints)) {
+ const mountPoints = config.polarFsConfig.mountPoints.map(
+ (mountPoint: any) =>
+ new PolarFsMountConfig({
+ instanceId: mountPoint.instanceId,
+ mountDir: mountPoint.mountDir,
+ remoteDir: mountPoint.remoteDir,
+ }),
+ );
+ polarFsConfig = new PolarFsConfig({
+ groupId: config.polarFsConfig.groupId,
+ mountPoints,
+ userId: config.polarFsConfig.userId,
+ });
+ }
+
+ return { nasConfig, ossMountConfig, polarFsConfig };
+ }Then simplify both createFunctionSession and updateFunctionSession:
async createFunctionSession(functionName: string, config: any) {
- let nasConfig: NASConfig | undefined;
- if (config.nasConfig && !_.isEmpty(config.nasConfig.mountPoints)) {
- // ... 50 lines of config construction
- }
+ const { nasConfig, ossMountConfig, polarFsConfig } = this.buildFileSystemConfigs(config);
const createSessionInput = new CreateSessionInput({
disableSessionIdReuse: config.disableSessionIdReuse,
nasConfig,
ossMountConfig,
polarFsConfig,
sessionId: config.sessionId,
sessionTTLInSeconds: config.sessionTTLInSeconds,
sessionIdleTimeoutInSeconds: config.sessionIdleTimeoutInSeconds,
}); async updateFunctionSession(functionName: string, sessionId: string, config: any) {
- let nasConfig: NASConfig | undefined;
- if (config.nasConfig && !_.isEmpty(config.nasConfig.mountPoints)) {
- // ... 50 lines of config construction
- }
+ const { nasConfig, ossMountConfig, polarFsConfig } = this.buildFileSystemConfigs(config);
const updateSessionInput = new UpdateSessionInput({
disableSessionIdReuse: config.disableSessionIdReuse,
nasConfig,
ossMountConfig,
polarFsConfig,
sessionTTLInSeconds: config.sessionTTLInSeconds,
sessionIdleTimeoutInSeconds: config.sessionIdleTimeoutInSeconds,
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/resources/fc/index.ts` around lines 1057 - 1106, Extract the duplicated
filesystem construction logic into a shared private helper (e.g.,
buildFilesystemConfigs or constructFsConfigs) and call it from both
createFunctionSession and updateFunctionSession: move the mapping code that
creates NASMountConfig/NASConfig, OSSMountPoint/OSSMountConfig, and
PolarFsMountConfig/PolarFsConfig into that helper, accept the raw config object
(with mountPoints) and return the constructed typed config objects (nasConfig,
ossMountConfig, polarFsConfig) or undefined when empty; update both
createFunctionSession and updateFunctionSession to use this helper, preserving
types and the same field mappings (enableTLS, mountDir, serverAddr, bucketName,
bucketPath, endpoint, readOnly, instanceId, remoteDir, groupId, userId).
liuzewen99
force-pushed
the
fix-session
branch
3 times, most recently
from
a584eb5 to
941dde5
Compare
… and session resume
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
__tests__/it/integration_test.ts (1)
651-655: ⚡ Quick winAvoid passing on arbitrary errors in pause/resume test.
Catching any thrown error and asserting only
toBeDefined()makes unrelated failures pass (auth/network/argument regressions). Assert an expected error signature/code for the unsupported-runtime path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/it/integration_test.ts` around lines 651 - 655, The catch block that currently does expect(error).toBeDefined() is too permissive; update the pause/resume test to assert the specific unsupported-runtime error signature instead of any thrown error. Replace the generic assertion in the catch(error) of the integration test with checks that the error has the expected code or message for the unsupported-runtime path (e.g., assert error.code === 'UNSUPPORTED_RUNTIME' or error.message.includes('pause/resume requires SESSION_EXCLUSIVE' / 'unsupported runtime')), and if the thrown error does not match that signature rethrow it so unrelated failures (auth/network/argument regressions) fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/it/integration_test.ts`:
- Around line 558-621: The test "session operations" can skip cleanup if an
assertion or API call throws; wrap the test body so session and function cleanup
always run (use try/finally): capture sessionId from
myFcInstance.session(createInputs) if created, then in a finally block attempt
to remove the session via myFcInstance.session(removeSessionInputs) only if
sessionId is set, and always attempt to remove the function via
myFcInstance.remove(removeInputs), swallowing errors there as needed; ensure you
reference the existing variables and methods (sessionId, myFcInstance.session,
myFcInstance.remove, and the removeInputs/removeSessionInputs) when implementing
the finally-based cleanup.
---
Nitpick comments:
In `@__tests__/it/integration_test.ts`:
- Around line 651-655: The catch block that currently does
expect(error).toBeDefined() is too permissive; update the pause/resume test to
assert the specific unsupported-runtime error signature instead of any thrown
error. Replace the generic assertion in the catch(error) of the integration test
with checks that the error has the expected code or message for the
unsupported-runtime path (e.g., assert error.code === 'UNSUPPORTED_RUNTIME' or
error.message.includes('pause/resume requires SESSION_EXCLUSIVE' / 'unsupported
runtime')), and if the thrown error does not match that signature rethrow it so
unrelated failures (auth/network/argument regressions) fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a1c1bdb-863d-4ff5-9580-8e3929b49ff8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
__tests__/it/integration_test.ts__tests__/ut/commands/session_test.tspackage.jsonsrc/commands-help/session.tssrc/resources/fc/index.tssrc/subCommands/session/index.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/resources/fc/index.ts
- src/subCommands/session/index.ts
- src/commands-help/session.ts
Comment on lines
+558
to
+621
| test('session operations', async () => { | ||
| // Deploy function with sessionAffinity enabled (required for session operations) | ||
| const deployInputs = _.cloneDeep(inputs); | ||
| deployInputs.props.sessionAffinity = 'GENERATED_COOKIE'; | ||
| await myFcInstance.deploy(deployInputs); | ||
|
|
||
| // Test create session | ||
| const createInputs = _.cloneDeep(inputs); | ||
| createInputs.command = 'session'; | ||
| createInputs.args = ['create', '--qualifier', 'LATEST', '--st', '3600', '--si', '1800']; | ||
| const createOutput = await myFcInstance.session(createInputs); | ||
| expect(createOutput).toBeDefined(); | ||
| expect(createOutput.sessionId).toBeDefined(); | ||
| const sessionId = createOutput.sessionId; | ||
|
|
||
| // Test get session | ||
| const getInputs = _.cloneDeep(inputs); | ||
| getInputs.command = 'session'; | ||
| getInputs.args = ['get', '--session-id', sessionId, '--qualifier', 'LATEST']; | ||
| const getOutput = await myFcInstance.session(getInputs); | ||
| expect(getOutput).toBeDefined(); | ||
| expect(getOutput.sessionId).toBe(sessionId); | ||
|
|
||
| // Test list sessions | ||
| const listInputs = _.cloneDeep(inputs); | ||
| listInputs.command = 'session'; | ||
| listInputs.args = ['list', '--qualifier', 'LATEST']; | ||
| const listOutput = await myFcInstance.session(listInputs); | ||
| expect(listOutput).toBeDefined(); | ||
| expect(Array.isArray(listOutput)).toBe(true); | ||
|
|
||
| // Test update session | ||
| const updateInputs = _.cloneDeep(inputs); | ||
| updateInputs.command = 'session'; | ||
| updateInputs.args = [ | ||
| 'update', | ||
| '--session-id', | ||
| sessionId, | ||
| '--qualifier', | ||
| 'LATEST', | ||
| '--st', | ||
| '7200', | ||
| '--si', | ||
| '3600', | ||
| ]; | ||
| const updateOutput = await myFcInstance.session(updateInputs); | ||
| expect(updateOutput).toBeDefined(); | ||
|
|
||
| // Test remove session (DELETE operation may return undefined) | ||
| const removeSessionInputs = _.cloneDeep(inputs); | ||
| removeSessionInputs.command = 'session'; | ||
| removeSessionInputs.args = ['remove', '--session-id', sessionId, '--qualifier', 'LATEST', '-y']; | ||
| await myFcInstance.session(removeSessionInputs); | ||
|
|
||
| // Clean up: remove the function | ||
| const removeInputs = _.cloneDeep(inputs); | ||
| removeInputs.command = 'remove'; | ||
| removeInputs.args = ['--function', '--assume-yes']; | ||
| try { | ||
| await myFcInstance.remove(removeInputs); | ||
| } catch (error) { | ||
| // Ignore cleanup errors | ||
| } | ||
| }, 120000); |
There was a problem hiding this comment.
Ensure cleanup always runs in session operations.
If any assertion/API call fails before the cleanup block, the session/function cleanup is skipped. That can leak test resources and destabilize later integration runs.
Suggested fix
test('session operations', async () => {
- // Deploy function with sessionAffinity enabled (required for session operations)
- const deployInputs = _.cloneDeep(inputs);
- deployInputs.props.sessionAffinity = 'GENERATED_COOKIE';
- await myFcInstance.deploy(deployInputs);
-
- // Test create session
- const createInputs = _.cloneDeep(inputs);
- createInputs.command = 'session';
- createInputs.args = ['create', '--qualifier', 'LATEST', '--st', '3600', '--si', '1800'];
- const createOutput = await myFcInstance.session(createInputs);
- expect(createOutput).toBeDefined();
- expect(createOutput.sessionId).toBeDefined();
- const sessionId = createOutput.sessionId;
-
- // ... get/list/update/remove ...
-
- // Clean up: remove the function
- const removeInputs = _.cloneDeep(inputs);
- removeInputs.command = 'remove';
- removeInputs.args = ['--function', '--assume-yes'];
- try {
- await myFcInstance.remove(removeInputs);
- } catch (error) {
- // Ignore cleanup errors
- }
+ let sessionId: string | undefined;
+ try {
+ const deployInputs = _.cloneDeep(inputs);
+ deployInputs.props.sessionAffinity = 'GENERATED_COOKIE';
+ await myFcInstance.deploy(deployInputs);
+
+ const createInputs = _.cloneDeep(inputs);
+ createInputs.command = 'session';
+ createInputs.args = ['create', '--qualifier', 'LATEST', '--st', '3600', '--si', '1800'];
+ const createOutput = await myFcInstance.session(createInputs);
+ expect(createOutput.sessionId).toBeDefined();
+ sessionId = createOutput.sessionId;
+
+ // ... get/list/update ...
+ } finally {
+ if (sessionId) {
+ const removeSessionInputs = _.cloneDeep(inputs);
+ removeSessionInputs.command = 'session';
+ removeSessionInputs.args = ['remove', '--session-id', sessionId, '--qualifier', 'LATEST', '-y'];
+ await myFcInstance.session(removeSessionInputs).catch(() => undefined);
+ }
+
+ const removeInputs = _.cloneDeep(inputs);
+ removeInputs.command = 'remove';
+ removeInputs.args = ['--function', '--assume-yes'];
+ await myFcInstance.remove(removeInputs).catch(() => undefined);
+ }
}, 120000);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/it/integration_test.ts` around lines 558 - 621, The test "session
operations" can skip cleanup if an assertion or API call throws; wrap the test
body so session and function cleanup always run (use try/finally): capture
sessionId from myFcInstance.session(createInputs) if created, then in a finally
block attempt to remove the session via
myFcInstance.session(removeSessionInputs) only if sessionId is set, and always
attempt to remove the function via myFcInstance.remove(removeInputs), swallowing
errors there as needed; ensure you reference the existing variables and methods
(sessionId, myFcInstance.session, myFcInstance.remove, and the
removeInputs/removeSessionInputs) when implementing the finally-based cleanup.
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
1 participant
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.
…se and session resume
Summary by CodeRabbit
New Features
Documentation
Tests
Chores