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.

Related Pull Requests

Search NCBI for papers associated with private datasets #606

Changes

PanoramaPublicModule schedules the reminder job from startBackgroundThreads().
NcbiPublicationSearchServiceImpl.getString retries eutils requests on 5xx, read timeouts and 429.
- 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.
executeSearch throws NcbiSearchException instead of returning an empty list when a request fails.
- 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, is never rendered back into the form
- It can only be removed through the Remove the saved key checkbox, so editing the reminder settings cannot erase it.
- The key is redacted from the retry warning and from NCBI's 4xx response body before either reaches a log.
- NCBI echoes the submitted key back in its 400 body, and for the reminder job the log is the pipeline job log.
PrivateDataReminderJob checks a configured NCBI API key before iterating over datasets.
- The check is skipped when publication search is off or no key is saved.
- If the key is rejected, the job is stopped with no reminders posted.
- A 429, a 5xx or a timeout leaves the key unchecked. 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 429, a 5xx or a timeout is reported as a check that could not be completed.

Tests

NcbiPublicationSearchServiceImpl.TestCase
- Retries 5xx, timeouts and 429, 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.
- NCBI's reason reaches the log for a 4xx but not a 5xx.
- A 400 from NCBI is a rejected key, while a 429, 5xx or timeout is a check that could not be completed.
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 button reports NCBI's rejection on a dev machine, and acceptance through the mock on TeamCity.
- The Remove the saved key checkbox removes the key. 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.

+            boolean rejected = e.getStatusCode() >= 400 && e.getStatusCode() < 500
+                    && e.getStatusCode() != TOO_MANY_REQUESTS;


+    // 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.


+            if (Thread.currentThread().isInterrupted())
+            {
+                // Cancelling the job clears the NCBI rate limiter, because every sleep from here on
+                // throws at once. Stop instead of running the rest at full speed.
+                log.warn("Job was interrupted. Stopping before experiment {}.", experimentAnnotationsId);
+                break;


+                "false", getPrivateDataReminderSettings().get("ncbiApiKeySaved"));
+
+        _savedTestApiKey = true;
+        savePrivateDataReminderSettings("2", "0", "0", true, TEST_API_KEY);


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

Navigation Menu

Uh oh!

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 12 commits into
release26.7-SNAPSHOTfrom
26.7_fb_panoramapublic-reminder-fixes

vagisha commented Sep 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		boolean rejected = e.getStatusCode() >= 400 && e.getStatusCode() < 500
		&& e.getStatusCode() != TOO_MANY_REQUESTS;

Uh oh!

Conversation

vagisha commented Sep 1, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants