P2: Service-aware upgrade coordination (stop/start service safely) #46
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideCoordinates proxy install/update with the Windows Service by stopping it before modifying the proxy binary and attempting a restart afterward, and hardens proxy downloads by using a temporary file with checksum verification before atomically replacing the final executable. Sequence diagram for service-aware proxy installation workflowsequenceDiagram
actor User
participant CLI as InstallCommand
participant Service as WindowsService
participant Proxy as ProxyProcess
participant Updater as UpdaterModule
User->>CLI: run install
CLI->>Service: isServiceInstalled
Service-->>CLI: boolean
CLI->>Service: isServiceRunning (if installed)
Service-->>CLI: boolean serviceWasRunning
alt serviceWasRunning
CLI->>Service: stopService
Service-->>CLI: stopped
end
CLI->>Proxy: isRunning
Proxy-->>CLI: boolean
alt proxyRunning
CLI->>Proxy: stopProxy
Proxy-->>CLI: stopped
end
CLI->>Updater: getLatestVersion (optional)
Updater-->>CLI: version
CLI->>Updater: downloadProxy(version, targetPath)
Updater-->>CLI: success or error
alt install success
CLI-->>User: log Installation complete
else install failure
CLI-->>User: log Installation failed
CLI->>CLI: process.exit(1)
end
opt serviceWasRunning and stopService succeeded
CLI->>Service: startService (in finally block)
Service-->>CLI: started or error
alt restart error
CLI-->>User: log Failed to restart Windows Service
end
end
Sequence diagram for hardened proxy download and atomic replacementsequenceDiagram
participant Caller as UpdaterCaller
participant Updater as UpdaterModule
participant FS as FileSystem
participant HTTP as Axios
participant Logger as Logger
Caller->>Updater: downloadProxy(version, targetPath)
Updater->>FS: ensureDir(dirname(targetPath))
FS-->>Updater: ok
Updater->>FS: remove(`${targetPath}.download`)
FS-->>Updater: ok
Updater->>FS: createWriteStream(tmpPath)
FS-->>Updater: writer
Updater->>HTTP: GET downloadUrl (stream)
HTTP-->>Updater: responseStream
Updater->>FS: pipe responseStream.data to writer
Updater->>Updater: await writer finish
Updater->>Logger: info Download complete
Updater->>Logger: info Verifying checksum
Updater->>Updater: verifyChecksum(tmpPath, expectedChecksum)
Updater-->>Updater: isValid
alt checksum ok
Updater->>Logger: info Checksum verified
Updater->>FS: move(tmpPath, targetPath, overwrite=true)
FS-->>Updater: ok
Updater-->>Caller: success
else checksum failed or other error
Updater->>Logger: warn Failed to download/verify proxy
Updater->>FS: remove(tmpPath)
FS-->>Updater: ok
Updater-->>Caller: throw error
end
Class diagram for updated updater and service-aware commandsclassDiagram
class UpdaterModule {
+downloadProxy(version string, targetPath string) Promise~void~
+getLatestVersion() Promise~string~
-verifyChecksum(filePath string, expectedChecksum string) Promise~boolean~
}
class InstallCommandModule {
+installCommand Command
}
class UpdateCommandModule {
+updateCommand Command
}
class ServiceModule {
+isServiceInstalled() Promise~boolean~
+isServiceRunning() Promise~boolean~
+startService() Promise~void~
+stopService() Promise~void~
}
class ProxyModule {
+isRunning() Promise~boolean~
+stopProxy() Promise~void~
}
class LoggerModule {
+info(message string, meta any) void
+warn(message string, meta any) void
+error(message string, meta any) void
}
InstallCommandModule --> UpdaterModule : uses
InstallCommandModule --> ServiceModule : coordinates
InstallCommandModule --> ProxyModule : coordinates
InstallCommandModule --> LoggerModule : logs
UpdateCommandModule --> UpdaterModule : uses
UpdateCommandModule --> ServiceModule : coordinates
UpdateCommandModule --> ProxyModule : coordinates
UpdateCommandModule --> LoggerModule : logs
UpdaterModule --> LoggerModule : logs
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The service stop/start logic in
installCommandandupdateCommandis nearly identical; consider extracting this into a shared helper (e.g.,withServiceStopped(fn)) to reduce duplication and keep behavior consistent between commands. - In
downloadProxy, you currently only attach error handling to the write stream; it would be safer to also handle errors on the HTTP response stream (e.g.,responseStream.data.on('error', ...)) to ensure the temporary file is cleaned up if the download stream fails before thewriteremits an error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The service stop/start logic in `installCommand` and `updateCommand` is nearly identical; consider extracting this into a shared helper (e.g., `withServiceStopped(fn)`) to reduce duplication and keep behavior consistent between commands.
- In `downloadProxy`, you currently only attach error handling to the write stream; it would be safer to also handle errors on the HTTP response stream (e.g., `responseStream.data.on('error', ...)`) to ensure the temporary file is cleaned up if the download stream fails before the `writer` emits an error.
## Individual Comments
### Comment 1
<location> `src/core/updater.ts:64-68` </location>
<code_context>
+ const tmpPath = `${targetPath}.download`;
+ await fs.remove(tmpPath);
+
+ const writer = fs.createWriteStream(tmpPath);
const responseStream = await axios({
url: downloadUrl,
method: 'GET',
responseType: 'stream'
});
- responseStream.data.pipe(writer);
-
- await new Promise((resolve, reject) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle errors from the HTTP response stream to avoid hanging on download failures.
The awaited promise only subscribes to `writer` events, so errors from `responseStream.data` are ignored. If the readable stream errors, the writable may never emit `finish`/`error`, leaving `downloadProxy` stuck on failed requests.
Add an `error` listener on `responseStream.data` that rejects the promise (and/or closes `writer`), or use `stream.pipeline` (optionally `promisified`) to handle backpressure and propagate errors in one place.
</issue_to_address>
### Comment 2
<location> `src/commands/install.ts:11-20` </location>
<code_context>
.description('Download and install Cloud SQL Proxy')
.option('-v, --version <version>', 'Specific version to install')
.action(async (options) => {
+ const serviceInstalled = await isServiceInstalled();
+ const serviceWasRunning = serviceInstalled && await isServiceRunning();
+ let serviceStopped = false;
</code_context>
<issue_to_address>
**suggestion:** Consider extracting the repeated Windows service stop/start logic into a shared helper.
This lifecycle (`isServiceInstalled` → `isServiceRunning` → `stopService` → `startService`, with `serviceWasRunning`/`serviceStopped` and logging) is duplicated in both `install` and `update`. A shared helper (e.g., taking an async callback to run while the service is stopped) would reduce duplication and keep service-management behavior consistent as it evolves.
Suggested implementation:
```typescript
import { getLatestVersion, downloadProxy } from '../core/updater.js';
import { logger } from '../core/logger.js';
import { isRunning, stopProxy } from '../core/proxy.js';
import { isServiceInstalled, isServiceRunning, startService, stopService } from '../system/service.js';
async function withServiceStoppedDuring(
phase: string,
fn: () => Promise<void>,
): Promise<void> {
const serviceInstalled = await isServiceInstalled();
const serviceWasRunning = serviceInstalled && await isServiceRunning();
let serviceStopped = false;
try {
if (serviceWasRunning) {
logger.info(`Stopping Windows Service before ${phase}...`);
await stopService();
serviceStopped = true;
}
await fn();
} finally {
if (serviceStopped) {
logger.info(`Starting Windows Service after ${phase}...`);
await startService();
}
}
}
```
```typescript
export const installCommand = new Command('install')
.description('Download and install Cloud SQL Proxy')
.option('-v, --version <version>', 'Specific version to install')
.action(async (options) => {
await withServiceStoppedDuring('installation', async () => {
if (await isRunning()) {
logger.info('Stopping running proxy before installation...');
```
Because only part of the file is visible, you’ll need to:
1. **Inside `installCommand`**
- Move all the existing install logic that should run while the service is stopped (everything that currently follows the `if (await isRunning()) { ... }` block and is inside the same `try` as the `stopService` call) into the callback passed to `withServiceStoppedDuring('installation', async () => { ... })`.
- Remove the original `try { ... } finally { ... }` that manages `serviceStopped` and calls `startService()`; that is now handled by `withServiceStoppedDuring`.
2. **In the `update` command (likely in the same file or a sibling command file)**
- Identify the duplicated Windows service lifecycle logic:
- `const serviceInstalled = await isServiceInstalled();`
- `const serviceWasRunning = serviceInstalled && await isServiceRunning();`
- `let serviceStopped = false;`
- `stopService()` / `startService()` and related logging.
- Replace that block with `await withServiceStoppedDuring('update', async () => { /* existing update logic */ });`, moving the update logic into the callback and removing the now-unnecessary local service lifecycle code.
3. **Logging messages**
- If the `update` command uses slightly different log messages today (e.g., “before update” vs another wording), adjust the `phase` string argument (`'update'`, `'installation'`, etc.) or, if needed, extend `withServiceStoppedDuring` to accept custom log messages while preserving shared behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+64
to
68
| const writer = fs.createWriteStream(tmpPath); | ||
| const responseStream = await axios({ | ||
| url: downloadUrl, | ||
| method: 'GET', | ||
| responseType: 'stream' |
There was a problem hiding this comment.
issue (bug_risk): Handle errors from the HTTP response stream to avoid hanging on download failures.
The awaited promise only subscribes to writer events, so errors from responseStream.data are ignored. If the readable stream errors, the writable may never emit finish/error, leaving downloadProxy stuck on failed requests.
Add an error listener on responseStream.data that rejects the promise (and/or closes writer), or use stream.pipeline (optionally promisified) to handle backpressure and propagate errors in one place.
There was a problem hiding this comment.
Pull request overview
This pull request adds service-aware upgrade coordination for the Cloud SQL Proxy Windows service, ensuring safe stop/start operations during install and update commands. The changes also harden the proxy download process by using atomic file operations.
Key changes:
- Enhanced download process with temporary file staging, checksum verification, and atomic file replacement
- Added service-aware logic to install and update commands that stops the Windows Service before replacing the binary and restarts it afterwards
- Improved error handling and cleanup for failed download/verification operations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/core/updater.ts | Hardens download process by writing to temporary file, verifying checksum, then atomically moving to target location with improved error handling |
| src/commands/update.ts | Adds service-aware update logic that stops Windows Service before update and restarts it in finally block to maintain system state |
| src/commands/install.ts | Adds service-aware installation logic that stops Windows Service before installation and restarts it in finally block to maintain system state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| writer.on('error', reject); | ||
| }); | ||
| try { | ||
| responseStream.data.pipe(writer); |
There was a problem hiding this comment.
Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #27
Summary
Local tests
Rollback
Summary by Sourcery
Make proxy installation and update service-aware and harden the proxy download process with checksum-verified, atomic replacement.
New Features:
Enhancements: