Repository navigation
Conversation
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
…ncellable Signed-off-by: letv1nnn <letv1n2007@icloud.com>
Signed-off-by: letv1nnn <letv1n2007@icloud.com>
|
thanks for porting this! I'll follow up on any review here. |
rhuss
left a comment
rhuss
left a comment
There was a problem hiding this comment.
cc-review Summary
What Went Well
openshell/v1/sandbox_watch_logs.gorunWatchLogs (lines 145-147) keeps the resume cursor as a high-water mark with a comment explaining out-of-order live delivery across the log and platform sources, so a later lower cursor cannot rewind the resume point.TestWatchLogs_CursorIsHighWaterMarksends cursor 5 then 2 before a drop and asserts the redial resumes from 5.openshell/v1/sandbox_watch_logs.goretryWatchLogs and isRetryableWatchLogsError isolate the retry decision in one place and retry only on the raw gRPCUnavailablecode. ResourceExhausted (also mapped toErrorUnavailable), PermissionDenied and Unauthenticated stay terminal, and OUT_OF_RANGE is surfaced as a terminalEventErroras the proto contract requires.openshell/v1/watch.goemit and sleep (lines 38-62) select on the result channel,w.doneandctx.Done(), and sleep stops its timer withdefer. Stop therefore interrupts both a blocked send and a backoff wait, verified byTestWatchLogs_StopInterruptsBackoff.openshell/v1/sandbox_watch_logs.gorunWatchLogs (lines 90-91) defersclose(w.result)andw.cancel(), so every exit path releases the gRPC stream and the single producer closes the channel exactly once.openshell/v1/sandbox_watch_logs.goline 139 resets the backoff only after a converted item is delivered, not after the skipped status snapshot, which delivers the stated fix commit.TestWatchLogs_BackoffGrowsWhenOnlySnapshotsArrivepins the 100/200/400ms sequence.openshell/v1/internal/converter/watch_log.goWatchLogEventFromProto keeps all proto oneof dispatch in the converter and returns nil for non-stream payloads, with table coverage inTestWatchLogEventFromProtoSkipsNonStreamPayloads. PlatformEventFromProto copies the metadata map, and the request copiesLogSources, so caller and SDK share no mutable state.openshell/v1/sandbox_watch_logs.goWatchLogs resolves the sandbox withGetunder the caller's workspace before opening the stream and passesnamedWorkspaceScope(workspace)on every redial, consistent with the other workspace-scoped clients.openshell/v1/types/errors.goappendsErrorOutOfRangeat the end of the iota block, so existingErrorCodevalues are unchanged, and adds a matchingStringcase andIsOutOfRangehelper.openshell/v1/types/watch_log.goandopenshell/v1/sandbox.gofollow the established types package plus v1 alias pattern, andopenshell/v1/fake/sandbox.goWatchLogs mirrors the fake GetLogs convention (closed client returnsErrorUnavailable, otherwiseErrorUnimplemented).openshell/v1/sandbox_client_test.gomockSandboxServer.watchFunc/watchRequests clone every request per attempt, so tests can script each reconnect, andTestWatchLogs_OutOfRangeIsTerminalassertsIsOutOfRange, notIsInvalidArgument, and exactly one dial.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Important | openshell/v1/sandbox_watch_logs.go:145 |
Avoid advancing the resume cursor past undelivered events. | codex |
| Minor | openshell/v1/fake/sandbox.go:904 |
The new fake WatchLogs (Unimplemented, or Unavailable when the client is closed) has no tests... | test-quality |
| Minor | openshell/v1/sandbox.go:121 |
WatchLogs is added directly to the exported SandboxInterface, breaking every external... | goal-alignment |
| Minor | openshell/v1/sandbox_watch_logs.go:34 |
A package-level mutable function variable (testHookWatchLogsSleep) is added to production code... | architecture |
| Minor | openshell/v1/sandbox_watch_logs.go:120 |
The transparent-reconnect guarantee depends on stream.Recv returning Unavailable, but the client... | production |
| Minor | openshell/v1/sandbox_watch_logs.go:176 |
WatchLogs retries Unavailable forever with no signal to the consumer, so a gateway that stays... | gator |
| Minor | openshell/v1/sandbox_watch_logs.go:192 |
The status.Code fallback in isRetryableWatchLogsError is unreachable, because its only caller... | architecture |
| Minor | openshell/v1/sandbox_watch_logs_test.go:174 |
TestWatchLogs_RequestFields never asserts the request's Sandbox name or WorkspaceScope, and no... | test-quality |
| Minor | openshell/v1/sandbox_watch_logs_test.go:348 |
No test covers ResourceExhausted, the one code where the SDK classification (ErrorUnavailable)... | test-quality |
| Minor | openshell/v1/sandbox_watch_logs_test.go:414 |
The backoff cap (watchLogsMaxBackoff via min()) is never exercised: the backoff tests stop at... | test-quality |
| Minor | openshell/v1/sandbox_watch_logs_test.go:579 |
TestWatchLogs_ContextCancelClosesChannel throws away the drained events, so it cannot detect a... | test-quality |
| Minor | openshell/v1/types/errors.go:28 |
ErrorOutOfRange and IsOutOfRange are documented as meaning only 'a stream resume cursor is... | architecture |
| Notable | openshell/v1/sandbox_watch_logs.go:54 |
Need is UNCLEAR: the PR description only states that the change is ported from... | goal-alignment |
| Notable | openshell/v1/watch.go:36 |
The new watcher.emit/sleep/stopped helpers centralize the send-or-stop select, but the existing... | architecture |
All findings are posted as inline comments. The one that matters most is the Important resume-cursor issue in sandbox_watch_logs.go (lines 145-146): a high-water cursor can skip an undelivered lower-cursor event after a drop. Everything else is nonblocking polish, documentation and test coverage.
Review Details
- Review type: initial review, full scope (no earlier review on this PR)
- Findings posted: 14
- Findings reviewed and not posted: 0
- Gate outcome: FAIL (one Important finding)
- Participating reviewers: architecture, goal-alignment, production, test-quality, gator, codex
- Coverage: incomplete. The correctness and security reviewers ran and found nothing, but the CodeRabbit reviewer failed to run, so this review does not count as complete coverage.
Coverage limitation: required reviewer coderabbit failed, so this run cannot establish a completed review baseline.
|
|
||
| // WatchLogs returns Unimplemented — the fake client has no log or platform | ||
| // event stream to replay, so it cannot honor the cursor resume contract. | ||
| func (c *fakeSandboxClient) WatchLogs(_ context.Context, _, _ string, _ ...v1.WatchLogsOptions) (types.WatchInterface[*types.WatchLogEvent], error) { |
There was a problem hiding this comment.
Minor: The fake WatchLogs returns Unimplemented, or Unavailable once the client is closed, but neither path has a test. The sibling GetLogs stub has both cases covered in fake/sandbox_test.go.
Why this matters: If this stub regressed to returning (nil, nil) or skipped the closedFunc() check, everything would still compile and pass. Callers built on the fake would then dereference a nil WatchInterface, or see Unimplemented where they expect Unavailable.
Suggested fix: Add TestSandbox_WatchLogs_ReturnsUnimplemented and TestSandbox_WatchLogs_ClosedReturnsUnavailable, modeled on the GetLogs pair (fake/sandbox_test.go:994-1006), asserting a nil watcher and the matching Is* predicate.
Source: test-quality agent
| WaitReady(ctx context.Context, workspace, name string, opts ...WaitOptions) (*Sandbox, error) | ||
| WaitStopped(ctx context.Context, workspace, name string, opts ...WaitOptions) (*Sandbox, error) | ||
| Watch(ctx context.Context, workspace, name string, opts ...WatchOptions) (WatchInterface[*Sandbox], error) | ||
| WatchLogs(ctx context.Context, workspace, name string, opts ...WatchLogsOptions) (WatchInterface[*WatchLogEvent], error) |
There was a problem hiding this comment.
Minor: WatchLogs goes straight into the exported SandboxInterface. Further down this file, SandboxTemplateCreateInterface and SandboxSuspendResumeInterface were added as separate optional interfaces whose doc comments say they are additive "without widening SandboxInterface", and the README promises the original interface stays unchanged for existing mocks and wrappers.
Why this matters: Any downstream type that implements v1.SandboxInterface (hand-written mocks, wrappers, decorators in operator test suites) stops compiling on upgrade. This PR already had to patch three in-repo stubs (stubSandboxResolver, mockSandboxResolver, flippableResolver), and the description doesn't mention the break.
Suggested fix: Either move WatchLogs into an optional interface such as SandboxWatchLogsInterface, implemented by sandboxClient and the fake, or call out the source-incompatible change in the PR description and the README migration section.
Source: goal-alignment agent
| // testHookWatchLogsSleep, when set by a test, observes each requested backoff | ||
| // delay. It never waits: watcher.sleep performs the only real, cancellable | ||
| // wait. It is nil in production. | ||
| var testHookWatchLogsSleep func(time.Duration) |
There was a problem hiding this comment.
Minor: testHookWatchLogsSleep is a package-level mutable function variable in production code, used as a test seam. Nothing else under openshell/ does this.
Why this matters: retryWatchLogs reads the global from every watcher goroutine, while the backoff tests assign and restore it without synchronization. That rules out t.Parallel for the whole package and ships a test concern in the library.
Suggested fix: Make the backoff observer (or a sleep func) a field on sandboxClient, or an unexported parameter of runWatchLogs/retryWatchLogs, so each test sets it on its own client and no global state is shared.
Source: architecture agent
| // | ||
| // The returned watcher's channel closes when the server ends the stream, when | ||
| // the context is cancelled, or after Stop. Call Stop to release the stream. | ||
| func (s *sandboxClient) WatchLogs(ctx context.Context, workspace, name string, opts ...WatchLogsOptions) (WatchInterface[*WatchLogEvent], error) { |
There was a problem hiding this comment.
Notable: The PR description says this is ported from NVIDIA/OpenShell#3662 but doesn't say who needs resumable log and event watching in this SDK. The fit itself looks fine (the core proto already has resume_after_cursor with OUT_OF_RANGE semantics, and the README lists watch primitives and streaming logs), so this is about the record, not the change.
Why this matters: Without a use case it's hard to judge whether the reconnect loop and backoff are worth their complexity compared with GetLogs or the existing Watch.
Suggested fix: Copy one concrete example from the upstream PR into the description, for instance which consumer needs loss-aware resume across reconnects and what breaks today without it.
Source: goal-alignment agent
|
|
||
| reconnect := false | ||
| for { | ||
| ev, recvErr := stream.Recv() |
There was a problem hiding this comment.
Minor: The transparent reconnect relies on stream.Recv() returning Unavailable when the connection dies, but NewConnection in internal/grpc/conn.go sets no gRPC keepalive. On a half-open connection Recv just blocks.
Why this matters: A quiet sandbox (only FollowLogs, nothing logged for minutes) behind a load balancer or NAT with an idle timeout, or a gateway node lost without a FIN/RST, leaves the TCP connection half-open. No error reaches the HTTP/2 transport, so the resume path at lines 119-128 never runs and the caller sees a silent channel until its context ends. That's exactly the long-lived stream case the "reconnects transparently" contract is meant for.
Suggested fix: Configure client keepalive in NewConnection, for example grpc.WithKeepaliveParams(keepalive.ClientParameters{Time: 30 * time.Second, Timeout: 10 * time.Second, PermitWithoutStream: false}) aligned with the gateway's enforcement policy, or expose it as a client option, so a dead transport surfaces as Unavailable and triggers the resume loop.
Source: production agent
| assert.Equal(t, 1, attempts, "OUT_OF_RANGE must not be retried") | ||
| } | ||
|
|
||
| func TestWatchLogs_NonRetryableErrorIsTerminal(t *testing.T) { |
There was a problem hiding this comment.
Minor: Nothing tests ResourceExhausted, the one code where the SDK classification (ErrorUnavailable) and the retry decision (GRPCCode == codes.Unavailable) disagree. TestWatchLogs_NonRetryableErrorIsTerminal only uses PermissionDenied.
Why this matters: The GRPCCode check in isRetryableWatchLogsError is deliberate and correct, but a plausible refactor to types.IsUnavailable(err) would make a rate-limited gateway get redialed forever while this test keeps passing.
Suggested fix: Make the test table-driven over non-retryable codes and include codes.ResourceExhausted, asserting one EventError with GRPCCode == ResourceExhausted and len(mock.watchRequests) == 1.
Source: test-quality agent
| assert.Equal(t, watchLogsInitialBackoff, recordedBackoffs[1]) | ||
| } | ||
|
|
||
| func TestWatchLogs_BackoffGrowsWhenOnlySnapshotsArrive(t *testing.T) { |
There was a problem hiding this comment.
Minor: The 2s backoff cap (min(*backoff*2, watchLogsMaxBackoff)) is never reached in tests. TestWatchLogs_BackoffGrowsWhenOnlySnapshotsArrive records 100/200/400ms, and StopInterruptsBackoff stops at the first delay of 800ms or more.
Why this matters: Dropping or mis-setting the cap would pass every test, and a gateway that stays down would then back off without bound (1.6s, 3.2s, 6.4s and so on). The tests can't cheaply reach the cap because the hook only observes and w.sleep waits in real time.
Suggested fix: Let the test seam replace the wait (or inject a sleep func on the watcher) so a test can drive about six retries instantly and assert that the sequence ends at watchLogsMaxBackoff and stays there.
Source: test-quality agent
| defer w.Stop() | ||
|
|
||
| cancel() | ||
| drainWatchLogs(t, w) |
There was a problem hiding this comment.
Minor: TestWatchLogs_ContextCancelClosesChannel discards what drainWatchLogs returns, so it can't catch a spurious EventError on cancellation. TestWatchLogs_StopClosesChannel has the same gap.
Why this matters: After cancel(), stream.Recv returns a Canceled status, and only the ctx.Err() != nil guard in retryWatchLogs keeps a terminal EventError from being emitted. If that guard went away, this test would still pass.
Suggested fix: Check the drained events, for example events := drainWatchLogs(t, w); for _, ev := range events { assert.NotEqual(t, EventError, ev.Type) }, or simply assert.Empty(t, events) since the mock sends nothing in this test.
Source: test-quality agent
| ErrorUnimplemented | ||
| ErrorConflict | ||
| ErrorUnauthenticated | ||
| // ErrorOutOfRange reports unrecoverable loss on a resumable stream: the |
There was a problem hiding this comment.
Minor: ErrorOutOfRange and IsOutOfRange are documented as meaning a stream resume cursor is gone, but converter.grpcToSDK now maps gRPC OUT_OF_RANGE to it for every RPC (files, exec, policies and so on).
Why this matters: Any other RPC that returns OUT_OF_RANGE (the standard code for reads or seeks past a valid range) will surface with a documented meaning of "events after a resume cursor are lost". Callers that matched IsInvalidArgument for it before will silently stop matching, and the PR doesn't call out that remap.
Suggested fix: Describe ErrorOutOfRange generically, mirroring gRPC OUT_OF_RANGE, and keep the resume-cursor meaning in the WatchLogs and WatchLogsOptions.ResumeAfterCursor docs. Also note the InvalidArgument to OutOfRange remap in docs/src/error-handling.md.
Source: architecture agent
| } | ||
| } | ||
|
|
||
| // emit delivers an event unless the watcher was stopped or the context ended |
There was a problem hiding this comment.
Notable: The new watcher.emit/sleep/stopped helpers centralize the send-or-stop select, but the existing Watch goroutine in sandbox_client.go (lines 440-444 and 454-462) still hand-rolls the same selects. The two already differ: emit honors ctx.Done(), the inline selects don't.
Why this matters: With two delivery patterns on the same watcher type, future fixes to delivery semantics have to be made twice.
Suggested fix: Optionally switch the existing Watch goroutine to emit/stopped in a follow-up. It touches unchanged code, so it's fine to leave it out of this PR.
Source: architecture agent
Summary
Add
WatchLogsto the Go SDK, a loss-aware, resumable log and platform event stream helper that mirrors the Rust SDK's reconnect, backoff, and cursor-resume semantics. Recoverable loss surfaces asWatchLogKindWarningevents; unrecoverable loss (trimmed cursor) terminates withErrorOutOfRange.Related Issue
Refs NVIDIA/OpenShell#3055 (partial, item B of NVIDIA/OpenShell#3209 TODO).
Changes
SandboxInterface.WatchLogs(ctx, workspace, name, opts...)returnsWatchInterface[*WatchLogEvent]with transparent reconnect onUnavailable, capped exponential backoff (100ms→2s) reset on each delivered event, and cursor-aware replay from the highest observed cursor.PlatformEvent,WatchLogEvent{Kind, Log, Event, Warning, Cursor},WatchLogKind{Log,Event,Warning},WatchLogsOptions{FollowLogs, FollowEvents, LogSources, LogMinLevel, ResumeAfterCursor, LogTailLines, EventTail}.ErrorOutOfRangeto distinguish terminal gaps (cursor trimmed/from dead cursor space) from invalid arguments. RemappedgRPC OutOfRangefromErrorInvalidArgument→ErrorOutOfRange.PlatformEventFromProto,WatchLogEventFromProtowith deep-copy semantics; returns nil for non-resumable payloads (status snapshots, draft policy updates).ErrorUnimplemented(matchingGetLogs, no replayable buffer contract).OUT_OF_RANGEterminal (no retry), backoff reset, Stop/context cancellation, and error cases. 8 converter tests + error mapping + code string tests.WatchLogssection indocs/src/api/sandboxes.mdwith usage examples, loss semantics, and cursor contract. Error-handling doc updated withIsOutOfRangepredicate.Testing
mise run pre-commitpassesChecklist
Port note
Ported from NVIDIA/OpenShell#3662 by @letv1nnn after the Go SDK moved out of NVIDIA/OpenShell at 8ed8bf0c907fd06572d616d64f22ede915d0c10f. The commits are the original ones from the core PR at
6186bba777cb6b9af835441a26f6ea292d8f0a87, with the same authors and sign-offs; the only change is the module path (github.com/NVIDIA/OpenShell/sdk/gois nowgithub.com/ai-openshell/sdk-go). The core PR changed onlysdk/go, so nothing stays in core.The two follow-up commits came after the description above was written. The Testing section refers to core's tasks.