Skip to content

Use shutdown-first detached reboot wrappers#137

Merged
jamesyc merged 6 commits into
mainfrom
codex/sync-shutdown-reboot
May 20, 2026
Merged

Use shutdown-first detached reboot wrappers#137
jamesyc merged 6 commits into
mainfrom
codex/sync-shutdown-reboot

Conversation

@jamesyc
Copy link
Copy Markdown
Owner

@jamesyc jamesyc commented May 20, 2026

Summary

  • update direct SSH reboot requests to sync, sleep, try shutdown -r, then fall back to reboot while preserving the detached wrapper
  • use the same detached shutdown-first reboot wrapper after fsck
  • update progress/test assertions for the new reboot behavior

Tests

  • /Users/jameschang/git/TimeCapsuleSMB/.venv/bin/pytest tests/test_deploy_modules.py::DeployModuleTests::test_remote_request_reboot_uses_explicit_reboot_timeout tests/test_deploy_modules.py::DeployModuleTests::test_remote_request_shutdown_reboot_uses_shutdown_with_reboot_fallback tests/test_cli.py::CliTests::test_fsck_yes_reboots_and_waits_by_default tests/test_cli.py::CliTests::test_fsck_no_reboot_omits_reboot_and_waits tests/test_cli_flows.py::CliFlowTests::test_request_ssh_reboot_uses_ssh_only_strategy_and_progress_log

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the reboot logic across the system to include a sync command and a check for the /sbin/shutdown utility before falling back to /sbin/reboot. These changes are reflected in the CLI flows, fsck scripts, and deployment executor, along with updated test coverage. Feedback focuses on reducing code duplication by unifying the reboot command constants across different modules and ensuring that the sync operation is applied consistently to all related reboot commands.

Comment thread src/timecapsulesmb/deploy/executor.py Outdated
Comment thread src/timecapsulesmb/cli/fsck.py Outdated
@jamesyc
Copy link
Copy Markdown
Owner Author

jamesyc commented May 20, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the remote reboot command to include a sync operation and prioritize the shutdown command over a direct reboot for improved reliability. The review identified that the two reboot functions in the executor module are now redundant and should be consolidated, and noted that a progress message override in the CLI flows module needs to be updated for consistency with the new command logic.

Comment thread src/timecapsulesmb/deploy/executor.py
Comment thread src/timecapsulesmb/cli/flows.py Outdated
@jamesyc
Copy link
Copy Markdown
Owner Author

jamesyc commented May 20, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request unifies the reboot logic by replacing DETACHED_REBOOT_COMMAND with an updated DETACHED_SHUTDOWN_REBOOT_COMMAND that includes a sync operation and a fallback mechanism. These changes are applied to the CLI flows and fsck scripts, and the test suite has been updated to verify the new command strings. Feedback suggests refactoring the hardcoded progress messages into a constant to improve maintainability and ensure consistency across the codebase.

Comment thread src/timecapsulesmb/cli/flows.py Outdated
@jamesyc
Copy link
Copy Markdown
Owner Author

jamesyc commented May 20, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the remote reboot mechanism to consistently use a shutdown-first approach by including /bin/sync and removing the conditional check for /sbin/shutdown. It standardizes the reboot command and progress messages across the CLI and deployment modules, while refactoring remote_request_shutdown_reboot to call remote_request_reboot. The reviewer suggests further simplifying remote_request_shutdown_reboot by aliasing it to remote_request_reboot.

Comment thread src/timecapsulesmb/deploy/executor.py Outdated
@jamesyc
Copy link
Copy Markdown
Owner Author

jamesyc commented May 20, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request consolidates and simplifies the remote reboot logic by removing the redundant remote_request_shutdown_reboot function and DETACHED_REBOOT_COMMAND constant. It introduces a unified DETACHED_SHUTDOWN_REBOOT_COMMAND that includes a /bin/sync call and a fallback mechanism. Corresponding updates were made to the CLI flows, FSCK script, and test suites. Feedback suggests moving the /bin/sync command outside the backgrounded subshell in the reboot command to ensure data integrity even if backgrounding fails.

Comment thread src/timecapsulesmb/deploy/executor.py
@jamesyc jamesyc merged commit 5ef7cac into main May 20, 2026
12 checks passed
@jamesyc jamesyc deleted the codex/sync-shutdown-reboot branch May 20, 2026 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant