feat(logging): per-session log files for isolated debugging - #112
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR adds per-session file logging for DeepResearch sessions. It introduces helpers that create ChangesPer-session file logging
Sequence Diagram(s)sequenceDiagram
participant RunSession as MultiSessionManager._run_session
participant Setup as setup_session_logging
participant RootLogger as root logger
participant FileHandler
participant SessionLog as logs/session-<session_id>.log
participant Teardown as teardown_session_logging
RunSession->>Setup: session_id, topic
Setup->>RootLogger: add FileHandler with SessionFilter
Setup->>FileHandler: emit session-start delimiter
FileHandler->>SessionLog: append formatted record
RunSession->>Teardown: handler
Teardown->>RootLogger: remove handler
Teardown->>FileHandler: close()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Acharnite
left a comment
Acharnite
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: APPROVE with nits — The implementation is correct, well-structured, and follows best practices. A few minor issues noted below.
Step 3 — PR Description Review
Minor issues:
-
Missing placeholder in path: The description reads:
logs/session-.logThis is missing the
<session_id>placeholder. Should be:logs/session-<session_id>.log -
Missing
topicparameter: The description sayssetup_session_logging(session_id)but the actual function signature issetup_session_logging(session_id, topic). Thetopicparameter (used for the initial delimiter line) is omitted from the description.
Step 4 — Code Review
observability/session_logging.py ✅
- Path resolution (
_get_log_dir): UsesPath(__file__).resolve().parent.parent.parent.parent / "logs"— identical toserver.pyline 34. Consistent. ✅ SessionFilter: Cleanlogging.Filtersubclass. Injectssession_idintoLogRecordcorrectly. ✅setup_session_logging: Properly creates directory, configuresFileHandlerwith DEBUG level, sets formatter, addsSessionFilter, registers on root logger. Writes initial delimiter line usinghandler.handle()(which runs filters — correct approach). ✅teardown_session_logging: Removes handler from root logger and closes it. Idempotent-safe. ✅- Error handling:
mkdirandFileHandlerraiseOSError(docstring accurate). ✅
sessions.py — _run_session() wiring ✅
Good:
- Setup in
try/exceptwith a clear warning message andexc_info=True✅ - Teardown in
finallyblock ✅ - Lazy import pattern consistent with the rest of the module ✅
_session_log_handler: logging.Handler | Nonetype annotation ✅
One structural concern (minor):
There is a gap between the setup block (lines 297–310) and the main try: block (line 325). Lines 317–324 (cancel event creation + lazy imports) execute outside the inner try/finally. If those intermediate lines raise, the per-session handler would leak (added to root logger but never removed).
# Lines 297-310: setup (ok, caught)
_session_log_handler = ... # might succeed
# Lines 317-324: intermediate code (NOT covered)
cancel_event = asyncio.Event() # very unlikely to fail
from deepresearch.orchestrator ... # very unlikely to fail
# Line 325: inner try/finally starts
try:
...
finally:
# teardown only runs if we reached line 325
if _session_log_handler:
teardown_session_logging(_session_log_handler)In practice, asyncio.Event() and from imports virtually never raise. But if they did (e.g., import error from a dependency change), the handler would leak. To be maximally robust, consider wrapping the entire post-setup body in a single try/finally, or adding an additional guard. Not a blocker.
ADR-0003 & Design Doc Updates ✅
Accurate and consistent with code:
- Version bumped to 1.4 (ADR) and 1.6 (design doc) ✅
- Per-session enhancement noted in ADR §File-Based Logging section ✅
- Design doc §6.5 matches implementation: format, level, lifecycle, safety ✅
- New positive/negative consequences listed ✅
One minor doc mismatch:
Both documents state that the per-session log captures "all loggers under the deepresearch.* namespace." However, the handler is added to the root logger (logging.getLogger()), meaning all loggers (including third-party) will write to the per-session file. This is consistent with how the global deepresearch.log handler works (also on root logger), so it's behaviorally correct — but the "scope" description in §6.5 is technically narrower than reality. Consider updating §6.5 to say "the root logger (which includes all deepresearch.* loggers)."
ADR-0049 / The Ladder Compliance
Lean already. Ship.
- Stdlib only: No new dependencies introduced.
logging.Filter,logging.FileHandler,pathlib.Path— all stdlib. ✅ - No unnecessary abstractions:
SessionFilteris a minimal, idiomaticlogging.Filtersubclass._get_log_dir()exists only to share path resolution withserver.py. No interfaces, factories, or speculative generality. ✅ - No ponytail comments needed: The implementation is simple enough that intentional-shortcut annotations are unnecessary. ✅
"Not lazy about" check ✅
- Trust-boundary validation:
session_idis auto-generated (uuid.uuid4()[:8]), not user-supplied. No injection risk. ✅ - Data-loss error handling: Graceful degradation — setup failure warns and continues. ✅
- Security: Log paths derived from internal IDs, not user input. ✅
Tests
No test file for the new module was found. The existing .testers_done marker suggests testers have signed off, but session_logging.py has 114 lines of new production code without dedicated tests. Consider adding basic tests for setup_session_logging (happy path + error path) and teardown_session_logging (including idempotency).
Summary of Issues
| Severity | Issue | Location |
|---|---|---|
| 🟢 Nit | PR description: logs/session-.log missing <session_id> |
PR body |
| 🟢 Nit | PR description: omits topic parameter from function signature |
PR body |
| 🟢 Nit | Leak gap between setup block and inner try/finally | sessions.py:310-325 |
| 🟢 Nit | Doc scope says "deepresearch.*" but root logger captures all | docs/design/README.md §6.5 |
| 🟡 Minor | No tests for 114 lines of new production code | — |
None of these are blockers. The code is correct, well-documented, and ready to merge.
1 participant
Per-Session Log Files
What: Each session now writes its own dedicated log file at
logs/session-<id>.log, keeping output fully isolated per concurrent session.Why: Prior to this change, all sessions shared a single log stream, producing interleaved output that was difficult to trace during concurrent debugging. Per-session files give developers a clean, session-scoped view of log output.
How: A new module
observability/session_logging.pyprovidessetup_session_logging(session_id)andteardown_session_logging(). These are wired into_run_session()insessions.pyso every session gets automatic log isolation on creation and cleanup on teardown.Safety: Failure to create or write to a per-session log file degrades gracefully — the session continues without interruption and falls back to a console warning. No session is ever blocked by a logging error.
Docs:
Files changed:
observability/session_logging.py— new module (setup/teardown helpers)sessions.py— integration into_run_session()docs/adr/ADR-0003-deepresearch-architecture.md— updateddocs/design/README.md— §6.5 addedSummary by CodeRabbit
New Features
Documentation
Bug Fixes