Skip to content

Commit b4bd3ee

Browse files
authored
Make plugin rollback cancellation-safe
Signed-off-by: Samuele Verzi <samu@stacklok.com>
1 parent 190f421 commit b4bd3ee

2 files changed

Lines changed: 80 additions & 7 deletions

File tree

‎pkg/plugins/pluginsvc/install.go‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"log/slog"
1111
"net/http"
1212
"strings"
13+
"time"
1314

1415
"github.com/stacklok/toolhive-core/httperr"
1516
"github.com/stacklok/toolhive/pkg/groups"
@@ -19,14 +20,16 @@ import (
1920
"github.com/stacklok/toolhive/pkg/skills/lockfile"
2021
)
2122

23+
// installRollbackTimeout bounds context-aware compensation after an install fails.
24+
const installRollbackTimeout = 5 * time.Second
25+
2226
// Install installs a plugin. When the Name field contains a git reference
2327
// (git://...), the repo is cloned and the plugin tree is built in memory. When
2428
// it contains an OCI reference, the artifact is pulled and extracted. A plain
2529
// name is resolved against the local OCI store, then the registry lookup.
2630
// Structural mirror of skillsvc.Install, substituting the plugin install
27-
// backends — but the failure semantics deliberately diverge: skills discards
28-
// rollback errors and fails forward, while plugins joins every compensation
29-
// error with the trigger and can abort (see rollbackInstall).
31+
// backends. Rollback errors are joined with the trigger so partial
32+
// compensation is never reported as a clean failure (see rollbackInstall).
3033
func (s *service) Install(ctx context.Context, opts plugins.InstallOptions) (*plugins.InstallResult, error) {
3134
return s.install(ctx, opts, false, nil)
3235
}
@@ -487,24 +490,26 @@ func (s *service) rollbackInstall(
487490
pluginName := result.Plugin.Metadata.Name
488491
scope := result.Plugin.Scope
489492
projectRoot := result.Plugin.ProjectRoot
493+
rollbackCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), installRollbackTimeout)
494+
defer cancel()
490495

491496
var errs []error
492497
if result.PreExisting != nil {
493-
if err := s.store.Update(ctx, *result.PreExisting); err != nil {
498+
if err := s.store.Update(rollbackCtx, *result.PreExisting); err != nil {
494499
errs = append(errs, fmt.Errorf("restoring pre-existing DB record: %w", err))
495500
}
496-
} else if err := s.store.Delete(ctx, pluginName, scope, projectRoot); err != nil {
501+
} else if err := s.store.Delete(rollbackCtx, pluginName, scope, projectRoot); err != nil {
497502
errs = append(errs, fmt.Errorf("deleting rolled-back DB record: %w", err))
498503
}
499504

500505
if result.RestoreFiles != nil {
501-
if err := result.RestoreFiles(ctx); err != nil {
506+
if err := result.RestoreFiles(rollbackCtx); err != nil {
502507
errs = append(errs, err)
503508
}
504509
}
505510

506511
if params.addedToGroup && s.groupManager != nil {
507-
if err := groups.RemovePluginFromGroup(ctx, s.groupManager, params.groupName, pluginName); err != nil {
512+
if err := groups.RemovePluginFromGroup(rollbackCtx, s.groupManager, params.groupName, pluginName); err != nil {
508513
errs = append(errs, fmt.Errorf("removing plugin from group: %w", err))
509514
}
510515
}

‎pkg/plugins/pluginsvc/upgrade_reanchor_test.go‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -751,6 +751,74 @@ func TestUpgrade_SameDigestCrossRepositoryMovePersistsAuthorizedReference(t *tes
751751
assert.Equal(t, []byte("preserve me"), sentinelContents)
752752
}
753753

754+
//nolint:paralleltest // serial: real sqlite + on-disk client materialization per test
755+
func TestUpgrade_SameDigestMetadataRollbackSurvivesCancellationAndConflict(t *testing.T) {
756+
inner, projectRoot, _, _ := newPluginKeyReanchorFixture(t)
757+
newRef := "ghcr.io/new-org/my-plugin:v2"
758+
require.NoError(t, lockfile.Update(mustOpenRoot(t, projectRoot), func(lf *lockfile.Lockfile) error {
759+
entry, ok := lf.GetPlugin("my-plugin")
760+
require.True(t, ok)
761+
entry.Source = newRef
762+
lf.UpsertPlugin(entry)
763+
return nil
764+
}))
765+
766+
beforeInstalled, err := inner.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot)
767+
require.NoError(t, err)
768+
callerCtx, cancel := context.WithCancel(t.Context())
769+
t.Cleanup(cancel)
770+
var updateContextErrors []error
771+
var hookErr error
772+
var competingEntry lockfile.Entry
773+
hookedStore := &hookPluginStore{
774+
PluginStore: inner.store,
775+
afterUpdate: func(updateCtx context.Context, call int) {
776+
updateContextErrors = append(updateContextErrors, updateCtx.Err())
777+
if call != 1 {
778+
return
779+
}
780+
hookErr = lockfile.Update(mustOpenRoot(t, projectRoot), func(lf *lockfile.Lockfile) error {
781+
entry, exists := lf.GetPlugin("my-plugin")
782+
if !exists {
783+
return fmt.Errorf("planned lock entry disappeared")
784+
}
785+
entry.Digest = validLockDigestAlt()
786+
lf.UpsertPlugin(entry)
787+
competingEntry = entry
788+
return nil
789+
})
790+
cancel()
791+
},
792+
}
793+
inner.store = hookedStore
794+
795+
result, err := inner.Upgrade(callerCtx, plugins.UpgradeOptions{
796+
ProjectRoot: projectRoot,
797+
AllowRefChange: true,
798+
AllowSignerChange: true,
799+
PublicKey: reanchorPublicKeyB64,
800+
})
801+
require.NoError(t, err)
802+
require.NoError(t, hookErr)
803+
require.Len(t, result.Outcomes, 1)
804+
assert.Equal(t, plugins.UpgradeStatusFailed, result.Outcomes[0].Status)
805+
assert.Contains(t, result.Outcomes[0].Error, lockfile.ErrEntryChanged.Error())
806+
assert.ErrorIs(t, callerCtx.Err(), context.Canceled)
807+
require.Len(t, updateContextErrors, 2,
808+
"the metadata update must be followed by a compensating database update")
809+
assert.NoError(t, updateContextErrors[0])
810+
assert.NoError(t, updateContextErrors[1],
811+
"compensation must detach from the canceled request context")
812+
813+
afterInstalled, getErr := inner.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot)
814+
require.NoError(t, getErr)
815+
assert.Equal(t, beforeInstalled, afterInstalled)
816+
afterEntry, ok := loadPluginLockEntry(t, projectRoot)
817+
require.True(t, ok)
818+
assert.Equal(t, competingEntry, afterEntry,
819+
"rollback must not overwrite the competing lock update")
820+
}
821+
754822
func trustOnlyReanchorPlan(entry lockfile.Entry) upgradePlan {
755823
return upgradePlan{
756824
entry: entry,

0 commit comments

Comments
 (0)