Skip to content

Cache API verification results across CI runs #5042

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.

Sign up for GitHub

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

Merged
ahpook merged 4 commits into from
Mar 20, 2026
Merged

Cache API verification results across CI runs #5042

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa0901c
Cache API verification results across CI runs
kenyonj Feb 13, 2026
ba1b294
Address review feedback on cache persistence
kenyonj Feb 13, 2026
083835c
Merge branch 'main' into cache-api-results-across-runs
ahpook Mar 20, 2026
cd2fa52
Merge branch 'main' into cache-api-results-across-runs
ahpook Mar 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ jobs:
with:
bundler-cache: true

- name: Restore API cache
if: |
(matrix.test_type == 'collections' && steps.collections.outputs.changed) ||
(matrix.test_type == 'all' && steps.all.outputs.changed)
uses: actions/cache@v4
with:
path: .api-cache.json
key: api-cache-${{ matrix.test_type }}-${{ github.run_id }}
restore-keys: |
api-cache-${{ matrix.test_type }}-

- name: Build and test with Rake
if: |
(matrix.test_type == 'topics' && steps.topics.outputs.changed) ||
Expand Down
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ vendor
.bundle
.idea
.tool-versions
.api-cache.json
76 changes: 76 additions & 0 deletions test/test_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,79 @@ def add_message(type, file, line_number, message)
client.messages << "::#{type} file=#{file},line=#{line_number}::#{message}"
end

CACHE_FILE = File.expand_path("../.api-cache.json", File.dirname(__FILE__))
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

The CACHE_FILE path uses File.expand_path("../.api-cache.json", File.dirname(__FILE__)). Since __FILE__ is test/test_helper.rb, File.dirname(__FILE__) is test/, and ../.api-cache.json relative to test/ is .api-cache.json in the repository root. This is correct and matches the .gitignore entry and workflow path. However, using File.expand_path("../.api-cache.json", __dir__) would be more idiomatic Ruby.

Suggested change
CACHE_FILE = File.expand_path("../.api-cache.json", File.dirname(__FILE__))
CACHE_FILE = File.expand_path("../.api-cache.json", __dir__)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@kenyonj kenyonj Feb 13, 2026

Choose a reason for hiding this comment

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

Fixed — changed to File.expand_path("../.api-cache.json", __dir__). More idiomatic Ruby.

CACHE_TTL_SECONDS = 24 * 60 * 60 # 24 hours

def self.load_api_cache!
return unless File.exist?(CACHE_FILE)

data = JSON.parse(File.read(CACHE_FILE))
now = Time.now.to_i
ttl = CACHE_TTL_SECONDS

if data["repos"]
data["repos"].each do |key, entry|
next if now - entry["cached_at"].to_i > ttl
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

The TTL check compares now - entry["cached_at"].to_i > ttl, which skips entries that are older than the TTL. However, if the cache file is corrupted and entry["cached_at"] is missing or not a valid number, .to_i will return 0, making now - 0 a very large number, which will correctly skip the entry. This is safe, but consider adding an explicit check for missing or invalid cached_at values for clarity.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@kenyonj kenyonj Feb 13, 2026

Choose a reason for hiding this comment

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

Fixed — added explicit next unless cached_at guard before the TTL comparison in both repos and users loops. Makes the intent clear rather than relying on .to_i returning 0 for nil.


result = entry["value"]
# Reconstruct a minimal object that responds to .full_name
cached = if result.nil?
nil
else
Struct.new(:full_name).new(result["full_name"])
Comment on lines +290 to +296
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

If a cache entry's value is not nil but also doesn't have a full_name key (e.g., corrupted data), the code on line 292 will call .new(nil) since result["full_name"] will be nil. This creates a struct with full_name = nil, which might not match the expected behavior. Consider validating that required keys exist in the cached value before reconstructing the struct.

This issue also appears on line 303 of the same file.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@kenyonj kenyonj Feb 13, 2026

Choose a reason for hiding this comment

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

Fixed — added next unless result["full_name"] and next unless result["login"] validation before reconstructing the Struct, so corrupted/incomplete cache entries are skipped instead of creating Struct.new(...).new(nil).

end
NewOctokit.class_variable_get(:@@repos)[key] = cached
end
end

if data["users"]
data["users"].each do |key, entry|
next if now - entry["cached_at"].to_i > ttl

result = entry["value"]
cached = if result.nil?
nil
else
Struct.new(:login).new(result["login"])
end
NewOctokit.class_variable_get(:@@users)[key] = cached
end
end
rescue JSON::ParserError, StandardError => e
warn "Failed to load API cache: #{e.message}"
end

def self.save_api_cache!
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

The method should be defined as def save_api_cache! instead of def self.save_api_cache!. All other helper methods in this file are defined as instance methods without self., and this is being called as save_api_cache! on line 357 which expects an instance method. Using self. makes this a method on the main object's singleton class, which is inconsistent with the rest of the codebase.

This issue also appears on line 276 of the same file.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@kenyonj kenyonj Feb 13, 2026

Choose a reason for hiding this comment

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

Fixed — removed self. from both load_api_cache! and save_api_cache! so they're instance methods consistent with the rest of the helpers.

now = Time.now.to_i
repos_data = {}
users_data = {}

NewOctokit.class_variable_get(:@@repos).each do |key, value|
next if key == :skip_requests

repos_data[key.to_s] = {
"cached_at" => now,
"value" => value.nil? ? nil : { "full_name" => value.respond_to?(:full_name) ? value.full_name : value.to_s },
Comment on lines +332 to +334
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

When a cached entry has value.nil?, it's stored as nil in the JSON (line 325), and when loaded back (line 289-290), it's correctly reconstructed as nil. However, if a repository truly doesn't exist (NotFound), the code stores nil in the cache (line 92 in NewOctokit#repository), but this is indistinguishable from rate-limited entries where repos[item] = true is stored (line 90). When saving the cache, a rate-limited entry with value = true will be converted to { "full_name" => "true" } (line 325), which may cause issues when loading since true doesn't respond to .full_name correctly. Consider handling the true sentinel value explicitly.

This issue also appears on line 332 of the same file.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@kenyonj kenyonj Feb 13, 2026

Choose a reason for hiding this comment

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

Fixed — added next if value == true before serializing both repos and users entries, so rate-limit sentinel values are skipped and won't produce corrupted cache data like { "full_name" => "true" }.

}
end

NewOctokit.class_variable_get(:@@users).each do |key, value|
next if key == :skip_requests

users_data[key.to_s] = {
"cached_at" => now,
"value" => value.nil? ? nil : { "login" => value.respond_to?(:login) ? value.login : value.to_s },
}
end

File.write(CACHE_FILE, JSON.pretty_generate({ "repos" => repos_data, "users" => users_data }))
rescue StandardError => e
warn "Failed to save API cache: #{e.message}"
end

# Load cached API results at startup
load_api_cache!

Minitest.after_run do
warn "Repo checks were rate limited during this CI run" if NewOctokit.repos_skipped?
warn "User checks were rate limited during this CI run" if NewOctokit.users_skipped?
Expand All @@ -279,4 +352,7 @@ def add_message(type, file, line_number, message)
NewOctokit.messages.each do |message|
puts message
end

# Persist cache for next CI run
save_api_cache!
end
Loading