Repository navigation
[bot] Fast-forward for 26.10.0 - #679
Merged
Merged
Conversation
…failure handling (#675) ## Rationale - The Private Data Reminder job stopped firing on production after a Tomcat restart. - NCBI eutils can return a 5xx for requests, even when they are well under the rate limit. A failed search was indistinguishable from a dataset with no published paper. - The job status reported COMPLETE even when there were errors in sending reminders to one or more datasets. ## Related Pull Requests - #606 ## Changes - `PanoramaPublicModule` schedules the reminder job from `startBackgroundThreads()`. - `NcbiHttpClient.getString` retries eutils requests on 5xx, 429, read timeouts, connection resets and closed connections. - Up to three attempts, waiting 500ms before the second and 1000ms before the third. - HttpClient's own retry layer is disabled so the attempt count means what it says. - `searchForPublication` throws `NcbiSearchException` only when no publication was found and at least one request failed, so an empty result still means no paper was found. - The reminder job records the failure against the experiment and still posts the reminder. - The two controller actions report an error rather than "no publications found". - Added an optional NCBI API key to _Private Data Reminder Settings_, raising the eutils rate ceiling from 3 to 10 requests / second. - It is held in the encrypted property store and is never rendered back into the form. - It can only be removed through the **Remove the saved key** checkbox, which is displayed only when a key is saved. - The form rejects a new key submitted with **Remove the saved key** checked. - Save checks a new key with NCBI and saves it only if NCBI accepts it. A rejected key and a check that could not be completed are each refused, with the reason on the form. - The key is redacted from the retry warning and from NCBI's 4xx response body before either reaches a log. - `PrivateDataReminderSettings.get()` runs without an NCBI API key when the server has no encryption key configured, or when the saved key cannot be decrypted, instead of throwing. - With no encryption key, the settings page displays a note in place of the key controls, and a submitted key is rejected before anything is saved. - A key that cannot be decrypted is replaced when an admin saves a new one. - `PrivateDataReminderJob` checks a configured NCBI API key before iterating over datasets. - If the key is rejected, the job is stopped with no reminders posted. - Only a 400 is a rejection. When the check fails any other way, including a timeout, the job logs a warning and runs with the key anyway. - Added a **Validate** button that checks a key against NCBI before it is saved. - The result is displayed beside the field with the rejection details behind a **Details** link. - A status that is not 400 is reported as a check that could not be completed. - `PrivateDataReminderJob.run` reports a status computed from what the job recorded, rather than always reporting complete. An ERROR logged through the job's logger already sets the status to error, and the status set at the end was overwriting it. - ERROR means the job could not start, or a reminder that should have posted for a dataset was not. - A failed publication search is a WARN, since the reminder is still posted. It reports an error when the search failed for half or more of the datasets it ran for, or was stopped. - A missing support message thread is an error only when the submission has an announcement id. Older datasets, submitted before submission requests were posted to a message board, do not have an announcement id. - `PrivateDataReminderJob` stops the publication search for the rest of a run after 3 datasets in a row where every NCBI request failed. Reminders are still posted, the run ends in ERROR, and the end-of-run summary lists the datasets that were not searched. - Added `RestorePrivateDataReminderSettingsAction`, a dev-mode test support action. `NcbiApiKeyTest`, `PublicationSearchTest` and `PrivateDataReminderTest` restore the reminder settings through it in `@After`, without loading a page. `PrivateDataReminderTest` did not restore them before. ## Tests - `NcbiHttpClient.TestCase` - Retries 5xx, 429, timeouts, connection resets and closed connections, but not other 4xx. - A persistent 5xx is tried 3 times with 500ms then 1000ms backoff, then rethrown. - An interrupted thread stops after one attempt. - The key is stripped from the logged URL and from an echoed error body, including a key that crosses the point where the body is shortened. - NCBI's reason reaches the log for a 4xx but not a 5xx. - `NcbiPublicationSearchServiceImpl.TestCase` - A 400 from NCBI is a rejected key, while a 403, 404, 429, 5xx or timeout is a check that could not be completed. - The key check goes through the retry loop, trying a 503 three times and a 400 once. - A search reports whether every NCBI request failed or only some. - The mock NCBI service returns registered data through `NcbiHttpClient`. - `PrivateDataReminderJob.TestCase` - The error count used for the job status, and the half-or-more failure rate threshold. - A missing support thread counts as an error only for a submission with an announcement id. - The search stops after 3 datasets in a row where every request failed, and a completed or partly failed search resets the count. - `NcbiApiKeyTest` - new Selenium test for the NCBI API key settings. - Uses the mock NCBI service on every server. Save refuses a key the mock rejects with a 400 and a key it cannot check because of a 503. - Fails with instructions if a key is already saved on the server, rather than overwriting one it cannot restore. - The saved key is never rendered into the form, and saving with the field blank keeps the stored key. - **Validate** reports a key the mock accepts as accepted, and a key it rejects as rejected. - The **Remove the saved key** checkbox removes the key, and is no longer displayed afterwards. - Restores the settings and removes its own key in `@After` through an API call, so the failure screenshot shows the page that failed. - `PrivateDataReminderSettings.TestCase` - the extension and reminder boundary tests now take the current date from the expiry the settings calculate, so they no longer fail on the last days of a month. Co-authored-by: Claude <noreply@anthropic.com>
cnathe
approved these changes
Oct 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Generated automatically.
Approve all matching PRs simultaneously.
Approval will trigger automatic merge.
View all PRs: https://internal.labkey.com/Scrumtime/Backlog/harvest-gitOpenPullRequests.view?branch=26.10_ff_bot_26.10.0