Skip to content

Resolve short IDs for sandbox exec, logs, and deploy cancel#769

Merged
phinze merged 3 commits into
mainfrom
phinze/mir-1002-short-ids-dont-resolve-for-runner-hosted-sandboxes-in-logs
Apr 17, 2026
Merged

Resolve short IDs for sandbox exec, logs, and deploy cancel#769
phinze merged 3 commits into
mainfrom
phinze/mir-1002-short-ids-dont-resolve-for-runner-hosted-sandboxes-in-logs

Conversation

@phinze
Copy link
Copy Markdown
Contributor

@phinze phinze commented Apr 17, 2026

Short IDs (those 3-8 char base58 suffixes we show in most output) weren't resolving in a handful of commands, which made them useless for the exact case they're most useful: typing a short ID you just saw in a log or status message. Worst offender was deploy cancel, which reported success but silently no-op'd because the write-back used the original unresolved short ID, leaving the deploy lock stuck until its 30-minute timeout.

First commit teaches EntityServer.Get's short ID fallback to handle kind-prefixed input like sandbox/3sA. The index stores bare suffixes, but the CLI often normalizes to a kind/id form before sending. That single change also fixes sandbox-pool set-desired and any other command that flows through the entity server with a prefixed short ID.

Second commit tackles the three commands called out in the issue. sandbox exec and logs sandbox never resolved IDs at all and needed explicit resolution added. deploy cancel was resolving on read but using the original string for the write; fixed by reassigning from the Get response.

Closes MIR-1002

phinze added 2 commits April 17, 2026 14:28
The short ID index stores bare base58 suffixes like `3sA`, but callers
often send kind-prefixed forms like `sandbox/3sA` — the CLI normalizes
sandbox IDs this way, and `sandbox-pool set-desired` does the same for
`pool/3sA`. The existing fallback looked up the full prefixed string
in the index and missed.

Strip everything up to the last `/` before retrying the index lookup.
Every caller going through EntityServer.Get now handles prefixed short
IDs automatically.
Three commands each had their own short ID bug.

`sandbox exec` never resolved the ID at all — it went straight to a
containerd label query, which stores full entity IDs. Added an EAC.Get
call before the query, soft-failing so full IDs still work if the
entity server is unreachable.

`logs sandbox` passed the user-provided ID directly to the log reader.
Added a resolveSandboxID helper that goes through the entity server,
wired into both SandboxLogs and resolveLogTarget.

`deploy cancel` did resolve the ID on the Get, but then wrote back
using the original unresolved value, so the Put targeted a nonexistent
key and silently succeeded — leaving the deploy lock stuck until its
30-minute timeout. Use the resolved entity ID for the write.

Closes MIR-1002
@phinze phinze requested a review from a team as a code owner April 17, 2026 19:28
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 078c3baf-3d8b-4563-8663-fc31c6fe2fc2

📥 Commits

Reviewing files that changed from the base of the PR and between c3b3acc and 3f38764.

📒 Files selected for processing (1)
  • servers/logs/logs.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • servers/logs/logs.go

📝 Walkthrough

Walkthrough

The PR adds ID resolution across several servers. DeploymentServer.CancelDeployment now replaces the provided deploymentId with the resolved entity ID before further operations. EntityServer.Get gains an EtcdStore fallback that, when an ID contains /, strips up to the last / and retries the DBShortId lookup. Server.Exec pre-resolves short container/entity IDs and uses the full ID in the containerd selector. Logs server adds resolveSandboxID and uses it in SandboxLogs and resolveLogTarget. Tests now create a sandbox entity before streaming logs.


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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

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 the current code and only fix it if needed.

Inline comments:
In `@servers/logs/logs.go`:
- Around line 201-205: The resolveSandboxID function currently maps every error
from s.EC.EAC().Get into a "not found" message; change it to inspect the
returned error and only convert it to a not-found error when the underlying
error actually indicates a missing sandbox (e.g. use errors.Is(err, ErrNotFound)
or, if the EAC client returns gRPC errors, check status.Code(err) ==
codes.NotFound), otherwise return the original error (or wrap it while
preserving the cause with %w) so upstream/service failures are not masked;
update resolveSandboxID to perform that conditional check and only
fmt.Errorf("sandbox %q not found: %w", sandboxID, err) for true not-found cases.
🪄 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: 6a066b34-29ca-43e0-aef7-2974eaf4b70b

📥 Commits

Reviewing files that changed from the base of the PR and between 8149629 and c3b3acc.

📒 Files selected for processing (5)
  • servers/deployment/server.go
  • servers/entityserver/entityserver.go
  • servers/exec/exec.go
  • servers/logs/logs.go
  • servers/logs/logs_test.go

Comment thread servers/logs/logs.go
Don't mislabel every resolveSandboxID failure as "not found" — upstream
or transport errors should surface as themselves. The wrapped cause is
still available via errors.Is for callers that care.
@phinze phinze merged commit 82dd634 into main Apr 17, 2026
12 checks passed
@phinze phinze deleted the phinze/mir-1002-short-ids-dont-resolve-for-runner-hosted-sandboxes-in-logs branch April 17, 2026 22:20
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.

2 participants