Add CLI cancellation propagation#59
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughThis PR implements comprehensive end-to-end CLI cancellation support by threading an AbortSignal from the process entry point through all CLI subsystems. The entrypoint creates an AbortController wired to SIGINT/SIGTERM, the signal is stored in CliRuntime, and cancellation is detected in the command runner to emit a standardized COMMAND_CANCELED error (exit code 130). Filesystem I/O, authentication, API calls, polling loops, preview/build, provider, and token refresh-lock logic now accept and propagate the signal; tests added/updated cover abort behavior. ChangesCLI Cancellation Propagation
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/cli/src/controllers/project.ts (1)
111-111:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing signal parameter in fixture mode.
The call to
readProjectListLocalBindingis missing the requiredsignalparameter. The real-mode branch at line 93 correctly passescontext.runtime.signal, but this fixture-mode branch omits it.🐛 Proposed fix
- const localBinding = await readProjectListLocalBinding(context.runtime.cwd, workspace, result.projects); + const localBinding = await readProjectListLocalBinding(context.runtime.cwd, workspace, result.projects, context.runtime.signal);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/controllers/project.ts` at line 111, The fixture-mode call to readProjectListLocalBinding is missing the required signal argument; update the fixture branch where readProjectListLocalBinding(context.runtime.cwd, workspace, result.projects) is invoked to include the same signal passed in the real branch (context.runtime.signal) so it calls readProjectListLocalBinding(context.runtime.cwd, workspace, result.projects, context.runtime.signal).packages/cli/src/adapters/git.ts (1)
14-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRethrow aborts from
readGitOriginRemote.The blanket
catchturns an abortedgit configintonull, so Ctrl-C/SIGTERM is treated as “no origin remote” and cancellation stops propagating.Suggested fix
- } catch { - return null; + } catch (error) { + if (signal?.aborted || (error instanceof Error && error.name === "AbortError")) { + throw error; + } + return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/adapters/git.ts` around lines 14 - 24, The catch in readGitOriginRemote swallows aborts; change it to catch the thrown error as a variable and rethrow when it's an abort (e.g., err.name === 'AbortError' or signal?.aborted) so cancellation propagates, otherwise return null for other failures; update readGitOriginRemote to inspect the caught error and only suppress non-abort errors.packages/cli/src/adapters/token-storage.ts (1)
123-151:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClean up the lock file when acquisition fails after
open("wx").Once
fs.open(..., "wx")succeeds, this process owns the lock path. Ifhandle.writeFile(lockId, { signal })aborts or throws, the function exits without returninglockId, but the empty/partial lock file remains. The next caller then sits in the retry loop until the stale-lock timeout expires.Suggested fix
try { // open does not accept AbortSignal; check before the filesystem boundary. const handle = await fs.open(this.lockFilePath, "wx"); + let wroteLockId = false; try { await handle.writeFile(lockId, { encoding: "utf8", signal: this.signal }); + wroteLockId = true; } finally { await handle.close(); + if (!wroteLockId) { + await fs.unlink(this.lockFilePath).catch(() => {}); + } } return lockId;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/adapters/token-storage.ts` around lines 123 - 151, In acquireRefreshLock, after fs.open(this.lockFilePath, "wx") succeeds the process owns the lock but an error during handle.writeFile(...) can leave a partial/empty lock file; change the inner try/finally so that if writeFile throws you close the handle and remove the lock file (fs.unlink or equivalent on this.lockFilePath) before rethrowing so other callers won't wait for the stale-lock timeout; ensure you still call releaseRefreshLock(staleLockId) and preserve AbortSignal handling and error propagation (referencing acquireRefreshLock, lockFilePath, handle.writeFile, releaseRefreshLock, and getStaleRefreshLockId).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/package.json`:
- Around line 44-46: The package versions in packages/cli/package.json
(specifically `@prisma/compute-sdk`@0.20.0 and `@prisma/management-api-sdk`@1.35.0)
don’t appear to accept an AbortSignal, so stop passing options.signal blindly
into SDK calls; inspect the TypeScript signatures of sdk.createProject and
sdk.showService (and any other sdk.* calls where you pass options.signal) and
either (A) remove the signal property from the call site (e.g., stop forwarding
options.signal) or (B) upgrade the dependency to a version that explicitly
supports a signal param and update package.json accordingly; ensure conditional
forwarding if you choose to detect support (check for a declared signal
parameter in the method signature or a runtime option flag) and add a short
comment at each call site (sdk.createProject / sdk.showService) documenting why
signal is not forwarded or which SDK version is required.
In `@packages/cli/src/adapters/local-state.ts`:
- Around line 96-100: The write method in the LocalState adapter currently
passes the root AbortSignal into fs.writeFile which can produce
partially-written JSON on abort; change write (method LocalState.write) to avoid
passing this.signal into writeFile: call this.signal?.throwIfAborted() before
starting and again after the write completes, and invoke writeFile without the {
signal } option, or alternately implement a safe-write by writing to a temporary
file (near this.stateFilePath) and then atomically renaming it to
this.stateFilePath on success so aborts cannot leave a corrupt state.json; keep
the mkdir(path.dirname(this.stateFilePath), { recursive: true }) and ensure
throwIfAborted is used to short-circuit before/after the filesystem mutation.
In `@packages/cli/tests/auth-login.test.ts`:
- Line 77: The mock for handleCallback currently returns undefined (vi.fn()), so
awaiting it resolves immediately; change the mock to return a never-resolving
promise instead (e.g., mock handleCallback to return new Promise(() => {}) or
use vi.fn().mockReturnValue(new Promise(() => {}))) so the test truly waits and
can exercise abort behavior; update the mock for handleCallback in the
auth-login.test.ts setup accordingly.
---
Outside diff comments:
In `@packages/cli/src/adapters/git.ts`:
- Around line 14-24: The catch in readGitOriginRemote swallows aborts; change it
to catch the thrown error as a variable and rethrow when it's an abort (e.g.,
err.name === 'AbortError' or signal?.aborted) so cancellation propagates,
otherwise return null for other failures; update readGitOriginRemote to inspect
the caught error and only suppress non-abort errors.
In `@packages/cli/src/adapters/token-storage.ts`:
- Around line 123-151: In acquireRefreshLock, after fs.open(this.lockFilePath,
"wx") succeeds the process owns the lock but an error during
handle.writeFile(...) can leave a partial/empty lock file; change the inner
try/finally so that if writeFile throws you close the handle and remove the lock
file (fs.unlink or equivalent on this.lockFilePath) before rethrowing so other
callers won't wait for the stale-lock timeout; ensure you still call
releaseRefreshLock(staleLockId) and preserve AbortSignal handling and error
propagation (referencing acquireRefreshLock, lockFilePath, handle.writeFile,
releaseRefreshLock, and getStaleRefreshLockId).
In `@packages/cli/src/controllers/project.ts`:
- Line 111: The fixture-mode call to readProjectListLocalBinding is missing the
required signal argument; update the fixture branch where
readProjectListLocalBinding(context.runtime.cwd, workspace, result.projects) is
invoked to include the same signal passed in the real branch
(context.runtime.signal) so it calls
readProjectListLocalBinding(context.runtime.cwd, workspace, result.projects,
context.runtime.signal).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8547c874-1844-4b49-8dde-822c9bfaac11
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
docs/product/error-conventions.mdpackages/cli/package.jsonpackages/cli/src/adapters/git.tspackages/cli/src/adapters/local-state.tspackages/cli/src/adapters/mock-api.tspackages/cli/src/adapters/token-storage.tspackages/cli/src/bin.tspackages/cli/src/cli.tspackages/cli/src/controllers/app-env.tspackages/cli/src/controllers/app.tspackages/cli/src/controllers/auth.tspackages/cli/src/controllers/project.tspackages/cli/src/lib/app/bun-project.tspackages/cli/src/lib/app/local-dev.tspackages/cli/src/lib/app/preview-build.tspackages/cli/src/lib/app/preview-provider.tspackages/cli/src/lib/auth/auth-ops.tspackages/cli/src/lib/auth/guard.tspackages/cli/src/lib/auth/login.tspackages/cli/src/lib/project/interactive-setup.tspackages/cli/src/lib/project/local-pin.tspackages/cli/src/lib/project/resolution.tspackages/cli/src/lib/project/setup.tspackages/cli/src/shell/command-runner.tspackages/cli/src/shell/errors.tspackages/cli/src/shell/runtime.tspackages/cli/tests/app-bun-compat.test.tspackages/cli/tests/app-controller.test.tspackages/cli/tests/app-local-dev.test.tspackages/cli/tests/app-state.test.tspackages/cli/tests/auth-login.test.tspackages/cli/tests/auth-ops.test.tspackages/cli/tests/auth-real-mode.test.tspackages/cli/tests/command-runner-auth.test.tspackages/cli/tests/helpers.tspackages/cli/tests/project-controller.test.tspackages/cli/tests/project-real-mode.test.tspackages/cli/tests/token-storage.test.ts
luanvdw
left a comment
luanvdw
left a comment
There was a problem hiding this comment.
Thanks @rtbenfield this is a great step forward for the overall architecture 🙏
I ran through it with my agent to educate my on the changes and see if it spots anything worth flagging for you to consider (these are mostly because this is a cross-cutting CLI contract)
These are all boundary-contract items rather than objections to the overall architecture:
-
packages/cli/src/adapters/local-state.ts:96-100LocalStateStore.write()now passes the commandAbortSignaldirectly into the persistentstate.jsonwrite. If cancellation lands mid-write, we can leave partially-written JSON behind and make the next CLI invocation fail for a reason unrelated to the user’s command.Suggestion: make persistent state writes atomic, or at least do pre/post
throwIfAborted()around an unabortable write. Since this is durable local CLI state, I’d prefer temp-file + rename, similar to the local pin path. -
packages/cli/src/adapters/token-storage.ts:123-139After
fs.open(lockFilePath, "wx")succeeds, this process owns the refresh lock. Ifhandle.writeFile(lockId, { signal })aborts or throws, we close the handle but leave the lock file behind, andacquireRefreshLock()never returns alockId, so the normalreleaseRefreshLock()cleanup path never runs.Suggestion: either avoid aborting that tiny lock-id write and check before/after, or track whether the lock id was written and unlink the lock file on write failure before rethrowing. Cancellation should shorten waits, not create a stale-lock wait for the next command.
-
packages/cli/src/adapters/git.ts:14-24readGitOriginRemote()now passessignaltoexecFile, but the blanket catch still converts aborts intonull. That means cancellation can be misreported as “no origin remote” at this adapter boundary.Suggestion: catch the error, rethrow when
signal?.abortedor the error is an abort, and only returnnullfor normal git lookup failures. -
packages/cli/src/controllers/project.tsThere are a few type-boundary drifts from threading
signalthrough this file:readProjectListLocalBinding()now requiressignal, but the fixture-mode call at line 111 still omits it.- The local
SourceRepositoryApiClientoverloads don’t acceptsignal, but the new call sites pass it intoGET/POST/DELETEoptions.
I know
tsc --noEmitalready has pre-existing repo failures, so I’m not treating global typecheck as newly broken by this PR. But these specific errors are PR-owned and they matter here because local types are the contract documenting which boundaries are cancellation-aware.
Non-blocking maintainability note: this PR now has small abortable sleep / pre-post boundary-check patterns in a few places. That’s okay for this patch, but once these fixes land we should consider a tiny shared helper for “abortable delay” and “unsupported boundary pre/post check” so future cancellation work does not drift by copy/paste.
Again: I think the architecture is spot on. These change are geared towards tightening boundaries.
|
Responding to the boundary-contract points here:
I did not extract the shared abortable delay / boundary-check helper in this PR pass. That cleanup is worth considering, but I kept this focused on tightening the concrete boundaries called out here rather than widening the patch. Verification run:
🤖 written by AI |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/bin.ts`:
- Around line 7-10: The worker branch calls runUpdateDiscoveryWorker() without
handling rejections; update that call site to attach a .catch handler to
runUpdateDiscoveryWorker() (the same call in packages/cli/src/bin.ts) that logs
the error and sets process.exitCode to a non-zero value (e.g., 1) so failures
don't yield unhandled promise rejections and the process exit code is
deterministic; keep the existing .then handler to set process.exitCode = 0 on
success and ensure the .catch handles any thrown error from
runUpdateDiscoveryWorker().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e50297cc-bca1-4250-9152-f1ca8369efbe
📒 Files selected for processing (2)
packages/cli/src/bin.tspackages/cli/src/cli.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/bin.ts`:
- Around line 7-10: The worker branch calls runUpdateDiscoveryWorker() without
handling rejections; update that call site to attach a .catch handler to
runUpdateDiscoveryWorker() (the same call in packages/cli/src/bin.ts) that logs
the error and sets process.exitCode to a non-zero value (e.g., 1) so failures
don't yield unhandled promise rejections and the process exit code is
deterministic; keep the existing .then handler to set process.exitCode = 0 on
success and ensure the .catch handles any thrown error from
runUpdateDiscoveryWorker().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e50297cc-bca1-4250-9152-f1ca8369efbe
📒 Files selected for processing (2)
packages/cli/src/bin.tspackages/cli/src/cli.ts
🛑 Comments failed to post (1)
packages/cli/src/bin.ts (1)
7-10:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle rejection in the update-check worker path.
runUpdateDiscoveryWorker()has no.catch(). If it rejects, this produces an unhandled promise rejection andprocess.exitCodeis never set, unlike the normal path below which deterministically resolves an exit code. Add a rejection handler so worker failures surface a non-zero exit code instead of an unhandled rejection.🛡️ Proposed fix
- runUpdateDiscoveryWorker().then(() => { - process.exitCode = 0; - }); + runUpdateDiscoveryWorker().then(() => { + process.exitCode = 0; + }).catch(() => { + process.exitCode = 1; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/bin.ts` around lines 7 - 10, The worker branch calls runUpdateDiscoveryWorker() without handling rejections; update that call site to attach a .catch handler to runUpdateDiscoveryWorker() (the same call in packages/cli/src/bin.ts) that logs the error and sets process.exitCode to a non-zero value (e.g., 1) so failures don't yield unhandled promise rejections and the process exit code is deterministic; keep the existing .then handler to set process.exitCode = 0 on success and ensure the .catch handles any thrown error from runUpdateDiscoveryWorker().
luanvdw
left a comment
luanvdw
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up! 🙇♂️
The remaining CodeRabbit note about adding a .catch() to the update-check worker promise seems like valid hygiene, but I don’t see it as introduced by or blocking this cancellation PR. Fine to handle opportunistically or as a small follow-up.
2 participants
Propagates command cancellation through the Prisma CLI so SIGINT/SIGTERM and AbortSignal-driven exits produce the documented COMMAND_CANCELED behavior instead of leaking provider-specific failures or hanging waits.
Changes
Why
Cancellation is a cross-cutting CLI contract, so the signal is owned at the runtime boundary and passed downward rather than creating command-local controllers. Supported SDK, child-process, and filesystem APIs receive the signal directly; unsupported boundaries use immediate pre/post abort checks so cancellation stays deterministic without pretending external APIs are interruptible.