Skip to content

fix(claude): retain scratch until background cleanup completes - #665

Merged
zzwong merged 3 commits into
mainfrom
draft/issue613-cleanup-order-20261008
Oct 11, 2026
Merged

zzwong merged 3 commits into
mainfrom
draft/issue613-cleanup-order-20261008

Conversation

@zzwong

@zzwong zzwong commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Keep invocation scratch until the existing Claude background job cleanup returns by removing the premature cleanup call. The final cleanup call remains in place.
  • Add four synthetic-helper cases covering success, task failure, failed stop, and refused removal. At scratch cleanup, check the recorded commands, exact warnings, and job-directory effects, while preserving the original task error or successful response.
  • Use TempDir-rooted file operations in the regression without adding process discovery or signaling.

This is a narrow cleanup-order fix. It does not establish ownership of detached tool processes or terminate orphaned descendants. #613 remains open.

Validation

Go 1.26.8 on Linux with keyring_nopassage, using isolated synthetic helpers:

  • All four new cases passed normally and with the race detector; no race diagnostics.
  • Selected existing background launch, resume, stale-job GC, and cleanup-warning regressions passed on both unchanged main and the candidate.
  • The same four new cases failed on unchanged production ordering at the intended callback-time removal/warning assertions.
  • Separate negative controls for disabled stop-failure injection, disabled rm-failure injection, suppressed cleanup warnings, and joined cleanup errors failed only their expected cases. These were assertion failures, not compilation or setup failures.
  • All nine planned phases completed without retries, skipped selected tests, resource stops, or source changes.
  • Diagnostic gofmt -l on the exact two changed Go files returned no paths; no formatting edits were needed.

No real Claude or other provider calls were made.

Hosted CI

CI run 37738010272 completed successfully at exact published head 3cb2f1e0d3b63909b4d2ca6e2cb3b5b5a59b3c6f: all 10 jobs passed, including the Linux/macOS/Windows builds, required build aggregate, test, opt-out test, static smoke, lint, identity, and title checks. This hosted result is separate from the focused local validation above and does not validate combined #660/#627 behavior or orphan-process ownership.

Scope and Integration

The published source is based on 999e59ddfc74a67ffeef3c4b4a78a8528365afe6, with exact tested Git tree 2a41227d765c5c82b7c3d77b7456919af38ebd60. Only the production cleanup-call deletion and the new regression file are included.

Locally, full adapter-package tests, full repository tests, lint, make check, standalone builds, native Darwin/Windows checks, cancellation/fallback coverage, and combined integration with #660 were not run for this candidate. This remains a qualified draft.

#660 extracts this cleanup tail into finishClaudeBG. When integrating the two changes, apply this deletion inside finishClaudeBG and preserve #660's timeout fixture rather than replacing that file with this branch's older version. Revalidate the combined source. #627 also changes other parts of subprocess.go; combined behavior is unverified.

Related Issues

Refs #613.

Keep invocation scratch available through existing job cleanup and add
synthetic regressions for ordering, warnings, and result preservation.

Refs #613

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: 3cb2f1e0d3b6
Profile: open-cli-collective-review - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 0

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • go:implementation-tests — complete (broad); scope: 2 files; examples: internal/llmadapters/subprocess.go, internal/llmadapters/subprocess_cleanup_order_test.go; body-inspected: 2 files; examples: internal/llmadapters/subprocess.go, internal/llmadapters/subprocess_cleanup_order_test.go; skipped: none; context (not coverage): 2 files; examples: docs/development.md, internal/llmadapters/subprocess_test.go; constraints: Read-only review; tests and race checks were not executed. Assessment is based on the pinned diff, cleanup implementation, and synthetic-helper assertions.

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 37s | $0.03 | openai-codex/gpt-6.1-sol | cr 0.10.328
Field Value
Model openai-codex/gpt-6.1-sol
Reviewers go:implementation-tests
Engine pi_rpc · openai-codex/gpt-6.1-sol
Reviewed by cr · monit-reviewer
Duration 37s wall · 28s compute
Cost $0.03
Tokens 11.4k in / 281 out

Per-workstream usage

  • orchestrator-selection — openai-codex/gpt-6.1-sol
    • In: 5.7k
    • Out: 109
    • Cache read: 0
    • Cache create: 0
    • Cost: $0.01
    • Duration: 4s
  • go:implementation-tests — openai-codex/gpt-6.1-sol
    • In: 2.0k
    • Out: 125
    • Cache read: 22.3k
    • Cache create: 0
    • Cost: $0.01
    • Duration: 21s
  • orchestrator-rollup — openai-codex/gpt-6.1-sol
    • In: 3.7k
    • Out: 47
    • Cache read: 0
    • Cache create: 0
    • Cost: $0.01
    • Duration: 2s

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: 6548c857ead6
Profile: open-cli-collective-review - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 0

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • go:implementation-tests — complete (constrained); scope: 2 files; examples: internal/llmadapters/subprocess.go, internal/llmadapters/subprocess_cleanup_order_test.go; body-inspected: 2 files; examples: internal/llmadapters/subprocess.go, internal/llmadapters/subprocess_cleanup_order_test.go; skipped: none; context (not coverage): 1 file; examples: internal/llmadapters/subprocess_test.go; constraints: Review used static inspection only; tests were not executed.

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 29s | $0.01 | openai-codex/gpt-6.1-sol | cr 0.10.328
Field Value
Model openai-codex/gpt-6.1-sol
Reviewers go:implementation-tests
Engine pi_rpc · openai-codex/gpt-6.1-sol
Reviewed by cr · monit-reviewer
Duration 29s wall · 20s compute
Cost $0.01
Tokens 5.2k in / 132 out

Per-workstream usage

  • go:implementation-tests — openai-codex/gpt-6.1-sol
    • In: 1.6k
    • Out: 81
    • Cache read: 25.3k
    • Cache create: 0
    • Cost: $0.01
    • Duration: 17s
  • orchestrator-rollup — openai-codex/gpt-6.1-sol
    • In: 3.6k
    • Out: 51
    • Cache read: 0
    • Cache create: 0
    • Cost: $0.01
    • Duration: 2s

@zzwong
zzwong marked this pull request as ready for review October 11, 2026 02:10
@zzwong
zzwong merged commit f5855c0 into main Oct 11, 2026
11 checks passed
@zzwong
zzwong deleted the draft/issue613-cleanup-order-20261008 branch October 11, 2026 02:11
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.

2 participants