Fixed the private data reminder schedule and NCBI publication search failure handling by vagisha · Pull Request #675 · LabKey/MacCossLabModules

vagisha · 2026-09-01T17:49:34Z

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

Search NCBI for papers associated with private datasets #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.
- 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 only when the search failed for half or more of the datasets it ran for.
- 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.
Cancel in the pipeline UI stops PrivateDataReminderJob before the next dataset.

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.
- The mock used on TeamCity 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.
NcbiApiKeyTest - new Selenium test for the NCBI API key settings.
- 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 displays the check result. Against live NCBI a fake key is not reported as accepted. Against the mock used on TeamCity it is reported as accepted.
- The Remove the saved key checkbox removes the key, and is no longer displayed afterwards. Removes its own key in @After even if the test fails partway.
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

…on search - PanoramaPublicModule.startupAfterSpringConfig now re-establishes the daily reminder's Quartz schedule on startup; it was only set when an admin saved the settings form, so a Tomcat restart silently killed the job (Quartz's in-memory job store does not survive a JVM restart). * Added an "NCBI API key" admin field wired into buildCommonParams as &api_key= (raises NCBI rate limit 3->10 req/sec); contextual Logger threaded through getString/getJson so retry warnings follow the job log vs. server log. - NcbiPublicationSearchServiceImpl.getString retries eutils calls (3 attempts, 500/1000ms backoff) on 5xx and read timeouts (~40% transient failure rate); 4xx fails fast, with the NCBI response body included so a bad key's "API key invalid" reaches the log instead of a bare 400. - Added JUnit (retry/backoff, api_key, 4xx body) and Selenium API-key coverage. Follow-up to PR #606.

…ore the bad-key search so incidental NCBI 5xx in earlier steps don't make checkExpectedErrors flaky on dev

…PublicBaseTest's reminder-settings helper

…rm/id query params instead of substring-scanning the whole request URL

…ve-up-after-MAX_HTTP_ATTEMPTS, no-retry-on-4xx); made executeGet protected so a test subclass can drive it - Corrected stale comments in NcbiPublicationSearchServiceImpl and its mock, and tightened the others.

* Redacted the API key from the retry warning and from NCBI's 4xx body before either reaches a log * Moved the key to the encrypted property store, and stopped the form displaying or erasing it * Threw NcbiSearchException from executeSearch so a rejected key no longer reads as a dataset with no paper * Added a Validate button that checks a key against NCBI before it is saved * Scheduled the reminder job from startBackgroundThreads so a SchedulerException cannot fail server startup * Added redaction unit tests and reworked the Selenium NCBI API key coverage Co-Authored-By: Claude <noreply@anthropic.com>

@after

* Added NcbiApiKeyTest, which fails with instructions rather than skipping when a key is already saved * Covered the Validate button on TeamCity through the mock NCBI service, so nothing is skipped there * Removed the key coverage from PublicationSearchTest, which no longer saves or removes a site-wide key * Removed the test's own key in @after so a failed run cannot leave one that breaks every later search Co-Authored-By: Claude <noreply@anthropic.com>

… error bodies * Retried 429, which is NCBI's answer when the request rate is exceeded and was failing fast as a 4xx * Disabled HttpClient's own retries, which doubled the requests MAX_HTTP_ATTEMPTS names on a persistent 503 * Stopped retrying once the thread is interrupted, since every later sleep throws at once and drops the backoff * Stopped the reminder job starting another experiment after interruption, which would run with no rate limit * Bounded the error body read and caught its ParseException, which could otherwise discard the HTTP status Co-Authored-By: Claude <noreply@anthropic.com>

* Took the current date from the expiry the settings calculate, rather than from today plus an offset * On 31 August, six months back then forward again gives 28 August, so the expiry fell before the date checked * Removed the currentDate and minutesOffset parameters that the change left always null and zero * Left the production arithmetic alone, since adding months and clamping to a shorter month is correct * Corrected two test comments, one naming a map key that was renamed and one naming the wrong cleanup method Co-Authored-By: Claude <noreply@anthropic.com>

* PrivateDataReminderJob checks a configured key before any dataset, and errors without posting when NCBI rejects it * checkApiKey replaces validateApiKey and reports VALID, REJECTED or UNCONFIRMED, so an NCBI outage does not stop the run * The Validate button now reports a request that never reached NCBI separately from a rejected key * The end-of-run message no longer tells an admin to check the key when the failure was something else * Added a unit test for the classification, driven through an executeGet override so it needs no network Co-Authored-By: Claude <noreply@anthropic.com>

* sleepMs and rateLimit became instance methods, so a test subclass can replace the waiting * The retry tests now assert the delays the loop used, 500ms then 1000ms, which retryDelayMs alone cannot show * The 4xx test asserts the loop did not wait at all Co-Authored-By: Claude <noreply@anthropic.com>

* checkApiKey reports a 429 as unconfirmed rather than rejected Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Pull request overview

Improves Panorama Public reminder scheduling and NCBI publication-search reliability.

Changes:

Restores reminder scheduling during application startup.
Adds NCBI retries and explicit search-failure handling.
Adds encrypted API-key configuration, validation, and tests.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`PublicationSearchTest.java`	Preserves reminder settings during testing.
`PanoramaPublicBaseTest.java`	Adds API-key settings test helpers.
`NcbiApiKeyTest.java`	Tests API-key management and validation.
`privateDataRemindersSettingsForm.jsp`	Adds API-key controls and feedback.
`PrivateDataReminderJob.java`	Handles key checks and search failures.
`PanoramaPublicModule.java`	Schedules reminders during startup.
`PanoramaPublicController.java`	Adds key validation and persistence.
`NcbiSearchException.java`	Distinguishes failed searches.
`NcbiPublicationSearchServiceImpl.java`	Adds retries, redaction, and key support.
`NcbiPublicationSearchService.java`	Extends the NCBI service contract.
`NcbiApiKeyCheck.java`	Models key-validation outcomes.
`MockNcbiPublicationSearchService.java`	Updates mocked request handling.
`PrivateDataReminderSettings.java`	Stores encrypted keys and stabilizes date tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

vagisha · 2026-09-08T06:05:57Z

+    // 500ms after the first failure, then doubling. Jitter is not needed. Both callers, the daily
+    // reminder job and the UI search, issue NCBI requests sequentially, so a retry never collides
+    // with a sibling request.


You are right. The UI search actions are request handlers and can overlap each other or the reminder job, so the comment claiming a retry never collides with a sibling request was wrong and has been corrected in bf13cb4

Not adding jitter. These are site-admin actions so collisions will be rare.

… job * A failed NCBI request no longer abandons the remaining PMC strategies or the PubMed fallback * NcbiPublicationSearchServiceImpl.checkApiKey treats only HTTP 400 as a rejected key * PrivateDataReminderJob.run reports cancelled or error instead of complete when it stops early * Failed NCBI requests log at WARN, leaving ERROR for the per-dataset and run summary * NcbiApiKeyTest reads a new data-key-saved attribute rather than the field placeholder * NcbiApiKeyTest restores the settings it overwrites and no longer requires NCBI to reject a key * Corrected comments and javadoc, and small consistency cleanups Co-Authored-By: Claude <noreply@anthropic.com>

…om log levels * An ERROR through the job's logger sets the pipeline status, and the status set at the end overwrote it. run now computes it from what ProcessingResults recorded * ERROR means the job could not start or a dataset was not sent a reminder. A failed publication search is WARN, since the reminder still goes out * A publication search failing for more than half the searches that reached NCBI is an ERROR * A missing support thread is an error only when the submission has an announcement id. The rest predate the message board and can never be reminded * An unexpected exception on a dataset is now recorded and counted * A citation that will not parse logs at WARN, since it does not fail the match * Added PrivateDataReminderJob.TestCase for the error count, the failure rate boundary, and the announcement id split Co-Authored-By: Claude <noreply@anthropic.com>

* PrivateDataReminderSettingsAction rejects a new key entered with "Remove the saved key" checked. The typed key used to be discarded while the page reported success * The settings form reports a saved key after a validation error, not only on first display * PrivateDataReminderSettings.get reads the key whether or not other reminder settings are saved * NcbiPublicationSearchServiceImpl.isRetryable retries a SocketException (connection reset) * publicationSearchFailingWidely triggers at half or more of the searches, not more than half * Renamed the PrivateDataReminderSettings.TestCase boundary helpers and dropped an unused argument * Reworded comments in the NCBI classes, the reminder job and the Selenium tests. US spelling Co-Authored-By: Claude <noreply@anthropic.com>

* PrivateDataReminderSettings.getNcbiApiKeyValue returns no key when the server has no encryption key configured. Reading the encrypted store threw, which failed every call to PrivateDataReminderSettings.get, including extension requests and scheduling the job at startup * saveNcbiApiKey still throws, so an admin saving a key sees the configuration error Co-Authored-By: Claude <noreply@anthropic.com>

* PrivateDataReminderJob overrides canInterrupt, so Cancel sets the flag checkInterrupted returns. Before, the loop checked only the thread interrupt flag, which Cancel never sets * processExperiments checks checkInterrupted before each dataset and stops, reporting cancelled Co-Authored-By: Claude <noreply@anthropic.com>

…tings page * NcbiPublicationSearchServiceImpl.isRetryable retries NoHttpResponseException (connection closed) * errorDetail removes the API key from a 4xx response body before shortening it for the log. Shortening first could cut the key in two and leave its first characters in the log * PrivateDataReminderJob holds the per-dataset transaction only around the post and the DatasetStatus update, not while the NCBI requests run * PrivateDataReminderJob checks the API key after the empty-list return, so a run with no datasets sends no request to NCBI * PrivateDataReminderJob.TestCase turns its logger off, so its ERROR lines stay out of the server log * The NCBI API key field uses autocomplete="new-password", so a browser does not fill in a saved password * "Remove the saved key" is displayed only when a key is saved * savePrivateDataReminderSettings asserts a saved key only for a non-blank key argument Co-Authored-By: Claude <noreply@anthropic.com>

* PrivateDataReminderSettingsAction.validateCommand rejects a new key or a key removal when no encryption key is configured. Before, the other reminder settings were saved, then saving the key threw, and the reminder schedule was not reapplied * The settings page displays a note in place of the key field, the Validate button and the remove checkbox, so Validate no longer reports a key as accepted that cannot be saved Co-Authored-By: Claude <noreply@anthropic.com>

* NcbiHttpClient holds the request, retry, error message and API key redaction code and constants that were in NcbiPublicationSearchServiceImpl, with their unit tests. No behavior change * NcbiPublicationSearchServiceImpl sends its requests through an NcbiHttpClient * MockNcbiPublicationSearchService supplies an NcbiHttpClient that returns canned responses from executeGet, so the request loop in NcbiHttpClient.getString runs in the Selenium tests on TeamCity * Added a unit test that checks the mock returns registered data through NcbiHttpClient Co-Authored-By: Claude <noreply@anthropic.com>

* PrivateDataReminderSettings reads no key when the saved key cannot be decrypted, for example after the encryption key changed. saveNcbiApiKey deletes such a key and saves the new one * ValidateNcbiApiKeyAction responds with the configuration message when no encryption key is configured. The message now says a key cannot be saved or removed * checkApiKey takes the caller's logger, so the reminder job's key check retry warnings go to the job log * testCheckApiKey counts requests, so it fails if the service stops going through the retry loop * NcbiHttpClient has no logger of its own. Its tests use a test logger * PanoramaPublicBaseTest.getPrivateDataReminderSettings reports no saved key when the key field is not displayed Co-Authored-By: Claude <noreply@anthropic.com>

labkey-jeckels

A few suggestions, largely compliments of Claude.

labkey-jeckels · 2026-09-30T17:43:15Z

+        NcbiApiKeyCheck check = NcbiPublicationSearchService.get().checkApiKey(apiKey, getLogger());
+        if (check.isRejected())
+        {
+            getLogger().error("NCBI rejected the API key, so no reminders were posted. Correct the key on the Private Data Reminder Settings page and run the job again. {}",


Optiona: A rejected API key stops the whole job, so no reminders go out, even though the key is optional and reminders don't depend on the publication search. A typo in a key saved without clicking Validate, or a key revoked or rotated at NCBI, would make every daily run end in ERROR with nothing posted until an admin notices. Consider we log the rejection as an error and then keep going, either searching without the key or skipping the search? Validating the key in validateCommand when it's saved would also catch typos before they reach the job

labkey-jeckels · 2026-09-30T17:44:01Z

+    }
+
+    @After
+    public void removeTestApiKey()


This @After navigates (loads the settings page, checks the checkbox, clicks Save), which the test guidelines rule out because it keeps the base class from capturing the failure screenshot. It can also mask the real failure: _savedTestApiKey is set at line 68, before the save, so if the save fails, checkCheckbox("clearNcbiApiKey") throws here on a checkbox that isn't rendered. Could the key removal and settings restore go through an API call instead (or into doCleanup), with _savedTestApiKey set only after the save succeeds?

labkey-jeckels · 2026-09-30T17:45:25Z

            log.info("RUNNING IN TEST MODE - MESSAGES WILL NOT BE POSTED.");
        }
+        boolean completed = true;
        for (Integer experimentAnnotationsId : exptIds)


When NCBI is down or hanging, each request can now take about 30s before it gives up (3 attempts × 10s timeouts in NcbiHttpClient, plus backoff), and each dataset makes several requests (the PMC strategies, the metadata fetch, the PubMed fallback, citations). publicationSearchFailingWidely is only checked after the loop, so a run over many due datasets could take hours and hold the pipeline queue. Could the job stop searching (while still posting reminders) once some number of consecutive datasets fail their search, or once the failure rate crosses the threshold partway through?

* PrivateDataReminderSettingsAction.validateCommand 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 each display a reason on the form and log a WARN * MockNcbiPublicationSearchService responds to two designated keys with a 400 and a 503 * NcbiApiKeyTest uses the mock on every server and checks that neither key is saved Co-Authored-By: Claude <noreply@anthropic.com>

@after

…h an API call * Added PanoramaPublicController.RestorePrivateDataReminderSettingsAction, a dev-mode test support action that sets the reminder settings passed in and can remove a saved NCBI API key * NcbiApiKeyTest and PublicationSearchTest restore settings in @after through the action, so the cleanup loads no page and the failure screenshot shows the page that failed * NcbiApiKeyTest.restoreAfterTest sets clearNcbiApiKey when the settings captured at the start of the test report no saved key, replacing the _savedTestApiKey flag. The action removes the key only if PrivateDataReminderSettings.hasNcbiApiKey() returns true * PrivateDataReminderTest restores the reminder settings it changes, which it did not before * Renamed requireDevModeForMockNcbiService to requireDevModeForTestSupport Co-Authored-By: Claude <noreply@anthropic.com>

…not respond * NcbiSearchException reports whether every NCBI request for the search failed. NcbiPublicationSearchServiceImpl sets it when no PMC or PubMed search request completed * PrivateDataReminderJob stops searching for the rest of the run after 3 datasets in a row whose requests all failed, and still posts the reminders. A completed or partly completed search resets the count * The run ends in ERROR when the search was stopped, and the end-of-run summary lists the experiments that were not searched * Added PrivateDataReminderJob.TestCase.testPublicationSearchStopped and NcbiPublicationSearchServiceImpl.TestCase.testSearchReportsWhetherAllRequestsFailed Co-Authored-By: Claude <noreply@anthropic.com>

vagisha and others added 12 commits August 28, 2026 12:07

Captured PublicationSearchTest's server error-count baseline just bef…

02022c4

…ore the bad-key search so incidental NCBI 5xx in earlier steps don't make checkExpectedErrors flaky on dev

Asserted enablePublicationSearch round-trips through save in Panorama…

21a0cc3

…PublicBaseTest's reminder-settings helper

Scoped MockNcbiPublicationSearchService matching to the decoded db/te…

3c4bdfd

…rm/id query params instead of substring-scanning the whole request URL

Added unit tests for the getString retry loop (retry-then-succeed, gi…

0c2058f

…ve-up-after-MAX_HTTP_ATTEMPTS, no-retry-on-4xx); made executeGet protected so a test subclass can drive it - Corrected stale comments in NcbiPublicationSearchServiceImpl and its mock, and tightened the others.

Stopped a 429 from the API key check looking like a rejected key

151992f

* checkApiKey reports a 429 as unconfirmed rather than rejected Co-Authored-By: Claude <noreply@anthropic.com>

vagisha requested a balanced review from Copilot September 1, 2026 17:49

Copilot started reviewing on behalf of vagisha September 1, 2026 17:50 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

vagisha and others added 9 commits September 7, 2026 15:24

vagisha marked this pull request as ready for review September 28, 2026 15:39

vagisha requested a review from labkey-jeckels September 28, 2026 15:40

labkey-jeckels approved these changes Sep 30, 2026

View reviewed changes

vagisha and others added 3 commits October 4, 2026 18:15

Navigation Menu

Fixed the private data reminder schedule and NCBI publication search failure handling - #675

Fixed the private data reminder schedule and NCBI publication search failure handling#675
vagisha wants to merge 24 commits into
release26.7-SNAPSHOTfrom
26.7_fb_panoramapublic-reminder-fixes

vagisha commented Sep 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

vagisha Sep 8, 2026

Uh oh!

Uh oh!

Uh oh!

labkey-jeckels left a comment

Uh oh!

labkey-jeckels Sep 30, 2026

Uh oh!

labkey-jeckels Sep 30, 2026

Uh oh!

labkey-jeckels Sep 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

vagisha commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Related Pull Requests

Changes

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

vagisha Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

labkey-jeckels left a comment

Choose a reason for hiding this comment

Uh oh!

labkey-jeckels Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

labkey-jeckels Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

labkey-jeckels Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vagisha commented Sep 1, 2026 •

edited

Loading