c3788110e8d865774fff8197cf5b47350472ba64 chmalee Tue Aug 4 11:26:55 2026 -0700 Fix hubSpace hook error handling and nested hub path handling, refs #37964 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> diff --git src/hg/hgHubConnect/hooks/pre-finish.c src/hg/hgHubConnect/hooks/pre-finish.c index 751c6ecd3f1..a414861b735 100644 --- src/hg/hgHubConnect/hooks/pre-finish.c +++ src/hg/hgHubConnect/hooks/pre-finish.c @@ -115,30 +115,40 @@ errAbort("Invalid genome name '%s': only letters, digits, '.', '_' and '-' are allowed", db); } reqLm = jsonQueryString(req, "", "Event.Upload.MetaData.lastModified", NULL); if (reqLm) lastModified = sqlLongLong(reqLm) / 1000; // yes Javascript dates are in millis else lastModified = time(NULL); // fallback to current time if not provided parentDir = jsonQueryString(req, "", "Event.Upload.MetaData.parentDir", NULL); // must match what pre-create did to this value, or we build a different path parentDir = normalizeParentDir(parentDir); fprintf(stderr, "parentDir = '%s'\n", parentDir ? parentDir : "(null)"); // strip out plain leading '.' and '/' components // middle '.' components are dealt with later if (parentDir && (startsWith("./", parentDir) || startsWith("/", parentDir))) parentDir = skipBeyondDelimit(parentDir, '/'); + // check the value we are about to build paths from, after the strip. pre-create + // applies these same checks, but an upload created before they existed can + // still finish here, and a hub name of "", ".", ".." or "/" ends up pointing + // outside the hub + if (isEmpty(parentDir)) + errAbort("No hub name for this upload, please give the hub a name"); + if (!isValidParentDir(parentDir)) + errAbort("Hub name '%s' can only contain letters, numbers, periods and " + "underscores, in '/' separated components. Please rename the hub.", + parentDir); tusFile = jsonQueryString(req, "", "Event.Upload.Storage.Path", NULL); tusInfo = jsonQueryString(req, "", "Event.Upload.Storage.InfoPath", NULL); if (fileName == NULL) { errAbort("No filename found in upload metadata (checked fileName, filename, and name)"); } else if (tusFile == NULL) { errAbort("No Event.Path setting"); } else { userDataDir = dataDir = getDataDir(userName); // if parentDir provided we are throwing the files in there if (parentDir) @@ -183,94 +193,110 @@ // which was causing confusion in the UI code char *canonicalPath = realpath(tusFile, NULL); if (canonicalPath != NULL) row->location = canonicalPath; else { // all upload data should have been received and thus the realpath // should not fail, but just in case, put something valid here row->location = tusFile; } row->md5sum = md5HexForFile(row->location); row->parentDir = encodedParentDir ? encodedParentDir : ""; // Derive hubType server-side; never trust the client's hubType. // A 2bit always promotes its hub to assembly. Otherwise inherit // the existing hub's type, defaulting to trackHub. - char *parentDirForCheck = encodedParentDir ? hubNameFromPath(encodedParentDir) : ""; + // both lookups below are about the hub as a whole, whose row and hub.txt + // live at the top level, so use the hub component of parentDir + char *parentDirForCheck = encodedParentDir ? hubRootFromParentDir(encodedParentDir) : ""; if (sameOk(fileType, "2bit")) row->hubType = "assemblyHub"; else { char *existingType = existingHubTypeForDir(userName, parentDirForCheck); row->hubType = existingType ? existingType : "trackHub"; } char *batchHasHubTxtStr = jsonQueryString(req, "", "Event.Upload.MetaData.batchHasHubTxt", NULL); boolean batchHasHubTxt = sameOk(batchHasHubTxtStr, "true"); boolean userOwnNamedHubTxt = userHasOwnNamedHubTxtInDir(userName, parentDirForCheck); boolean userAuth = batchHasHubTxt || userOwnNamedHubTxt; boolean isHubTxt = sameOk(fileType, "hub.txt"); boolean isTwoBit = sameOk(fileType, "2bit"); // Serialize hub.txt read-modify-write across parallel pre-finish // processes for the same hub. flock is held for the entire // decision + action so writeHubText's fileExists check and the // upgrade's read-rewrite are atomic with respect to siblings. - // Without a parentDir there is no hub to protect. - int hubLockFd = encodedParentDir ? lockHubDir(dataDir) : -1; + // Lock the directory the hub.txt is in, so uploads into different + // subdirectories of one hub still serialize against each other. + int hubLockFd = -1; + if (encodedParentDir) + { + char *hubDir = hubPathFromParentDir(encodedParentDir, userDataDir); + hubLockFd = lockHubDir(hubDir); + freeMem(hubDir); + } if (!isHubToolsUpload && !isHubTxt) { if (!userAuth) { if (isTwoBit) { - if (!literalHubTxtExistsOnDisk(parentDirForCheck, userDataDir)) - createNewTempHubForUpload(reqId, row, userDataDir, encodedParentDir); + // createNewTempHubForUpload is a no-op when the hub.txt and its + // row are already there, and it backfills the row when they are not + createNewTempHubForUpload(reqId, row, userDataDir); upgradeExistingHubToAssembly(row, userDataDir, encodedParentDir); } else - createNewTempHubForUpload(reqId, row, userDataDir, encodedParentDir); + createNewTempHubForUpload(reqId, row, userDataDir); } else if (isTwoBit) { // user's hub.txt is authoritative; just flip rows to assemblyHub. upgradeExistingHubToAssembly(row, userDataDir, encodedParentDir); } } - unlockHubDir(hubLockFd); + // still under the hub lock: makeParentDirRows checks for a row and then + // inserts it, so two uploads to one hub would otherwise both insert the + // same directory row // first make the parentDir rows makeParentDirRows(row->userName, sqlDateToUnixTime(row->lastModified), row->db, row->parentDir, userDataDir, row->hubType); row->parentDir = encodedParentDir ? hubNameFromPath(encodedParentDir) : ""; addHubSpaceRowForFile(row); + unlockHubDir(hubLockFd); fprintf(stderr, "added hubSpace row for file '%s'\n", fileName); fflush(stderr); } } // pop the handlers before handling the error, the cleanup below can itself // errAbort, which would longjmp back into this same block errCatchEnd(errCatch); if (errCatch->gotError) { // App-level reject: exit 0 + RejectUpload=true is the tusd protocol for // forwarding HTTPResponse verbatim. Non-zero gets wrapped. - rejectUpload(response, errCatch->message->string); - // must remove the tusd temp files so if the users tries again after a temp error - // the upload will work - if (tusFile) - { - mustRemove(tusFile); - mustRemove(tusInfo); - } + rejectUpload(response, "%s", errCatch->message->string); + // clear the partial upload so the user can try again. pre-create hands tusd a + // ChangeFileInfo, so tusFile is the file in the user's directory, not a temp + // copy, and tusInfo is tusd's .info alongside it. remove() rather than + // mustRemove(): this is the error path, and an abort here exits before the + // response is printed, leaving the client with a bare 500 instead of the + // reason the upload failed + if (tusFile && remove(tusFile) != 0) + fprintf(stderr, "could not remove '%s': %s\n", tusFile, strerror(errno)); + if (tusInfo && remove(tusInfo) != 0) + fprintf(stderr, "could not remove '%s': %s\n", tusInfo, strerror(errno)); // TODO: if the first mysql request in createNewTempHubForUpload() works but then // either of makeParentDirRows() or addHubSpaceRowForFile() fails, we need to also // drop any rows we may have added because the upload didn't full go through exitStatus = 0; } } // always print a response no matter what jsonPrintToFile(response, NULL, stdout, 0); return exitStatus; } int main(int argc, char *argv[]) /* Process command line. */ { optionInit(&argc, argv, options);