Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix shared layer logic #364

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Fix shared layer logic #364

merged 1 commit into from
Dec 11, 2024

Conversation

schneems
Copy link
Contributor

@schneems schneems commented Dec 11, 2024

There was a bug reported where deploying a Ruby application after modifying the Gemfile.lock would result in an error because we were skipping running bundle install. Debugging this lead me to find that we were returning the incorrect value from our shared cache logic (accidentally returning the currnt/now value rather than the old value).

The idea behind cached_layer_write_metadata is to either return the old cached data or return a message stating why it couldn't be returned. It was accidentally returning the new/current value instead. This feature is used in most (nearly all) of the layers, like

LayerState::Restored { cause } => {
bullet = bullet.sub_bullet(cause);
match cause {
Meta::Data(old) => install_state(old, metadata),
Meta::Message(_) => InstallState::Run(String::new()),
}
}
. In this logic there are some values in the metadata that trigger a cache clear
let Metadata {
distro_name,
distro_version,
cpu_architecture,
ruby_version,
force_bundle_install_key: _,
digest: _,
} = old;
. The rest of the values are used to persist data between runs. In this case we're looking at Gemfile and Gemfile.lock SHAs to see if they changed. The behavior fails when the current metadata object is returned rather than the old one, because that means the comparison between the value returned and the current metadata will always be identical.

I added a unit test to assert this behavior. While developing I used an integration test to debug the issue (given below).

This commit fixes the integration test given below:

Gemfile.lock cache invalidation integration reproduction

$ cargo test test_gemfile_lock_invalidates_cache -- --exact
#[test]
fn test_gemfile_lock_invalidates_cache() {
    let app_dir = tempfile::tempdir().unwrap();
    fs_err::write(
        app_dir.path().join("Gemfile"),
        r#"
        source "https://rubygems.org"

        gem "rake"
    "#,
    )
    .unwrap();

    fs_err::write(
        app_dir.path().join("Gemfile.lock"),
        r"
GEM
  remote: https://rubygems.org/
  specs:
    rake (13.2.0)

PLATFORMS
  ruby

DEPENDENCIES
  rake
",
    )
    .unwrap();
    let mut config = amd_arm_builder_config("heroku/builder:24", &app_dir.path().to_string_lossy());
    TestRunner::default().build(
        config.clone()
        .buildpacks([
            BuildpackReference::CurrentCrate,
        ]),
        |context| {
            let stdout = context.pack_stdout.clone();
            println!("{}", stdout);
            assert_contains!(stdout, "# Heroku Ruby Buildpack");
            assert_contains!(
                stdout,
                r#"`BUNDLE_BIN="/layers/heroku_ruby/gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/gems" BUNDLE_WITHOUT="development:test" bundle install`"#
            );
            assert_contains!(stdout, "Installing rake");

    fs_err::write(
        app_dir.path().join("Gemfile.lock"),
        r"
GEM
  remote: https://rubygems.org/
  specs:
    rake (13.2.1)

PLATFORMS
  ruby

DEPENDENCIES
  rake
",
    )
    .unwrap();

            context.rebuild(
                config.buildpacks([BuildpackReference::CurrentCrate]),
                |rebuild_context| {
                    println!("{}", rebuild_context.pack_stdout);

                assert_contains!(
                    rebuild_context.pack_stdout,
                    r#"`BUNDLE_BIN="/layers/heroku_ruby/gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/gems" BUNDLE_WITHOUT="development:test" bundle install`"#
                );
                assert_contains!(rebuild_context.pack_stdout, "Installing rake");
                },
            );
        });
}

There was a bug reported where deploying a Ruby application after modifying the Gemfile.lock would result in an error because we were skipping running `bundle install`. Debugging this lead me to find that we were returning the incorrect value from our shared cache logic (accidentally returning the currnt/now value rather than the old value).

The idea behind `cached_layer_write_metadata` is to either return the old cached data or return a message stating why it couldn't be returned. It was accidentally returning the new/current value instead.

I added a unit test to assert this behavior. While developing I used an integration test to debug the issue (given below).

This commit fixes the integration test given below:

## Gemfile.lock cache invalidation integration reproduction


```
$ cargo test test_gemfile_lock_invalidates_cache -- --exact
```

```
#[test]
fn test_gemfile_lock_invalidates_cache() {
    let app_dir = tempfile::tempdir().unwrap();
    fs_err::write(
        app_dir.path().join("Gemfile"),
        r#"
        source "https://rubygems.org"

        gem "rake"
    "#,
    )
    .unwrap();

    fs_err::write(
        app_dir.path().join("Gemfile.lock"),
        r"
GEM
  remote: https://rubygems.org/
  specs:
    rake (13.2.0)

PLATFORMS
  ruby

DEPENDENCIES
  rake
",
    )
    .unwrap();
    let mut config = amd_arm_builder_config("heroku/builder:24", &app_dir.path().to_string_lossy());
    TestRunner::default().build(
        config.clone()
        .buildpacks([
            BuildpackReference::CurrentCrate,
        ]),
        |context| {
            let stdout = context.pack_stdout.clone();
            println!("{}", stdout);
            assert_contains!(stdout, "# Heroku Ruby Buildpack");
            assert_contains!(
                stdout,
                r#"`BUNDLE_BIN="/layers/heroku_ruby/gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/gems" BUNDLE_WITHOUT="development:test" bundle install`"#
            );
            assert_contains!(stdout, "Installing rake");

    fs_err::write(
        app_dir.path().join("Gemfile.lock"),
        r"
GEM
  remote: https://rubygems.org/
  specs:
    rake (13.2.1)

PLATFORMS
  ruby

DEPENDENCIES
  rake
",
    )
    .unwrap();

            context.rebuild(
                config.buildpacks([BuildpackReference::CurrentCrate]),
                |rebuild_context| {
                    println!("{}", rebuild_context.pack_stdout);

                assert_contains!(
                    rebuild_context.pack_stdout,
                    r#"`BUNDLE_BIN="/layers/heroku_ruby/gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/gems" BUNDLE_WITHOUT="development:test" bundle install`"#
                );
                assert_contains!(rebuild_context.pack_stdout, "Installing rake");
                },
            );
        });
}
```
@schneems schneems force-pushed the schneems/gemfile-lock-digest-fix branch from 5e122ab to e37006b Compare December 11, 2024 18:16
@schneems schneems changed the base branch from main to schneems/cargo-fmt-clippy-dec-11 December 11, 2024 19:19
@schneems schneems changed the title Fix shared layer logic [stacked] Fix shared layer logic Dec 11, 2024
@schneems schneems force-pushed the schneems/gemfile-lock-digest-fix branch from e37006b to 6c9c53a Compare December 11, 2024 19:22
@schneems schneems marked this pull request as ready for review December 11, 2024 19:26
@schneems schneems requested a review from a team as a code owner December 11, 2024 19:26
Base automatically changed from schneems/cargo-fmt-clippy-dec-11 to main December 11, 2024 19:45
@schneems schneems changed the title [stacked] Fix shared layer logic Fix shared layer logic Dec 11, 2024
@schneems schneems force-pushed the schneems/gemfile-lock-digest-fix branch from 6c9c53a to e37006b Compare December 11, 2024 19:46
@schneems schneems merged commit 51f0399 into main Dec 11, 2024
11 of 12 checks passed
@schneems schneems deleted the schneems/gemfile-lock-digest-fix branch December 11, 2024 20:26
heroku-linguist bot added a commit that referenced this pull request Dec 11, 2024
## heroku/ruby

### Fixed

- A bug introduced in 4.0.0 would result in incorrectly skipping running `bundle install` when the `Gemfile` or `Gemfile.lock` or environment variables had changed. The bug is now fixed. ([#364](#364))
@heroku-linguist heroku-linguist bot mentioned this pull request Dec 11, 2024
heroku-linguist bot added a commit to heroku/cnb-builder-images that referenced this pull request Dec 11, 2024
## heroku/ruby

### Fixed

- A bug introduced in 4.0.0 would result in incorrectly skipping running `bundle install` when the `Gemfile` or `Gemfile.lock` or environment variables had changed. The bug is now fixed. ([#364](heroku/buildpacks-ruby#364))
heroku-linguist bot added a commit to heroku/cnb-builder-images that referenced this pull request Dec 11, 2024
## heroku/ruby

### Fixed

- A bug introduced in 4.0.0 would result in incorrectly skipping running `bundle install` when the `Gemfile` or `Gemfile.lock` or environment variables had changed. The bug is now fixed. ([#364](heroku/buildpacks-ruby#364))

Co-authored-by: heroku-linguist[bot] <136119646+heroku-linguist[bot]@users.noreply.github.com>
schneems pushed a commit to BrianBorge/buildpacks-ruby that referenced this pull request Dec 13, 2024
## heroku/ruby

### Fixed

- A bug introduced in 4.0.0 would result in incorrectly skipping running `bundle install` when the `Gemfile` or `Gemfile.lock` or environment variables had changed. The bug is now fixed. ([heroku#364](heroku#364))

Co-authored-by: heroku-linguist[bot] <136119646+heroku-linguist[bot]@users.noreply.github.com>
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.

2 participants