Skip to content

fix: image ref parse consistency in storage - #247

Open
scook12 wants to merge 7 commits into
smol-machines:mainfrom
scook12:fix/image-ref-parser
Open

fix: image ref parse consistency in storage#247
scook12 wants to merge 7 commits into
smol-machines:mainfrom
scook12:fix/image-ref-parser

Conversation

@scook12

@scook12 scook12 commented May 10, 2026

Copy link
Copy Markdown

#246

I ran into this ^ while working on a project that has smolvm upstream and ended up down the rabbit hole. This is one of my first "real" chunks of work in Rust, so don't be shy about rejecting/tearing down the PR.

Thanks for reading!

Changes:

  • extract image ref parsing to dedicated module
  • ensure the storage layer consistently uses canonical form of all image refs with wiring in main/storage
  • extract the pull/use cache decision to a small, testable layer between query_image and call sites
  • unit + integration tests
  • unrelated: some cfg flags to avoid cstr/libcmount type errors on macOS

TODO:

  • change Reference['digest'] over to Option<String>
  • fill in empty test stubs
  • better/complete doc strings
  • there's small duplication in oci.rs - review, find any todos
  • fix fail branch; always clean and pull on error

);
return CacheDecision::UseCache(info);
}
Err(e) => CacheDecision::Fail(format!("cache query failed: {e}")),

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is a tidbit I'm uncertain about; is there a reason to fail when manifest parsing errors out? This can easily be swapped out for just pulling instead

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.

Are there cases that lead to manifest failures where it is still wise to continue to pull?

My thinking is that there isn't a good reason to fail to parse the manifest ever unless something unrecoverable has happened.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah, it does seem like in most cases we'd just end up pulling instead of panicking. If we have an io/parse error, we can always just remove the manifest and pull, similar to how we do when there's missing layers.

If we panicked and didn't remove the manifest when it's corrupted or otherwise blocking the pull, then subsequent pulls for that image would always fail which seems undesirable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

At call site, we now handle the Err branch by performing cleanup

@scook12
scook12 marked this pull request as ready for review May 12, 2026 21:52
@scook12
scook12 marked this pull request as draft May 12, 2026 21:52
@stage-review

stage-review Bot commented May 27, 2026

Copy link
Copy Markdown

Ready to review this PR? Stage has broken it down into 4 individual chapters for you:

Title
1 Add regex dependency and platform compatibility flags
2 Implement canonical image reference parsing module
3 Refactor storage to use canonical image references
4 Extract cache resolution logic and add tests
Open in Stage

Chapters generated by Stage for commit e9fe1cf on May 28, 2026 7:25pm UTC.

@scook12
scook12 marked this pull request as ready for review May 28, 2026 19:25
@scook12
scook12 requested a review from BinSquare May 28, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants