Handle orchestrator spawn failures gracefully

t-298.3·WorkTask·
·
·
·Omni/Agent/Tools/Tasks.hs
Parent:t-298·Created1 month ago·Updated1 month ago

Description

Edit

Currently work_on_task uses spawnProcess which doesn't capture errors. If the orchestrator crashes immediately (missing command, permission error, etc), Ava doesn't know and reports success.

Should either: 1. Use createProcess and check exit code after a short delay 2. Have orchestrator write a 'started' sentinel file 3. Check for log file creation 4. Use a different spawn mechanism that can report early failures

Timeline (4)

🔄[human]Open → InProgress1 month ago
💬[human]1 month ago

CHANGES REQUESTED: Looking at this code review task, let me analyze the changes against the requirements.

Task Analysis

The task asks to handle orchestrator spawn failures gracefully. The current code used spawnProcess which doesn't capture errors. The task suggested several approaches: 1. Use createProcess and check exit code after a short delay 2. Have orchestrator write a 'started' sentinel file 3. Check for log file creation 4. Use a different spawn mechanism

Code Review

Does it accomplish the task?

Yes - the implementation uses approach #1: createProcess with a 500ms delay to check if the process exits early with a failure code. This catches:

  • Missing script/command errors
  • Permission errors
  • Immediate crashes

Are there any bugs or issues?

The implementation looks solid:

  • Uses try @SomeException to catch spawn failures
  • Sets new_session = True to detach the process
  • Waits 500ms to catch immediate failures
  • Returns appropriate error messages for different failure modes
  • The pattern match (_, _, _, ph) ignores stdin/stdout/stderr handles which is fine since they're set to NoStream

One minor concern: if the orchestrator exits with ExitSuccess immediately, it's reported as success. This is noted as "suspicious but ok" in the comment, which seems reasonable.

Did lint/build/test pass?

  • Build: ✅ Passed (green output)
  • Test: ✅ Passed ("All 1 tests passed")
  • Lint: ⚠️ Has 2 hints:

1. Unused {-# LANGUAGE DeriveGeneric #-} pragma 2. Suggestion to use </ instead of <$> on line 45

These are warnings/suggestions, not errors. The unused pragma and the <$> vs </ style issue are minor and don't affect correctness.

Is the code clean and following conventions?

  • Uses qualified imports as required by project conventions
  • Properly uses <| operator (project's convention for $)
  • Good separation with the new spawnOrchestrator helper function
  • Clear comments explaining the logic
  • Follows the existing code style

Lint Issues

The lint warnings should probably be fixed: 1. Remove the unused DeriveGeneric pragma 2. Change WorkOnTaskArgs <$> v .: "task_id" to WorkOnTaskArgs </ (v .: "task_id")

These are minor style issues but the lint tool explicitly calls them out.

The implementation is correct and accomplishes the task well. However, the lint warnings should be addressed before merging: 1. Remove the unused {-# LANGUAGE DeriveGeneric #-} pragma 2. Use </ instead of <$> as suggested by hlint (line 45)

🔄[human]InProgress → Done1 month ago