-
-
Notifications
You must be signed in to change notification settings - Fork 5
Improve shutdown for FwLiteWeb in Paratext #1992
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.
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
Conversation
* Replace direct process killing with a dedicated `shutdownFwLite` function for graceful termination. * Update FwLiteWeb to properly handle `shutdown` commands via stdin. * Adjust process stdio configuration for consistency.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a console-triggered shutdown path: the web app starts a background Task that reads stdin and calls app.StopAsync() when it reads exactly "shutdown". The extension enables stdin for the spawned process, adds a shutdownFwLite helper that sends "shutdown\n", awaits exit (rejects on non-zero), and enforces a 10s SIGKILL if needed; cleanup now uses that helper. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Potential review focus:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/FwLite/FwLiteWeb/Program.cs (1)
18-25: Optional: guard against unexpected read errors and log.Wrap the loop in try/catch so unexpected I/O failures don’t tear down the background task silently. If you have logging on the host, log and call StopAsync().
platform.bible-extension/src/main.ts (1)
296-315: Optional: kill the process tree on Windows if FwLiteWeb spawns children.If FwLiteWeb ever spawns child processes, consider terminating the whole tree (e.g., taskkill /T /F or a library like tree-kill) in the fallback, otherwise stdout/stderr handles may keep the parent alive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/FwLite/FwLiteWeb/Program.cs(1 hunks)platform.bible-extension/src/main.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Platform.Bible extension CI/CD
platform.bible-extension/src/main.ts
[warning] 1-1: Code style issues found in 'src/main.ts'. Run 'prettier --write .' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: frontend
- GitHub Check: frontend-component-unit-tests
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
platform.bible-extension/src/main.ts (3)
252-252: Good: register graceful cleanup instead of hard kill.Hooking cleanup to a shutdown function is the right direction. Verify the activation host treats a rejected Promise from a disposer as a non-fatal deactivation; otherwise prefer resolving false instead of rejecting.
279-279: Stdin pipe is required here—LGTM.Switching stdio to ['pipe','pipe','pipe'] is necessary so the backend can read the shutdown command.
1-1: Run Prettier via the package’s format script
CI flagged formatting in platform.bible-extension/src/main.ts; execute in that folder:cd platform.bible-extension && pnpm run formatLikely an incorrect or invalid review comment.
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
myieye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🙃 😬
|
@copilot Please review and implement as appropriate the open suggestions from coderabbitai. So: |
|
@imnasnainaec I've implemented the AI feedback. Would you be willing to take another look and maybe actually run it? |
imnasnainaec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting on paranext/paranext-core#1936 (or something equivalent).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/FwLite/FwLiteWeb/Program.cs (1)
23-30: Stdin shutdown listener correctly handles EOF and whitespace.The implementation properly exits on both null (EOF) and the "shutdown" command, preventing a busy loop if stdin closes. The use of
.Trim()and the fire-and-forget pattern are appropriate.Optional: Consider case-insensitive comparison for robustness
While the current case-sensitive match works with the controlled sender, a case-insensitive comparison would be more resilient:
- // Windows doesn't allow sending SIGINT to a process, so we need to listen for a shutdown command - _ = Task.Run(async () => - { - // Wait for the "shutdown" command from stdin - while ((await Console.In.ReadLineAsync())?.Trim() is not ("shutdown" or null)) { } - - await app.StopAsync(); - }); + // Windows doesn't allow sending SIGINT to a process, so we need to listen for a shutdown command + _ = Task.Run(async () => + { + string? line; + while ((line = await Console.In.ReadLineAsync()) is not null + && !string.Equals(line.Trim(), "shutdown", StringComparison.OrdinalIgnoreCase)) + { /* keep waiting */ } + + await app.StopAsync(); + });This matches the suggestion from the previous review and allows "Shutdown", "SHUTDOWN", etc.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/FwLite/FwLiteWeb/Program.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*.cs
📄 CodeRabbit inference engine (backend/AGENTS.md)
backend/**/*.cs: Use async/await for asynchronous operations in C# code; avoid using .Result or .Wait()
Prefer Records for DTOs and immutable data structures in C#Use Entity Framework Core for data access in backend projects following the LexData layer pattern
Files:
backend/FwLite/FwLiteWeb/Program.cs
backend/**/{*Kernel.cs,Program.cs}
📄 CodeRabbit inference engine (backend/AGENTS.md)
Use AddScoped for request-scoped services and AddSingleton for app-wide services in dependency injection registration
Files:
backend/FwLite/FwLiteWeb/Program.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: frontend-component-unit-tests
- GitHub Check: frontend
- GitHub Check: Build FW Lite and run tests
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
- GitHub Check: Build FW Lite and run tests
myieye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finishing this up @imnasnainaec
It turns out that windows doesn't have a gracefull shutdown of a process outside of Ctrl+C which you can't just send to a process you've spawned. Hence this fix.
shutdownFwLitefunction for graceful termination, replacing direct process killing.shutdowncommands via stdin.