Provision a single shared sandbox app per test run#705
Open
SimonWoolf wants to merge 6 commits into
Open
Conversation
fullPresenceDecoder reused the same destination slice across pages in the Items() auto-paging iterator. When decoding msgpack, the codec reuses the existing slice elements, so page 2+ decoded wire bytes into PresenceMessages whose Data field already held the previous page's decoded values. With non-uniform Data types across messages this corrupted decoding and produced errors such as "msgpack decode error: invalid byte descriptor for decoding bytes". Reset the destination slice to nil before decoding in both CodecDecodeSelf and UnmarshalJSON. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Updates the ably-common submodule to a revision whose test-resources/test-app-setup.json includes the mutable namespace (required by the message-update tests) alongside the shared presence fixtures. Consumed by the shared-sandbox-app test setup. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Integration tests previously provisioned a fresh sandbox app for every test (~150 per run) via the per-call-site NewSandbox/NewRealtime/NewREST helpers. Each provision creates a whole account and app server-side, which is expensive and puts needless load on the sandbox environment; no other Ably SDK does this. Provision one shared app per test run instead: - ablytest/sandbox.go: drop the dynamic Config/Key/Namespace/Presence/Channel structs used to build the request body (no test ever passed a custom config) and POST the static ably-common appspec (test-app-setup.json) verbatim, located relative to the source via runtime.Caller. A minimal Config decodes only the appId and keys from the response. - Add an Ably-Agent header (RSC7d) to the /apps POST and DELETE, reusing the client's agent string via the newly exported ably.AgentIdentifier and ably.AblyAgentHeaderName. - GetSharedApp provisions the shared app once (sync.Once); NewSandbox(nil), MustSandbox, NewRealtime and NewREST all return it. Sandbox.Close is a no-op unless the app is owned; the shared app is torn down once via CloseSharedApp from TestMain. - KeyParts now selects the key carrying the [*]* wildcard capability rather than keys[0], since the shared appspec's first key has only the default capability (which does not grant qualified/derived channels). - Add ablytest.ChannelName for process-unique channel names, and use it in the history/presence tests that assert on aggregate channel state, which can no longer rely on per-app isolation. - PresenceFixtures now matches the appspec's six members, including the AES-encrypted client_encoded member, decoded in the presence-fixtures read tests via the new PresenceFixturesCipher helper. The full integration suite passes under both the JSON and msgpack protocols. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors the test sandbox to a single shared app per process, fixes presence pagination decoding, exposes Ably-Agent helpers, adds process-unique channel naming, updates presence fixture decoding, and expands ErrorCode string mappings. ChangesSandbox and Presence Test Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ablytest/sandbox.go (1)
269-269: 💤 Low valueUse
http.NewRequestWithContextfor the DELETE request.The static analysis correctly identifies that
http.NewRequestshould be replaced withhttp.NewRequestWithContextfor proper context propagation. While this is test infrastructure, using contexts allows for proper cancellation and timeout handling.Proposed fix
- req, err := http.NewRequest("DELETE", app.URL("apps", app.Config.AppID), nil) + req, err := http.NewRequestWithContext(context.Background(), "DELETE", app.URL("apps", app.Config.AppID), nil)Note: You'll need to add
"context"to the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ablytest/sandbox.go` at line 269, Replace the call to http.NewRequest in the DELETE request creation with http.NewRequestWithContext to propagate cancellation/timeouts; update the statement using an existing context value (e.g. ctx) or context.Background() if no context is available, and add the "context" import. Locate the line that constructs the request (the req, err := http.NewRequest("DELETE", app.URL("apps", app.Config.AppID), nil) call) and change it to use http.NewRequestWithContext(ctx, "DELETE", app.URL(...), nil) and ensure the ctx variable is supplied or created.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ablytest/sandbox.go`:
- Line 269: Replace the call to http.NewRequest in the DELETE request creation
with http.NewRequestWithContext to propagate cancellation/timeouts; update the
statement using an existing context value (e.g. ctx) or context.Background() if
no context is available, and add the "context" import. Locate the line that
constructs the request (the req, err := http.NewRequest("DELETE",
app.URL("apps", app.Config.AppID), nil) call) and change it to use
http.NewRequestWithContext(ctx, "DELETE", app.URL(...), nil) and ensure the ctx
variable is supplied or created.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccab2521-343b-410c-b891-ea0c1c3cc25b
📒 Files selected for processing (8)
ably/main_integration_test.goably/proto_http.goably/rest_channel_spec_integration_test.goably/rest_presence.goably/rest_presence_spec_integration_test.goablytest/ablytest.goablytest/sandbox.gocommon
Member
Author
|
Re the nitpick about using It was flagged only because line 269 is the line this PR touched, but there are three identical More importantly, Leaving all three as-is for consistency. — Claude |
lmars
reviewed
Jun 10, 2026
Address review feedback on the shared-sandbox setup: - Read the presence fixtures directly from the appspec JSON instead of duplicating them in Go, removing the "must stay in sync" hazard. - Drop the unused config argument from NewSandbox and MustSandbox, and remove NewSandboxWithEndpoint (both callers passed the default endpoint); update the call sites accordingly. - Use sync.OnceValue / sync.OnceValues for the appspec load and the shared app instead of hand-rolled sync.Once + package vars. - Remove the dead `owned` field and dedicated-app code path: no test provisions a non-shared app. Close is now unconditionally a no-op and the shared app is deleted via an internal delete method from CloseSharedApp. No behavioural change; the full integration suite passes under both the JSON and msgpack protocols. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ablytest/sandbox.go (1)
214-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose 504 responses before retrying.
When Line 214 takes the retry path for a 504, the response body stays open. A few consecutive provisioning timeouts will leak sockets and prevent connection reuse in the same run.
Suggested patch
- if err != nil || (resp != nil && resp.StatusCode == 504) { // gateway timeout + if err != nil || (resp != nil && resp.StatusCode == http.StatusGatewayTimeout) { + if resp != nil && resp.Body != nil { + resp.Body.Close() + } // Timeout. Back off before allowing another attempt. log.Println("warn: request timeout, attempting retry") time.Sleep(retryInterval) retryInterval *= 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ablytest/sandbox.go` around lines 214 - 220, When the retry branch (err != nil || (resp != nil && resp.StatusCode == 504)) is taken we must close the response body to avoid leaking sockets; update the branch so that if resp != nil you close (and optionally drain) resp.Body before sleeping and doubling retryInterval. Locate the conditional that checks err and resp.StatusCode == 504 and add logic to call resp.Body.Close() (or drain then close) for the 504 case prior to time.Sleep(retryInterval) so the existing defer in the else branch remains correct.
🧹 Nitpick comments (1)
ablytest/sandbox.go (1)
285-292: ⚡ Quick winMatch the wildcard capability semantically.
Line 286 compares the server-provided capability JSON by exact string value. Any harmless reformatting of that JSON makes the wildcard key invisible and silently falls back to
Keys[0], which is the wrong behavior for the shared-app tests that need[*]*.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ablytest/sandbox.go` around lines 285 - 292, The code currently does a string equality check against wildcardCapability; instead parse the capability JSON in each key (k.Capability) and semantically detect the wildcard form (unmarshal into map[string][]string and check that there is an entry with key "*" whose value slice contains "*" ), returning that key when matched; update the loop that inspects app.Config.Keys (and the wildcardCapability reference) to attempt json.Unmarshal for each k and continue on unmarshal errors, so harmless formatting changes in the JSON won’t cause a silent fallback to app.Config.Keys[0].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ably/rest_channel_integration_test.go`:
- Around line 28-30: Tests use a shared sandbox from NewSandbox() but still use
fixed channel names which causes cross-test state leakage; update every subtest
that asserts exact history/state (where it checks len(history), dedupe results,
fallback publish counts, etc.) to derive per-test unique channel names by
calling ablytest.ChannelName(...) instead of hard-coded strings (replace
occurrences in the asserting subtests around the ranges noted, e.g., the blocks
starting ~lines 35-99, 154-185, 188-224, 244-265, 267-294, 351-379) so each test
uses a unique channel name when creating channels and publishing/reading
history.
In `@ablytest/sandbox.go`:
- Around line 178-183: CloseSharedApp currently calls sharedApp()
unconditionally and swallows errors; change logic so teardown never triggers
initialization and surface provisioning errors: introduce/ensure a boolean flag
sharedAppInitialized that the initializer (e.g., NewSandbox or the wrapper that
first calls sharedApp()) sets to true when a real provisioning attempt started;
in CloseSharedApp check sharedAppInitialized first and return nil immediately if
false (do not call sharedApp()), otherwise call sharedApp() and if it returns an
error propagate that error instead of returning nil, then call app.delete() on
the valid app.
---
Outside diff comments:
In `@ablytest/sandbox.go`:
- Around line 214-220: When the retry branch (err != nil || (resp != nil &&
resp.StatusCode == 504)) is taken we must close the response body to avoid
leaking sockets; update the branch so that if resp != nil you close (and
optionally drain) resp.Body before sleeping and doubling retryInterval. Locate
the conditional that checks err and resp.StatusCode == 504 and add logic to call
resp.Body.Close() (or drain then close) for the 504 case prior to
time.Sleep(retryInterval) so the existing defer in the else branch remains
correct.
---
Nitpick comments:
In `@ablytest/sandbox.go`:
- Around line 285-292: The code currently does a string equality check against
wildcardCapability; instead parse the capability JSON in each key (k.Capability)
and semantically detect the wildcard form (unmarshal into map[string][]string
and check that there is an entry with key "*" whose value slice contains "*" ),
returning that key when matched; update the loop that inspects app.Config.Keys
(and the wildcardCapability reference) to attempt json.Unmarshal for each k and
continue on unmarshal errors, so harmless formatting changes in the JSON won’t
cause a silent fallback to app.Config.Keys[0].
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c61c382-8738-4060-a89f-b523125a5cf9
📒 Files selected for processing (12)
ably/auth_integration_test.goably/http_paginated_response_integration_test.goably/main_integration_test.goably/message_updates_integration_test.goably/realtime_channel_integration_test.goably/realtime_channel_spec_integration_test.goably/realtime_client_integration_test.goably/realtime_conn_spec_integration_test.goably/rest_channel_integration_test.goably/rest_channel_spec_integration_test.goably/rest_client_integration_test.goablytest/sandbox.go
✅ Files skipped from review due to trivial changes (1)
- ably/message_updates_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- ably/main_integration_test.go
The sync.OnceValues rewrite dropped the guard that made CloseSharedApp a no-op when no test ever requested the shared app: it called sharedApp() unconditionally, which would provision an app on teardown just to delete it (for a test run that touches no sandbox). It also returned nil when provisioning had failed, masking a real error. Track whether NewSandbox was ever called and short-circuit CloseSharedApp when it wasn't; otherwise propagate any provisioning error. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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
2 participants
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
Integration tests previously provisioned a fresh sandbox app for every test (~150 per run) via the per-call-site
NewSandbox/NewRealtime/NewRESThelpers. Each provision creates a whole account and app server-side, which is expensive and puts needless load on the sandbox environment — and no other Ably SDK does this (ably-js provisions one app per suite from a shared appspec).This PR provisions one shared app per test run instead, modelled on ably-js's approach.
Commits
fix:reset presence decoder destination between paginated pages — a pre-existing SDK bug (not test-only).fullPresenceDecoderreused the same destination slice across pages in theItems()auto-paging iterator; under msgpack the codec reuses existing slice elements, so page 2+ decoded intoPresenceMessages whoseDatastill held the previous page's decoded values. With non-uniformDatatypes this corrupted decoding (msgpack decode error: invalid byte descriptor for decoding bytes). Fixed by resetting the destination to nil before decoding. Affects any user paginating presence viaItems()over msgpack with mixed data.chore:bump ably-common submodule — to a revision whosetest-app-setup.jsonincludes themutablenamespace (needed by the message-update tests) alongside the shared presence fixtures.test:provision a single shared sandbox app per run — the refactor itself.What the refactor does (
ablytest/)sandbox.go: drops the dynamicConfig/Key/Namespace/Presence/Channelrequest-body structs (no test ever passed a custom config) and POSTs the static ably-common appspec verbatim, located viaruntime.Caller. A minimalConfigdecodes onlyappIdandkeysfrom the response.Ably-Agentheader (RSC7d) to the/appsPOST and DELETE, reusing the client's agent string via the newly exportedably.AgentIdentifier/ably.AblyAgentHeaderName.GetSharedAppprovisions the shared app once (sync.Once);NewSandbox(nil),MustSandbox,NewRealtime,NewRESTall return it.Sandbox.Closeis a no-op unless the app isowned; teardown happens once viaCloseSharedAppfrom a newTestMain.KeyPartsselects the key carrying the[*]*wildcard capability rather thankeys[0], since the shared appspec's first key has only the default capability (which doesn't grant qualified/derived channels).ablytest.ChannelNamefor process-unique channel names, used in the history/presence tests that assert on aggregate channel state and can no longer rely on per-app isolation.PresenceFixturesnow matches the appspec's six members, including the AES-encryptedclient_encoded, decoded in the presence-fixtures read tests via the newPresenceFixturesCipherhelper.Testing
The full integration suite passes under both the JSON and msgpack protocols.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements