Fix project save bugs that wedged locks, lost updates and stored "null"

src/auth/identity-lookup-policy.osl

@@ -0,0 +1,11 @@
+export {}
+
+def profileLookupOutcome(number status) string (
+  if status == 200 (
+    return "found"
+  )
+  if status == 0 or status == 408 or status == 429 or status >= 500 (
+    return "unavailable"
+  )
+  return "missing"
+)

src/auth/identity-lookup-policy.test.osl

@@ -0,0 +1,9 @@
+import "std:testing"
+import "./identity-lookup-policy.osl"
+
+testing.assert(profileLookupOutcome(200) == "found", "a 200 profile is a found user")
+testing.assert(profileLookupOutcome(404) == "missing", "a 404 means the user is gone")
+testing.assert(profileLookupOutcome(400) == "missing" and profileLookupOutcome(403) == "missing", "a malformed or banned username is not found")
+testing.assert(profileLookupOutcome(0) == "unavailable", "an unreachable Rotur is unavailable")
+testing.assert(profileLookupOutcome(408) == "unavailable" and profileLookupOutcome(429) == "unavailable", "a timeout or rate limit is unavailable")
+testing.assert(profileLookupOutcome(500) == "unavailable" and profileLookupOutcome(502) == "unavailable" and profileLookupOutcome(503) == "unavailable", "server errors are unavailable")

src/auth/user-identity.osl

@@ -1,5 +1,6 @@
 export {
   discoverUserIdentity,
+  discoverUserIdentityOutcome,
   identityMatches,
   loadUserIds,
   purgeUser,
@@ -528,34 +529,39 @@ def refreshUsernameForId(string userId) string (
 )
 
 def discoverUserIdentity(string username) string (
+  return discoverUserIdentityOutcome(username).id.toStr()
+)
+
+def discoverUserIdentityOutcome(string username) object (
   string key = normalizeUsername(username)
   if key == "" or isStudentUsername(key) (
-    return ""
+    return {id: "", outcome: "missing"}
   )
   object resp = requests.get(roturBase ++ "/profile/" ++ url.escape(username) ++ "?include_posts=0", roturServiceOptions()).unwrapOr({body: null, error: "request failed", headers: {}, raw: null, status: 0, success: false})
-  if resp.status == 0 (
+  string outcome = profileLookupOutcome(resp.status)
+  if outcome != "found" (
     if resp.status == 404 (
       string uid = userIdForUsername(username)
       if uid != "" (
         identityRefreshCache.delete(uid)
         refreshUsernameForId(uid)
       )
     )
-    return ""
+    return {id: "", outcome: outcome}
   )
   auto parsed = json.parse(resp.body, {type: "object"})
   if parsed.isErr() (
-    return ""
+    return {id: "", outcome: "unavailable"}
   )
   object profile = parsed.unwrap().assert(object)
   string userId = profile.id.toStr()
   string currentUsername = profile.username.toStr()
   if userId == "" or currentUsername == "" (
-    return ""
+    return {id: "", outcome: "missing"}
   )
   ensureUserIdentity(userId, currentUsername)
   identityRefreshCache.set(userId, currentUsername)
-  return userId
+  return {id: userId, outcome: "found"}
 )
 
 def usernameForId(string userId) string (

src/history/history-migration.osl

@@ -153,9 +153,8 @@ def generateProjectHistory(object project, string parentPath, string parentHead,
 def ensureUploadedProjectHistory(object project) object (
   string lockName = "project-history-" ++ project.id.toStr()
   sync.unlock(lockName)
-  object res = ensureGeneratedProjectHistory(project, {}, false)
-  sync.lock(lockName)
-  return res
+  defer sync.lock(lockName)
+  return ensureGeneratedProjectHistory(project, {}, false)
 )
 
 def ensureGeneratedProjectHistory(object project, object visiting, boolean allowMissingAssets) object (
@@ -221,6 +220,10 @@ def ensureGeneratedProjectHistory(object project, object visiting, boolean allow
   )
 
   sync.lock("project-history-" ++ id)
+  defer (
+    sync.unlock("project-history-" ++ id)
+    void fs.remove(parentPath)
+  )
   auto currentResult = loadProject(id)
   object res = {ok: false, error: "project disappeared during migration"}
   if currentResult.ok (
@@ -233,8 +236,6 @@ def ensureGeneratedProjectHistory(object project, object visiting, boolean allow
       res = generateProjectHistory(current, parentPath, parentHead, allowMissingAssets)
     )
   )
-  sync.unlock("project-history-" ++ id)
-  void fs.remove(parentPath)
   return res
 )
 

src/history/history-migration.test.osl

@@ -15,6 +15,7 @@ number saves = 0
 number readFailures = 0
 boolean failRead = false
 boolean failCopy = false
+boolean throwRead = false
 number serial = 0
 
 def randomString(number length) string (
@@ -23,6 +24,9 @@ def randomString(number length) string (
 )
 
 def loadProject(string id) object (
+  if throwRead (
+    throw "project read exploded"
+  )
   if readFailures > 0 (
     readFailures -= 1
     return {ok: false, retryable: true}
@@ -209,4 +213,30 @@ sync.lock("project-history-stale-upload")
 object staleHistory = ensureUploadedProjectHistory(staleUpload)
 sync.unlock("project-history-stale-upload")
 testing.assert(staleHistory.ok == false and saves == before, "history preparation rejects a replaced upload snapshot")
+def releaseUploadLock(string lockName) boolean (
+  sync.unlock(lockName)
+  return true
+)
+def uploadLockFree(string lockName) boolean (
+  return thread.new(def() -> (
+    boolean taken = sync.tryLock(lockName)
+    if taken (
+      sync.unlock(lockName)
+    )
+    return taken
+  )).wait()
+)
+object throwingChild = {id: "throwing-child", owner: "Mist", created: 9000, jsonKey: "throwing-child.json", lastUploadAt: 9000, remixParent: "parent", remixBaseCommit: base, assets: []}
+throwRead = true
+sync.lock("project-history-throwing-child")
+testing.assert(try(ensureUploadedProjectHistory(throwingChild)).isErr(), "a failing ancestor read reaches the upload")
+throwRead = false
+testing.assert(!uploadLockFree("project-history-throwing-child") and !try(releaseUploadLock("project-history-throwing-child")).isErr(), "a failing ancestor read hands the lock back to the upload")
+testing.assert(uploadLockFree("project-history-throwing-child"), "the upload can then release its lock")
+object throwingRoot = {id: "throwing-root", owner: "Mist", created: 9000, jsonKey: "throwing-root.json", lastUploadAt: 9000, assets: []}
+throwRead = true
+sync.lock("project-history-throwing-root")
+testing.assert(try(ensureUploadedProjectHistory(throwingRoot)).isErr(), "a failing read under the history lock reaches the upload")
+throwRead = false
+testing.assert(!try(releaseUploadLock("project-history-throwing-root")).isErr() and uploadLockFree("project-history-throwing-root"), "a failing read under the history lock still hands the lock back once")
 void fs.remove(tmpDir)

src/projects/extensions.osl

@@ -199,7 +199,9 @@ def readProjectExtensionUrls(*json.Stream stream) object (
 )
 
 def inspectProjectExtensionUrls(string jsonPath) object (
-  *json.Stream stream = json.open(jsonPath, maxProjectJsonBytes)
+  *json.Stream stream = json.open(jsonPath, maxProjectJsonBytes) catch (err) (
+    return {ok: false, error: "invalid extension URLs"}
+  )
   object inspected = readProjectExtensionUrls(stream) catch (err) (
     stream.close()
     return {ok: false, error: "invalid extension URLs"}
@@ -220,6 +222,9 @@ def parseExtensionSources(string rawSources) object (
 )
 
 def collectProjectExtensions(string jsonPath, string rawSources) object (
+  if !fs.isFile(jsonPath) (
+    return {ok: false, error: "sb3 has no project.json"}
+  )
   object inspected = inspectProjectExtensionUrls(jsonPath)
   if !inspected.ok (
     return inspected
@@ -337,6 +342,8 @@ def storeProjectExtensions(string projectId, array previousExtensions, array ext
     if !projects.contains(projectId) (
       projects.append(projectId)
     )
+    record.urls = recordUrls
+    record.projects = projects
     if !writeFile(extensionRecordPath(hash), record) (
       sync.unlock(lockName)
       return false
@@ -361,7 +368,7 @@ def extensionUsageMigrated(object state) boolean (
   if !data.contains("version") (
     return false
   )
-  return data.version.toNum() >= 1
+  return data.version.toNum() >= 2
 )
 
 def migrateExtensionUsage() boolean (
@@ -388,7 +395,7 @@ def migrateExtensionUsage() boolean (
       )
     )
   )
-  boolean saved = writeFile(statePath, {version: 1})
+  boolean saved = writeFile(statePath, {version: 2})
   sync.unlock("extension-index-migration")
   return saved
 )

src/projects/extensions.test.osl

@@ -0,0 +1,110 @@
+import "std:testing"
+import "std:json"
+import "std:fs"
+import "std:regex"
+import "std:sync"
+import "std:serve"
+import "std:zip"
+import "./extensions.osl"
+
+string tmpDir = "/tmp/mistwarp-extensions-test-" ++ timestamp.toStr() ++ "/"
+string extensionsDir = tmpDir ++ "extensions/"
+string extensionsFile = tmpDir ++ "extensions.json"
+string projectsDir = tmpDir ++ "projects/"
+number maxProjectJsonBytes = 1048576
+number maxProjectExtensions = 20
+number maxExtensionSourceBytes = 2097152
+number maxProjectExtensionBytes = 10485760
+void fs.mkdir(extensionsDir, {recursive: true})
+
+def readFile(string path) object (
+  auto text = fs.readFile(path)
+  if text.isErr() (
+    return {ok: false}
+  )
+  auto parsed = json.parse(text.unwrap())
+  if parsed.isErr() (
+    return {ok: false}
+  )
+  return {ok: true, data: parsed.unwrap()}
+)
+def writeFile(string path, object data) boolean (
+  return fs.writeFile(path, json.stringify(data))
+)
+def randomString(number length) string (
+  return "r" * length
+)
+def removeString(array items, string value) array (
+  array kept = []
+  for i items.len (
+    if items[i].toStr() != value (
+      kept.append(items[i])
+    )
+  )
+  return kept
+)
+def loadProject(string id) object (
+  return {ok: false, missing: true}
+)
+def copyStoredProjectJsonToPath(object project, string path) boolean (
+  return false
+)
+def loadProfile(string username) object (
+  return {}
+)
+def saveProfile(object profile) boolean (
+  return true
+)
+def addNotification(string target, string actor, string kind, object payload) (
+  return
+)
+def saveProject(object project) boolean (
+  return true
+)
+def canViewProjectJson(object project, *serve.Context c) boolean (
+  return true
+)
+def requestJSON(*serve.Context c) object (
+  return {}
+)
+def upsertIndexEntry(object project) boolean (
+  return true
+)
+
+object noJson = collectProjectExtensions(tmpDir ++ "unpacked/project.json", "")
+testing.assert(noJson.ok == false and noJson.error == "sb3 has no project.json", "an sb3 without project.json is refused")
+void fs.writeFile(tmpDir ++ "project.json", json.stringify({targets: [], extensionURLs: {ext: "https://example.com/ext.js"}}))
+object collected = collectProjectExtensions(tmpDir ++ "project.json", json.stringify({"https://example.com/ext.js": "class Ext {}"}))
+testing.assert(collected.ok == true and collected.extensions.len == 1 and collected.extensions[1].url == "https://example.com/ext.js", "extension URLs are collected from project.json")
+
+object stored = collected.extensions[1].assert(object)
+string hash = stored.hash.toStr()
+testing.assert(storeProjectExtensions("p1", [], [stored], collected.sources.assert(object)), "extensions are stored")
+testing.assert(storeProjectExtensions("p2", [], [stored], {}), "a second project using the extension is stored")
+object record = readExtensionRecord(hash)
+testing.assert(record.urls == ["https://example.com/ext.js"] and record.projects == ["p1", "p2"], "the record lists the extension's URL and every project using it")
+testing.assert(fs.exists(extensionSourcePath(hash)), "the source is kept")
+testing.assert(storeProjectExtensions("p1", [stored], [], {}) and readExtensionRecord(hash).projects == ["p2"], "a project that drops the extension leaves the record")
+object gallery = {url: "https://extensions.turbowarp.org/a.js", hash: "https://extensions.turbowarp.org/a.js".hashSHA256()}
+testing.assert(storeProjectExtensions("p3", [], [gallery], {}) and readExtensionRecord(gallery.hash.toStr()).projects == ["p3"] and readExtensionRecord(gallery.hash.toStr()).gallery == true, "gallery extensions are recorded too")
+
+string staleUrl = "https://example.com/stale.js"
+string staleHash = staleUrl.hashSHA256()
+void fs.mkdir(projectsDir, {recursive: true})
+void writeFile(projectsDir ++ "p4.json", {id: "p4", extensions: [{url: staleUrl, hash: staleHash}]})
+void writeFile(projectsDir ++ "p5.json", {id: "p5", extensions: [{url: staleUrl, hash: staleHash}, {url: "https://example.com/ext.js", hash: hash}]})
+void writeFile(extensionRecordPath(staleHash), {status: "untrusted", urls: [], projects: [], firstSeen: 1})
+void writeFile(extensionsDir ++ "index.json", {version: 1})
+testing.assert(migrateExtensionUsage(), "the extension usage migration runs again after the record fix")
+object backfilled = readExtensionRecord(staleHash)
+array backfilledProjects = backfilled.projects.assert(array)
+testing.assert(backfilled.urls == [staleUrl] and backfilledProjects.len == 2 and backfilledProjects.contains("p4") and backfilledProjects.contains("p5") and backfilled.status == "untrusted" and backfilled.firstSeen == 1, "records written before the fix get their URLs and projects back")
+testing.assert(readFile(extensionsDir ++ "index.json").data.version == 2, "the migration records its new version")
+string afterFirst = json.stringify(readExtensionRecord(staleHash))
+string sharedAfterFirst = json.stringify(readExtensionRecord(hash))
+void writeFile(extensionsDir ++ "index.json", {version: 1})
+testing.assert(migrateExtensionUsage() and json.stringify(readExtensionRecord(staleHash)) == afterFirst and json.stringify(readExtensionRecord(hash)) == sharedAfterFirst, "running the migration again changes nothing")
+array sharedProjects = readExtensionRecord(hash).projects.assert(array)
+testing.assert(sharedProjects.contains("p2") and sharedProjects.contains("p5"), "the backfill keeps projects the record already listed")
+
+void fs.remove(tmpDir)

src/projects/project-lock.osl

@@ -0,0 +1,11 @@
+export {}
+
+def projectSaveLock(string id) string (
+  return "project-history-" ++ id
+)
+
+def runLocked(string lockName, function work) (
+  sync.lock(lockName)
+  defer sync.unlock(lockName)
+  work()
+)

src/projects/project-lock.test.osl

@@ -0,0 +1,34 @@
+import "std:testing"
+import "std:sync"
+import "std:thread"
+import "./project-lock.osl"
+
+def lockIsFree(string lockName) boolean (
+  boolean free = thread.new(def() -> (
+    boolean taken = sync.tryLock(lockName)
+    if taken (
+      sync.unlock(lockName)
+    )
+    return taken
+  )).wait()
+  return free
+)
+
+testing.assert(projectSaveLock("p1") == "project-history-p1", "saves share the lock the history writers take")
+
+boolean heldInside = true
+runLocked("save-lock-test", def() -> (
+  heldInside = !lockIsFree("save-lock-test")
+))
+testing.assert(heldInside, "the lock is held while the work runs")
+testing.assert(lockIsFree("save-lock-test"), "the lock is released after the work returns")
+
+def failingSave() (
+  throw "bad request body"
+)
+def lockedFailingSave() boolean (
+  runLocked("save-lock-test", failingSave)
+  return true
+)
+testing.assert(try(lockedFailingSave()).isErr(), "a failure in the work still reaches the caller")
+testing.assert(lockIsFree("save-lock-test"), "the lock is released when the work throws")

src/projects/project-team.osl

@@ -1,13 +1,16 @@
 export {}
 
-def applyCollaborators(object project, array incoming) Refusal (
+def applyCollaborators(object project, array incoming, UpdateLookups lookups) Refusal (
   array collaborators = []
   number totalShare = 0
   for i incoming.len (
+    if typeof(incoming[i]) != "object" (
+      return refuse(400, "each teammate must be an object")
+    )
     if collaborators.len < 25 (
       object entry = incoming[i].assert(object)
-      string username = normalizeUsername(entry.username.toStr())
-      string role = entry.role.toStr().toLower().strip()
+      string username = normalizeUsername((entry.username ?? "").toStr())
+      string role = (entry.role ?? "").toStr().toLower().strip()
       boolean validRole = role == "editor" or role == "maintainer" or role == "contributor" or role == "tester"
       number share = 0
       if entry.contains("share") and entry.share != null (
@@ -17,7 +20,11 @@ def applyCollaborators(object project, array incoming) Refusal (
         return refuse(400, "each teammate share must be between 0 and 50 percent")
       )
       if username != "" and validRole and normalizeUsername(username) != normalizeUsername(project.owner.toStr()) (
-        string teammateId = discoverUserIdentity(username)
+        object identity = lookupIdentity(lookups, username)
+        if identity.outcome == "unavailable" (
+          return roturUnavailableRefusal(username)
+        )
+        string teammateId = identity.id.toStr()
         if teammateId == "" (
           return refuse(400, "Rotur user not found: " ++ username)
         )