Skip to content

fix: Progressive runs ignore cancellation while persisting synthetic follow-up/finalization turns#791

Closed
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-d29a76fc
Closed

fix: Progressive runs ignore cancellation while persisting synthetic follow-up/finalization turns#791
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-d29a76fc

Conversation

@sam-saffron-jarvis

Copy link
Copy Markdown
Contributor

What changed

  • replaced context.Background() for progressive synthetic continuation/finalization persistence with a short derived context that honors the active progressive run's cancellation/deadline
  • added progressiveSyntheticUserMessageContext to bound OnSyntheticUserMessage callbacks to 5 seconds at most, while still stopping immediately when the parent progressive context is canceled or times out
  • added regression tests covering both synthetic continuation persistence and synthetic finalization persistence so these callbacks cannot block forever on a detached background context

Why this is high-value

Progressive runs persist synthetic user turns through callbacks that ultimately write to the session SQLite store. Those writes can block behind SQLite busy/retry handling. Before this change, those two persistence calls used context.Background(), so a timed-out run, canceled job, or Ctrl-C could stay stuck persisting synthetic turns long after the progressive run was supposed to stop.

This fix closes that shutdown hole on the real ask --progressive / progressive jobs path without changing the broader progressive control flow.

Validation

  • gofmt -w cmd/progressive.go cmd/progressive_test.go
  • go build ./...
  • targeted regression tests passed:
    • go test ./cmd -run 'TestRunProgressiveSessionSyntheticContinuationPersistenceHonorsContext|TestAttemptProgressiveFinalizationSyntheticPersistenceHonorsContext|TestRunProgressiveSessionTimeoutDoesNotStartDetachedFinalizationAfterDeadline|TestRunProgressiveSessionFinalizesOnNaturalCompletion'
  • git diff --check
  • go test ./... was run twice; the suite still hits the pre-existing unrelated flaky failure in internal/tools (TestRunAgentScriptTool_TimeoutKillsGrandchildren) when the entire repo is run together, but that test passes when rerun in isolation via go test ./internal/tools -run TestRunAgentScriptTool_TimeoutKillsGrandchildren -count=1 -v

@SamSaffron

Copy link
Copy Markdown
Owner

Applied to the working tree locally (along with #789/#790/#791 as a combined cancellation-correctness pass); will land in a direct commit.

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