Skip to content

review(workspace 2/3): gRPC handlers, compute, drivers #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rhuss wants to merge 1 commit into
base: main
Choose a base branch
from
Open

review(workspace 2/3): gRPC handlers, compute, drivers #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions crates/openshell-driver-docker/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ use openshell_core::config::{
use openshell_core::driver_mounts;
use openshell_core::driver_utils::{
LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE, LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME,
LABEL_SANDBOX_NAMESPACE, SUPERVISOR_IMAGE_BINARY_PATH, supervisor_image_should_refresh,
LABEL_SANDBOX_NAMESPACE, LABEL_SANDBOX_WORKSPACE, SUPERVISOR_IMAGE_BINARY_PATH,
supervisor_image_should_refresh,
};
use openshell_core::gpu::{
CdiGpuDefaultSelector, CdiGpuInventory, CdiGpuSelectionError, driver_gpu_requirements,
Expand Down Expand Up @@ -1535,6 +1536,7 @@ fn pending_sandbox_snapshot(
conditions: vec![condition],
deleting,
}),
workspace: sandbox.workspace.clone(),
}
}

Expand Down Expand Up @@ -2292,6 +2294,10 @@ fn build_container_create_body_with_gpu_devices(
);
labels.insert(LABEL_SANDBOX_ID.to_string(), sandbox.id.clone());
labels.insert(LABEL_SANDBOX_NAME.to_string(), sandbox.name.clone());
labels.insert(
LABEL_SANDBOX_WORKSPACE.to_string(),
sandbox.workspace.clone(),
);
// The list/get/find paths filter by `config.sandbox_namespace`, so use
// the same value here. `DriverSandbox.namespace` is unset on the request
// path (the gateway elides it), and using it would produce containers
Expand Down Expand Up @@ -2723,6 +2729,10 @@ fn sandbox_from_container_summary(
.get(LABEL_SANDBOX_NAMESPACE)
.cloned()
.unwrap_or_default();
let workspace = labels
.get(LABEL_SANDBOX_WORKSPACE)
.cloned()
.unwrap_or_default();

let supervisor_connected = readiness.is_supervisor_connected(&id);
Some(DriverSandbox {
Expand All @@ -2735,6 +2745,7 @@ fn sandbox_from_container_summary(
&name,
supervisor_connected,
)),
workspace,
})
}

Expand Down Expand Up @@ -2908,24 +2919,26 @@ const CONTAINER_NAME_PREFIX: &str = "openshell-";

fn container_name_for_sandbox(sandbox: &DriverSandbox) -> String {
let id_suffix = sanitize_docker_name(&sandbox.id);
let workspace = sanitize_docker_name(&sandbox.workspace);
let name = sanitize_docker_name(&sandbox.name);

// Format: openshell-{workspace}--{name}-{id}
// The workspace and id are never truncated — they ensure uniqueness.
// Only the sandbox name portion is truncated when the total exceeds
// MAX_CONTAINER_NAME_LEN.

if name.is_empty() {
let mut base = format!("{CONTAINER_NAME_PREFIX}{id_suffix}");
// The prefix is always < MAX_CONTAINER_NAME_LEN. Truncate the id
// suffix only if the sandbox id itself is pathologically long.
let mut base = format!("{CONTAINER_NAME_PREFIX}{workspace}---{id_suffix}");
if base.len() > MAX_CONTAINER_NAME_LEN {
base.truncate(MAX_CONTAINER_NAME_LEN);
}
return base;
return trim_container_name_tail(base);
}

// Reserve space for the prefix and the `-<id_suffix>` tail so the id
// suffix — which is what makes the name unique between sandboxes that
// share a human-readable prefix — is never truncated away.
let reserved = CONTAINER_NAME_PREFIX.len() + 1 + id_suffix.len();
// Reserve space for fixed parts: prefix + workspace + "--" + "-" + id
let reserved = CONTAINER_NAME_PREFIX.len() + workspace.len() + 2 + 1 + id_suffix.len();
if reserved >= MAX_CONTAINER_NAME_LEN {
// Pathological sandbox id. Fall back to `<prefix><id>` and truncate.
let mut base = format!("{CONTAINER_NAME_PREFIX}{id_suffix}");
let mut base = format!("{CONTAINER_NAME_PREFIX}{workspace}---{id_suffix}");
base.truncate(MAX_CONTAINER_NAME_LEN);
return trim_container_name_tail(base);
Comment on lines +2925 to 2943

@coderabbitai coderabbitai Bot Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant section with line numbers
sed -n '2900,2975p' crates/openshell-driver-docker/src/lib.rs | cat -n

# Find the helper and related constants/usages
rg -n "CONTAINER_NAME_PREFIX|MAX_CONTAINER_NAME_LEN|trim_container_name_tail|id_suffix|container name" crates/openshell-driver-docker/src/lib.rs

# Show the full helper implementation if present nearby
ast-grep outline crates/openshell-driver-docker/src/lib.rs --view expanded | sed -n '1,220p'

Repository: rhuss/OpenShell

Length of output: 15827

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect nearby tests and references to container_name_for_sandbox
rg -n "container_name_for_sandbox|MAX_CONTAINER_NAME_LEN|trim_container_name_tail|oversized|truncate" crates/openshell-driver-docker/src/lib.rs crates/openshell-driver-docker -g '*test*' -g '*.rs'

# Read the test module area around the relevant function
start=$(rg -n "^#\[cfg\(test\)\]" crates/openshell-driver-docker/src/lib.rs | head -n 1 | cut -d: -f1 || true)
if [ -n "${start:-}" ]; then
  sed -n "${start},$((start+260))p" crates/openshell-driver-docker/src/lib.rs | cat -n
fi

Repository: rhuss/OpenShell

Length of output: 16316

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the container-name tests around the relevant area
sed -n '1960,2045p' crates/openshell-driver-docker/src/tests.rs | cat -n

Repository: rhuss/OpenShell

Length of output: 3528

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search for validation or limits on workspace/name lengths
rg -n "workspace.*len|name.*len|validate.*workspace|workspace.*validate|sandbox.*workspace|DriverSandboxTemplate|DriverSandbox" crates -g '*.rs'

# Inspect the sandbox validation area
sed -n '410,520p' crates/openshell-driver-docker/src/lib.rs | cat -n

Repository: rhuss/OpenShell

Length of output: 50371

🏁 Script executed:

python3 - <<'PY'
MAX_CONTAINER_NAME_LEN = 200
PREFIX = "openshell-"

def sanitize(value: str) -> str:
    out = []
    for ch in value:
        if ch.isascii() and (ch.isalnum() or ch in "_.-"):
            out.append(ch.lower())
        else:
            out.append("-")
    return "".join(out)

def trim_tail(value: str) -> str:
    while value and value[-1] in "-._":
        value = value[:-1]
    return value

def container_name(workspace: str, name: str, sid: str) -> str:
    id_suffix = sanitize(sid)
    workspace = sanitize(workspace)
    name = sanitize(name)

    if not name:
        base = f"{PREFIX}{workspace}---{id_suffix}"
        if len(base) > MAX_CONTAINER_NAME_LEN:
            base = base[:MAX_CONTAINER_NAME_LEN]
        return trim_tail(base)

    reserved = len(PREFIX) + len(workspace) + 2 + 1 + len(id_suffix)
    if reserved >= MAX_CONTAINER_NAME_LEN:
        base = f"{PREFIX}{workspace}---{id_suffix}"
        base = base[:MAX_CONTAINER_NAME_LEN]
        return trim_tail(base)

    budget = MAX_CONTAINER_NAME_LEN - reserved
    truncated_name = trim_tail(name[:budget]) if len(name) > budget else name
    return f"{PREFIX}{workspace}--{truncated_name}-{id_suffix}"

# Find examples where the ID suffix is truncated away.
cases = [
    ("w" * 180, "n", "id-1234567890"),
    ("w" * 190, "n", "id-1234567890"),
    ("w" * 195, "n", "id-1234567890"),
]
for ws, nm, sid in cases:
    cn = container_name(ws, nm, sid)
    print("len(workspace)=", len(ws), "len(id)=", len(sid), "len(name)=", len(cn), "endswith id?", cn.endswith(sanitize(sid)))
    print(cn)
    print()

# Search for a collision by varying only the ID while keeping a long workspace.
ws = "w" * 190
nm = "n"
a = container_name(ws, nm, "id-aaaaaaaaaaaaaaaaaaaaaaaaaaaa")
b = container_name(ws, nm, "id-bbbbbbbbbbbbbbbbbbbbbbbbbbbb")
print("collision?", a == b)
print(a)
print(b)
PY

Repository: rhuss/OpenShell

Length of output: 1375

Keep the sandbox ID when names overflow. In the name.is_empty() and reserved >= MAX_CONTAINER_NAME_LEN branches, base.truncate(MAX_CONTAINER_NAME_LEN) can cut off id_suffix entirely for long workspaces, so different sandboxes can end up with the same container name. Truncate the workspace/name portion instead and add a regression test for an oversized workspace.

🤖 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 `@crates/openshell-driver-docker/src/lib.rs` around lines 2925 - 2943, The
overflow branches in the container-name construction function can truncate away
id_suffix, causing collisions. Update the name.is_empty() and reserved >=
MAX_CONTAINER_NAME_LEN paths to shorten only the workspace/name portion while
always preserving the complete id_suffix, and add a regression test covering an
oversized workspace with distinct sandbox IDs.

}
Expand All @@ -2936,7 +2949,7 @@ fn container_name_for_sandbox(sandbox: &DriverSandbox) -> String {
} else {
name
};
format!("{CONTAINER_NAME_PREFIX}{truncated_name}-{id_suffix}")
format!("{CONTAINER_NAME_PREFIX}{workspace}--{truncated_name}-{id_suffix}")
}

/// Docker container names may not end with `-`, `.`, or `_`. Truncation can
Expand Down
10 changes: 8 additions & 2 deletions crates/openshell-driver-docker/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ fn test_sandbox() -> DriverSandbox {
sandbox_token: String::new(),
}),
status: None,
workspace: String::new(),
}
}

Expand Down Expand Up @@ -1980,6 +1981,7 @@ fn container_name_preserves_id_suffix_for_long_names() {
namespace: "default".to_string(),
spec: None,
status: None,
workspace: "default".to_string(),
};
let second = DriverSandbox {
id: "sbx-second-0987654321".to_string(),
Expand All @@ -2005,15 +2007,19 @@ fn container_name_preserves_id_suffix_for_long_names() {
}

#[test]
fn container_name_empty_sandbox_name_uses_id_only() {
fn container_name_empty_sandbox_name_uses_workspace_and_id() {
let sandbox = DriverSandbox {
id: "sbx-abc".to_string(),
name: String::new(),
namespace: "default".to_string(),
spec: None,
status: None,
workspace: "default".to_string(),
};
assert_eq!(container_name_for_sandbox(&sandbox), "openshell-sbx-abc",);
assert_eq!(
container_name_for_sandbox(&sandbox),
"openshell-default---sbx-abc",
);
}

#[test]
Expand Down
Loading
Loading