Repository navigation
GH Issue 1720: Run saveBatch and saveRuns transform scripts outside the save transaction - #8220
Conversation
… material handling
…e save transaction
…or background imports
…ion once per assay
…e transform output is recorded A run edited while its scripts ran now fails the save instead of overwriting the edit, duplicate runs and missing batches are rejected before any script runs, and prepared inputs are keyed by file.
…port transaction Events for files saved before the transform runs are held on the PreparedTransform; a failed import writes them only for files that still exist.
| } | ||
|
|
||
| /** Builds the upload context handleRun would build for this run, without writing anything, and runs its transform scripts. */ | ||
| private @Nullable PreparedTransform prepareTransform(ViewContext context, ExpProtocol protocol, JSONObject runJson) throws ExperimentException, ValidationException |
There was a problem hiding this comment.
I got very confused by all of these prepareTransform methods because of the similarities between the method names and the object name of the return value.
Consider renaming some of the private methods to bring out more distinction or inline one into the other.
There was a problem hiding this comment.
Agreed, the overloads were the worst of it. The private pair is gone: prepareTransforms now reads each posted run once (readPostedRun) and hands it to runTransformScripts, which calls runCreator.prepareTransform, so that name now refers only to the run creator's method and the PreparedTransform it returns. I also renamed the snapshot helpers (SCRIPT_INPUT_KEYS, setScriptInputs/scriptInputsMatch) so "prepared" only refers to PreparedTransform objects.
| /** Whether the run, read inside the transaction, still holds what its scripts saw before it. */ | ||
| private boolean matchesPrepared(JSONObject runJson, @Nullable JSONObject serializedRun) | ||
| { | ||
| JSONObject seen = _preparedSerializations.get(runJson); |
There was a problem hiding this comment.
Could some of this tracking and validation of the runs be handled inside of PreparedTransform?
There was a problem hiding this comment.
Moved the snapshot of what the scripts read onto PreparedTransform along with the check against it, so the handler no longer keeps a second map keyed by run JSON. PreparedTransform also now keeps only the inputs the participant visit resolver added, so the save no longer strips the posted inputs out and merges them back in. Which parts of the run JSON get snapshotted stays in the handler, since it depends on the save API's JSON shape and PreparedTransform also serves the non-JSON import paths.
| } | ||
|
|
||
| /** For an import that failed before recording its held audits; cleanup may already have deleted some of the files. */ | ||
| public static void addFileAuditEventsForRemainingFiles(@Nullable User user, @NotNull List<PendingFileAudit> audits) |
There was a problem hiding this comment.
Seems odd to me that auditing is handled here, although I can't think of a better place to track events.
There was a problem hiding this comment.
It's here because PreparedTransform is the one object that spans from before the import transaction opens until after it commits or rolls back: the events have to be held until the transaction exists, recorded inside it, and written on close() if the import never gets that far, the same lifecycle as the scripts' working directories. I did promote PendingFileAudit to a top-level type so the file writer and upload context don't reference it through PreparedTransform.
|
@labkey-klum I pushed edits. I had Claude add replies to your specific comments, which are wordier than I would have liked, but overall I tried to improve naming and consolidate a bit more. See what you think. |
labkey-klum
left a comment
labkey-klum
left a comment
There was a problem hiding this comment.
Thanks, the last pass cleaned up some of those readability / complexity issues.
… folder delete (#3265) ## Rationale Adds test coverage for [GH Issue 1720](LabKey/internal-issues#1720): saving assay batches or runs through the API must not hold a database transaction while transform scripts run. ## Related Pull Requests - LabKey/platform#8220 ## Changes - saveBatch and saveRuns variants of the folder-delete callback test - A batch whose second run's script fails saves nothing and cleans up the scripts' working directories - Updating an existing run keeps its results when another run in the request fails, and replaces them when the save succeeds --------- Co-authored-by: Lum <klum@labkey.com>
…he save transaction (#8220) ## Rationale The assay batch and run save APIs now run Standard assay transform scripts before opening their database transaction, so a script that calls back into the server can no longer deadlock with other work such as a folder delete ([GH Issue 1720](LabKey/internal-issues#1720)). This extends the GH Issue 1707 fix for single-run imports to requests that save several runs together: every run's scripts finish before the shared transaction opens, and if any script fails nothing is written. It also fixes a regression from #8183 that broke `BiologicsAssayFilesTest.testImportAssayWithFileFieldData` ([TeamCity failure](https://teamcity.labkey.org/buildConfiguration/LabKey_Trunk_Premium_ProductSuites_Biologics_BiologicsBPostgres/4231858)): when an assay has a transform script, the uploaded data file's audit event was recorded outside the import transaction and lost its transaction ID. ## Related Pull Requests - LabKey/testAutomation#3265 ## Changes - Save handlers can look up a request's data and materials without creating them - saveBatch and saveRuns run each run's transform scripts in a read-only first pass, then save the runs inside the transaction and record the scripts' output - A run whose data changes while its scripts run fails the save and asks the client to retry - saveBatch and saveRuns reject a request that lists the same run more than once - Standard assays configured for background upload get the same behavior through these APIs - Custom file-based assay save handlers keep running scripts inside the transaction - Audit events for files uploaded before a transform script runs are recorded in the import's transaction again; a failed import records them only for files it keeps Tested locally: AssayTransformCallbackTest passes, including the new saveBatch and saveRuns cases and the new file audit cases. --------- Co-authored-by: Lum <klum@labkey.com>
Rationale
The assay batch and run save APIs now run Standard assay transform scripts before opening their database transaction, so a script that calls back into the server can no longer deadlock with other work such as a folder delete (GH Issue 1720). This extends the GH Issue 1707 fix for single-run imports to requests that save several runs together: every run's scripts finish before the shared transaction opens, and if any script fails nothing is written.
It also fixes a regression from #8183 that broke
BiologicsAssayFilesTest.testImportAssayWithFileFieldData(TeamCity failure): when an assay has a transform script, the uploaded data file's audit event was recorded outside the import transaction and lost its transaction ID.Related Pull Requests
Changes
Tested locally: AssayTransformCallbackTest passes, including the new saveBatch and saveRuns cases and the new file audit cases.