Repository navigation
fix(sql): cancel remote queries after subprocess timeout - #115
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ) | ||
| const result = yield* Effect.scoped( | ||
| Effect.gen(function* () { | ||
| const lifecycle = yield* ShellLifecycle.acquire({ timeout }, warnings) |
There was a problem hiding this comment.
HIGH (Section A — upstream invasiveness). Confidence: high.
const lifecycle = yield* ShellLifecycle.acquire({ timeout }, warnings)This edit to packages/core/src/tool/bash.ts (plus the import { ShellLifecycle } from "../shell-lifecycle" at the top, the env: addition on line 165, and the Effect.scoped re-wrap) carries no cz-cli change banner and has no entry in packages/cz-cli/UPSTREAM-PATCHES.md.
packages/core is one of the four packages the ledger's invariant requires to stay pristine, and the failure mode here is the one the ledger was written to prevent: rg -n "cz-cli change" packages/core ... finds nothing, so at the next baseline bump upstream's bash.ts overwrites this file and the acquire call disappears. Unlike a crash, that failure is silent — every bash tool timeout goes back to leaking its remote ClickZetta job with no error on any surface.
Note the v2 bash tool does not trigger the shell.env plugin hook at all (unlike packages/opencode/src/tool/shell.ts:418), so for this file there is no partial-hook alternative even for the env half — the intrusion is unavoidable. That is a good argument to record in the ledger entry's Why intrusive (no hook) field, not a reason to skip the entry.
Please add the banner around the import and the Effect.scoped block, and fold this file into the same new INTRUSIVE entry as packages/core/src/shell-lifecycle.ts and packages/opencode/src/tool/shell.ts (the ledger already precedents multi-file entries — see entries 12 and 13).
| const lifecycle = yield* ShellLifecycle.acquire({ timeout: input.timeout }, cleanupWarnings) | ||
| yield* Effect.addFinalizer(closeSink) | ||
| const handle = yield* spawner.spawn(cmd(input.shell, input.command, input.cwd, input.env)) | ||
| const handle = yield* spawner.spawn( | ||
| cmd(input.shell, input.command, input.cwd, { ...input.env, ...lifecycle.env }), |
There was a problem hiding this comment.
HIGH (Section A — upstream invasiveness). Confidence: high.
const lifecycle = yield* ShellLifecycle.acquire({ timeout: input.timeout }, cleanupWarnings)
yield* Effect.addFinalizer(closeSink)
const handle = yield* spawner.spawn(
cmd(input.shell, input.command, input.cwd, { ...input.env, ...lifecycle.env }),
)No cz-cli change banner, and no matching entry in packages/cz-cli/UPSTREAM-PATCHES.md. packages/opencode is covered by the same pristine-package invariant, and the ledger's re-baseline sweep (rg -n "cz-cli change" packages/core packages/opencode packages/tui packages/schema) finds nothing here.
On whether a hook could have carried this — it is closer than for bash.ts, and the answer deserves to be in the ledger rather than left implicit. Unlike the v2 bash tool, this file already triggers a plugin hook for exactly the env half of the change:
// packages/opencode/src/tool/shell.ts:417-427
const shellEnv = Effect.fn("ShellTool.shellEnv")(function* (ctx: Tool.Context, cwd: string) {
const extra = yield* plugin.trigger("shell.env", { cwd, sessionID: ctx.sessionID, callID: ctx.callID }, { env: {} })
return { ...process.env, ...extra.env }
})shell.env carries callID, so SqlCleanupPlugin could have created the scope there and returned { CZ_SQL_CLEANUP: ... } keyed by callID with zero edits to this file for the injection side.
What shell.env cannot give is the release side, and that is the load-bearing argument: tool.execute.after (packages/plugin/src/index.ts:274) is triggered at packages/opencode/src/session/tools.ts:413-417, after the Effect.withSpan("Tool.execute") block, so it does not run when the tool fails or is interrupted — precisely the timeout/abort/SIGKILL cases this feature exists for. So the intrusion is justified, but only for the finalizer, and the entry should say so plainly: otherwise the next reader sees shell.env sitting right there and assumes the patch was avoidable.
Two consequences worth deciding deliberately and recording:
- Doing both halves here (rather than env via
shell.env, release via the intrusive finalizer) is the right call for keeping the two halves adjacent — but it means the patch is larger than strictly necessary, which raises the re-baseline conflict surface. Worth one sentence in the entry. - The banner must wrap all four edits in this file: the
Fiberimport, theShellLifecycleimport, theacquire+envblock here, and the[...cleanupWarnings]seed on line 573 (see my separate comment on line 566 about theFiber.joindrain, which I think does not belong in this PR at all).
| if (exit.kind === "exit") { | ||
| // Process exit can beat delivery of the last buffered output chunk. | ||
| // Bound the drain in case a background descendant keeps a pipe open. | ||
| yield* Fiber.join(output).pipe(Effect.timeoutOption("1 second")) | ||
| } |
There was a problem hiding this comment.
MEDIUM (Section B — unrelated drive-by; Section C — new failure propagation). Confidence: high.
if (exit.kind === "exit") {
// Process exit can beat delivery of the last buffered output chunk.
// Bound the drain in case a background descendant keeps a pipe open.
yield* Fiber.join(output).pipe(Effect.timeoutOption("1 second"))
}This, together with capturing the fiber as const output = ... and adding { startImmediately: true } on line 537, is an output-buffering fix that has nothing to do with SQL cancellation. Nothing in this PR's cleanup path reads the child's stdout — warnings travel through cleanupWarnings, not through list. It is a separate (plausibly real) bug about losing the last chunk of a short-lived command's output, and it lands in a pristine upstream file under the same missing banner.
Two reasons to split it out rather than carry it here:
1. It changes an error path from "swallowed" to "kills the tool". Effect.forkScoped previously left the reader fiber unobserved, so a failure inside Stream.runForEach was discarded and the tool still returned whatever output had accumulated. Fiber.join now re-raises that failure into the enclosing gen, which is .pipe(Effect.orDie) on line 571 — so it becomes a defect, not a ToolFailure. The reader callback has two failure sources on lines 511-527:
return trunc.write(full).pipe(
Effect.andThen((next) => Effect.sync(() => { file = next; ... })),
Effect.andThen(ctx.metadata({ metadata: { output: last } })),
)trunc.write is disk I/O (ENOSPC, EACCES on the truncation directory) and ctx.metadata is a session-state write. Either failing during a large-output command now takes the session down via orDie, where before it degraded to partial output. packages/core/src/tool/AGENTS.md is explicit that leaves should "Translate only expected typed errors into ToolFailure" and that defects must survive — turning a previously-swallowed I/O error into a defect is the opposite direction.
If the drain is kept, Fiber.await + ignoring the failure (or Fiber.join(output).pipe(Effect.ignore, Effect.timeoutOption("1 second"))) preserves the old tolerance while still getting the flush.
2. It is gated on exit.kind === "exit" only. On the timeout and abort paths the drain does not run, so the last-chunk loss this is meant to fix still happens there — and those are the paths this PR otherwise cares most about. If the buffering bug is real, that asymmetry looks unintentional; if it is deliberate (don't wait on a child you just killed), a comment saying so would help.
Test coverage: I could not find a test that exercises the drain. packages/opencode/test/tool/shell.test.ts is not run by cz-test.yml at all (see my comment there), so neither the fix nor the new defect path is covered by a check on this fork. I have not run anything — this is from reading the workflow and the Effect semantics, not from execution.
| const lease = yield* Effect.acquireRelease( | ||
| Effect.promise(() => factory(input)), | ||
| (lease) => | ||
| Effect.promise(async () => { | ||
| const result = await lease | ||
| .close() | ||
| .catch(() => ["Shell resource cleanup failed; remote work may still be running."]) | ||
| warnings.push(...result) | ||
| result.forEach((warning) => process.stderr.write(`${warning}\n`)) | ||
| }), | ||
| ) | ||
| Object.assign(env, lease.env) |
There was a problem hiding this comment.
MEDIUM (Section B — a fallback that hides the failure; Section D — wrong output channel). Confidence: high.
const lease = yield* Effect.acquireRelease(
Effect.promise(() => factory(input)),
(lease) =>
Effect.promise(async () => {
const result = await lease
.close()
.catch(() => ["Shell resource cleanup failed; remote work may still be running."])
warnings.push(...result)
result.forEach((warning) => process.stderr.write(`${warning}\n`))
}),
)Three separate problems in this block.
1. Effect.promise turns a factory rejection into a defect. Effect.promise models a promise that cannot fail — a rejection becomes an unrecoverable defect, not a typed error. createSqlCleanupScope calls Bun.serve({ hostname: "127.0.0.1", port: 0 }), which can reject for ordinary environmental reasons (EMFILE under fd pressure, a sandbox that forbids listening sockets, EADDRNOTAVAIL where loopback is restricted). In bash.ts the enclosing pipe is Effect.mapError(() => new ToolFailure(...)) (line 205), which maps typed errors only — a defect sails straight past it. So on a box that cannot bind a loopback port, every bash invocation dies with a defect, including ls. packages/core/src/tool/AGENTS.md is explicit that leaves should translate expected typed errors into ToolFailure; a lease factory failing to start is an expected typed error, not a defect. Effect.tryPromise with a tagged failure, or catching at the acquire boundary and degrading to "no lease + a warning", both keep the shell usable.
2. The .catch(() => [...]) discards the cause. Whatever close() actually threw — a DNS failure, an expired credential, a 403 from /lh/cancelJob — is replaced by a fixed string with no cause. The user is told "remote work may still be running" and given nothing to act on, and nothing is logged. This is the "fallback that hides the failure instead of surfacing it" pattern: including String(error) (or the job ID, which cancelJobAndWait already has) costs nothing and is the difference between an actionable warning and a dead end. Note the cz-side close() in cleanup-scope.ts is careful to name the job ID in its own warnings; that care is lost the moment it goes through this catch.
3. process.stderr.write from inside packages/core. The warnings are already returned via warnings, and both call sites surface them in structured tool output (bash.ts line 189/201 ...(warnings.length ? { warnings } : {}), shell.ts line 573 const meta: string[] = [...cleanupWarnings]). Writing the same text to process.stderr as well is redundant, and in the TUI it is actively harmful: the shell tool runs in the server worker of a process whose terminal is owned by an OpenTUI renderer, so a raw \n-terminated write lands mid-frame and corrupts the rendering. This is also the kind of thing the repo is careful about elsewhere — the ledger's hook entry 2 exists precisely because writing to the terminal from the wrong place is hard to get right here. Dropping the stderr write loses nothing, since both consumers already report the warnings.
| jobId, | ||
| hints: buildExecHints(opts?.hints, traceContext), | ||
| asynchronous: opts?.asynchronous, | ||
| jobTimeoutMs: opts?.asynchronous ? timeoutMs : lease.timeoutMs, |
There was a problem hiding this comment.
MEDIUM-HIGH (Section C — changed default with wide blast radius). Please confirm intent. Confidence: high on the mechanism, medium on the impact.
jobTimeoutMs: opts?.asynchronous ? timeoutMs : lease.timeoutMs,execSql has never passed jobTimeoutMs before. submit.ts:181 only emits the field when it is > 0, so until now no execSql call sent a server-side job timeout at all — the job ran under whatever default the deployment enforces. After this change every call sends one, and combined with the ?? 300_000 default on line 134 that means a 300-second server-side kill on every statement issued by every execSql caller that does not pass timeoutMs.
That set is large. rg -n 'execSql\(' packages/cz-cli/src finds ~30 call sites, and these pass no timeoutMs at all:
commands/table.ts— 9 sites, including DDL andALTERcommands/schema.ts— 4 sitescommands/workspace.ts— 2 sitescommands/status.ts— 2 sitescommands/fs.ts:219— volumePUT/GET, i.e. bulk file transfercommands/setup.ts:880commands/sql.ts:310,321—SHOW TABLES/DESC TABLE
cz-cli sql itself is unaffected in practice (--timeout already defaults to 300 and was already passed to pollJobResult as a client-side bound), so the new exposure is specifically the other commands, which previously had no time bound in either direction. A large CREATE TABLE AS SELECT through cz-cli table, or a multi-gigabyte cz-cli fs put, that legitimately runs past five minutes will now be killed by the server rather than completing.
specs/sql-cancellation.md does acknowledge this ("Every submitted ordinary SQL job receives a finite server timeout (default 300 seconds)... This is the last fallback if both processes die"), so I read it as deliberate for the shell-tool scenario the spec is about. What is not obviously deliberate is applying it to every non-shell CLI command too, where there is no orphan-job problem to solve: those commands run in the user's own terminal with no supervisor, so registerSqlCleanup returns undefined and the 300s cap buys nothing while capping work that used to succeed.
If that is intended, it is a user-visible default change that belongs in the PR description and the changelog. If not, gating on whether a supervisor was actually acquired (supervisor !== undefined) would confine the cap to the shell-tool path it was designed for.
Test coverage: none of table.ts / fs.ts / schema.ts / status.ts is exercised against a submit server in this PR's tests; the new suites drive execSql directly. I have not run the tests.
| const timeoutMs = opts?.timeoutMs ?? (Number(opts?.hints?.["sdk.job.timeout"]) * 1000 || 300_000) | ||
| if (!Number.isFinite(timeoutMs) || timeoutMs <= 0) throw new Error("SQL timeout must be positive and finite") |
There was a problem hiding this comment.
MEDIUM (Section C — CLI semantics change). Please confirm intent. Confidence: high.
const timeoutMs = opts?.timeoutMs ?? (Number(opts?.hints?.["sdk.job.timeout"]) * 1000 || 300_000)
if (!Number.isFinite(timeoutMs) || timeoutMs <= 0) throw new Error("SQL timeout must be positive and finite")cz-cli sql --timeout 0 previously meant no timeout: timeoutMs: 0 reached pollJobResult as jobTimeoutMs: 0, and poll.ts:488 guards with jobTimeoutMs !== undefined && ... — but before this PR that call site received opts?.timeoutMs directly, and submit.ts:181 skips emitting a non-positive jobTimeoutMs, so zero produced an unbounded poll with no server cap. The same held for a negative value.
Now both hard-error before anything is submitted, with a message ("SQL timeout must be positive and finite") that does not name the flag the user actually typed. The flag's own help text is "Job timeout in seconds (default: 300)" (sql.ts:841) and says nothing about zero being rejected, while its sibling --limit documents "0 for unlimited" — so zero-means-unlimited is a reasonable thing for a user to have assumed and scripted against.
Worth deciding explicitly:
- If rejecting non-positive is intended, the message should name the flag (
--timeout must be a positive number of seconds) and--timeout'sdescribeshould say so, since yargs will otherwise accept the value and fail deep insideexecSql. - If
0should keep meaning "no bound", it needs to survive to a sentinel rather than being validated away — though note that with this PR's design an unbounded lease also means an unbounded supervisor deadline, so "unlimited" may genuinely no longer be supportable. If that is the reason, saying so in the error message would help.
Test coverage: no test covers sql --timeout 0. The two hits for --timeout 0 in the suite (parameter-hardening.test.ts:569, runtime-config.test.ts:295) are agent run --timeout, an unrelated flag. I have not run the tests.
| export * as ShellLifecycle from "./shell-lifecycle" | ||
|
|
||
| import { channel } from "node:diagnostics_channel" | ||
| import { Effect } from "effect" | ||
|
|
||
| export interface Lease { | ||
| env: Record<string, string> | ||
| close(): Promise<readonly string[]> | ||
| } | ||
|
|
||
| export interface Input { | ||
| timeout: number | ||
| } | ||
|
|
||
| type Factory = (input: Input) => Promise<Lease> | ||
| type Request = { factories: Map<string, Factory> } | ||
|
|
||
| // Runtime plugins may be separately bundled. Node channels share subscriptions |
There was a problem hiding this comment.
HIGH (Section A — upstream invasiveness). Confidence: high.
This is a brand-new file inside packages/core, one of the four packages UPSTREAM-PATCHES.md requires to stay pristine. It carries no cz-cli change banner, and this PR does not touch packages/cz-cli/UPSTREAM-PATCHES.md at all (confirmed: git diff origin/main HEAD -- packages/cz-cli/UPSTREAM-PATCHES.md is empty).
export * as ShellLifecycle from "./shell-lifecycle"
import { channel } from "node:diagnostics_channel"rg -n "cz-cli change" packages/core packages/opencode packages/tui packages/schema — step 2 of the ledger's own re-baseline procedure — returns zero hits for this file. That grep is the only mechanism that finds these patches when upstream is bumped, and the ledger records that this exact failure has already cost this repo real patches (entry 1 was lost in the 1.4.7 → 1.17.11 re-baseline; entries 4, 7, 8, 9 were invisible to the checklist because they carried only cz_change: comments).
A whole new module is the hardest case for the sweep to recover, because unlike an edited line there is no upstream conflict to notice: a re-baseline that fast-forwards packages/core simply won't carry the file, and then packages/core/src/tool/bash.ts and packages/opencode/src/tool/shell.ts fail to resolve ../shell-lifecycle — or, if those edits are also dropped, SQL cancellation silently stops working with no error anywhere.
Two things are needed:
- Wrap the file body in the scannable banner:
//======================== cz-cli change ======================== ... rationale ... //====================== end cz-cli change ====================== - Add an INTRUSIVE entry carrying every field the ledger's existing entries carry: File, Marker, Upstream value (this file does not exist upstream), What/why, Why intrusive (no hook), History, Verify.
For the Verify field, note that the two tests this PR adds inside upstream packages are not reachable by CI — see my separate comment on .github/workflows/cz-test.yml's "Run cz-owned tests inside upstream packages" step.
For the Why intrusive field, the honest argument is available and worth writing down explicitly, because it is not obvious: tool.execute.after (packages/plugin/src/index.ts:274) is triggered at packages/opencode/src/session/tools.ts:413-417, after the Effect.withSpan("Tool.execute") block — so on interruption or failure the gen short-circuits and the hook never fires. Since the entire point of this feature is cleanup on timeout/abort/SIGKILL, that hook genuinely cannot carry it, and there is no other seam that gives a finalizer guaranteed to run after child-process teardown. Say that in the entry so the next person does not re-litigate it.
ab4595c to
1517c99
Compare
| await mkdir(directory, { recursive: true }) | ||
| await appendFile(logfile, "", { mode: 0o600 }) | ||
| return createSqlSupervisor({ |
There was a problem hiding this comment.
HIGH — a failure to open the diagnostics log disables supervision, which takes down all agent SQL. (confidence: high)
await mkdir(directory, { recursive: true })
await appendFile(logfile, "", { mode: 0o600 })
return createSqlSupervisor({All three of these sit inside one IIFE with a single .catch(() => undefined) on line 21, so mkdir or appendFile failing is indistinguishable from the TCP server failing to bind. Either way CZ_SQL_CLEANUP becomes "unavailable", and registerSqlCleanup then throws for every SQL call:
SQL cleanup supervisor unavailable; query was not submitted.
That means a read-only or non-writable $HOME/.clickzetta — a sandbox, a container with a different HOME, a root-created ~/.clickzetta with restrictive perms, a full disk — turns into 100% of agent SQL failing, even though the loopback socket and the cancellation logic would both have worked fine. The diagnostic sink is not load-bearing for the guarantee this feature makes; it should not be able to withdraw admission.
The root cause looks like ordering-for-convenience: the comment says "Open diagnostics before admitting jobs, not in a failing finalizer", which is right about not deferring the failure, but the conclusion should be to degrade the sink, not the supervisor. The smaller correct change is to create the supervisor regardless and let onWarning fall back (stderr when no TUI owns the terminal, or drop with a one-time note) when the log is unwritable — so an unconfirmed-cleanup record is lost rather than every query being refused.
Separately: { mode: 0o600 } only applies when the file is created, so an existing sql-cleanup.jsonl with looser permissions keeps them. Worth a chmod if the 0600 matters.
| jobId, | ||
| hints: buildExecHints(opts?.hints, traceContext), | ||
| asynchronous: opts?.asynchronous, | ||
| jobTimeoutMs: opts?.asynchronous ? timeoutMs : lease.timeoutMs, |
There was a problem hiding this comment.
Please confirm the intent: --async jobs now get a hard 300s server-side timeout by default. (behavioral change, high confidence on the mechanism)
jobTimeoutMs: opts?.asynchronous ? timeoutMs : lease.timeoutMs,execSql previously never passed jobTimeoutMs, so jobDesc.jobTimeoutMs was never emitted (submit.ts:181 only emits it when > 0) and the server used its deployment default. Two consequences of now emitting it:
cz-cli sql --async.sql.ts:382setsasyncHints["sdk.job.timeout"] = String(argv.timeout), and--timeouthas a yargsdefault: 300— so an invocation that never mentioned a timeout producestimeoutMs = 300_000on line 137, which is now sent to the server. The flag's own help text says--asyncis "for large/long-running queries", and itsaiMessagetells the user to come back later withcz-cli job status. Those jobs will now be killed server-side at 5 minutes. The default is indistinguishable from an explicit--timeout 300at this layer, sospecs/sql-cancellation.md's "Positive explicit timeouts are sent to the server, including for--async" doesn't actually separate the two cases.- Agent-supervised metadata commands. Line 138's
process.env[CLEANUP_ENV] ? 300_000 : undefinedgivestable list,schema show,fs, etc. a 5-minute server-side cap when run under the agent, where standalone they keep the deployment default.sql-timeout-options.test.ts:38-45asserts the standalone case staysundefined; nothing covers the supervised case, and nothing covers the--asyncdefault.
If the --async cap is intended, the fix is probably to distinguish "user passed --timeout" from the yargs default (argv.timeout !== undefined on a default-less option, or yargs.parsed.defaulted) and only send it when the user asked.
Minor, same line: the ternary has the same value on both branches — lease.timeoutMs is the timeoutMs argument trackSqlJob was handed on line 143, so opts?.asynchronous ? timeoutMs : lease.timeoutMs reduces to timeoutMs. If the two were meant to differ, one of them is wrong.
| if (lease.signal.aborted) { | ||
| const failure = new Error(`Job ${jobId.id} interrupted or timed out`) | ||
| Object.assign(failure, { jobId: jobId.id }) | ||
| throw failure |
There was a problem hiding this comment.
MEDIUM — this replaces every error with a message that hides which of three causes fired, and it mislabels the result as a timeout. (confidence: high)
if (lease.signal.aborted) {
const failure = new Error(`Job ${jobId.id} interrupted or timed out`)
Object.assign(failure, { jobId: jobId.id })
throw failure
}lease.signal aborts for three distinct reasons, and the caller can no longer tell them apart:
- the
abortAfter(timeoutMs)deadline (a real timeout), lease.abort()fromstop()on SIGINT/SIGTERM,lease.abort()passed asonLosttoregisterSqlCleanup— the supervisor connection died while the query was healthy.
The third is the one that matters: a user whose query was killed because the loopback supervisor dropped gets told their query timed out. classifyExecError (exec.ts:251) matches on the message substring /timed out/i with a jobId present, so all three render as JOB_TIMEOUT with aiMessage advising "For long-running queries, use --async" — actively wrong advice for a dropped supervisor, and it will send the agent down the wrong path.
It also discards the original error unconditionally. A ClickZettaApiError (say a 403 from submitJob) that happens to land in the same tick as a SIGINT is reported as a timeout.
Suggested shape: carry the abort reason through. controller.abort(new Error("SQL execution interrupted")) in sql-lifecycle.ts:21 and the onLost path already have distinct reasons available — thread lease.signal.reason into the message (or set a distinct code so classifyExecError can branch) and keep { cause: error } on the wrapper so the original survives --debug.
| error: { code: "ABORTED", message: `Execution interrupted by ${signal}.` }, | ||
| job_ids: jobs.map(([id]) => id), |
There was a problem hiding this comment.
MEDIUM — the Ctrl-C output shape changed: job_id (string) became job_ids (array), and the message text changed too. (confidence: high)
error: { code: "ABORTED", message: `Execution interrupted by ${signal}.` },
job_ids: jobs.map(([id]) => id),The handler this replaces (sql.ts, removed in this PR) emitted:
const payload: Record<string, unknown> = { error: { code: "ABORTED", message: "Execution interrupted by user." } }
if (currentJobId) payload.job_id = currentJobIdSo for cz-cli sql under --format json, a Ctrl-C response goes from {"error":{...},"job_id":"…"} to {"error":{...},"job_ids":["…"]}, and the message from Execution interrupted by user. to Execution interrupted by SIGINT.. Anything reading .job_id off an aborted run — a wrapper script, a retry harness, the agent's own prompt text — silently gets undefined. renderErrorOutput under a row format changes the same way.
If the plural key is needed (it is — status.ts:21, schema.ts:67 and profile.ts:626 run execSql concurrently via Promise.all, so more than one job can be in flight), consider emitting both: job_ids plus job_id: jobs[0]?.[0] for the single-job case, which is the overwhelmingly common one. Either way this belongs in the changelog — I don't see a test pinning either shape.
| shutdown = Promise.resolve().then(async () => { | ||
| const jobs = [...active.entries()] | ||
| jobs.forEach(([, lease]) => lease.abort()) | ||
| const deadline = setTimeout(() => process.exit(exitCode), 2000) |
There was a problem hiding this comment.
LOW/MEDIUM — both process.exit() calls here skip telemetry flushing, and SIGTERM is a newly-intercepted signal. (confidence: high)
const deadline = setTimeout(() => process.exit(exitCode), 2000)…and line 112's process.exit(exitCode).
.github/claude-review-context.md flags this repo specifically: "process.exit() paths. Early exits skip telemetry flushing and cleanup." bootstrap/runtime.ts and run-cli.ts reach exit through flushOtel() / flushLangfuse(); this path does not, so an interrupted SQL run loses its spans. The removed sql.ts handler had the same gap for SIGINT, so that part isn't new — but two things are:
- SIGTERM is now intercepted. Previously SIGTERM used Node's default (immediate termination). Now it runs the same cancel-then-exit path. That's the point of the PR, but it means
timeout 30 cz-cli sql …,kill, and container shutdown all now take up to 2s longer and go through an exit path that skips flushing. - The hard 2s bound races the cancellation it's protecting.
lease.finish("cancel")callscancelJobAndWait(opts, job, 1500), andcancelJobAndWaitpollscancelJob+getJobResultRawin a loop with a 250ms delay. On a slow link a single round trip can exceed 1500ms, so the common outcome under load isconfirmed: false→ the "cancellation unconfirmed" stderr line →process.exitat the 2s deadline before the write on line 101 lands. Not wrong, but the user sees the unconfirmed warning rather than a confirmed cancel most of the time on a slow network, and in the standalone case there's no supervisor to recover it.
Worth adding await flushOtel() (guarded by its own short timeout) before both exits.
| if (Object.keys(raw).length > 0 && value === undefined && raw.code === undefined) { | ||
| throw new ClickZettaApiError("INVALID_CANCEL_RESPONSE", "Missing cancellation status") | ||
| } |
There was a problem hiding this comment.
MEDIUM — this heuristic can turn a successful cancellation into a hard error, and it now applies to the pre-existing cz-cli job cancel. (confidence: medium — depends on the real CancelJobResponse JSON)
if (Object.keys(raw).length > 0 && value === undefined && raw.code === undefined) {
throw new ClickZettaApiError("INVALID_CANCEL_RESPONSE", "Missing cancellation status")
}The rule is: {} is success, but any non-empty object lacking respStatus/resp_status/code is an error. Proto3 JSON omits empty fields individually, not the whole message — so a CancelJobResponse that carries any other populated field (a job_id echo, a state, a gateway-added envelope key like requestId) while resp_status is empty-and-omitted lands in this branch and is reported as a failure of a cancellation that actually succeeded. The comment on line 16 acknowledges {} is valid but the code doesn't extend that reasoning to { someOtherField }.
Line 31 has the same character: a benign errorCode such as "job already finished" becomes a throw.
Blast radius is wider than the new code. packages/cz-cli/src/commands/job.ts:399 — await cancelJob(ctx.clientOpts, jobId) — is an existing caller that was not updated: it previously could not throw on a 2xx, and now reports JOB_CANCEL_ERROR with this message. Inside cancelJobAndWait the damage is bounded (the error only sets reason, and the getJobResultRaw poll still confirms), but job cancel has no such second opinion.
Inverting the rule would be safer and matches proto3 semantics: treat a 2xx with no populated error status as success, and only reject when respStatus/resp_status is present and carries an errorCode/errorMsg. If you want to keep the strict check, job.ts:399 should be switched to cancelJobAndWait so the confirmation poll decides, rather than the response-shape guess.
Related: cancelJob also changed from request() to requestRaw(), so its declared return type went from ApiResponse<unknown> to unknown. That's benign today (both paths run the same doRequest body and parseWrapper is unused), but it is a public SDK export — worth a note if the SDK is versioned independently.
| if (!Number.isFinite(argv.timeout) || argv.timeout <= 0) { | ||
| error("USAGE_ERROR", "--timeout must be a positive, finite number of seconds (0 is not supported).", { format, exitCode: 2 }) | ||
| return | ||
| } |
There was a problem hiding this comment.
MEDIUM — --timeout 0 goes from "works" to exit code 2. That's a breaking CLI change worth calling out explicitly. (confidence: high)
if (!Number.isFinite(argv.timeout) || argv.timeout <= 0) {
error("USAGE_ERROR", "--timeout must be a positive, finite number of seconds (0 is not supported).", { format, exitCode: 2 })Previous behavior of --timeout 0 differed by mode:
--async:if (argv.timeout)on line 382 treated 0 as falsy, so nosdk.job.timeouthint was sent and the job was submitted on the deployment default.cz-cli sql "…" --async --timeout 0succeeded; it now exits 2 without submitting.- sync:
jobTimeoutMs: 0madepoll.ts:488'sDate.now() - startTime > 0true on the first iteration, so it failed almost immediately withJob … timed out after 0ms. Rejecting it up front is strictly better here.
So the async case is a real regression for anyone who used 0 as "no client-side limit" — a natural reading, and it did work. specs/sql-cancellation.md says "zero is not an unlimited-timeout sentinel", which is a fine decision, but this is still an exit-code change on a previously-working invocation and belongs in release notes.
sql-timeout-options.test.ts:22-30 covers the new rejection for 0, -1, NaN, Infinity in both modes, so the new behavior is pinned; nothing documents the migration.
One inconsistency: this gate runs at the top of handler, but argv["job-profile"] is handled a few lines below and never uses argv.timeout. cz-cli sql --job-profile <id> --timeout 0 now fails for a flag that path ignores.
| baseUrl: z | ||
| .string() | ||
| .url() | ||
| .refine((url) => ["http:", "https:"].includes(new URL(url).protocol)), |
There was a problem hiding this comment.
LOW — the supervisor will POST a client-supplied credential to a client-supplied URL, so the capability token is an outbound-request primitive, not just a cancel primitive. (confidence: high on the mechanism, low on exploitability)
baseUrl: z
.string()
.url()
.refine((url) => ["http:", "https:"].includes(new URL(url).protocol)),baseUrl is validated for scheme only, then stored on line 105 and used by cancelJobAndWait → requestRaw on disconnect. So anything holding the secret can register baseUrl: "http://attacker.example/", disconnect, and make the agent process issue an authenticated POST to /lh/cancelJob there with a body and credential of the caller's choosing, retried in a loop for the full 5s cleanup budget. The registering client supplies its own credential, so this leaks nothing of the user's — but it does give a request originating from inside the agent's network position, which is the interesting part in a VPC.
The gate is the secret, which is in CZ_SQL_CLEANUP in the environment of every process the agent spawns — including arbitrary commands the model chooses to run and any MCP server. Two practical exposures worth weighing: the agent running env/printenv puts the token into the transcript and, per .github/claude-review-context.md, command arguments and output are recorded to OTel; and any tool that dumps its environment on crash does the same.
Not a blocker given it needs local same-user access. Two cheap hardenings:
- Pin
baseUrlto the origin the child's ownclient.baseUrlresolved to, or at least reject non-public/loopback/link-local hosts. The supervisor never legitimately needs an origin the cz layer didn't configure. - Line 97's
result.data.secret !== secretis a non-constant-time comparison. Loopback + a UUID makes timing exploitation unrealistic, butcrypto.timingSafeEqualcosts nothing here.
| while (!socket.destroyed) { | ||
| if (socket.writableLength > 64 * 1024) return socket.destroy() |
There was a problem hiding this comment.
LOW — writableLength is the outbound buffer; checking it in the read loop can kill a healthy connection. (confidence: high)
while (!socket.destroyed) {
if (socket.writableLength > 64 * 1024) return socket.destroy()The surrounding comment says "Bound each newline-delimited UTF-8 frame before parsing", and the two checks that do that are the end > 64 * 1024 on the next line and the buffer.length check at the end. socket.writableLength measures bytes queued for writing to the peer — unrelated to frame size. On the supervisor side it means: if a child stops draining its socket while the supervisor keeps writing 1/s heartbeat replies, the supervisor eventually crosses the threshold and destroy()s the connection, which on line 86-90 routes to cleanup() and cancels the child's still-running query.
64 KiB of {"type":"heartbeat"}\n is ~3,300 replies, so at 1/s this needs a peer wedged for roughly an hour — unlikely to bite in practice, but the heartbeat lease on line 116 is the mechanism that's supposed to handle a frozen peer, and it does so in 10s with the right semantics. This check adds a second, slower, differently-motivated trigger for the same condition.
I'd just drop the line. If backpressure on writes is a real concern, check socket.write()'s return value at the two write sites instead.
| } finally { | ||
| if (previous === undefined) delete process.env[CLEANUP_ENV] | ||
| else process.env[CLEANUP_ENV] = previous | ||
| await supervisor?.close() |
There was a problem hiding this comment.
MEDIUM — agent shutdown now blocks on cleanup, and this await can reject and replace main()'s exit code with an unhandled rejection. (confidence: high)
await supervisor?.close()Two things, both downstream of the process.exit() that this PR removed from runtime.ts's finally (line 493 now return (process.exitCode as number) ?? 0).
Exit latency. close() does for (const socket of sockets) cleanup(socket) then Promise.allSettled([...pending]), and each cleanup runs cancelJobAndWait(…, 5000). So quitting cz-agent while a SQL job is registered — a /sql still running, a bash-tool cz-cli sql that hasn't unregistered — now stalls for up to 5 seconds with no output, after the TUI has already torn down. Previously process.exit() in main()'s finally made exit immediate. specs/sql-cancellation.md covers this as "Normal bootstrap return closes the supervisor and attempts cancellation of all outstanding registrations", but it doesn't mention that the user waits for it. Consider a shorter budget on the shutdown path specifically, or printing a one-line "cancelling N remote queries…" so the pause is explained rather than looking like a hang.
Rejection. close()'s tail is server.close((error) => (error ? reject(error) : resolve())). This await sits in a finally, so a rejection here discards run()'s return value and propagates out of main(). boot.ts:16 is process.exit(await main(args)) and run-cli.ts:372 is const code = await main(rawArgs, true) — neither has a catch, so instead of exiting with the command's real exit code the process dies on an unhandled rejection. .catch(() => {}) on this line is enough; closing a listener that is already unref'd and whose sockets have all been destroyed has nothing left to report.
For the record, I did check the process.exit() removal itself and it looks correct: both callers (boot.ts:16, run-cli.ts:372-373) call process.exit with the returned code, so lingering handles from the TUI's Bun Worker are still killed, and flushOtel/flushLangfuse still run first. The only new failure mode is the rejection path above.
| const deadline = abortAfter(2000, signal) | ||
| try { | ||
| const credential = await abortable(client.tokens.get(), deadline.signal) |
There was a problem hiding this comment.
MEDIUM/HIGH — the 2s registration budget covers a credential resolution that can legitimately take longer than 2s, and blowing it fails SQL closed. (confidence: high)
const deadline = abortAfter(2000, signal)
try {
const credential = await abortable(client.tokens.get(), deadline.signal)client.tokens.get() is not a cache read in the general case. For a profile-backed OAuth identity it goes through auth/token.ts:167-168 → refreshOrLogin → refreshAccessToken, i.e. an HTTP round trip to the portal (token.ts:104). The rest of the SDK gives that call the ordinary DEFAULT_TIMEOUT_MS = 60_000 (client.ts:18); here it gets 2s shared with the TCP connect and the register/ack handshake.
Consequence on a cold or just-expired token over a slow link: abortable rejects → the catch on line 274 sets done, abandon()s, and rethrows → trackSqlJob catch → execSql throws before submitting. The user-visible message is the one built in lost():
SQL job …: cleanup supervisor connection lost
which points at the supervisor when the actual cause was a slow token refresh. So the first query after a token expiry — reliably the worst-latency moment — is the one most likely to be refused, with a message that misdirects debugging. specs/sql-cancellation.md lists Registration as "2 seconds, also subject to the query deadline" but doesn't note that credential acquisition is inside that budget.
Two options: resolve the credential before starting the 2s clock (it's needed regardless of supervision, and a real timeout on it is a better error than a supervisor error), or give the credential step its own budget derived from the query deadline and keep 2s for socket + handshake only.
The same 2s also bounds ready.promise on line 273, which is the part 2s is genuinely right for.
Review summaryTen inline findings. Verdict per section: A. Upstream invasiveness — no issues found. This PR touches no file under B. Clean fix vs. hole drilled around the problem — mostly the right fix, three exceptions. The core premise is correct and not a symptom patch: killing a subprocess genuinely does not cancel a submitted job, there is no upstream hook exposing the shell's remaining budget, and threading a real
Also one leftover: C. Regression risk — see inline; six behavioral changes, three of them uncovered by tests.
Callers I checked with Grep rather than assuming: every One thing not worth an inline comment but worth weighing: under the agent, every SQL call — including short metadata queries from I did not run any tests — nothing above should be read as a claim that the suite passes. |
| job: z.object({ | ||
| id: z.string().min(1).max(256), | ||
| workspace: z.string().min(1), | ||
| instanceId: z.number().int().positive(), |
There was a problem hiding this comment.
HIGH — instanceId: 0 is a legitimate sentinel, and this schema rejects it, which fails SQL closed. (confidence: high on the code path)
instanceId: z.number().int().positive(),Two existing execSql callers deliberately pass instanceId: () => 0 because no instance id has been resolved yet:
packages/cz-cli/src/commands/profile.ts:374—execSql({ …, instanceId: () => 0 }, "SELECT 1", …), with the comment "A verification query on a profile that does not exist yet, so there is no resolved instance id to give its JobID"packages/cz-cli/src/commands/setup.ts:892— same, "Setup runs this before the profile is written"
Under the agent (CZ_SQL_CLEANUP set in the child's env), the path is: registration.safeParse fails → cleanup(connection) destroys the socket → child's socket.on("close", lost) → ready.reject → registerSqlCleanup throws SQL job …: cleanup supervisor connection lost → trackSqlJob catch → execSql throws before submitting anything. So cz-cli profile add --verify and cz-cli setup break with a supervisor error that has nothing to do with what the user did, whenever the agent's shell runs them.
credential.instanceId on line 27 is correctly z.number() without .positive(), which makes the asymmetry look unintentional rather than a deliberate rejection of 0.
Smallest correct change: z.number().int().nonnegative(). The value is only used to build the cancel body and the dedupe key, neither of which needs it to be non-zero.
Also worth checking workspace: z.string().min(1) on line 18 for the same reason — setup.ts runs before the profile is written, so confirm config.workspace is always non-empty there.
No test covers this: every case in sql-cleanup-scope.test.ts and sql-cancellation.test.ts uses instanceId: () => 1.
--dry-run reported per-statement errors but exited 0. Batch and dry-run failures now also record lastError so telemetry carries the error. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- accept instanceId 0 so profile add --verify and setup work under supervision - keep supervision when the diagnostics log is unwritable; tolerate close errors - resolve credentials under the query deadline, not the 2s handshake budget - name abort causes (timeout, signal, lost supervisor) and keep the cause - keep job_id and the user-interrupt message in the signal envelope - send no server timeout for --async unless --timeout is explicit - accept proto3 cancel responses without an error status - validate --timeout after --job-profile, which ignores it - drop the outbound-buffer read check; compare secrets in constant time Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A definitive 4xx from submitJob means no job was created, so waiting 1.5s to cancel it only delayed batch failures and printed a false unconfirmed-cancellation warning. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1517c99 to
13f661b
Compare
| const explicitTimeoutMs = opts?.timeoutMs ?? ( | ||
| opts?.hints?.["sdk.job.timeout"] !== undefined ? Number(opts.hints["sdk.job.timeout"]) * 1000 : undefined | ||
| ) | ||
| if (explicitTimeoutMs !== undefined && (!Number.isFinite(explicitTimeoutMs) || explicitTimeoutMs <= 0)) { | ||
| throw new Error("SQL --timeout must be a positive, finite number of seconds") | ||
| } | ||
| // HYBRID mode: submitJob may return the result directly if the query | ||
| // finished within hybridPollingTimeout. Check for a terminal state. | ||
| const raw = submitResp as { status?: { state?: string } } | ||
| let result: QueryResult | ||
| if (raw?.status?.state && ["SUCCEED", "FAILED", "CANCELLED"].includes(raw.status.state)) { | ||
| const errorCode = retryableSubmitCode(submitResp) | ||
| if (isRetryableErrorCode(errorCode)) { | ||
| result = await pollJobResult(ctx.clientOpts, jobId, { jobTimeoutMs: opts?.timeoutMs, timezone }) | ||
| // Supervised jobs need a finite fallback in case both processes die before cleanup. | ||
| const timeoutMs = explicitTimeoutMs ?? (process.env[CLEANUP_ENV] ? 300_000 : undefined) | ||
| const lease = await trackSqlJob(ctx.clientOpts, jobId, timeoutMs) | ||
| const client = { ...ctx.clientOpts, signal: lease.signal } | ||
| let disposition: "terminal" | "detached" | "cancel" = "cancel" | ||
| try { | ||
| const traceContext = currentTraceContext() | ||
| const submitResp = await submitJob(client, { | ||
| sql: normalizedSql, | ||
| workspace: ctx.config.workspace, | ||
| schema: ctx.config.schema, | ||
| vcluster: ctx.config.vcluster, | ||
| instanceName: ctx.config.instance, | ||
| instanceId: ctx.instanceId(), | ||
| jobId, | ||
| hints: buildExecHints(opts?.hints, traceContext), | ||
| asynchronous: opts?.asynchronous, | ||
| // A detached job outlives its supervisor by design: only an explicit timeout reaches the server. | ||
| jobTimeoutMs: opts?.asynchronous ? explicitTimeoutMs : timeoutMs, |
There was a problem hiding this comment.
MEDIUM (confidence: high) — the 300 s default does reach the server on the sync path, contradicting the spec.
const explicitTimeoutMs = opts?.timeoutMs ?? (
opts?.hints?.["sdk.job.timeout"] !== undefined ? Number(opts.hints["sdk.job.timeout"]) * 1000 : undefined
)
...
jobTimeoutMs: opts?.asynchronous ? explicitTimeoutMs : timeoutMs,specs/sql-cancellation.md states: "Positive explicit timeouts are sent to the server … the 300-second default is not, because it bounds waiting rather than a detached job." sql.ts adds explicitTimeout to keep that promise — but only the --async branch reads it. Every sync branch passes the defaulted value:
sql.ts:407,410,455,476,662,710,733→timeoutMs: argv.timeout * 1000argv.timeoutisargs.timeout ?? 300(registration wrapper), so with no--timeoutthis is300000
So opts.timeoutMs = 300000 → explicitTimeoutMs = 300000 → timeoutMs = 300000 → jobTimeoutMs: 300000, and submit.ts:181 emits jobDesc.jobTimeoutMs = 300000.
Before this PR exec.ts never passed jobTimeoutMs to submitJob at all. Net effect: every default cz-cli sql now installs a server-enforced 300 s job kill, and cz-cli profile add --verify (profile.ts:374, timeoutMs: 30000) and job.ts:425 now ship a 30 s server-side kill too. Previously these were purely client-side polling bounds, with a best-effort cancelJob from poll.ts:488.
explicitTimeoutMs cannot tell "user typed --timeout" from "default applied" because sql.ts collapses both into opts.timeoutMs before execSql sees them. The smaller correct change is to thread the explicit/default distinction down to execSql the same way sql.ts already threads it for --async (e.g. an explicitTimeoutMs field on opts, with timeoutMs kept as the wait bound), rather than re-deriving it from a value that has already lost the distinction.
test/sql-cancellation.test.ts:12,61 asserts jobDesc.jobTimeoutMs is sent for explicit timeouts; nothing covers the "default must not be sent" case, which is why the divergence is invisible.
| process.on("SIGINT", () => { | ||
| // Active SQL owns graceful cancellation before exiting. | ||
| if (hasActiveSqlJobs()) return |
There was a problem hiding this comment.
MEDIUM (confidence: high) — this guard only exists on the dev entry point, so the shipped binary loses SIGINT coverage for the pre-submission phase of cz-cli sql.
process.on("SIGINT", () => {
// Active SQL owns graceful cancellation before exiting.
if (hasActiveSqlJobs()) returnsql.ts's old handler was installed at the top of handler(), before isSplitEnabled() and getExecContext() — so Ctrl-C during profile resolution / token fetch (a portal round trip, which is the slow part) printed the ERROR ABORTED envelope and exited 130. This PR deletes it and relies on the handlers that trackSqlJob installs, which only exist once execSql is already running.
The gap is masked in dev because src/main.ts has the global handler above. The released binary does not go through this file: package.json declares bin: ./src/main.ts, but script/build.ts:258 compiles ../cz-cli/src/bootstrap/boot.ts, and boot.ts → runtime.main → runCliWithTracking registers no SIGINT handler anywhere (rg "process.on\(" packages/cz-cli/src → only main.ts, sql-lifecycle.ts, runtime.ts's rejection handlers, and the otel plugin).
So in the shipped binary, Ctrl-C before the job exists now falls through to Node's default: no envelope on stdout, and scripts that parsed ERROR ABORTED / {"error":{"code":"ABORTED"}} see nothing. No test covers this window — test/sql-cancellation.test.ts signals the child only after submission (ready.promise).
The straightforward fix is to install the same global handler in boot.ts/runtime.ts rather than only in main.ts, so both entry points behave identically.
| if (raw === "unavailable") | ||
| throw new Error( | ||
| "SQL cleanup supervisor unavailable; query was not submitted. Check local socket and ~/.clickzetta write permissions.", | ||
| ) |
There was a problem hiding this comment.
MEDIUM (confidence: high) — the error names a cause that can never produce it.
if (raw === "unavailable")
throw new Error(
"SQL cleanup supervisor unavailable; query was not submitted. Check local socket and ~/.clickzetta write permissions.",
)supervisor-runtime.ts sets "unavailable" only when createSqlSupervisor(...) rejects — i.e. the loopback TCP bind failed. The ~/.clickzetta write is handled separately and is explicitly best-effort:
const diagnostics = await mkdir(directory, { recursive: true })
.then(() => appendFile(logfile, "", { mode: 0o600 }))
.then(() => chmod(logfile, 0o600))
.then(() => true, () => false)with the comment "an unwritable ~/.clickzetta loses unconfirmed-cleanup records, but must not withdraw supervision". So an unwritable ~/.clickzetta never reaches this message, and a user who follows the advice will chase the wrong thing while the real cause (no loopback listener — container network policy, sandbox, exhausted ports) goes undiagnosed.
Worse, createSqlSupervisor's rejection is swallowed by .catch(() => undefined), so the actual bind error is discarded entirely. Propagating that error text into the env value (or at least logging it) would make this message able to name the real cause.
| if (raw.code !== undefined && ![0, "0", 200, "200", "SUCCESS"].includes(raw.code as string | number)) { | ||
| throw new ClickZettaApiError(String(raw.code), String(raw.message ?? raw.msg ?? "Cancellation rejected")) | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: medium) — the new raw.code allowlist changes cz-cli job cancel output and will reject some success responses.
if (raw.code !== undefined && ![0, "0", 200, "200", "SUCCESS"].includes(raw.code as string | number)) {
throw new ClickZettaApiError(String(raw.code), String(raw.message ?? raw.msg ?? "Cancellation rejected"))
}Two concerns:
-
code: nullthrows.null !== undefinedpasses the guard and is not in the allowlist, so a gateway that serialises an absent code asnull(common for JSON envelopes that don't use proto3 omission) yieldsClickZettaApiError("null", "Cancellation rejected"). TherespStatuscheck directly above is careful to treat an absent status as success; this one is not.test/fixtures/cancellation.ts:23covers{ code: 0, data: {} }but notnull. -
Changed behavior for an existing command.
packages/cz-cli/src/commands/job.ts:399callscancelJoband, on success, prints{ job_id, cancelled: true }. With this validation a coordinator rejection now throws, so that call site emitsJOB_CANCEL_ERRORwith exit 1 where it previously reportedcancelled: trueand exit 0. That is almost certainly the better behavior, but it is an output-shape change forcz-cli job cancelthat scripts may parse, it is unrelated to the PR's stated purpose, and nothing in the test suite covers thejob cancelcommand path.
(The request → requestRaw switch itself is inert — doRequest's parseWrapper is unused, void parseWrapper at client.ts:196 — so the only behavior delta here is this validation.)
| // The default 300s bounds waiting, not a detached job: only an explicit value is sent. | ||
| if (argv.explicitTimeout !== undefined) asyncHints["sdk.job.timeout"] = String(argv.explicitTimeout) |
There was a problem hiding this comment.
MEDIUM — please confirm this is intended (confidence: high)
// The default 300s bounds waiting, not a detached job: only an explicit value is sent.
if (argv.explicitTimeout !== undefined) asyncHints["sdk.job.timeout"] = String(argv.explicitTimeout)Previously this was if (argv.timeout) with default: 300, so every --async / --no-sync submission carried sdk.job.timeout=300. After this change a plain cz-cli sql --async "<query>" sends no sdk.job.timeout at all, so the detached job has no server-side bound — it runs until it finishes or an operator kills it.
The rationale in the comment is coherent for a job nobody is waiting on, and specs/sql-cancellation.md records it. But the blast radius is a runaway query on a billed lakehouse, and it affects the one path where no client is left to clean up. Worth confirming you want the previous implicit 300 s ceiling removed rather than, say, keeping it for --async and dropping it only where the deadline is genuinely a wait bound.
No test covers the "no sdk.job.timeout when the default applies" case; test/sql-timeout-options.test.ts and test/sql-cancellation.test.ts:12,61 both assert the explicit-value path.
| // Per-statement failures are already rendered inline, so error() would print a | ||
| // second envelope; set the exit code and telemetry error it would have set. | ||
| function markStatementFailure(message: string) { | ||
| process.exitCode = EXIT_BIZ_ERROR | ||
| ;(process as unknown as Record<string, unknown>).lastError = message | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: high) — unrelated drive-by behavior change, and it leaves the envelope and the exit code disagreeing.
function markStatementFailure(message: string) {
process.exitCode = EXIT_BIZ_ERROR
;(process as unknown as Record<string, unknown>).lastError = message
}This PR is "cancel remote queries after subprocess timeout". The batch/dry-run exit-code change is a separate fix with its own test file (test/sql-batch-exit-code.test.ts) and its own --batch / --dry-run describe-text rewrites. Mixing it in means the cancellation work can't be reverted independently, and it changes a user-visible CLI contract: cz-cli sql -B and cz-cli sql --dry-run now exit 1 when any statement fails, where they previously exited 0. Any CI step or set -e script that runs a batch with a tolerated failure starts failing on upgrade. Worth splitting out, or at minimum calling out in the PR description.
On the dry-run path specifically (line 675-676), there is an inconsistency:
success({ statements: results, count: statements.length }, { format, rowsKey: "statements" })
const failed = results.find((r) => r.status === "error")
if (failed) markStatementFailure(String(failed.error))success() has already written a success envelope and set process.exitCode = EXIT_OK (output/index.ts:68); markStatementFailure then flips the code to 1. So a consumer reading the JSON sees the success shape (no top-level error key) while the shell sees exit 1 — the two signals now contradict each other. test/sql-batch-exit-code.test.ts:76 asserts only the exit code, so this is uncovered. The batch path at 713/726 doesn't have this problem because it writes raw renderOutput lines rather than going through success().
| .then(() => true, () => false) | ||
| const supervisor = await createSqlSupervisor({ | ||
| async onWarning(warning) { | ||
| // Never write into a terminal owned by the TUI renderer or persist credentials. | ||
| if (!diagnostics) return | ||
| await appendFile(logfile, JSON.stringify({ time: new Date().toISOString(), ...warning }) + "\n") | ||
| }, | ||
| }).catch(() => undefined) | ||
| // Keep non-SQL tools usable if the loopback socket is unavailable. | ||
| // SQL must still fail closed rather than silently run without supervision. | ||
| process.env[CLEANUP_ENV] = supervisor?.env[CLEANUP_ENV] ?? "unavailable" | ||
| try { | ||
| return await run() |
There was a problem hiding this comment.
MEDIUM — please confirm this is intended (confidence: high)
const supervisor = await createSqlSupervisor({ ... }).catch(() => undefined)
// Keep non-SQL tools usable if the loopback socket is unavailable.
// SQL must still fail closed rather than silently run without supervision.
process.env[CLEANUP_ENV] = supervisor?.env[CLEANUP_ENV] ?? "unavailable"This makes a working loopback TCP listener a hard prerequisite for all agent SQL. If createSqlSupervisor can't bind — restricted container network policy, a sandbox with no loopback, seccomp/AppArmor, port exhaustion — every SQL call through the agent fails closed at registerSqlCleanup with "query was not submitted", on a setup where SQL worked fine before this PR. The rejection reason is discarded by .catch(() => undefined), so there is nothing to diagnose from either (see my note on cleanup-scope.ts:197).
Failing closed is defensible for the correctness goal, but it converts a soft problem (an orphaned remote query) into total loss of SQL capability, and the decision is made silently. Confirming intent, plus surfacing the bind error, would make this much easier to live with.
Secondary, lower-stakes note on the same block: withSqlSupervisor wraps the whole agent runtime, so mkdir + appendFile + chmod + a TCP bind now run on every agent invocation, including cz-cli agent llm show, cz-cli agent stats and --help, none of which can issue SQL. Gating setup on the commands that can actually submit SQL would keep those paths at their current cost.
| if (!Number.isFinite(argv.timeout) || argv.timeout <= 0) { | ||
| error("USAGE_ERROR", "--timeout must be a positive, finite number of seconds (0 is not supported).", { format, exitCode: 2 }) | ||
| return | ||
| } |
There was a problem hiding this comment.
LOW (confidence: high) — --timeout 0 flips from "runs" to a usage error; worth a changelog line.
if (!Number.isFinite(argv.timeout) || argv.timeout <= 0) {
error("USAGE_ERROR", "--timeout must be a positive, finite number of seconds (0 is not supported).", { format, exitCode: 2 })
return
}Previously --timeout 0 was accepted. It did not mean "unlimited" — poll.ts:488 is jobTimeoutMs !== undefined && Date.now() - startTime > jobTimeoutMs, so 0 timed the job out on the first poll iteration — but it did submit the query and the command exited 1 with a JOB_TIMEOUT envelope. It now exits 2 with USAGE_ERROR and submits nothing.
Rejecting it is the right call (the old behavior was a trap), and the describe text now says so. Flagging only because the exit code and error code both change for an input that used to be accepted, which is the kind of thing a wrapper script notices. test/sql-timeout-options.test.ts covers the new rejection; nothing documents the old behavior as removed.
Also note the validation is reachable only for the sql command. execSql's own guard (exec.ts:138-140) throws a bare Error rather than going through error(), so a non-positive sdk.job.timeout arriving via --set surfaces as EXEC_ERROR/exit 1 rather than USAGE_ERROR/exit 2 — two codes for the same user mistake.
| await cancelJob(client, jobId).catch((error: unknown) => { | ||
| reason = | ||
| error instanceof ClickZettaApiError ? `Cancellation rejected (${error.code})` : "Cancellation request failed" | ||
| }) |
There was a problem hiding this comment.
LOW (confidence: high) — the deadline overwrites reason with a wrong attribution, and the diagnostic record is the only place it is ever read.
await cancelJob(client, jobId).catch((error: unknown) => {
reason =
error instanceof ClickZettaApiError ? `Cancellation rejected (${error.code})` : "Cancellation request failed"
})When the 5 s / 1.5 s budget expires mid-request, client.signal aborts, cancelJob rejects with the TimeoutError DOMException, and this handler records "Cancellation request failed" — indistinguishable from a genuine transport failure. The fallback reason initialised above ("not confirmed before the cleanup deadline") is the accurate one in that case but gets clobbered on the final iteration.
That string is what lands in ~/.clickzetta/sql-cleanup.jsonl via createSqlSupervisor's onWarning, which per specs/sql-cancellation.md is the sole record of an unconfirmed cleanup — so the one artifact meant for post-mortem diagnosis systematically mislabels timeouts as request failures. Skipping the assignment when client.signal.aborted would preserve the real cause.
| !response.respStatus?.errorCode && | ||
| !response.resp_status?.error_code && | ||
| !isRetryableErrorCode(status?.errorCode) && | ||
| ["SUCCEED", "FAILED", "CANCELLED"].includes(status?.state ?? "") |
There was a problem hiding this comment.
LOW (confidence: medium) — CANCELLING and SUCCEEDED never confirm, so the common cancel path burns the full budget and logs a false warning.
["SUCCEED", "FAILED", "CANCELLED"].includes(status?.state ?? "")Two states the SDK demonstrably sees are missing:
CANCELLING—poll.ts:206-208mapscase "CANCELLED": case "CANCELLING":toJobStatus.CANCELLED, i.e. the SDK already knows the coordinator reports this intermediate state. Since a successful cancel transitionsRUNNING → CANCELLING → CANCELLED, a job that is still cancelling at the deadline returnsconfirmed: falseand writes an unconfirmed-cleanup record tosql-cleanup.jsonl— even though cancellation is observably in progress. With the child budget at 1.5 s and a 250 ms poll interval, that will not be rare, and it makes the diagnostic log noisy enough to stop being useful.SUCCEEDED—poll.ts:197-200handlescase "SUCCEEDED": case "SUCCEED":. Here only the short spelling confirms.
The short-spelling-only convention does match poll.ts:10's TERMINAL_STATES, session.ts:497 and volume.ts:593, so this is consistent with existing code rather than newly wrong — but toJobStatus is the function that actually normalises wire states, and it accepts both. Reusing a shared helper (or at least TERMINAL_STATES) instead of a fourth inline copy of the list would keep these from drifting; adding CANCELLING as "confirmed enough for cleanup" is the part with a visible payoff.
| return processVolumeSql( | ||
| { clientOpts: ctx.clientOpts, workspace: ctx.config.workspace, instanceId: ctx.instanceId() }, | ||
| jobId, | ||
| result, | ||
| normalizedSql, | ||
| ) |
There was a problem hiding this comment.
LOW (confidence: high) — the volume-transfer path is the one piece of execSql left outside the lease.
return processVolumeSql(
{ clientOpts: ctx.clientOpts, workspace: ctx.config.workspace, instanceId: ctx.instanceId() },
jobId,
result,
normalizedSql,
)Everywhere else in this function now uses client (= { ...ctx.clientOpts, signal: lease.signal }); this call still passes the unsignalled ctx.clientOpts. So a PUT/GET file transfer ignores the query deadline and the SIGINT/SIGTERM abort, and disposition is already "terminal" by the time it runs — meaning finally won't cancel either. On Ctrl-C during a large volume upload, stop()'s 2 s hard process.exit will kill the transfer mid-flight rather than letting it unwind.
Whether a partially-completed transfer should be abortable is a judgement call, but as written it's an inconsistency rather than a decision — nothing in specs/sql-cancellation.md mentions volume SQL. If it's deliberate, a comment saying so would help; if not, passing client is the one-line fix.
| if (active.size === 0) { | ||
| process.on("SIGINT", interrupt) | ||
| process.on("SIGTERM", terminate) | ||
| } |
There was a problem hiding this comment.
LOW (confidence: high) — these handlers now cover every execSql caller, not just cz-cli sql, which changes Ctrl-C output and latency for a dozen other commands.
if (active.size === 0) {
process.on("SIGINT", interrupt)
process.on("SIGTERM", terminate)
}trackSqlJob runs inside execSql, and execSql has ~25 call sites outside the sql command — status.ts:21-22, table.ts (9 calls), schema.ts (5), workspace.ts, fs.ts:219, profile.ts:374,626-627, setup.ts:880, job.ts:425. All of them now get stop() on Ctrl-C instead of whatever ran before. Two observable consequences:
- Message change.
stop()emits"Execution interrupted by user."with ajob_idfield. For those commands, Ctrl-C previously producedmain.ts's"Operation aborted by user."(dev) or no envelope at all (shipped binary, which has no global handler — see my note onmain.ts:13). A script matching on the old text or on the absence ofjob_idsees something new. - Added exit latency.
stop()awaitslease.finish("cancel")→cancelJobAndWait(..., 1500)per job before exiting, bounded by the 2 ssetTimeout.cz-cli statusandschema.ts:67issuePromise.allof 2-3 concurrentexecSqlcalls, so Ctrl-C on a quick metadata lookup can now take up to ~2 s where it was instant.
Both are arguably improvements (the queries really are cancellable now), but neither is mentioned in specs/sql-cancellation.md, which frames the change as being about cz-cli sql. Nothing tests the non-sql commands' interrupt behavior.
Related, same block: once stop() has run, shutdown is set, so a second Ctrl-C is a no-op — main.ts's handler early-returns on hasActiveSqlJobs() (true forever once shutdown is set) and stop() returns at if (shutdown) return. The user has to wait out the 2 s timer. The old sql.ts handler exited immediately on every press.
| return new Promise<T>((resolve, reject) => { | ||
| const abort = () => reject(signal.reason) | ||
| if (signal.aborted) abort() | ||
| else signal.addEventListener("abort", abort, { once: true }) | ||
| work.then(resolve, reject).finally(() => signal.removeEventListener("abort", abort)) |
There was a problem hiding this comment.
LOW (confidence: high) — the abort listener is only removed when work settles, so a work that never settles leaks it.
return new Promise<T>((resolve, reject) => {
const abort = () => reject(signal.reason)
if (signal.aborted) abort()
else signal.addEventListener("abort", abort, { once: true })
work.then(resolve, reject).finally(() => signal.removeEventListener("abort", abort))
})Cleanup hangs off work, not off the race outcome. abortable exists precisely to escape promises that may never resolve — client.tokens.get() against a stalled portal is the motivating case, and test/fixtures/cancellation.ts:78 (get: () => new Promise(() => {})) builds exactly that. In that scenario the outer promise rejects on abort, the caller moves on, and the listener stays attached to signal for the signal's lifetime along with the retained work and its closure.
It's bounded in practice — the long-lived signals here are per-query (trackSqlJob's deadline) or per-cleanup (cancelJobAndWait's 5 s budget), so they are discarded shortly after. The one place it could accumulate is a long agent session where each supervised job leaks one listener on its own signal, which then goes away with the signal. So: real but low impact.
Attaching the removal to the race result instead of to work fixes it:
| return new Promise<T>((resolve, reject) => { | |
| const abort = () => reject(signal.reason) | |
| if (signal.aborted) abort() | |
| else signal.addEventListener("abort", abort, { once: true }) | |
| work.then(resolve, reject).finally(() => signal.removeEventListener("abort", abort)) | |
| return new Promise<T>((resolve, reject) => { | |
| const abort = () => reject(signal.reason) | |
| const done = () => signal.removeEventListener("abort", abort) | |
| if (signal.aborted) abort() | |
| else signal.addEventListener("abort", abort, { once: true }) | |
| work.then( | |
| (value) => { | |
| done() | |
| resolve(value) | |
| }, | |
| (error) => { | |
| done() | |
| reject(error) | |
| }, | |
| ) | |
| }) |
|
Review summary A. Upstream invasiveness — no issues found No files under B. Clean fix, or a hole drilled around the problem? Mostly the clean fix. Killing a shell subprocess genuinely cannot cancel a submitted remote job, no upstream hook exposes the shell's remaining budget, and the chosen shape — ownership established before submission, released only on confirmed terminal state or acknowledged handoff — addresses the cause rather than the symptom. Threading Three things in this category:
C. Regression risk I could not run anything, so nothing below is claimed to pass. Behavioral changes I found, with coverage:
No tests were deleted, skipped, or had assertions loosened — the diff is +1883/-151 with four new test files and no removals from existing suites. Nothing under any Two items I raised as questions rather than bugs, because they look deliberate but have large blast radius: the D. Correctness No critical defects found. I traced the abort plumbing for unhandled rejections and listener lifetimes: Lower-severity items are inline: the misleading supervisor error message ( 13 inline comments total. All are suggestions — take or leave as you see fit. 🤖 Generated with Claude Code |
# Conflicts: # packages/cz-cli/src/commands/sql.ts
| !response.respStatus?.errorCode && | ||
| !response.resp_status?.error_code && | ||
| !isRetryableErrorCode(status?.errorCode) && | ||
| ["SUCCEED", "FAILED", "CANCELLED"].includes(status?.state ?? "") |
There was a problem hiding this comment.
MEDIUM — terminal-state set is narrower than the states this repo already knows the gateway sends (confidence: medium-high)
["SUCCEED", "FAILED", "CANCELLED"].includes(status?.state ?? "")This is the only place confirmation can come from, and it omits SUCCEEDED plus the lowercase forms. Two places in-tree say those reach us:
packages/clickzetta-sdk/src/sql/poll.ts:198-200maps both"SUCCEEDED"and"SUCCEED"toJobStatus.SUCCEEDED.packages/cz-cli/src/commands/job-profile.ts:179treats["SUCCEED", "SUCCEEDED", "FAILED", "CANCELLED", "succeed", "failed", "cancelled"]as terminal.
Failure scenario: a job finishes and the deployment reports state: "SUCCEEDED". cancelJobAndWait never matches, so the while (!client.signal.aborted) loop keeps issuing cancelJob + getJobResultRaw every 250 ms for the entire budget — ~12 round trips at the 1.5 s child budget, ~40 at the 5 s supervisor budget — and then returns { confirmed: false }. Downstream that becomes a false SQL job <id>: cancellation unconfirmed; parent cleanup or server timeout must recover it. on stderr (sql-lifecycle.ts:34-36) or a bogus record in ~/.clickzetta/sql-cleanup.jsonl, for a job that completed normally.
Smaller correct change: reuse one terminal-state predicate rather than a third inline literal — poll.ts already has TERMINAL_STATES (line 10, same narrow set) and job-profile.ts:179 has the wide one. Exporting the wide predicate from the SDK and calling it here (and ideally from poll.ts and exec.ts:178) removes the drift instead of adding to it.
| /** Name the abort cause; classifyExecError maps "timed out" to JOB_TIMEOUT and other codes verbatim. */ | ||
| function abortFailure(jobId: string, reason: unknown, cause: unknown) { | ||
| if (reason instanceof DOMException && reason.name === "TimeoutError") | ||
| return Object.assign(new Error(`Job ${jobId} timed out`, { cause }), { jobId }) | ||
| const message = reason instanceof Error ? reason.message : "SQL execution interrupted" | ||
| const code = (reason as { code?: unknown })?.code | ||
| return Object.assign(new Error(`Job ${jobId}: ${message}`, { cause }), { | ||
| jobId, | ||
| code: typeof code === "string" ? code : "ABORTED", | ||
| }) |
There was a problem hiding this comment.
MEDIUM — the SQL_SUPERVISOR_LOST code this function is careful to preserve is thrown away by classifyExecError (confidence: high)
/** Name the abort cause; classifyExecError maps "timed out" to JOB_TIMEOUT and other codes verbatim. */"other codes verbatim" does not hold for the lost-supervisor case. supervisorLost() (sql-lifecycle.ts:9-10) carries the message SQL cleanup supervisor connection lost, so this builds Job <id>: SQL cleanup supervisor connection lost with code: "SQL_SUPERVISOR_LOST". But classifyExecError checks isNetworkError(err) (line 273) before the code ?? "EXEC_ERROR" fallthrough (line 282), and isNetworkError matches on msg.includes("connection") (line 233).
Failure scenario: the agent's event loop stalls or the supervisor socket dies mid-query. The user/agent sees:
CONNECTION_ERROR: Job <id>: SQL cleanup supervisor connection lost
Cannot connect to ClickZetta. Check network connectivity and verify the instance/service URL in the profile.
The aiMessage sends the agent to debug the ClickZetta endpoint and the profile, when the actual fault is a local loopback socket. specs/sql-cancellation.md claims "a lost supervisor is SQL_SUPERVISOR_LOST" — that string never reaches output.
Smallest correct fix is in classifyExecError: check the explicit errorCode(err) before the heuristic isNetworkError fallback, so an error that names its own code wins over substring matching. Rewording the message to avoid "connection" would work too but leaves the same trap for the next explicit code.
| const timeoutMs = explicitTimeoutMs ?? (process.env[CLEANUP_ENV] ? 300_000 : undefined) | ||
| const lease = await trackSqlJob(ctx.clientOpts, jobId, timeoutMs) | ||
| const client = { ...ctx.clientOpts, signal: lease.signal } | ||
| let disposition: "terminal" | "detached" | "cancel" = "cancel" |
There was a problem hiding this comment.
MEDIUM — "cancel" as the default disposition makes every non-abort failure in every execSql caller pay a cancellation round trip (confidence: medium-high)
let disposition: "terminal" | "detached" | "cancel" = "cancel"disposition is only narrowed in two places: the submit-4xx .catch below (line 163-169) and disposition = "terminal" after the poll returns (line 189). Everything else that throws between those two points lands on "cancel", which in the finally runs cancelJobAndWait(opts, job, 1500) — up to 1.5 s and ~12 HTTP round trips (see the cancel.ts comment).
The case I'd single out: pollJobResult throws on a fatal server error code (poll.ts, isFatalErrorCode branch). That is a job that has already reached a terminal state server-side, and we now cancel it anyway. For cz-cli sql --batch over a file where many statements fail, that is 1.5 s plus two requests added per failing statement.
The blast radius is wider than the sql command. execSql is called from table.ts, schema.ts, fs.ts, profile.ts, setup.ts, status.ts, workspace.ts and job.ts — all of them now take this path on any thrown failure, and all of them can emit SQL job <id>: cancellation unconfirmed… on stderr, which none of them emitted before.
Smaller correct change: when the error carries a terminal job state (the isFatalErrorCode throw from poll.ts, same as the 4xx narrowing you already added for submit), set disposition = "terminal". That keeps "cancel" for the genuinely ambiguous cases — network drop, abort, unknown error — which is what it is for.
| hints: buildExecHints(opts?.hints, traceContext), | ||
| asynchronous: opts?.asynchronous, | ||
| // A detached job outlives its supervisor by design: only an explicit timeout reaches the server. | ||
| jobTimeoutMs: opts?.asynchronous ? explicitTimeoutMs : timeoutMs, |
There was a problem hiding this comment.
MEDIUM — please confirm intent: sync cz-cli sql now imposes a hard 300 s server-side job timeout where it previously imposed none (confidence: high on the mechanism, asking about intent)
jobTimeoutMs: opts?.asynchronous ? explicitTimeoutMs : timeoutMs,Before this PR execSql passed no jobTimeoutMs to submitJob at all, so jobDesc.jobTimeoutMs was never emitted (submit.ts:181) and the deployment default governed the job. The 300 s from --timeout was purely client-side: sql.ts passes timeoutMs: argv.timeout * 1000 into pollJobResult's jobTimeoutMs, which bounded how long we waited.
Now, for the default sync path, timeoutMs is 300 000 and reaches the server. Behavioral consequence: a long INSERT OVERWRITE … SELECT or heavy DDL that used to keep running server-side after the CLI gave up is now killed by the coordinator at 300 s. For anyone who relied on "fire it with the default, the job finishes even if my shell times out", that is a silent change from client gave up to job was killed.
Two related points:
- The same applies to every supervised
execSqlwith no explicit timeout —timeoutMs = explicitTimeoutMs ?? (process.env[CLEANUP_ENV] ? 300_000 : undefined)(line 142). So under the agent,cz-cli table list/schema list/fscalls now carry a 300 s server cap they did not before.specs/sql-cancellation.mddocuments the fallback but not that it is sent to the server. - Test coverage:
sql-timeout-options.test.tsasserts[12000]for an explicit--timeout 12,[undefined]for--async, andundefinedfortable list/schema list. Nothing pins the sync default, which is the case that changed. Asql --synccase assertingtimeoutsequals[300000]would at least make the new contract explicit and catch a future flip.
If this is deliberate (it reads like it is — it is what makes the "both processes died" fallback finite), a line in the spec's timeout section saying the default now reaches the server, and the test above, would be enough.
| process.on("SIGINT", () => { | ||
| // Active SQL owns graceful cancellation before exiting. | ||
| if (hasActiveSqlJobs()) return |
There was a problem hiding this comment.
MEDIUM — the fallback this guard defers to does not exist in the shipped binary (confidence: high)
process.on("SIGINT", () => {
// Active SQL owns graceful cancellation before exiting.
if (hasActiveSqlJobs()) returnThis file is the dev entry (bun run src/main.ts, per its own header comment). The compiled binary enters at src/bootstrap/boot.ts, which registers no signal handlers at all — rg -n "SIGINT|SIGTERM" packages/cz-cli/src returns only this file, sql-lifecycle.ts, and opencode-plugin/otel/index.ts (loaded in the agent runtime, not the standalone CLI).
Before this PR, sql.ts's handler() installed its own SIGINT handler as its first action, so the whole command was covered. That handler is now gone and sql-lifecycle.ts only installs one while active.size > 0 — i.e. from trackSqlJob to lease.finish.
Failure scenario, shipped binary: cz-cli sql "select 1" --format json, Ctrl-C while getExecContext is still running (OAuth/portal round trips, cookie resolution, connectionContext — easily seconds on a cold profile). No handler is registered, so the default SIGINT disposition terminates the process with no {"error":{"code":"ABORTED",…}} envelope on stdout. Same for Ctrl-C during result rendering/masking after lease.finish has run. Previously both printed the structured envelope. A script parsing --format json output now gets empty stdout instead of a parseable error.
No test covers this: sql-cancellation.test.ts signals the child only after a submit/poll has started (it waits on ready.promise), which is exactly the window that is still covered.
The smaller fix is to keep a process-wide fallback handler on the real entry path — either move this process.on("SIGINT", …) into boot.ts/run-cli.ts where both entries reach it, or restore a command-scoped handler in sql.ts that defers to hasActiveSqlJobs() the same way this one does.
| timer = setInterval(() => { | ||
| if (performance.now() - lastSeen >= heartbeatTimeoutMs) return lost() | ||
| if (!socket.destroyed) socket.write('{"type":"heartbeat"}\n') | ||
| }, heartbeatMs) |
There was a problem hiding this comment.
MEDIUM — a 10 s event-loop stall in either process kills a healthy job, and the two directions fail differently (confidence: medium)
timer = setInterval(() => {
if (performance.now() - lastSeen >= heartbeatTimeoutMs) return lost()
if (!socket.destroyed) socket.write('{"type":"heartbeat"}\n')
}, heartbeatMs)The lease is symmetric in code but not in consequence:
- Supervisor stalls ≥10 s. The supervisor lives in the agent's main process (
withSqlSupervisorwrapsrunRuntime(args, true)inbootstrap/runtime.ts:57-61) — the same event loop as the TUI renderer and the opencode host. A long synchronous render, a big GC pause, or a blocking bundle step means noheartbeatreplies. The child's interval keeps firing, crossesheartbeatTimeoutMs, callslost()→onLost()→controller.abort(supervisorLost()), and a perfectly healthy long-running query is aborted. TCP write buffering meanssocket.writegives no warning. - Child stalls ≥10 s.
coerceValueover millions of rows,decodeArrowPayload, orfetchTextFromUrlson a large result set can block the child's single thread well past 10 s. The supervisor's sweep (line 144-152) then callscleanup(connection)→cancelJobAndWaitagainst a job that is already finished, and destroys the socket. The child, on resuming, findsrelease()failing and printsSQL job <id>: cleanup acknowledgement failed.to stderr even though it returned correct results.
specs/sql-cancellation.md says the lease exists for "frozen processes", so some version of this is intended. What I'd ask you to confirm is whether 10 s is the right headroom given that one side of the lease is a TUI render loop, and whether the child should treat a missed heartbeat as fatal (onLost() aborts the query) rather than, say, requiring two consecutive misses or widening only the child's tolerance. The supervisor-stall direction is the one that converts someone else's latency spike into a failed user query.
No test covers a stall: healthy heartbeats keep a long-running query alive (sql-cleanup-scope.test.ts) exercises the opposite, and the two ${expiry} expiry cases drive expiry by holding a socket open with no heartbeats at all, never by blocking a real event loop.
| test("SQL cancellation against real HTTP boundaries", async () => { | ||
| // Other SDK suites replace global fetch. Keep real socket/abort semantics | ||
| // isolated rather than replacing fetch again or depending on test order. | ||
| const child = Bun.spawn([process.execPath, "test", "./test/fixtures/cancellation.ts"], { | ||
| cwd: new URL("..", import.meta.url).pathname, | ||
| stdout: "pipe", |
There was a problem hiding this comment.
MEDIUM — this whole suite never runs in CI (confidence: high)
packages/clickzetta-sdk/package.json has no test script (only typecheck), and no workflow invokes one. .github/workflows/cz-test.yml runs:
bun run typecheckandbun run testinpackages/cz-clinode --testinpackages/npm/cz-cli- two named upstream test files in
packages/core/packages/opencode
baseline-test.yml only builds and runs scripts/check-baseline.sh. So cancel.test.ts and the 10 cases in test/fixtures/cancellation.ts — the only coverage for abortable/abortAfter/delay, the cancelJob response-validation branches, the proto3-empty-status acceptance, and "abort prevents business-level submit retries" — are never executed on a PR. packages/clickzetta-sdk is not typechecked by CI either, so the SDK half of this change has no gate at all.
That matters more than usual here because the SDK changes are the load-bearing part: client.ts's retry loop now threads opts.signal and opts.maxRetries through four decision points, and nothing in the gated packages/cz-cli suite reaches cancelJob's validation branches directly.
Two lines in cz-test.yml would close it, matching the pattern already used for the upstream files:
- name: Run clickzetta-sdk tests
working-directory: packages/clickzetta-sdk
run: bun testplus a bun run typecheck step for the same directory. Note bun test from packages/clickzetta-sdk will also pick up the existing suites there, which may need checking first.
Minor, same file: cwd: new URL("..", import.meta.url).pathname yields /C:/… on Windows. Test-only, and irrelevant while CI is ubuntu-latest, but it would break anyone running the suite on the Windows host that release-cos.yml already uses for the win32 build.
| if (raw === "unavailable") | ||
| throw new Error( | ||
| "SQL cleanup supervisor unavailable; query was not submitted. Check local socket and ~/.clickzetta write permissions.", |
There was a problem hiding this comment.
LOW — error message is stale after the diagnostics change in commit 4 (confidence: high)
"SQL cleanup supervisor unavailable; query was not submitted. Check local socket and ~/.clickzetta write permissions.",supervisor-runtime.ts no longer withdraws supervision when diagnostics are unwritable — diagnostics degrades to false and onWarning silently skips the append. The only remaining route to "unavailable" is createSqlSupervisor() itself rejecting, i.e. the loopback bind failing. ~/.clickzetta write permissions can no longer cause this, so the message sends the reader to check the one thing that is now irrelevant.
Dropping the second half (Check the local loopback socket.) matches what the code actually does. The accompanying test was renamed to unwritable diagnostics keep supervision and SQL admission and now asserts expect(stderr).not.toContain("unavailable"), which confirms the two have diverged.
| } | ||
| } else { | ||
| await executeSingle(ctx, stmt, argv, accumulatedHints, configStatements, (id) => { currentJobId = id }) | ||
| await executeSingle(ctx, stmt, argv, accumulatedHints, configStatements, undefined) |
There was a problem hiding this comment.
LOW — two leftovers from removing the command-scoped SIGINT handler (confidence: high)
await executeSingle(ctx, stmt, argv, accumulatedHints, configStatements, undefined)-
onJobIdis now dead plumbing. Both call sites passundefined(this line and line 746), andexecuteSingleis module-local, so theonJobId?: (id: string) => voidparameter at line 343 — and everywhere it is threaded onward (lines 386, 407, 410, 455, 476) andexecSql's ownopts.onJobIdcall atexec.ts:134— can never fire from this command. The removedsigintHandler'scurrentJobIdwas its only consumer; job IDs now come fromactive's keys insql-lifecycle.ts. PerAGENTS.md("Do not extract single-use helpers preemptively… inline the logic at the call site") the parameter and the sixonJobIdpass-throughs would be better deleted than left wired to nothing. -
renderErrorOutputis now an unused import at line 6.rg -n renderErrorOutput packages/cz-cli/src/commands/sql.tsmatches only the import.parseOutputArgsfrom the same line is still live (line 707), so onlyrenderErrorOutputcomes out.tsconfigsets nonoUnusedLocals, sobun typecheckwill not flag it.
If onJobId is being kept deliberately for a future caller, ignore (1) — but worth saying so, since nothing in-tree reaches it today.
| socket.write( | ||
| JSON.stringify({ | ||
| type: "register", | ||
| secret: address.secret, | ||
| job, | ||
| baseUrl: client.baseUrl, | ||
| credential, | ||
| customHeaders: client.customHeaders, | ||
| timeoutMs, | ||
| }) + "\n", | ||
| ) |
There was a problem hiding this comment.
LOW — the registration frame puts a live ClickZetta token on an unauthenticated loopback socket, and the capability that gates it reaches every agent-spawned process (confidence: medium; flagging because this repo's review guidance calls out credential-handling paths specifically)
socket.write(
JSON.stringify({
type: "register",
secret: address.secret,
job,
baseUrl: client.baseUrl,
credential,Three observations, none of which is a privilege escalation on a single-user machine, but which are worth stating since the design is new:
-
CZ_SQL_CLEANUPis inherited by every child of the agent process, which is the point forcz-cli sql, but also means the endpoint andsecretland in the environment of every arbitrary command the agent's bash tool runs. An LLM-drivenenv,printenv, or a shell wrapper that echoes its environment into a log or a network call discloses the capability. Holding it lets a local process register its own job (it must supply its own credential), so the direct consequence is bounded — but the supervisor will then make outbound HTTP to anyhttp(s)baseUrlthe registrant names, sinceregistration.baseUrlis validated only for scheme (lines 23-26). -
Direction matters for a hostile env var.
registerSqlCleanupvalidates127.0.0.1/tcp:/ a UUID secret, which stops a remote redirect, but aCZ_SQL_CLEANUPpointing at any local port makes a standalonecz-cli sqlhand its token to whatever is listening there. That is exactly whatsql-cleanup-scope.test.ts's "lost async acknowledgement" case does with a hand-rolled server. Since the var normally comes from the parent, this is only reachable if it leaks into a persisted shell environment — but nothing in the protocol would detect it. -
The token crosses the socket in plaintext. Loopback sniffing needs root/
CAP_NET_RAW, so this is the least of the three.
What would help without redesigning anything: have the supervisor pin baseUrl to the set it was started with rather than accepting it per-registration, and note in specs/sql-cancellation.md that the capability is visible to every descendant process. The spec currently says "Credentials remain in memory and never enter the command line, disk or diagnostic output" — accurate (the onWarning payload is {jobId, reason} only, and the test asserts no private-token), but it does not mention that the secret is broadcast through the inherited environment.
| } finally { | ||
| await flushOtel() | ||
| await flushLangfuse() | ||
| process.exit() | ||
| } | ||
| return (process.exitCode as number) ?? 0 |
There was a problem hiding this comment.
LOW — please confirm intent: dropping process.exit() makes agent/TUI shutdown wait on supervisor cleanup (confidence: high on the mechanism)
} finally {
await flushOtel()
await flushLangfuse()
}
return (process.exitCode as number) ?? 0
}The exit code itself is preserved — process.exit() with no argument used process.exitCode, and both callers now do process.exit(await main(...)) (boot.ts:16, run-cli.ts:372-373). No concern there.
What changes is that the agent runtime no longer terminates at this point. Control returns to withSqlSupervisor's finally, which awaits supervisor.close() → Promise.allSettled([...pending]), where each pending entry is a cancelJobAndWait(entry.client, entry.job, 5000). So quitting the TUI with orphaned registrations outstanding now blocks for up to 5 seconds (concurrent, so ~5 s total rather than per job) before the process exits — and during that window the TUI's Bun Worker and opencode host are still alive and can still write to the terminal.
This looks deliberate (specs/sql-cancellation.md: "Normal bootstrap return closes the supervisor and attempts cancellation of all outstanding registrations"), and the budget is bounded because cancelJobAndWait's loop is driven by an abortAfter signal. Two things I could not rule out:
server.close(cb)only fires its callback once every connection is released.close()destroys all tracked sockets first andclosingrejects new ones, so this should settle — but if it ever does not, the process hangs on exit with no timeout anywhere in the chain. A bound onsupervisor.close()would make that unhangable.- The
cancel.tsstate-matching issue I flagged separately makesconfirmed: falsemore likely than intended, which is what pushes this toward the full 5 s rather than one round trip.
Worth a sentence in the spec's budget table (it lists "Supervisor cleanup 5 seconds per job, concurrently" but not that normal exit waits on it).
| await cancelJob(client, jobId).catch((error: unknown) => { | ||
| reason = | ||
| error instanceof ClickZettaApiError ? `Cancellation rejected (${error.code})` : "Cancellation request failed" | ||
| }) | ||
| const raw = await getJobResultRaw(client, jobId).catch(() => undefined) | ||
| if (raw && typeof raw === "object" && "status" in raw) { | ||
| const response = raw as { | ||
| status?: { state?: string; errorCode?: string } | ||
| respStatus?: { errorCode?: string } | ||
| resp_status?: { error_code?: string } | ||
| } | ||
| const status = response.status | ||
| if ( | ||
| status?.state && | ||
| !response.respStatus?.errorCode && | ||
| !response.resp_status?.error_code && | ||
| !isRetryableErrorCode(status?.errorCode) && | ||
| ["SUCCEED", "FAILED", "CANCELLED"].includes(status?.state ?? "") | ||
| ) { | ||
| return { confirmed: true, state: status.state } | ||
| } | ||
| } | ||
| await delay(250, client.signal).catch(() => {}) |
There was a problem hiding this comment.
LOW — unconditional re-cancellation every 250 ms makes the request volume scale with the number of orphans (confidence: medium)
await cancelJob(client, jobId).catch((error: unknown) => { ... })
const raw = await getJobResultRaw(client, jobId).catch(() => undefined)
...
await delay(250, client.signal).catch(() => {})Each iteration issues two requests, so one unconfirmed job costs ~12 requests at the child's 1.5 s budget and ~40 at the supervisor's 5 s default. createSqlSupervisor admits up to 256 connections (cleanup-scope.ts:85) and close() fans out cleanup across all of them concurrently, so a worst-case shutdown is on the order of 10⁴ requests to the gateway inside 5 seconds — from a client that is trying to exit.
The comment justifies the repeat as covering "a submit that becomes visible after the first cancel", which is a real race, but it only needs to be covered for as long as submission could still be in flight — not for the whole budget at full rate. Cancelling once, polling, and re-cancelling only after the state is still non-terminal for a couple of polls would keep the property and cut the volume several-fold. Backing off the 250 ms interval would help too.
This compounds with the SUCCEEDED state-matching gap I flagged on line 68: that bug is what turns a job that finished cleanly into a full-budget retry loop rather than a single confirmed round trip.
Review summaryReviewed against Note on scope: the first two commits in the PR's commit list ( A. Upstream invasiveness — no issues foundNothing in this PR touches Worth recording as a positive: the design deliberately reaches upstream through hooks it already exposes rather than patching it. The B. Clean fix vs. hole drilled around the problem — mostly the clean fix; three exceptionsThe core approach is right, and I want to say so plainly: "killing a shell does not cancel the SQL it submitted" is genuinely not expressible through an upstream hook, the signal is threaded through the real transport boundaries ( Three places where the change treats a symptom:
C. Regression riskBehavior that already worked and could change:
No tests were deleted, skipped, or loosened. The new suites use real sockets, subprocesses and HTTP servers rather than mocks, which matches D. General correctnessChecked and found clean: Remaining D-category items are the inline findings: the |
2 participants
No description provided.