feat(windows): handle a hard windows reset occurring while downloading updated keyman files #13128

rc-swag · 2025-02-05T05:42:57Z

When kmshell.exe is started to run configuration on any other number of switches, is in the downloading state, the state machine would just let the execution continue. This PR adds a check to ensure the Download process hasn't crashed in such away that let the state 'stuck' in the downloading state. It know resets the StateMachine if it finds that is the State Machine is in the downloading state but there is no active download process.

I used KeymanMutex because it was quite simple. If however it turns out that the mutex is not removed when the process crashes. Then it maybe better to use something like this https://stackoverflow.com/questions/876224/how-to-check-if-a-process-is-running-using-delphi and check for the -bd command line argument.

User Testing

TEST_DOWNLOAD_INTERUPTED_TASK_MANAGER

Install Keyman attached the PR
Start Keyman
Open Regedit WinR type regedit
Go to the current user key update state found at (Computer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman Engine).
If required set the state back to usIdle
Open Windows explorer at "C:\Users"yourusername"\AppData\Local\Keyman\UpdateCache" folder.
Delete cache.json, any *.kmp and *.exe files.
Go to the update tab and click check for updates
Open a command prompt (kbd>WinR type cmd)
Change to the directory where kmshell is installed
cd "c:\Program Files (x86)\Keyman\Keyman Desktop"
Type kmshell.exe -buc
In the Regedit window verify the update state has advanced to either usUpdateAvailable or usDownloading. Press F5 to refresh the view.
Press ctrl + shift + esc to open the task manager
In task manager find the command prompt that is running Keyman Configuration right click and choose End Task from the pop-menu
Observe that the update state still says usDownloading.
This test now wants to see recovery from this state and not stuck in usDownloading.
Type kmshell.exe -c
In the registry editor press F5 to observe the state changes. In my testing sometimes I missed seeing all the updates as it quickly resets to usIdle -> usUpdateAvailable -> usDownloading -> usWaitingRestart
Expected Result The state should go to usWaitingRestart.

TEST_DOWNLOAD_INTERUPTED_HARD_POWER_OFF

I am not sure if the Test team can complete this test but I tested it twice.

Install Keyman attached the PR
Open Keyman Configuration and turn off start with Windows
Open Regedit WinR type regedit
Go to the current user key update state found at (Computer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman Engine).
If required set the state back to usIdle
Open Windows explorer at "C:\Users"yourusername"\AppData\Local\Keyman\UpdateCache" folder.
Delete cache.json, any *.kmp and *.exe files.
Go to the update tab and click check for updates
Open a command prompt (kbd>WinR type cmd)
Change to the directory where kmshell is installed
cd "c:\Program Files (x86)\Keyman\Keyman Desktop"
Type kmshell.exe -buc
In the Regedit window verify the update state has advanced to either usUpdateAvailable or usDownloading. Press F5 to refresh the view.
Power off the machine (not a clean shutdown but a power-off)
Restart the Machine
Observe that the update state still says usDownloading using the registry editor
This test now wants to see recovery from this state and not stuck in usDownloading.
Type kmshell.exe -c
In the registry editor press F5 to observe the state changes. In my testing sometimes I missed seeing all the updates as it quickly resets to usIdle -> usUpdateAvailable -> usDownloading -> usWaitingRestart
Expected Result The state should go to usWaitingRestart.

Using the KeymanMutex wrapper to check if a download process is occuring if it isn't and we are in the downloading state this means the download process exited early. We can then clean up any downloaded files and reset the statemachine and check for updates again.

keymanapp-test-bot · 2025-02-05T05:43:01Z

User Test Results

Test specification and instructions

✅ TEST_DOWNLOAD_INTERUPTED_TASK_MANAGER (PASSED) (notes)
✅ TEST_DOWNLOAD_INTERUPTED_HARD_POWER_OFF (PASSED): notes developer tested

Test Artifacts

Windows

rc-swag · 2025-02-07T06:12:26Z

TEST_DOWNLOAD_INTERUPTED_HARD_POWER_OFF (PASS): notes developer tested

ermshiperete · 2025-02-07T15:18:09Z

windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas

+    ChangeState(IdleState);
+    bucStateContext.CurrentState.HandleCheck;
+  end;
+  FreeAndNil(FMutex);


Does it matter that we call FreeAndNil twice if FMutex.MutexOwned? (same below)

Yeah good catch, I should exit early to avoid the redundant call. I have also added a comment to make clear to future me or other developers that the mutex needs to be freed before changing state and triggering a force check. Updated to use a try .. finally handling.

FWIW, FreeAndNil is safe to call on an already-nil variable, but it's definitely a code smell in any case 😁

Updated to use a try .. finally handling.

Note that exiting early still runs the finally section

dinakaranr · 2025-02-08T09:54:58Z

Test Results

I tested this issue with the attached Keyman"18.0.184-alpha-test-13128" build(08/02/2025) on Windows 10. Here I am sharing my observation.

TEST_DOWNLOAD_INTERUPTED_TASK_MANAGER (Passed):

Install Keyman-18.0.184.exe file
Start Keyman
Open Regedit WinR type regedit
Go to the current user key update state found at (Computer\HKEY_CURRENT_USER\SOFTWARE\Keyman\Keyman Engine).
If required set the state back to usIdle
Open Windows Explorer at "C:\Users"yourusername"\AppData\Local\Keyman\UpdateCache" folder.
Delete cache.json, any *.kmp and *.exe files.
Go to the update tab and click the "check for new updates" button.
Verified that the state is changing usUpdateAvailable -> usDownloading -> usWaitingRestart
Open a command prompt (kbd>WinR type cmd)
Change to the directory where kmshell is installed (cd "c:\Program Files (x86)\Keyman\Keyman Desktop")
Type kmshell.exe -buc
In the Regedit window verify the update state has advanced to either usUpdateAvailable --> usDownloading. Press F5 to refresh the view.
In the usDownloading state: Press ctrl + shift + esc to open the task manager
In the task manager find the command prompt that is running Keyman Configuration right click and choose End Task from the pop-menu
Verified that the update state still says "usDownloading".
This test now wants to see recovery from this state and not be stuck in usDownloading.
Type kmshell.exe -c
In the registry editor press F5 to observe the state changes.
Verified that the "Update State" changing usIdle -> usUpdateAvailable -> usDownloading -> usWaitingRestart (It happens in very short duration so, refresh and see the changes)
Verified that the "usWaitingRestart" state for "Update state"
It works well. Thank you.

mcdurdin

Asking for some changes to this PR...

So, TKeymanMutex is my fault and has the terribly named MutexOwned function which takes ownership of the mutex. Let's fix that -- rename it to TakeOwnership and add a ReleaseOwnership function. That means a handful of one-line changes elsewhere but clarifies usage dramatically IMHO!

mcdurdin · 2025-02-10T02:34:48Z

windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas

+  try
+    FMutex := TKeymanMutex.Create('KeymanDownloading');


This should be outside the try-finally:

Suggested change

try

FMutex := TKeymanMutex.Create('KeymanDownloading');

FMutex := TKeymanMutex.Create('KeymanDownloading');

try

Also, given we use the same name in multiple calls to the constructor, can we make that a constant at top of implementation section?

const KeymanDownloadMutexName = 'KeymanDownloading';

mcdurdin · 2025-02-10T02:35:42Z

windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas

 begin
  // Enter DownloadingState
  bucStateContext.SetRegistryState(usDownloading);

  RetryCount := 0;
  DownloadResult := False;
+  FMutex := nil;


Suggested change

FMutex := nil;

With the construction of FMutex outside try/finally, this is not needed

mcdurdin · 2025-02-10T02:36:13Z

windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas

+    while (not DownloadResult) and (RetryCount < 3) do
+    begin
+      DownloadResult := DownloadUpdatesBackground;
+      if not DownloadResult then
+        Inc(RetryCount);
+    end;


This could be a repeat/until loop which is a little cleaner.

Suggested change

while (not DownloadResult) and (RetryCount < 3) do

begin

DownloadResult := DownloadUpdatesBackground;

if not DownloadResult then

Inc(RetryCount);

end;

RetryCount := 0;

repeat

DownloadResult := DownloadUpdatesBackground;

Inc(RetryCount);

until DownloadResult or (RetryCount = 3);

mcdurdin · 2025-02-10T02:36:33Z

windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas

+  try
+    FMutex := TKeymanMutex.Create('KeymanDownloading');


Suggested change

try

FMutex := TKeymanMutex.Create('KeymanDownloading');

FMutex := TKeymanMutex.Create('KeymanDownloading');

try

ditto

mcdurdin · 2025-02-10T02:36:39Z

windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas

 begin
-  // Downloading state, in other process, so continue
+  FMutex := nil;


Suggested change

FMutex := nil;

mcdurdin · 2025-02-10T02:42:35Z

windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas

+    if FMutex.MutexOwned then
+    begin
+      bucStateContext.RemoveCachedFiles;
+      FreeAndNil(FMutex); // Mutex must be freed before changing state


I think it would it be cleaner to relinquish ownership without freeing here. Add TKeymanMutex.Release function which calls ReleaseMutex()

mcdurdin · 2025-02-10T02:44:17Z

windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas

+  FMutex := nil;
+  // If downloading process is not running clean files and return to idle
+  try
+    FMutex := TKeymanMutex.Create('KeymanDownloading');


Suggested change

FMutex := nil;

// If downloading process is not running clean files and return to idle

try

FMutex := TKeymanMutex.Create('KeymanDownloading');

// If downloading process is not running clean files and return to idle

FMutex := TKeymanMutex.Create('KeymanDownloading');

try

mcdurdin · 2025-02-10T02:44:26Z

windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas

+    if FMutex.MutexOwned then
+    begin
+      bucStateContext.RemoveCachedFiles;
+      FreeAndNil(FMutex);  // Mutex must be freed before changing state


ditto .Release

mcdurdin · 2025-02-10T02:44:35Z

windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas

+      FreeAndNil(FMutex);  // Mutex must be freed before changing state
+      ChangeState(IdleState);
+      bucStateContext.CurrentState.HandleCheck;
+      Exit;


ditto not required

Suggested change

Exit;

mcdurdin · 2025-02-10T02:45:22Z

windows/src/desktop/kmshell/main/Keyman.System.UpdateStateMachine.pas

  RetryCount := 0;
  DownloadResult := False;


I think these should be immediately before their first use in the repeat/until loop, and then DownloadResult does not need to be pre-initialized either

Suggested change

RetryCount := 0;

DownloadResult := False;

…into feat/windows/13119/handle-reset-downloading # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Feb 5, 2025

keymanapp-test-bot bot added this to the B18S1 milestone Feb 5, 2025

rc-swag changed the base branch from master to feat/windows/13114/merge-update-reg-key February 5, 2025 05:43

github-actions bot added windows/ windows/config/ feat labels Feb 5, 2025

Base automatically changed from feat/windows/13114/merge-update-reg-key to master February 6, 2025 11:46

rc-swag changed the title ~~feat(windows): handle reset downloading~~ feat(windows): handle a hard windows reset occurring while downloading updated keyman files Feb 7, 2025

keymanapp-test-bot bot added the has-user-test label Feb 7, 2025

rc-swag marked this pull request as ready for review February 7, 2025 05:19

rc-swag requested a review from ermshiperete as a code owner February 7, 2025 05:19

keymanapp-test-bot bot added user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels Feb 7, 2025

rc-swag self-assigned this Feb 7, 2025

ermshiperete approved these changes Feb 7, 2025

View reviewed changes

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Feb 8, 2025

feat(windows): remove redunant FreeAndNil

01690ed

rc-swag requested a review from ermshiperete February 10, 2025 00:09

mcdurdin requested changes Feb 10, 2025

View reviewed changes

rc-swag mentioned this pull request Feb 10, 2025

feat(windows): add release function to KeymanMutex unit and rename MutexOwned #13167

Open

mcdurdin mentioned this pull request Feb 10, 2025

refactor(windows): rename TKeymanMutex.MutexOwned to TakeOwnership and add ReleaseOwnership #13168

Open

rc-swag added 2 commits February 10, 2025 14:04

feat(windows): review changes

8281e32

github-actions bot added the windows/engine/ label Feb 10, 2025

rc-swag changed the base branch from master to feat/windows/13167/add-release-kmutex February 10, 2025 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(windows): handle a hard windows reset occurring while downloading updated keyman files #13128

feat(windows): handle a hard windows reset occurring while downloading updated keyman files #13128

rc-swag commented Feb 5, 2025 •

edited

Loading

keymanapp-test-bot bot commented Feb 5, 2025 •

edited

Loading

rc-swag commented Feb 7, 2025

ermshiperete Feb 7, 2025

rc-swag Feb 10, 2025

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

dinakaranr commented Feb 8, 2025

mcdurdin left a comment

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

mcdurdin Feb 10, 2025

feat(windows): handle a hard windows reset occurring while downloading updated keyman files #13128

Are you sure you want to change the base?

feat(windows): handle a hard windows reset occurring while downloading updated keyman files #13128

Conversation

rc-swag commented Feb 5, 2025 • edited Loading

User Testing

TEST_DOWNLOAD_INTERUPTED_TASK_MANAGER

TEST_DOWNLOAD_INTERUPTED_HARD_POWER_OFF

keymanapp-test-bot bot commented Feb 5, 2025 • edited Loading

User Test Results

Test Artifacts

rc-swag commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinakaranr commented Feb 8, 2025

Test Results

mcdurdin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rc-swag commented Feb 5, 2025 •

edited

Loading

keymanapp-test-bot bot commented Feb 5, 2025 •

edited

Loading