Fix forked workers exiting immediately and false memory-persistence failures - #634
Merged
Conversation
Forking channel history by default populated `prior_history` for every new worker, and `run_inner` read that field as "this worker already ran and relayed its result". Every forked worker skipped its task loop and returned empty in about a millisecond. Resumption now keys off the state the worker was built with — only `Worker::resume` yields WaitingForInput. Also read `worker_runs.transcript` as nullable. A NULL blob decodes to an empty vec rather than erroring, so it became `Some([])`, failed the gzip header read, and suppressed the live-transcript fallback.
A memory-persistence run called `memory_persistence_complete` and then took one more LLM turn with nothing left to say. Providers answer that with an empty message, the response parser rejects it, and the branch reported failure after every memory had already been persisted. Terminate the loop once the terminal outcome is recorded, matching what channel turns already do after reply/skip. Branches conclude from the recorded outcome; ingestion treats the same signal as a completed chunk instead of a failure.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughMemory-persistence completion now records terminal outcomes, stops hook processing, and propagates completion through ingestion and branch execution. Worker resumption and empty transcript handling also receive state-specific updates. ChangesMemory persistence completion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bugs from the worker-reliability work, both found by testing #633 against a live instance.
Forked workers never ran
Forking channel history by default populated
prior_historyfor every new worker.run_innerread that field as "this worker already ran and relayed its result", so every forked worker skipped its task loop entirely and returned an empty result in about a millisecond:No LLM call, no tools, nothing. The agent got empty results back and re-spawned the same worker four times trying to verify work that never happened.
prior_historywas carrying two meanings: a resumed idle worker, and a fresh worker seeded with channel context. Only the first has already relayed a result, and the state machine already distinguishes them —Worker::resumeis the only pre-run site that setsWaitingForInput:Second,
worker_runs.transcriptwas read asVec<u8>. A NULL blob decodes to an empty vec rather than erroring, so NULL becameSome([]), failed the gzip header read withunexpected end of file, and suppressed the live-transcript fallback — which is why worker transcripts came back empty in the UI and viaworker_inspect.Memory persistence reported failure after succeeding
A persistence run called
memory_persistence_complete, then took one more LLM turn with nothing left to say. The provider answers that with an emptymessage, the Responses parser rejects it, and the branch reported:Every memory had already been persisted. Channel turns already terminate after
reply/skipfor exactly this reason; the branch's terminal tool never got the same treatment. The hook now terminates once a valid terminal outcome is recorded, branches conclude from that outcome, and ingestion treats the signal as a completed chunk. Tool errors still continue, so the contract-retry path is unchanged.This also drops one LLM call per persistence run and per ingestion chunk.
Testing
Two hook tests cover the new termination: a valid terminal outcome stops the loop with the right reason (both
savedandno_memories), and a tool error does not. Full lib suite passes at 1100.The worker fix has no test — nothing constructs a
Workeroutsidechannel_dispatchwith liveAgentDeps, so covering resume-vs-fork needs a test fixture that doesn't exist yet. Worth adding separately, since this is the second bug in that path.