Data lifecycle GC scanning fails for nested/namespaced model types

[umag]

Description

In src/domain/data/data_lifecycle_service.ts (lines 186-195), findExpiredData() iterates the .swamp/data/ directory using readDir(), which yields only the first path segment. For nested types like command/shell or @docker/host, it reads command or @docker as the type name. Subdirectories (shell, host) are incorrectly treated as model IDs, causing the scan to fail silently.

Steps to Reproduce

1. Create a model of type command/shell or @docker/host
2. Run the model to produce data with a lifetime (e.g., 1h)
3. Wait for the data to expire
4. Run garbage collection
5. The expired data is never cleaned up

Expected Behavior

GC scanning should correctly handle nested type directory structures (type/subtype/modelId/dataName/version/).

Actual Behavior

The scanner treats the type subdirectory (shell, host) as a model UUID, resulting in failed lookups. Data for all nested/namespaced types is never garbage collected.

Summary

This affects the data lifecycle service's directory scanning logic. The fix would involve walking the directory tree to the correct depth based on the type structure, or using the model registry to enumerate known types and their directory paths rather than relying on directory listing.

[stack72]

/plan

[github-actions]

Implementation Plan

Summary

The DataLifecycleService.findExpiredData() and deleteExpiredData() methods in src/domain/data/data_lifecycle_service.ts use a naive directory listing approach that only handles single-level type paths. When iterating over .swamp/data/, it treats the first directory segment as the complete type and any nested subdirectories as model IDs, causing GC to fail silently for nested types like command/shell or @docker/host.

Root Cause: At lines 186-195 and 294-299, the service reads the first level of directories and incorrectly passes typeEntry.name (e.g., "command") directly to ModelType.create(), then expects the next directory level to be model IDs. For nested types like command/shell, the type directory is actually command/shell/, but the scanner treats shell/ as a model ID directory.

Solution: Leverage the existing UnifiedDataRepository.findAllGlobal() method which already handles nested directory structures correctly (see collectAllData() at lines 342-390 of unified_data_repository.ts). The lifecycle service should delegate directory traversal to the repository rather than implementing its own scanning logic.

DDD Analysis

Domain Concepts Involved:

• Data (Aggregate Root): The entity being garbage collected, with lifetime and GC policies
• ModelType (Value Object): Represents nested type paths like command/shell
• DataLifecycleService (Domain Service): Orchestrates lifecycle operations across multiple aggregates

Design Decision: The DataLifecycleService should not contain directory traversal logic. This is infrastructure concern that belongs in the repository layer. The service should use repository methods to enumerate data rather than directly scanning the filesystem.

Pattern Applied: Repository Pattern - the service delegates all persistence-related operations (including enumeration) to the repository, keeping domain logic separate from storage details.

Files to Modify

1. src/domain/data/data_lifecycle_service.ts

Changes needed:

• findExpiredData() method (lines 181-255): Replace the manual directory traversal with a call to this.dataRepo.findAllGlobal(), which already correctly handles nested type directories. Then filter the results for expired data.

• deleteExpiredData() method (lines 257-334): Similarly, replace the version GC directory scan (lines 294-325) with findAllGlobal() results, grouping by type+modelId for GC calls.

Key changes in findExpiredData():

async findExpiredData(): Promise<ExpiredDataInfo[]> {
  const expired: ExpiredDataInfo[] = [];
  const allData = await this.dataRepo.findAllGlobal();

  for (const { data, modelType, modelId } of allData) {
    try {
      const isExpired = await this.isExpired(data);
      if (isExpired) {
        let reason: ExpiredDataInfo["reason"];
        if (data.lifetime === "workflow" || data.lifetime === "job") {
          reason = data.lifetime === "workflow"
            ? "workflow-deleted"
            : "job-deleted";
        } else {
          reason = "duration-expired";
        }

        expired.push({
          type: modelType,
          modelId,
          dataName: data.name,
          data,
          reason,
        });
      }
    } catch (error) {
      console.error(
        `Error checking data ${modelType.toDirectoryPath()}/${modelId}/${data.name}:`,
        error,
      );
    }
  }

  return expired;
}

Key changes in deleteExpiredData() for version GC (around lines 291-325):

// Collect unique type+modelId pairs for GC
const modelsToGC = new Map<string, { type: ModelType; modelId: string }>();
const allData = await this.dataRepo.findAllGlobal();
for (const { modelType, modelId } of allData) {
  const key = `${modelType.toDirectoryPath()}:${modelId}`;
  if (!modelsToGC.has(key)) {
    modelsToGC.set(key, { type: modelType, modelId });
  }
}

for (const { type, modelId } of modelsToGC.values()) {
  try {
    const result = await this.dataRepo.collectGarbage(type, modelId);
    versionsDeleted += result.versionsRemoved;
    bytesReclaimed += result.bytesReclaimed;
  } catch (error) {
    console.error(
      `Error running GC on ${type.toDirectoryPath()}/${modelId}:`,
      error,
    );
  }
}

2. src/domain/data/data_lifecycle_service_test.ts

Changes needed:

Add new tests to verify GC works correctly with nested/namespaced model types:

1. Test: findExpiredData - handles nested type directories (command/shell)

   • Mock findAllGlobal() to return data with nested types
   • Verify the service correctly identifies expired data for these types

2. Test: findExpiredData - handles namespaced type directories (@docker/host)

   • Similar test for user-namespaced types

3. Test: deleteExpiredData - collects garbage for nested types

   • Verify GC is called correctly for models with nested type paths

4. Test: deleteExpiredData - handles empty data directory gracefully

   • Ensure no errors when findAllGlobal() returns empty array

Files to Create

None. The fix reuses existing infrastructure.

Implementation Steps

1. Update findExpiredData() method:

   • Replace the try/catch block containing Deno.readDir() loops (lines 185-253)
   • Use this.dataRepo.findAllGlobal() to get all data across all types
   • Iterate over results and check expiration status
   • Build and return the ExpiredDataInfo[] array

2. Update deleteExpiredData() method:

   • Keep the expired data processing logic (lines 267-289) unchanged
   • Replace the version GC scanning logic (lines 291-325)
   • Use findAllGlobal() to collect unique type+modelId pairs
   • Call collectGarbage() for each unique pair

3. Add unit tests:

   • Update MockDataRepository to include a findAllGlobal mock method
   • Add tests for nested type handling in expiration scanning
   • Add tests for GC with nested types
   • Verify edge cases (empty results, errors during iteration)

4. Verify with existing tests:

   • Run deno run test src/domain/data/data_lifecycle_service_test.ts
   • Ensure all existing tests still pass

5. Run full verification suite:

   • deno check
   • deno lint
   • deno fmt
   • deno run test

Testing Strategy

Unit Tests (in data_lifecycle_service_test.ts):

1. Mock findAllGlobal() returning data with various type structures:

   • Simple types: swamp/echo
   • Nested types: command/shell, aws/ec2/vpc
   • Namespaced types: @docker/host, @user/custom/model

2. Test findExpiredData():

   • Verify it correctly processes all returned data
   • Verify it handles errors during expiration checks gracefully
   • Verify it returns the correct ExpiredDataInfo structure

3. Test deleteExpiredData():

   • Verify collectGarbage() is called for each unique type+modelId
   • Verify bytesReclaimed and versionsDeleted are summed correctly
   • Verify dryRun mode skips actual deletion

Integration Testing (manual or in integration/):

1. Create a test repository with nested type data:

   .swamp/data/command/shell/{model-id}/{data-name}/...
   .swamp/data/@docker/host/{model-id}/{data-name}/...
   

2. Run swamp data gc and verify expired data is cleaned up

Code Style Checklist

• TypeScript strict mode compliance
• No any types (using proper typing from existing interfaces)
• Named exports only (consistent with existing code)
• Uses @std/assert for test assertions
• AGPLv3 license header maintained
• Follows existing error handling patterns (console.error for non-fatal errors)

Potential Challenges

1. Performance with large data directories: The findAllGlobal() method reads all data entries. For repositories with many models, this could be slower than the previous approach. However, correctness is more important, and this is consistent with how other parts of the codebase handle similar scenarios.

2. Mock complexity: The MockDataRepository in tests will need to implement findAllGlobal(). This requires returning both the Data objects and their associated ModelType and modelId, which is more complex than the current mocks.

3. Backwards compatibility: No concerns - this is a bug fix that makes the existing API work correctly for all model types.

[stack72]

Proposed Fix

The UnifiedDataRepository interface already has findAllGlobal() which correctly handles arbitrary type nesting depth using recursive directory walking with isModelIdDirectory() detection. The fix is to replace the manual directory scanning in both findExpiredData() and deleteExpiredData() with calls to findAllGlobal().

This eliminates the repoDir constructor parameter from DefaultDataLifecycleService entirely since the service no longer does its own filesystem scanning.

Changes

src/domain/data/data_lifecycle_service.ts

• findExpiredData() (lines 181-255): Replace the 3-level readDir loop with findAllGlobal() iteration — for each data entry, check expiration and collect expired entries.
• deleteExpiredData() version GC loop (lines 293-325): Replace the second readDir loop with unique type+modelId pairs derived from findAllGlobal(), then call collectGarbage() on each.
• Constructor: Remove repoDir parameter — no longer needed since all directory scanning is delegated to the repository.
• Remove unused imports: join from @std/path, SWAMP_SUBDIRS, and swampPath.

src/cli/commands/data_gc.ts (line 66-70): Update constructor call to remove repoDir.

src/domain/data/data_lifecycle_service_test.ts: Update mock to include findAllGlobal, remove "/tmp/test" from constructors, add tests for findExpiredData with nested model types.

No user-facing changes

The DataLifecycleService interface stays the same — same methods, signatures, and return types. swamp data gc works identically. The fix means GC now correctly finds and cleans up expired data for nested types that was silently failing before.