Surfaced by the high-effort retrospective /review of PR #7 (v0.2.1: hardening — resume idempotency + orphan cleanup + port collision).
Symptom
dockerutil.UpOrReuse (pkg/dockerutil/container.go) returns skip=true as soon as it finds a same-named container that is Running, with zero comparison against what the caller would actually configure now. Every plugin's dockerUp computes its desired config (image ref, env vars, command-line flags) and THEN calls UpOrReuse — so when skip=true, none of that freshly-computed config is ever applied to the already-running container. Three concrete, independently-confirmed instances:
- mysql/postgres/elasticsearch: image drift.
imageRef := pickDockerImage(req) is computed before the UpOrReuse skip-check; bumping the image pin in .bough.yaml (e.g. mysql:8.0 → mysql:8.4) and resuming an existing worktree silently keeps the OLD container running — Up/ReadyCheck both report success.
- elasticsearch: JVM heap.
pickHeap/buildDockerEnv (ES_JAVA_OPTS) are also computed after the skip-check. Raising es.heap to head off a known OOM before a big reindex has zero effect on a resumed worktree; the next heavy workload OOM-kills the same under-sized heap.
- redis: auth password (security-relevant). The
redis-server --requirepass <pw> command-line flag (from req.Extras["redis.password"]) is assembled after the skip-check too. Setting or rotating redis.password and resuming does nothing — the running redis-server keeps enforcing whatever auth state it was launched with. A rotation meant to invalidate a compromised password silently fails to take effect; enabling auth on a previously-open instance silently fails to enable it.
The only way to actually apply any of these three today is an undocumented manual bough remove + bough create.
Why this needs a design decision, not a bundled fix
UpOrReuse's current contract is intentionally cheap (one ContainerInspect, no config comparison) and is shared by all 4 engine plugins. Properly closing this requires deciding, per plugin, what "config" means for drift-detection purposes (image tag? full env diff? specific flags only?) and how to compare it against ContainerInspect's reported Config — a real design surface, not a same-shape bug each plugin already has a fix for. Options:
- Extend
UpOrReuse (or a new sibling) to accept an expected image ref (+ maybe a config-hash label written at create time) and compare against ContainerInspect before returning skip=true; recreate on mismatch.
- Have each plugin do its own post-skip drift check for the fields it cares about, logging a loud warning ("resumed container's heap/password does not match .bough.yaml; run
bough remove + bough create to apply") rather than silently doing nothing — cheaper, but still requires a design call on what to check per plugin.
- At minimum, document the current limitation prominently (README / PLUGIN_AUTHOR_GUIDE) so "why didn't my config change take effect" isn't a silent trap.
Given the redis password case has real security implications (a rotation silently not taking effect), this is worth prioritizing over the image/heap cases even if they're deferred further.
Surfaced by the high-effort retrospective
/reviewof PR #7 (v0.2.1: hardening — resume idempotency + orphan cleanup + port collision).Symptom
dockerutil.UpOrReuse(pkg/dockerutil/container.go) returnsskip=trueas soon as it finds a same-named container that isRunning, with zero comparison against what the caller would actually configure now. Every plugin'sdockerUpcomputes its desired config (image ref, env vars, command-line flags) and THEN callsUpOrReuse— so whenskip=true, none of that freshly-computed config is ever applied to the already-running container. Three concrete, independently-confirmed instances:imageRef := pickDockerImage(req)is computed before theUpOrReuseskip-check; bumping the image pin in.bough.yaml(e.g.mysql:8.0→mysql:8.4) and resuming an existing worktree silently keeps the OLD container running —Up/ReadyCheckboth report success.pickHeap/buildDockerEnv(ES_JAVA_OPTS) are also computed after the skip-check. Raisinges.heapto head off a known OOM before a big reindex has zero effect on a resumed worktree; the next heavy workload OOM-kills the same under-sized heap.redis-server --requirepass <pw>command-line flag (fromreq.Extras["redis.password"]) is assembled after the skip-check too. Setting or rotatingredis.passwordand resuming does nothing — the runningredis-serverkeeps enforcing whatever auth state it was launched with. A rotation meant to invalidate a compromised password silently fails to take effect; enabling auth on a previously-open instance silently fails to enable it.The only way to actually apply any of these three today is an undocumented manual
bough remove+bough create.Why this needs a design decision, not a bundled fix
UpOrReuse's current contract is intentionally cheap (oneContainerInspect, no config comparison) and is shared by all 4 engine plugins. Properly closing this requires deciding, per plugin, what "config" means for drift-detection purposes (image tag? full env diff? specific flags only?) and how to compare it againstContainerInspect's reportedConfig— a real design surface, not a same-shape bug each plugin already has a fix for. Options:UpOrReuse(or a new sibling) to accept an expected image ref (+ maybe a config-hash label written at create time) and compare againstContainerInspectbefore returning skip=true; recreate on mismatch.bough remove+bough createto apply") rather than silently doing nothing — cheaper, but still requires a design call on what to check per plugin.Given the redis password case has real security implications (a rotation silently not taking effect), this is worth prioritizing over the image/heap cases even if they're deferred further.