Skip to content

fix: A lost trigger response hides an already-running queued agent - #1012

Open
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-3cc95d91
Open

fix: A lost trigger response hides an already-running queued agent#1012
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-3cc95d91

Conversation

@sam-saffron-jarvis

Copy link
Copy Markdown
Contributor

What changed

  • Reconcile a failed queue_agent trigger response against /v2/runs?job_id=... using a detached, two-second context.
  • Return the original job/run IDs when the jobs server already admitted the run, rather than reporting a misleading execution failure.
  • Preserve the created job ID in a structured partial error when reconciliation cannot find the run, with guidance to use wait_for_jobs before retrying.
  • Add HTTP tests for a connection closed after trigger admission, cancellation immediately after admission, and an unresolved trigger. The recovery cases assert that only one trigger request is sent.

Why this is high-value

The jobs server commits and wakes a queued run before writing the trigger response. A cancelled turn or transient loopback connection failure could therefore hide an expensive agent that was already queued or running. Jarvis could then retry, launch duplicate work, and lose the original run's handle and result. Reconciliation keeps the existing run observable without retriggering.

Validation

  • go build ./...
  • go test ./internal/tools -count=1
  • go test -race ./internal/tools -run 'TestQueueAgent(RecoversRunAfterLostTriggerResponse|PreservesJobIDWhenTriggerCannotBeReconciled)$' -count=1
  • HOME=<clean temp dir> go test ./...
  • git diff --check

A first go test ./... under the agent's populated home exposed two unrelated cmd skill-discovery tests that omit their temporary project skill when the global 31-skill catalog exceeds the configured display limit. Both tests, and the full suite, pass with a clean temporary HOME matching CI isolation.

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.

1 participant