Skip to content

feat(runtime): expose precise SnapshotProgress during setup - #97

Open
IOHelpMe wants to merge 3 commits into
highcard-dev:masterfrom
IOHelpMe:marvin/hig-24-improve-fetching-scroll-progress-visibility
Open

feat(runtime): expose precise SnapshotProgress during setup#97
IOHelpMe wants to merge 3 commits into
highcard-dev:masterfrom
IOHelpMe:marvin/hig-24-improve-fetching-scroll-progress-visibility

Conversation

@IOHelpMe

@IOHelpMe IOHelpMe commented Jul 20, 2026

Copy link
Copy Markdown

Summary

  • keep one stable SnapshotProgress across setup and lifecycle operations
  • read SteamCMD progress independently from the lossless console stream
  • preserve confirmed progress to one decimal through the existing callback and health contract

Why

The existing whole-percent storage rounded away valid sub-percent SteamCMD samples, while console backpressure could delay newer samples. Atomic fixed-point tenths preserve those real values without extending SnapshotProgress or the public schema; no progress is estimated or fabricated. LinuxGSM pipe buffering is fixed separately in highcard-dev/scrolls#71.

Verification

  • go test ./...
  • focused fresh regression tests for domain storage, Kubernetes parsing, callback transport, and health output
  • git diff --check
  • live ARK validation confirmed continuously arriving progress

Merge highcard-dev/monorepo#318 after this PR. Scrolls#71 can merge independently; all three are required for the complete LGSM path.

@druid-infra

Copy link
Copy Markdown
Contributor
Error: This repo is not allowlisted for Atlantis.

@IOHelpMe
IOHelpMe requested a review from MarcStdt July 20, 2026 23:24
@IOHelpMe

IOHelpMe commented Jul 22, 2026

Copy link
Copy Markdown
Author

GPT-5.6 follow-up review is implemented in dc3ec32 through ba2b879.

The reporter is now serialized, retries failed progress posts without creating concurrent requests, and shuts down deterministically. Callback input is validated at the HTTP boundary, progress is cleared across completion/cancellation/retry lifecycles, and OCI progress cannot report 100% before copy, manifest fetch/decode, and metadata writes have all succeeded. A post-copy write-failure regression specifically proves failed pulls remain below 100% and return to idle.

Fresh local verification:

  • go test ./...
  • go test -race -count=1 ./apps/druid/adapters/cli
  • go test -race -count=1 ./internal/core/services/registry
  • make validate-api
  • git diff --check

All passed. This remains the slim SnapshotProgress HIG-24 scope; ETA, stage/speed telemetry, procedure progress, and Scroll changes are intentionally not part of this PR. Merge this PR before highcard-dev/monorepo#318.

Remote CI update: build, unit-tests, integration-tests, and validate-api all pass. The remaining prerelease job reaches release creation and then receives 403 Resource not accessible by integration; this is the known GitHub token restriction for a fork PR, not a compile/test failure in this change: https://github.com/highcard-dev/druid-cli/actions/runs/29949322672/job/89023644458

@IOHelpMe IOHelpMe changed the title feat(runtime): expose scroll pull SnapshotProgress feat(runtime): expose scroll and installation progress Jul 23, 2026
@IOHelpMe

Copy link
Copy Markdown
Author

GPT-5.6 follow-up is implemented in 91b140fa1caef416b6b7bbb1a46e95abe2b9257f.

This extends the existing slim SnapshotProgress work with the missing server installation/launch path: queue/API procedure progress, Docker/Kubernetes lifecycle stages, a bounded stdout control protocol, stable daemon-side speed/ETA estimation, and the SteamCMD translator. During final review I also pinned the existing generated LockStatusStatus* and RuntimeScrollStatus* names so the OpenAPI regeneration does not introduce an unrelated breaking Go API rename.

Fresh verification:

  • go test ./... — all packages passed
  • make validate-api — passed
  • git diff --check — passed
  • fresh local ARK deployment — 249 progress updates, approximately 9m15s install, then all launch procedures completed and deployment became Active

Merge order: this PR → highcard-dev/scrolls#70 → highcard-dev/monorepo#318.

@IOHelpMe

Copy link
Copy Markdown
Author

Remote CI final status: build, unit-tests, integration-tests, and validate-api all pass. The only red job is prerelease, which successfully downloads the built artifacts and then receives 403 Resource not accessible by integration when the fork PR token tries to create GitHub release pr-97. This is a fork-token permission restriction, not a product/test failure, so no code workaround was added: https://github.com/highcard-dev/druid-cli/actions/runs/30043103676/job/89328599533

@IOHelpMe

Copy link
Copy Markdown
Author

Follow-up after senior review:

  • fixed sparse estimator history so 6–15 second SteamCMD gaps no longer keep speed/ETA permanently unavailable
  • added the central HTTP download producer and bounded SteamCMD line buffering
  • made the local build import both runtime images, with druid:local-steamcmd inheriting the freshly built druid:local
  • validated CLI adapter/services, Dockerfile resolution, realistic sparse traces, and both SteamCMD/HTTP local flows

Latest fix commit: d3e78ca.

@IOHelpMe

Copy link
Copy Markdown
Author

CI follow-up: the failing prerelease job was caused by GitHub correctly giving fork pull requests a read-only token, so release creation returned 403. Commit 6102f0b keeps all validation jobs for forks but gates prerelease and Docker publishing to same-repository branches. No elevated pull_request_target workflow or write access for untrusted fork code was introduced.

@IOHelpMe
IOHelpMe force-pushed the marvin/hig-24-improve-fetching-scroll-progress-visibility branch from 6102f0b to cad3b6b Compare July 24, 2026 00:00
@IOHelpMe IOHelpMe changed the title feat(runtime): expose scroll and installation progress feat(runtime): expose scroll pull SnapshotProgress Jul 24, 2026
@IOHelpMe

Copy link
Copy Markdown
Author

Clean rewrite completed at cad3b6b. The PR now keeps SnapshotProgress unchanged and forwards only its percentage through a small authenticated worker callback into the existing health.progress field. Initialize/update/restore OCI pulls are covered; no queue contract, procedure progress, speed, bytes, or ETA were added. Verified with go test ./..., go vet ./..., and git diff --check.

@IOHelpMe

Copy link
Copy Markdown
Author

CI follow-up: validate-api, unit-tests, build, and integration-tests all passed. The only red job is the downstream prerelease publisher: softprops/action-gh-release receives HTTP 403 Resource not accessible by integration when trying to create tag/release pr-97 from this fork PR. This is a GitHub Actions token-permission/infrastructure issue, not a test or product-code failure; no scope-expanding workaround was added.

@IOHelpMe

Copy link
Copy Markdown
Author

Local Windows/WSL E2E validation for the slim SnapshotProgress path:

  • The rebuilt druid:local worker image containing this PR was imported into k3d and used by the runtime workers.
  • A real progress callback was observed through the public health endpoint as { mode: ok, progress: 0 } during materialization; progress was removed again after the worker completed, matching the intended transient SnapshotProgress lifetime.
  • Fresh ARK (3247f39d-a40b-4360-bccb-ca40696de5f7) and Minecraft Vanilla (033ba4f0-7ff9-454d-9107-e37e8133cb7b) deployments both completed materialization and reached Active with ready runtime pods and populated Kubernetes endpoints.
  • Their exposed local TCP ports were reachable (11011 for ARK RCON; 11000 and 11017 for Minecraft).

Because each tested Scroll artifact is effectively a single OCI layer, the unchanged SnapshotProgress source can legitimately expose only the synthesized 0% followed by completion. This PR still does not infer intermediate procedure/download progress and does not extend SnapshotProgress.

@IOHelpMe IOHelpMe changed the title feat(runtime): expose scroll pull SnapshotProgress feat(runtime): expose SnapshotProgress during setup Jul 24, 2026
@IOHelpMe

Copy link
Copy Markdown
Author

Follow-up after live ARK validation and senior review (d8a8e13):

  • Confirmed SnapshotProgress itself did not report 44 -> 67 -> 88 jumps; the Pod emitted granular real byte samples.
  • Fixed Pod-to-health lag by separating progress parsing from the blocking console stream. The console remains lossless, the progress stream is joined before job cleanup, and no unbounded queue is used.
  • Parse native SteamCMD byte lines directly, so the slim PR no longer depends on the previously built experimental v0.1.256 wrapper. Optional DRUID_PROGRESS_V1 frames remain supported.
  • Hardened SnapshotProgress ownership across create/update/restore/stop and overlapping sessions.

Validation: go test ./..., focused regressions, git diff --check, and live ARK source-vs-health investigation. No queue/OpenAPI/Scroll changes, no speed/ETA, and SnapshotProgress remains unchanged.

@IOHelpMe IOHelpMe changed the title feat(runtime): expose SnapshotProgress during setup feat(runtime): expose precise SnapshotProgress during setup Jul 25, 2026
@IOHelpMe

Copy link
Copy Markdown
Author

@MarcStdt Ready for review. SnapshotProgress remains the only source of truth; it now preserves real tenths instead of rounding valid SteamCMD samples to whole percentages. The existing callback/health schema is unchanged, and the focused plus full Go tests pass. The upstream LinuxGSM buffering fix is highcard-dev/scrolls#71.

@IOHelpMe

Copy link
Copy Markdown
Author

CI note for review: unit, integration, API validation, and build are green. Only prerelease fails while creating release pr-97 with HTTP 403 (Resource not accessible by integration); this fork-triggered job receives a read-only token and no secrets, so it is unrelated to the implementation.

@druid-infra druid-infra left a comment

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.

Compared against the current repo patterns and architecture notes. One design concern called out inline: this adds a handwritten callback route where the repo has otherwise been moving API shape back under generated OpenAPI contracts. No other blocking issues from this pass.

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.

This works, but it does step outside the repo’s current API pattern. CONTEXT.md says the REST surface should come from the generated OpenAPI handlers, with only /health and websocket attach kept manual. Since this adds another handwritten callback route, I’d strongly prefer adding the progress endpoint to the callback OpenAPI contract and regenerating internal/callbackapi instead of letting the route/client shape drift in two places.

@MarcStdt

Copy link
Copy Markdown
Contributor

the scope is still to big please remove the progress reading from pod logs, rater only rely on SnapshotProgress.
Don't read logs from a steam game or so. also adjust other prs accrodingly

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.

3 participants