fix(sdk): degrade per event when a stored event cannot be deserialized - #4114
Draft
JoshKappler wants to merge 1 commit into
Draft
fix(sdk): degrade per event when a stored event cannot be deserialized#4114JoshKappler wants to merge 1 commit into
JoshKappler wants to merge 1 commit into
Conversation
An event whose kind is not registered in this process (a custom tool's observation whose module was never imported, or an event from a newer writer) failed the entire conversation load. The agent-server startup scan drops a conversation on any load exception, so one unknown event 404'd a conversation whose other events were all intact on disk. Skip the unreadable event instead of raising, across every read in the load path: path_to_root, the legacy leaf scan in _resolve_active_leaf, and the RUNNING crash-recovery scan in EventService.start. path_to_root recovers the skipped event's parent_id from the raw payload (it stays readable when the event as a whole does not validate) so the walk stays on its own branch instead of falling back to the linear chain and splicing in a sibling; the recovered id falls back to that chain unless it names an event we hold. Extends the approach OpenHands#3754 took for the pagination path. Closes OpenHands#4080
Collaborator
|
[Automatic Post]: It has been a while since there was any activity on this PR. @JoshKappler, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. This comment was created by an AI agent (OpenHands) on behalf of the user. |
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.
HUMAN:
AGENT:
The change and this description were prepared by an AI agent (Claude) on behalf of Josh Kappler. Leaving the HUMAN section and the ready-for-review flip to him, per the template. Opening as a draft for that reason.
Why
A conversation used a custom tool whose observation subclasses
Observationwithkind="CanvasUIObservation". Resuming without that module imported leaves the kind unregistered, so those events do not deserialize.Event.model_validate_jsonraises, the error propagates up throughrebuild_view->path_to_root, and the agent-server startup scan wraps the whole per-conversation load in onetry/except. The conversation is skipped: it 404s and never appears in listings, even though every other event is intact on disk. One unknown event kind takes down the whole conversation.#3754 already took the skip-unreadable approach for the pagination and search path. This does the same for the load path.
Summary
path_to_rootskips an event it cannot deserialize instead of raising. It recovers the skipped event'sparent_idfrom the raw payload (a plain top-level string, still readable when the event as a whole does not validate), so the walk stays on its own branch instead of falling back to the linear chain and splicing in a sibling. That recovered id comes from a payload that just failed validation, so it is only trusted once it names an event we actually hold, and otherwise falls back to the linear chain._resolve_active_leaf(the legacy scan, taken when no leaf is persisted) and the RUNNING crash-recovery scan inEventService.startnow read per event as well. Both scan the full log, so either one raising still dropped the conversation.UNREADABLE_EVENT_ERRORS,read_event_or_noneandreadable_eventsinevents_list_base.py, using the same exception set as [codex] Skip unreadable events during pagination #3754.Issue Number
Closes #4080
How to Test
Two processes, because the point is that the writer's custom kind is not registered in the reader.
writer.pydefinesCanvasUIObservationand persists a conversation using it;reader.pyresumes that same conversation and never imports it.Unit tests:
Video/Screenshots
Terminal, no GUI surface. Same on-disk conversation in both runs, written once by
writer.py.On current main (v1.36.0):
With this branch, reading that same conversation directory:
The unreadable observation is skipped and both messages survive. The action it would have paired with is dropped from the view by
ToolCallMatchingProperty, so the view never carries a tool call with no result.Each of the three new regression tests was also confirmed to fail without its fix:
ValidationErrorfor the legacy-tail and crash-recovery tests,KeyError: 'ffffffff'for the unknown-parent test.Type
Notes
EventLog.__iter__and__getitem__still raise, deliberately, solen(list(log)) == len(log)stays honest. Reads outside the load path (the fork path inlocal_conversation.py, the goal loop,get_agent_final_response) therefore still surface aValidationError. Those fail one call rather than dropping a conversation at startup, so I left them alone. Say the word if you want them covered here too.