59e2bbf2d3a99e5f18436a756e1e5c8b64308e33 braney Tue Sep 8 09:21:02 2026 -0700 Copy a track collection's hub file when the program that writes it asks for a copy, instead of on every session load. cartCopyLocalHubs ran on every session load, in every CGI and in four more places in hgSession. For each customComposite-<db> cart variable it copied the collection's hub file to a fresh trash name and registered the copy in hgcentral.hubStatus to get a hub id. Nothing removed the old row. 81% of the 3.1 million rows in hubStatus on the RR are these dead registrations, and each load also took the central_hubStatus advisory lock and wrote to a MyISAM table, which serializes every concurrent load of a shared session that carries a collection. The copy itself is needed. A saved session's hub file lives under sessionDataDir and every load of that session names it, so hgCollection must not write it in place. It was just being made for every load, and almost no load is followed by an edit. hgCollection is the only program that writes one of these files, so it now asks for its own copy: main() calls cartRequestLocalHubCopy() before it opens the cart, and cartNew() makes the copy for it. That has to be a property of the program rather than of the request, and the copy has to be made at cart open, above hubConnectLoadHubs. Copying the file gives the hub a new id, the hubs are loaded during cart open, and trackList carries that id in every track name, so a copy made any later leaves printTrackDbListToHub looking for the collection under an id trackList does not have and the hub file comes out with a header and no tracks. A condition that instead tries to work out whether this particular request will write cannot be correct either: "cmd" is not in hgTracks' excludeVars, so it persists in the cart and hgCollection dispatches on the cart rather than on the CGI variable, a settings file can carry it, and a command-line run has no SCRIPT_NAME to test. Three other parts. copyLocalHubs skips a hub the cart already owns, so repeated edits reuse one file and one hub id rather than renumbering the hub on every drag; the test requires a plain file, because saveTrackFile leaves the trash name behind as a symbolic link to the durable copy and writing through that link would rewrite the saved session's own file. copyLocalHubs also screens the path with isServerUserFilePath before opening it, the way getHubName does, and selects on customComposite-<db> with the dash so that the names it acts on are the ones fileNameCartVarPrefixes screens. saveTrackFile copies when the source is a local hub outside trash that is not under this session's own directory, so loading one session and saving it under a new name gives the new session its own file instead of a reference into the first one's directory. pathIsUnderDir is no longer static in trashDir.c. All of it is behind the hg.conf gate collectionHubCopyOnWrite, default off, which reproduces the old behavior exactly. Retiring the gate is not uniform: the body of cartCopyLocalHubsOnSessionLoad is the old behavior and that function and its five callers go away with the gate, while the other three tests lose only the gate term. Measured on hgcentraltest with one binary. With the gate off, five loads of a session carrying a collection add five hubStatus rows and three later edits add none; with it on, the five loads add none and the first edit adds one. Saving a loaded session costs two rows off and one on. The resulting hub file is byte identical either way apart from the hub id, and the file the saved session names is unchanged by md5 and mtime, including after an edit made straight after a save, and including when the cart names a trash symbolic link into session storage. refs #38273 diff --git src/hg/lib/cart.c src/hg/lib/cart.c index 7a6035e03ac..10c2f554fed 100644 --- src/hg/lib/cart.c +++ src/hg/lib/cart.c @@ -552,52 +552,104 @@ { struct dyString *dy = dyStringNew(1024); int useCount; sqlDyStringPrintf(dy, "SELECT useCount FROM %s " "WHERE userName = '%s' AND sessionName = '%s';", namedSessionTable, encUserName, encSessionName); useCount = sqlQuickNum(conn, dy->string) + 1; dyStringClear(dy); sqlDyStringPrintf(dy, "UPDATE %s SET useCount = %d, lastUse=now() " "WHERE userName = '%s' AND sessionName = '%s';", namedSessionTable, useCount, encUserName, encSessionName); sqlUpdate(conn, dy->string); dyStringFree(&dy); } +boolean cartCollectionHubCopyOnWrite() +/* Return TRUE if a track collection hub file is copied when the program that writes it asks for + * a copy, rather than on every session load. hg.conf gate for #38273; drop it once the new + * behavior has been through a release. + * + * Retiring the gate is not a uniform "delete the if, keep the body": in + * cartCopyLocalHubsOnSessionLoad() the body is the OLD behavior and the whole function and its + * callers go away, while the two tests below and the one in sessionData.c lose only the gate + * term. */ +{ +return cfgOptionBooleanDefault("collectionHubCopyOnWrite", FALSE); +} + +static boolean gLocalHubCopyRequested = FALSE; + +void cartRequestLocalHubCopy() +/* Declare that this program rewrites the track collection hub file that the cart names, so that + * cartNew() replaces it with a private copy in trash before the hubs are loaded. + * + * Call this before opening the cart. It has to be a property of the program rather than of the + * request, because the copy gives the hub a new id and the hubs are loaded during cart open, + * before any CGI's doMiddle() can decide whether this particular request will write. hgCollection + * is the only caller; it is the only program that writes one of these files. refs #38273 */ +{ +gLocalHubCopyRequested = TRUE; +} + +static boolean hubFileIsOurScratchCopy(char *hubFileName) +/* Return TRUE if hubFileName is a plain file in the trash directory, which means it is a working + * copy this cart already owns and hgCollection may rewrite in place. + * + * A trash path that is a symbolic link is NOT one: saveTrackFile() replaces the trash file with a + * link to the session's durable copy when a session is saved (see sessionData.c), and writing + * through that link would rewrite the file the saved session names. Do not use realpath() here; + * these links are deliberate. */ +{ +if (!isTrashPath(hubFileName)) + return FALSE; +struct stat st; +if (lstat(hubFileName, &st) != 0) + return FALSE; +return !S_ISLNK(st.st_mode); +} + static void copyLocalHubs(struct cart *cart, struct hashEl *el) -/* Copy a set of custom composites to a new hub file. Update the +/* Copy a custom composite hub to a new hub file in trash. Update the * relevant cart variables. */ { struct tempName hubTn; char *hubFileVar = el->name; char *oldHubFileName = el->val; -if (startsWith(customCompositeCartName, el->name)) - trashDirDateFile(&hubTn, "hgComposite", "hub", ".txt"); -else if (startsWith(quickLiftCartName, el->name)) - trashDirDateFile(&hubTn, "quickLift", "hub", ".txt"); -char *newHubFileName = cloneString(hubTn.forCgi); + +// The cart is not ours: every cart variable can be set from the URL. Screen the path the same +// way getHubName() does before opening it, and leave a rejected value in the cart so the user +// can still fix it. +if (!isServerUserFilePath(oldHubFileName)) + return; // let's make sure the hub hasn't been cleaned up int fd = open(oldHubFileName, O_RDONLY); if (fd < 0) { cartRemove(cart, hubFileVar); return; } - close(fd); + +// Under copy-on-write a hub we already own is left alone. Copying it again would give the hub a +// new id on every edit, and the track names the browser is holding carry that id. refs #38273 +if (cartCollectionHubCopyOnWrite() && hubFileIsOurScratchCopy(oldHubFileName)) + return; + +trashDirDateFile(&hubTn, "hgComposite", "hub", ".txt"); +char *newHubFileName = cloneString(hubTn.forCgi); copyFile(oldHubFileName, newHubFileName); cartReplaceHubVars(cart, hubFileVar, oldHubFileName, newHubFileName); } void cartReplaceHubVars(struct cart *cart, char *hubFileVar, char *oldHubUrl, char *newHubUrl) /* Replace all cart variables corresponding to oldHubUrl (and/or its hub ID) with * equivalents for newHubUrl. */ { if (! (startsWith(customCompositeCartName, hubFileVar) || startsWith(quickLiftCartName, hubFileVar) )) errAbort("cartReplaceHubVars: expected hubFileVar to begin with '"customCompositeCartName"' " "or '"quickLiftCartName";" "but got '%s'", hubFileVar); char *errorMessage; unsigned oldHubId = hubFindOrAddUrlInStatusTable(cart, oldHubUrl, &errorMessage); unsigned newHubId = hubFindOrAddUrlInStatusTable(cart, newHubUrl, &errorMessage); @@ -643,43 +695,56 @@ { char *name = hv->name + oldNameLength; safef(buffer, sizeof buffer, "%s%d_%s", hubTrackPrefix, newHubId, name); cartSetString(cart, buffer, cloneString(hv->val)); cartRemove(cart, hv->name); } // need to change hgtgroup_hub_#hubNumber# (blue bar open ) // need to change expOrder_hub_#hubNumber#, simOrder_hub_#hubNumber# (sorting) -- values too // need to change trackHubs #hubNumber# cartSetString(cart, hgHubConnectRemakeTrackHub, "on"); cartSetString(cart, hubFileVar, newHubUrl); } -void cartCopyLocalHubs(struct cart *cart) -/* Find any custom composite hubs and copy them so they can be modified. */ +static void cartCopyLocalHubs(struct cart *cart) +/* Find any custom composite hubs and copy them so they can be modified. Under the + * collectionHubCopyOnWrite gate a hub this cart already owns is left alone; see + * copyLocalHubs(). */ { struct hashEl *el, *elList = hashElListHash(cart->hash); for (el = elList; el != NULL; el = el->next) { - // we probably shouldn't be doing this until the user actually makes a change in the collection - if (startsWith(customCompositeCartName, el->name)) + // the "-" matters: it is what fileNameCartVarPrefixes screens on, and a name that only + // starts with "customComposite" has had no path check applied to its value + if (startsWith(customCompositeCartName "-", el->name)) copyLocalHubs(cart, el); } } +void cartCopyLocalHubsOnSessionLoad(struct cart *cart) +/* Copy any custom composite hubs after loading a session. This is the pre-#38273 behavior and + * costs an hgcentral.hubStatus row per load; under the collectionHubCopyOnWrite gate it does + * nothing, because the program that writes the hub asks for its own copy instead. When the gate + * goes away, this function and every call to it go with it. */ +{ +if (!cartCollectionHubCopyOnWrite()) + cartCopyLocalHubs(cart); +} + static void storeInOldVars(struct cart *cart, struct hash *oldVars, char *var) /* Store all cart hash elements for var into oldVars (if it exists). */ { if (oldVars == NULL) return; struct hashEl *hel = hashLookup(cart->hash, var); // NOTE: New cgi vars not in old cart cannot be distinguished from vars not newly set by cgi. // Solution: Add 'empty' var to old vars for cgi vars not already in cart if (hel == NULL) hashAdd(oldVars, var, cloneString(CART_VAR_EMPTY)); while (hel != NULL) { hashAdd(oldVars, var, cloneString(hel->val)); @@ -1946,38 +2011,47 @@ warn("Unable to load session file: %s", dyMessage->string); } didSessionLoad = ok; dyStringFree(&dyMessage); } } #endif /* GBROWSE */ /* wire up the assembly hubs so we can operate without sql */ setUdcOptions(cart); if (cartVarExists(cart, hgHubDoDisconnect)) doDisconnectHub(cart); if (didSessionLoad) { - cartCopyLocalHubs(cart); + cartCopyLocalHubsOnSessionLoad(cart); // Loading a session empties the cart and then puts the CGI variables back, which // undoes the work fixUpDb did above. A Genark accession in db= has to be turned // back into a genome and a hubUrl before we connect the hubs. refs #38184 resolveGenarkDb(cart); } +// A program that rewrites the track collection hub file has asked for its own copy of it (see +// cartRequestLocalHubCopy). If the file belongs to a saved session then every load of that +// session names it and other users may load it too, so it must not be written in place. Under +// copy-on-write this is the only place the copy is made, and it has to happen here, before the +// hubs are loaded below, because the copy gets a new hub id and the track names come from it. +// refs #38273 +if (cartCollectionHubCopyOnWrite() && gLocalHubCopyRequested) + cartCopyLocalHubs(cart); + char *newDatabase = hubConnectLoadHubs(cart); if (newDatabase != NULL) { char *cartDb = cartOptionalString(cart, "db"); char *oldDb = (oldVars != NULL) ? hashFindVal(oldVars, "db") : NULL; if ((cartDb == NULL) || differentString(cartDb, newDatabase)) { // resolveGenarkDb takes db out of the cart, so a Genark db= that names the assembly // the cart was already on looks like a database change here. It is not one, and the // magic below would replace the position we just loaded from a session with the // assembly default and drop the multi-region variables. refs #38184 boolean sameDb = !IS_CART_VAR_EMPTY(oldDb) && sameString(oldDb, newDatabase);