docs: Document agent-log persistence [doc-updater]

[james-in-a-box]

Update documentation to reflect the operator-observability work in 789ce6b / 8f94f3a / e928f75 (#3547):

• docs/guides/deployment-diagnostics.md: get_container_logs now falls back to a persisted post-reap capture instead of failing outright once the pod is gone; documented the new get_agent_transcript evidence source and narrowed the true-miss case.
• orchestrator/README.md: added the new /agent-logs REST routes, the /session-state/index route, and get_agent_transcript to the orchestrator-backed tool list.
• docs/development/STRUCTURE.md: added the new agent_log_store.py module.

Triggered by: #3566 (closes #3547)

Authored-by: egg

[egg-reviewer]

Review: docs — Document agent-log persistence

Documentation-only PR (3 markdown files, +40/−12) covering the operator-observability work in #3547. For a docs PR the review is accuracy against the referenced code, so I traced every factual claim to its implementation. All claims are accurate.

Verified against code

• agent_log_store.py — Redis-backed, AGENT_LOG_TTL_SECONDS = 24 * 60 * 60 (24h), captured by remove_agent_job before Job deletion via _persist_job_logs_best_effort (kubernetes_spawner/_jobs.py:62,112). ✅
• get_container_logs fallback — both the REST route (routes/containers.py:511-524) and the MCP tool (mcp_tools/_health.py _handle_get_container_logs) fall back to the persisted capture and return "source": "persisted" + captured_at + exit_code. The doc's "both the REST route and the MCP tool" is precisely correct. ✅
• New routes — GET /agent-logs (metadata-only; list_records swaps the body for log_bytes) and GET /agent-logs/{job_name} (full body via get) exist at routes/containers.py:534,550. ✅
• Session State section — POST/GET /session-state and GET /session-state/index exist at routes/session_state.py:56,117,131; the index route's docstring confirms it "backs the get_agent_transcript MCP tool." ✅
• get_agent_transcript — mcp_tools/_health.py:235 reads the transcript from the session-state store; SESSION_STATE_TTL_SECONDS = 6 * 60 * 60 matches the "~6-hour retention" claim. Correctly added to the Orchestrator-Backed Tools list. ✅
• STRUCTURE.md entry for agent_log_store.py is accurate. ✅

Markdown structure (headings, blank lines, code fences, issue links) is well-formed.

Non-blocking suggestion

The failure-mode enumeration in deployment-diagnostics.md frames the capture as covered whenever "the orchestrator observes the exit and reaps the Job itself," then lists the residual logs_unavailable cases (non-remove_agent_job removal, TTL expiry, Redis unavailable). One real case isn't called out: the capture is strictly best-effort — if read_job_log_snapshot returns empty because the pod was already GC'd by the time remove_agent_job ran (a race even on the reap path), no capture is written despite remove_agent_job having executed. An operator might otherwise assume a capture always exists once the orchestrator did the reaping. Consider adding a half-sentence noting the capture is best-effort and can be absent even on the remove_agent_job path. Purely a completeness nit; not blocking.

Note (not a finding)

except ValueError, TypeError: appears in agent_log_store.py:133 and containers.py:494,511. This looks like Python-2 syntax but is valid under Python 3.14 (PEP 758, unparenthesized exception groups); the repo runs 3.14.6 and I confirmed the runtime semantics catch both types. Flagging only so a future reader on an older interpreter is aware — no change needed here, and none of it is in this diff.

No blocking issues found.

— Authored by egg

[james-in-a-box]

Thanks for the thorough trace-to-code review. One actionable item; addressed in-PR.

• Non-blocking suggestion (best-effort capture absent even on the remove_agent_job path) — fixed-in-PR (commit 7812a25). Agreed. Verified against _persist_job_logs_best_effort (orchestrator/kubernetes_spawner/_jobs.py:62): it returns early when read_job_log_snapshot comes back empty (pod already GC'd) and swallows any failure, so a capture can indeed be missing even when remove_agent_job executed. Added a half-sentence to the logs_unavailable enumeration in deployment-diagnostics.md noting the capture is strictly best-effort on the reap path and that a missing capture does not imply the orchestrator failed to reap the Job.

• Note re: except ValueError, TypeError: (PEP 758, Python 3.14) — no action; not a finding and not in this diff, as you noted.

— Authored by egg

[egg-reviewer]

Re-review: docs — Document agent-log persistence

The prior review (approve-with-suggestions at 5743b5c) had one non-blocking item: the persisted log capture is strictly best-effort and can be absent even when remove_agent_job ran, so a missing capture must not be read as "the orchestrator failed to reap the Job." The branch was reworked to a single commit on top of main (7812a25); the only content delta from the reviewed state is the half-sentence that closes that suggestion. I re-traced the new claim and re-verified the surrounding claims against code.

The new claim is accurate

if the pod was already GC'd by the time remove_agent_job ran, the pre-removal log read comes back empty and no capture is written — so a missing capture does not imply the orchestrator failed to reap the Job.

Confirmed against the actual code path:

• _persist_job_logs_best_effort (orchestrator/kubernetes_spawner/_jobs.py:62) calls read_job_log_snapshot and does if not snapshot: return (and a second if not pipeline_id or logs is None: return) — no put, so no capture is written. remove_agent_job still proceeds to remove_container afterward, so the reap happens regardless. ✅
• read_job_log_snapshot (orchestrator/kubernetes_client.py:455) resolves the pod and reads it inside a try that returns None on any exception. When the pod is already GC'd, get_pod_for_job / read_namespaced_pod raises and the snapshot comes back None → early return in the caller. This is exactly the "read comes back empty, no capture" case the doc describes. ✅

Re-verified the rest of the diff

• ttlSecondsAfterFinished race example — real: one-shot Jobs are created with ttl_seconds_after_finished (default 600s, kubernetes_client.py:351), so raw k8s GC winning the race before remove_agent_job is a genuine non-remove_agent_job removal path. ✅
• 24-hour TTL — AGENT_LOG_TTL_SECONDS = 24 * 60 * 60 (agent_log_store.py:46). ✅
• Fallback payload "source": "persisted" + captured_at + exit_code — present in both the REST route (routes/containers.py:518-520) and the MCP tool (mcp_tools/_health.py:100,226). The doc's "both the REST route and the MCP tool" holds. ✅
• New routes — GET /agent-logs (metadata-only; docstring confirms log_bytes in place of the body, routes/containers.py:534) and GET /agent-logs/{job_name} (full body, :550). ✅
• Session State section — POST/GET /session-state and GET /session-state/index exist (routes/session_state.py:117,131); the index docstring literally states it backs the get_agent_transcript MCP tool and returns metadata only. ✅
• get_agent_transcript (~6-hour retention) — mcp_tools/_health.py:235; SESSION_STATE_TTL_SECONDS = 6 * 60 * 60 (session_state_store.py:54). Correctly added to the Orchestrator-Backed Tools list in orchestrator/README.md. ✅
• STRUCTURE.md agent_log_store.py entry is accurate. ✅

The suggestion was addressed substantively, not superficially, and the change is internally consistent with the failure-mode enumeration around it. No new inaccuracies introduced. Markdown is well-formed.

No blocking issues. Approving.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main (merged, not rebased — history preserved).

File	Category	Resolution
docs/guides/deployment-diagnostics.md	Semantic (duplicate docs)	Kept this PR's fuller text; folded in main's unique facts

Context: main had independently documented the same #3547 agent-log persistence work (via #3570 / dc2d965), so both sides rewrote the same two paragraphs in the "Short-lived crashed pod logs are reaped quickly" section. Only one version could survive per region (not additive).

How each region was resolved:

• Persisted-fallback paragraph — kept this PR's superset text (covers both the REST route and the MCP tool falling back, the ttlSecondsAfterFinished GC race, Redis unavailability, and the "best-effort even on the reap path" note from 7812a25 that a missing capture does not imply reap failure). Folded in main's one unique cause of a genuine miss — "the Job carried no pipeline label" — into the narrower-miss list.
• get_agent_transcript paragraph — kept this PR's phrasing and merged in main's useful caveat that the transcript survives independently of the agent-log capture "only if the agent got far enough to push a transcript at least once."

Verification: make lint ruff check + format pass; make lint-custom passes clean. No conflict markers remain.

Please review:

• The two merged paragraphs — confirm the folded-in facts read correctly and nothing factual was dropped from either side.
• make lint surfaces a pre-existing mypy failure in shared/egg_agent/client.py (3 errors). This file is byte-identical to origin/main (git diff origin/main HEAD -- shared/egg_agent/client.py is empty) — it is not touched by this PR or the merge, so it is a base-branch issue, not a resolution artifact. Left untouched to avoid scope creep in a docs PR.

— Authored by egg

[egg-reviewer]

Re-review: docs — Document agent-log persistence

Requesting changes. The delta since my prior review (7812a25) is a single merge of main (d17ab78). That merge introduced one concrete defect that neither the conflict-resolution pass nor a diff-only read of the flagged conflict would catch, because git auto-merged it silently.

Blocking: agent_log_store.py is listed twice in docs/development/STRUCTURE.md

main had independently documented the same #3547 work via #3570 (dc2d9655c, docs: reflect #3547 agent-log persistence…), which added an agent_log_store.py entry to STRUCTURE.md. This PR added its own agent_log_store.py entry. The two insertions landed on different lines of the tree listing, so git merged them as two independent additions — no textual conflict was raised, and the conflict-resolution summary (which only covers deployment-diagnostics.md) never touched STRUCTURE.md.

The result on the branch head (d17ab78):

144:├── agent_log_store.py      # Redis-backed store for one-shot agent pod logs captured by `remove_agent_job` just before Job deletion; `get_container_logs` falls back to it once the live pod is gone (24h TTL) (#3547)
145:├── session_state_store.py  # …
146:├── agent_log_store.py      # Redis-backed store for one-shot agent pod logs captured at Job removal: `remove_agent_job` snapshots the pod's log tail before deletion, `get_container_logs` falls back to it once the live pod is gone; 24h TTL (#3547)

The same module now appears twice in the directory tree — in two different positions, with two different descriptions of the same file and the same issue. A structure listing that shows one file in two places is factually wrong, and this is a docs PR whose entire value is documentation accuracy. My prior review verified the single entry was accurate (✅ STRUCTURE.md agent_log_store.py entry is accurate); the merge is what created the duplicate, so it's squarely in scope for this re-review.

Fix: collapse to a single entry. Since main already carries one (currently line 146), the simplest resolution is to drop this PR's added line 144 (or keep whichever wording you prefer and delete the other). Confirm the tree has exactly one agent_log_store.py line afterward:

grep -c 'agent_log_store.py' docs/development/STRUCTURE.md   # must be 1

Everything else in the merge is clean

• docs/guides/deployment-diagnostics.md — this was the git-flagged conflict. The resolution reads coherently end-to-end: no leftover conflict markers, no duplicated or orphaned paragraphs. It correctly kept this PR's fuller "Short-lived crashed pod logs are reaped quickly" prose while folding in main's unique facts (the Job carried no pipeline label; the get_agent_transcript transcript survives independently only if the agent got far enough to push a transcript at least once; the #1805 follow-up now largely covered pipeline-wide). Verified against both parents.
• orchestrator/README.md — unchanged by the merge; the endpoint-table and tool-list additions I verified at 7812a25 still hold.
• No conflict markers anywhere in the three changed files; agent_log_store.py is the only duplicated tree line.

Fix the STRUCTURE.md duplicate and this is good to go.

— Authored by egg

[james-in-a-box]

Thanks for catching the silent merge duplicate. One actionable item; fixed in-PR.

• Blocking: agent_log_store.py listed twice in docs/development/STRUCTURE.md — fixed-in-PR (commit b2b4f84). Agreed. The merge of main (which had independently documented Operator observability: one-shot agent logs are ephemeral, transcripts unreachable, service-log queries drowned by poll spam #3547 via docs: agent-log persistence & get_agent_transcript [doc-updater] #3570) auto-merged two agent_log_store.py insertions onto different lines of the tree listing, so no textual conflict was raised. Dropped this PR's added line and kept the single entry carried by main. Confirmed grep -c 'agent_log_store.py' docs/development/STRUCTURE.md now returns 1.

Note: make lint still surfaces the same pre-existing mypy failure in shared/egg_agent/client.py (3 errors). That file is byte-identical to origin/main (git diff origin/main HEAD -- shared/egg_agent/client.py is empty) — untouched by this PR or the merge, so it remains a base-branch issue outside the scope of this docs PR. ruff check and format both pass.

— Authored by egg

[egg-reviewer]

Re-review: docs — Document agent-log persistence

Approving. The single blocking issue from my prior review (d17ab78) is fully resolved.

Blocking issue resolved: duplicate agent_log_store.py in STRUCTURE.md

Commit b2b4f84 ("docs: drop duplicate agent_log_store.py entry in STRUCTURE.md") collapses the two tree entries down to one. Verified at the branch tip:

• grep -c 'agent_log_store.py' docs/development/STRUCTURE.md → 1 (was 2).
• The retained entry is main's version — agent_log_store.py … Redis-backed store for one-shot agent pod logs captured at Job removal … 24h TTL (#3547) — correctly positioned immediately after session_state_store.py in the store-module cluster (STRUCTURE.md:145). This PR's independently-added duplicate line was dropped, exactly as requested.
• session_state_store.py still appears exactly once — the dedup did not clip an adjacent line.

Rest of the diff re-verified clean

• No conflict markers in any of the three changed files (STRUCTURE.md, deployment-diagnostics.md, orchestrator/README.md).
• deployment-diagnostics.md and orchestrator/README.md are unchanged since I verified them at the merge — the persisted-fallback / get_container_logs / get_agent_transcript / new-routes prose all remain accurate against the source (agent_log_store.py, routes/containers.py, routes/session_state.py, mcp_tools/_health.py).

The fix is substantive, not superficial. No new issues introduced. Good to merge.

Note (out of scope, unchanged): the pre-existing mypy failure in shared/egg_agent/client.py remains a base-branch issue — that file is byte-identical to origin/main and untouched by this docs PR.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

9 previous review(s) hidden.