Bug: Delete silently succeeds for non-existent artifacts

[zesty-clawd]

Summary
The Delete method in artifact/inmemory.go silently succeeds when deleting non-existent artifacts (both specific versions and latest-version case), while Load correctly returns fs.ErrNotExist. This inconsistency violates the principle of least surprise and makes error detection difficult.

Location

• File: artifact/inmemory.go
• Method: inMemoryService.Delete()
• Lines: ~175-191

Current Behavior

func (s *inMemoryService) Delete(ctx context.Context, req *DeleteRequest) error {
	// ... validation and lock ...

	if version != 0 {
		s.delete(appName, userID, sessionID, fileName, version)
		return nil  // ← Always succeeds, even if version doesn't exist
	}

	// Delete all versions (version == 0)
	lo := artifactKey{...Version: math.MaxInt64}.Encode()
	hi := artifactKey{...FileName: fileName}.Encode()
	s.artifacts.DeleteRange(lo, hi)
	return nil  // ← Always succeeds, even if no artifacts match
}

Both paths always return nil, regardless of whether the artifact existed.

Issue

1. Version-specific delete: Calls s.delete() → omap.Delete(), which is idempotent (no error if key missing)
2. Latest-version delete: Calls DeleteRange(), also idempotent
3. No existence check: Neither path verifies the artifact existed before deletion

Inconsistency with Load

func (s *inMemoryService) Load(...) (*LoadResponse, error) {
	// ...
	_, artifact, ok := s.find(appName, userID, sessionID, fileName)
	if !ok {
		return nil, fmt.Errorf("artifact not found: %w", fs.ErrNotExist)
	}
	return &LoadResponse{Part: artifact}, nil
}

Load returns an error when the artifact doesn't exist. Users expect the same from Delete.

Expected Behavior
Delete should return fs.ErrNotExist (or wrapped error) when attempting to delete a non-existent artifact, consistent with:

• Load() in the same service
• Standard filesystem operations (os.Remove returns fs.ErrNotExist)
• RESTful API conventions (DELETE returns 404 for missing resources)

Reproduction

package main

import (
	"context"
	"errors"
	"fmt"
	"io/fs"
	"google.golang.org/adk/artifact"
)

func main() {
	svc := artifact.InMemoryService()
	ctx := context.Background()

	// Delete non-existent artifact
	err := svc.Delete(ctx, &artifact.DeleteRequest{
		AppName:   "app1",
		UserID:    "user1",
		SessionID: "sess1",
		FileName:  "nonexistent.txt",
		Version:   0,  // Delete latest version
	})

	// Expected: errors.Is(err, fs.ErrNotExist) == true
	// Actual: err == nil
	fmt.Printf("Delete result: %v (is ErrNotExist: %v)\n", err, errors.Is(err, fs.ErrNotExist))
}

Output:

Delete result: <nil> (is ErrNotExist: false)

Suggested Fix
Check for existence before deletion:

func (s *inMemoryService) Delete(ctx context.Context, req *DeleteRequest) error {
	// ... validation, namespace adjustment, lock ...

	if version != 0 {
		// Check if specific version exists
		if _, ok := s.get(appName, userID, sessionID, fileName, version); !ok {
			return fmt.Errorf("artifact not found: %w", fs.ErrNotExist)
		}
		s.delete(appName, userID, sessionID, fileName, version)
		return nil
	}

	// Check if any version exists before deleting all (version == 0)
	if _, _, ok := s.find(appName, userID, sessionID, fileName); !ok {
		return fmt.Errorf("artifact not found: %w", fs.ErrNotExist)
	}

	lo := artifactKey{AppName: appName, UserID: userID, SessionID: sessionID, FileName: fileName, Version: math.MaxInt64}.Encode()
	hi := artifactKey{AppName: appName, UserID: userID, SessionID: sessionID, FileName: fileName}.Encode()
	s.artifacts.DeleteRange(lo, hi)
	return nil
}

Impact

• API Consistency: Breaks user expectations when compared to Load()
• Error Detection: Applications can't distinguish between successful deletion and no-op
• Debugging: Silent failures make troubleshooting harder
• Idempotency: While idempotent behavior is sometimes desired for HTTP DELETE, it should be explicit (e.g., via a flag like IgnoreMissing bool in DeleteRequest), not the only option

Test Case

func TestDelete_NonExistent(t *testing.T) {
	svc := artifact.InMemoryService()
	ctx := context.Background()

	err := svc.Delete(ctx, &artifact.DeleteRequest{
		AppName:   "app1",
		UserID:    "user1",
		SessionID: "sess1",
		FileName:  "missing.txt",
		Version:   0,
	})

	if !errors.Is(err, fs.ErrNotExist) {
		t.Errorf("expected fs.ErrNotExist, got: %v", err)
	}
}

Additional Context
This affects the InMemoryService implementation. The same issue may exist in other artifact service implementations (e.g., GCS-backed). Consistency across all implementations would be valuable.

[verdverm]

what does the database version do? what do the other language implementations do?

It seems prudent to keep consistency across implementations of artifact.Delete, and if the update is desired, make it uniformly applied

[koriyoshi2041]

This inconsistency between Delete and Load is a real problem for debugging. A silent success on deleting a non-existent artifact makes it impossible to detect typos in artifact names or version numbers at the application level.

One approach: check existence before deletion and return fs.ErrNotExist to match the Load behavior. For the version==0 (delete all) case, we could either return an error if no versions exist, or treat it as idempotent (which is common for bulk-delete semantics).

I'd suggest aligning with the Load contract — return fs.ErrNotExist for specific version deletes, but keep the bulk delete (version==0) idempotent since "delete everything for this artifact" naturally succeeds when there's nothing to delete. Happy to submit a PR for this.