feat(persistence): wire execution event writers (stage 3.2) - #2145
feat(persistence): wire execution event writers (stage 3.2)#2145rogercloud wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new execution-event storage and writer mechanism (stages 3.1–3.2) for version-two tasks, adding the task_execution_events table and integrating event-store primitives and trace adapters to record authoritative execution facts. The review feedback highlights two critical issues: missing imports (current_task_lease and READABLE_CHECKPOINT_TYPES) in trace_handlers.py that will cause runtime NameErrors, and a potential StopIteration exception in the migration script when calling next() on a generator without a default value.
ee62cc5 to
de1f19c
Compare
f5ae593 to
5ac2910
Compare
| batch_id = str(uuid4()) | ||
| for tool_call in tool_calls: | ||
| tool_call["assistant_message_id"] = batch_id | ||
| tool_call["tool_attempt_id"] = str(uuid4()) |
There was a problem hiding this comment.
[P1] Use this attempt identity for control-tool outbound messages
These IDs protect normal tool execution, but the send_message and ask_user_question control paths never pass them to runtime.send_message(). After the after_llm checkpoint has stored this pending call, the outbound transaction can commit and the process can exit before the following checkpoint removes the call. Recovery then executes the same control call again, generates a new outbound UUID, and sends/persists the same message twice.
I reproduced the persistence side directly: replaying the same outbound event_id produces one deduplicated outbound fact but two assistant_message facts and two chat rows, because stage_chat_message_no_commit() assigns a random identity to non-terminal assistant messages. Please propagate tool_attempt_id/assistant_message_id through the control-message path and use a stable identity for both the outbound fact and chat projection, or route control tools through the same attempt start/end protocol.
Wire stage 3.1's execution-event store into explicitly created version-two test tasks. Inputs, commands, runtime/tool events, recovery state, outbound streams, and execution settlement now produce durable facts. Existing tasks and public task creation retain version-one routing.
Commit facts before broadcasting and derive transitional chat/Trace/checkpoint records in the same transaction. Preserve tool batch and attempt identities in recovery state, reject stale bound leases, and propagate persistence failures through Runner, DAG, and delegated agents so uncertain external effects are not automatically retried. Add a unique event reference to compatible chat rows to make message projection replay idempotent.
This is migration stage 3.2 and addresses part of #2089. Event-based readers and production rollout remain subsequent stages; version-two tasks must remain test-only. Schema downgrade refuses while version-two tasks exist.
Stage 3.1 dependency #2141 has merged. This branch is rebased onto upstream main; the PR diff now contains only stage 3.2 and its review/CI fixes.
Validation: 1,036 tests passed across SQLite, isolated PostgreSQL, and relevant legacy paths. All applicable pre-commit hooks passed, including repository-wide mypy, Ruff, isort, codespell, and the Alembic revision-graph check. Alembic used the project's Python 3.11 test environment.
After syncing the stage 3.1 pagination fix: all 68 event-store and event-writer tests passed on SQLite and isolated PostgreSQL, and all applicable pre-commit hooks passed.
Web CI follow-up: 683 focused tests passed after rebasing onto main, including checkpoint compatibility and the previously failing Web tests. All applicable pre-commit hooks passed.