diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 10dd5ac..a6f9ce7 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -263,7 +263,14 @@ validation behavior across the two supported CI paths. ## 6. Make pending-asset bookkeeping linear — P3 -Status: open. +Status: complete. + +**Completed:** Replaced repeated slice removals with pending-map updates and a +single deferred compaction pass on every return path. Category bookkeeping is +now expected O(`n`) with O(`n`) metadata, preserving pending observation order. +Added an early-return regression with out-of-order observations for definitive +and uncertain failures. Delivery/Gitea tests and focused facade/CLI recovery +tests passed. **Evidence:** `uploadMissingAssets`, `reconcileAssetUpload`, and `removeIdentity` in [shared asset delivery](../../internal/delivery/remote_assets.go). diff --git a/internal/delivery/remote_assets.go b/internal/delivery/remote_assets.go index 58fd73e..b03b0ff 100644 --- a/internal/delivery/remote_assets.go +++ b/internal/delivery/remote_assets.go @@ -113,15 +113,26 @@ func remoteAssetDigest(ctx context.Context, backend forge.Backend, handle any) ( return hex.EncodeToString(hasher.Sum(nil)), nil } -func uploadMissingAssets(ctx context.Context, backend forge.Backend, releaseHandle any, inspection Inspection) (UploadResult, error) { +func uploadMissingAssets(ctx context.Context, backend forge.Backend, releaseHandle any, inspection Inspection) (result UploadResult, err error) { expected := orderedExpected(inspection.Assets) if err := artifact.ValidateExpectedAssets(expected); err != nil { return UploadResult{State: Partial}, &forge.Failure{Kind: forge.FailureInvalid, Err: err} } - result := UploadResult{State: Partial} + result = UploadResult{State: Partial} preflightConflict := false preflightUnverifiable := false - missing := make(map[string]artifact.ExpectedAsset, len(expected)) + pending := make(map[string]struct{}, len(expected)) + // Keep the initial observation order and compact pending identities once, + // including on early returns after partial or unresolved uploads. + defer func() { + remaining := result.Pending[:0] + for _, item := range result.Pending { + if _, ok := pending[item.Filename]; ok { + remaining = append(remaining, item) + } + } + result.Pending = remaining + }() for _, observation := range inspection.Assets { item := identity(observation.Expected) switch observation.State { @@ -129,7 +140,7 @@ func uploadMissingAssets(ctx context.Context, backend forge.Backend, releaseHand result.Matching = append(result.Matching, item) case AssetMissing: result.Pending = append(result.Pending, item) - missing[observation.Expected.Filename] = observation.Expected + pending[observation.Expected.Filename] = struct{}{} case AssetConflicting: preflightConflict = true result.Conflicting = append(result.Conflicting, item) @@ -148,13 +159,13 @@ func uploadMissingAssets(ctx context.Context, backend forge.Backend, releaseHand result.State = NotAttempted return result, &forge.Failure{Kind: forge.FailureUnverifiable, Err: errors.New("one or more existing attachments could not be verified")} } - if len(missing) == 0 { + if len(pending) == 0 { result.State = Complete return result, nil } for _, item := range orderMissing(expected) { - if _, needsUpload := missing[item.Filename]; !needsUpload { + if _, needsUpload := pending[item.Filename]; !needsUpload { continue } content, err := assetContent(item) @@ -165,18 +176,19 @@ func uploadMissingAssets(ctx context.Context, backend forge.Backend, releaseHand closeErr := content.Close() if uploadErr == nil { result.Uploaded = append(result.Uploaded, identity(item)) - result.Pending = removeIdentity(result.Pending, item.Filename) + delete(pending, item.Filename) continue } operationErr := errors.Join(uploadErr, closeErr) if evidence.Disposition != forge.WriteConflict && evidence.Disposition != forge.WriteUncertain { return result, operationErr } + delete(pending, item.Filename) if err := reconcileAssetUpload(ctx, backend, releaseHandle, item, evidence, operationErr, &result); err != nil { return result, err } } - if len(result.Pending) == 0 { + if len(pending) == 0 { result.State = Complete } return result, nil @@ -186,7 +198,6 @@ func reconcileAssetUpload(ctx context.Context, backend forge.Backend, releaseHan inspection, inspectErr := inspectRemoteAssets(ctx, backend, releaseHandle, []artifact.ExpectedAsset{item}) if inspectErr != nil || len(inspection.Assets) != 1 { result.State = Unknown - result.Pending = removeIdentity(result.Pending, item.Filename) result.Uncertain = append(result.Uncertain, identity(item)) return &forge.Failure{Kind: forge.FailureUnknown, Err: errors.Join(uploadErr, inspectErr)} } @@ -194,15 +205,12 @@ func reconcileAssetUpload(ctx context.Context, backend forge.Backend, releaseHan switch observation.State { case AssetMatching: result.Matching = append(result.Matching, identity(item)) - result.Pending = removeIdentity(result.Pending, item.Filename) return nil case AssetConflicting: result.State = Conflict - result.Pending = removeIdentity(result.Pending, item.Filename) result.Conflicting = append(result.Conflicting, identity(item)) return &forge.Failure{Kind: forge.FailureConflict, Err: uploadErr} case AssetMissing: - result.Pending = removeIdentity(result.Pending, item.Filename) if evidence.Disposition == forge.WriteConflict { result.State = Conflict result.Conflicting = append(result.Conflicting, identity(item)) @@ -213,7 +221,6 @@ func reconcileAssetUpload(ctx context.Context, backend forge.Backend, releaseHan return &forge.Failure{Kind: forge.FailureUnknown, Err: uploadErr} default: result.State = Unknown - result.Pending = removeIdentity(result.Pending, item.Filename) result.Uncertain = append(result.Uncertain, identity(item)) return &forge.Failure{Kind: forge.FailureUnknown, Err: errors.Join(uploadErr, observation.Err)} } @@ -251,12 +258,3 @@ func orderedExpected(observations []AssetInspection) []artifact.ExpectedAsset { func identity(asset artifact.ExpectedAsset) AssetIdentity { return AssetIdentity{Filename: asset.Filename, SHA256: asset.SHA256} } - -func removeIdentity(identities []AssetIdentity, filename string) []AssetIdentity { - for index, item := range identities { - if item.Filename == filename { - return append(identities[:index], identities[index+1:]...) - } - } - return identities -} diff --git a/internal/delivery/remote_assets_test.go b/internal/delivery/remote_assets_test.go index f32ab9f..8042f5e 100644 --- a/internal/delivery/remote_assets_test.go +++ b/internal/delivery/remote_assets_test.go @@ -276,6 +276,38 @@ func TestUploadMissingAssetsPreservesAcknowledgedProgressOnDefinitiveFailure(t * } } +func TestUploadMissingAssetsRetainsPendingObservationOrderOnEarlyReturn(t *testing.T) { + assets := []artifact.ExpectedAsset{ + expectedAsset("z.bin", []byte("z")), + expectedAsset("a.bin", []byte("a")), + expectedAsset("c.bin", []byte("c")), + expectedAsset("b.bin", []byte("b")), + } + for _, disposition := range []forge.WriteDisposition{forge.WriteRejected, forge.WriteUncertain} { + t.Run(string(disposition), func(t *testing.T) { + backend := &assetBackend{uploads: []assetUploadResult{ + {evidence: forge.WriteEvidence{Attempted: true, Disposition: forge.WriteAccepted}}, + {evidence: forge.WriteEvidence{Attempted: true, Disposition: disposition}, err: errors.New("upload stopped")}, + }} + result, err := uploadMissingAssets(context.Background(), backend, "release", inspectionFor(assets, map[string]AssetState{ + "z.bin": AssetMissing, "a.bin": AssetMissing, "c.bin": AssetMissing, "b.bin": AssetMissing, + })) + wantPending := []AssetIdentity{identity(assets[0]), identity(assets[2]), identity(assets[3])} + wantState := Partial + if disposition == forge.WriteUncertain { + wantPending = wantPending[:2] + wantState = Unknown + if !reflect.DeepEqual(result.Uncertain, []AssetIdentity{identity(assets[3])}) { + t.Fatalf("uncertain = %#v", result.Uncertain) + } + } + if err == nil || result.State != wantState || !reflect.DeepEqual(result.Pending, wantPending) || !reflect.DeepEqual(result.Uploaded, []AssetIdentity{identity(assets[1])}) { + t.Fatalf("result = %#v, error %v; want pending %#v", result, err, wantPending) + } + }) + } +} + func TestUploadMissingAssetsBorrowsReaderAndClosesItAfterCall(t *testing.T) { filePath := t.TempDir() + "/asset.bin" if err := os.WriteFile(filePath, []byte("asset"), 0o600); err != nil {