Skip to content

feat(processor): [TKC-6839] wrap a cached step in a restore and a save - #8345

Closed
vsukhin wants to merge 1 commit into
vsukhin/cache/9-control-planefrom
vsukhin/cache/10-processor
Closed

vsukhin wants to merge 1 commit into
vsukhin/cache/9-control-planefrom
vsukhin/cache/10-processor

Conversation

@vsukhin

@vsukhin vsukhin commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

The restore runs before the step and the save after it passes. A parent step with children gets one cache around all of them, which is usually what is wanted: caching each child separately packs and uploads the same tree more than once.

Save runs only when the step passed. Publishing a half-finished install under a content-hash key would have every later run restore the broken tree, and an entry cannot be replaced.

Cached paths must resolve before the pod starts, because the volumes are decided then while the toolkit resolves paths inside it; a path that can only be resolved in the pod is refused at build time rather than silently doing nothing.

Pull request description

Checklist (choose whats happened)

  • breaking change! (describe)
  • tested locally
  • tested on cluster
  • added new dependencies
  • updated the docs
  • added a test

Breaking changes

Changes

Fixes

@vsukhin
vsukhin added this pull request to stack #8336 September 17, 2026 20:16
@vsukhin
vsukhin requested a review from a team as a code owner September 17, 2026 20:16
@vsukhin
vsukhin requested review from caiomede-tk and removed request for a team September 17, 2026 20:16
@testkubebot

testkubebot Bot commented Sep 17, 2026

Copy link
Copy Markdown

✅ Testkube GitHub Integration

Review based on commit 1f93a1a.

All tests and quality gates passed.


Phase Status
Test Workflow Execution ✅ Passed
Quality Gate ✅ Passed

7 workflows executed

lint-go passed
in 8m5s (🚀 21. Sep. 2026 - 14:19:22 UTC / 🏁 21. Sep. 2026 - 14:27:27 UTC)

lint-proto passed
in 39s (🚀 21. Sep. 2026 - 14:19:22 UTC / 🏁 21. Sep. 2026 - 14:20:02 UTC)

integration-tests passed
in 11m18s (🚀 21. Sep. 2026 - 14:19:22 UTC / 🏁 21. Sep. 2026 - 14:30:40 UTC)

unit-tests passed
in 7m38s (🚀 21. Sep. 2026 - 14:19:22 UTC / 🏁 21. Sep. 2026 - 14:27:01 UTC)

verify-crds passed
in 3m11s (🚀 21. Sep. 2026 - 14:19:22 UTC / 🏁 21. Sep. 2026 - 14:22:33 UTC)

verify-protobuf passed
in 37s (🚀 21. Sep. 2026 - 14:19:22 UTC / 🏁 21. Sep. 2026 - 14:20:00 UTC)

lint-pr passed
in 33s (🚀 21. Sep. 2026 - 14:19:22 UTC / 🏁 21. Sep. 2026 - 14:19:56 UTC)


Manage this Integration

@greptile-apps

greptile-apps Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 3/5

The PR is not safe to merge because cache-enabled workflows invoke a nonexistent toolkit command, and an accepted cache configuration can mount an emptyDir over the container root.

Findings

  1. P1 Cache Command Is Missing
  2. P1 Root Mount Validation Bypass
  3. P2 Required Documentation Is Missing

Summary

The PR adds processor stages that mount cache paths, restore before step operations, and save successful results afterward. Two blocking runtime issues remain:

  • The generated stages invoke a toolkit cache command that is not implemented or registered in the shipped binary.
  • Relative . combined with a root cache working directory bypasses root-path validation and mounts an emptyDir at /.
  • Repository-required architecture documentation also needs updating.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  B[Bundle cached step] --> V[Validate declared paths]
  V --> M[Mount shared cache paths]
  M --> R[Run toolkit cache restore]
  R -->|cache command absent| F[Stage exits unsuccessfully]
  R --> S[Run step operations]
  S -->|passed| C[Run toolkit cache save]
Loading

Reviews (1) · Last reviewed commit: "feat(processor): wrap a cached step in a..."

// cacheToolkitCommand builds the toolkit invocation, passing the step's volume mounts so
// the walker knows which roots it may read and write.
func cacheToolkitCommand(container stage.Container, verb string) []string {
cmd := []string{constants.DefaultToolkitPath, "cache", verb}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Cache Command Is Missing

Every cache-enabled step invokes /.tktw/toolkit cache restore and later cache save, but the toolkit built and shipped by this repository does not register a cache command. Cobra will reject the first invocation, causing the cache stage to exit unsuccessfully instead of restoring the cache or continuing with an uncached run.

Comment on lines +63 to +65
cleaned := path.Clean(declared)
if cleaned == "/" {
return fmt.Errorf("cache.paths[%d]: %q: caching the container root is not supported", i, declared)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Root Mount Validation Bypass

Root-path validation runs before relative paths are resolved against cache.workingDir. A configuration with workingDir: / and paths: [.] therefore passes this check, but mountCachePaths later resolves the path to / and mounts an emptyDir over the container root. This hides the image filesystem from the step and bypasses the explicit prohibition on caching the container root.

Comment thread pkg/testworkflows/testworkflowprocessor/operations_cache.go
@vsukhin
vsukhin requested a review from a team as a code owner September 17, 2026 20:42
@vsukhin
vsukhin requested a review from tkonieczny September 17, 2026 20:42
@vsukhin
vsukhin force-pushed the vsukhin/cache/10-processor branch from eaa2f9f to 1cd2f5f Compare September 17, 2026 20:42
@vsukhin vsukhin changed the title feat(processor): wrap a cached step in a restore and a save feat(processor): [TKC-6839] wrap a cached step in a restore and a save Sep 18, 2026
@vsukhin
vsukhin force-pushed the vsukhin/cache/10-processor branch from 1cd2f5f to 23aac78 Compare September 18, 2026 11:28
@vsukhin
vsukhin force-pushed the vsukhin/cache/10-processor branch from 23aac78 to 5fa4144 Compare September 21, 2026 11:35
@vsukhin
vsukhin force-pushed the vsukhin/cache/10-processor branch 2 times, most recently from bb12a79 to ee0ad67 Compare September 21, 2026 12:17
@vsukhin
vsukhin force-pushed the vsukhin/cache/10-processor branch from ee0ad67 to 304cd66 Compare September 21, 2026 12:56
The restore runs before the step and the save after it passes. A parent step
with children gets one cache around all of them, which is usually what is
wanted: caching each child separately packs and uploads the same tree more than
once.

Save runs only when the step passed. Publishing a half-finished install under a
content-hash key would have every later run restore the broken tree, and an
entry cannot be replaced.

Cached paths must resolve before the pod starts, because the volumes are
decided then while the toolkit resolves paths inside it; a path that can only
be resolved in the pod is refused at build time rather than silently doing
nothing.

The two end-to-end workflows land here rather than with the rest of the
fixtures, because the bundling test in this package reads them: a test and the
file it opens have to arrive together or the layer does not pass on its own.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vsukhin
vsukhin force-pushed the vsukhin/cache/10-processor branch from 304cd66 to 1f93a1a Compare September 21, 2026 14:17
@vsukhin vsukhin closed this Sep 21, 2026
@vsukhin
vsukhin deleted the vsukhin/cache/10-processor branch September 21, 2026 15:26
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.

1 participant