Store MP3s in file cache + HOT & COLD instances #44
Conversation
|
Warning Rate limit exceeded@gerhard has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (75)
📝 WalkthroughWalkthroughConsolidates CI acceptance steps; adds Fly.io per-app artifacts and Just recipes; refactors Varnish into modular includes with dynamic backends and disk-backed file cache support; extends Dagger varnish options; updates VCL, tests, and logging to surface storage and keep metadata. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Varnish
participant Director as DynamicDirector
participant Backend as Backend(Service)
participant Storage as Storage
Note over Client,Varnish: Request handled by modular VCL with dynamic backend selection
Client->>Varnish: HTTP request (Host, Path, headers)
alt Health endpoint ("/health")
Varnish-->>Client: synth 204
else Normal request
Varnish->>Director: determine backend (includes: app/assets/feeds, host/path rules)
Director->>Backend: forward request (adds X-backend headers)
Backend-->>Varnish: beresp (status, headers, body)
Varnish->>Varnish: vcl_backend_response sets beresp.ttl/grace/keep & beresp.storage
Varnish->>Storage: persist object (memory or disk)
Varnish-->>Client: deliver response (includes cache-status with storage/keep/region/hits)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
gerhard
force-pushed
the
store-mp3s-in-file-cache
branch
from
2ec8905 to
278f61a
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
test/vtc/purge.vtc (1)
28-33: LGTM!The test correctly validates successful PURGE with the token matching the environment variable.
Optionally, consider adding edge case tests such as:
- Case sensitivity of the token
- Behavior when
PURGE_TOKENenvironment variable is not set- Verification that the purge actually clears cached content
varnish/vcl/practicalai.vcl (1)
8-26: Consider consolidating the redirect logic.Both redirect paths share identical structure. While the current implementation is clear and functional, you could reduce duplication by computing the target URL and HTML body in variables.
🔎 Proposed refactor to reduce duplication
sub vcl_synth { + declare local var.redirect_url STRING; + declare local var.redirect_html STRING; + if (req.url == "/practicalai/feed" - && resp.status == 301) { - set resp.http.location = "https://feeds.transistor.fm/practical-ai-machine-learning-data-science-llm"; - set resp.body = {" - <html><body>You are being <a href="https://feeds.transistor.fm/practical-ai-machine-learning-data-science-llm">redirected</a>.</body></html> - "}; - return(deliver); - } - - if (req.url == "/practicalai" && resp.status == 301) { - set resp.http.location = "https://practicalai.fm"; - set resp.body = {" - <html><body>You are being <a href="https://practicalai.fm">redirected</a>.</body></html> - "}; + if (req.url == "/practicalai/feed") { + set var.redirect_url = "https://feeds.transistor.fm/practical-ai-machine-learning-data-science-llm"; + } else if (req.url == "/practicalai") { + set var.redirect_url = "https://practicalai.fm"; + } + set resp.http.location = var.redirect_url; + set resp.body = "<html><body>You are being <a href=""" + var.redirect_url + """>redirected</a>.</body></html>"; return(deliver); } }varnish/vcl/feeds-backend.vcl (1)
16-16: Address the TODO comment.The comment asks about uploading feed.json. This suggests incomplete functionality.
Do you want me to help generate a solution for handling feed.json uploads, or should this TODO be tracked in a separate issue?
dagger/main.go (1)
143-156: Consider validating varnishCacheSize format.The varnishCacheSize parameter is used directly in environment variables and file paths without validation. Invalid formats (e.g., missing unit suffix, negative values) could cause runtime errors in Varnish.
💡 Validation suggestion
Consider adding basic format validation before using the size parameter:
if varnishFileCache { // Validate size format (e.g., "100M", "1G") if !regexp.MustCompile(`^\d+[KMGkmg]$`).MatchString(varnishCacheSize) { return nil, fmt.Errorf("invalid varnish cache size format: %s (expected format: <number><unit>, e.g., 100M, 1G)", varnishCacheSize) } varnishDefaultSize, err := pipely.Varnish.EnvVariable(ctx, "VARNISH_SIZE") // ... rest of implementation }This would catch configuration errors early rather than at runtime.
varnish/vcl/default.vcl (1)
59-81: Environment-driven BERESP overrides are clearly documented for dev/testing.The comments clearly indicate these overrides are "only recommended for dev/testing" and will overwrite backend-specific settings. This is a reasonable pattern for testing flexibility.
Consider adding production safeguards if these environment variables could accidentally be set in production (e.g., log a warning when they're active).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.godocs/local_dev.mdfly.io/cdn-2025-02-05/.envrcfly.io/cdn-2025-02-05/fly.tomlfly.io/cdn-2025-02-05/justfilefly.io/cdn-2025-02-05/machines.txtfly.io/cdn-2025-02-05/regions.txtfly.io/cdn-2025-02-05/sizes.txtfly.tomljust/_config.justjust/_config_fly.justjust/fly.justjust/hurl.justjust/op.justjustfileregions.txttest/acceptance/assets.hurltest/acceptance/fastly/admin.hurltest/acceptance/fastly/feed.hurltest/acceptance/fastly/feeds.hurltest/acceptance/fastly/homepage.hurltest/acceptance/fastly/www.hurltest/acceptance/local/http.hurltest/acceptance/local/www.hurltest/acceptance/news-mp3.hurltest/acceptance/production/admin.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/acceptance/production/health.hurltest/acceptance/production/homepage.hurltest/vtc/app-backend.vtctest/vtc/assets-backend.vtctest/vtc/assets.vtctest/vtc/cache-status.vtctest/vtc/client-ip.vtctest/vtc/default.vtctest/vtc/feeds-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/fly/request-id.vtctest/vtc/health.vtctest/vtc/http.vtctest/vtc/news-mp3.vtctest/vtc/practicalai.vtctest/vtc/purge.vtctest/vtc/websockets.vtctest/vtc/www.vtcvarnish/changelog.com.vclvarnish/varnish-json-response.bashvarnish/vcl/app-backend.vclvarnish/vcl/assets-backend.vclvarnish/vcl/backend-health.vclvarnish/vcl/default.vclvarnish/vcl/disable-caching-for-5xx.vclvarnish/vcl/disable-default-backend.vclvarnish/vcl/feeds-backend.vclvarnish/vcl/fly/client-ip.vclvarnish/vcl/fly/region.vclvarnish/vcl/fly/request-id.vclvarnish/vcl/http.vclvarnish/vcl/news-mp3.vclvarnish/vcl/practicalai.vclvarnish/vcl/purge.vclvarnish/vcl/test/disable-caching-for-testing.vclvarnish/vcl/varnish-health.vclvarnish/vcl/www.vclvector/changelog.com/debug_s3.yamlvector/changelog.com/debug_varnish.yamlvector/changelog.com/debug_varnish_geoip.yamlvector/changelog.com/default.yamlvector/changelog.com/geoip.yaml
💤 Files with no reviewable changes (11)
- test/acceptance/fastly/feed.hurl
- test/vtc/health.vtc
- test/vtc/assets.vtc
- regions.txt
- test/acceptance/fastly/feeds.hurl
- fly.toml
- test/vtc/cache-status.vtc
- test/acceptance/fastly/homepage.hurl
- test/acceptance/fastly/www.hurl
- test/acceptance/fastly/admin.hurl
- test/vtc/client-ip.vtc
✅ Files skipped from review due to trivial changes (2)
- fly.io/cdn-2025-02-05/justfile
- test/acceptance/local/www.hurl
🚧 Files skipped from review as they are similar to previous changes (23)
- test/acceptance/production/feed.hurl
- test/vtc/practicalai.vtc
- just/_config_fly.just
- test/vtc/fly/request-id.vtc
- test/acceptance/production/homepage.hurl
- varnish/vcl/purge.vcl
- test/acceptance/local/http.hurl
- .github/workflows/_github.yml
- fly.io/cdn-2025-02-05/sizes.txt
- .github/workflows/_namespace.yml
- just/_config.just
- fly.io/cdn-2025-02-05/machines.txt
- test/vtc/assets-backend.vtc
- varnish/vcl/assets-backend.vcl
- varnish/vcl/varnish-health.vcl
- docs/local_dev.md
- varnish/vcl/app-backend.vcl
- test/vtc/websockets.vtc
- just/fly.just
- varnish/varnish-json-response.bash
- test/vtc/default.vtc
- varnish/vcl/http.vcl
- varnish/vcl/disable-default-backend.vcl
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-03T08:51:11.320Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 18
File: dagger/main.go:292-304
Timestamp: 2025-07-03T08:51:11.320Z
Learning: When Go tools in dagger/main.go are pinned to specific commit hashes rather than release tags, this is intentional to access newer changes not yet included in releases. Avoid suggesting updates to release tags when commits are used, as the commits likely contain important unreleased features or fixes.
Applied to files:
dagger/main.go
🪛 LanguageTool
fly.io/cdn-2025-02-05/regions.txt
[uncategorized] ~32-~32: Did you mean “São Paulo” (= city in Brazil)?
Context: ... South America Sao Paulo, Brazil gru 11...
(SAO_PAOLO)
🔇 Additional comments (38)
test/vtc/purge.vtc (3)
3-3: LGTM!Setting the
PURGE_TOKENenvironment variable is a clean approach for externalizing configuration, and the value is correctly used in the test on line 30.
12-26: LGTM!Both test cases correctly validate that unauthorized PURGE requests (missing or invalid token) return a 401 status with an appropriate error message.
5-10: The included VCL files exist and correctly implement the expected behavior.Both
disable-default-backend.vclandpurge.vclare properly located invarnish/vcl/and correctly implemented. Thepurge.vclfile imports thestdmodule and validates thePURGE_TOKENenvironment variable invcl_recv, returning a 401 response if the token is invalid. Thedisable-default-backend.vclfile correctly disables the default backend. The refactoring to use modular VCL includes is sound.test/vtc/fly/client-ip.vtc (1)
1-86: Test coverage is comprehensive but only validates logging.The test suite is well-structured and covers all client IP precedence scenarios thoroughly. However, the tests only verify
VCL_Logoutput, not whether the determined client IP is actually stored and available for use by downstream code.Once the VCL code is updated to store the determined IP (e.g., in
req.http.x-client-ip), consider enhancing these tests to verify the stored value. For example:varnish v1 -vcl+backend { vcl 4.1; include "fly/client-ip.vcl"; sub vcl_deliver { # Echo back the stored client IP for testing set resp.http.x-client-ip = req.http.x-client-ip; } } -startThen add assertions like:
client c1 { txreq -url "/" -hdr "fly-client-ip: 151.101.129.162" rxresp expect resp.status == 200 expect resp.http.x-client-ip == "151.101.129.162" } -runThis ensures the IP is not only logged correctly but also stored and usable by the rest of the VCL logic.
just/op.just (1)
16-22: Color token refactoring matches hurl.just.The color variable refactoring is consistent with the changes in
just/hurl.just. The same verification of variable definitions applies here.test/acceptance/assets.hurl (1)
56-56: LGTM! Disk storage validation for MP3 chunks.The new assertions correctly validate that MP3 assets are cached on disk as per the PR objectives. Testing first, middle, and end chunks ensures comprehensive coverage of range request behavior.
Also applies to: 70-70, 84-84
varnish/changelog.com.vcl (1)
662-663: LGTM! Header name normalization.The change from
Locationtolocationimproves consistency with Varnish's internal header handling, which uses lowercase. HTTP header names are case-insensitive, so this is functionally equivalent.Also applies to: 686-686, 708-708
test/vtc/http.vtc (1)
11-11: LGTM! Test updates align with VCL refactoring.The switch to relative includes and the header name normalization are consistent with the broader modular VCL refactoring across this PR. The host update to
pipely.localimproves test clarity.Also applies to: 30-30, 33-33
varnish/vcl/test/disable-caching-for-testing.vcl (1)
1-5: LGTM! Clean test utility.This focused test-specific VCL module effectively disables caching for testing scenarios. The modular approach aligns with the PR's broader VCL refactoring objectives.
container/justfile (5)
36-36: LGTM! Improved varnishstat output.Adding the
-1flag provides single-shot output, which is more suitable for scripting and one-time inspection.
40-40: LGTM! Recursive VTC test discovery.The addition of
test/vtc/**/*.vtcenables testing of VTC files in subdirectories, supporting better test organization.
52-52: LGTM! Expanded test coverage.Including
test/acceptance/production/*.hurlin the local acceptance tests ensures production-level test coverage during local development.
56-56: LGTM! Enhanced cache metrics.The expanded filter set (
MAIN.cache*,MAIN.shm*,SMA.*,SMF.*) provides comprehensive visibility into memory and file-based storage metrics, which is essential for monitoring the disk cache feature introduced in this PR.
66-84: LGTM! Improved benchmark organization.The renaming to a numbered sequence (app-2-fly, app-3-local, feed-2-fly, feed-3-local) creates a clearer progression and improves discoverability.
README.md (2)
19-21: LGTM! Roadmap accurately updated.The documentation correctly reflects the completion of the MP3 file cache and HOT/COLD instances feature with a link to this PR.
94-112: LGTM! Recipe documentation synchronized.The recipe list is properly synchronized with the changes made to
container/justfile, ensuring documentation accuracy.varnish/vcl/feeds-backend.vcl (1)
115-131: LGTM: Cache configuration is appropriate for feeds.The memory-only storage, 12h TTL, 1d grace, and 7d keep settings are well-suited for RSS/XML feeds that update periodically but should remain available during backend issues.
dagger/main.go (2)
238-243: LGTM: Dynamic Varnish options integration is well-implemented.The Procfile template correctly uses
strings.Jointo inject VarnishOptions, enabling flexible runtime configuration of Varnish parameters.
314-338: LGTM: Vector configuration path consolidation aligns with PR objectives.The path changes from
vector/pipedream.changelog.com/tovector/changelog.com/correctly reflect the consolidation mentioned in the PR summary.varnish/vcl/www.vcl (1)
21-21: LGTM: Header name normalization.Lowercasing the
locationheader name is conventional and aligns with the header normalization across other VCL files and tests in this PR.varnish/vcl/backend-health.vcl (1)
1-31: LGTM: Health probe configuration is correct.The health probe parameters are well-documented and correctly configured. The threshold of 5 successful probes out of a 10-probe window provides reasonable fault tolerance (50% success rate required).
Note: The past review comment about incorrect documentation is now obsolete—the current code and comments are aligned.
test/acceptance/news-mp3.hurl (1)
1-251: LGTM: Test assertions updated for header normalization.All assertions correctly updated to expect lowercase
locationheaders, maintaining consistency with the VCL changes throughout this PR.varnish/vcl/disable-caching-for-5xx.vcl (1)
1-16: LGTM: Proper 5xx handling with graceful degradation.The implementation correctly prevents caching of 5xx responses while preserving previously cached objects during background fetches. The inline comments clearly explain the bgfetch abandonment logic.
test/vtc/www.vtc (1)
3-16: LGTM!The test correctly verifies the www redirect behavior using the modular VCL includes. The test structure is clean, with proper VCL initialization and a straightforward client assertion for the 301 redirect from
www.pipely.localtohttps://pipely.local/.test/vtc/app-backend.vtc (1)
1-41: LGTM!Well-structured test that correctly validates the app backend routing. The test properly:
- Sets up environment variables for dynamic backend configuration
- Configures both memory and disk storage (aligning with the file cache feature)
- Verifies the
/app_health→/healthURL rewrite behaviortest/vtc/news-mp3.vtc (2)
46-52: Good addition: x-redirect header ignore test.This test case validates that the
x-redirectheader from untrusted sources is properly ignored, preventing potential redirect manipulation. The 308 redirect to the expected assets path confirms the correct behavior.
1-68: LGTM!The test file is well-structured with comprehensive coverage for MP3 redirect scenarios. The updates properly align with the modular VCL approach and use the normalized lowercase
locationheader.test/vtc/feeds-backend.vtc (2)
276-306: LGTM!The feeds backend test setup correctly configures the environment and Varnish instance. The inclusion of
test/disable-caching-for-testing.vclappropriately disables caching to ensure each request hits the mock backend for assertion purposes.
1-274: Comprehensive feed endpoint coverage.The mock server covers all feed path variations (base, trailing slash, query strings) for the complete set of podcast feeds. While verbose, this thoroughness ensures reliable regression testing for feed URL routing.
fly.io/cdn-2025-02-05/fly.toml (3)
1-4: Verify the directory/app name date mismatch is intentional.The directory is named
cdn-2025-02-05but the app name iscdn-2025-02-25. If this is intentional versioning, consider adding a comment explaining the naming convention to avoid confusion.
13-15: Good cache sizing with documented rationale.The relationship between
VARNISH_FILE_SIZE(45G) and mountinitial_size(50gb) is well-documented with the 90%/+10% comments, ensuring the file cache has adequate headroom.Also applies to: 46-50
40-44: Concurrency limits properly documented.The comment linking
hard_limit = 2700to Varnish'sthread_pools * thread_pool_maxprovides valuable operational context for future tuning.fly.io/cdn-2025-02-05/.envrc (1)
1-12: LGTM!The HOT/COLD resource split is well-configured:
- HOT instances get 2x CPU and 2x memory (2 CPUs, 16GB) compared to default (1 CPU, 8GB)
- Region configuration is comprehensive with commented alternatives for testing flexibility
This aligns well with the PR objective of splitting Varnish instances into HOT (high traffic) and COLD tiers.
varnish/vcl/default.vcl (2)
11-40: LGTM!The dynamic director configuration is well-structured with appropriate timeouts:
- App backend has extended
first_byte_timeout = 300sfor MP3 uploads- Feeds and assets have standard 10s timeouts
- Health probes attached to all directors
106-117: Typo fixed and storage reporting added.The previous "objet" typo is now corrected to "object", and the new storage type reporting (
obj.storage) provides valuable cache observability for debugging file vs memory cache behavior.justfile (3)
41-41: Verify--network hostdoesn't break local testing on macOS.The switch from port publishing (
-p) to--network hostworks differently on Linux vs macOS (where host networking is not fully supported in Docker Desktop). Ensure this doesn't affect local development workflows.
61-75: LGTM: Consolidated acceptance testing to production.The rename from
test-acceptance-pipedreamtotest-acceptance-productionaligns with the PR objective of removing pipedream.changelog.com and consolidating to changelog.com.
136-137: APP_IMAGE is properly defined. The variable is correctly defined injust/_config.justwith a sensible default value (ghcr.io/thechangelog/pipely), so the switch fromFLY_APP_IMAGEtoAPP_IMAGEis appropriate.
gerhard
force-pushed
the
store-mp3s-in-file-cache
branch
from
278f61a to
d3c02d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
container/justfile (1)
43-52: Remove production tests fromtest-acceptance-localtarget.The production test files contain assertions checking for infrastructure-specific headers (Cloudflare
cf-ray, Fly.iofly-request-id, Varnish cache headers with region/origin/ttl/grace/keep/storage values). These tests will fail when run againstlocalhost:9000because a local environment lacks the production infrastructure (Cloudflare, Fly.io, Varnish). Production tests should run only against the production environment, not in the local acceptance suite.
🧹 Nitpick comments (4)
test/vtc/feeds-backend.vtc (1)
282-283: Consider adding a dedicated test for MP3 disk caching behavior.While the storage backends are correctly configured (memory + disk), caching is disabled in this test (line 296), so the PR's core objective—verifying that MP3s use disk storage while other content uses memory—isn't exercised here.
Do you have a separate test that validates MP3 files are stored in the disk cache and that other content (e.g., XML feeds) uses memory? If not, consider adding a test case that:
- Enables caching
- Requests an MP3 file
- Verifies it's stored in the disk storage backend
- Requests a non-MP3 asset
- Verifies it's stored in the memory storage backend
You can verify this by checking
varnishstatcounters or adding VCL logging that tracks which storage backend is selected.just/fly.just (1)
12-26: Consider usingFLY_APPinstead of hardcoded 'pipely'.The
awkpattern on lines 15 and 67 uses hardcoded'pipely'to filter machines. For consistency and maintainability, consider using theFLY_APPvariable:- | awk '/pipely/ { print $1, $5 }' \ + | awk '/{{ FLY_APP }}/ { print $1, $5 }' \This would ensure the script works correctly if the app name changes.
varnish/vcl/assets-backend.vcl (1)
46-70: Core caching logic looks solid.The implementation correctly achieves the PR's main goal:
- MP3s stored on disk to prevent memory fragmentation
- Streaming enabled for efficient large file delivery
- ESI disabled appropriately for binary content
- Reasonable TTL/grace/keep values (overridable via environment variables)
Optional: Consider case-insensitive MP3 detection
The current regex
bereq.url ~ "\.mp3"is case-sensitive and won't match uppercase extensions like.MP3or.Mp3. While MP3 files typically use lowercase extensions, you could make this more robust:- if (bereq.url ~ "\.mp3") { + if (bereq.url ~ "(?i)\.mp3") {This is a minor edge case and entirely optional.
dagger/main.go (1)
144-161: Move validation outside the conditional and simplify the logic.The validation at line 146 only runs when
varnishFileCacheis true, butvarnishCacheSizeis always used for memory cache at line 139. If an invalid size format is passed without enabling file cache, Varnish will fail at startup with an unclear error.Additionally, lines 149-152 read back
VARNISH_SIZEfrom the container immediately after setting it at line 139. This adds unnecessary complexity.🔎 Proposed refactor
pipely.Varnish = dag.Container().From("varnish:"+varnishVersion). WithUser("root"). WithEnvVariable("VARNISH_HTTP_PORT", fmt.Sprintf("%d", varnishPort)). WithEnvVariable("VARNISH_SIZE", fmt.Sprintf("%s", varnishCacheSize)). WithExposedPort(varnishPort). WithEnvVariable("HONEYCOMB_DATASET", honeycombDataset). WithEnvVariable("AWS_REGION", awsRegion) + // Validate cache size format for all use cases (memory and file cache) + if !regexp.MustCompile(`^\d+[KMGkmg]$`).MatchString(varnishCacheSize) { + return nil, fmt.Errorf("invalid varnish cache size format: %s (expected format: <number><unit>, e.g., 100M, 1G)", varnishCacheSize) + } + if varnishFileCache { - // Validate size format (e.g., "100M", "1G") - if !regexp.MustCompile(`^\d+[KMGkmg]$`).MatchString(varnishCacheSize) { - return nil, fmt.Errorf("invalid varnish cache size format: %s (expected format: <number><unit>, e.g., 100M, 1G)", varnishCacheSize) - } - varnishDefaultSize, err := pipely.Varnish.EnvVariable(ctx, "VARNISH_SIZE") - if err != nil { - return nil, err - } - - fileCacheDir := "/var/lib/varnish/cache/" + fileCacheDir := "/var/lib/varnish/cache" pipely.Varnish = pipely.Varnish. - WithEnvVariable("VARNISH_FILE_SIZE", varnishDefaultSize). + WithEnvVariable("VARNISH_FILE_SIZE", varnishCacheSize). WithExec([]string{"mkdir", "-p", fileCacheDir}) option := fmt.Sprintf("-s disk=file,%s/file.bin,$VARNISH_FILE_SIZE", fileCacheDir) pipely.VarnishOptions = append(pipely.VarnishOptions, option) }This also removes the trailing slash from
fileCacheDirto avoid the double-slash path.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.godocs/local_dev.mdfly.io/cdn-2025-02-05/.envrcfly.io/cdn-2025-02-05/fly.tomlfly.io/cdn-2025-02-05/justfilefly.io/cdn-2025-02-05/machines.txtfly.io/cdn-2025-02-05/regions.txtfly.io/cdn-2025-02-05/sizes.txtfly.tomljust/_config.justjust/_config_fly.justjust/fly.justjust/hurl.justjust/op.justjustfileregions.txttest/acceptance/assets.hurltest/acceptance/fastly/admin.hurltest/acceptance/fastly/feed.hurltest/acceptance/fastly/feeds.hurltest/acceptance/fastly/homepage.hurltest/acceptance/fastly/www.hurltest/acceptance/local/http.hurltest/acceptance/local/www.hurltest/acceptance/news-mp3.hurltest/acceptance/production/admin.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/acceptance/production/health.hurltest/acceptance/production/homepage.hurltest/vtc/app-backend.vtctest/vtc/assets-backend.vtctest/vtc/assets.vtctest/vtc/cache-status.vtctest/vtc/client-ip.vtctest/vtc/default.vtctest/vtc/feeds-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/fly/request-id.vtctest/vtc/health.vtctest/vtc/http.vtctest/vtc/news-mp3.vtctest/vtc/practicalai.vtctest/vtc/purge.vtctest/vtc/websockets.vtctest/vtc/www.vtcvarnish/changelog.com.vclvarnish/varnish-json-response.bashvarnish/vcl/app-backend.vclvarnish/vcl/assets-backend.vclvarnish/vcl/backend-health.vclvarnish/vcl/default.vclvarnish/vcl/disable-caching-for-5xx.vclvarnish/vcl/disable-default-backend.vclvarnish/vcl/feeds-backend.vclvarnish/vcl/fly/client-ip.vclvarnish/vcl/fly/region.vclvarnish/vcl/fly/request-id.vclvarnish/vcl/http.vclvarnish/vcl/news-mp3.vclvarnish/vcl/practicalai.vclvarnish/vcl/purge.vclvarnish/vcl/test/disable-caching-for-testing.vclvarnish/vcl/varnish-health.vclvarnish/vcl/www.vclvector/changelog.com/debug_s3.yamlvector/changelog.com/debug_varnish.yamlvector/changelog.com/debug_varnish_geoip.yamlvector/changelog.com/default.yamlvector/changelog.com/geoip.yaml
💤 Files with no reviewable changes (11)
- test/acceptance/fastly/admin.hurl
- fly.toml
- test/acceptance/fastly/feed.hurl
- test/vtc/assets.vtc
- test/acceptance/fastly/feeds.hurl
- test/vtc/health.vtc
- test/acceptance/fastly/homepage.hurl
- regions.txt
- test/vtc/client-ip.vtc
- test/acceptance/fastly/www.hurl
- test/vtc/cache-status.vtc
✅ Files skipped from review due to trivial changes (1)
- fly.io/cdn-2025-02-05/sizes.txt
🚧 Files skipped from review as they are similar to previous changes (24)
- varnish/vcl/purge.vcl
- .github/workflows/_namespace.yml
- test/acceptance/news-mp3.hurl
- varnish/varnish-json-response.bash
- test/acceptance/local/http.hurl
- test/vtc/fly/request-id.vtc
- just/_config.just
- test/vtc/default.vtc
- varnish/vcl/fly/client-ip.vcl
- varnish/vcl/http.vcl
- varnish/vcl/feeds-backend.vcl
- varnish/vcl/test/disable-caching-for-testing.vcl
- just/hurl.just
- test/acceptance/production/homepage.hurl
- varnish/changelog.com.vcl
- test/acceptance/local/www.hurl
- fly.io/cdn-2025-02-05/fly.toml
- test/vtc/fly/client-ip.vtc
- varnish/vcl/varnish-health.vcl
- varnish/vcl/backend-health.vcl
- test/acceptance/assets.hurl
- varnish/vcl/app-backend.vcl
- fly.io/cdn-2025-02-05/.envrc
- docs/local_dev.md
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, explicit repetition is preferred over complex regex patterns for maintainability, even when it results in more lines of code.
📚 Learning: 2025-12-30T13:48:47.598Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/news-mp3.vcl:97-97
Timestamp: 2025-12-30T13:48:47.598Z
Learning: In varnish/vcl/news-mp3.vcl, the ASSETS_HOST environment variable should NOT have a fallback or default value. The code should fail if ASSETS_HOST is not set, following a fail-fast pattern for required configuration.
Applied to files:
varnish/vcl/news-mp3.vcltest/vtc/news-mp3.vtctest/vtc/feeds-backend.vtcvarnish/vcl/assets-backend.vclvarnish/vcl/default.vcltest/vtc/practicalai.vtctest/vtc/assets-backend.vtctest/vtc/http.vtcvarnish/vcl/fly/region.vcl
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, prefer explicit code repetition over complex regular expressions for maintainability and readability, even if it increases lines of code. This guideline applies to all .vcl files within varnish/vcl/ (and its subdirectories) to keep patterns straightforward and easier to review.
Applied to files:
varnish/vcl/news-mp3.vclvarnish/vcl/practicalai.vclvarnish/vcl/www.vclvarnish/vcl/assets-backend.vclvarnish/vcl/default.vclvarnish/vcl/disable-default-backend.vclvarnish/vcl/disable-caching-for-5xx.vclvarnish/vcl/fly/region.vcl
📚 Learning: 2025-12-30T13:45:37.201Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: just/hurl.just:13-17
Timestamp: 2025-12-30T13:45:37.201Z
Learning: In Pipely's Justfiles, avoid defining color variables (GREEN, NORMAL, MAGENTA, BOLD, WHITE, CYAN, YELLOW, BLUE). Rely on Just's default color handling so output remains uncolored when colors aren’t defined. Do not hardcode empty strings or override defaults; let the tool manage color defaults for consistent behavior across Justfiles.
Applied to files:
just/op.justjust/fly.justjust/_config_fly.just
📚 Learning: 2025-12-30T13:53:53.196Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/fly/client-ip.vcl:3-16
Timestamp: 2025-12-30T13:53:53.196Z
Learning: In the varnish/vcl/fly/client-ip.vcl file, the client IP determination logic only logs the selected IP via std.log and does not store it in a request header. This is intentional because the health-checker reads directly from log output rather than from request headers.
Applied to files:
varnish/vcl/www.vclvarnish/vcl/assets-backend.vclvarnish/vcl/default.vcltest/vtc/practicalai.vtctest/vtc/http.vtctest/vtc/websockets.vtc
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, explicit repetition is preferred over complex regex patterns for maintainability, even when it results in more lines of code.
Applied to files:
test/vtc/practicalai.vtctest/vtc/www.vtctest/vtc/http.vtctest/vtc/websockets.vtc
📚 Learning: 2025-12-30T13:54:21.688Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: fly.io/cdn-2025-02-05/regions.txt:33-33
Timestamp: 2025-12-30T13:54:21.688Z
Learning: Preserve the exact verbatim content of CLI tool outputs (e.g., regions.txt) as produced by the tool. Do not normalize spelling, spacing, or formatting. Treat these files as canonical records of command output; document the source command (and timestamp, if relevant) in a brief header or comment if possible. This guideline applies to any file named regions.txt across the repository.
Applied to files:
fly.io/cdn-2025-02-05/regions.txt
🪛 LanguageTool
fly.io/cdn-2025-02-05/regions.txt
[uncategorized] ~32-~32: Did you mean “São Paulo” (= city in Brazil)?
Context: ... South America Sao Paulo, Brazil gru 11...
(SAO_PAOLO)
🔇 Additional comments (75)
test/vtc/feeds-backend.vtc (3)
1-1: LGTM: Test scope is clear and focused.The test appropriately validates feeds backend routing behavior.
295-297: LGTM: VCL includes are appropriate for routing tests.The inclusion of
disable-caching-for-testing.vclis intentional and appropriate for validating routing behavior without interference from caching logic.
276-280: LGTM: Environment setup follows fail-fast pattern.The environment variables are correctly configured for testing, and the use of
std.getenv("BACKEND_FEEDS_FQDN")without a fallback (line 291) appropriately follows the fail-fast pattern for required configuration.Based on learnings, required environment variables should fail if not set rather than using fallback values.
Also applies to: 289-293
just/_config_fly.just (1)
1-32: Clean configuration structure with appropriate safety defaults.The configuration properly distinguishes between:
FLY_ORGwith a sensible default ("changelog")- Production-critical variables (
FLY_APP, regions, CPU/memory specs) without defaults, ensuring explicit configuration is requiredThe module version pinning for
DAGGER_FLY_MODULEis good practice for reproducibility.just/op.just (1)
16-22: Color token unification aligns with project conventions.The change from prefixed tokens (
_GREEN,_RESET,_MAGENTA) to non-prefixed names (GREEN,NORMAL,MAGENTA) follows the project's approach of relying on Just's default color handling. Based on learnings, this ensures output remains uncolored when colors aren't defined.just/fly.just (7)
1-8: Deploy recipe looks solid.The 70% memory allocation for
VARNISH_SIZEleaves appropriate headroom for OS and other processes. The--ha=falseflag aligns with the HOT/COLD architecture described in the PR objectives.
28-36: Secure secrets management pattern.Good use of 1Password references (
op://) for runtime secret injection. The--stageflag appropriately stages secrets without immediately restarting machines.
38-51: Clean certificate management recipes.
53-61: LGTM!
63-76: Good restart sequence with retry logic.The one-by-one restart with sleep intervals and retry logic is a solid pattern for graceful machine rotation. The final check for stopped machines provides good operational visibility.
Same note as above: consider replacing hardcoded
'pipely'on line 67 with{{ FLY_APP }}.
78-91: Useful Varnish debugging commands.The
--selectflag allows interactive machine selection, which is helpful for diagnosing specific instances. These recipes align well with the PR's goal of tuning Varnish based on deployment metrics.
93-96: Idempotent app creation guard.The check-before-create pattern ensures the recipe is safe to run multiple times without side effects.
test/acceptance/production/feed.hurl (2)
13-14: LGTM! Cache-status metadata assertions added.The new assertions for
keep=andstorage=validate the cache metadata fields introduced in this PR, aligning with the objectives to expose cache configuration details for observability.
35-35: LGTM! Storage verification for feed content.The assertion confirms that feed content (XML/RSS) is cached in memory rather than disk, which aligns with the PR's goal to store only MP3 files on disk while keeping other assets in memory.
varnish/vcl/www.vcl (1)
21-21: LGTM! Header name normalization.The change from
resp.http.Locationtoresp.http.locationaligns with the PR's broader header-naming convention standardization. HTTP headers are case-insensitive per RFC 7230, so this is purely a stylistic consistency improvement.varnish/vcl/fly/region.vcl (1)
4-20: LGTM! Region metadata handling.The region detection logic correctly reads
FLY_REGION, defaults to "LOCAL" for local development, and logs the region asserver_datacenter. The cache-status header assignment approach was previously reviewed and confirmed by passing VTC & acceptance tests.test/vtc/http.vtc (2)
11-11: LGTM! Modular VCL include.The change from an absolute include path to a relative include
"http.vcl"aligns with the PR's modular VCL refactoring and improves test portability.
30-33: LGTM! Test updates for standardization.The updates to use
pipely.localas the test host andresp.http.location(lowercase) for header assertions align with the PR's test standardization and header naming normalization.varnish/vcl/disable-caching-for-5xx.vcl (1)
1-16: LGTM! Proper 5xx error handling with bgfetch protection.The implementation correctly prevents caching of 5xx responses while handling the background fetch edge case. The
return (abandon)on Line 13 ensures that during asynchronous background updates, a 5xx response doesn't evict the previously cached valid object—preserving availability during backend issues.test/vtc/purge.vtc (2)
3-10: LGTM! Refactored to use modular VCL and environment configuration.The test now uses environment-driven configuration (
PURGE_TOKEN) and modular VCL includes (disable-default-backend.vcl,purge.vcl), aligning with the PR's objective to eliminate VCL duplication and improve maintainability.
22-33: LGTM! Token values updated to match environment configuration.The test token values correctly reflect the new environment-driven approach, with "invalid" for the negative test case and "sudo" (matching
PURGE_TOKEN) for the positive test case. Test expectations remain consistent.test/vtc/app-backend.vtc (1)
1-41: LGTM! Comprehensive app backend health check test.The test correctly validates the app backend health flow with environment-driven configuration, dynamic director setup, and dual storage (memory + disk) matching the PR's cache architecture. The path transformation from
/app_healthto/health(Lines 7, 37) is properly tested against the mock backend.varnish/vcl/news-mp3.vcl (1)
97-97: LGTM! Centralized assets routing via ASSETS_HOST.The change from
req.http.hosttostd.getenv("ASSETS_HOST")centralizes assets host configuration. The lack of a fallback is intentional, following a fail-fast pattern for required configuration—ensuring misconfigurations are caught immediately rather than producing malformed redirects.Based on learnings, ASSETS_HOST should fail if not set.
varnish/vcl/disable-default-backend.vcl (1)
1-2: LGTM!Clear and concise approach to enforce dynamic-backend-only routing. The comment accurately documents the intent.
test/vtc/practicalai.vtc (1)
6-7: LGTM!The refactoring to use includes instead of inline VCL improves maintainability while preserving test coverage. The test assertions remain unchanged and continue to verify the expected redirect behavior.
fly.io/cdn-2025-02-05/regions.txt (1)
1-34: LGTM!This file correctly captures the verbatim output from
flyctl, which serves as a canonical reference for deployment regions. Based on learnings, CLI tool outputs should be preserved exactly as returned.varnish/vcl/practicalai.vcl (1)
1-26: LGTM!The redirect implementation is straightforward and correct. The explicit URL matching aligns with maintainability preferences for VCL code. Both redirects properly return 301 status with location headers and HTML fallback bodies.
test/vtc/www.vtc (2)
6-7: LGTM!The refactoring to use modular VCL includes improves maintainability and aligns with the broader architectural changes in this PR.
12-15: LGTM!The test updates correctly reflect the new hostname conventions (pipely.local) and lowercase header name (location), which align with the updated VCL behavior.
fly.io/cdn-2025-02-05/machines.txt (1)
1-21: LGTM!This file correctly captures the machine inventory snapshot from the Fly.io CLI. All 15 machines show healthy status (1/1 checks passing) with consistent image versions, which provides a good baseline for the HOT/COLD instance split mentioned in the PR objectives.
test/vtc/news-mp3.vtc (5)
11-16: LGTM!Environment variable setup correctly includes ASSETS_HOST without a fallback, following the fail-fast pattern for required configuration. The APP_HOST and BACKEND_APP_* variables properly configure the dynamic backend testing environment.
19-20: LGTM!Adding both memory and disk cache storage aligns with the PR objective to store MP3s on disk while keeping other assets in memory. This enables testing the differential storage behavior.
27-36: LGTM!The dynamic director setup and modular VCL includes align with the architectural refactoring. The app backend initialization correctly uses environment variables for dynamic configuration.
46-52: LGTM!Excellent addition to verify that the x-redirect header is properly ignored. This test ensures that external redirect attempts cannot override the configured assets host.
59-67: LGTM!The redirect expectations correctly use assets.local hostname, which improves test clarity over the previous IP-based approach and aligns with the environment-driven configuration strategy.
test/vtc/assets-backend.vtc (6)
39-42: LGTM!Environment variable setup correctly configures the assets backend for dynamic director initialization, following the fail-fast pattern established for required configuration.
45-66: LGTM!The Varnish configuration correctly sets up both memory and disk caches, which is essential for testing the differential storage behavior. The dynamic director initialization and modular VCL includes align with the architectural refactoring.
76-82: LGTM!Excellent test coverage for method restrictions. The 405 response with the Allow header properly documents the permitted methods.
106-121: LGTM!The logexpect assertion correctly verifies that PNG files are stored in memory cache ("malloc memory"), which aligns with the PR objective to keep non-MP3 assets in memory to prevent fragmentation.
123-138: LGTM!The logexpect assertion correctly verifies that CSS files are stored in memory cache, consistent with the storage strategy for non-MP3 assets.
140-155: LGTM!Excellent test coverage for the core PR objective. The logexpect assertion correctly verifies that MP3 files are stored in the file-based disk cache ("file disk"), preventing memory fragmentation from large files while enabling efficient caching of both large MP3s and small assets.
README.md (2)
19-21: LGTM!Documentation updates accurately reflect the new MP3 file cache feature and updated tooling requirements.
Also applies to: 83-83
94-112: LGTM!Recipe documentation has been improved with clearer descriptions and updated URLs that align with the refactored tooling.
varnish/vcl/assets-backend.vcl (3)
1-23: LGTM!The
vcl_recvimplementation correctly:
- Routes assets requests to the appropriate backend without fallbacks (fail-fast pattern)
- Enforces security via method restrictions (GET/HEAD/OPTIONS/PURGE only)
- Handles health check redirects appropriately
25-32: LGTM!Standard 405 Method Not Allowed handling with appropriate Allow header.
34-41: LGTM!CORS wildcard header is appropriate for public CDN assets and aligns with typical CDN behavior.
varnish/vcl/default.vcl (4)
7-40: LGTM!Dynamic director configuration is well-structured:
- Appropriate timeout values with increased
first_byte_timeoutfor the app backend to support MP3 uploads- Consistent health probe configuration across all backends
- Clear comments explaining the rationale
42-57: LGTM!The modular include approach significantly improves maintainability and eliminates VCL duplication, which aligns with the PR objectives.
59-81: LGTM!Environment variable overrides are well-designed:
- Allow testing without code changes
- Clear documentation that they override backend-specific settings
- Appropriate use of
std.duration()for parsing- Provides flexibility for development/testing scenarios
83-127: LGTM!The
vcl_deliverimplementation provides comprehensive cache metrics:
- Enhanced
cache-statusheader includes storage information (memory vs. disk)- Useful for monitoring and validating that MP3s are stored on disk as intended
- Structured logging supports observability goals
container/justfile (4)
35-36: LGTM!Adding
-1tovarnishstatprovides one-shot output which is appropriate for thestattarget. This makes the command more useful for scripting and one-time checks.
55-56: LGTM!The expanded cache statistics filters now include both memory (SMA) and file (SMF) storage metrics, which aligns perfectly with the PR's objective of monitoring disk cache for MP3 files alongside in-memory cache.
65-84: LGTM!The benchmark recipe reorganization properly reflects the infrastructure changes:
- Fastly → Fly.io rebranding
- Removal of pipedream.changelog.com
- Consistent renumbering after removals
- Addition of local benchmarks for app and feed
These changes align with the PR objectives of consolidating to changelog.com and using Fly.io as the production platform.
39-40: No action needed—the glob pattern is correct as-is.VTC files exist at two directory levels: 10 files at
test/vtc/root and 2 files intest/vtc/fly/. The patterntest/vtc/*.vtc test/vtc/**/*.vtcis necessary because**does not match the current directory level—only subdirectories. Removing either pattern would lose coverage.Likely an incorrect or invalid review comment.
justfile (9)
17-23: LGTM!The addition of
--varnish-file-cache=trueenables disk caching for MP3 files as per the PR objectives. Keeping--beresp-ttl=5sin the local-debug target is appropriate for faster iteration during development.
25-30: LGTM!The file cache flag is correctly added to enable disk caching for local runs.
53-59: LGTM!File cache flag added consistently. The short TTL is appropriate for fast iteration during local acceptance testing.
86-99: Note the removal of --beresp-ttl=5s.The
local-debug-productiontarget no longer sets an explicit short TTL, which means it will use default or environment-driven TTL values. This change makes local debugging more representative of actual production behavior but may slow iteration slightly.
101-115: LGTM!Consistent with
local-debug-production, this target now uses default TTL values and enables file caching, making local runs more representative of production behavior.
117-123: LGTM!The URL change from
pipedream.changelog.comtochangelog.comaligns with the PR's consolidation objective.
134-137: APP_IMAGE is properly defined and ready to use.The publish target now uses
APP_IMAGEinstead ofFLY_APP_IMAGE. The variable is defined injust/_config.justwith a sensible default (ghcr.io/thechangelog/pipely) and no orphanedFLY_APP_IMAGEreferences remain in the codebase. The change is consistent across all affected targets.
61-75: Verify the resolved IP address for production testing.The production acceptance tests hardcode DNS resolution for
changelog.comandcdn.changelog.comto137.66.2.20. Ensure this IP address matches your current production Fly.io deployment.
33-43: The--network hostchange is intentional for this development/testing use case. The comment on line 32 explicitly states the target "works on remote servers too," confirming this design choice enables simplified access without port mapping configuration.Docker's documentation confirms that
--network hostdoes remove network isolation and increases attack surface if the container is compromised. However, this trade-off is acceptable for local and remote development/testing scenarios where convenience and direct host access outweigh the security concerns of isolated bridged networking.No changes needed.
fly.io/cdn-2025-02-05/justfile (1)
3-5: The imported configuration files exist in the repository and the relative paths are correct. All three files are present at the expected locations:
just/_config.justjust/_config_fly.justjust/fly.justThe imports are valid and will resolve correctly from the justfile's location.
.github/workflows/_github.yml (1)
26-29: Thetest-acceptance-productiontarget is defined in the justfile and properly configured to run acceptance tests against the production environment using hurl.dagger/main.go (7)
8-9: LGTM!The new imports support the file cache validation (regexp) and VarnishOptions construction (strings).
49-50: LGTM!The VarnishOptions field provides good extensibility for building dynamic Varnish command-line options.
79-92: Well-structured parameter additions.The new parameters cleanly extend the configuration surface for Varnish. The optional pattern for beresp parameters is consistent.
163-176: LGTM!The conditional BERESP_* environment variable handling is clean and follows a consistent pattern.
271-273: LGTM!Installing procps provides sysctl for kernel parameter tuning, aligning with the PR objective to tune Varnish configuration.
319-343: Consistent consolidation of vector config paths.The path changes from
vector/pipedream.changelog.com/tovector/changelog.com/correctly implement the PR objective to consolidate from pipedream.changelog.com to changelog.com. All five config file references have been updated consistently.
243-248: The multi-storage configuration is correctly implemented.The procfile construction properly integrates VarnishOptions. The codebase confirms that when file cache is enabled, Varnish is configured with both memory (
-s memory=malloc,$VARNISH_SIZE) and disk (-s disk=file,...,$VARNISH_FILE_SIZE) storage backends. The test suite (test/vtc/assets-backend.vtc) validates this configuration, and MP3s are routed to the file cache as intended (PR #44).test/vtc/websockets.vtc (3)
1-1: LGTM! Test name is now version-agnostic.Removing the Varnish version reference makes the test name more maintainable and aligns with the modular VCL refactoring approach.
1-60: LGTM! Comprehensive WebSocket upgrade test.The test correctly validates:
- WebSocket upgrade handshake (status 101, upgrade headers)
- Security headers (Sec-WebSocket-Key, Sec-WebSocket-Accept)
- Protocol negotiation (Sec-WebSocket-Protocol)
- Bidirectional communication through Varnish pipe
The test structure is well-organized and thoroughly verifies WebSocket connection handling.
34-34: The relative include path works correctly. The test infrastructure (dagger/main.go) mountsvarnish/vcl/to/etc/varnish/in the test container before varnishtest runs. Since varnishtest searches standard Varnish paths like/etc/varnish/by default, the relative include"websockets.vcl"will resolve correctly to/etc/varnish/websockets.vcl. This pattern is consistent across all VTC tests in the repository (www.vtc, purge.vtc, app-backend.vtc, etc.), confirming it's working as intended.Likely an incorrect or invalid review comment.
gerhard
force-pushed
the
store-mp3s-in-file-cache
branch
from
d3c02d6 to
1f91ec8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
test/vtc/app-backend.vtc (1)
41-41: Add newline at end of file.The file is missing a trailing newline, which is a POSIX text file convention and expected by many tools.
test/vtc/default.vtc (1)
75-93: Consider consolidating vcl_init blocks.While multiple
vcl_initblocks are syntactically valid and execute in order, consolidating them into a singlevcl_initblock would improve readability and align with common VCL patterns.🔎 Proposed consolidation
- # Use an app backend with no probe when testing - sub vcl_init { + sub vcl_init { + # Use an app backend with no probe when testing new app = dynamic.director( host_header = std.getenv("BACKEND_APP_FQDN"), ); - } - - # Use an assets backend with no probe when testing - sub vcl_init { + + # Use an assets backend with no probe when testing new assets = dynamic.director( host_header = std.getenv("BACKEND_ASSETS_FQDN"), ); - } - - # Use a feeds backend with no probe when testing - sub vcl_init { + + # Use a feeds backend with no probe when testing new feeds = dynamic.director( host_header = std.getenv("BACKEND_FEEDS_FQDN"), ); }test/vtc/feeds-backend.vtc (1)
300-730: Consider adding storage verification like assets-backend.vtc.The client tests comprehensively validate feed routing. However, unlike
test/vtc/assets-backend.vtc, this test doesn't verify which storage backend (memory vs. disk) is used for cached objects. Since feeds are XML (not MP3), memory storage is expected, but adding logexpect assertions would confirm feeds aren't inadvertently using disk cache.🔎 Example storage verification pattern
Add a logexpect block similar to assets-backend.vtc lines 107-121:
+# Verify feeds use memory storage, not disk +logexpect l1 -v v1 -d 1 -g vxid -q "Storage" -T 5 { + expect 0 * Begin + expect * * BereqURL "/podcast.xml" + expect * * Storage "malloc memory" + expect * * End +} -start client c2 { txreq -url "/podcast/feed" rxresp expect resp.status == 200 expect resp.body == "podcast.xml" } -run +logexpect l1 -waitjust/fly.just (3)
18-18: Remove hardcoded color variable references.Line 18 references
BLUE,BOLD, andNORMALcolor variables. Based on learnings, avoid defining or relying on color variables—let Just handle color defaults to ensure consistent behavior across Justfiles.🔎 Proposed fix
- echo -e "\n🌑 Scaling machine in {{ BLUE }}COLD region {{ BOLD }}$region{{ NORMAL }} to {{ BOLD }}{{ FLY_APP_REGION_DEFAULT_CPU_KIND }}-{{ FLY_APP_REGION_DEFAULT_CPU_NUM }}C-{{ FLY_APP_REGION_DEFAULT_MEM }}M{{ BOLD }} ..."; \ + echo -e "\n🌑 Scaling machine in COLD region $region to {{ FLY_APP_REGION_DEFAULT_CPU_KIND }}-{{ FLY_APP_REGION_DEFAULT_CPU_NUM }}C-{{ FLY_APP_REGION_DEFAULT_MEM }}M ..."; \Based on learnings, avoid using color variables in Pipely's Justfiles.
17-17: Potential regex matching issue with region filtering.The regex
[[ ! {{ FLY_APP_REGIONS_HOT }} =~ $region ]]may produce unexpected results ifFLY_APP_REGIONS_HOTcontains regex metacharacters or if partial region names match. Consider exact matching or word boundaries.🔎 Suggested improvement
For exact matching, consider using a comma-delimited approach or grep with word boundaries:
if ! echo ",{{ FLY_APP_REGIONS_HOT }}," | grep -q ",$region,"; then # scale COLD region fi
75-75: Remove hardcoded color variable references.Line 75 uses
MAGENTAandNORMALcolor variables. Per learnings, avoid defining or referencing color variables in Pipely's Justfiles—rely on Just's default color handling instead.🔎 Proposed fix
- @echo {{ MAGENTA }}🧐 Any stopped machines?{{ NORMAL }} + @echo "🧐 Any stopped machines?"Based on learnings, avoid using color variables in Pipely's Justfiles.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.godocs/local_dev.mdfly.io/cdn-2025-02-05/.envrcfly.io/cdn-2025-02-05/fly.tomlfly.io/cdn-2025-02-05/justfilefly.io/cdn-2025-02-05/machines.txtfly.io/cdn-2025-02-05/regions.txtfly.io/cdn-2025-02-05/sizes.txtfly.tomljust/_config.justjust/_config_fly.justjust/fly.justjust/hurl.justjust/op.justjustfileregions.txttest/acceptance/assets.hurltest/acceptance/fastly/admin.hurltest/acceptance/fastly/feed.hurltest/acceptance/fastly/feeds.hurltest/acceptance/fastly/homepage.hurltest/acceptance/fastly/www.hurltest/acceptance/local/http.hurltest/acceptance/local/www.hurltest/acceptance/news-mp3.hurltest/acceptance/production/admin.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/acceptance/production/health.hurltest/acceptance/production/homepage.hurltest/vtc/app-backend.vtctest/vtc/assets-backend.vtctest/vtc/assets.vtctest/vtc/cache-status.vtctest/vtc/client-ip.vtctest/vtc/default.vtctest/vtc/feeds-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/fly/request-id.vtctest/vtc/health.vtctest/vtc/http.vtctest/vtc/news-mp3.vtctest/vtc/practicalai.vtctest/vtc/purge.vtctest/vtc/websockets.vtctest/vtc/www.vtcvarnish/changelog.com.vclvarnish/varnish-json-response.bashvarnish/vcl/app-backend.vclvarnish/vcl/assets-backend.vclvarnish/vcl/backend-health.vclvarnish/vcl/default.vclvarnish/vcl/disable-caching-for-5xx.vclvarnish/vcl/disable-default-backend.vclvarnish/vcl/feeds-backend.vclvarnish/vcl/fly/client-ip.vclvarnish/vcl/fly/region.vclvarnish/vcl/fly/request-id.vclvarnish/vcl/http.vclvarnish/vcl/news-mp3.vclvarnish/vcl/practicalai.vclvarnish/vcl/purge.vclvarnish/vcl/test/disable-caching-for-testing.vclvarnish/vcl/varnish-health.vclvarnish/vcl/www.vclvector/changelog.com/debug_s3.yamlvector/changelog.com/debug_varnish.yamlvector/changelog.com/debug_varnish_geoip.yamlvector/changelog.com/default.yamlvector/changelog.com/geoip.yaml
💤 Files with no reviewable changes (11)
- test/acceptance/fastly/feeds.hurl
- regions.txt
- test/acceptance/fastly/feed.hurl
- test/acceptance/fastly/admin.hurl
- test/vtc/health.vtc
- test/vtc/assets.vtc
- test/vtc/cache-status.vtc
- test/vtc/client-ip.vtc
- fly.toml
- test/acceptance/fastly/www.hurl
- test/acceptance/fastly/homepage.hurl
✅ Files skipped from review due to trivial changes (2)
- docs/local_dev.md
- just/_config_fly.just
🚧 Files skipped from review as they are similar to previous changes (24)
- just/_config.just
- test/vtc/fly/client-ip.vtc
- fly.io/cdn-2025-02-05/sizes.txt
- .github/workflows/_namespace.yml
- varnish/vcl/www.vcl
- varnish/vcl/disable-caching-for-5xx.vcl
- varnish/vcl/test/disable-caching-for-testing.vcl
- fly.io/cdn-2025-02-05/fly.toml
- varnish/vcl/disable-default-backend.vcl
- test/vtc/websockets.vtc
- varnish/vcl/news-mp3.vcl
- varnish/vcl/fly/region.vcl
- fly.io/cdn-2025-02-05/machines.txt
- just/op.just
- README.md
- varnish/vcl/http.vcl
- fly.io/cdn-2025-02-05/.envrc
- test/acceptance/production/feed.hurl
- varnish/varnish-json-response.bash
- varnish/changelog.com.vcl
- varnish/vcl/purge.vcl
- varnish/vcl/app-backend.vcl
- varnish/vcl/backend-health.vcl
- varnish/vcl/assets-backend.vcl
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-30T13:48:47.598Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/news-mp3.vcl:97-97
Timestamp: 2025-12-30T13:48:47.598Z
Learning: In varnish/vcl/news-mp3.vcl, the ASSETS_HOST environment variable should NOT have a fallback or default value. The code should fail if ASSETS_HOST is not set, following a fail-fast pattern for required configuration.
Applied to files:
test/vtc/default.vtctest/vtc/news-mp3.vtctest/vtc/practicalai.vtctest/vtc/assets-backend.vtctest/vtc/feeds-backend.vtcvarnish/vcl/feeds-backend.vcltest/vtc/http.vtcvarnish/vcl/default.vcl
📚 Learning: 2025-12-30T13:53:53.196Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/fly/client-ip.vcl:3-16
Timestamp: 2025-12-30T13:53:53.196Z
Learning: In the varnish/vcl/fly/client-ip.vcl file, the client IP determination logic only logs the selected IP via std.log and does not store it in a request header. This is intentional because the health-checker reads directly from log output rather than from request headers.
Applied to files:
test/vtc/default.vtctest/vtc/practicalai.vtctest/vtc/fly/request-id.vtcvarnish/vcl/varnish-health.vclvarnish/vcl/fly/client-ip.vclvarnish/vcl/feeds-backend.vcltest/vtc/http.vtcvarnish/vcl/default.vcl
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, explicit repetition is preferred over complex regex patterns for maintainability, even when it results in more lines of code.
Applied to files:
test/vtc/default.vtctest/vtc/www.vtctest/vtc/practicalai.vtctest/vtc/fly/request-id.vtctest/vtc/http.vtc
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, prefer explicit code repetition over complex regular expressions for maintainability and readability, even if it increases lines of code. This guideline applies to all .vcl files within varnish/vcl/ (and its subdirectories) to keep patterns straightforward and easier to review.
Applied to files:
varnish/vcl/practicalai.vclvarnish/vcl/varnish-health.vclvarnish/vcl/fly/client-ip.vclvarnish/vcl/feeds-backend.vclvarnish/vcl/default.vcl
📚 Learning: 2025-12-30T13:45:37.201Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: just/hurl.just:13-17
Timestamp: 2025-12-30T13:45:37.201Z
Learning: In Pipely's Justfiles, avoid defining color variables (GREEN, NORMAL, MAGENTA, BOLD, WHITE, CYAN, YELLOW, BLUE). Rely on Just's default color handling so output remains uncolored when colors aren’t defined. Do not hardcode empty strings or override defaults; let the tool manage color defaults for consistent behavior across Justfiles.
Applied to files:
just/hurl.justjust/fly.just
📚 Learning: 2025-12-30T13:54:21.688Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: fly.io/cdn-2025-02-05/regions.txt:33-33
Timestamp: 2025-12-30T13:54:21.688Z
Learning: Preserve the exact verbatim content of CLI tool outputs (e.g., regions.txt) as produced by the tool. Do not normalize spelling, spacing, or formatting. Treat these files as canonical records of command output; document the source command (and timestamp, if relevant) in a brief header or comment if possible. This guideline applies to any file named regions.txt across the repository.
Applied to files:
fly.io/cdn-2025-02-05/regions.txt
🪛 LanguageTool
fly.io/cdn-2025-02-05/regions.txt
[uncategorized] ~32-~32: Did you mean “São Paulo” (= city in Brazil)?
Context: ... South America Sao Paulo, Brazil gru 11...
(SAO_PAOLO)
🔇 Additional comments (77)
fly.io/cdn-2025-02-05/regions.txt (1)
1-34: LGTM! File preserved as verbatim CLI output.This file correctly captures the output from
flyctlas-is, including any spellings or formatting variations from the tool itself. Per the established learning and past review feedback, preserving the exact output is the right approach for maintaining a canonical record of the tool's state.just/hurl.just (1)
13-17: LGTM! Clean color token refactoring.The replacement of
_GREEN/_RESETwithGREEN/NORMALand_MAGENTA/_RESETwithMAGENTA/NORMALis consistent with the PR's refactoring objectives. The added echo statements for the HURL path and symlink command improve debugging visibility without altering functionality.Based on learnings, the undefined color variables are intentional defaults and will degrade gracefully.
test/vtc/purge.vtc (2)
3-3: LGTM: Token values are consistent across environment setup and test cases.The environment variable
PURGE_TOKENis set to "sudo" at line 3, and this value is correctly used in the test case at line 30. The invalid token test at line 22 uses a different value ("invalid"), ensuring proper validation. The test coverage is comprehensive, validating three scenarios: no token, invalid token, and correct token.Also applies to: 22-22, 30-30
8-9: No issues found. The include paths follow the established codebase convention and all referenced VCL files exist invarnish/vcl/with the expected PURGE token validation logic.dagger/main.go (8)
8-9: LGTM!The new imports are properly utilized for cache size validation and dynamic option assembly.
49-50: LGTM!The
VarnishOptionsfield enables dynamic varnishd configuration and is properly integrated into the procfile generation.
79-92: LGTM!The new parameters follow Dagger conventions and provide good control over Varnish caching behavior. The
varnishFileCacheflag cleanly enables the file-backed storage feature.
144-144: LGTM!Making the cache size configurable via parameter and environment variable improves flexibility.
164-177: LGTM!The conditional environment variable assignment pattern cleanly handles optional BERESP configuration parameters.
244-249: LGTM on dynamic option assembly!The
strings.Join(m.VarnishOptions, " ")correctly integrates additional varnishd options into the procfile. The dual storage backend sizing concern is addressed in the earlier comment about lines 149-162.
272-274: LGTM!Installing
procpsprovides essential system monitoring tools (ps,sysctl, etc.) for operational observability.
320-344: LGTM!The vector configuration paths are consistently updated from
pipedream.changelog.comtochangelog.com, with all referenced config files present at the new locations and no orphaned references to old paths remaining.test/acceptance/news-mp3.hurl (1)
5-251: LGTM! Consistent header name normalization.The change from
"Location"to"location"standardizes header name casing across all redirect assertions. HTTP header names are case-insensitive per RFC 2616, and this lowercase convention aligns with the broader normalization effort across the VCL and test suite.test/vtc/www.vtc (1)
3-15: LGTM! Test updated for modular VCL architecture.The changes correctly refactor the test to use the new include-based VCL composition (
disable-default-backend.vclandwww.vcl), update test hostnames topipely.local, and normalize the header name to lowercase—all of which align with the PR's modular VCL architecture objectives.test/vtc/news-mp3.vtc (4)
3-16: LGTM! Clean test setup with proper environment configuration.The mock backend setup and environment variables are well-structured. Note that
ASSETS_HOSTcorrectly has no fallback value, following the fail-fast pattern for required configuration (based on learnings).
19-36: LGTM! Tests dual-storage configuration for MP3s.The test correctly exercises the key PR objective of storing MP3s on disk while keeping other assets in memory. The
-s memory=malloc,10Mand-s disk=file,${tmpdir}/file.bin,10Marguments validate the HOT/COLD instance architecture and prevent memory fragmentation from large files.
38-52: LGTM! Valuable test coverage additions.The new test cases validate:
- Proper 404 handling from the app backend
- Security: the
x-redirectheader is correctly ignored, preventing potential redirect manipulation
54-68: LGTM! Redirect tests updated for new environment.The redirect expectations now correctly target
assets.localand use the normalized lowercaselocationheader name, consistent with the broader test standardization effort.varnish/vcl/feeds-backend.vcl (2)
4-10: LGTM! Health endpoint properly configured.The
/feeds_healthendpoint correctly bypasses caching viareturn(pass)and rewrites to/healthfor the backend, following standard health check patterns.
115-131: LGTM! Cache configuration optimized for feeds.The cache settings are well-tuned:
- Memory storage: Appropriate for small XML feeds
- 12h TTL: Reasonable freshness window for podcast feeds
- 1d grace: Enables resilient stale-while-revalidate behavior
- 7d keep: Directly supports the PR objective of avoiding re-downloads when responses return 304 Not Modified
test/acceptance/local/http.hurl (1)
12-13: LGTM! Enhanced cache observability.The new assertions validate the presence of
keep=andstorage=fields in thecache-statusheader, providing important observability for the new dual-storage architecture and conditional request optimizations introduced in this PR.test/acceptance/local/www.hurl (1)
4-4: LGTM! Consistent header normalization.The lowercase
locationheader name aligns with the standardization effort across all tests and VCL files.varnish/vcl/varnish-health.vcl (1)
1-5: LGTM! Simple and effective Varnish health endpoint.The
/healthendpoint returns a synthetic 204 No Content response, which is the standard pattern for health checks. This validates Varnish itself is responding without requiring a backend call.test/acceptance/production/homepage.hurl (2)
13-14: LGTM! Past issue resolved.The assertions correctly validate cache-status metadata fields. Line 13 now properly checks for
keep=(the previous copy-paste error has been fixed), and line 14 adds the newstorage=validation as intended.
24-24: LGTM!The assertion correctly validates that the cached homepage is served from memory storage, aligning with the PR's cache-status instrumentation improvements.
test/vtc/fly/request-id.vtc (1)
18-18: LGTM!The switch from an absolute include path to a relative path improves portability and aligns with the PR's modular VCL architecture.
test/acceptance/assets.hurl (1)
56-56: LGTM!The assertions correctly validate that MP3 chunks are cached on disk storage, which is the core objective of this PR. The coverage across first, middle, and end chunks ensures comprehensive validation of the disk caching implementation for large audio files.
Also applies to: 70-70, 84-84
test/vtc/practicalai.vtc (1)
6-7: LGTM!The refactoring from inline VCL to modular includes improves maintainability and aligns with the PR's objective to break the monolithic VCL into reusable fragments. Test expectations remain unchanged, confirming behavior preservation.
test/vtc/http.vtc (2)
11-11: LGTM!The switch to a relative include path aligns with the PR's modular VCL architecture and improves portability.
30-30: LGTM!The test host update to
pipely.localand the corresponding redirect URL adjustment correctly align the test with the local test environment. The lowercaselocationheader assertion follows HTTP conventions.Also applies to: 33-33
varnish/vcl/practicalai.vcl (2)
1-6: LGTM!The
vcl_recvlogic correctly identifies the two redirect paths using explicit string matching, which aligns with the codebase preference for maintainability over complex patterns.
8-26: LGTM!The
vcl_synthlogic correctly handles both redirect paths with appropriate location headers and user-friendly HTML bodies. The explicit separation of conditions enhances clarity and maintainability.test/vtc/app-backend.vtc (4)
1-9: LGTM!The mock backend correctly expects the
/healthendpoint (rewritten from/app_healthby the VCL) and responds appropriately for the health check flow.
11-14: LGTM!The environment variables are correctly configured to wire the dynamic director to the mock backend server.
17-33: LGTM!The Varnish configuration correctly sets up memory and disk storage, initializes the dynamic director with environment-based configuration, and includes the necessary modular VCL components for app backend routing.
36-41: LGTM!The client test correctly validates the app backend health check flow, ensuring
/app_healthis properly routed to the backend and rewritten to/health.test/vtc/default.vtc (7)
1-56: LGTM!The mock server setup comprehensively covers all backend types (default, app, assets, feeds) with appropriate health check and content endpoints. Environment variable configuration is consistent and correct.
57-60: LGTM!The cache tuning environment variables provide test-time control over TTL, GRACE, and KEEP values, enabling comprehensive cache behavior validation. Values are appropriate for testing.
95-99: LGTM!The includes correctly wire in all modular VCL components for backend routing, region awareness, and health checks, demonstrating the PR's refactored architecture.
101-123: LGTM!The
vcl_backend_responseimplementation correctly provides test-time overrides for cache tuning parameters while preserving backend-specific defaults when environment variables are not set. The warning comments appropriately flag these as test-only features.
125-128: LGTM!The
vcl_passimplementation correctly marks bypassed requests for cache-status instrumentation, enabling comprehensive validation of caching behavior in tests.
130-169: LGTM!The
vcl_deliverimplementation constructs a comprehensivecache-statusheader that captures all relevant cache metrics (origin, TTL, grace, keep, storage, hit/miss/stale status, and hit count). The parallel logging viastd.log()enables external monitoring tools to track cache behavior.
172-250: LGTM!The client tests provide comprehensive coverage of all backends (Varnish, app, assets, feeds) and cache states (health/bypass, miss, hit). Each test validates the detailed
cache-statusheader format, confirming that TTL/grace/keep tuning, storage attribution, and cache hit tracking all work correctly.test/vtc/feeds-backend.vtc (2)
1-274: LGTM! Comprehensive feed routing test coverage.The server mock setup correctly implements health checks and comprehensive feed route testing. The pattern of rewriting
/*/feedURLs to/*.xmlendpoints is clear and well-tested.
276-279: LGTM! Environment variables properly configured.The environment variables correctly configure the feeds backend for the dynamic director, with BACKEND_FEEDS_FQDN used in the vcl_init block.
test/vtc/assets-backend.vtc (4)
1-37: LGTM! Well-structured assets backend mock.The server mock correctly handles health checks and various HTTP methods for different asset types (PNG, CSS, MP3). The inclusion of query parameters in the MP3 test adds valuable coverage.
39-66: LGTM! Dual-cache configuration enables storage verification.The Varnish configuration with both memory and file caches correctly enables testing of the PR's core objective: verifying MP3s are stored on disk while other assets use memory. The dynamic director setup and includes are properly structured.
68-104: LGTM! Comprehensive HTTP method validation.The tests thoroughly validate allowed and disallowed HTTP methods, including proper CORS headers and the Allow header for rejected methods. This ensures the assets backend behaves correctly for different request types.
106-155: Excellent! Critical validation of storage backend assignment.These logexpect assertions validate the core PR objective: MP3 files are stored in the file cache (
"file disk"at line 145) while other assets like PNG and CSS use memory cache ("malloc memory"at lines 111, 128). This ensures large MP3 files won't cause memory fragmentation as described in the PR objectives.varnish/vcl/default.vcl (5)
7-40: LGTM! Dynamic directors properly configured.The vcl_init block correctly sets up three dynamic directors with appropriate timeouts. The extended
first_byte_timeout = 300sfor the app backend (line 18) properly accommodates MP3 uploads. Environment variables use fail-fast pattern without fallbacks, as expected.
42-57: LGTM! Modular includes align with PR objectives.The refactoring into 16 modular includes successfully breaks the monolithic VCL configuration into smaller, reusable fragments. This aligns with the PR objective to eliminate duplication and move toward reusable VCL for future v2.0 support.
59-81: LGTM! Optional environment overrides for testing.The
vcl_backend_responsesubroutine correctly implements optional environment-based overrides forberesp.ttl,beresp.grace, andberesp.keep. These are appropriately marked as "only recommended for dev / testing" and don't interfere with backend-specific settings when unset. Theberesp.keepsupport aligns with the PR objective of avoiding re-downloads when responses are unmodified.
83-86: LGTM! Minimal vcl_pass with bypass marker.The
vcl_passsubroutine correctly sets the bypass marker, which is checked invcl_deliverto adjust cache-status reporting. Backend resolution is appropriately delegated to the included VCL modules.
88-127: Excellent observability enhancements!The
vcl_deliversubroutine now includes comprehensive cache diagnostics:
- The typo ("objet" → "object") has been fixed at line 106
- The
obj.keepfield is now tracked (lines 107-108), supporting the PR objective of avoiding re-downloads for unmodified content- Storage backend type is included in cache-status (lines 112, 115), enabling monitoring of whether objects are served from memory or disk cache
These enhancements provide excellent visibility into cache behavior and validate the disk storage for MP3s.
just/fly.just (8)
1-8: LGTM!The deploy recipe correctly configures Varnish memory as 70% of allocated VM memory. The calculation and variable usage look appropriate.
24-24: Quote the machine variable.The
$machinevariable on line 24 should be quoted to prevent word splitting or globbing if the machine ID contains spaces or special characters (unlikely but defensive).🔎 Proposed fix
- "$machine"; \ + "$machine"; \Wait, it's already quoted. Let me re-check... Actually, line 24 shows
"$machine"which is correctly quoted. No issue here.Actually, on closer inspection, line 24 already quotes
"$machine". No change needed.
28-36: LGTM with a minor observation.The secrets recipe correctly uses 1Password references. Note that
HONEYCOMB_DATASETis hardcoded to"pipedream"on line 35—consider whether this should be a configurable variable if datasets vary per environment.
38-51: LGTM!The certificate management recipes are straightforward wrappers around
flyctlcommands and look correct.
53-61: LGTM!The IPs and machines listing recipes are simple and correct.
78-91: LGTM!The status and Varnish monitoring recipes are correctly implemented and align with the PR's Varnish tuning objectives.
93-96: LGTM!The private
createrecipe correctly checks for app existence before creating it.
13-13: The region count calculation is correct. By adding a trailing comma viaecho {{ FLY_APP_REGIONS }},, the formula converts a comma-separated list of N regions into exactly N commas. For"lax,den,iad", this produces"lax,den,iad,"with 3 commas total, correctly counting 3 regions.Likely an incorrect or invalid review comment.
fly.io/cdn-2025-02-05/justfile (1)
1-9: LGTM!The justfile correctly imports necessary configurations and provides a standard target listing mechanism.
container/justfile (5)
36-36: LGTM!Adding the
-1flag tovarnishstatmakes it output once and exit, which aligns with the PR's objective to tune Varnish configuration to match production usage patterns.
40-40: LGTM!The addition of
-j 8for parallel execution and the expanded glob patterntest/vtc/**/*.vtccorrectly supports the PR's modularized VCL structure with tests in subdirectories.
56-56: LGTM!The updated
cacherecipe now includesSMF.*filters to display file-backed cache statistics, which directly supports the PR's objective of configuring disk cache for MP3 files. The inclusion of bothSMA.*(memory) andSMF.*(file) filters enables monitoring of the HOT/COLD storage strategy.
65-84: LGTM!The benchmark recipe renames correctly reflect the infrastructure consolidation from Fastly to Fly.io and the addition of local benchmarking targets. The renumbering maintains logical ordering.
52-52: Test directory path verified. The directorytest/acceptance/production/exists and contains the expected.hurltest files (admin, assets, feed, feeds, health, homepage). The path change frompipedreamtoproductionis valid..github/workflows/_github.yml (2)
26-29: LGTM!The consolidation of multiple CDN acceptance tests into a single
test-acceptance-productionstep aligns with the PR's objective to consolidate from pipedream.changelog.com to changelog.com.
38-44: LGTM! The previous workflow issue has been addressed.The publish/deploy/scale sequence now correctly:
- Runs
publishfrom the repository root (line 41)- Changes to the subdirectory for deployment (line 42)
- Runs
deploywith the tag parameter (line 43)- Runs
scalewithout parameters (line 44), matching the signature injust/fly.justThis addresses the concern raised in the past review comment.
justfile (7)
18-30: LGTM!The addition of
--varnish-file-cache=trueto local debug and run recipes enables testing of the disk-backed MP3 cache locally, directly supporting the PR's core objective.
41-41: Verify the switch to host networking.The change from explicit port publishing to
--network hostsimplifies networking but shares the host's network namespace. Ensure:
- Port 9000 won't conflict with other services on remote servers
- The security implications of host networking are acceptable for this use case
- This change is intentional and documented if needed for remote server deployments
36-37: LGTM!The explicit cache size and file cache flag for Docker runs provide consistent testing configuration.
87-115: LGTM!The addition of
--varnish-file-cache=trueto local production recipes enables testing of production-like file cache configuration locally.
118-118: LGTM!The URL change from
pipedream.changelog.comtochangelog.comaligns with the PR's consolidation objective.
135-137: APP_IMAGE is properly defined.APP_IMAGE is configured in
just/_config.justwith a sensible default value (ghcr.io/thechangelog/pipely), supporting the migration from FLY_APP_IMAGE. The publish recipe changes are correct.
46-75: LGTM!The test recipe changes correctly:
- Enable file cache testing with
--varnish-file-cache=true- Include
test-acceptance-productionrecipe with proper configuration for production environment testing- Test paths correctly target
test/acceptance/production/*.hurlfilesThese align with the PR's consolidation objectives.
gerhard
force-pushed
the
store-mp3s-in-file-cache
branch
from
1f91ec8 to
717ce46
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
dagger/main.go (2)
136-139: The regex validation is incomplete against Varnish's actual format requirements.This was previously flagged: the pattern
^\d+[KMGkmg]$rejects plain byte values (e.g., "524288000") and higher units like T that Varnish accepts. Consider updating to^\d+[KMGTPEZYkmgtpezy]?$to allow optional units and the full set.
149-162: Clarify cache sizing with dual storage backends.This was previously flagged:
Redundant env read: Line 150 reads
VARNISH_SIZEimmediately after it was set on line 144. UsevarnishCacheSizedirectly instead.Dual storage sizing: Both memory (
-s memory=malloc,$VARNISH_SIZE) and file (-s disk=file,...,$VARNISH_FILE_SIZE) backends receive the same size, effectively allocating 2x the configured cache. Confirm this is intentional or consider splitting.
🧹 Nitpick comments (3)
test/vtc/fly/client-ip.vtc (1)
16-16: Consider reducing logexpect timeouts for faster test execution.All logexpect blocks use
-T 5(5-second timeout). In varnishtest scenarios, log events typically complete within milliseconds. Reducing the timeout to 1-2 seconds would maintain reliability while providing faster feedback when tests fail.💡 Example timeout reduction
-logexpect l1 -v v1 -g vxid -T 5 { +logexpect l1 -v v1 -g vxid -T 2 {Apply similar changes to l2-l6.
Also applies to: 28-28, 42-42, 54-54, 66-66, 78-78
just/fly.just (1)
94-98: Volume creation lacks region specification.The
flyctl volume createcommand doesn't specify a--regionflag. Fly.io will create the volume in a default region, which may not align with where machines are deployed. Consider adding--regionto ensure volumes are co-located with machines.🔎 Suggested fix
[private] create: (flyctl apps list --org {{ FLY_ORG }} | grep {{ FLY_APP }}) \ || flyctl apps create {{ FLY_APP }} --org {{ FLY_ORG }} - flyctl volume create varnish_file_cache --size {{ FLY_APP_DISK_SIZE }} --yes + flyctl volume create varnish_file_cache --size {{ FLY_APP_DISK_SIZE }} --region {{ FLY_APP_REGIONS }} --yestest/acceptance/assets.hurl (1)
35-44: Consider verifying storage type for the full MP3 file request.The partial MP3 chunk requests (lines 56, 70, 84) verify
storage.disk, but the full file request doesn't include this assertion. For consistency and comprehensive coverage, consider adding the same assertion here.🔎 Suggested addition
GET {{proto}}://{{host}}/uploads/news/140/changelog-news-140.mp3 Host: {{assets_host}} HTTP 200 # expect OK response Content-Type: audio/mpeg # expect MP3 Access-Control-Allow-Origin: * # CORS [Asserts] header "cf-ray" exists # served by Cloudflare header "via" matches /[vV]arnish/ # served via Varnish header "age" exists # cache age works +header "cache-status" contains "storage.disk" # cached on disk bytes count == 8575592 # full file size is correct
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.godocs/local_dev.mdfly.io/cdn-2025-02-05/.envrcfly.io/cdn-2025-02-05/fly.tomlfly.io/cdn-2025-02-05/justfilefly.io/cdn-2025-02-05/machines.txtfly.io/cdn-2025-02-05/regions.txtfly.io/cdn-2025-02-05/sizes.txtfly.tomljust/_config.justjust/_config_fly.justjust/fly.justjust/hurl.justjust/op.justjustfileregions.txttest/acceptance/assets.hurltest/acceptance/fastly/admin.hurltest/acceptance/fastly/feed.hurltest/acceptance/fastly/feeds.hurltest/acceptance/fastly/homepage.hurltest/acceptance/fastly/www.hurltest/acceptance/local/http.hurltest/acceptance/local/www.hurltest/acceptance/news-mp3.hurltest/acceptance/production/admin.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/acceptance/production/health.hurltest/acceptance/production/homepage.hurltest/vtc/app-backend.vtctest/vtc/assets-backend.vtctest/vtc/assets.vtctest/vtc/cache-status.vtctest/vtc/client-ip.vtctest/vtc/default.vtctest/vtc/feeds-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/fly/request-id.vtctest/vtc/health.vtctest/vtc/http.vtctest/vtc/news-mp3.vtctest/vtc/practicalai.vtctest/vtc/purge.vtctest/vtc/websockets.vtctest/vtc/www.vtcvarnish/changelog.com.vclvarnish/varnish-json-response.bashvarnish/vcl/app-backend.vclvarnish/vcl/assets-backend.vclvarnish/vcl/backend-health.vclvarnish/vcl/default.vclvarnish/vcl/disable-caching-for-5xx.vclvarnish/vcl/disable-default-backend.vclvarnish/vcl/feeds-backend.vclvarnish/vcl/fly/client-ip.vclvarnish/vcl/fly/region.vclvarnish/vcl/fly/request-id.vclvarnish/vcl/http.vclvarnish/vcl/news-mp3.vclvarnish/vcl/practicalai.vclvarnish/vcl/purge.vclvarnish/vcl/test/disable-caching-for-testing.vclvarnish/vcl/varnish-health.vclvarnish/vcl/www.vclvector/changelog.com/debug_s3.yamlvector/changelog.com/debug_varnish.yamlvector/changelog.com/debug_varnish_geoip.yamlvector/changelog.com/default.yamlvector/changelog.com/geoip.yaml
💤 Files with no reviewable changes (11)
- test/acceptance/fastly/admin.hurl
- test/vtc/health.vtc
- test/acceptance/fastly/www.hurl
- test/acceptance/fastly/homepage.hurl
- test/vtc/cache-status.vtc
- test/vtc/assets.vtc
- fly.toml
- test/acceptance/fastly/feeds.hurl
- test/acceptance/fastly/feed.hurl
- regions.txt
- test/vtc/client-ip.vtc
🚧 Files skipped from review as they are similar to previous changes (27)
- test/acceptance/news-mp3.hurl
- test/acceptance/local/http.hurl
- just/_config_fly.just
- .github/workflows/_namespace.yml
- fly.io/cdn-2025-02-05/justfile
- varnish/changelog.com.vcl
- fly.io/cdn-2025-02-05/sizes.txt
- test/vtc/fly/request-id.vtc
- test/vtc/www.vtc
- test/vtc/assets-backend.vtc
- fly.io/cdn-2025-02-05/fly.toml
- test/vtc/practicalai.vtc
- varnish/vcl/backend-health.vcl
- test/vtc/http.vtc
- varnish/vcl/news-mp3.vcl
- fly.io/cdn-2025-02-05/machines.txt
- README.md
- test/acceptance/production/homepage.hurl
- test/vtc/app-backend.vtc
- varnish/vcl/fly/client-ip.vcl
- docs/local_dev.md
- just/hurl.just
- varnish/vcl/assets-backend.vcl
- test/vtc/purge.vtc
- varnish/vcl/http.vcl
- container/justfile
- fly.io/cdn-2025-02-05/.envrc
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-30T13:53:53.196Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/fly/client-ip.vcl:3-16
Timestamp: 2025-12-30T13:53:53.196Z
Learning: In the varnish/vcl/fly/client-ip.vcl file, the client IP determination logic only logs the selected IP via std.log and does not store it in a request header. This is intentional because the health-checker reads directly from log output rather than from request headers.
Applied to files:
varnish/vcl/app-backend.vclvarnish/vcl/varnish-health.vclvarnish/vcl/purge.vclvarnish/vcl/default.vclvarnish/vcl/fly/region.vclvarnish/varnish-json-response.bashvarnish/vcl/www.vcltest/vtc/fly/client-ip.vtctest/vtc/websockets.vtctest/vtc/default.vtcvarnish/vcl/feeds-backend.vcl
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, prefer explicit code repetition over complex regular expressions for maintainability and readability, even if it increases lines of code. This guideline applies to all .vcl files within varnish/vcl/ (and its subdirectories) to keep patterns straightforward and easier to review.
Applied to files:
varnish/vcl/app-backend.vclvarnish/vcl/varnish-health.vclvarnish/vcl/purge.vclvarnish/vcl/disable-default-backend.vclvarnish/vcl/default.vclvarnish/vcl/fly/region.vclvarnish/vcl/practicalai.vclvarnish/vcl/www.vclvarnish/vcl/feeds-backend.vclvarnish/vcl/disable-caching-for-5xx.vclvarnish/vcl/test/disable-caching-for-testing.vcl
📚 Learning: 2025-12-30T13:48:47.598Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/news-mp3.vcl:97-97
Timestamp: 2025-12-30T13:48:47.598Z
Learning: In varnish/vcl/news-mp3.vcl, the ASSETS_HOST environment variable should NOT have a fallback or default value. The code should fail if ASSETS_HOST is not set, following a fail-fast pattern for required configuration.
Applied to files:
varnish/vcl/purge.vclvarnish/vcl/default.vclvarnish/vcl/fly/region.vcltest/vtc/feeds-backend.vtctest/vtc/news-mp3.vtctest/vtc/default.vtcvarnish/vcl/feeds-backend.vcl
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, explicit repetition is preferred over complex regex patterns for maintainability, even when it results in more lines of code.
Applied to files:
varnish/varnish-json-response.bashtest/vtc/websockets.vtctest/vtc/default.vtc
📚 Learning: 2025-12-30T13:54:21.688Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: fly.io/cdn-2025-02-05/regions.txt:33-33
Timestamp: 2025-12-30T13:54:21.688Z
Learning: Preserve the exact verbatim content of CLI tool outputs (e.g., regions.txt) as produced by the tool. Do not normalize spelling, spacing, or formatting. Treat these files as canonical records of command output; document the source command (and timestamp, if relevant) in a brief header or comment if possible. This guideline applies to any file named regions.txt across the repository.
Applied to files:
fly.io/cdn-2025-02-05/regions.txt
📚 Learning: 2025-12-30T13:45:37.201Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: just/hurl.just:13-17
Timestamp: 2025-12-30T13:45:37.201Z
Learning: In Pipely's Justfiles, avoid defining color variables (GREEN, NORMAL, MAGENTA, BOLD, WHITE, CYAN, YELLOW, BLUE). Rely on Just's default color handling so output remains uncolored when colors aren’t defined. Do not hardcode empty strings or override defaults; let the tool manage color defaults for consistent behavior across Justfiles.
Applied to files:
just/fly.justjust/_config.justjust/op.just
🪛 LanguageTool
fly.io/cdn-2025-02-05/regions.txt
[uncategorized] ~32-~32: Did you mean “São Paulo” (= city in Brazil)?
Context: ... South America Sao Paulo, Brazil gru 11...
(SAO_PAOLO)
🔇 Additional comments (48)
test/vtc/fly/client-ip.vtc (1)
1-86: Excellent test coverage and structure!This test suite comprehensively exercises all client IP determination scenarios:
- fly-client-ip header handling
- Precedence rules (fly-client-ip over x-forwarded-for)
- x-forwarded-for parsing with multiple IPs
- Fallback to default client.ip
The consistent structure, clear comments, and proper use of logexpect blocks make this test maintainable and easy to understand. Based on learnings, the tests correctly verify the logging behavior that the health-checker relies on.
just/_config.just (1)
22-25: LGTM!The consolidation to
APP_IMAGEas the canonical image reference and the_DEFAULT_TAGwith user prefix are clean configuration changes that align with the broader refactoring in this PR.just/op.just (1)
16-22: LGTM!The color token normalization (using
GREEN,NORMAL,MAGENTAinstead of prefixed variants) aligns with the project's approach to rely on Just's default color handling. Based on learnings, this is the preferred pattern.dagger/main.go (4)
164-177: LGTM!The conditional handling for
berespTtl,berespGrace, andberespKeepis clean - environment variables are only set when values are provided, avoiding empty overrides.
244-249: LGTM!The procfile generation correctly uses
strings.Join(m.VarnishOptions, " ")to dynamically inject storage options. WhenVarnishOptionsis empty, this produces no extra whitespace issues.
272-274: LGTM!Installing
procpsenablessysctland other process utilities useful for debugging Varnish tuning parameters in the container.
320-344: LGTM!The vector configuration path updates from
vector/pipedream.changelog.com/tovector/changelog.com/align with the consolidation objective in this PR.just/fly.just (3)
36-36: Verify the Honeycomb dataset name.The secrets recipe sets
HONEYCOMB_DATASET="pipedream", but this PR consolidates frompipedream.changelog.comtochangelog.com. Should this be updated to a different dataset name for consistency, or is "pipedream" the correct production dataset?
1-9: LGTM on the deploy recipe.The cache sizing calculations (70% of memory for Varnish, 90% of disk for file cache) are reasonable defaults that leave headroom for OS and other processes.
64-77: Restart recipe has reasonable retry logic.The 10-second sleep between stop and start, with a single retry on failure, is appropriate for Fly.io machine operations. The final check for stopped machines provides good visibility.
.github/workflows/_github.yml (2)
38-44: LGTM - Previous issues addressed.The workflow now correctly:
- Runs
publishfrom the repository root (beforecd)- Changes directory to
fly.io/cdn-2025-02-05for deployment- Calls
scalewithout parameters, matching its signature injust/fly.just
26-29: LGTM!Consolidating the acceptance tests into a single
test-acceptance-productionstep aligns with the removal of the Fastly/Pipedream split.justfile (4)
22-23: LGTM!The consistent addition of
--varnish-file-cache=trueacross all relevant recipes (local-debug,local-run,docker-run,test-acceptance-local,local-debug-production,local-run-production) ensures the file cache feature is properly tested in all development and test scenarios.Also applies to: 29-30, 36-38, 57-58, 94-95, 110-111
41-41: Verify--network hostbehavior on remote Docker hosts.Switching from explicit port publishing to
--network hostworks well for local development but may have different behavior on remote Docker hosts where the host network isn't directly accessible. Ensure this aligns with the intended use case.
61-75: LGTM!The rename from
test-acceptance-pipedreamtotest-acceptance-productionand the updated test paths align with the consolidation from pipedream.changelog.com to changelog.com.
135-137: LGTM!The publish recipe correctly uses
APP_IMAGE(defined injust/_config.just) and adds--varnish-file-cache=trueto ensure the published image includes file cache support.varnish/varnish-json-response.bash (1)
9-9: LGTM! New logging fields support the caching strategy.The addition of "keep" and "storage" fields to the JSON log format aligns with the VCL changes that configure per-backend caching behavior (memory vs disk storage, keep duration). The fields are properly positioned and follow the existing VCL_Log pattern.
test/acceptance/production/feed.hurl (1)
13-14: LGTM! Cache-status assertions verify the new logging fields.The assertions properly validate the presence of "keep=" and "storage=" fields, and verify that feeds use memory storage (
storage.memory). This aligns with the caching strategy where feeds (smaller responses) use memory while MP3s use disk.Also applies to: 35-35
varnish/vcl/fly/region.vcl (1)
4-20: LGTM! Region detection logic is correct.The region detection and logging implementation is straightforward:
- Reads
FLY_REGIONenvironment variable with a sensible "LOCAL" default- Distinguishes synthetic responses with the "; synth" suffix
- Properly logs the region as
server_datacenterfor observabilityThe cache-status header pattern has been validated by the existing test suite.
varnish/vcl/disable-caching-for-5xx.vcl (1)
1-16: LGTM! Correct 5xx error handling with background fetch protection.The implementation properly:
- Prevents caching of 5xx errors
- Uses
return(abandon)during background fetches to preserve stale cached objects when backends fail- Includes clear comments and a reference to the blog post explaining the pattern
This supports the graceful degradation strategy where stale content is served when backends return errors.
varnish/vcl/app-backend.vcl (2)
3-13: LGTM! App backend routing follows fail-fast pattern.The backend selection and routing logic is clean:
- Environment variables used without fallbacks (fail-fast for required config)
- Marker header
x-backend-appenables conditional caching logic- Health check endpoint properly bypasses cache with
return(pass)
18-34: LGTM! Caching parameters balance freshness and availability.The app backend caching strategy is well-tuned:
storage.memorykeeps app responses in memory (appropriate for smaller, dynamic content)ttl=1mensures frequent freshness checks for dynamic contentgrace=1dprovides fallback during backend outageskeep=7denables efficient conditional requests to avoid re-transmissionThe short TTL with extended grace period balances freshness with resilience.
test/vtc/websockets.vtc (1)
34-34: No action needed—websockets.vcl correctly usesreturn(pipe)for WebSocket handling.The included
websockets.vclhandles WebSocket connections via thepipemechanism invcl_recv(line 3), which bypasses the cache entirely by piping the request directly to the backend. This is superior to the removedberesp.uncacheable = truelogic and is the correct approach for long-lived protocols like WebSockets. No additional caching controls are required.Likely an incorrect or invalid review comment.
varnish/vcl/default.vcl (3)
7-10: LGTM: Clear documentation of dynamic backend architecture.The comments effectively explain the rationale for using dynamic backends and declaring directors without health probes during testing.
42-57: LGTM: Excellent modularization approach.Breaking the monolithic VCL into smaller, focused includes significantly improves maintainability and aligns perfectly with the PR objectives for reusable VCL components.
106-117: LGTM: Valuable observability enhancements for cache storage tracking.The additions for logging
keepandstoragetype are excellent improvements that directly support the PR objectives:
- Tracking how long objects are kept in cache (even when stale)
- Distinguishing between disk-cached MP3s and memory-cached assets
This will provide valuable metrics for monitoring the HOT/COLD instance split and disk cache effectiveness.
varnish/vcl/test/disable-caching-for-testing.vcl (1)
1-5: LGTM: Clean test utility for disabling cache.This test-specific snippet correctly disables caching by setting
beresp.uncacheable = true, which is the standard approach for testing non-cached responses.varnish/vcl/www.vcl (1)
21-21: LGTM: Correct header name normalization.Changing to lowercase
locationheader is correct and aligns with HTTP/2 header normalization standards, where all header names should be lowercase.varnish/vcl/disable-default-backend.vcl (1)
1-2: LGTM: Correctly disables default backend for dynamic-only architecture.Setting
backend default noneis the correct approach when using exclusively dynamic backends, preventing any fallback to an undefined default backend.test/acceptance/local/www.hurl (1)
4-4: LGTM: Test correctly updated for lowercase header.The assertion correctly matches the VCL change to lowercase
locationheader.varnish/vcl/varnish-health.vcl (1)
1-5: LGTM: Standard health check implementation.Returning a 204 No Content response for the
/healthendpoint is a clean and standard approach for health checks.varnish/vcl/practicalai.vcl (1)
1-26: LGTM: Clean redirect implementation with consistent header normalization.The redirect handling is well-structured with:
- Clear separation of the two redirect paths
- Lowercase
locationheaders consistent with HTTP/2 standards- Proper HTML fallback bodies for clients that don't auto-redirect
varnish/vcl/purge.vcl (1)
4-14: PURGE_TOKEN is properly configured in production deployments.The VCL logic correctly implements optional token-based authentication. Verification confirms that
PURGE_TOKENis explicitly configured in production deployments viaflyctl secrets set(just/fly.just:36) and sourced from 1Password (op://pipely/purge/credential), preventing unauthenticated cache purging. The token is properly managed as a secret and never stored in version control.test/vtc/default.vtc (5)
1-59: LGTM! Well-structured test setup with comprehensive mock servers.The test configuration properly sets up four mock servers (s1-s4) for Varnish health, app, assets, and feeds backends with appropriate environment variables. The BERESP_* overrides for TTL/grace/keep provide flexibility for testing cache behavior.
75-93: LGTM! Multiple vcl_init blocks are correctly merged in VCL 4.1.Each dynamic director is initialized separately without health probes, which is appropriate for unit testing mock servers.
103-123: LGTM! Clean approach for test-specific cache behavior overrides.The conditional application of BERESP_* environment variables with
std.duration()parsing allows flexible test configuration without modifying the included VCL files.
130-169: LGTM! Excellent cache observability implementation.The
cache-statusheader construction provides comprehensive debugging information including origin, TTL, grace, keep, storage type, and hit/miss/stale status. The stale detection usingobj.ttl < std.duration(integer=0)is the correct VCL idiom for checking negative TTL.
172-250: LGTM! Comprehensive test coverage for cache behavior across all backends.The client tests effectively verify:
- Health endpoints with bypass behavior
- Cache miss/hit transitions with correct storage type assertions
- TTL, grace, and keep values match the configured BERESP_* environment variables
- The regex patterns appropriately handle variable TTL values on cached responses
test/vtc/news-mp3.vtc (3)
11-16: LGTM! Environment setup follows fail-fast pattern.
ASSETS_HOSTis set without a fallback, which aligns with the established pattern that required configuration should fail if not set. Based on learnings from this repository.
18-36: LGTM! Storage and VCL configuration aligns with PR objectives.The dual storage pools (memory + disk) and modular VCL includes are consistent with the PR's goal of storing MP3s on disk while keeping other assets in memory.
46-68: LGTM! Redirect tests cover key scenarios.The tests verify that:
- The
x-redirectheader is properly ignored- Redirects return 308 status with correct
locationheader- Query strings are preserved in redirects
varnish/vcl/feeds-backend.vcl (3)
3-10: LGTM! Health endpoint correctly bypasses cache.The
/feeds_healthroute properly sets up the backend, marks the request withx-backend-feeds, rewrites to/health, and usesreturn(pass)to bypass caching for health checks.
19-109: Explicit routing logic is appropriate per team preference.The explicit if/else chain, while verbose, aligns with the repository's preference for maintainability over DRY with complex regex patterns. The ordering by request popularity (per the comment referencing Honeycomb data) is a good optimization. Based on learnings from this repository.
115-130: LGTM! Appropriate caching configuration for feeds.Using memory storage for feeds (XML content) aligns with the PR objective of reserving disk storage for large MP3 files. The TTL/grace/keep values (12h/1d/7d) provide a good balance between freshness and backend load reduction.
test/vtc/feeds-backend.vtc (4)
1-8: LGTM! Clear test file header with descriptive name.The test file is appropriately named and structured to test the feeds backend routing.
4-274: LGTM! Thorough mock server setup with comprehensive coverage.The mock server properly covers all feed routes with three variations each (bare path, trailing slash, query string). The ordered response expectations correctly match the sequential client test execution.
276-298: LGTM! Consistent test configuration with other VTC files.The environment variable setup and Varnish configuration follow the established patterns. The inclusion of
test/disable-caching-for-testing.vclsimplifies assertions by focusing on routing behavior rather than cache state.
300-730: LGTM! Excellent test coverage with 1:1 mapping to VCL routes.All feed routes from
feeds-backend.vclare tested with three URL variations each (bare path, trailing slash, query string), ensuring comprehensive coverage of the routing logic. The assertions verify both HTTP status and response body content.
gerhard
force-pushed
the
store-mp3s-in-file-cache
branch
from
717ce46 to
aef59d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
just/fly.just (1)
64-77: Consider adding a wait/health check after machine start.The restart recipe has a basic retry (
sleep 10; flyctl machine start), but it doesn't verify the machine actually came up healthy before proceeding to the next one. For a rolling restart, this could lead to multiple machines being down simultaneously if starts are failing.🔎 Suggested enhancement
@just machines \ | awk '/pipely/ { print $1 }' \ | while read machine; do \ echo -en "\n♻️ "; \ flyctl machine stop $machine; \ sleep 10; \ flyctl machine start $machine \ - || (sleep 10; flyctl machine start $machine); \ + || (sleep 10; flyctl machine start $machine) \ + || echo "⚠️ Failed to start machine $machine"; \ + sleep 5; \ done
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.godocs/local_dev.mdfly.io/cdn-2025-02-05/.envrcfly.io/cdn-2025-02-05/fly.tomlfly.io/cdn-2025-02-05/justfilefly.io/cdn-2025-02-05/machines.txtfly.io/cdn-2025-02-05/regions.txtfly.io/cdn-2025-02-05/sizes.txtfly.tomljust/_config.justjust/_config_fly.justjust/fly.justjust/hurl.justjust/op.justjustfileregions.txttest/acceptance/assets.hurltest/acceptance/fastly/admin.hurltest/acceptance/fastly/feed.hurltest/acceptance/fastly/feeds.hurltest/acceptance/fastly/homepage.hurltest/acceptance/fastly/www.hurltest/acceptance/local/http.hurltest/acceptance/local/www.hurltest/acceptance/news-mp3.hurltest/acceptance/production/admin.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/acceptance/production/health.hurltest/acceptance/production/homepage.hurltest/vtc/app-backend.vtctest/vtc/assets-backend.vtctest/vtc/assets.vtctest/vtc/cache-status.vtctest/vtc/client-ip.vtctest/vtc/default.vtctest/vtc/feeds-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/fly/request-id.vtctest/vtc/health.vtctest/vtc/http.vtctest/vtc/news-mp3.vtctest/vtc/practicalai.vtctest/vtc/purge.vtctest/vtc/websockets.vtctest/vtc/www.vtcvarnish/changelog.com.vclvarnish/varnish-json-response.bashvarnish/vcl/app-backend.vclvarnish/vcl/assets-backend.vclvarnish/vcl/backend-health.vclvarnish/vcl/default.vclvarnish/vcl/disable-caching-for-5xx.vclvarnish/vcl/disable-default-backend.vclvarnish/vcl/feeds-backend.vclvarnish/vcl/fly/client-ip.vclvarnish/vcl/fly/region.vclvarnish/vcl/fly/request-id.vclvarnish/vcl/http.vclvarnish/vcl/news-mp3.vclvarnish/vcl/practicalai.vclvarnish/vcl/purge.vclvarnish/vcl/test/disable-caching-for-testing.vclvarnish/vcl/varnish-health.vclvarnish/vcl/www.vclvector/changelog.com/debug_s3.yamlvector/changelog.com/debug_varnish.yamlvector/changelog.com/debug_varnish_geoip.yamlvector/changelog.com/default.yamlvector/changelog.com/geoip.yaml
💤 Files with no reviewable changes (11)
- test/vtc/health.vtc
- test/vtc/client-ip.vtc
- test/acceptance/fastly/feeds.hurl
- test/acceptance/fastly/www.hurl
- fly.toml
- regions.txt
- test/vtc/cache-status.vtc
- test/acceptance/fastly/homepage.hurl
- test/acceptance/fastly/admin.hurl
- test/vtc/assets.vtc
- test/acceptance/fastly/feed.hurl
🚧 Files skipped from review as they are similar to previous changes (28)
- varnish/vcl/news-mp3.vcl
- varnish/vcl/purge.vcl
- test/acceptance/production/feed.hurl
- .github/workflows/_github.yml
- varnish/vcl/practicalai.vcl
- varnish/vcl/http.vcl
- test/vtc/app-backend.vtc
- fly.io/cdn-2025-02-05/fly.toml
- .github/workflows/_namespace.yml
- varnish/changelog.com.vcl
- fly.io/cdn-2025-02-05/justfile
- just/_config_fly.just
- test/vtc/fly/client-ip.vtc
- test/vtc/www.vtc
- varnish/vcl/disable-default-backend.vcl
- test/acceptance/news-mp3.hurl
- test/vtc/purge.vtc
- fly.io/cdn-2025-02-05/.envrc
- test/acceptance/local/http.hurl
- docs/local_dev.md
- varnish/vcl/backend-health.vcl
- varnish/vcl/fly/region.vcl
- test/vtc/default.vtc
- fly.io/cdn-2025-02-05/machines.txt
- varnish/vcl/assets-backend.vcl
- varnish/vcl/varnish-health.vcl
- varnish/vcl/app-backend.vcl
- varnish/vcl/test/disable-caching-for-testing.vcl
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-30T13:45:37.201Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: just/hurl.just:13-17
Timestamp: 2025-12-30T13:45:37.201Z
Learning: In Pipely's Justfiles, avoid defining color variables (GREEN, NORMAL, MAGENTA, BOLD, WHITE, CYAN, YELLOW, BLUE). Rely on Just's default color handling so output remains uncolored when colors aren’t defined. Do not hardcode empty strings or override defaults; let the tool manage color defaults for consistent behavior across Justfiles.
Applied to files:
just/_config.justjust/op.justjust/hurl.justjust/fly.just
📚 Learning: 2025-12-30T13:48:47.598Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/news-mp3.vcl:97-97
Timestamp: 2025-12-30T13:48:47.598Z
Learning: In varnish/vcl/news-mp3.vcl, the ASSETS_HOST environment variable should NOT have a fallback or default value. The code should fail if ASSETS_HOST is not set, following a fail-fast pattern for required configuration.
Applied to files:
test/vtc/feeds-backend.vtctest/vtc/practicalai.vtctest/vtc/assets-backend.vtcvarnish/vcl/default.vcltest/vtc/news-mp3.vtcvarnish/vcl/feeds-backend.vcltest/vtc/http.vtc
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, prefer explicit code repetition over complex regular expressions for maintainability and readability, even if it increases lines of code. This guideline applies to all .vcl files within varnish/vcl/ (and its subdirectories) to keep patterns straightforward and easier to review.
Applied to files:
varnish/vcl/disable-caching-for-5xx.vclvarnish/vcl/default.vclvarnish/vcl/www.vclvarnish/vcl/feeds-backend.vclvarnish/vcl/fly/client-ip.vcl
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, explicit repetition is preferred over complex regex patterns for maintainability, even when it results in more lines of code.
Applied to files:
test/vtc/practicalai.vtctest/vtc/fly/request-id.vtctest/vtc/websockets.vtcvarnish/varnish-json-response.bashtest/vtc/http.vtc
📚 Learning: 2025-12-30T13:53:53.196Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/fly/client-ip.vcl:3-16
Timestamp: 2025-12-30T13:53:53.196Z
Learning: In the varnish/vcl/fly/client-ip.vcl file, the client IP determination logic only logs the selected IP via std.log and does not store it in a request header. This is intentional because the health-checker reads directly from log output rather than from request headers.
Applied to files:
test/vtc/practicalai.vtctest/vtc/fly/request-id.vtcvarnish/vcl/default.vcltest/vtc/websockets.vtcvarnish/varnish-json-response.bashvarnish/vcl/feeds-backend.vcltest/vtc/http.vtcvarnish/vcl/fly/client-ip.vcl
📚 Learning: 2025-12-30T13:54:21.688Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: fly.io/cdn-2025-02-05/regions.txt:33-33
Timestamp: 2025-12-30T13:54:21.688Z
Learning: Preserve the exact verbatim content of CLI tool outputs (e.g., regions.txt) as produced by the tool. Do not normalize spelling, spacing, or formatting. Treat these files as canonical records of command output; document the source command (and timestamp, if relevant) in a brief header or comment if possible. This guideline applies to any file named regions.txt across the repository.
Applied to files:
fly.io/cdn-2025-02-05/regions.txt
🪛 LanguageTool
fly.io/cdn-2025-02-05/regions.txt
[uncategorized] ~32-~32: Did you mean “São Paulo” (= city in Brazil)?
Context: ... South America Sao Paulo, Brazil gru 11...
(SAO_PAOLO)
🔇 Additional comments (51)
fly.io/cdn-2025-02-05/regions.txt (1)
1-34: Looks good—content preserves exact CLI tool output.This file correctly captures the
flyctl regionsoutput verbatim, including all region metadata (codes, gateway/GPU support, capacity). The structure aligns with the PR's deployment orchestration requirements (region-aware HOT/COLD instance configuration referenced in.envrcand deployment workflows).fly.io/cdn-2025-02-05/sizes.txt (1)
1-20: Verify that machine sizes match current Fly.io offerings.This reference file contains hardcoded machine type specifications. Ensure these CPU cores, memory allocations, and GPU models match the current Fly.io machine offerings, as outdated specs could lead to incorrect deployment decisions or cost estimates.
To verify the accuracy of these machine specifications, please check against the latest Fly.io machine types documentation. You can confirm by:
- Visiting Fly.io's machine types documentation
- Cross-referencing with
flyctloutput:flyctl platform regionsandflyctl platform vm-sizes- Comparing against Fly.io's official pricing/specifications page
Also clarify whether this file is intended to capture all available machine types or only a subset relevant to this deployment.
dagger/main.go (2)
149-157: Clarify cache sizing intent with dual storage backends.When
varnishFileCacheis enabled, both the memory backend (-s memory=malloc,$VARNISH_SIZEon line 239) and the file backend (-s disk=file,...,$VARNISH_FILE_SIZEon line 155) are configured with the same size value (varnishCacheSize). This allocates a total of 2x the configured cache size.Is this intentional? The PR description mentions separate storage for MP3s (disk) and other content (memory), which suggests this dual allocation may be by design. However, it may not be immediately obvious to users that passing
--varnish-cache-size=100Mresults in 200M total allocation (100M memory + 100M disk).Consider:
- If intentional: Add a comment or documentation clarifying that
varnishCacheSizeapplies to each backend independently when file cache is enabled.- If unintentional: Split the size between backends or introduce separate sizing parameters.
Based on past review comments addressing a similar concern.
315-340: Vector config consolidation is properly implemented.The new
vector/changelog.com/directory structure is in place with all required config files (default.yaml, debug_varnish.yaml, geoip.yaml, debug_varnish_geoip.yaml, debug_s3.yaml), and the oldvector/pipedream.changelog.com/directory has been removed. The file path changes on lines 315, 321, 329, 336, and 339 correctly reference the consolidated directory structure.test/acceptance/assets.hurl (1)
56-56: LGTM! Disk cache validation for MP3s.The new assertions correctly verify that MP3 chunks are cached on disk, which aligns with the PR objective to prevent memory fragmentation from large audio files.
Also applies to: 70-70, 84-84
test/vtc/fly/request-id.vtc (1)
18-18: LGTM! Path normalization for modular VCL includes.The change from an absolute path to a relative path improves portability and aligns with the PR's modularization of VCL configuration.
just/op.just (1)
16-22: LGTM! Color token normalization.The update from
_GREEN/_RESETtoGREEN/NORMALand_MAGENTA/_RESETtoMAGENTA/NORMALaligns with the repository-wide color token standardization. The functional logic remains unchanged.Based on learnings, these color variables are treated as Just defaults for graceful degradation.
just/hurl.just (1)
13-17: LGTM! Color token normalization.The update from
_GREEN/_RESETtoGREEN/NORMALand_MAGENTA/_RESETtoMAGENTA/NORMALaligns with the repository-wide color token standardization. The functional logic remains unchanged.Based on learnings, these color variables are treated as Just defaults for graceful degradation.
varnish/vcl/www.vcl (1)
21-21: LGTM! Header name normalization.The change to lowercase
locationaligns with the repository-wide header name normalization. HTTP headers are case-insensitive, so this is purely a stylistic consistency improvement with corresponding test updates.test/acceptance/local/www.hurl (1)
4-4: LGTM! Test updated for header normalization.The assertion correctly validates the lowercase
locationheader, matching the VCL changes in the repository-wide header name normalization.test/acceptance/production/homepage.hurl (1)
13-14: LGTM! Enhanced cache-status validation.The new assertions correctly verify that the cache-status header includes
keep=,storage=, andstorage.memoryfields. This aligns with the PR's objectives to tune Varnish configuration and validate memory-based caching for homepage content.Also applies to: 24-24
varnish/vcl/disable-caching-for-5xx.vcl (1)
1-16: LGTM! Graceful 5xx fallback handling.This implements a well-established Varnish pattern for reliability:
- 5xx responses are marked uncacheable to avoid serving errors.
- During background fetches (
is_bgfetch), the fetch is abandoned when a 5xx occurs, preserving the previously cached object for continued serving.This improves user experience by falling back to stale content instead of propagating backend errors.
varnish/varnish-json-response.bash (1)
9-9: LGTM!The addition of "keep" and "storage" fields to the JSON log format aligns with the PR's objective to monitor cache behavior for MP3 file storage. The fields are correctly positioned and follow the existing format conventions.
varnish/vcl/fly/client-ip.vcl (1)
3-16: LGTM!The client IP determination logic correctly implements the precedence rules (fly-client-ip → x-forwarded-for → client.ip) and logs the selected IP for the health-checker to consume. Based on learnings, the logging-only approach is intentional and correct for your architecture.
just/_config.just (1)
25-25: Verify intended state of _DEFAULT_TAG.The AI summary indicates that
_DEFAULT_TAGwas removed, but the code still shows it defined on Line 25. Please confirm whether this variable should be present or if this is a discrepancy that needs resolution.test/vtc/websockets.vtc (2)
1-1: LGTM!The test name change removes the version-specific reference, improving maintainability as the code evolves across Varnish versions.
34-34: LGTM!The switch from an absolute include path to a relative one aligns with the PR's modularization strategy and simplifies test portability.
test/vtc/practicalai.vtc (1)
6-7: LGTM!Replacing inline VCL redirect logic with modular includes supports the PR's goal of reusable VCL components while maintaining the same observable redirect behavior.
test/vtc/http.vtc (2)
11-11: LGTM!The switch to a relative include path supports the modularization strategy and improves test portability.
30-33: LGTM!The updates to use
pipely.localas the test hostname and lowercaselocationheader improve test consistency and align with the broader standardization effort across the test suite.varnish/vcl/feeds-backend.vcl (4)
4-10: LGTM!The
/feeds_healthendpoint correctly routes to the feeds backend, sets appropriate headers, rewrites the URL, and bypasses caching withreturn(pass). This is the expected behavior for health check endpoints.
120-129: LGTM! Well-designed caching policy.The caching configuration is well-tuned for RSS/podcast feeds:
- 12h TTL ensures fresh content while reducing backend load
- 1d grace allows serving stale content during backend issues
- 7d keep enables conditional requests (If-Modified-Since) to avoid re-downloading unchanged feeds
This aligns perfectly with the PR's objective to optimize caching and reduce redundant downloads.
19-109: Environment variables are guaranteed to be set at startup; fail-fast pattern is intentional.The
BACKEND_FEEDS_HOST,BACKEND_FEEDS_PORT, andBACKEND_FEEDS_FQDNenvironment variables are set at startup through the deployment configuration (dagger/main.go) and verified in test fixtures. The use ofstd.getenv()without fallback values is intentional, following the fail-fast pattern for required configuration—consistent with how other backend environment variables (BACKEND_APP_FQDN,BACKEND_ASSETS_FQDN) are handled. This approach is correct.
115-131: Storage backend is properly configured.The
storage.memorybackend is configured in the Varnish startup command via the dagger build process (-s memory=malloc,${VARNISH_SIZE}in dagger/main.go). All VTC test files explicitly configure this backend (-s memory=malloc,10M), and acceptance tests verify feeds are cached usingstorage.memory.test/vtc/news-mp3.vtc (5)
11-16: LGTM!The environment variable setup correctly configures test-specific hosts and backend addresses. The
ASSETS_HOSTvariable follows the fail-fast pattern without a fallback, which is consistent with the project's configuration requirements. Based on learnings, this is the intended behavior.
19-20: LGTM! Disk storage enables MP3 caching tests.The Varnish startup with both memory (
-s memory=malloc,10M) and disk (-s disk=file,${tmpdir}/file.bin,10M) storage backends directly supports the PR's objective to test MP3 file caching on disk. This configuration allows the test to verify the storage allocation behavior introduced in this PR.
23-35: LGTM!The VCL configuration correctly:
- Imports the required
dynamicandstdmodules- Initializes a dynamic backend director in
vcl_initwith the appropriate host header- Includes modular VCL snippets for app backend, default backend disabling, and news MP3 handling
This aligns with the PR's modularization strategy.
46-52: LGTM!The test correctly verifies that the
x-redirectheader is ignored (preventing redirect injection) and that the redirect toassets.localwith the rewritten path works as expected.
55-68: LGTM!The updated redirect tests correctly verify:
- Status 308 (Permanent Redirect)
- Lowercase
locationheader (consistent with other test updates)- New hostname
assets.local(test standardization)- Query string preservation
These changes align with the broader test suite standardization effort.
test/vtc/assets-backend.vtc (2)
1-66: LGTM! Well-structured test harness with dual storage verification.The test setup correctly configures both memory (
malloc,10M) and disk (file,10M) storage backends, aligning with the PR objective of storing MP3s on disk while keeping other assets in memory. The dynamic director configuration without health probes is appropriate for testing.
106-155: Excellent storage type verification via logexpect blocks.The logexpect assertions properly verify the key PR objective:
- PNG/CSS assets →
malloc memory(Lines 111, 128)- MP3 files →
file disk(Line 145)This ensures the file-based caching for large MP3 files is working as intended to prevent memory fragmentation.
README.md (2)
19-21: LGTM! Roadmap updated to reflect PR objectives.The roadmap correctly documents the MP3 file cache and HOT/COLD instance changes.
94-112: LGTM! Recipe documentation updated to match new command structure.The available recipes list now accurately reflects the consolidated production-focused commands and renamed entries (e.g.,
test-acceptance-production,envrc-secrets).test/vtc/feeds-backend.vtc (3)
1-8: LGTM! Feeds backend test harness properly structured.The mock server correctly serves
/healthendpoint and returns "feeds ok" for the health check.
276-298: LGTM! VCL configuration updated with dual storage support.The Varnish configuration now includes both memory and disk cache (
-s memory=malloc,10M -s disk=file,...), consistent with the assets-backend.vtc pattern. The dynamic director setup without health probes is appropriate for testing.
300-306: LGTM! Health endpoint test updated.The
/feeds_healthclient test correctly validates the health check endpoint response.varnish/vcl/default.vcl (4)
7-40: LGTM! Clean modular backend director setup.The dynamic directors for
app,feeds, andassetsare well-configured with appropriate timeouts. The comment explaining why directors are declared here (to support testing without health probes) is helpful for maintainability. Based on learnings, this follows the preferred pattern of explicit configuration over complex abstractions.
42-57: LGTM! Well-organized modular VCL includes.The modular include structure improves maintainability by separating concerns. This aligns with the PR objective of breaking the monolithic VCL into smaller fragments.
59-81: LGTM! Environment variable overrides for testing/dev.The
vcl_backend_responsesubroutine correctly handles optional environment variable overrides for TTL, grace, and keep values. The guards (if (std.getenv(...))) ensure these only apply when explicitly set, making them safe for production where they won't be configured.
106-117: Excellent addition of storage type reporting for observability.The cache-status header now includes
storage=information (malloc memory,file disk, ornonefor misses). This is critical for monitoring the effectiveness of the MP3 file cache strategy and detecting any unexpected storage behavior.just/fly.just (3)
1-9: LGTM! Deploy recipe with calculated cache sizes.The Varnish memory (70% of instance memory) and file cache (90% of disk) allocations are reasonable, leaving headroom for the OS and other processes.
11-27: LGTM! HOT/COLD region scaling logic.The scale recipe correctly differentiates between HOT and COLD regions, applying appropriate resource allocations. COLD regions receive reduced specs (
FLY_APP_REGION_DEFAULT_*), which aligns with the PR objective of splitting instances by traffic patterns.
94-98: Volume creation lacks region specification.The
flyctl volume createcommand doesn't specify a--regionflag. This will create volumes in the default region, which may not match the intended machine placement.Verify if the volume should be created in specific regions or if this is intentional for initial setup only.
container/justfile (3)
36-40: LGTM! Test command updated to include nested VTC files.The
test-vtccommand now recursively includestest/vtc/**/*.vtc, which supports the modular test organization.
52-56: LGTM! Cache stats include file storage metrics.Adding
SMF.*to the cache stats filter enables monitoring of file-backed storage, which is essential for tracking MP3 cache effectiveness.
63-84: LGTM! Bench recipes consolidated and renamed.The benchmark recipes are renamed from
fastlytoflyand reorganized with new local variants (bench-app-3-local,bench-feed-3-local), reflecting the consolidation to Fly.io.justfile (5)
17-30: LGTM! File cache enabled for local development.The
--varnish-file-cache=trueflag is consistently applied tolocal-debugandlocal-runcommands, ensuring local development mirrors production behavior with MP3 file caching.
32-43: LGTM! Docker run updated with network host mode.Using
--network hostsimplifies container networking and eliminates the need for explicit port publishing. The added--varnish-cache-size=1Gand--varnish-file-cache=trueflags enable file-based caching.
61-75: LGTM! Production acceptance tests consolidated.The renamed
test-acceptance-productionrecipe correctly excludestest/acceptance/local/*.hurltests, running only production-appropriate acceptance tests.
117-123: LGTM! HTTP profile URL updated to production endpoint.The default URL changed from
pipedream.changelog.comtochangelog.com, aligning with the consolidation to the main domain.Note: The color variables (
BOLD,WHITE,CYAN, etc.) in the curl output format are defined elsewhere in the justfile imports. Based on learnings, these rely on Just's default color handling.
133-137: LGTM! Publish recipe updated with file cache and APP_IMAGE.The publish recipe now includes
--varnish-file-cache=trueand usesAPP_IMAGEvariable, consistent with the broader configuration refactoring.
gerhard
force-pushed
the
store-mp3s-in-file-cache
branch
from
aef59d0 to
a69c5a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
dagger/main.go (1)
150-156: Double slash in file cache path.
fileCacheDirends with/, and line 155 appends/file.bin, resulting in//in the path. While Unix typically handles this, it's cleaner to avoid.Suggested fix
- fileCacheDir := "/var/lib/varnish/cache/" + fileCacheDir := "/var/lib/varnish/cache"
Regarding dual storage sizing: The memory backend (
-s memory=malloc,$VARNISH_SIZEin the procfile) and disk backend (-s disk=file,...,$VARNISH_FILE_SIZE) both use the same size value, allocating 2× the specified cache size when file cache is enabled. Per the previous discussion, please confirm this is intentional or add a clarifying comment.
🧹 Nitpick comments (1)
dagger/main.go (1)
144-144: Remove redundantfmt.Sprintffor string value.
varnishCacheSizeis already astring, so wrapping it infmt.Sprintf("%s", ...)is unnecessary.Suggested fix
- WithEnvVariable("VARNISH_SIZE", fmt.Sprintf("%s", varnishCacheSize)). + WithEnvVariable("VARNISH_SIZE", varnishCacheSize).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.godocs/local_dev.mdfly.io/cdn-2025-02-05/.envrcfly.io/cdn-2025-02-05/fly.tomlfly.io/cdn-2025-02-05/justfilefly.io/cdn-2025-02-05/machines.txtfly.io/cdn-2025-02-05/regions.txtfly.io/cdn-2025-02-05/sizes.txtfly.tomljust/_config.justjust/_config_fly.justjust/fly.justjust/hurl.justjust/op.justjustfileregions.txttest/acceptance/assets.hurltest/acceptance/fastly/admin.hurltest/acceptance/fastly/feed.hurltest/acceptance/fastly/feeds.hurltest/acceptance/fastly/homepage.hurltest/acceptance/fastly/www.hurltest/acceptance/local/http.hurltest/acceptance/local/www.hurltest/acceptance/news-mp3.hurltest/acceptance/production/admin.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/acceptance/production/health.hurltest/acceptance/production/homepage.hurltest/vtc/app-backend.vtctest/vtc/assets-backend.vtctest/vtc/assets.vtctest/vtc/cache-status.vtctest/vtc/client-ip.vtctest/vtc/default.vtctest/vtc/feeds-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/fly/request-id.vtctest/vtc/health.vtctest/vtc/http.vtctest/vtc/news-mp3.vtctest/vtc/practicalai.vtctest/vtc/purge.vtctest/vtc/websockets.vtctest/vtc/www.vtcvarnish/changelog.com.vclvarnish/varnish-json-response.bashvarnish/vcl/app-backend.vclvarnish/vcl/assets-backend.vclvarnish/vcl/backend-health.vclvarnish/vcl/default.vclvarnish/vcl/disable-caching-for-5xx.vclvarnish/vcl/disable-default-backend.vclvarnish/vcl/feeds-backend.vclvarnish/vcl/fly/client-ip.vclvarnish/vcl/fly/region.vclvarnish/vcl/fly/request-id.vclvarnish/vcl/http.vclvarnish/vcl/news-mp3.vclvarnish/vcl/practicalai.vclvarnish/vcl/purge.vclvarnish/vcl/test/disable-caching-for-testing.vclvarnish/vcl/varnish-health.vclvarnish/vcl/www.vclvector/changelog.com/debug_s3.yamlvector/changelog.com/debug_varnish.yamlvector/changelog.com/debug_varnish_geoip.yamlvector/changelog.com/default.yamlvector/changelog.com/geoip.yaml
💤 Files with no reviewable changes (11)
- regions.txt
- test/vtc/client-ip.vtc
- test/acceptance/fastly/feeds.hurl
- test/vtc/health.vtc
- test/vtc/cache-status.vtc
- test/acceptance/fastly/admin.hurl
- test/acceptance/fastly/homepage.hurl
- test/acceptance/fastly/www.hurl
- test/vtc/assets.vtc
- test/acceptance/fastly/feed.hurl
- fly.toml
🚧 Files skipped from review as they are similar to previous changes (28)
- varnish/vcl/test/disable-caching-for-testing.vcl
- test/acceptance/news-mp3.hurl
- fly.io/cdn-2025-02-05/regions.txt
- test/vtc/practicalai.vtc
- test/acceptance/assets.hurl
- test/acceptance/production/homepage.hurl
- varnish/vcl/disable-caching-for-5xx.vcl
- docs/local_dev.md
- just/hurl.just
- varnish/vcl/assets-backend.vcl
- varnish/vcl/practicalai.vcl
- test/vtc/assets-backend.vtc
- test/vtc/purge.vtc
- varnish/vcl/fly/region.vcl
- test/vtc/websockets.vtc
- fly.io/cdn-2025-02-05/justfile
- varnish/vcl/backend-health.vcl
- varnish/vcl/news-mp3.vcl
- varnish/vcl/www.vcl
- test/vtc/default.vtc
- test/acceptance/production/feed.hurl
- just/_config_fly.just
- fly.io/cdn-2025-02-05/fly.toml
- varnish/vcl/disable-default-backend.vcl
- fly.io/cdn-2025-02-05/sizes.txt
- varnish/changelog.com.vcl
- fly.io/cdn-2025-02-05/.envrc
- varnish/vcl/fly/client-ip.vcl
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-30T13:45:37.201Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: just/hurl.just:13-17
Timestamp: 2025-12-30T13:45:37.201Z
Learning: In Pipely's Justfiles, avoid defining color variables (GREEN, NORMAL, MAGENTA, BOLD, WHITE, CYAN, YELLOW, BLUE). Rely on Just's default color handling so output remains uncolored when colors aren’t defined. Do not hardcode empty strings or override defaults; let the tool manage color defaults for consistent behavior across Justfiles.
Applied to files:
just/_config.justjust/fly.justjust/op.just
📚 Learning: 2025-12-30T13:53:53.196Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/fly/client-ip.vcl:3-16
Timestamp: 2025-12-30T13:53:53.196Z
Learning: In the varnish/vcl/fly/client-ip.vcl file, the client IP determination logic only logs the selected IP via std.log and does not store it in a request header. This is intentional because the health-checker reads directly from log output rather than from request headers.
Applied to files:
varnish/varnish-json-response.bashvarnish/vcl/http.vcltest/vtc/http.vtcvarnish/vcl/app-backend.vclvarnish/vcl/purge.vcltest/vtc/fly/client-ip.vtcvarnish/vcl/varnish-health.vclvarnish/vcl/default.vcltest/vtc/fly/request-id.vtcvarnish/vcl/feeds-backend.vcl
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, explicit repetition is preferred over complex regex patterns for maintainability, even when it results in more lines of code.
Applied to files:
varnish/varnish-json-response.bashtest/vtc/http.vtctest/vtc/www.vtctest/vtc/fly/request-id.vtc
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, prefer explicit code repetition over complex regular expressions for maintainability and readability, even if it increases lines of code. This guideline applies to all .vcl files within varnish/vcl/ (and its subdirectories) to keep patterns straightforward and easier to review.
Applied to files:
varnish/vcl/http.vclvarnish/vcl/app-backend.vclvarnish/vcl/purge.vclvarnish/vcl/varnish-health.vclvarnish/vcl/default.vclvarnish/vcl/feeds-backend.vcl
📚 Learning: 2025-12-30T13:48:47.598Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/news-mp3.vcl:97-97
Timestamp: 2025-12-30T13:48:47.598Z
Learning: In varnish/vcl/news-mp3.vcl, the ASSETS_HOST environment variable should NOT have a fallback or default value. The code should fail if ASSETS_HOST is not set, following a fail-fast pattern for required configuration.
Applied to files:
test/vtc/http.vtcvarnish/vcl/purge.vcltest/vtc/feeds-backend.vtcvarnish/vcl/default.vclvarnish/vcl/feeds-backend.vcltest/vtc/news-mp3.vtc
🔇 Additional comments (40)
varnish/varnish-json-response.bash (3)
1-3: LGTM!The script header follows best practices with proper shebang and error handling via
set -eu.
9-9: LGTM!The new "keep" and "storage" fields are correctly formatted and positioned. They follow the same pattern as other VCL_Log fields and align with the VCL changes that set beresp.keep and beresp.storage for cache entries.
11-11: LGTM!The exec command is correctly structured with proper variable quoting and appropriate flags for varnishncsa JSON output.
dagger/main.go (3)
239-244: LGTM!The
strings.Join(m.VarnishOptions, " ")cleanly integrates optional storage backends into the procfile command, and handles the empty case gracefully when file cache is disabled.
267-269: LGTM!Consistent with the existing package installation pattern, and
sysctl -Vvalidates the installation.
315-339: LGTM!Vector config paths consistently updated from
pipedream.changelog.comtochangelog.com, aligning with the PR's consolidation objective.test/acceptance/local/http.hurl (1)
12-13: LGTM!The new assertions correctly verify the presence of
keep=andstorage=fields in the cache-status header, consistent with the VCL changes that introduce these metadata fields.just/_config.just (1)
22-25: Verify consistency with AI summary.The AI summary states that
_DEFAULT_TAGwas removed, but it's present at line 25. If the removal was intended, please complete it; otherwise, the summary should be corrected..github/workflows/_github.yml (2)
26-29: LGTM!The consolidation of acceptance tests into a single production-focused step (
just test-acceptance-production) simplifies the workflow and aligns with the PR objectives to streamline CI/CD.
38-44: LGTM!The deployment sequence is now correct:
publishruns from the repository root (line 41)- Directory changes to the deployment subdirectory (line 42)
deployandscalerun from the correct working directory (lines 43-44)This addresses the previous review concern about working directory context.
.github/workflows/_namespace.yml (2)
30-33: LGTM!The workflow changes mirror those in
_github.yml, consolidating acceptance tests into a single production-focused step for consistency across CI environments.
42-48: LGTM!The deployment sequence correctly executes
publishfrom the root before changing to the deployment directory fordeployandscalecommands, matching the pattern in_github.yml.just/op.just (1)
16-22: LGTM!The color token normalization (replacing
_GREEN/_RESETwithGREEN/NORMAL) aligns with the broader project-wide token harmonization effort mentioned in the AI summary.varnish/vcl/app-backend.vcl (2)
3-13: LGTM!The
vcl_recvsubroutine correctly:
- Configures the app backend using environment variables
- Propagates backend FQDN for tracking
- Handles health check endpoint rewriting
- Uses pass for health checks to avoid caching
The implementation is clean and straightforward.
18-34: LGTM!The caching strategy for app backend is well-configured:
- In-memory storage prevents disk I/O overhead for frequently accessed content
- Short TTL (1m) ensures freshness
- Grace period (1d) enables stale-while-revalidate pattern
- Keep period (7d) allows conditional requests to avoid re-downloads when content is unchanged
This aligns with the PR objective to optimize cache efficiency and prevent re-downloading.
README.md (2)
19-21: LGTM!The roadmap update correctly marks PR #44 as complete and expands the scope to include both MP3 and feed checks, which provides better clarity on the planned improvements.
94-112: LGTM!The updated recipe list reflects the production-focused workflow changes and Docker-centric approach introduced in this PR, making the available commands clearer for users.
fly.io/cdn-2025-02-05/machines.txt (1)
1-2: Verify the app name consistency.The file is located at
fly.io/cdn-2025-02-05/but references app namecdn-2025-02-25(note the date discrepancy:02-05vs02-25). Confirm whether this is intentional or a typo.just/fly.just (2)
31-37: Secrets handling looks appropriate.The secrets are managed through 1Password and passed via environment variables to the bash context. While command-line arguments can be visible in process listings, the
just op runwrapper and the bash -c encapsulation provide reasonable protection for this operational workflow.
66-77: Restart logic with retry handling looks good.The recipe properly stops machines one-by-one, waits for shutdown, and includes retry logic for the start operation. The color variable usage aligns with the coding guidelines to rely on centrally defined color variables.
container/justfile (2)
36-36: Good improvement to use one-shot varnishstat output.The
-1flag provides a single snapshot of statistics, which is more suitable for scripting and monitoring compared to the continuous output.
40-40: Parallel test execution improves test performance.Adding
-j 8enables parallel execution of VTC tests, which should significantly reduce test suite runtime. The expanded test paths ensure comprehensive coverage across the new modular VCL structure.justfile (2)
22-22: Varnish file cache integration aligns with PR objectives.The
--varnish-file-cache=trueflag is consistently added across local, debug, test, and production workflows, enabling disk-backed storage for MP3 files to prevent memory fragmentation. This directly supports the PR's goal of storing MP3s on disk while keeping other assets in memory.Also applies to: 29-29, 37-37, 57-57, 94-94, 110-110, 136-136
41-41: Network host mode simplifies local development.Using
--network hostallows the container to bind directly to the host's network interface, simplifying access to the service on localhost:9000 without complex port mapping.test/acceptance/local/www.hurl (1)
4-4: Header name standardization to lowercase.The change from "Location" to "location" aligns with HTTP/2 conventions and ensures consistency across the codebase. This matches similar updates in VCL files like http.vcl and www.vcl.
varnish/vcl/varnish-health.vcl (1)
1-5: Clean health check implementation.The health endpoint returns a 204 No Content response, which is appropriate for readiness probes. The implementation is minimal and efficient, avoiding unnecessary backend calls for health checks.
varnish/vcl/http.vcl (1)
12-12: Header standardization to lowercase.Changing the Location header to lowercase aligns with HTTP/2 conventions and maintains consistency with similar updates throughout the codebase (www.vcl, test files).
test/vtc/fly/request-id.vtc (1)
18-18: Include path change supports modular VCL structure.The switch from an absolute path to a relative include path aligns with the PR's move toward reusable VCL across different top-level configs. This improves maintainability and portability of the VCL modules.
varnish/vcl/purge.vcl (2)
1-14: Well-structured PURGE authorization implementation.The implementation correctly uses environment variables for token-based authentication and provides clear error messages for unauthorized requests. The modular design allows easy inclusion in different VCL configurations.
8-9: I'm unable to verify this review comment due to persistent repository access issues. To complete the verification, I need:
- The full contents of varnish/vcl/purge.vcl
- Your production deployment configuration (fly.toml or equivalent environment setup)
- Documentation on how PURGE_TOKEN is configured in production
While the logical concern in the VCL code is valid—the condition does permit unauthenticated PURGEs when
PURGE_TOKENis empty—I cannot confirm whether this is intentionally configured in your production environment without accessing your deployment files.test/vtc/http.vtc (1)
11-11: LGTM! Test updated to align with modular VCL refactoring.The changes correctly reflect:
- Relative include path for modular VCL
- Updated host expectations to pipely.local
- Lowercase location header normalization
Also applies to: 30-30, 33-33
test/vtc/app-backend.vtc (1)
1-41: LGTM! Comprehensive test coverage for app backend.The test harness correctly:
- Sets up a mock backend with health endpoint
- Configures dynamic director without health probes for testing
- Verifies /app_health routing and response
- Uses both memory and disk storage configurations
test/vtc/www.vtc (1)
6-7: LGTM! Test updated for modular VCL and header normalization.The changes correctly reflect:
- Modular includes (disable-default-backend.vcl and www.vcl)
- Updated host to www.pipely.local
- Lowercase location header expectation
Also applies to: 12-12, 15-15
test/vtc/fly/client-ip.vtc (1)
1-86: LGTM! Excellent test coverage for client IP determination.The test suite comprehensively covers all scenarios:
- fly-client-ip header priority
- Precedence over x-forwarded-for
- Regex extraction of first IP from multiple IPs
- Fallback to default client.ip
The use of logexpect to verify VCL_Log output aligns with the intentional design where client IP is logged rather than stored in request headers.
test/vtc/news-mp3.vtc (1)
11-16: LGTM! Test correctly refactored for modular VCL architecture.The changes properly:
- Configure environment variables for dynamic backends
- Set up memory and disk storage for MP3 caching tests
- Use modular includes (app-backend.vcl, news-mp3.vcl)
- Test redirect behavior with updated expectations for assets.local
- Verify x-redirect header is properly ignored
The fail-fast pattern for ASSETS_HOST (no fallback) is correctly maintained.
Also applies to: 19-35, 46-68
varnish/vcl/feeds-backend.vcl (1)
1-131: LGTM! Well-structured feed routing with appropriate caching strategy.The implementation correctly:
- Routes feed health checks to /health endpoint
- Uses explicit per-feed URL rewrites (intentional per coding guidelines for maintainability)
- Sets memory-only storage to prevent disk fragmentation from large XML files
- Configures appropriate cache lifetimes (TTL: 12h, grace: 1d, keep: 7d)
- Propagates backend headers for routing and monitoring
The explicit repetition pattern (lines 19-109) aligns with the maintainability guidelines for this codebase. Based on learnings, this approach is preferred over complex regex patterns.
test/vtc/feeds-backend.vtc (1)
1-730: LGTM! Exceptionally thorough test coverage for feeds backend.This test suite provides comprehensive coverage:
- Health endpoint routing verification
- All 16 feed routes (podcast, gotime, master, feed, jsparty, shipit, news, brainscience, founderstalk, interviews, friends, rfc, spotlight, afk, posts, plusplus)
- Each route tested with 3 variants: base path, trailing slash, query string preservation
- Special cases: /rss → /feed.xml and /feeds/* catch-all
- Dynamic backend configuration with proper environment variables
The methodical testing approach ensures all URL rewrites and backend routing work correctly.
varnish/vcl/default.vcl (3)
5-40: LGTM! Clean modular architecture with well-documented dynamic backends.The refactoring successfully:
- Separates concerns through modular includes (app-backend, feeds-backend, assets-backend, etc.)
- Configures dynamic directors with appropriate timeouts and health probes
- Documents backend initialization clearly
- Supports future extensibility through the include pattern
The increased
first_byte_timeout(300s) for the app backend is appropriate for MP3 upload operations.Also applies to: 42-57
59-81: LGTM! Environment variable overrides properly scoped for dev/testing.The implementation correctly:
- Allows BERESP_TTL, BERESP_GRACE, and BERESP_KEEP overrides via environment variables
- Clearly documents that overrides are only for dev/testing
- Positions the override logic after all includes to ensure it runs last
- Uses std.duration() for proper type conversion
88-127: LGTM! Enhanced observability with storage and keep metadata.The vcl_deliver implementation now provides comprehensive cache diagnostics:
- Origin backend identification
- Bypass status tracking
- TTL and grace values
- New: keep duration for conditional request optimization
- New: storage type (memory/disk/none) for cache hit attribution
- Hit/miss/stale status and hit count
The added
storageandkeepfields (lines 107-108, 112-116) align with the PR objective to track MP3 disk caching and object retention for conditional requests.
gerhard
force-pushed
the
store-mp3s-in-file-cache
branch
from
a69c5a1 to
b68b214
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
just/fly.just (2)
14-14: Consider a more explicit region count calculation.The comma-counting approach relies on adding a trailing comma (
{{ FLY_APP_REGIONS }},) to get the correct count. While it works, it's somewhat fragile and non-obvious to future maintainers.🔎 Alternative approach using word count
- flyctl scale count $(echo {{ FLY_APP_REGIONS }}, | grep -o ',' | wc -l) --max-per-region 1 --region {{ FLY_APP_REGIONS }} --app {{ FLY_APP }} + flyctl scale count $(echo {{ FLY_APP_REGIONS }} | tr ',' '\n' | wc -l) --max-per-region 1 --region {{ FLY_APP_REGIONS }} --app {{ FLY_APP }}This converts commas to newlines and counts lines, making the intent clearer.
64-75: Consider more robust retry logic.The restart logic retries machine start only once after a 10-second delay. For production deployments, consider adding more retries with exponential backoff or logging failures for manual intervention.
🔎 Enhanced retry example
@just machines \ | awk '/pipely/ { print $1 }' \ | while read machine; do \ echo -en "\n♻️ "; \ flyctl machine stop $machine; \ sleep 10; \ - flyctl machine start $machine \ - || (sleep 10; flyctl machine start $machine); \ + for i in 1 2 3; do \ + flyctl machine start $machine && break; \ + echo "Retry $i failed, waiting..."; \ + sleep $((10 * i)); \ + done; \ done
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.godocs/local_dev.mdfly.io/cdn-2025-02-05/.envrcfly.io/cdn-2025-02-05/fly.tomlfly.io/cdn-2025-02-05/justfilefly.io/cdn-2025-02-05/machines.txtfly.io/cdn-2025-02-05/regions.txtfly.io/cdn-2025-02-05/sizes.txtfly.tomljust/_config.justjust/_config_fly.justjust/fly.justjust/hurl.justjust/op.justjustfileregions.txttest/acceptance/assets.hurltest/acceptance/fastly/admin.hurltest/acceptance/fastly/feed.hurltest/acceptance/fastly/feeds.hurltest/acceptance/fastly/homepage.hurltest/acceptance/fastly/www.hurltest/acceptance/local/http.hurltest/acceptance/local/www.hurltest/acceptance/news-mp3.hurltest/acceptance/production/admin.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/acceptance/production/health.hurltest/acceptance/production/homepage.hurltest/vtc/app-backend.vtctest/vtc/assets-backend.vtctest/vtc/assets.vtctest/vtc/cache-status.vtctest/vtc/client-ip.vtctest/vtc/default.vtctest/vtc/feeds-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/fly/request-id.vtctest/vtc/health.vtctest/vtc/http.vtctest/vtc/news-mp3.vtctest/vtc/practicalai.vtctest/vtc/purge.vtctest/vtc/websockets.vtctest/vtc/www.vtcvarnish/changelog.com.vclvarnish/varnish-json-response.bashvarnish/vcl/app-backend.vclvarnish/vcl/assets-backend.vclvarnish/vcl/backend-health.vclvarnish/vcl/default.vclvarnish/vcl/disable-caching-for-5xx.vclvarnish/vcl/disable-default-backend.vclvarnish/vcl/feeds-backend.vclvarnish/vcl/fly/client-ip.vclvarnish/vcl/fly/region.vclvarnish/vcl/fly/request-id.vclvarnish/vcl/http.vclvarnish/vcl/news-mp3.vclvarnish/vcl/practicalai.vclvarnish/vcl/purge.vclvarnish/vcl/test/disable-caching-for-testing.vclvarnish/vcl/varnish-health.vclvarnish/vcl/www.vclvector/changelog.com/debug_s3.yamlvector/changelog.com/debug_varnish.yamlvector/changelog.com/debug_varnish_geoip.yamlvector/changelog.com/default.yamlvector/changelog.com/geoip.yaml
💤 Files with no reviewable changes (11)
- test/vtc/cache-status.vtc
- test/acceptance/fastly/admin.hurl
- test/acceptance/fastly/feeds.hurl
- regions.txt
- test/vtc/health.vtc
- test/acceptance/fastly/feed.hurl
- test/acceptance/fastly/www.hurl
- test/vtc/assets.vtc
- test/acceptance/fastly/homepage.hurl
- test/vtc/client-ip.vtc
- fly.toml
🚧 Files skipped from review as they are similar to previous changes (28)
- varnish/vcl/test/disable-caching-for-testing.vcl
- varnish/vcl/disable-caching-for-5xx.vcl
- varnish/vcl/www.vcl
- test/vtc/fly/request-id.vtc
- varnish/vcl/backend-health.vcl
- test/acceptance/production/homepage.hurl
- varnish/vcl/assets-backend.vcl
- varnish/vcl/app-backend.vcl
- test/acceptance/news-mp3.hurl
- varnish/vcl/fly/region.vcl
- fly.io/cdn-2025-02-05/sizes.txt
- test/acceptance/production/feed.hurl
- test/vtc/websockets.vtc
- varnish/vcl/news-mp3.vcl
- varnish/vcl/purge.vcl
- varnish/varnish-json-response.bash
- varnish/vcl/varnish-health.vcl
- varnish/vcl/http.vcl
- just/_config.just
- varnish/changelog.com.vcl
- fly.io/cdn-2025-02-05/machines.txt
- fly.io/cdn-2025-02-05/justfile
- varnish/vcl/feeds-backend.vcl
- test/acceptance/local/www.hurl
- container/justfile
- test/vtc/http.vtc
- fly.io/cdn-2025-02-05/.envrc
- test/vtc/www.vtc
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-30T13:53:53.196Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/fly/client-ip.vcl:3-16
Timestamp: 2025-12-30T13:53:53.196Z
Learning: In the varnish/vcl/fly/client-ip.vcl file, the client IP determination logic only logs the selected IP via std.log and does not store it in a request header. This is intentional because the health-checker reads directly from log output rather than from request headers.
Applied to files:
varnish/vcl/fly/client-ip.vcltest/vtc/default.vtctest/vtc/practicalai.vtctest/vtc/fly/client-ip.vtcvarnish/vcl/default.vcl
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, prefer explicit code repetition over complex regular expressions for maintainability and readability, even if it increases lines of code. This guideline applies to all .vcl files within varnish/vcl/ (and its subdirectories) to keep patterns straightforward and easier to review.
Applied to files:
varnish/vcl/fly/client-ip.vclvarnish/vcl/disable-default-backend.vclvarnish/vcl/default.vclvarnish/vcl/practicalai.vcl
📚 Learning: 2025-12-30T13:48:47.598Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/news-mp3.vcl:97-97
Timestamp: 2025-12-30T13:48:47.598Z
Learning: In varnish/vcl/news-mp3.vcl, the ASSETS_HOST environment variable should NOT have a fallback or default value. The code should fail if ASSETS_HOST is not set, following a fail-fast pattern for required configuration.
Applied to files:
test/vtc/default.vtctest/vtc/news-mp3.vtctest/vtc/practicalai.vtcvarnish/vcl/default.vcltest/vtc/assets-backend.vtctest/vtc/feeds-backend.vtc
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, explicit repetition is preferred over complex regex patterns for maintainability, even when it results in more lines of code.
Applied to files:
test/vtc/default.vtctest/vtc/practicalai.vtc
📚 Learning: 2025-12-30T13:45:37.201Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: just/hurl.just:13-17
Timestamp: 2025-12-30T13:45:37.201Z
Learning: In Pipely's Justfiles, avoid defining color variables (GREEN, NORMAL, MAGENTA, BOLD, WHITE, CYAN, YELLOW, BLUE). Rely on Just's default color handling so output remains uncolored when colors aren’t defined. Do not hardcode empty strings or override defaults; let the tool manage color defaults for consistent behavior across Justfiles.
Applied to files:
just/fly.justjust/_config_fly.justjust/hurl.justjust/op.just
📚 Learning: 2025-12-30T13:54:21.688Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: fly.io/cdn-2025-02-05/regions.txt:33-33
Timestamp: 2025-12-30T13:54:21.688Z
Learning: Preserve the exact verbatim content of CLI tool outputs (e.g., regions.txt) as produced by the tool. Do not normalize spelling, spacing, or formatting. Treat these files as canonical records of command output; document the source command (and timestamp, if relevant) in a brief header or comment if possible. This guideline applies to any file named regions.txt across the repository.
Applied to files:
fly.io/cdn-2025-02-05/regions.txt
🪛 LanguageTool
fly.io/cdn-2025-02-05/regions.txt
[uncategorized] ~32-~32: Did you mean “São Paulo” (= city in Brazil)?
Context: ... South America Sao Paulo, Brazil gru 11...
(SAO_PAOLO)
🔇 Additional comments (51)
fly.io/cdn-2025-02-05/regions.txt (1)
1-34: Approved: file correctly preservesflyctltool output.This file captures a snapshot of Fly.io regions output from the
flyctlcommand. Preserving it verbatim—including formatting, spacing, and any spelling variations in city names—ensures it remains a canonical record of the tool's output. The data structure is sound, with all regions properly formatted and capacity values correctly recorded.Based on learnings, this file should be kept exactly as returned by the tool for accuracy and consistency with the CLI output.
varnish/vcl/disable-default-backend.vcl (1)
1-2: LGTM!The snippet correctly disables the default backend for a dynamic-backend-only architecture. The comment clearly documents the intent.
varnish/vcl/fly/client-ip.vcl (1)
3-16: LGTM!The client IP selection logic correctly implements the documented precedence (fly-client-ip → x-forwarded-for → client.ip) and logs the result for the health-checker. The log-only approach (without storing in a request header) is intentional and correct for your architecture.
Based on learnings, the health-checker consumes
std.logoutput directly rather than reading from request headers.test/vtc/purge.vtc (1)
3-36: LGTM!The refactoring from inline PURGE logic to modular includes (purge.vcl, disable-default-backend.vcl) aligns with the PR's goal of modular VCL composition. The test token values are correctly updated to match the new environment-driven flow, and all test expectations remain sound.
test/vtc/fly/client-ip.vtc (1)
1-86: LGTM!Comprehensive test coverage for all client IP determination scenarios. The six test cases correctly validate header precedence, regex extraction, and fallback behavior using
logexpectto verify VCL_Log output. Test structure is clear and well-documented.test/acceptance/local/http.hurl (1)
12-13: LGTM!The new cache-status assertions for
keep=andstorage=align with the PR's goal of surfacing cache metadata. These assertions are consistent with similar additions in production acceptance tests.test/acceptance/assets.hurl (1)
56-84: LGTM!The
storage.diskassertions for MP3 chunk requests correctly validate that MP3 files are cached on disk (as per the PR objective to prevent memory fragmentation from large files). The assertions are appropriately placed only in MP3 tests, not in PNG/CSS tests.test/vtc/practicalai.vtc (1)
6-7: LGTM!The refactoring from inline redirect logic to modular includes (disable-default-backend.vcl, practicalai.vcl) aligns with the PR's modular VCL composition strategy. Test expectations are preserved.
test/vtc/app-backend.vtc (1)
1-41: LGTM! Well-structured test for the app backend.The test correctly sets up:
- Mock backend responding to
/health- Environment variables without defaults (fail-fast pattern per learnings)
- Memory + disk cache storage configuration
- Dynamic director without probe for testing isolation
The
/app_health→/healthURL translation is handled by the includedapp-backend.vcl, making the test appropriately focused on validating backend routing.test/vtc/default.vtc (1)
1-250: LGTM! Comprehensive test coverage for the default VCL.This test file provides excellent coverage:
- Health endpoints for all backends (varnish, app, assets, feeds)
- Cache miss vs. hit scenarios with proper storage verification
- Bypass behavior for health checks
- Cache-status header validation including
ttl,grace,keep,storage, andhitsThe multiple
sub vcl_initblocks (lines 75-79, 82-86, 89-93) correctly leverage VCL's subroutine merging behavior to define separate dynamic directors for each backend.varnish/vcl/practicalai.vcl (1)
1-26: LGTM! Clear redirect handling with explicit URL matching.The implementation correctly:
- Uses exact URL matches in
vcl_recv(following the explicit repetition pattern per learnings)- Sets lowercase
locationheaders (consistent with PR normalization)- Provides HTML fallback bodies for clients that don't auto-redirect
- Handles the more specific
/practicalai/feedbefore the general/practicalaipath invcl_synthtest/vtc/assets-backend.vtc (2)
140-155: Excellent validation of MP3 disk storage - key PR objective.This test directly validates the core PR feature: MP3 files are stored in the file/disk cache while other assets (PNG, CSS) use memory storage. The
logexpectassertions on lines 111, 128, and 145 explicitly verify:
- PNG →
malloc memory- CSS →
malloc memory- MP3 →
file diskThis ensures memory fragmentation from large files is avoided as intended.
1-74: LGTM! Comprehensive assets backend test setup.The test correctly validates:
- Health endpoint routing (
/assets_health→/health)- Environment variables without defaults (fail-fast pattern)
- Memory + disk cache configuration
- HEAD-to-GET conversion for caching (line 12-13 comment explains this)
test/vtc/feeds-backend.vtc (1)
276-306: LGTM! Well-structured feeds backend test configuration.The changes properly set up:
- Environment variables for the feeds backend
- Dynamic director with probe-free configuration for testing
- Appropriate includes:
feeds-backend.vcl,test/disable-caching-for-testing.vcl,disable-default-backend.vclThe extensive feed URL testing (54 client tests) follows the explicit repetition pattern per learnings, ensuring each feed path is individually validated rather than using complex regex patterns.
varnish/vcl/default.vcl (3)
11-40: LGTM! Well-configured dynamic directors with appropriate timeouts.The dynamic director setup properly differentiates backends:
app: 300sfirst_byte_timeoutfor MP3 uploadsfeedsandassets: 10sfirst_byte_timeoutfor faster responsesAll directors share consistent
connect_timeout(10s) andbetween_bytes_timeout(60s), with health probes enabled for production resilience.
42-57: LGTM! Clean modular VCL organization.The include statements logically group functionality:
- Backend routing:
app-backend.vcl,assets-backend.vcl,feeds-backend.vcl- Infrastructure:
backend-health.vcl,varnish-health.vcl,fly/client-ip.vcl,fly/request-id.vcl,fly/region.vcl- Features:
purge.vcl,websockets.vcl,http.vcl,news-mp3.vcl,practicalai.vcl,www.vcl- Safety:
disable-caching-for-5xx.vcl,disable-default-backend.vclThis modular approach supports the PR's goal of reusable VCL across top-level configs.
106-117: Excellent addition: storage type visibility for cache diagnostics.The new
storage=field incache-statusheader enables:
- Verification that MP3s are stored in disk cache vs. memory
- Operational monitoring of cache behavior post-deployment
- Debugging cache placement issues
This directly supports the PR objective of driving further improvements via deployment metrics.
fly.io/cdn-2025-02-05/fly.toml (5)
6-11: LGTM!The process lifecycle and deployment strategy are well configured. SIGTERM enables graceful Varnish shutdown, the 30-second timeout is reasonable for connection draining, and the rolling deployment with
max_unavailable = 1ensures zero-downtime updates.
13-14: LGTM!The 60-second idle timeout is appropriate for CDN workloads and aligns with typical Varnish keep-alive connection handling.
28-34: LGTM!The port configuration correctly sets up standard HTTP (80) and HTTPS (443) with appropriate handlers.
20-26: The/healthendpoint is properly configured in the Varnish VCL. It is implemented invarnish/vcl/varnish-health.vcland returns a 204 No Content response, which is appropriate for health checks. The health check configuration in fly.toml correctly references this endpoint.
1-44: Verify HOT/COLD instance configuration.The PR objectives mention splitting Varnish instances into HOT (high-traffic) and COLD, but the deployed machines show only one explicitly configured COLD instance ("cold-frog-9880") and no explicitly named HOT instances among 15 total machines. The single shared fly.toml file contains no HOT/COLD differentiation. Confirm whether:
- HOT instances are deployed under different naming conventions
- The HOT/COLD split is yet to be implemented as part of this PR
- Instance differentiation relies on size (performance-1x vs performance-2x) rather than explicit HOT/COLD configuration
docs/local_dev.md (1)
35-35: LGTM!Documentation correctly updated to reflect the renamed benchmark recipe (
bench-app-2-fly), aligning with the workflow consolidation from Pipedream to Fly..github/workflows/_github.yml (2)
26-29: LGTM!The consolidation of CDN acceptance steps into a single production acceptance step simplifies the workflow and aligns with the broader PR objective of consolidating to changelog.com.
38-44: LGTM!The deploy sequence correctly:
- Runs
publishfrom repo root (where it's defined)- Changes directory to
fly.io/cdn-2025-02-05fordeployandscale- Calls
scalewithout parameters (matching the target signature)This addresses the concerns from previous reviews.
.github/workflows/_namespace.yml (2)
30-33: LGTM!Consistent with the GitHub workflow consolidation. The production acceptance step aligns with the broader CDN consolidation effort.
42-48: LGTM!The deploy sequence mirrors
_github.yml, maintaining consistency between the GitHub and Namespace.so workflows.just/op.just (1)
12-23: LGTM!Color token naming standardized to
GREEN,NORMAL,MAGENTAfor consistency across Justfiles. Based on learnings, these variables are intentionally left undefined and treated as defaults by Just.just/hurl.just (1)
10-18: LGTM!Color token naming standardized consistently with
just/op.just. Based on learnings, these undefined color variables are intentional, allowing Just to handle them gracefully.README.md (3)
19-21: LGTM!Roadmap updates accurately reflect the PR's contributions: MP3 file cache with HOT/COLD instances and updated periodic checks.
83-83: LGTM!Just version requirement updated from 1.27.0 to 1.35.0. As acknowledged in previous review, the update to 1.45.0 will follow in the next PR.
94-116: LGTM!The available recipes list is updated to reflect the current functionality, including the production-focused commands and updated descriptions.
dagger/main.go (8)
8-9: LGTM!New imports for
regexpandstringsare used appropriately for cache size validation and procfile generation.
49-50: LGTM!The
VarnishOptionsfield provides a clean way to accumulate additional Varnish command-line options dynamically.
136-139: LGTM!Cache size validation enforces explicit units (K/M/G) for clarity, as confirmed in previous review discussions.
149-157: Clarify dual storage sizing intent.When
varnishFileCacheis enabled, line 152 setsVARNISH_FILE_SIZEto the same value asVARNISH_SIZE(line 144). Combined with the procfile at line 239 which uses both-s memory=malloc,$VARNISH_SIZEand the appended disk option, this allocates 2× the configured cache size (e.g., 100M memory + 100M disk = 200M total).If this is intentional, consider adding a comment to clarify. Otherwise, you may want to split the size between backends or introduce separate size parameters.
159-172: LGTM!The conditional environment variable setup for
BERESP_TTL,BERESP_GRACE, andBERESP_KEEPis clean and only sets variables when values are provided.
239-244: LGTM!The procfile generation correctly uses
strings.Jointo append any accumulatedVarnishOptionsto the varnishd command line.
267-269: LGTM!Installing
procpsprovides thesysctlcommand needed for system configuration checks.
315-340: LGTM!Vector config paths correctly updated from
vector/pipedream.changelog.com/tovector/changelog.com/, aligning with the consolidation objective.just/_config_fly.just (1)
1-38: LGTM!The environment-driven configuration for Fly.io deployment supports the HOT/COLD instance architecture. The private variables are well-organized with sensible grouping (default regions, HOT regions).
Consider documenting required environment variables. Most variables lack defaults and depend on environment values. If these aren't set,
env()calls will return empty strings. Ensure the required variables are documented (e.g., in the README or a.env.examplefile) so deployments don't fail silently due to missing configuration.just/fly.just (4)
3-9: LGTM!The deployment configuration correctly calculates Varnish memory and file cache sizes as percentages of the allocated resources, aligning with the PR's goal to store MP3s on disk while keeping other assets in memory.
36-36: Verify the hardcodedHONEYCOMB_DATASETvalue.The PR objectives mention removing
pipedream.changelog.comand consolidating tochangelog.com. However, the Honeycomb dataset is still hardcoded to"pipedream". Confirm whether this dataset name should remain as-is or be updated to align with the consolidation.
39-62: LGTM!These recipes are clean wrappers around Fly.io CLI commands with appropriate variable substitution.
79-92: LGTM!These diagnostic recipes provide useful introspection into app and Varnish status without any issues.
justfile (6)
18-30: LGTM!The addition of
--varnish-file-cache=trueis consistent with the PR's goal to enable disk-backed file caching for MP3s.
41-41: Verify--network hostis intentional.The Docker run command now uses
--network host, which removes network isolation and allows the container to access the host's network stack directly. This change may have security implications and differs from typical container isolation patterns. Confirm this is necessary for the use case (e.g., accessing local services or simplified networking for remote server deployments).
61-75: LGTM!The renaming from
test-acceptance-pipedreamtotest-acceptance-productionand the host consolidation tochangelog.comalign perfectly with the PR's objective to removepipedream.changelog.com.
87-115: LGTM!The addition of
--varnish-file-cache=trueto production debugging and local run commands maintains consistency across all deployment scenarios.
118-123: LGTM!The URL change from
pipedream.changelog.comtochangelog.comis consistent with the PR's consolidation objectives.
135-137: LGTM!The publish recipe correctly integrates
--varnish-file-cache=trueand usesAPP_IMAGE, maintaining consistency with the new deployment architecture.
gerhard
force-pushed
the
store-mp3s-in-file-cache
branch
3 times, most recently
from
77b82db to
6cf7ecb
Compare
This is a big change that includes: - pipedream.changelog.com removal (it's just changelog.com now) - break down the monolithic default.vcl into smaller fragments (this allowed the tests to be refactored, no more VCL duplication!) - keep objects in cache to prevent re-downloading if-not-modified - Varnish tuning to match changelog.com `varnishstat -1` - split instances into HOT (receive the most traffic) & COLD All this so that we can configure disk cache for storing those MP3 files. Without a disk cache, the memory becomes fragmented due to all the large files, and other large files can no longer be stored. After a long enough period, small files can no longer be stored either, which means that caching stops working altogether. We are now storing MP3 files on disk, and everything else in memory. Once we get this deployed and check the stats, the next improvement will become obvious. Another important side-effect of this change is a concrete step towards allowing VCL re-use across different top-level configs which will be necessary for v2.0 - making it easy for others to deploy this. Signed-off-by: Gerhard Lazu <[email protected]>
gerhard
force-pushed
the
store-mp3s-in-file-cache
branch
from
6cf7ecb to
2dbe6a6
Compare
This is a big change that includes:
allowed the tests to be refactored, no more VCL duplication!)
varnishstat -1All this so that we can configure disk cache for storing those MP3 files. Without a disk cache, the memory becomes fragmented due to all the large files, and other large files can no longer be stored. After a long enough period, small files can no longer be stored either, which means that caching stops working altogether.
We are now storing MP3 files on disk, and everything else in memory. Once we get this deployed and check the stats, the next improvement will become obvious.
Another important side-effect of this change is a concrete step towards allowing VCL re-use across different top-level configs which will be necessary for v2.0 - making it easy for others to deploy this.
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.