Files
SuperBizAgent-java/openspec/changes/session-run-trace-isolation/design.md
T

145 lines
8.7 KiB
Markdown

## Context
The current MVP persists diagnosis observability through `diagnosis_session`, `agent_step`, and `tool_invocation`. That model works for a single diagnosis per `sessionId`, but it conflates two lifecycles once a caller reuses the same `sessionId` for multi-turn conversation:
- conversation state: Redis `SessionContext.messageHistory` and session metadata;
- execution state: one diagnosis answer, trace, self-evaluation, and feedback target.
The E2E evidence in ISS-010 showed that Redis correctly preserved multi-turn context while MySQL mixed both rounds under the same `session_id`. This breaks trace replay, verifier/evaluation scoping, feedback targeting, and case-library provenance.
Constraints from existing work:
- `GET /api/diagnosis/{sessionId}/trace` is a read-only MVP/demo contract and must remain compatible.
- Current hooks and tools propagate `sessionId` through `RunnableConfig.metadata` and `SessionContextHolder`; `runId` must follow the same execution context boundary.
- Feedback currently writes `DiagnosisSession.feedback` and creates `case_library` from `DiagnosisSession.answer`.
- AIOps is a first-class traceable entry point and cannot be left permanently on the old mixed-run model.
## Goals / Non-Goals
**Goals:**
- Split conversation metadata from per-execution diagnosis state.
- Introduce `runId` as the official identifier for one replayable diagnosis execution.
- Preserve old `sessionId`-only callers by resolving the latest run where possible.
- Scope trace, evaluation, feedback, case creation, demo scripts, and Trace UI by run.
- Migrate historical data into compatibility runs without deleting the old table.
- Include Chat and AIOps in the same release-level change.
- Verify behavior with focused tests, E2E when needed, DB inspection, logs, and baseline drift checks.
**Non-Goals:**
- Do not persist full chat history in MySQL.
- Do not add `diagnosis_trace` or `trace_event`.
- Do not implement full run-list UI.
- Do not physically delete `diagnosis_session`.
- Do not attempt to reconstruct true historical round boundaries when only mixed `session_id` data exists.
## Decisions
| Decision | Choice | Alternative Considered | Rationale |
|---|---|---|---|
| Domain split | Add `chat_session` and `diagnosis_run` | Add `run_id` to `diagnosis_session` only | Separate tables keep conversation metadata and execution state from growing into one coupled table. |
| Trace detail storage | Reuse `agent_step` and `tool_invocation`, adding `run_id` | Add `diagnosis_trace` / `trace_event` | Existing detail tables already represent trace; isolation needs a run key, not a new event model. |
| API identity | `runId = "run-" + UUID` | Reuse short session id or DB id | Full UUID avoids collision and keeps external IDs independent from database internals. |
| Latest-run compatibility | `GET /api/diagnosis/{sessionId}/trace` resolves latest by `created_at DESC, id DESC` | Require `runId` immediately | Compatibility keeps existing demo/UI/scripts working while new clients migrate. `updated_at` is avoided because feedback/eval updates can reorder old runs. |
| Historical migration | Backfill one compatibility run per existing `diagnosis_session` | Try to split old mixed rows | Old rows do not contain reliable run boundaries. A compatibility run preserves auditability without inventing data. |
| Feedback fallback | Missing `runId` binds latest run and returns fallback metadata | Reject missing `runId` immediately | Short-term compatibility is needed for old clients; explicit fallback keeps ambiguity observable. |
| Case library provenance | New automatic cases store `diagnosis_id = run_id` | Add a new case-library column now | Existing column name can carry transitional provenance; docs and query logic must recognize old `session_id` and new `run_id`. |
| AIOps phase | Implement after Chat but before overall archive | Leave AIOps for a follow-up issue | AIOps is already a trace entry point; leaving it old-model would preserve the same bug on another endpoint. |
| Ownership integrity | Validate run/session ownership in application services; do not add database foreign keys in this change | Add foreign keys from `diagnosis_run`, `agent_step`, and `tool_invocation` | Existing historical/orphan compatibility data and rollback needs make additive, application-level validation safer for this release. |
## Data Model
```text
chat_session
id
session_id unique
status
message_pair_count
created_at
last_active_at
expires_at
diagnosis_run
id
run_id unique
session_id
query
status
agent_flow
answer
self_evaluation
feedback
total_duration_ms
total_token_count
step_count
tool_call_count
created_at
updated_at
agent_step
session_id
run_id
...
tool_invocation
session_id
run_id
...
```
`chat_session.expires_at` is nullable MySQL directory metadata and may be a best-effort Redis TTL snapshot when known. Redis TTL can expire `SessionContext.messageHistory`; persisted `diagnosis_run`, `agent_step`, and `tool_invocation` remain audit records.
Ownership between `chat_session`, `diagnosis_run`, `agent_step`, and `tool_invocation` is enforced by service-layer validation and indexed lookup in this change. The migration intentionally does not add database foreign keys so historical orphan trace detail rows and rollback paths remain compatible.
## API / Interface Impact
Interface level: L4.
- Database contract changes: new tables, new columns, backfill, indexes, and later non-null expectations for new writes.
- `/api/chat` response adds `runId`.
- `/api/ai_ops` SSE emits a compatible metadata message before report content. It keeps the existing SSE event name `message` and sends a JSON `SseMessage` with `type=metadata`; the metadata payload includes `sessionId` and `runId`. Report content continues to stream through the existing `type=content` message shape.
- Trace API accepts optional `runId`.
- Feedback request accepts preferred `runId`. Feedback response includes the actual bound `runId` and `fallbackToLatestRun`; wrong-session `runId`, missing session, and missing run use the existing failed feedback response path with HTTP 400 from `FeedbackController`.
- New run summary API: `GET /api/chat/session/{sessionId}/runs`, returned through the existing `ApiResponse<List<RunSummary>>` wrapper. A session with metadata but no runs returns an empty list; a missing session returns the existing not-found/error behavior.
Compatibility:
- Old `sessionId`-only trace and feedback calls bind to latest run.
- Old `diagnosis_session` is retained for rollback and historical comparison.
- New code must not keep writing new execution state into `diagnosis_session` after the write switch.
## Migration Plan
1. Add `chat_session` and `diagnosis_run`.
2. Add nullable `run_id` to `agent_step` and `tool_invocation`.
3. Backfill `diagnosis_run` from existing `diagnosis_session`.
4. Backfill old `agent_step.run_id` and `tool_invocation.run_id` from the compatibility run.
5. Add indexes for `session_id`, `run_id`, latest-run lookup, and trace-detail lookup.
6. Deploy repository and read-path compatibility.
7. Switch Chat write path to `chat_session + diagnosis_run`.
8. Switch Chat evaluation and verifier support reads to run-scoped data as part of the Chat write-path cutover.
9. Switch Trace run resolution and run listing.
10. Switch Feedback and case-library paths.
11. Switch AIOps write path, including `diagnosis_run.self_evaluation.aiops_rule_evaluation`.
12. Switch demo scripts, Trace UI, and MVP docs.
13. Verify no new rows are missing `run_id`; only then tighten application-level and, if safe, database-level non-null assumptions for new data.
Rollback:
- Keep `diagnosis_session` intact during this change.
- Migrations are additive until constraints are tightened.
- If write-switch rollout fails, rollback code can read the retained old table while migrated compatibility rows remain harmless.
## Risks / Trade-offs
- [Risk] ThreadLocal/context propagation may miss `runId` in nested agent/tool calls. -> Mitigation: introduce a unified execution context carrying both IDs and test hook/tool recording.
- [Risk] Baseline metrics change because cross-round tool rows are no longer counted. -> Mitigation: run baseline diff and document expected drift.
- [Risk] Legacy `case_library.diagnosis_id` values are ambiguous. -> Mitigation: document transitional semantics and keep lookup logic aware of old `session_id` values.
- [Risk] AIOps SSE clients may not parse a new metadata event. -> Mitigation: add metadata in a compatible stream message and keep final report streaming behavior.
- [Risk] Historical mixed trace cannot be truly separated. -> Mitigation: call this out as compatibility data, not reconstructed truth.
## Open Questions
None blocking. Long-term tightening of missing feedback `runId` remains a follow-up decision after clients migrate.