Skip to content

fix(scan): propagate Ctrl-C cancellation to scan runs - #996

Open
AllenMuu wants to merge 2 commits into
alibaba:mainfrom
AllenMuu:feat/scan-cancellation-resume
Open

fix(scan): propagate Ctrl-C cancellation to scan runs#996
AllenMuu wants to merge 2 commits into
alibaba:mainfrom
AllenMuu:feat/scan-cancellation-resume

Conversation

@AllenMuu

@AllenMuu AllenMuu commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Description

Propagates the ocr scan command context from the Cobra entrypoint to preview, telemetry, scan.Agent.Run, and result emission. Ctrl-C can therefore reach the scan runtime's existing cancellation and checkpoint/resume handling.

This also fixes the scan trace ID to read from the context returned by the scan.run span, and removes obsolete non-context wrappers after updating their test call sites.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing

Deterministic regression coverage verifies:

  • a pre-cancelled scan context stops before setup;
  • a completed scan item is checkpointed, cancellation occurs, and resume only calls the LLM for unfinished items;
  • the emitted trace ID belongs to the scan.run span.

Also ran make check, including formatting, license, and go vet checks.

Live LLM verification with Volcengine Ark (glm-5.2):

  • ocr llm test passed.
  • A default scan request using the template's max_completion_tokens=58888 reached /chat/completions but received HTTP 400 InvalidParameter.
  • A small-file scan with --max-tokens 4096 reached the model and completed OpenAI function-tool calls, so the basic scan request/tool format is accepted at that limit. The model did not call OCR's required task_done tool, so that file did not become reusable.
  • A live SIGINT during an in-flight request produced context canceled; the session persisted both review_item_failed and session_end (session 882e75fb-d251-49c0-9e14-2f19d2ba7f75).
  • A live --resume run with a completed file remains pending because the configured model did not complete a scan item. The deterministic test covers checkpoint-to-resume reuse.

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (not applicable; no user-facing documentation changes are needed)
  • I have signed the CLA

Related Issues

Closes #995

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 2 issue(s) in this PR.

  • ✅ Successfully posted inline: 1 comment(s)
  • ❌ Failed to post inline: 1 comment(s)

high

📄 cmd/opencodereview/scan_cmd.go (L226-L227)

⚠️ GitHub could not post this as an inline comment: Lines 226-227 could not be resolved (outside PR diff hunks)

Bug: After the refactoring, ctx now refers to the signal-aware context passed in, while runCtx is the context with the newly created span. TraceIDFromContext(ctx) will return an empty string (or a parent trace ID if one happened to exist), not the trace ID of the "scan.run" span we just created.

This should use runCtx to get the trace ID of the span that was started on line 223. The old code used ctx because that was the variable holding the span context before the refactoring renamed it to runCtx.

💡 Suggested Change

Before:

	if telemetry.IsEnabled() {
		traceID = telemetry.TraceIDFromContext(ctx)

After:

	if telemetry.IsEnabled() {
		traceID = telemetry.TraceIDFromContext(runCtx)

Comment thread cmd/opencodereview/scan_cmd.go Outdated
@AllenMuu
AllenMuu force-pushed the feat/scan-cancellation-resume branch from 1d5eba3 to 3dcc865 Compare August 18, 2026 12:26
@AllenMuu

Copy link
Copy Markdown
Contributor Author

Fixed in 3dcc865.

  • The scan trace ID is now read from the runCtx returned by telemetry.StartSpan, preserving the scan.run span trace.
  • Added a regression test for the trace context selection.
  • Added deterministic coverage for completed scan checkpoint → cancellation → persisted session → resume, asserting only unfinished files call the LLM after resume.

Validated with make check and make test (race enabled).

@AllenMuu
AllenMuu marked this pull request as ready for review August 18, 2026 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scan does not propagate Ctrl-C cancellation to the scan runtime

1 participant