feat: mark graph sessions as inactive so they don't show up in any UI #4285
Conversation
github-actions
bot
added
the
type: feature
src/commands/dev/dev.js
Outdated
| }) | ||
| // eslint-disable-next-line promise/prefer-await-to-then,promise/always-return | ||
| .then(() => { | ||
| process.exit(1) |
There was a problem hiding this comment.
I'm a little bit confused by the presence of this line - it exits too rapidly, and prevents the signal handlers below from actually completing currently. I've left it in looking for some guidance, since I'm not sure why it's here.
📊 Benchmark resultsComparing with 050bf90 Package size: 442 MB(no change) Legend
|
src/commands/dev/dev.js
Outdated
| process.on(signal, async () => { | ||
| if (!cleanupStarted) { | ||
| // eslint-disable-next-line no-unused-vars | ||
| const cleanupFinished = await Promise.all(cleanupWork.map((cleanup) => cleanup())) |
There was a problem hiding this comment.
Can we synchronise the cleanup work? Not sure if the promise in an process.on will be awaited propperly
There was a problem hiding this comment.
We cannot make it sync it, it's an IO call. I've tested locally that the promise is properly awaited though.
src/commands/dev/dev.js
Outdated
| } | ||
| }) | ||
| // eslint-disable-next-line promise/prefer-await-to-then | ||
| .then(() => { |
There was a problem hiding this comment.
Can we avoid this promise chaining and move all this code under the first then?
There was a problem hiding this comment.
How do we await the Promise.all inside of the first then?
There was a problem hiding this comment.
Hah, I see, it's an async function... on it!
src/commands/dev/dev.js
Outdated
| }) | ||
| // eslint-disable-next-line promise/prefer-await-to-then,promise/always-return | ||
| .then(() => { | ||
| process.exit(1) |
There was a problem hiding this comment.
Similar as before, can we move this under the first then?
|
@erezrokah cleaned up and centralized the This also fixes a bug that's probably been in the CLI for awhile where hitting
Because of the two separate |
| const fullSession = await OneGraphClient.fetchCliSession({ authToken: netlifyToken, appId, sessionId }) | ||
| // @ts-ignore | ||
| const heartbeatIntervalms = fullSession.session.cliHeartbeatIntervalMs || defaultHeartbeatFrequency | ||
| nextMarkActiveHeartbeat = heartbeatIntervalms |
There was a problem hiding this comment.
[sand] The above two lines can be merged by removing the intermediary heartbeatIntervalms variable.
There was a problem hiding this comment.
I specifically pulled it out so that it's obvious what the intention of that variable is via its name.
| cleanupStarted = true | ||
| try { | ||
| // eslint-disable-next-line no-unused-vars | ||
| const cleanupFinished = await Promise.all(cleanupWork.map((cleanup) => cleanup())) |
There was a problem hiding this comment.
[sand] Since cleanupFinished is not used, should it be removed?
|
@ehmicky Ok, addressed PR comments |
Summary
Fixes https://github.com/netlify/netlify-graph-project-planning/issues/44 (the cli portion)
We need to make sure we create sessions with the appropriate metadata, that we execute a heartbeat at server-specified intervals, and that on process exit we try our hardest to mark the session as inactive so that it can be removed from any UI list immediately, and also so that the server can GC sessions and associated resources.
Because the sessions may end up being GC'd, there's also logic to check if a session is still available at startup, and to create a new one without user intervention if not.
For us to review and ship your PR efficiently, please perform the following steps: