fix(review): make cancellation shutdown atomic and time-bounded

[lizhengfeng101]

Description

ocr review handles SIGINT — and SIGTERM once #1111 lands — by cancelling the context so the run closes through its normal defer chain. The core of that path is solid: finalizeManifest() and session.Finalize() run inside ag.Run (internal/agent/agent.go:390-396), before any cleanup that could stall, so --resume stays usable no matter what happens later in the shutdown.

Three narrower gaps remain in the shutdown tail. All three predate #1111 — that PR only widens the set of scenarios that reach them, from an interactive Ctrl-C to docker stop, CI cancellation and editor cancel buttons.

1. The --output report is not written atomically

lazyFileWriter opens the target with os.Create on the first write (cmd/opencodereview/shared.go:504), truncating in place. The type exists precisely so a run that produces no output leaves an existing file untouched, so the effective guarantee for a target path today is "complete report, or previous content". That guarantee disappears the moment the first byte lands: a write failure, or a SIGKILL arriving inside the emit window, leaves a truncated JSON or SARIF document at a path other tools read. A truncated SARIF is the worst case, since consumers treat one upload as the complete alert set.

Writing to a temp file in the same directory and renaming on Close restores the guarantee for every failure mode.

This also settles a user-visible question that is currently decided by the open mode rather than on purpose: a cancelled run's partial-coverage report replaces a previous complete report at the same path. That may well be the desired behavior — it should just be a decision.

2. Shutdown can take ~15s per stdio MCP server, sequentially

review_cmd.go closes MCP clients in a defer loop, one at a time. Each Close reaches the SDK's pipeRWC.Close (modelcontextprotocol/go-sdk@v1.7.0/mcp/cmd.go:69), which is bounded but generous: close stdin, wait TerminateDuration, send SIGTERM, wait again, send SIGKILL, wait again. internal/mcp/client.go:41 constructs CommandTransport without setting TerminateDuration, so the SDK default of 5s applies at each of the three stages — up to ~15s for one unresponsive server, multiplied by the number of configured stdio servers.

That overruns docker stop's 10s grace period and the VS Code extension's 3s SIGKILL escalation, so the process is hard-killed mid-cleanup. Nothing critical is lost when that happens — the manifest, session_end and the report are already on disk — but the MCP subprocesses are then orphaned, which is exactly the outcome the graceful path exists to prevent.

Options: set a short TerminateDuration, close clients concurrently, and/or put one overall deadline on the close loop.

3. A second Ctrl-C cannot interrupt a slow shutdown

signal.NotifyContext keeps the signal registered until stop() runs, so every signal after the first is delivered to a channel nobody reads and dropped — the default kill behavior stays suppressed for the whole shutdown. While gap 2 plays out the user has no way to abandon it. Owning the channel instead, cancelling on the first signal and exiting on the second, is the usual shape.

Scope

• cmd/opencodereview/shared.go — lazyFileWriter: os.Create to temp file + rename on close
• internal/mcp/client.go, cmd/opencodereview/review_cmd.go — bound the MCP close phase
• cmd/opencodereview/review_cmd.go (and scan_cmd.go once fix(scan): propagate Ctrl-C cancellation to scan runs #996 lands) — signal registration

The three items are independent and are easier to review as separate PRs.

Acceptance Criteria

• --output writes land in a temp file in the target directory and are renamed into place on close; an interrupted or failed write leaves the previous file content intact
• Total MCP shutdown time stays well inside 3s regardless of how many stdio servers are configured or how unresponsive they are
• A second SIGINT/SIGTERM during shutdown terminates the process instead of being absorbed
• Tests cover rename-on-close semantics, an unresponsive stdio MCP server, and second-signal exit
• Tests pass (make test)
• Code check passes (make check)

Context

Found while reviewing #1111, which extends graceful shutdown to SIGTERM and thereby makes this tail reachable from docker stop, CI job cancellation and editor cancel buttons rather than only from an interactive Ctrl-C. Related: #902 introduced the SIGINT-only NotifyContext; #996 adds the same handling to scan.

Deliberately out of scope, because it needs a maintainer decision before any implementation: a cancelled run exits 1, indistinguishable by exit code from a config error or a provider failure, because the process no longer dies by a signal (launcherExitCode in bin/ocr.js:21 only produces 128+N for signal death). The run manifest does record run_failure.classification = "cancelled", so machine-readable consumers can still tell the difference, and SIGINT has always exited 1 — so this may be working as intended. Restoring 130/143 by re-raising the signal after cleanup would be breaking for action.yml, the launcher and the VS Code extension, and deserves its own discussion.

[lizhengfeng101]

Implementation notes for item 1, mostly the parts that are easy to miss until review.

The truncation happens at the start of writing, not the end. os.Create sits inside the first Write (shared.go:502-513) and Close only closes the handle (shared.go:557-563), so the target path holds a partial document for the entire duration of the emit. Anything that kills the process in that window leaves it there for good. A mid-write failure does the same thing: writeErr is recorded and the command exits non-zero, but the file on disk stays half-written.

Two failure modes look similar and are not:

• A half-written JSON or SARIF file fails to parse, so consumers at least error out.
• A half-written text report is still valid text. It just stops early, possibly before the summary, and nothing complains.

Rough shape of the fix:

w.once.Do(func() {
    f, err := os.CreateTemp(filepath.Dir(w.path), ".ocr-out-*") // same dir, see (1)
    ...
})

func (w *lazyFileWriter) Close() error {
    if w.tmp == nil {
        return nil // never produced output; leave the target alone (current behavior)
    }
    name := w.tmp.Name()
    if w.writeErr != nil {
        w.tmp.Close()
        os.Remove(name)
        return nil // the write error is already reported through Err()
    }
    if err := w.tmp.Sync(); err != nil { ... }
    if err := w.tmp.Close(); err != nil { os.Remove(name); return err }
    return os.Rename(name, w.path)
}

Gotchas:

1. The temp file has to live in the target's directory. os.TempDir() is often a different filesystem and os.Rename then fails with EXDEV.
2. os.CreateTemp creates the file 0600; os.Create creates it 0666 &^ umask. Renaming without a chmod silently downgrades report permissions and breaks anything that reads the file as a different user, CI artifact collection being the obvious one. Stat the existing target first and reuse its mode when there is one.
3. os.Create follows a symlink and writes through to its destination. Rename replaces the symlink itself with a regular file. Either resolve with filepath.EvalSymlinks first, or say outright that a symlinked --output is unsupported.
4. Rename swaps the inode, so a reader holding an older fd keeps seeing the old content and hard links to the target break. That is usually the point (readers get a consistent snapshot), but it belongs in a comment.
5. The Results written to hint prints after the first successful Write today (shared.go:527-530). The comment there explains the intent as never handing an agent a path hint for a file that was not persisted, which is exactly what the current placement fails at: print the hint, die, and the agent reads a truncated file. Move it after a successful rename.
6. A SIGKILL leaves the temp file behind. A dot-prefixed fixed prefix keeps it identifiable, and a stray .ocr-out-xxxx is a much better thing to leave behind than a plausible-looking truncated report.
7. os.Rename on Windows goes through MoveFileEx with MOVEFILE_REPLACE_EXISTING, so it does replace an existing file, but it fails when another process holds the target open without sharing. Worth a test.
8. stripAnsiWriter carries an unfinished escape sequence across Write calls (shared.go:383) and Close does not flush it. Only escape-sequence fragments are dropped, so this is minor, but confirm it does not change under the new underlying writer.
9. Rename is atomic with respect to path lookup, not to durability. Sync the temp file before renaming; a report is small enough that the fsync does not matter.

Tests worth having:

• An existing complete file at the target plus an injected write failure partway through: assert the old bytes are unchanged.
• No writes at all (preview error, every file failed): assert the target is neither created nor truncated.
• Happy path: complete content, expected mode, no leftover temp file in the directory.
• Windows: rename over an existing file.

One thing this does not fix, and I would not bundle it in: a cancelled run still atomically replaces a previous complete report with a partial-coverage one. The file is syntactically fine and run_failure.classification says cancelled, but a consumer that ignores that field reads partial results as the whole review. That is the policy question in the issue body, and it wants a decision before code.

[dvd233]

Hi, I'd like to take item 1 only: make lazyFileWriter commit --output atomically.

I'll follow the implementation notes here: create the temp file in the target directory, preserve the existing/default output permissions, sync/close/rename only after successful writes, clean up the temp file on failures, move the Results written to hint until after the rename succeeds, and add focused tests for old-content preservation, the no-write case, successful replacement/mode/temp cleanup, and Windows replacement where portable. I will inspect and explicitly cover the current symlink behavior rather than changing it accidentally.

I found no open PR mentioning #1141 or lazyFileWriter. I will not touch MCP shutdown, signal handling, or the separate policy question about whether a cancelled partial report should replace an earlier complete report.

I am using Codex for automated assistance and will review and take responsibility for the resulting change.

[HOWILLMAKEIT]

Hi, I'm the author of #1111 (the SIGTERM graceful-shutdown PR that this issue builds on). I'm interested in taking items 2 and 3 — I'd like to submit them as two separate PRs, as suggested.

For item 2 (bounded MCP shutdown): my plan is to set an explicit short TerminateDuration on the CommandTransport construction in internal/mcp/client.go (the SDK's 5s×3-stage default is what makes a single unresponsive stdio server cost ~15s), close the clients concurrently instead of sequentially in review_cmd.go, and put an overall deadline on the close phase so the total stays well inside the 3s budget regardless of how many servers are configured.

For item 3 (second-signal exit): replace signal.NotifyContext in review_cmd.go with an owned channel — first signal cancels the context and starts the normal cleanup path, a second signal exits immediately. I'll mirror the same registration in scan_cmd.go after #996 lands so the two commands stay consistent.

Both changes are confined to the shutdown tail; nothing in the review path is touched. Tests will cover: an unresponsive stdio MCP server closing within the bound, second-signal exit, and the normal path remaining unaffected.I will run make test and make check before submitting each PR.