fix: include sandbox id in session logs#575
Closed
ColeMurray wants to merge 2 commits into
Closed
Conversation
📝 WalkthroughWalkthroughSession logging was made dynamically rebindable to include sandbox_id context: the DO updates its child logger when sandbox IDs change and propagates the new logger to lifecycle, WebSocket, event processor, participant/callback/message-queue services; fetch() preserves and restores logger context across requests. Changes
Sequence DiagramsequenceDiagram
participant DO as DurableObject(SessionDO)
participant Repo as Repository
participant Manager as SandboxLifecycleManager
participant WS as SessionWebSocketManager
participant Event as SessionSandboxEventProcessor
participant Service as Participant/Callback/Queue
DO->>Repo: repository.getSandbox() (maybe modal_sandbox_id)
Repo-->>DO: sandbox_id?
DO->>DO: refreshLoggerSandboxId()
DO->>Manager: manager.setLog(childLogger)
DO->>WS: ws.setLog(childLogger)
DO->>Event: eventProc.setLog(childLogger)
DO->>Service: participant/callback/queue.setLog(childLogger)
alt HTTP fetch request
DO->>DO: save current loggerSandboxId
DO->>DO: handle request (may change sandbox)
DO->>Repo: repository.getSandbox() (post-request)
DO-->>DO: new sandbox_id?
alt changed
DO->>DO: re-anchor child logger
DO->>Manager: manager.setLog(newChild)
DO->>WS: ws.setLog(newChild)
DO->>Event: eventProc.setLog(newChild)
DO->>Service: participant/callback/queue.setLog(newChild)
else unchanged
DO->>DO: restore original logger
end
end
Event->>Event: process sandbox event (heartbeat/token/prompt)
Event->>DO: logs include sandbox_id
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary
This PR updates session and sandbox-event logging so sandbox_id is carried more consistently through Durable Object initialization, sandbox spawn/connect transitions, and a few key event/error logs. I reviewed both changed files in context and did not find any blocking correctness, security, performance, or maintainability issues.
- PR Title and number:
fix: include sandbox id in session logs(#575) - Author:
@ColeMurray - Files changed: 2
- Diff stats: +30 / -8
Critical Issues
- None.
Suggestions
- None.
Nitpicks
- None.
Positive Feedback
- The
fetch()logger restoration logic preserves request-scoped correlation while still updating the base logger when the sandbox ID becomes available mid-request. - The added
sandbox_idfields target the highest-value logs (prompt.complete, background callback errors, and session lifecycle logs) without widening the scope unnecessarily. - Verification passed:
npm test -w @open-inspect/control-plane -- sandbox-eventsandnpm run typecheck -w @open-inspect/control-plane.
Questions
- None.
Verdict
- Approve: Ready to merge.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/control-plane/src/session/durable-object.ts`:
- Around line 773-779: The current refreshLoggerSandboxId replaces this.log but
leaves long-lived collaborators (those constructed earlier that captured the
original logger) unchanged; update the code so that when refreshLoggerSandboxId
runs it also rebinds or updates all logger-dependent singletons/helpers (the
instances that captured the logger at construction) by either (A) refactoring
those helpers to accept a getLog() callback and pass a closure that returns
this.log, or (B) storing and iterating a registry of logger consumers and
calling a provided setLog(newLog) on each; modify refreshLoggerSandboxId to call
that registry or callback so every consumer receives the new child logger and
the sandbox_id context is applied consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbef8e5d-cdd7-49e7-bf5e-4f84717f83e3
📒 Files selected for processing (2)
packages/control-plane/src/session/durable-object.tspackages/control-plane/src/session/sandbox-events.ts
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/control-plane/src/session/durable-object.ts (1)
784-791:⚠️ Potential issue | 🟠 MajorRebind currently misses
PresenceServicelogger consumer.
PresenceServiceis created withlog: this.log(Line 257-268) but is not updated inrebindLoggerConsumers(), so its logs can keep stale context aftersandbox_idchanges.Suggested follow-up
private rebindLoggerConsumers(): void { this._wsManager?.setLog(this.log); this._lifecycleManager?.setLog(this.log); this._participantService?.setLog(this.log); this._callbackService?.setLog(this.log); this._messageQueue?.setLog(this.log); this._sandboxEventProcessor?.setLog(this.log); + this._presenceService?.setLog(this.log); }Also add
setLog(log: Logger)toPresenceServiceif it does not already exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/control-plane/src/session/durable-object.ts` around lines 784 - 791, The rebindLoggerConsumers method omits the PresenceService causing its logger to remain stale; update rebindLoggerConsumers to call this._presenceService?.setLog(this.log) alongside the other consumers, and if PresenceService lacks a setLog(log: Logger) method, implement it (e.g., store the new logger and use it for subsequent logging) so PresenceService's logs reflect the current sandbox context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/control-plane/src/session/durable-object.ts`:
- Around line 784-791: The rebindLoggerConsumers method omits the
PresenceService causing its logger to remain stale; update rebindLoggerConsumers
to call this._presenceService?.setLog(this.log) alongside the other consumers,
and if PresenceService lacks a setLog(log: Logger) method, implement it (e.g.,
store the new logger and use it for subsequent logging) so PresenceService's
logs reflect the current sandbox context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebafb3f9-cc22-44d5-9a54-f7ef7f23ac12
📒 Files selected for processing (7)
packages/control-plane/src/sandbox/lifecycle/manager.tspackages/control-plane/src/session/callback-notification-service.tspackages/control-plane/src/session/durable-object.tspackages/control-plane/src/session/message-queue.tspackages/control-plane/src/session/participant-service.tspackages/control-plane/src/session/sandbox-events.tspackages/control-plane/src/session/websocket-manager.ts
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
1 participant
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.
Summary
sandbox_idto the session Durable Object logger context when an existing sandbox row is available during initialization.sandbox_iddirectly on sandbox event, prompt completion, and callback background error logs.Tests
npm test -w @open-inspect/control-plane -- sandbox-eventsnpm run typecheck -w @open-inspect/control-planeCreated with Open-Inspect
Summary by CodeRabbit