Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughA Fly.io deployment configuration file ( Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fly.toml (1)
14-16: Consider keeping one warm machine for production latency.With
min_machines_running = 0, first-request cold starts are expected after idle periods. If this serves interactive traffic, considermin_machines_running = 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fly.toml` around lines 14 - 16, The current Fly configuration sets auto_stop_machines = 'stop', auto_start_machines = true and min_machines_running = 0 which allows full scale-to-zero and causes cold starts; change min_machines_running from 0 to 1 to keep one warm instance for production/interative latency-sensitive traffic so the app avoids first-request cold starts while leaving auto-stop/start behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fly.toml`:
- Around line 20-23: The fly.toml currently defines both memory and memory_mb in
the VM config, which is unsupported; remove the memory_mb = 1024 entry so only
memory = '1gb' remains, leaving cpu_kind and cpus unchanged; update any comments
or related documentation referencing memory_mb and ensure no other code or
scripts rely on memory_mb instead of memory.
---
Nitpick comments:
In `@fly.toml`:
- Around line 14-16: The current Fly configuration sets auto_stop_machines =
'stop', auto_start_machines = true and min_machines_running = 0 which allows
full scale-to-zero and causes cold starts; change min_machines_running from 0 to
1 to keep one warm instance for production/interative latency-sensitive traffic
so the app avoids first-request cold starts while leaving auto-stop/start
behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| memory = '1gb' | ||
| cpu_kind = 'shared' | ||
| cpus = 1 | ||
| memory_mb = 1024 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Fly.io fly.toml [[vm]] configuration, can memoryandmemory_mb be set together? If both are present, which one takes precedence, and is this officially supported?
💡 Result:
Yes—flyctl will accept both memory and memory_mb in a [[vm]] stanza, because [[vm]] is decoded into a Compute struct that has both a memory field and an inlined MachineGuest (which includes memory_mb). The config reference, however, documents only memory for the [[vm]] section. [1]
If both are present, which wins?
In flyctl’s computeToGuest() implementation, it:
- Parses
memory(string with units) and setsguest.MemoryMB, then - Copies fields from
compute.MachineGuest(wherememory_mblives) ontoguestafterwards (with “ignore empty” semantics).
So, memory_mb takes precedence over memory when both are set (assuming memory_mb is non-zero). [2]
Is this “officially supported”?
- Official
fly.tomlreference: documentsmemory, notmemory_mb. So “set both and rely on precedence” is not something the reference specifies/supports. [1] - Fly’s own docs/examples: some official guides use
memory_mbinfly.tomlexamples (somemory_mbis clearly supported by tooling/docs in practice). [3] - Machines API field: the underlying Machine “guest” schema uses
memory_mbas the canonical field. [4]
Practical takeaway: don’t set both; pick one. If you do set both, memory_mb wins in current flyctl. [2]
Sources:
[1] Fly.io config reference ([[vm]] / memory) (fly.io)
[2] flyctl source (computeToGuest: parses memory then applies memory_mb) (github.com)
[3] Fly.io “Use a MySQL Database” guide using memory_mb in fly.toml (fly.io)
[4] Fly Machines API schema (memory_mb) (fly.io)
Citations:
- 1: https://fly.io/docs/reference/configuration/
- 2: https://github.com/superfly/flyctl/raw/refs/heads/master/internal/appconfig/machines.go
- 3: https://fly.io/docs/app-guides/mysql-on-fly/
- 4: https://fly.io/docs/machines/api/machines-resource/?utm_source=openai
Remove memory_mb to conform to the official fly.toml specification.
The [[vm]] section officially documents only memory, not memory_mb. Having both fields present is unsupported: when set together, memory_mb silently takes precedence over memory, creating potential for unexpected configuration drift. Keep only memory (currently set to 1gb).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fly.toml` around lines 20 - 23, The fly.toml currently defines both memory
and memory_mb in the VM config, which is unsupported; remove the memory_mb =
1024 entry so only memory = '1gb' remains, leaving cpu_kind and cpus unchanged;
update any comments or related documentation referencing memory_mb and ensure no
other code or scripts rely on memory_mb instead of memory.
1 participant
Summary by CodeRabbit