Compare volumes and mounts in specsMatch to prevent stale pool reuse#718
Conversation
specsMatch was only comparing container-level fields (image, command, env, ports) when deciding whether to reuse an existing sandbox pool. Volume and mount entries were silently ignored, so any disk config change — adding, removing, or modifying a disk — would reuse the old pool with stale volume configuration. New sandboxes would launch without the expected mounts. Added volumesEqual, mountsEqual, and labelsEqual helpers alongside the existing portsEqual pattern. Also made specsMatch a package-level function since it doesn't use any Launcher state. The test file includes a recursive struct fingerprint tripwire that hashes the field names of SandboxSpec and all its nested structs — if anyone adds a field at any depth, the hash changes and the test fails, nudging them to update specsMatch. Fixes MIR-944
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR refactors sandbox spec comparison by changing Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/deployment/launcher.go (1)
777-818:⚠️ Potential issue | 🟠 MajorInclude
ShutdownTimeoutin the pool-reuse comparison.Line 719 copies the service shutdown timeout into the container spec, but this matcher never compares it. A deploy that only changes that grace period will still reuse the old pool and keep the stale timeout.
Suggested fix
if c1.Directory != c2.Directory { return fmt.Sprintf("container[%d] directory mismatch: %s vs %s", i, c1.Directory, c2.Directory), false } + if c1.ShutdownTimeout != c2.ShutdownTimeout { + return fmt.Sprintf("container[%d] shutdown timeout mismatch: %s vs %s", i, c1.ShutdownTimeout, c2.ShutdownTimeout), false + } // Compare env vars (order-independent) if !envVarsEqual(c1.Env, c2.Env) { return fmt.Sprintf("container[%d] environment variables mismatch", i), false }Please add a
TestSpecsMatchcase for this too, since the new fingerprint test will not catch already-existing fields that are skipped here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/deployment/launcher.go` around lines 777 - 818, The spec comparison currently omits ShutdownTimeout so changes to that field are ignored; update the matcher (the function that compares spec1 and spec2—used by TestSpecsMatch) to compare spec1.ShutdownTimeout and spec2.ShutdownTimeout and return a clear mismatch message like "shutdown timeout mismatch" when they differ, and then add a new case to the TestSpecsMatch unit test that alters only the ShutdownTimeout to ensure the comparison fails when expected.
🧹 Nitpick comments (2)
controllers/deployment/launcher.go (1)
820-878: Make these helper comments carry the non-obvious contract, or drop them.Right now the comments just restate the function names. The useful detail here is that all three comparisons are positional, so reorder-only diffs are treated as mismatches.
As per coding guidelines, "Only add comments when they provide valuable context or explain 'why' something is done, avoiding redundant comments that restate what the code does" and "Function/method comments should explain the purpose and any important side effects, not just restate the name".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/deployment/launcher.go` around lines 820 - 878, Update the three helper comments to document the non-obvious contract: volumesEqual, labelsEqual, and mountsEqual perform element-wise positional comparisons (they require identical slice length and compare entries by index), so reordering of elements will be treated as a mismatch; alternatively, if the intended behavior is order-insensitive, implement sorting or set-based comparison and update the comment to describe that. Make the comment for each function explicitly state the positional comparison semantics and any assumptions about input ordering rather than merely restating the function name.controllers/deployment/specs_match_test.go (1)
17-35: Preserve composite type information in the fingerprint.
walkcurrently collapsesTand[]Tto the same representation, andName()is empty for unnamed composite types like pointers and maps. That leaves holes where a realSandboxSpecshape change will not trip this sentinel.Suggested refactor
func structFingerprint(t reflect.Type) string { var walk func(reflect.Type) string walk = func(t reflect.Type) string { - if t.Kind() == reflect.Slice { - t = t.Elem() - } - if t.Kind() != reflect.Struct { - return t.Name() - } - var parts []string - for i := range t.NumField() { - f := t.Field(i) - parts = append(parts, f.Name+":"+walk(f.Type)) - } - return strings.Join(parts, ",") + switch t.Kind() { + case reflect.Slice, reflect.Array, reflect.Pointer: + return t.Kind().String() + "<" + walk(t.Elem()) + ">" + case reflect.Map: + return "map<" + walk(t.Key()) + "," + walk(t.Elem()) + ">" + case reflect.Struct: + var parts []string + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + parts = append(parts, f.Name+":"+walk(f.Type)) + } + return "struct{" + strings.Join(parts, ",") + "}" + default: + return t.String() + } } h := sha256.Sum256([]byte(walk(t))) return fmt.Sprintf("%x", h[:8]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/deployment/specs_match_test.go` around lines 17 - 35, The fingerprinting collapses composite types (e.g., T vs []T, pointers, maps) and uses Name() which is empty for unnamed types, so changes can be missed; update structFingerprint's inner walk to preserve composite structure by encoding slice ("[]"+walk(elem)), pointer ("*"+walk(elem)), map ("map["+walk(key)+"]"+walk(elem)) and for unnamed types fall back to t.String() or t.Kind().String() instead of t.Name(); also ensure field iteration uses a proper for i := 0; i < t.NumField(); i++ and reference t.Field(i) when building the parts so the fingerprint reflects full type shapes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@controllers/deployment/launcher.go`:
- Around line 777-818: The spec comparison currently omits ShutdownTimeout so
changes to that field are ignored; update the matcher (the function that
compares spec1 and spec2—used by TestSpecsMatch) to compare
spec1.ShutdownTimeout and spec2.ShutdownTimeout and return a clear mismatch
message like "shutdown timeout mismatch" when they differ, and then add a new
case to the TestSpecsMatch unit test that alters only the ShutdownTimeout to
ensure the comparison fails when expected.
---
Nitpick comments:
In `@controllers/deployment/launcher.go`:
- Around line 820-878: Update the three helper comments to document the
non-obvious contract: volumesEqual, labelsEqual, and mountsEqual perform
element-wise positional comparisons (they require identical slice length and
compare entries by index), so reordering of elements will be treated as a
mismatch; alternatively, if the intended behavior is order-insensitive,
implement sorting or set-based comparison and update the comment to describe
that. Make the comment for each function explicitly state the positional
comparison semantics and any assumptions about input ordering rather than merely
restating the function name.
In `@controllers/deployment/specs_match_test.go`:
- Around line 17-35: The fingerprinting collapses composite types (e.g., T vs
[]T, pointers, maps) and uses Name() which is empty for unnamed types, so
changes can be missed; update structFingerprint's inner walk to preserve
composite structure by encoding slice ("[]"+walk(elem)), pointer
("*"+walk(elem)), map ("map["+walk(key)+"]"+walk(elem)) and for unnamed types
fall back to t.String() or t.Kind().String() instead of t.Name(); also ensure
field iteration uses a proper for i := 0; i < t.NumField(); i++ and reference
t.Field(i) when building the parts so the fingerprint reflects full type shapes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e42444b-1550-4846-b105-94465312f3ae
📒 Files selected for processing (2)
controllers/deployment/launcher.gocontrollers/deployment/specs_match_test.go
Compare ShutdownTimeout in specsMatch — it's user-configurable via the service config and was being silently ignored, so a grace period change would reuse the stale pool. Improved the struct fingerprint test to encode composite type shapes (slice, pointer, map) rather than collapsing them, so type-level changes are caught too. Dropped redundant helper comments that restated function names per project style guidelines.
|
|
||
| // structFingerprint recursively builds a string from a type's field names and | ||
| // their types, so that adding a field at any depth changes the hash. | ||
| func structFingerprint(t reflect.Type) string { |
There was a problem hiding this comment.
I'm a sucker for a good fingerprint.
phinze
deleted the
phinze/mir-944-specsmatch-ignores-volumesmounts-causing-stale-pool-reuse-on
branch
2 participants
specsMatchdecides whether the deployment launcher can reuse an existing sandbox pool across deploys. It was comparing the usual suspects — image, command, env vars, ports — but completely ignoring volumes and container mounts. So if you added a[[services.web.disks]]block and redeployed, the launcher would happily say "specs match!" and hand you back the old pool with no volume configuration. Your app would crash because the mount path didn't exist.The fix adds volume and mount comparison following the same pattern as the existing
portsEqualhelper. We also pulledspecsMatchoff theLauncherreceiver since it's a pure function that doesn't touch any instance state.On the testing side, we went TDD — wrote the failing volume/mount cases first, confirmed they were red, then made them green. There's also a fun little struct fingerprint test that recursively hashes field names across the entire
SandboxSpectype tree. If someone adds a field to any of the compared structs, the hash changes and the test breaks, reminding them to go updatespecsMatch. No more silent gaps.Fixes MIR-944