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

feat: mark graph sessions as inactive so they don't show up in any UI #4285

Merged
merged 6 commits into from
Feb 14, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 53 additions & 23 deletions src/commands/dev/dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const { startFunctionsServer } = require('../../lib/functions/server')
const {
OneGraphCliClient,
loadCLISession,
markCliSessionInactive,
persistNewOperationsDocForSession,
startOneGraphCLISession,
} = require('../../lib/one-graph/cli-client')
Expand Down Expand Up @@ -78,6 +79,30 @@ const isNonExistingCommandError = ({ command, error: commandError }) => {
)
}

/**
* @type {(() => Promise<void>)[]} - array of functions to run before the process exits
*/
const cleanupWork = []

let cleanupStarted = false

/**
* @param {object} input
* @param {number=} input.exitCode The exit code to return when exiting the process after cleanup
*/
const cleanupBeforeExit = async ({ exitCode }) => {
// If cleanup has started, then wherever started it will be responsible for exiting
if (!cleanupStarted) {
cleanupStarted = true
try {
// eslint-disable-next-line no-unused-vars
const cleanupFinished = await Promise.all(cleanupWork.map((cleanup) => cleanup()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[sand] Since cleanupFinished is not used, should it be removed?

} finally {
process.exit(exitCode)
}
}
}

/**
* Run a command and pipe stdout, stderr and stdin
* @param {string} command
Expand All @@ -100,30 +125,29 @@ const runCommand = (command, env = {}) => {

// we can't try->await->catch since we don't want to block on the framework server which
// is a long running process
// eslint-disable-next-line promise/catch-or-return,promise/prefer-await-to-then
commandProcess.then(async () => {
const result = await commandProcess
const [commandWithoutArgs] = command.split(' ')
// eslint-disable-next-line promise/always-return
if (result.failed && isNonExistingCommandError({ command: commandWithoutArgs, error: result })) {
log(
NETLIFYDEVERR,
`Failed running command: ${command}. Please verify ${chalk.magenta(`'${commandWithoutArgs}'`)} exists`,
)
} else {
const errorMessage = result.failed
? `${NETLIFYDEVERR} ${result.shortMessage}`
: `${NETLIFYDEVWARN} "${command}" exited with code ${result.exitCode}`

log(`${errorMessage}. Shutting down Netlify Dev server`)
}
process.exit(1)
})
;['SIGINT', 'SIGTERM', 'SIGQUIT', 'SIGHUP', 'exit'].forEach((signal) => {
process.on(signal, () => {
commandProcess.kill('SIGTERM', { forceKillAfterTimeout: 500 })
process.exit()
// eslint-disable-next-line promise/catch-or-return
commandProcess
// eslint-disable-next-line promise/prefer-await-to-then
.then(async () => {
const result = await commandProcess
const [commandWithoutArgs] = command.split(' ')
if (result.failed && isNonExistingCommandError({ command: commandWithoutArgs, error: result })) {
log(
NETLIFYDEVERR,
`Failed running command: ${command}. Please verify ${chalk.magenta(`'${commandWithoutArgs}'`)} exists`,
)
} else {
const errorMessage = result.failed
? `${NETLIFYDEVERR} ${result.shortMessage}`
: `${NETLIFYDEVWARN} "${command}" exited with code ${result.exitCode}`

log(`${errorMessage}. Shutting down Netlify Dev server`)
}

return await cleanupBeforeExit({ exitCode: 1 })
})
;['SIGINT', 'SIGTERM', 'SIGQUIT', 'SIGHUP', 'exit'].forEach((signal) => {
process.on(signal, async () => await cleanupBeforeExit({}))
})

return commandProcess
Expand Down Expand Up @@ -336,12 +360,18 @@ const dev = async (options, command) => {
const oneGraphSessionId = loadCLISession(state)

await persistNewOperationsDocForSession({
netlifyGraphConfig,
netlifyToken,
oneGraphSessionId,
operationsDoc: graphqlDocument,
siteId: site.id,
siteRoot: site.root,
})

const cleanupSession = () => markCliSessionInactive({ netlifyToken, sessionId: oneGraphSessionId, siteId: site.id })

cleanupWork.push(cleanupSession)

const graphEditUrl = getGraphEditUrlBySiteId({ siteId: site.id, oneGraphSessionId })

log(
Expand Down
21 changes: 7 additions & 14 deletions src/commands/graph/graph-edit.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
// @ts-check
const gitRepoInfo = require('git-repo-info')

const {
OneGraphCliClient,
createCLISession,
generateSessionName,
loadCLISession,
upsertMergeCLISessionMetadata,
} = require('../../lib/one-graph/cli-client')
const { OneGraphCliClient, ensureCLISession, upsertMergeCLISessionMetadata } = require('../../lib/one-graph/cli-client')
const {
defaultExampleOperationsDoc,
getGraphEditUrlBySiteId,
Expand Down Expand Up @@ -48,13 +42,12 @@ const graphEdit = async (options, command) => {

await ensureAppForSite(netlifyToken, siteId)

let oneGraphSessionId = loadCLISession(state)
if (!oneGraphSessionId) {
const sessionName = generateSessionName()
const oneGraphSession = await createCLISession({ netlifyToken, siteId: site.id, sessionName, metadata: {} })
state.set('oneGraphSessionId', oneGraphSession.id)
oneGraphSessionId = state.get('oneGraphSessionId')
}
const oneGraphSessionId = await ensureCLISession({
metadata: {},
netlifyToken,
site,
state,
})

const { branch } = gitRepoInfo()
const persistedDoc = await createPersistedQuery(netlifyToken, {
Expand Down
130 changes: 115 additions & 15 deletions src/lib/one-graph/cli-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ const {

const { parse } = GraphQL
const { defaultExampleOperationsDoc, extractFunctionsFromOperationDoc } = NetlifyGraph
const { createPersistedQuery, ensureAppForSite, updateCLISessionMetadata } = OneGraphClient
const {
createPersistedQuery,
ensureAppForSite,
executeMarkCliSessionActiveHeartbeat,
executeMarkCliSessionInactive,
updateCLISessionMetadata,
} = OneGraphClient

const internalConsole = {
log,
Expand All @@ -51,13 +57,31 @@ InternalConsole.registerConsole(internalConsole)
* @param {function} input.onEvents A function to call when CLI events are received and need to be processed
* @param {string} input.sessionId The session id to monitor CLI events for
* @param {StateConfig} input.state A function to call to set/get the current state of the local Netlify project
* @param {any} input.site The site object
* @returns
*/
const monitorCLISessionEvents = (input) => {
const { appId, netlifyGraphConfig, netlifyToken, onClose, onError, onEvents, sessionId, state } = input
const { appId, netlifyGraphConfig, netlifyToken, onClose, onError, onEvents, sessionId, site, state } = input

const frequency = 5000
// 30 minutes
const defaultHeartbeatFrequency = 1_800_000
let shouldClose = false
let nextMarkActiveHeartbeat = defaultHeartbeatFrequency

const markActiveHelper = async () => {
const fullSession = await OneGraphClient.fetchCliSession({ authToken: netlifyToken, appId, sessionId })
// @ts-ignore
const heartbeatIntervalms = fullSession.session.cliHeartbeatIntervalMs || defaultHeartbeatFrequency
nextMarkActiveHeartbeat = heartbeatIntervalms
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[sand] The above two lines can be merged by removing the intermediary heartbeatIntervalms variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I specifically pulled it out so that it's obvious what the intention of that variable is via its name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright 👍

const markCLISessionActiveResult = await executeMarkCliSessionActiveHeartbeat(netlifyToken, site.id, sessionId)
if (markCLISessionActiveResult.errors && markCLISessionActiveResult.errors.length !== 0) {
warn(`Failed to mark CLI session active: ${markCLISessionActiveResult.errors.join(', ')}`)
}
setTimeout(markActiveHelper, nextMarkActiveHeartbeat)
}

setTimeout(markActiveHelper, nextMarkActiveHeartbeat)

const enabledServiceWatcher = async (innerNetlifyToken, siteId) => {
const enabledServices = state.get('oneGraphEnabledServices') || ['onegraph']
Expand Down Expand Up @@ -371,6 +395,7 @@ const upsertMergeCLISessionMetadata = async ({ netlifyToken, newMetadata, oneGra

const detectedMetadata = detectLocalCLISessionMetadata({ siteRoot })

// @ts-ignore
const finalMetadata = { ...metadata, ...detectedMetadata, ...newMetadata }
return OneGraphClient.updateCLISessionMetadata(netlifyToken, siteId, oneGraphSessionId, finalMetadata)
}
Expand Down Expand Up @@ -429,19 +454,13 @@ const loadCLISession = (state) => state.get('oneGraphSessionId')
const startOneGraphCLISession = async (input) => {
const { netlifyGraphConfig, netlifyToken, site, state } = input
OneGraphClient.ensureAppForSite(netlifyToken, site.id)
let oneGraphSessionId = loadCLISession(state)
if (!oneGraphSessionId) {
const sessionName = generateSessionName()
const sessionMetadata = {}
const oneGraphSession = await createCLISession({
netlifyToken,
siteId: site.id,
sessionName,
metadata: sessionMetadata,
})
state.set('oneGraphSessionId', oneGraphSession.id)
oneGraphSessionId = state.get('oneGraphSessionId')
}

const oneGraphSessionId = await ensureCLISession({
metadata: {},
netlifyToken,
site,
state,
})

const enabledServices = []
const schema = await OneGraphClient.fetchOneGraphSchema(site.id, enabledServices)
Expand Down Expand Up @@ -481,6 +500,7 @@ const startOneGraphCLISession = async (input) => {
netlifyToken,
netlifyGraphConfig,
sessionId: oneGraphSessionId,
site,
state,
onEvents: async (events) => {
for (const event of events) {
Expand All @@ -500,6 +520,20 @@ const startOneGraphCLISession = async (input) => {
})
}

/**
* Mark a session as inactive so it doesn't show up in any UI lists, and potentially becomes available to GC later
* @param {object} input
* @param {string} input.netlifyToken The (typically netlify) access token that is used for authentication, if any
* @param {string} input.siteId A function to call to set/get the current state of the local Netlify project
* @param {string} input.sessionId The session id to monitor CLI events for
*/
const markCliSessionInactive = async ({ netlifyToken, sessionId, siteId }) => {
const result = await executeMarkCliSessionInactive(netlifyToken, siteId, sessionId)
if (result.errors) {
warn(`Unable to mark CLI session ${sessionId} inactive: ${JSON.stringify(result.errors, null, 2)}`)
}
}

/**
* Generate a session name that can be identified as belonging to the current checkout
* @returns {string} The name of the session to create
Expand All @@ -511,6 +545,70 @@ const generateSessionName = () => {
return sessionName
}

/**
* Ensures a cli session exists for the current checkout, or errors out if it doesn't and cannot create one.
*/
const ensureCLISession = async ({ metadata, netlifyToken, site, state }) => {
let oneGraphSessionId = loadCLISession(state)
let parentCliSessionId = null

// Validate that session still exists and we can access it
try {
if (oneGraphSessionId) {
const sessionEvents = await OneGraphClient.fetchCliSessionEvents({
appId: site.id,
authToken: netlifyToken,
sessionId: oneGraphSessionId,
})
if (sessionEvents.errors) {
warn(`Unable to fetch cli session: ${JSON.stringify(sessionEvents.errors, null, 2)}`)
log(`Creating new cli session`)
parentCliSessionId = oneGraphSessionId
oneGraphSessionId = null
}
}
} catch (fetchSessionError) {
warn(`Unable to fetch cli session events: ${JSON.stringify(fetchSessionError, null, 2)}`)
oneGraphSessionId = null
}

if (!oneGraphSessionId) {
// If we can't access the session in the state.json or it doesn't exist, create a new one
const sessionName = generateSessionName()
const detectedMetadata = detectLocalCLISessionMetadata({ siteRoot: site.root })
const newSessionMetadata = parentCliSessionId ? { parentCliSessionId } : {}
const sessionMetadata = {
...detectedMetadata,
...newSessionMetadata,
...metadata,
}
const oneGraphSession = await createCLISession({
netlifyToken,
siteId: site.id,
sessionName,
metadata: sessionMetadata,
})
state.set('oneGraphSessionId', oneGraphSession.id)
oneGraphSessionId = state.get('oneGraphSessionId')
}

if (!oneGraphSessionId) {
error('Unable to create or access Netlify Graph CLI session')
}

const { errors: markCLISessionActiveErrors } = await executeMarkCliSessionActiveHeartbeat(
netlifyToken,
site.id,
oneGraphSessionId,
)

if (markCLISessionActiveErrors) {
warn(`Unable to mark cli session active: ${JSON.stringify(markCLISessionActiveErrors, null, 2)}`)
}

return oneGraphSessionId
}

const OneGraphCliClient = {
ackCLISessionEvents: OneGraphClient.ackCLISessionEvents,
createPersistedQuery,
Expand All @@ -522,10 +620,12 @@ const OneGraphCliClient = {
module.exports = {
OneGraphCliClient,
createCLISession,
ensureCLISession,
extractFunctionsFromOperationDoc,
handleCliSessionEvent,
generateSessionName,
loadCLISession,
markCliSessionInactive,
monitorCLISessionEvents,
persistNewOperationsDocForSession,
refetchAndGenerateFromOneGraph,
Expand Down