Skip to content

build(huntsman): Add local build tasks for Spider Huntsman images. #381

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

Merged
LinZhihao-723 merged 5 commits into from
Jul 10, 2026
+81 −0
Merged

build(huntsman): Add local build tasks for Spider Huntsman images. #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b4ef4e0
build: Add a Taskfile to build the Spider service images.
20001020ycx Jul 8, 2026
3250c44
Apply suggestions from code review
20001020ycx Jul 9, 2026
2a13373
Apply suggestion from @junhaoliao
20001020ycx Jul 9, 2026
76ca648
Merge branch 'main' into build/2026-07-07-docker-compose-taskfile
20001020ycx Jul 9, 2026
46a600c
Merge branch 'main' into build/2026-07-07-docker-compose-taskfile
20001020ycx Jul 10, 2026
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
1 change: 1 addition & 0 deletions taskfile.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ version: "3"
includes:
build: "taskfiles/build.yaml"
deps: "taskfiles/deps.yaml"
docker: "taskfiles/docker.yaml"
docs: "taskfiles/docs.yaml"
lint: "taskfiles/lint.yaml"
test: "taskfiles/test.yaml"
Expand Down
23 changes: 23 additions & 0 deletions taskfiles/docker.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
version: "3"

vars:
G_BUILD_SCRIPT: "{{.ROOT_DIR}}/tools/docker/build.sh"

tasks:
build:
cmds:
- task: "build-storage"
- task: "build-scheduler"
- task: "build-worker"

build-storage:
deps: [":init"]
cmd: "'{{.G_BUILD_SCRIPT}}' storage"

build-scheduler:
deps: [":init"]
cmd: "'{{.G_BUILD_SCRIPT}}' scheduler"

build-worker:
deps: [":init"]
cmd: "'{{.G_BUILD_SCRIPT}}' worker"
Comment on lines +12 to +23

@LinZhihao-723 LinZhihao-723 Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shall we consider making these tasks private?

@20001020ycx 20001020ycx Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think public also makes sense, developer may just run task docker:build-storage for faster docker build.

@20001020ycx 20001020ycx Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@junhaoliao what do you think? :)

I am fine with both

57 changes: 57 additions & 0 deletions tools/docker/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
#!/usr/bin/env bash

set -o errexit
set -o nounset
set -o pipefail

service="${1:?Usage: build.sh <storage|scheduler|worker>}"

remove_temp_file_and_prev_image() {
rm -f "$temp_iid_file"

if [[ -z "$new_image_id" ]]; then
rm -f "$iid_file"
elif [[ "$prev_image_id" == "$new_image_id" ]]; then
return
fi

[[ -z "$prev_image_id" ]] && return

docker image inspect "$prev_image_id" >/dev/null 2>&1 || return

echo "Removing previous image $prev_image_id."
docker image remove "$prev_image_id" || true
}
trap remove_temp_file_and_prev_image EXIT
Comment on lines +9 to +25

@coderabbitai coderabbitai Bot Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Previous image is deleted on build failure.

When docker build fails, errexit exits the script before new_image_id is populated (it stays "" from line 31). The trap then evaluates line 12: -z "$prev_image_id" is false (prior build exists) and "$prev_image_id" == "$new_image_id" is false (old_id != ""), so the guard does not short-circuit. The previous working image is removed, yet iid_file still references it — breaking downstream docker compose consumers that read that file.

🔒 Proposed fix: short-circuit when no new image was built 
-    [[ -z "$prev_image_id" || "$prev_image_id" == "$new_image_id" ]] && return
+    [[ -z "$prev_image_id" || -z "$new_image_id" || "$prev_image_id" == "$new_image_id" ]] && return
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
remove_temp_file_and_prev_image() {
rm -f "$temp_iid_file"
[[ -z "$prev_image_id" || "$prev_image_id" == "$new_image_id" ]] && return
docker image inspect "$prev_image_id" >/dev/null 2>&1 || return
echo "Removing previous image $prev_image_id."
docker image remove "$prev_image_id" || true
}
trap remove_temp_file_and_prev_image EXIT
remove_temp_file_and_prev_image() {
rm -f "$temp_iid_file"
[[ -z "$prev_image_id" || -z "$new_image_id" || "$prev_image_id" == "$new_image_id" ]] && return
docker image inspect "$prev_image_id" >/dev/null 2>&1 || return
echo "Removing previous image $prev_image_id."
docker image remove "$prev_image_id" || true
}
trap remove_temp_file_and_prev_image EXIT
🤖 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 `@tools/docker/build.sh` around lines 9 - 19, The cleanup in
remove_temp_file_and_prev_image is deleting the prior working image even when
docker build fails and new_image_id was never set. Update the guard in the trap
handler so it exits early when no new image was built, using the existing
remove_temp_file_and_prev_image function and the new_image_id / prev_image_id
variables. Keep the previous image intact unless a new image ID was successfully
populated and differs from prev_image_id, so iid_file continues to point at a
valid image for downstream consumers.

@20001020ycx 20001020ycx Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@junhaoliao should we add -z "$prev_image_id" to the guard? Does this mean CLP also have the same bug?

@coderabbitai coderabbitai Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
# Fetch CLP's build.sh to check whether it has the same guard logic
gh api repos/y-scope/clp/contents/tools/docker/build.sh --jq '.content' 2>/dev/null | base64 -d || echo "NOT_FOUND_default_path"

Length of output: 410

🏁 Script executed:

#!/bin/bash
set -euo pipefail
gh search code "remove_temp_file_and_prev_image" --repo y-scope/clp
echo "---"
gh api repos/y-scope/clp/git/trees/main?recursive=1 --jq '.tree[].path' | grep -i docker | grep -i build || true

Length of output: 1027

🏁 Script executed:

#!/bin/bash
set -euo pipefail
gh api repos/y-scope/clp/contents/tools/docker-images/clp-package/build.sh --jq '.content' | base64 -d

Length of output: 2172

@20001020ycx Good catch, and yes — confirmed.

I fetched CLP's tools/docker-images/clp-package/build.sh from y-scope/clp (main branch), and it has the exact same guard:

[[ -z "$prev_image_id" || "$prev_image_id" == "$new_image_id" ]] && return

Both scripts initialize new_image_id="" before running docker build. If the build fails, errexit fires the EXIT trap before new_image_id is ever reassigned, so it stays "". At that point:

  • -z "$prev_image_id" → false (a previous image exists)
  • "$prev_image_id" == "$new_image_id" → false (old_id vs "")

So the guard doesn't short-circuit, and the previous working image gets deleted even though the build failed — leaving iid_file pointing at a now-nonexistent image. This confirms the bug exists in CLP as well, not just here.

Regarding the fix: to be precise, it's -z "$new_image_id" (not prev_image_id, which is already checked) that needs to be added:

-    [[ -z "$prev_image_id" || "$prev_image_id" == "$new_image_id" ]] && return
+    [[ -z "$prev_image_id" || -z "$new_image_id" || "$prev_image_id" == "$new_image_id" ]] && return

Since this is inherited from CLP, it'd be worth fixing here in Spider regardless, and optionally flagging it upstream to the CLP maintainers as a separate issue if you'd like — happy to help draft that if you want to go that route.

@junhaoliao junhaoliao Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

right, i agree that:

  1. since this is potentially a new build, we should always clean the older image if the new build is faulty, just so we don't mislead developers running services with older images
  2. if the image has been cleaned, we should not leave a stale iid file

i proposed https://github.com/y-scope/spider/pull/381/changes#r3553514597 instead then

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
repo_root="${script_dir}/../../"
iid_file="${repo_root}/build/spider-${service}-image.id"

prev_image_id=""
if [[ -f "$iid_file" ]]; then
prev_image_id=$(<"$iid_file")
fi

temp_iid_file="$(mktemp)"
new_image_id=""

docker build \
--pull \
--target "$service" \
--iidfile "$temp_iid_file" \
--file "${script_dir}/Dockerfile" \
"$repo_root"

if [[ -s "$temp_iid_file" ]]; then
new_image_id="$(<"$temp_iid_file")"
echo "$new_image_id" > "$iid_file"

user="${USER:-$(whoami 2>/dev/null)}" \
|| user=$(id -un 2>/dev/null) \
|| user=$(id -u 2>/dev/null) \
|| user="unknown";
short_id="${new_image_id#sha256:}"
short_id="${short_id:0:4}"
docker tag "$new_image_id" "spider-${service}:dev-${user}-${short_id}"
fi
Loading