Improve session initialization to check storage first #281
Conversation
Reduces unnecessary "session already exists" errors by checking DO storage before attempting to create a new session.
🦋 Changeset detectedLatest commit: 2cbbf09 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
🐳 Docker Images PublishedDefault (no Python): FROM cloudflare/sandbox:0.0.0-pr-281-956a215With Python: FROM cloudflare/sandbox:0.0.0-pr-281-956a215-pythonVersion: Use the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The storage check was defensive code that added latency but rarely helped since the constructor already loads defaultSession from storage via blockConcurrencyWhile.
ghostwriternr
force-pushed
the
fix/session-initialization
branch
from
414b353 to
933cf84
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Changes INTERNAL_ERROR (500) to RESOURCE_BUSY (409) for "session already exists" errors. This prevents noisy ERROR-level logs during wrangler dev hot reloads since the SDK only logs 5xx errors. Also improves the debug message to clarify state divergence.
|
Re: the guard change from The scenario where they differ (sandboxName changing mid-session) is contrived - sandboxName is set once via More importantly, the root cause of the noisy logs is now fixed - container returns 409 instead of 500, so hitting the error path is clean. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
String matching on error.message is fragile - if the message wording changes, the code silently breaks. Using error.code is type-safe and reliable.
Replaces RESOURCE_BUSY with a semantically correct error code that properly indicates a uniqueness constraint violation rather than a temporary lock.
There was a problem hiding this comment.
Claude Code Review
Previous threads resolved - the force push addressed all concerns:
- String matching replaced with
instanceof SessionAlreadyExistsError(type-safe) SESSION_ALREADY_EXISTSerror type properly implemented following thePORT_ALREADY_EXPOSEDpattern- Diagnostic logging added for state divergence
- Changeset description is accurate
Implementation quality is high. The new error type follows the established pattern perfectly across all layers:
- Error code, context, status map, class, adapter, suggestions all correct
- Container returns 409 instead of 500
- SDK catches specific error type instead of string matching
One gap: test coverage. The new error type has no tests. Consider adding:
- Container test verifying
SESSION_ALREADY_EXISTSwhen creating duplicate session - SDK test verifying graceful handling of the error (hot reload scenario)
Not blocking - implementation is sound and follows well-tested patterns.
Minor note: CLAUDE.md changes (commit message guidelines) are good but unrelated to session initialization. Consider separate PR for doc improvements.
Looks good to merge.
1 participant
Summary
Eliminates noisy ERROR logs for "session already exists" during wrangler dev hot reloads.
Changes
RESOURCE_BUSY(409) instead ofINTERNAL_ERROR(500) for session conflictsensureDefaultSession()with explicit fast path