Skip to content

Fix issue with progress directory not found in progress sync #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jovnc merged 6 commits into from
Jan 26, 2026
Merged

Fix issue with progress directory not found in progress sync #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6a13373
Fix issue with progress directory not found in progress sync
jovnc Jan 14, 2026
62da93a
Update fix to resolve race condition for Windows in deleting folder
jovnc Jan 14, 2026
c041e3d
Address copilot comments
jovnc Jan 14, 2026
2b57e8d
Add fix for race condition for all callsites to rmtree for Windows
jovnc Jan 14, 2026
9839695
Fix progress sync off and clean up code
jovnc Jan 14, 2026
fbe295c
Address comments
jovnc Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/commands/progress/sync/off.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ def off() -> None:
local_progress = json.load(file)

rmtree(PROGRESS_LOCAL_FOLDER_NAME)
os.makedirs(os.path.dirname(local_progress_filepath), exist_ok=True)
Copy link
Member

@woojiahao woojiahao Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I understand the rationale behind this? Is it because rmtree might not fully delete it on Windows, and as a result, we need to re-create the folder explicitly, rather than allowing open(..., "a") to create it?

Copy link
Collaborator Author

@jovnc jovnc Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is to address an existing bug for all OSes. It's more of a separate issue, but I just placed it here for convenience, since it's sort of related.

Image

Idea is because we do a rmtree at the top deletes the progress folder, but below we instantly open it

with open(local_progress_filepath, "a") as progress_file:

This line throws a FileNotFoundError. The problem is that open with "a" mode creates the file if it doesn't exist, but does NOT create the parent directory.


# Re-create just the progress folder
with open(local_progress_filepath, "a") as progress_file:
Expand Down
11 changes: 11 additions & 0 deletions app/commands/progress/sync/on.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,17 @@ def on() -> None:

clone_with_custom_name(f"{username}/{fork_name}", PROGRESS_LOCAL_FOLDER_NAME)

# Verify clone succeeded, else restore local progress before failing
if not os.path.exists(PROGRESS_LOCAL_FOLDER_NAME):
Copy link
Member

@woojiahao woojiahao Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, isn't it still possible for this to work even though the deletion still trails?

  1. rmtree fails, retry
  2. Clone runs
  3. Path exists, skip over if
  4. rmtree works, deletes the new folder

Copy link
Collaborator Author

@jovnc jovnc Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If rmtree keeps failing after 20 retries, we raise an exception at rmtree already and won't proceed.

This check here is more of a safety check to check that clone succeeds rather than for rmtree.

It's more of a safety net, as if clone fails, we can still restore the existing local progress.json file for the user (otherwise if it fails, it will leave users in unexpected state without progress folder).

But I'm also ok with leaving this check out to be honest, as i don't feel its entirely necessary.

os.makedirs(os.path.dirname(local_progress_filepath), exist_ok=True)
with open(local_progress_filepath, "w") as file:
file.write(json.dumps(local_progress, indent=2))
raise RuntimeError(
f"Clone failed for {PROGRESS_LOCAL_FOLDER_NAME}. "
"Your local progress has been restored."
"Re-run the command `gitmastery progress sync on` to try again."
)

# To reconcile the difference between local and remote progress, we merge by
# (exercise_name, start_time) which should be unique
remote_progress = []
Expand Down
22 changes: 22 additions & 0 deletions app/utils/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,35 @@
import os
import shutil
import stat
import time
from pathlib import Path
from typing import Union

MAX_DELETE_RETRIES = 20
Copy link
Member

@woojiahao woojiahao Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we might stall for 4 seconds in total? Seems like a pretty high upper bound. Do we want to lower it for now? I think a max retry of 5 is somewhat reasonable unless we hit an obvious issue

Copy link
Collaborator Author

@jovnc jovnc Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should be safer with a higher number, I increased it from 10 initially. Rationale is if we set too low a threshold, there is a higher likelihood of failure and we will need to update the app again for students, which we want to avoid.

MAX_RETRY_INTERVAL = 0.2


def rmtree(folder_name: Union[str, Path]) -> None:
"""
Remove a directory tree.

Raises RuntimeError if the folder still exists after max retries.
"""
if not os.path.exists(folder_name):
return

def force_remove_readonly(func, path, _):
os.chmod(path, stat.S_IWRITE)
func(path)

shutil.rmtree(folder_name, onerror=force_remove_readonly)

# Wait for folder to be fully deleted (Windows can be slow with permissions)
max_retries = MAX_DELETE_RETRIES
for _ in range(max_retries):
if not os.path.exists(folder_name):
return
time.sleep(MAX_RETRY_INTERVAL)

# If folder still exists after retries, raise error
raise RuntimeError(f"Failed to delete {folder_name} after {max_retries} retries")