update CLI: --maxUpdateWaitTimeMs is never applied, WaitForInstallDoneStepExecutor has no timeout and testTimeoutInstallUpdateArchive is disabled

[vharseko]

Summary

The update CLI command accepts --maxUpdateWaitTimeMs ("Timeout value to wait for update process to complete. Defaults to 30000 ms.", RemoteCommandScope.java:233-237) and carries it into UpdateCommandConfig.maxUpdateWaitTimeMs (:24, :261), but nothing ever reads it: getMaxUpdateWaitTimeMs() is referenced only from a Javadoc @see (UpdateCommand.java:710). WaitForInstallDoneStepExecutor polls the update log until a terminal status with no time limit at all, so the option is a silent no-op and the CLI can wait forever.

Found while analysing #219 (fixed by #221); kept separate because it is a different loop and a different decision.

Root cause

WaitForInstallDoneStepExecutor.execute (UpdateCommand.java:737-759):

while (!TERMINAL_STATE.contains(status)) {
    log("Update procedure is still processing...");
    Thread.sleep(config.getCheckCompleteFrequency());
    ...
    status = response.getContent().get("status").defaultTo(UPDATE_STATUS_IN_PROGRESS).asString().toUpperCase();
}
if (TERMINAL_STATE.contains(status)) {
    ...
    return ExecutorStatus.SUCCESS;
} else {
    log("The update process failed to complete within the allotted time.  " +
            "Please verify the state of OpenIDM.");
    return ExecutorStatus.FAIL;
}

The else branch is unreachable: the loop only exits when status is terminal. The startTime read from state.getStartInstallTime() (:728) is validated (startTime <= 0 → IllegalStateException) but never used for anything else.

This is not a regression on our side: upstream commit 9cc8612fd (OPENIDM-5529, 2016-04-06) removed the timeout = (System.currentTimeMillis() - startTime > config.getMaxUpdateWaitTimeMs()) evaluation from the loop when it added PENDING_REPO_UPDATES to TERMINAL_STATE, and in the same commit disabled the test that covered the timeout — UpdateCommandTest.testTimeoutInstallUpdateArchive is still there with //@Test (UpdateCommandTest.java:271). The CLI parameter, config setter, Javadoc and dead branch were left behind, so the tool still advertises a timeout it does not honour.

Suggested fix

Two consistent outcomes; either is better than the current state:

1. Honour the parameter. Check elapsed time (monotonic, via the WaitClock introduced in [#219] Make UpdateCommand job-wait loop verdict fresh and its test clock-independent #221) before each sleep and stop polling once maxUpdateWaitTimeMs is exceeded, then re-enable testTimeoutInstallUpdateArchive with the fake clock so it is deterministic. Note the current failure path is unsafe for this case: a FAIL from WAIT_FOR_INSTALL_DONE runs the recovery sequence (EXIT_MAINTENANCE_MODE, ENABLE_SCHEDULER, FORCE_RESTART) while the server-side install may still be in progress. A timeout should therefore stop waiting, print the update id and last status and exit non-zero, without running recovery — or the parameter should be documented as "give up waiting; the update continues on the server".
2. Drop the parameter. Remove --maxUpdateWaitTimeMs, UpdateCommandConfig.maxUpdateWaitTimeMs, the @see, the unreachable else branch and the dead test, and document that the CLI waits until the update reaches a terminal state.

Given that repo migrations can legitimately take longer than any fixed default and that upstream apparently removed the timeout on purpose, option 2 (or option 1 with the "stop waiting, do not recover" semantics) seems the safer direction; a 30 s default that triggers recovery mid-install would be worse than today's behaviour.

Environment

• Branch: master (7.1.3-SNAPSHOT), line numbers as of e8073da65
• Module: openidm-shell
• Related: UpdateCommandTest.testEnterMaintenanceMode is wall-clock dependent and flakes on macos-latest / JDK 26 #219, [#219] Make UpdateCommand job-wait loop verdict fresh and its test clock-independent #221

[shubhambhar007]

Hi @vharseko can i please pick this?

[vharseko]

Additional findings

Verified against issues/219 (master + #221); line numbers refer to that branch.

1. --maxUpdateWaitTimeMs is a documented option — removing it breaks the CLI

openidm-doc/src/main/asciidoc/install-guide/chap-update.adoc:223-226 documents the option ("The maximum time, in milliseconds, that the server should wait for the update process to complete", default 30000). Arguments are parsed by Felix Gogo's Reflective.invoke (CommandProxy.java:60-65): an unknown --maxUpdateWaitTimeMs 60000 would be treated as positional arguments, the update method has a single positional slot (archive), so Gogo throws IllegalArgumentException, CommandProxy prints the usage and returns null — exit code 0, update not started. Any script that passes the option today would silently stop updating. Option 2 therefore has to keep accepting the flag (deprecated no-op); at that point honouring it is not much more work.

2. Recovery cannot be skipped by returning a different ExecutorStatus

The recovery loop lives in a finally block (UpdateCommand.java:307-329) and runs after SUCCESS, FAIL and even ABORT (return inside try, :294). Existing tests rely on this: testCantFindArchive expects lastRecoveryStep == ENABLE_SCHEDULER after an ABORT on the very first step (UpdateCommandTest.java:92). A "stop waiting, do not recover" outcome needs a new mechanism — e.g. a detached flag on UpdateExecutionState checked by the onCondition of ExitMaintenanceMode, EnableScheduler and ForceRestart (:927, :974, :1011) — not a new status value.

3. What recovery actually does while the server is still installing

• restartRequired=false: recovery is maintenance/disable (MaintenanceService.java:148-152) + scheduler/resumeJobs. The server-side UpdateThread is unaffected and keeps replacing files/config/bundles, but maintenance mode is switched off and scheduled jobs resume mid-install. Not fatal, but it defeats the purpose of maintenance mode.
• restartRequired=true: FORCE_RESTART → restartNow() (UpdateManagerImpl.java:837-845) only sets restartImmediately=true when an updateThread exists; the restart happens after the install finishes (:1274-1284), just without the 30 s grace period. So the worst case described above — a restart mid-install — is prevented server-side. The real hazard is "maintenance mode / scheduler re-enabled too early", not a killed update.

4. Exit code is a separate problem

RemoteCommandScope.update ignores the UpdateExecutionState returned by execute() (:268), so the CLI exits 0 after any failed step today, not just after a timeout. "Exit non-zero on timeout" would be new behaviour for every failure path and should be its own issue rather than part of this one.

5. The 30 s default cannot simply be switched on

The effective behaviour today is "no limit". Enabling a real 30 s timeout with either semantics changes every update that takes longer than 30 s (bundle replacement, repo migrations):

• with recovery → point 3 above;
• with "detach" and restartRequired=false → nobody ever runs maintenance/disable / resumeJobs, so the server stays in maintenance mode with the scheduler paused until someone does it by hand (the flag is in-memory, only a restart clears it).

The default therefore has to become "wait indefinitely" (<= 0 or -1; note that -1 means "do not wait at all" for maxJobsFinishWaitTimeMs, so the two semantics must be documented explicitly). Changing the documented default to match reality is not a behaviour change.

Proposed direction

Option 1 with detach semantics: enforce the budget via the WaitClock from #221 (checked before each sleep, like WaitForJobsStepExecutor), on expiry log the update id, the last status and how to follow up (GET maintenance/update/log/{id}; for restartRequired=false, maintenance/disable + scheduler/resumeJobs once the update completes), mark the state as detached so the recovery steps are skipped, return FAIL. Default maxUpdateWaitTimeMs → unlimited; update @Descriptor, UpdateCommandConfig Javadoc and chap-update.adoc. Re-enable testTimeoutInstallUpdateArchive on FakeWaitClock (expecting lastRecoveryStep == null) and add a test that the unlimited default survives several IN_PROGRESS polls with a stall longer than 30 s. Depends on #221.

[vharseko]

Additional findings (2)

Verified against issues/219 (master + #221), same line numbers as above. These complement the proposed direction; none of them re-opens the points already made.

6. Three existing tests become wall-clock dependent once the budget is enforced

testSuccessfulInstall, testSuccessfulInstallWithRestart (setMaxUpdateWaitTimeMs(2000L)) and testFailedInstallUpdateArchive (5000L) build UpdateCommand without a FakeWaitClock. Today the budget is ignored, so that does not matter; as soon as it is enforced they get the same failure mode as #219 — a stalled CI runner crosses the budget in WAIT_FOR_INSTALL_DONE. All three have to move to FakeWaitClock(0L) in the same change.

7. startInstallTime is wall-clock time

InstallArchiveStepExecutor records state.setStartInstallTime(System.currentTimeMillis()) (UpdateCommand.java:736). It cannot be compared with clock.nanoTime() and is not driven by the fake clock. The budget should be measured from a monotonic start taken via WaitClock; startInstallTime stays as the "install step completed" marker it is validated as today.

8. The wait loop bypasses WaitClock and swallows interrupts

The loop calls Thread.sleep directly and ignores InterruptedException (:789-791), so only a terminal status or a ResourceException can end it. When it moves to clock.sleep, an interrupt should end the wait the same way the budget does (stop waiting, skip recovery), not keep spinning.

9. A polling error has the same hazard as a timeout

A ResourceException while polling maintenance/update/log/{id} returns FAIL (:808-810), which runs recovery while the server-side install may still be in progress — the log line itself says "The update might still be in process". Decision: treat it as a detach as well, otherwise the timeout fix leaves the same mid-install recovery reachable through a network blip.

Updated direction

On budget expiry, interrupt or polling error in WAIT_FOR_INSTALL_DONE: log the update id, the last known status and the follow-up (GET maintenance/update/log/{id}; for restartRequired=false, maintenance/disable + scheduler/resumeJobs once the update is complete), mark the execution state as detached so EXIT_MAINTENANCE_MODE, ENABLE_SCHEDULER and FORCE_RESTART are skipped, return FAIL. Default → no limit (<= 0). Tests: re-enable testTimeoutInstallUpdateArchive on FakeWaitClock, add polling-error and unlimited-default cases, move the three install tests above to FakeWaitClock. Stacked on #221.

[vharseko]

@shubhambhar007 thanks for the offer, and sorry for the slow reply. This one turned out to be tightly coupled to #221 (it needs the WaitClock introduced there) and to a behavioural decision about skipping the recovery steps, so we are handling it in a PR stacked on #221. You are welcome to pick up another open issue — just leave a comment there first so we can confirm the scope before you start.