Fix local workspace reuse #4791
Conversation
|
@nicknytko could you test this out? Does it look safe for prod to you? |
|
This seems especially important if anyone is using the xfce desktop workspace. It seems that xfce can fail to start up if it loads with a reused workspace directory because of the various locking and session files it creates, or something. In messing with the desktop image, I've had to continually |
|
@nicknytko bump :-D |
lib/workspace.js
Outdated
| @@ -245,16 +245,29 @@ module.exports = { | |||
| } | |||
| } else if (workspace.homedir_location == 'FileSystem') { | |||
| const root = config.workspaceHomeDirRoot; | |||
| const fullRemotePath = path.join(root, remotePath); | |||
| const timestampSuffix = (new Date()).toISOString().substring(0,19).replace(/[^a-zA-Z0-9]/g, '-'); | |||
There was a problem hiding this comment.
| const timestampSuffix = (new Date()).toISOString().substring(0,19).replace(/[^a-zA-Z0-9]/g, '-'); | |
| const timestampSuffix = (new Date()).toISOString().replace(/[:.]/g, '-'); |
Just curious, why strip the milliseconds? And with the regex, are you just worried that the ISO standard will change at some point? Otherwise I think [:.] is clearer.
There was a problem hiding this comment.
If this is for local workspaces, it's important to note that colon characters are not allowed in normal Windows filenames. These denote ADS (alternate data streams) on NTFS. I don't really expect the ISO standard to change, but I felt that simpler and less potentially breakable was better.
What if we just keep the full length string but use underscores or dots instead of hyphens, unless the hyphens are in the original?
const timestampSuffix = (new Date()).toISOString().replace(/[^a-zA-Z0-9-]/g, '_');
// e.g.: 2021-10-08T02_50_25_818ZThere was a problem hiding this comment.
Rereading your comment, I think I may have misunderstood your objection to begin with. The [:.] pattern is more concise, but it makes more of an assumption about what characters are in the ISO timestamp format. To me, the intentions of the [^a-zA-Z0-9-] are more clear. Whether to replace characters with dash or underscore is probably irrelevant; I doubt that people will dig up these directories anyway, unless in an emergency.
There was a problem hiding this comment.
What I meant about the ISO comment was just that I think replace(/[:.]/g, '-') is more readable while achieving the same thing, unless we're worried that toISOString() will introduce other illegal characters at some point. Not a big deal either way.
There was a problem hiding this comment.
I do think it's better to replace with underscore rather than hyphen if we leave the end part on the string, just because hyphen can have special meaning if the timezone is attached. (And, this slightly complicates the situation that a + might be changed to an _, which would just be confusing. I thought it might be simplest to trim the string to the part before the milliseconds ignoring the rest, just to have enough to make it unique as the workspace is spun back up.)
There was a problem hiding this comment.
@echuber2 I agree with your more verbose regex ([^a-zA-Z0-9-]), and I also think we should remove the .substring(0,19).
There was a problem hiding this comment.
I made this change in 62fd331.
Co-authored-by: Tim Yang <tdy@users.noreply.github.com>
Co-authored-by: Tim Yang <tdy@users.noreply.github.com>
|
This issue is still causing confusion for some course staff as they debug workspaces (just came up today in discussions with CS 233 staff). I hope we can merge. |
Co-authored-by: Tim Yang <tdy@users.noreply.github.com> Co-authored-by: Nathan Walters <nwalters512@gmail.com>
Fixes #4790
Moves a pre-existing local workspace mounted directory to a backup copy labeled with the (sanitized) ISO timestamp.
Please check that I've handled the error ignoring/rethrowing properly within the async context.