Skip to content

fix: handle instance start errors in dynamic and blocking strategies#854

Merged
acouvreur merged 2 commits into
sablierapp:mainfrom
mzyy94:fix-error-on-start
Apr 3, 2026
Merged

fix: handle instance start errors in dynamic and blocking strategies#854
acouvreur merged 2 commits into
sablierapp:mainfrom
mzyy94:fix-error-on-start

Conversation

@mzyy94

@mzyy94 mzyy94 commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

Summary

When InstanceStart returns an error (e.g. container not found), both strategies now handle it properly instead of silently ignoring it.

Problem

InstanceInfoWithError.Error was set when InstanceStart failed, but neither strategy checked it:

  • Dynamic: sessionStateToRenderOptionsInstanceState only used v.Instance, ignoring v.Error. The zero-value InstanceInfo{} was rendered as an empty loading screen that refreshed forever.
  • Blocking: RequestReadySession only checked session.IsReady() and the top-level error from RequestSession. Individual instance errors were never inspected, causing needless retries until timeout.

Fix

  • Dynamic (internal/api/start_dynamic.go): When v.Error is non-nil, render the instance with the requested name, unrecoverable status, and the provider error. This uses the existing error rendering support in all built-in themes ({{ if $instance.Error }}). The page continues to auto-refresh (200 OK), which is by design — the reverse proxy plugins use the X-Sablier-Status header for flow control, and the auto-refresh allows recovery if the user fixes the issue (e.g. creates the missing container).
  • Blocking (pkg/sablier/session_request.go): Check session.InstanceErrors() after each RequestSession call and return early with an error (500) instead of retrying until timeout.
  • Core (pkg/sablier/session.go): Add SessionState.InstanceErrors() method that collects and joins individual instance errors.

Before / After

http://localhost:10000/api/strategies/dynamic?names=foobar&session_duration=5m&theme=hacker-terminal

Before

before

After

after

Fixes #853

@mzyy94 mzyy94 requested a review from acouvreur as a code owner April 3, 2026 01:16
@sonarqubecloud

sonarqubecloud Bot commented Apr 3, 2026

Copy link
Copy Markdown

@acouvreur acouvreur left a comment

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.

LGTM! Thank you very much for this fix @mzyy94

@codecov

codecov Bot commented Apr 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 25.00000% with 15 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
internal/api/start_dynamic.go 12.50% 6 Missing and 1 partial ⚠️
pkg/sablier/session_request.go 0.00% 5 Missing and 1 partial ⚠️
pkg/sablier/session.go 66.66% 1 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
pkg/sablier/session.go 39.13% <66.66%> (+9.71%) ⬆️
pkg/sablier/session_request.go 47.25% <0.00%> (-3.34%) ⬇️
internal/api/start_dynamic.go 75.90% <12.50%> (-7.00%) ⬇️

@acouvreur acouvreur merged commit 69b3d57 into sablierapp:main Apr 3, 2026
4 of 7 checks passed
@sablier-bot sablier-bot Bot mentioned this pull request Apr 3, 2026
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.

Instance start errors are silently ignored in both dynamic and blocking strategies

2 participants