-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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 session cleanup test #126966
Fix session cleanup test #126966
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -444,20 +444,22 @@ export class SessionIndex { | |
* Trigger a removal of any outdated session values. | ||
*/ | ||
async cleanUp() { | ||
this.options.logger.debug(`Running cleanup routine.`); | ||
const { auditLogger, elasticsearchClient, logger } = this.options; | ||
logger.debug(`Running cleanup routine.`); | ||
|
||
let error: Error | undefined; | ||
let haveDeletedSessions = false; | ||
try { | ||
for await (const sessionValues of this.getSessionValuesInBatches()) { | ||
const operations: Array<Required<Pick<BulkOperationContainer, 'delete'>>> = []; | ||
sessionValues.forEach(({ _id, _source }) => { | ||
const { usernameHash, provider } = _source!; | ||
this.options.auditLogger.log( | ||
sessionCleanupEvent({ sessionId: _id, usernameHash, provider }) | ||
); | ||
auditLogger.log(sessionCleanupEvent({ sessionId: _id, usernameHash, provider })); | ||
operations.push({ delete: { _id } }); | ||
}); | ||
if (operations.length > 0) { | ||
const bulkResponse = await this.options.elasticsearchClient.bulk( | ||
haveDeletedSessions = true; | ||
const bulkResponse = await elasticsearchClient.bulk( | ||
{ | ||
index: this.indexName, | ||
operations, | ||
|
@@ -471,24 +473,36 @@ export class SessionIndex { | |
0 | ||
); | ||
if (errorCount < bulkResponse.items.length) { | ||
this.options.logger.warn( | ||
logger.warn( | ||
`Failed to clean up ${errorCount} of ${bulkResponse.items.length} invalid or expired sessions. The remaining sessions were cleaned up successfully.` | ||
); | ||
} else { | ||
this.options.logger.error( | ||
logger.error( | ||
`Failed to clean up ${bulkResponse.items.length} invalid or expired sessions.` | ||
); | ||
} | ||
} else { | ||
this.options.logger.debug( | ||
`Cleaned up ${bulkResponse.items.length} invalid or expired sessions.` | ||
); | ||
logger.debug(`Cleaned up ${bulkResponse.items.length} invalid or expired sessions.`); | ||
} | ||
} | ||
} | ||
} catch (err) { | ||
this.options.logger.error(`Failed to clean up sessions: ${err.message}`); | ||
throw err; | ||
logger.error(`Failed to clean up sessions: ${err.message}`); | ||
error = err; | ||
} | ||
|
||
if (haveDeletedSessions) { | ||
try { | ||
await elasticsearchClient.indices.refresh({ index: this.indexName }); | ||
logger.debug(`Refreshed session index.`); | ||
} catch (err) { | ||
logger.error(`Failed to refresh session index: ${err.message}`); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Maybe put this block in a finally statement if you want it executed irregardless of outcome? Thinks it's cleaner and you don't need to assign the thrown error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old code re-threw the error in the |
||
|
||
if (error) { | ||
// If we couldn't fetch or delete sessions, throw an error so the task will be retried. | ||
throw error; | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically speaking we haven't actually deleted anything at this point. Is it worth being stricter here and only marking sessions as having been touched after the bulk delete operation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably could have clarified this in the comments:
My reasoning for doing it here is that I'm not 100% sure if a failed
bulk
operation could have resulted in some of the sessions getting deleted. I figured it's safest to trigger an index refresh anyway, but I'm not sure how much of a practical impact this has tbh. I supposehaveAttemptedToDeleteSessions
would have been a more accurate variable name but that sounds like a mouthful 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed here: f4be021
Let me know what you think!