Skip to content

Add active sandbox timeout lifecycle management - #12

Open
arthur-zhang wants to merge 4 commits into
mainfrom
codex/docker-fuse
Open

Add active sandbox timeout lifecycle management#12
arthur-zhang wants to merge 4 commits into
mainfrom
codex/docker-fuse

Conversation

@arthur-zhang

@arthur-zhang arthur-zhang commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add active EndAt deadline timers with startup restoration, stale-callback validation, retry handling, and graceful shutdown
  • serialize pause, resume, connect, timeout, refresh, delete, reconciliation, and expiry with per-sandbox lifecycle locks
  • align raw REST timeout behavior with the 15-second API default while preserving SDK-provided timeouts
  • preserve tri-state internet-access policy and optional alias values across runtime restoration and API responses

Why

Sandbox expiry previously depended on request-time reconciliation, and lifecycle operations could race with expiry or runtime inspection. This change makes EndAt the source of truth, actively schedules deletion, and revalidates state under a per-sandbox lock before touching the runtime.

Impact

Running sandboxes are actively deleted when their deadline is reached. Paused sandboxes have their timers cancelled, resume establishes a fresh deadline, and connect/refresh never shorten an existing deadline. Slow runtime operations block only the same sandbox. Failed expiry deletion keeps the store record and retries after five seconds.

Validation

  • go test ./...
  • targeted deadline and lifecycle tests
  • go vet ./internal/gateway
  • gofmt
  • git diff --check

Summary by cubic

Adds active sandbox EndAt timers with startup restore and per‑sandbox lifecycle locks to expire sandboxes reliably without revival, and defers cleanup so requests don’t block. Also hardens the restart script to stop only the correct binary and wait until the new server is listening.

  • New Features

    • Active expiry with timers rebuilt on start; deletion runs in background with a 30s runtime delete timeout and 5s retry; shutdown waits for in‑flight deletions.
    • Per‑sandbox lifecycle locks serialize pause/resume/connect/timeout/refresh/delete/reconcile/expiry; slow I/O blocks only that sandbox.
    • Timeout semantics: default 15s when omitted; connect requires timeout; negative timeouts rejected; resume/connect/refresh extend without shortening; set‑timeout replaces earlier deadlines; extensions start counting after reconciliation.
    • Restore/API: default EndAt is derived from CreatedAt when missing; internet access uses a tri‑state policy that round‑trips; alias is optional in responses.
    • Script: scripts/restart-e2b-local-server.sh stops only listeners matching the managed binary and port and waits until the new process is listening.
  • Bug Fixes

    • Expired sandboxes cannot be revived by connect/refresh/timeout; requests return 404 immediately and schedule cleanup; failed runtime deletion keeps the record and retries.
    • Reconcile removes mappings for missing containers with best‑effort runtime cleanup and updates state via inspection; mappings are removed even if runtime delete fails, and lifecycle locks are released on failures.
    • Store clones record references and adds per‑sandbox locking; new ExtendEndAt avoids shortening existing deadlines.

Written for commit 62a6f03. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features
    • Sandboxes now expire via an in-memory deadline manager, with deadlines restored after restarts.
  • Bug Fixes
    • Timeout handling tightened: connect/create/refresh now require valid timeout, reject missing/negative values, and extend/refresh expiration consistently.
    • Expired-sandbox lifecycle operations no longer revive sandboxes; runtime deletion/cleanup failures no longer corrupt stored state.
    • Restored sandbox internet access is preserved consistently (via internet access policy).
  • Documentation
    • Updated English/Chinese docs detailing REST vs SDK timeout defaults and fallback behavior.
  • Chores
    • Ignore macOS .DS_Store files; improved local server restart script readiness checks.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The gateway now uses shared sandbox timeout defaults, explicit internet access policies, per-sandbox lifecycle locks, cloned store records, centralized deletion policies, and timer-based expiry restored at startup. Connect requests require timeout, and lifecycle tests cover concurrency, reconciliation, and deletion behavior.

Changes

Sandbox lifecycle and record model

Layer / File(s) Summary
Record contracts and timeout defaults
internal/gateway/types.go, internal/gateway/sandbox_timeout.go, internal/gateway/gateway_api.go, internal/backends/*, README*
Sandbox records use InternetAccessPolicy and string aliases, while timeout and response fallbacks consistently use the 15-second REST default.
Store lifecycle locking and cloning
internal/gateway/store.go, internal/gateway/store*_test.go
The store uses per-sandbox lifecycle locks, clone-safe records, and atomic state and deadline updates.
Deadline scheduling and deletion
internal/gateway/sandbox_deadlines.go, internal/gateway/sandbox_deletion.go, internal/gateway/server.go, internal/gateway/sandbox_deadlines_test.go
Sandbox deadlines are scheduled, restored, canceled, retried, and shut down through an in-memory timer manager with required or best-effort runtime deletion policies.
Gateway lifecycle operations and validation
internal/gateway/gateway_callbacks.go, internal/gateway/server_test.go, internal/backends/docker/runtime_integration_test.go
Creation, reconciliation, pause/resume, connect, timeout, refresh, and kill flows use lifecycle locking and deadline synchronization; invalid timeout inputs and missing connect timeouts are rejected.
Local restart verification
scripts/restart-e2b-local-server.sh, .gitignore
The restart script validates ports, replaces matching listeners, atomically swaps binaries, and verifies readiness; macOS metadata files are ignored.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 1.47% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: active sandbox timeout lifecycle management.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/docker-fuse

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@arthur-zhang
arthur-zhang marked this pull request as ready for review July 30, 2026 01:41

@duckpr duckpr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DuckPR 审阅器: ocr
模型: anthropic/glm-5.2

OpenCodeReview: 没有生成审阅意见。

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces active sandbox expiration management via a new sandboxDeadlineManager and refactors sandbox lifecycle locking to prevent race conditions during concurrent operations. It also aligns default timeouts with E2B REST API standards (15 seconds) and replaces the boolean internet access flag with a structured InternetAccessPolicy enum. Feedback on these changes highlights a potential resource leak in the Shutdown method of the gateway server, where a failure in shutting down deadlines prematurely aborts the shutdown of active template builds.

Comment on lines +124 to +130
if err := a.deadlines.shutdown(ctx); err != nil {
return err
}
if a.builds != nil {
return a.builds.shutdown(ctx)
}
return a.builds.shutdown(ctx)
return nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If a.deadlines.shutdown(ctx) returns an error (for example, due to a context timeout), the shutdown of a.builds is skipped entirely. This can lead to leaked resources or incomplete cleanup of active template builds during gateway shutdown. Both shutdown procedures should be attempted, returning the first error encountered.

Suggested change
if err := a.deadlines.shutdown(ctx); err != nil {
return err
}
if a.builds != nil {
return a.builds.shutdown(ctx)
}
return a.builds.shutdown(ctx)
return nil
err := a.deadlines.shutdown(ctx)
if a.builds != nil {
if buildErr := a.builds.shutdown(ctx); buildErr != nil && err == nil {
err = buildErr
}
}
return err

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (2)
internal/gateway/sandbox_deadlines_test.go (1)

132-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Tight wall-clock windows will be flaky on loaded CI.

20–60 ms deadlines and 120 ms negative-assertion windows are easy to violate on a contended runner (TestSetTimeoutReplacesEarlierDeadline in particular fails if the old 40 ms timer fires late but before the reschedule lands). Consider widening the scheduled offsets to a few hundred milliseconds, or gating these on testing.Short().

Also applies to: 169-185, 295-300, 373-378

🤖 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 `@internal/gateway/sandbox_deadlines_test.go` around lines 132 - 140, Widen the
short sandbox deadline and negative-assertion timing windows in the affected
tests, including TestSetTimeoutReplacesEarlierDeadline and the scenarios around
setSandboxEndAtForTest, to several hundred milliseconds so loaded CI has
sufficient scheduling margin. Preserve each test’s existing expiry and
non-expiry assertions while updating related timeout values consistently.
internal/gateway/store.go (1)

282-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

clone() is shallow for nested reference fields beyond the three cloned ones.

maps.Clone/slices.Clone cover Metadata, VolumeMounts, PublishedPorts, but any future map/slice/pointer added to SandboxRecord/SandboxRuntimeInfo will silently alias stored state again. Consider adding a short doc comment on clone() stating that new reference fields must be added here, since store isolation now depends entirely on it.

Also applies to: 343-346

🤖 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 `@internal/gateway/store.go` around lines 282 - 306, Add a concise doc comment
to SandboxRecord.clone stating that every reference-type field, including newly
added nested maps, slices, or pointers in SandboxRecord and SandboxRuntimeInfo,
must be explicitly cloned there to preserve store isolation. Do not change the
existing clone behavior.
🤖 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 `@internal/backends/docker/runtime.go`:
- Line 514: Update the endAt fallback in the Docker runtime restoration flow to
use the same future deadline behavior as the corresponding fallback in the
gateway server, rather than basing it on createdAt. Preserve the labeled end_at
value when present and reuse the established timeout/deadline calculation used
by the gateway implementation.

In `@internal/gateway/sandbox_deadlines_test.go`:
- Line 112: Update the three httptest.NewRequest call sites in the sandbox
deadline tests to use t.Context() via httptest.NewRequestWithContext or
req.WithContext, preserving the existing HTTP method, URL, and request behavior
while satisfying the noctx lint rule.

In `@internal/gateway/sandbox_deadlines.go`:
- Around line 67-102: Update the timer callback created in the sandbox deadline
scheduling flow to remove its sandboxID entry from m.timers when it fires, while
preserving any newer replacement timer. Also adjust the deadline reuse logic
around syncSandboxDeadline so an equal fireAt is reused only when the deadline
has not actually passed; otherwise rearm the timer, ensuring an early
expireSandboxLocked return cannot strand the sandbox.

In `@internal/gateway/server.go`:
- Around line 203-204: The restore fallback in internal/gateway/server.go at
lines 203-204 must derive the missing EndAt from the current UTC restore time,
not record.CreatedAt, so restored sandboxes receive a forward-looking deadline
before restoreSandboxDeadlines runs. Apply the same forward-looking fallback at
internal/backends/docker/runtime.go line 514 for containers missing
dockerLocalSandboxEndAtLabel, or leave EndAt zero there so the gateway supplies
the fallback.

---

Nitpick comments:
In `@internal/gateway/sandbox_deadlines_test.go`:
- Around line 132-140: Widen the short sandbox deadline and negative-assertion
timing windows in the affected tests, including
TestSetTimeoutReplacesEarlierDeadline and the scenarios around
setSandboxEndAtForTest, to several hundred milliseconds so loaded CI has
sufficient scheduling margin. Preserve each test’s existing expiry and
non-expiry assertions while updating related timeout values consistently.

In `@internal/gateway/store.go`:
- Around line 282-306: Add a concise doc comment to SandboxRecord.clone stating
that every reference-type field, including newly added nested maps, slices, or
pointers in SandboxRecord and SandboxRuntimeInfo, must be explicitly cloned
there to preserve store isolation. Do not change the existing clone behavior.
🪄 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 Plus

Run ID: 5283dbb5-e416-4473-808a-b35bcb61b177

📥 Commits

Reviewing files that changed from the base of the PR and between d8543bb and a6be9b6.

📒 Files selected for processing (22)
  • .gitignore
  • README.md
  • README.zh-CN.md
  • internal/backends/applecontainer/runtime_darwin.go
  • internal/backends/applecontainer/runtime_darwin_test.go
  • internal/backends/docker/runtime.go
  • internal/backends/docker/runtime_integration_test.go
  • internal/backends/orbstack/runtime.go
  • internal/gateway/gateway_api.go
  • internal/gateway/gateway_callbacks.go
  • internal/gateway/sandbox_deadlines.go
  • internal/gateway/sandbox_deadlines_test.go
  • internal/gateway/sandbox_deletion.go
  • internal/gateway/sandbox_timeout.go
  • internal/gateway/sandbox_timeout_test.go
  • internal/gateway/server.go
  • internal/gateway/server_test.go
  • internal/gateway/store.go
  • internal/gateway/store_locks_test.go
  • internal/gateway/store_test.go
  • internal/gateway/types.go
  • internal/gateway/types_test.go


createdAt := dockerTimeLabel(labels[dockerLocalSandboxCreatedAtLabel], dockerImageCreatedAt(inspect.Created, now))
endAt := dockerTimeLabel(labels[dockerLocalSandboxEndAtLabel], now.Add(time.Duration(defaultSandboxTimeoutSeconds)*time.Second))
endAt := dockerTimeLabel(labels[dockerLocalSandboxEndAtLabel], createdAt.Add(time.Duration(gateway.DefaultSandboxTimeoutSeconds)*time.Second))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Missing end_at label yields an already-past deadline.

Restored containers lacking dockerLocalSandboxEndAtLabel now get createdAt + 15s, which is in the past for any container older than 15 seconds; startup deadline restoration will then immediately expire and delete them. Same root cause as the fallback in internal/gateway/server.go Line 204.

🤖 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 `@internal/backends/docker/runtime.go` at line 514, Update the endAt fallback
in the Docker runtime restoration flow to use the same future deadline behavior
as the corresponding fallback in the gateway server, rather than basing it on
createdAt. Preserve the labeled end_at value when present and reuse the
established timeout/deadline calculation used by the gateway implementation.

t.Fatal("expected created sandbox deadline")
}

req := httptest.NewRequest(http.MethodGet, "/sandboxes/"+created.SandboxID, nil)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

noctx lint failures on httptest.NewRequest.

golangci-lint flags these three call sites; use httptest.NewRequestWithContext(t.Context(), ...) (or req.WithContext) to keep the linter green, or exclude noctx for _test.go in the lint config if the rule isn't intended for tests.

Also applies to: 170-170, 201-201

🧰 Tools
🪛 golangci-lint (2.12.2)

[error] 112-112: net/http/httptest.NewRequest must not be called. use net/http/httptest.NewRequestWithContext

(noctx)

🤖 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 `@internal/gateway/sandbox_deadlines_test.go` at line 112, Update the three
httptest.NewRequest call sites in the sandbox deadline tests to use t.Context()
via httptest.NewRequestWithContext or req.WithContext, preserving the existing
HTTP method, URL, and request behavior while satisfying the noctx lint rule.

Source: Linters/SAST tools

Comment on lines +67 to +102
if deadline, exists := m.timers[sandboxID]; exists {
// 截止时间没有变化时复用原定时器,避免检查状态时反复重建 timer。
if deadline.fireAt.Equal(fireAt) {
return
}
// 截止时间已经变化,停止旧定时器。即使旧回调已经启动,
// expire 也会通过 expectedEndAt 的二次校验拒绝过期结果。
deadline.timer.Stop()
}

delay := time.Until(fireAt)
if delay < 0 {
// 对已经到期的沙箱立即安排处理,而不是创建负延迟定时器。
delay = 0
}
timer := time.AfterFunc(delay, func() {
// 在加入 WaitGroup 前与 shutdown 串行,保证 shutdown 不会在
// Wait 返回后又看到一个新启动的回调。
m.mu.Lock()
if m.stopped {
m.mu.Unlock()
return
}
m.wg.Add(1)
m.mu.Unlock()

// expire 可能执行较慢的 runtime 删除,不能占用 manager 锁,
// 否则其他沙箱无法更新或取消自己的定时器。
defer m.wg.Done()
expire()
})
// 保存当前有效的触发时间,后续调用据此判断是复用还是替换 timer。
m.timers[sandboxID] = sandboxDeadline{
timer: timer,
fireAt: fireAt,
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

A fired timer stays in timers as if still armed, so a "too early" expiry can strand the sandbox.

time.AfterFunc entries are never removed from m.timers when they fire; only cancel/shutdown/replacement remove them. If expireSandboxLocked returns via the time.Now().UTC().Before(record.EndAt) guard (wall-clock vs. monotonic skew, or a coarse clock at the boundary), no new timer is armed, yet a later syncSandboxDeadline with the same EndAt hits the deadline.fireAt.Equal(fireAt) fast path at Line 69 and returns without rearming — the sandbox then never expires.

Two low-cost mitigations: drop the map entry when the callback runs, and rearm when the deadline hasn't actually passed.

♻️ Suggested fix
 	timer := time.AfterFunc(delay, func() {
 		m.mu.Lock()
 		if m.stopped {
 			m.mu.Unlock()
 			return
 		}
+		// The timer has fired and will not fire again; drop it so schedule()
+		// does not treat this sandbox as still armed.
+		if deadline, exists := m.timers[sandboxID]; exists && deadline.fireAt.Equal(fireAt) {
+			delete(m.timers, sandboxID)
+		}
 		m.wg.Add(1)
 		m.mu.Unlock()
 	if record.State != string(e2bapi.Running) ||
-		!record.EndAt.Equal(expectedEndAt) ||
-		time.Now().UTC().Before(record.EndAt) {
+		!record.EndAt.Equal(expectedEndAt) {
 		return
 	}
+	if time.Now().UTC().Before(record.EndAt) {
+		// Woke up before the deadline actually passed; rearm instead of dropping it.
+		a.syncSandboxDeadline(record)
+		return
+	}

Also applies to: 251-257

🤖 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 `@internal/gateway/sandbox_deadlines.go` around lines 67 - 102, Update the
timer callback created in the sandbox deadline scheduling flow to remove its
sandboxID entry from m.timers when it fires, while preserving any newer
replacement timer. Also adjust the deadline reuse logic around
syncSandboxDeadline so an equal fireAt is reused only when the deadline has not
actually passed; otherwise rearm the timer, ensuring an early
expireSandboxLocked return cannot strand the sandbox.

Comment on lines 203 to +204
if record.EndAt.IsZero() {
record.EndAt = time.Now().UTC().Add(time.Duration(defaultSandboxTimeoutSeconds) * time.Second)
record.EndAt = defaultSandboxEndAt(record.CreatedAt)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Default EndAt anchored to CreatedAt makes restored sandboxes expire immediately. Both restore paths derive the missing deadline from the sandbox's creation time, so any sandbox older than 15 seconds comes back already past its deadline; with the new active expiry timers it is deleted moments after startup.

  • internal/gateway/server.go#L203-L204: base the restore fallback on restore time (defaultSandboxEndAt(time.Now().UTC())) or otherwise guarantee a forward-looking deadline before restoreSandboxDeadlines runs.
  • internal/backends/docker/runtime.go#L514-L514: apply the same forward-looking fallback for containers with no dockerLocalSandboxEndAtLabel, or leave EndAt zero and let the gateway decide.
📍 Affects 2 files
  • internal/gateway/server.go#L203-L204 (this comment)
  • internal/backends/docker/runtime.go#L514-L514
🤖 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 `@internal/gateway/server.go` around lines 203 - 204, The restore fallback in
internal/gateway/server.go at lines 203-204 must derive the missing EndAt from
the current UTC restore time, not record.CreatedAt, so restored sandboxes
receive a forward-looking deadline before restoreSandboxDeadlines runs. Apply
the same forward-looking fallback at internal/backends/docker/runtime.go line
514 for containers missing dockerLocalSandboxEndAtLabel, or leave EndAt zero
there so the gateway supplies the fallback.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 22 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/gateway/sandbox_deadlines.go">

<violation number="1" location="internal/gateway/sandbox_deadlines.go:293">
P2: A failed expiry deletion during startup is retried immediately rather than after the intended five-second backoff. Preserve an already-scheduled retry when restoring deadlines so an unavailable runtime is not hit twice in rapid succession.</violation>
</file>

<file name="internal/backends/docker/runtime.go">

<violation number="1" location="internal/backends/docker/runtime.go:514">
P1: On gateway restart, running Docker sandboxes without an explicit EndAt label will have their deadline set in the past instead of receiving a fresh grace period. The fallback EndAt was changed from `now + 15s` to `createdAt + 15s` on line 514 of `runtime.go`. For any sandbox older than 15 seconds, this produces an EndAt timestamp already in the past, which causes the deadline manager to immediately treat it as expired and schedule deletion. The `enrichRestoredSandboxRecord` guard in server.go only checks for a zero EndAt, not a past one, so it doesn't catch this case. Consider using `time.Now().UTC()` (or the existing `now` variable) instead of `createdAt` for the EndAt fallback, matching the previous behavior that granted at least 15 seconds of life on restart.</violation>
</file>

<file name="internal/backends/docker/runtime_integration_test.go">

<violation number="1" location="internal/backends/docker/runtime_integration_test.go:288">
P1: Test expects 200 but connect handler now returns 201 when resuming a paused sandbox. Change assertion to accept both 200 and 201, or specifically expect 201 since this test pauses before connecting.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

}

connectReq := httptest.NewRequest(http.MethodPost, "/sandboxes/"+sandbox.SandboxID+"/connect", bytes.NewBufferString(`{}`))
connectReq := httptest.NewRequest(http.MethodPost, "/sandboxes/"+sandbox.SandboxID+"/connect", bytes.NewBufferString(`{"timeout":300}`))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Test expects 200 but connect handler now returns 201 when resuming a paused sandbox. Change assertion to accept both 200 and 201, or specifically expect 201 since this test pauses before connecting.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/backends/docker/runtime_integration_test.go, line 288:

<comment>Test expects 200 but connect handler now returns 201 when resuming a paused sandbox. Change assertion to accept both 200 and 201, or specifically expect 201 since this test pauses before connecting.</comment>

<file context>
@@ -285,7 +285,7 @@ func TestDockerRuntimeGatewayCreatePauseConnectDelete(t *testing.T) {
 	}
 
-	connectReq := httptest.NewRequest(http.MethodPost, "/sandboxes/"+sandbox.SandboxID+"/connect", bytes.NewBufferString(`{}`))
+	connectReq := httptest.NewRequest(http.MethodPost, "/sandboxes/"+sandbox.SandboxID+"/connect", bytes.NewBufferString(`{"timeout":300}`))
 	connectReq = connectReq.WithContext(ctx)
 	connectRec := httptest.NewRecorder()
</file context>

Comment thread internal/gateway/gateway_callbacks.go

createdAt := dockerTimeLabel(labels[dockerLocalSandboxCreatedAtLabel], dockerImageCreatedAt(inspect.Created, now))
endAt := dockerTimeLabel(labels[dockerLocalSandboxEndAtLabel], now.Add(time.Duration(defaultSandboxTimeoutSeconds)*time.Second))
endAt := dockerTimeLabel(labels[dockerLocalSandboxEndAtLabel], createdAt.Add(time.Duration(gateway.DefaultSandboxTimeoutSeconds)*time.Second))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: On gateway restart, running Docker sandboxes without an explicit EndAt label will have their deadline set in the past instead of receiving a fresh grace period. The fallback EndAt was changed from now + 15s to createdAt + 15s on line 514 of runtime.go. For any sandbox older than 15 seconds, this produces an EndAt timestamp already in the past, which causes the deadline manager to immediately treat it as expired and schedule deletion. The enrichRestoredSandboxRecord guard in server.go only checks for a zero EndAt, not a past one, so it doesn't catch this case. Consider using time.Now().UTC() (or the existing now variable) instead of createdAt for the EndAt fallback, matching the previous behavior that granted at least 15 seconds of life on restart.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/backends/docker/runtime.go, line 514:

<comment>On gateway restart, running Docker sandboxes without an explicit EndAt label will have their deadline set in the past instead of receiving a fresh grace period. The fallback EndAt was changed from `now + 15s` to `createdAt + 15s` on line 514 of `runtime.go`. For any sandbox older than 15 seconds, this produces an EndAt timestamp already in the past, which causes the deadline manager to immediately treat it as expired and schedule deletion. The `enrichRestoredSandboxRecord` guard in server.go only checks for a zero EndAt, not a past one, so it doesn't catch this case. Consider using `time.Now().UTC()` (or the existing `now` variable) instead of `createdAt` for the EndAt fallback, matching the previous behavior that granted at least 15 seconds of life on restart.</comment>

<file context>
@@ -512,22 +511,22 @@ func (r *DockerRuntime) restoreSandboxRecord(ctx context.Context, summary docker
 
 	createdAt := dockerTimeLabel(labels[dockerLocalSandboxCreatedAtLabel], dockerImageCreatedAt(inspect.Created, now))
-	endAt := dockerTimeLabel(labels[dockerLocalSandboxEndAtLabel], now.Add(time.Duration(defaultSandboxTimeoutSeconds)*time.Second))
+	endAt := dockerTimeLabel(labels[dockerLocalSandboxEndAtLabel], createdAt.Add(time.Duration(gateway.DefaultSandboxTimeoutSeconds)*time.Second))
 	templateID := strings.TrimSpace(labels[dockerLocalSandboxTemplateIDLabel])
 	if templateID == "" {
</file context>


func (a *App) restoreSandboxDeadlines() {
for _, record := range a.store.List() {
a.syncSandboxDeadline(record)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: A failed expiry deletion during startup is retried immediately rather than after the intended five-second backoff. Preserve an already-scheduled retry when restoring deadlines so an unavailable runtime is not hit twice in rapid succession.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/gateway/sandbox_deadlines.go, line 293:

<comment>A failed expiry deletion during startup is retried immediately rather than after the intended five-second backoff. Preserve an already-scheduled retry when restoring deadlines so an unavailable runtime is not hit twice in rapid succession.</comment>

<file context>
@@ -0,0 +1,295 @@
+
+func (a *App) restoreSandboxDeadlines() {
+	for _, record := range a.store.List() {
+		a.syncSandboxDeadline(record)
+	}
+}
</file context>

@duckpr duckpr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DuckPR 审阅器: ocr
模型: anthropic/glm-5.2

OpenCodeReview: 没有生成审阅意见。

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@scripts/restart-e2b-local-server.sh`:
- Around line 137-172: Serialize the entire restart workflow using a per-binary
lock derived from BIN_PATH, acquiring it before the build into BUILD_PATH and
holding it through stopping processes, swapping binaries, starting the gateway,
readiness verification, and failure cleanup. Ensure the lock is released on
every success and failure path, including signals, so concurrent invocations
cannot share BUILD_PATH or interleave PID-file and process management.
🪄 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 Plus

Run ID: 2797856d-96ec-4724-af71-b9cfb213506d

📥 Commits

Reviewing files that changed from the base of the PR and between a6be9b6 and c0e42c2.

📒 Files selected for processing (3)
  • internal/gateway/gateway_callbacks.go
  • internal/gateway/sandbox_deadlines_test.go
  • scripts/restart-e2b-local-server.sh

Comment on lines +137 to +172
BUILD_PATH="${BIN_PATH}.new"

echo "building gateway binary"
(
cd "$ROOT_DIR"
go build -o "$BUILD_PATH" ./cmd/e2b-local
)

mv "$BUILD_PATH" "$BIN_PATH"
chmod +x "$BIN_PATH"
old_pids="$(listening_pids)"
matching_old_pids=""
if [[ -n "$old_pids" ]]; then
# A matching port alone is not sufficient because unrelated applications can
# listen on the same port through another local address. Select only processes
# whose executable path also matches the managed gateway binary.
for old_pid in $old_pids; do
old_executable="$(process_executable_path "$old_pid")"
if [[ "$old_executable" == "$EXPECTED_BIN_PATH" ]]; then
matching_old_pids+="${matching_old_pids:+ }$old_pid"
else
echo "ignoring unrelated listener pid=$old_pid port=$SERVER_PORT executable=${old_executable:-unknown}"
fi
done
fi

if [[ -f "$PID_FILE" ]]; then
old_pid="$(tr -d '[:space:]' < "$PID_FILE")"
if [[ -n "$old_pid" ]] && kill -0 "$old_pid" >/dev/null 2>&1; then
echo "stopping existing gateway pid=$old_pid"
if [[ -n "$matching_old_pids" ]]; then
for old_pid in $matching_old_pids; do
echo "stopping existing gateway pid=$old_pid port=$SERVER_PORT executable=$EXPECTED_BIN_PATH"
stop_process_tree "$old_pid"
else
echo "removing stale pid file"
fi
rm -f "$PID_FILE"
done
else
echo "no existing gateway from $EXPECTED_BIN_PATH is listening on port $SERVER_PORT"
fi

rm -f "$PID_FILE"
mv "$BUILD_PATH" "$BIN_PATH"
chmod +x "$BIN_PATH"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Serialize concurrent restart invocations.

Two invocations share ${BIN_PATH}.new and can interleave stop/swap/PID-file operations. One restart can kill the other’s newly started gateway or delete its PID file, causing avoidable downtime. Acquire a per-binary lock before building and release it only after readiness or failure cleanup completes.

🤖 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 `@scripts/restart-e2b-local-server.sh` around lines 137 - 172, Serialize the
entire restart workflow using a per-binary lock derived from BIN_PATH, acquiring
it before the build into BUILD_PATH and holding it through stopping processes,
swapping binaries, starting the gateway, readiness verification, and failure
cleanup. Ensure the lock is released on every success and failure path,
including signals, so concurrent invocations cannot share BUILD_PATH or
interleave PID-file and process management.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 3 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="scripts/restart-e2b-local-server.sh">

<violation number="1" location="scripts/restart-e2b-local-server.sh:19">
P2: The restart helper now has an undocumented hard dependency on `lsof` and exits before using the existing PID-file mechanism when it is absent. On a machine with Go and a valid gateway/runtime installation but no `lsof` package (common in minimal Linux environments), `scripts/restart-e2b-local-server.sh` becomes unusable solely because of this new check. Falling back to the PID file or another available listener-discovery tool, or documenting and provisioning `lsof` as a script prerequisite, would avoid making the helper unexpectedly unavailable.</violation>

<violation number="2" location="scripts/restart-e2b-local-server.sh:34">
P2: Valid YAML configurations using a flow-style server mapping cannot be restarted by this helper. For example, `server: {addr: "127.0.0.1:3000"}` is accepted by the gateway's `yaml.Unmarshal`-based `LoadConfig` (`internal/gateway/config.go:226-244`), but this matcher only recognizes a block-style `server:` line and the helper exits with “server.addr ... was not found” before starting anything. Parsing the same configuration structure as the gateway, or supporting the valid flow-style form rather than relying on this restricted text pattern, would keep the helper compatible with custom configs.</violation>

<violation number="3" location="scripts/restart-e2b-local-server.sh:145">
P1: A running gateway that is still initializing is no longer stopped during restart. This scan only finds TCP listeners, but `NewAppWithCallbacks` performs runtime restoration and reconciliation before `ListenAndServe` is called (`internal/gateway/server.go:77-83`, and `cmd/e2b-local/main.go:157-168`); if that work is still in progress, the PID in `PID_FILE` is alive but absent from `old_pids`. The script then removes the PID file and launches a second gateway, which can later race the original for the port and fail to restart. Retaining the PID-file candidate as a fallback (while verifying its executable path) would preserve restart behavior for processes that have not begun listening yet.</violation>

<violation number="4" location="scripts/restart-e2b-local-server.sh:153">
P2: A symlink configured through `E2B_LOCAL_BIN_PATH` can prevent the existing gateway from being recognized. `resolve_target_path` canonicalizes only the parent directory, so `EXPECTED_BIN_PATH` remains the symlink path, while `/proc/<pid>/exe` (used by `process_executable_path`) resolves the final symlink to its target. The old process is consequently classified as unrelated; the subsequent `mv` replaces the symlink and the new process cannot bind while the old target process is still listening. Comparing canonical executable paths, or resolving the final binary target consistently before both matching and building, would avoid this restart failure.</violation>

<violation number="5" location="scripts/restart-e2b-local-server.sh:171">
P2: This restart flow builds to a shared `${BIN_PATH}.new` path, stops the previously listening process, and swaps in the new binary — but nothing prevents two concurrent invocations of this script from interleaving those steps. One invocation could kill the gateway just started by another invocation or remove its PID file, causing avoidable downtime. Consider acquiring a per-binary lock (e.g., via flock) before building and releasing it only after the new process is confirmed listening or cleanup completes.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic


mv "$BUILD_PATH" "$BIN_PATH"
chmod +x "$BIN_PATH"
old_pids="$(listening_pids)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: A running gateway that is still initializing is no longer stopped during restart. This scan only finds TCP listeners, but NewAppWithCallbacks performs runtime restoration and reconciliation before ListenAndServe is called (internal/gateway/server.go:77-83, and cmd/e2b-local/main.go:157-168); if that work is still in progress, the PID in PID_FILE is alive but absent from old_pids. The script then removes the PID file and launches a second gateway, which can later race the original for the port and fail to restart. Retaining the PID-file candidate as a fallback (while verifying its executable path) would preserve restart behavior for processes that have not begun listening yet.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/restart-e2b-local-server.sh, line 145:

<comment>A running gateway that is still initializing is no longer stopped during restart. This scan only finds TCP listeners, but `NewAppWithCallbacks` performs runtime restoration and reconciliation before `ListenAndServe` is called (`internal/gateway/server.go:77-83`, and `cmd/e2b-local/main.go:157-168`); if that work is still in progress, the PID in `PID_FILE` is alive but absent from `old_pids`. The script then removes the PID file and launches a second gateway, which can later race the original for the port and fail to restart. Retaining the PID-file candidate as a fallback (while verifying its executable path) would preserve restart behavior for processes that have not begun listening yet.</comment>

<file context>
@@ -55,26 +118,59 @@ stop_process_tree() {
 
-mv "$BUILD_PATH" "$BIN_PATH"
-chmod +x "$BIN_PATH"
+old_pids="$(listening_pids)"
+matching_old_pids=""
+if [[ -n "$old_pids" ]]; then
</file context>

exit 1
fi

if ! command -v lsof >/dev/null 2>&1; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The restart helper now has an undocumented hard dependency on lsof and exits before using the existing PID-file mechanism when it is absent. On a machine with Go and a valid gateway/runtime installation but no lsof package (common in minimal Linux environments), scripts/restart-e2b-local-server.sh becomes unusable solely because of this new check. Falling back to the PID file or another available listener-discovery tool, or documenting and provisioning lsof as a script prerequisite, would avoid making the helper unexpectedly unavailable.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/restart-e2b-local-server.sh, line 19:

<comment>The restart helper now has an undocumented hard dependency on `lsof` and exits before using the existing PID-file mechanism when it is absent. On a machine with Go and a valid gateway/runtime installation but no `lsof` package (common in minimal Linux environments), `scripts/restart-e2b-local-server.sh` becomes unusable solely because of this new check. Falling back to the PID file or another available listener-discovery tool, or documenting and provisioning `lsof` as a script prerequisite, would avoid making the helper unexpectedly unavailable.</comment>

<file context>
@@ -9,17 +9,80 @@ BIN_PATH="${E2B_LOCAL_BIN_PATH:-$ROOT_DIR/e2b-local}"
   exit 1
 fi
 
+if ! command -v lsof >/dev/null 2>&1; then
+  echo "lsof is required but was not found in PATH" >&2
+  exit 1
</file context>

/^[[:space:]]*#/ {
next
}
/^[[:space:]]*server:[[:space:]]*(#.*)?$/ {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Valid YAML configurations using a flow-style server mapping cannot be restarted by this helper. For example, server: {addr: "127.0.0.1:3000"} is accepted by the gateway's yaml.Unmarshal-based LoadConfig (internal/gateway/config.go:226-244), but this matcher only recognizes a block-style server: line and the helper exits with “server.addr ... was not found” before starting anything. Parsing the same configuration structure as the gateway, or supporting the valid flow-style form rather than relying on this restricted text pattern, would keep the helper compatible with custom configs.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/restart-e2b-local-server.sh, line 34:

<comment>Valid YAML configurations using a flow-style server mapping cannot be restarted by this helper. For example, `server: {addr: "127.0.0.1:3000"}` is accepted by the gateway's `yaml.Unmarshal`-based `LoadConfig` (`internal/gateway/config.go:226-244`), but this matcher only recognizes a block-style `server:` line and the helper exits with “server.addr ... was not found” before starting anything. Parsing the same configuration structure as the gateway, or supporting the valid flow-style form rather than relying on this restricted text pattern, would keep the helper compatible with custom configs.</comment>

<file context>
@@ -9,17 +9,80 @@ BIN_PATH="${E2B_LOCAL_BIN_PATH:-$ROOT_DIR/e2b-local}"
+    /^[[:space:]]*#/ {
+      next
+    }
+    /^[[:space:]]*server:[[:space:]]*(#.*)?$/ {
+      in_server = 1
+      next
</file context>

# whose executable path also matches the managed gateway binary.
for old_pid in $old_pids; do
old_executable="$(process_executable_path "$old_pid")"
if [[ "$old_executable" == "$EXPECTED_BIN_PATH" ]]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: A symlink configured through E2B_LOCAL_BIN_PATH can prevent the existing gateway from being recognized. resolve_target_path canonicalizes only the parent directory, so EXPECTED_BIN_PATH remains the symlink path, while /proc/<pid>/exe (used by process_executable_path) resolves the final symlink to its target. The old process is consequently classified as unrelated; the subsequent mv replaces the symlink and the new process cannot bind while the old target process is still listening. Comparing canonical executable paths, or resolving the final binary target consistently before both matching and building, would avoid this restart failure.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/restart-e2b-local-server.sh, line 153:

<comment>A symlink configured through `E2B_LOCAL_BIN_PATH` can prevent the existing gateway from being recognized. `resolve_target_path` canonicalizes only the parent directory, so `EXPECTED_BIN_PATH` remains the symlink path, while `/proc/<pid>/exe` (used by `process_executable_path`) resolves the final symlink to its target. The old process is consequently classified as unrelated; the subsequent `mv` replaces the symlink and the new process cannot bind while the old target process is still listening. Comparing canonical executable paths, or resolving the final binary target consistently before both matching and building, would avoid this restart failure.</comment>

<file context>
@@ -55,26 +118,59 @@ stop_process_tree() {
+  # whose executable path also matches the managed gateway binary.
+  for old_pid in $old_pids; do
+    old_executable="$(process_executable_path "$old_pid")"
+    if [[ "$old_executable" == "$EXPECTED_BIN_PATH" ]]; then
+      matching_old_pids+="${matching_old_pids:+ }$old_pid"
+    else
</file context>

fi

rm -f "$PID_FILE"
mv "$BUILD_PATH" "$BIN_PATH"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: This restart flow builds to a shared ${BIN_PATH}.new path, stops the previously listening process, and swaps in the new binary — but nothing prevents two concurrent invocations of this script from interleaving those steps. One invocation could kill the gateway just started by another invocation or remove its PID file, causing avoidable downtime. Consider acquiring a per-binary lock (e.g., via flock) before building and releasing it only after the new process is confirmed listening or cleanup completes.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/restart-e2b-local-server.sh, line 171:

<comment>This restart flow builds to a shared `${BIN_PATH}.new` path, stops the previously listening process, and swaps in the new binary — but nothing prevents two concurrent invocations of this script from interleaving those steps. One invocation could kill the gateway just started by another invocation or remove its PID file, causing avoidable downtime. Consider acquiring a per-binary lock (e.g., via flock) before building and releasing it only after the new process is confirmed listening or cleanup completes.</comment>

<file context>
@@ -55,26 +118,59 @@ stop_process_tree() {
 fi
 
+rm -f "$PID_FILE"
+mv "$BUILD_PATH" "$BIN_PATH"
+chmod +x "$BIN_PATH"
+
</file context>

@duckpr duckpr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DuckPR 审阅器: ocr
模型: anthropic/glm-5.2

OpenCodeReview: 没有生成审阅意见。

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
internal/gateway/sandbox_deadlines_test.go (1)

30-35: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Non-atomic idempotent channel close in InspectSandbox.

select { case <-r.inspectStarted: default: close(r.inspectStarted) } is a check-then-close pattern that would panic on double-close if InspectSandbox is ever invoked concurrently on the same runtime instance (not currently exercised, since each subtest issues a single request, but fragile if reused/extended later).

🔧 Suggested fix using sync.Once
 type blockingInspectRuntime struct {
 	recordingRuntime
 	inspectStarted chan struct{}
 	releaseInspect chan struct{}
+	inspectOnce    sync.Once
 }

 func (r *blockingInspectRuntime) InspectSandbox(ctx context.Context, info SandboxRuntimeInfo) (SandboxRuntimeInspection, error) {
-	select {
-	case <-r.inspectStarted:
-	default:
-		close(r.inspectStarted)
-	}
+	r.inspectOnce.Do(func() { close(r.inspectStarted) })

Also applies to: 53-65

🤖 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 `@internal/gateway/sandbox_deadlines_test.go` around lines 30 - 35, Make the
inspect-start notification in InspectSandbox safe for concurrent invocation by
adding a sync.Once field to blockingInspectRuntime and using it to guard closing
inspectStarted. Preserve the existing channel signaling and release behavior
while ensuring the channel is closed at most once.
internal/gateway/gateway_callbacks.go (1)

852-859: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated "don't shorten deadline" invariant between resume and non-paused connect paths.

The paused branch manually recomputes and caps endAt against record.EndAt before calling store.SetStateRuntimeInfoAndEndAt, while the non-paused branch delegates the same invariant to store.ExtendEndAt. Duplicating a safety-critical calculation (preventing deadline shortening) across two code paths risks silent divergence if one path is edited without updating the other.

Consider adding a compound store helper (e.g. ExtendStateRuntimeInfoAndEndAt) that applies the same monotonic-max policy as ExtendEndAt internally, and use it from the resumed branch instead of manually recomputing the cap here.

♻️ Illustrative refactor sketch
-		endAt := time.Now().UTC().Add(time.Duration(req.Timeout) * time.Second)
-		if record.EndAt.After(endAt) {
-			endAt = record.EndAt
-		}
-		updated, ok, err := a.store.SetStateRuntimeInfoAndEndAt(sandboxID, string(e2bapi.Running), runtimeInfo, endAt)
+		candidateEndAt := time.Now().UTC().Add(time.Duration(req.Timeout) * time.Second)
+		updated, ok, err := a.store.ExtendStateRuntimeInfoAndEndAt(sandboxID, string(e2bapi.Running), runtimeInfo, candidateEndAt)

Since store.go (with ExtendEndAt/SetStateRuntimeInfoAndEndAt) isn't in this review batch, please confirm ExtendEndAt's actual semantics match the manual cap used here before consolidating.

Also applies to: 869-880

🤖 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 `@internal/gateway/gateway_callbacks.go` around lines 852 - 859, Consolidate
the resumed connection deadline update around the store layer: verify that
ExtendEndAt preserves the monotonic maximum against the existing record.EndAt,
add a compound helper alongside ExtendEndAt and SetStateRuntimeInfoAndEndAt if
needed, and use it from the paused/resumed branch instead of manually
calculating and capping endAt. Preserve the existing runtime-info update and
ensure deadlines are never shortened.
🤖 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 `@internal/gateway/sandbox_deadlines_test.go`:
- Around line 309-313: Replace each new httptest.NewRequest call in the sandbox
deadline tests with the context-aware request constructor, supplying an explicit
context such as context.Background(). Update the occurrences around the request
setup in the affected test functions while preserving their existing methods,
URLs, and bodies.

---

Nitpick comments:
In `@internal/gateway/gateway_callbacks.go`:
- Around line 852-859: Consolidate the resumed connection deadline update around
the store layer: verify that ExtendEndAt preserves the monotonic maximum against
the existing record.EndAt, add a compound helper alongside ExtendEndAt and
SetStateRuntimeInfoAndEndAt if needed, and use it from the paused/resumed branch
instead of manually calculating and capping endAt. Preserve the existing
runtime-info update and ensure deadlines are never shortened.

In `@internal/gateway/sandbox_deadlines_test.go`:
- Around line 30-35: Make the inspect-start notification in InspectSandbox safe
for concurrent invocation by adding a sync.Once field to blockingInspectRuntime
and using it to guard closing inspectStarted. Preserve the existing channel
signaling and release behavior while ensuring the channel is closed at most
once.
🪄 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 Plus

Run ID: c270f9b5-461e-441d-b55c-c01cf1fad834

📥 Commits

Reviewing files that changed from the base of the PR and between c0e42c2 and 62a6f03.

📒 Files selected for processing (2)
  • internal/gateway/gateway_callbacks.go
  • internal/gateway/sandbox_deadlines_test.go

Comment on lines +309 to +313
req := httptest.NewRequest(
http.MethodPost,
"/sandboxes/"+created.SandboxID+tt.path,
bytes.NewBufferString(tt.body),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

noctx lint pattern recurs in these newly added tests.

httptest.NewRequest without a context was previously flagged by golangci-lint's noctx rule on this file. The same pattern is reintroduced in the new test functions added here.

🧰 Suggested fix
-			req := httptest.NewRequest(
+			req := httptest.NewRequestWithContext(
+				t.Context(),
 				http.MethodPost,
 				"/sandboxes/"+created.SandboxID+tt.path,
 				bytes.NewBufferString(tt.body),
 			)

Also applies to: 374-378, 418-422, 470-470.

🤖 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 `@internal/gateway/sandbox_deadlines_test.go` around lines 309 - 313, Replace
each new httptest.NewRequest call in the sandbox deadline tests with the
context-aware request constructor, supplying an explicit context such as
context.Background(). Update the occurrences around the request setup in the
affected test functions while preserving their existing methods, URLs, and
bodies.

Source: Linters/SAST tools

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/gateway/sandbox_deadlines_test.go">

<violation number="1" location="internal/gateway/sandbox_deadlines_test.go:309">
P3: These new test cases call `httptest.NewRequest` without a context, which will keep tripping the `noctx` golangci-lint rule on this file. Consider using `httptest.NewRequestWithContext` (or building the request with `http.NewRequestWithContext`) to keep lint clean.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

created := createSandboxForTest(t, app, "base", http.StatusCreated)
setSandboxEndAtForTest(t, app, created.SandboxID, time.Now().UTC().Add(2*time.Second))

req := httptest.NewRequest(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: These new test cases call httptest.NewRequest without a context, which will keep tripping the noctx golangci-lint rule on this file. Consider using httptest.NewRequestWithContext (or building the request with http.NewRequestWithContext) to keep lint clean.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/gateway/sandbox_deadlines_test.go, line 309:

<comment>These new test cases call `httptest.NewRequest` without a context, which will keep tripping the `noctx` golangci-lint rule on this file. Consider using `httptest.NewRequestWithContext` (or building the request with `http.NewRequestWithContext`) to keep lint clean.</comment>

<file context>
@@ -236,6 +272,84 @@ func TestConnectExtendsRunningSandboxDeadline(t *testing.T) {
+			created := createSandboxForTest(t, app, "base", http.StatusCreated)
+			setSandboxEndAtForTest(t, app, created.SandboxID, time.Now().UTC().Add(2*time.Second))
+
+			req := httptest.NewRequest(
+				http.MethodPost,
+				"/sandboxes/"+created.SandboxID+tt.path,
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant