fix(preview): atomic writes, pinned images, structured logging, and error handling
- Atomic temp-file-rename writes in PreviewStore to prevent partial writes - Pin busybox image to 1.37 instead of latest for reproducibility - Add zerowrap structured logging to WaitForContainer and dispatcher Handle - Handle filepath.Match errors in matchesTagPatterns (skip malformed patterns) - Distinguish not-found vs internal errors in preview admin HTTP handlers
- commit
- e2d78b5d419639d1afea9b691acf86550f4fa23a
- author
- brice · · 2026-03-19 05:27 UTC
- parent
- 9f0ec73
- signature
- unsigned
5 files · +57 -6
M internal/adapters/in/http/admin/preview_handler.go +10 -2
| @@ -92,7 +92,11 @@ func (h *Handler) handlePreviewAction(w http.ResponseWriter, r *http.Request, pa | |||
| 92 | 92 | ||
| 93 | 93 | if err := h.previewSvc.Delete(ctx, name); err != nil { | |
| 94 | 94 | log.Error().Err(err).Str("name", name).Msg("failed to delete preview") | |
| 95 | - | h.sendError(w, http.StatusNotFound, "preview not found") | |
| 95 | + | if strings.Contains(err.Error(), "not found") { | |
| 96 | + | http.Error(w, err.Error(), http.StatusNotFound) | |
| 97 | + | } else { | |
| 98 | + | http.Error(w, err.Error(), http.StatusInternalServerError) | |
| 99 | + | } | |
| 96 | 100 | return | |
| 97 | 101 | } | |
| 98 | 102 | ||
| @@ -127,7 +131,11 @@ func (h *Handler) handlePreviewAction(w http.ResponseWriter, r *http.Request, pa | |||
| 127 | 131 | ||
| 128 | 132 | if err := h.previewSvc.Extend(ctx, name, ttl); err != nil { | |
| 129 | 133 | log.Error().Err(err).Str("name", name).Dur("ttl", ttl).Msg("failed to extend preview TTL") | |
| 130 | - | h.sendError(w, http.StatusNotFound, "preview not found") | |
| 134 | + | if strings.Contains(err.Error(), "not found") { | |
| 135 | + | http.Error(w, err.Error(), http.StatusNotFound) | |
| 136 | + | } else { | |
| 137 | + | http.Error(w, err.Error(), http.StatusInternalServerError) | |
| 138 | + | } | |
| 131 | 139 | return | |
| 132 | 140 | } | |
| 133 | 141 | ||
| @@ -92,7 +92,11 @@ func (h *Handler) handlePreviewAction(w http.ResponseWriter, r *http.Request, pa | |||||
| 92 | 92 | ||||
| 93 | if err := h.previewSvc.Delete(ctx, name); err != nil { | 93 | if err := h.previewSvc.Delete(ctx, name); err != nil { | ||
| 94 | log.Error().Err(err).Str("name", name).Msg("failed to delete preview") | 94 | log.Error().Err(err).Str("name", name).Msg("failed to delete preview") | ||
| 95 | - | h.sendError(w, http.StatusNotFound, "preview not found") | 95 | + | if strings.Contains(err.Error(), "not found") { |
| 96 | + | http.Error(w, err.Error(), http.StatusNotFound) | |||
| 97 | + | } else { | |||
| 98 | + | http.Error(w, err.Error(), http.StatusInternalServerError) | |||
| 99 | + | } | |||
| 96 | return | 100 | return | ||
| 97 | } | 101 | } | ||
| 98 | 102 | ||||
| @@ -127,7 +131,11 @@ func (h *Handler) handlePreviewAction(w http.ResponseWriter, r *http.Request, pa | |||||
| 127 | 131 | ||||
| 128 | if err := h.previewSvc.Extend(ctx, name, ttl); err != nil { | 132 | if err := h.previewSvc.Extend(ctx, name, ttl); err != nil { | ||
| 129 | log.Error().Err(err).Str("name", name).Dur("ttl", ttl).Msg("failed to extend preview TTL") | 133 | log.Error().Err(err).Str("name", name).Dur("ttl", ttl).Msg("failed to extend preview TTL") | ||
| 130 | - | h.sendError(w, http.StatusNotFound, "preview not found") | 134 | + | if strings.Contains(err.Error(), "not found") { |
| 135 | + | http.Error(w, err.Error(), http.StatusNotFound) | |||
| 136 | + | } else { | |||
| 137 | + | http.Error(w, err.Error(), http.StatusInternalServerError) | |||
| 138 | + | } | |||
| 131 | return | 139 | return | ||
| 132 | } | 140 | } | ||
| 133 | 141 | ||||
M internal/adapters/out/docker/runtime.go +11 -1
| @@ -216,14 +216,24 @@ func (r *Runtime) StartContainer(ctx context.Context, containerID string) error | |||
| 216 | 216 | ||
| 217 | 217 | // WaitForContainer waits for a container to stop running. | |
| 218 | 218 | func (r *Runtime) WaitForContainer(ctx context.Context, containerID string) error { | |
| 219 | + | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ | |
| 220 | + | zerowrap.FieldLayer: "adapter", | |
| 221 | + | zerowrap.FieldAdapter: "docker", | |
| 222 | + | zerowrap.FieldAction: "WaitForContainer", | |
| 223 | + | zerowrap.FieldEntityID: containerID, | |
| 224 | + | }) | |
| 225 | + | log := zerowrap.FromCtx(ctx) | |
| 226 | + | ||
| 219 | 227 | statusCh, errCh := r.client.ContainerWait(ctx, containerID, container.WaitConditionNotRunning) | |
| 220 | 228 | select { | |
| 221 | 229 | case err := <-errCh: | |
| 222 | 230 | if err != nil { | |
| 223 | - | return err | |
| 231 | + | return log.WrapErr(err, "failed to wait for container") | |
| 224 | 232 | } | |
| 225 | 233 | case <-statusCh: | |
| 226 | 234 | } | |
| 235 | + | ||
| 236 | + | log.Debug().Msg("container wait completed") | |
| 227 | 237 | return nil | |
| 228 | 238 | } | |
| 229 | 239 | ||
| @@ -216,14 +216,24 @@ func (r *Runtime) StartContainer(ctx context.Context, containerID string) error | |||||
| 216 | 216 | ||||
| 217 | // WaitForContainer waits for a container to stop running. | 217 | // WaitForContainer waits for a container to stop running. | ||
| 218 | func (r *Runtime) WaitForContainer(ctx context.Context, containerID string) error { | 218 | func (r *Runtime) WaitForContainer(ctx context.Context, containerID string) error { | ||
| 219 | + | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ | |||
| 220 | + | zerowrap.FieldLayer: "adapter", | |||
| 221 | + | zerowrap.FieldAdapter: "docker", | |||
| 222 | + | zerowrap.FieldAction: "WaitForContainer", | |||
| 223 | + | zerowrap.FieldEntityID: containerID, | |||
| 224 | + | }) | |||
| 225 | + | log := zerowrap.FromCtx(ctx) | |||
| 226 | + | ||||
| 219 | statusCh, errCh := r.client.ContainerWait(ctx, containerID, container.WaitConditionNotRunning) | 227 | statusCh, errCh := r.client.ContainerWait(ctx, containerID, container.WaitConditionNotRunning) | ||
| 220 | select { | 228 | select { | ||
| 221 | case err := <-errCh: | 229 | case err := <-errCh: | ||
| 222 | if err != nil { | 230 | if err != nil { | ||
| 223 | - | return err | 231 | + | return log.WrapErr(err, "failed to wait for container") |
| 224 | } | 232 | } | ||
| 225 | case <-statusCh: | 233 | case <-statusCh: | ||
| 226 | } | 234 | } | ||
| 235 | + | ||||
| 236 | + | log.Debug().Msg("container wait completed") | |||
| 227 | return nil | 237 | return nil | ||
| 228 | } | 238 | } | ||
| 229 | 239 | ||||
M internal/adapters/out/filesystem/previewstore.go +25 -1
| @@ -6,6 +6,7 @@ import ( | |||
| 6 | 6 | "errors" | |
| 7 | 7 | "io/fs" | |
| 8 | 8 | "os" | |
| 9 | + | "path/filepath" | |
| 9 | 10 | "sync" | |
| 10 | 11 | ||
| 11 | 12 | "github.com/bnema/gordon/internal/domain" | |
| @@ -61,5 +62,28 @@ func (s *PreviewStore) Save(_ context.Context, previews []domain.PreviewRoute) e | |||
| 61 | 62 | if err != nil { | |
| 62 | 63 | return err | |
| 63 | 64 | } | |
| 64 | - | return os.WriteFile(s.path, data, 0o644) | |
| 65 | + | ||
| 66 | + | // Atomic write: temp file → rename | |
| 67 | + | dir := filepath.Dir(s.path) | |
| 68 | + | tmp, err := os.CreateTemp(dir, ".previews-*.json.tmp") | |
| 69 | + | if err != nil { | |
| 70 | + | return err | |
| 71 | + | } | |
| 72 | + | tmpName := tmp.Name() | |
| 73 | + | ||
| 74 | + | if _, err := tmp.Write(data); err != nil { | |
| 75 | + | tmp.Close() | |
| 76 | + | os.Remove(tmpName) | |
| 77 | + | return err | |
| 78 | + | } | |
| 79 | + | if err := tmp.Sync(); err != nil { | |
| 80 | + | tmp.Close() | |
| 81 | + | os.Remove(tmpName) | |
| 82 | + | return err | |
| 83 | + | } | |
| 84 | + | if err := tmp.Close(); err != nil { | |
| 85 | + | os.Remove(tmpName) | |
| 86 | + | return err | |
| 87 | + | } | |
| 88 | + | return os.Rename(tmpName, s.path) | |
| 65 | 89 | } | |
| @@ -6,6 +6,7 @@ import ( | |||||
| 6 | "errors" | 6 | "errors" | ||
| 7 | "io/fs" | 7 | "io/fs" | ||
| 8 | "os" | 8 | "os" | ||
| 9 | + | "path/filepath" | |||
| 9 | "sync" | 10 | "sync" | ||
| 10 | 11 | ||||
| 11 | "github.com/bnema/gordon/internal/domain" | 12 | "github.com/bnema/gordon/internal/domain" | ||
| @@ -61,5 +62,28 @@ func (s *PreviewStore) Save(_ context.Context, previews []domain.PreviewRoute) e | |||||
| 61 | if err != nil { | 62 | if err != nil { | ||
| 62 | return err | 63 | return err | ||
| 63 | } | 64 | } | ||
| 64 | - | return os.WriteFile(s.path, data, 0o644) | 65 | + | |
| 66 | + | // Atomic write: temp file → rename | |||
| 67 | + | dir := filepath.Dir(s.path) | |||
| 68 | + | tmp, err := os.CreateTemp(dir, ".previews-*.json.tmp") | |||
| 69 | + | if err != nil { | |||
| 70 | + | return err | |||
| 71 | + | } | |||
| 72 | + | tmpName := tmp.Name() | |||
| 73 | + | ||||
| 74 | + | if _, err := tmp.Write(data); err != nil { | |||
| 75 | + | tmp.Close() | |||
| 76 | + | os.Remove(tmpName) | |||
| 77 | + | return err | |||
| 78 | + | } | |||
| 79 | + | if err := tmp.Sync(); err != nil { | |||
| 80 | + | tmp.Close() | |||
| 81 | + | os.Remove(tmpName) | |||
| 82 | + | return err | |||
| 83 | + | } | |||
| 84 | + | if err := tmp.Close(); err != nil { | |||
| 85 | + | os.Remove(tmpName) | |||
| 86 | + | return err | |||
| 87 | + | } | |||
| 88 | + | return os.Rename(tmpName, s.path) | |||
| 65 | } | 89 | } | ||
M internal/usecase/auto/dispatcher.go +10 -1
| @@ -4,6 +4,8 @@ import ( | |||
| 4 | 4 | "context" | |
| 5 | 5 | "path/filepath" | |
| 6 | 6 | ||
| 7 | + | "github.com/bnema/zerowrap" | |
| 8 | + | ||
| 7 | 9 | "github.com/bnema/gordon/internal/boundaries/out" | |
| 8 | 10 | "github.com/bnema/gordon/internal/domain" | |
| 9 | 11 | ) | |
| @@ -38,8 +40,11 @@ func (d *ImagePushDispatcher) CanHandle(t domain.EventType) bool { | |||
| 38 | 40 | } | |
| 39 | 41 | ||
| 40 | 42 | func (d *ImagePushDispatcher) Handle(ctx context.Context, event domain.Event) error { | |
| 43 | + | log := zerowrap.FromCtx(ctx) | |
| 44 | + | ||
| 41 | 45 | payload, ok := event.Data.(domain.ImagePushedPayload) | |
| 42 | 46 | if !ok { | |
| 47 | + | log.Warn().Str("event_type", string(event.Type)).Msg("dispatcher received event with unexpected payload type; forwarding to route handler") | |
| 43 | 48 | return d.routeHandler.Handle(ctx, event) | |
| 44 | 49 | } | |
| 45 | 50 | ||
| @@ -51,7 +56,11 @@ func (d *ImagePushDispatcher) Handle(ctx context.Context, event domain.Event) er | |||
| 51 | 56 | ||
| 52 | 57 | func (d *ImagePushDispatcher) matchesTagPatterns(reference string) bool { | |
| 53 | 58 | for _, pattern := range d.config.GetPreviewTagPatterns() { | |
| 54 | - | if matched, _ := filepath.Match(pattern, reference); matched { | |
| 59 | + | matched, err := filepath.Match(pattern, reference) | |
| 60 | + | if err != nil { | |
| 61 | + | continue // skip malformed patterns | |
| 62 | + | } | |
| 63 | + | if matched { | |
| 55 | 64 | return true | |
| 56 | 65 | } | |
| 57 | 66 | } | |
| @@ -4,6 +4,8 @@ import ( | |||||
| 4 | "context" | 4 | "context" | ||
| 5 | "path/filepath" | 5 | "path/filepath" | ||
| 6 | 6 | ||||
| 7 | + | "github.com/bnema/zerowrap" | |||
| 8 | + | ||||
| 7 | "github.com/bnema/gordon/internal/boundaries/out" | 9 | "github.com/bnema/gordon/internal/boundaries/out" | ||
| 8 | "github.com/bnema/gordon/internal/domain" | 10 | "github.com/bnema/gordon/internal/domain" | ||
| 9 | ) | 11 | ) | ||
| @@ -38,8 +40,11 @@ func (d *ImagePushDispatcher) CanHandle(t domain.EventType) bool { | |||||
| 38 | } | 40 | } | ||
| 39 | 41 | ||||
| 40 | func (d *ImagePushDispatcher) Handle(ctx context.Context, event domain.Event) error { | 42 | func (d *ImagePushDispatcher) Handle(ctx context.Context, event domain.Event) error { | ||
| 43 | + | log := zerowrap.FromCtx(ctx) | |||
| 44 | + | ||||
| 41 | payload, ok := event.Data.(domain.ImagePushedPayload) | 45 | payload, ok := event.Data.(domain.ImagePushedPayload) | ||
| 42 | if !ok { | 46 | if !ok { | ||
| 47 | + | log.Warn().Str("event_type", string(event.Type)).Msg("dispatcher received event with unexpected payload type; forwarding to route handler") | |||
| 43 | return d.routeHandler.Handle(ctx, event) | 48 | return d.routeHandler.Handle(ctx, event) | ||
| 44 | } | 49 | } | ||
| 45 | 50 | ||||
| @@ -51,7 +56,11 @@ func (d *ImagePushDispatcher) Handle(ctx context.Context, event domain.Event) er | |||||
| 51 | 56 | ||||
| 52 | func (d *ImagePushDispatcher) matchesTagPatterns(reference string) bool { | 57 | func (d *ImagePushDispatcher) matchesTagPatterns(reference string) bool { | ||
| 53 | for _, pattern := range d.config.GetPreviewTagPatterns() { | 58 | for _, pattern := range d.config.GetPreviewTagPatterns() { | ||
| 54 | - | if matched, _ := filepath.Match(pattern, reference); matched { | 59 | + | matched, err := filepath.Match(pattern, reference) |
| 60 | + | if err != nil { | |||
| 61 | + | continue // skip malformed patterns | |||
| 62 | + | } | |||
| 63 | + | if matched { | |||
| 55 | return true | 64 | return true | ||
| 56 | } | 65 | } | ||
| 57 | } | 66 | } | ||
M internal/usecase/auto/preview/volumes.go +1 -1
| @@ -48,7 +48,7 @@ func CloneVolumes(ctx context.Context, cloner VolumeCloner, previewName string, | |||
| 48 | 48 | } | |
| 49 | 49 | ||
| 50 | 50 | helperConfig := &domain.ContainerConfig{ | |
| 51 | - | Image: "busybox:latest", | |
| 51 | + | Image: "busybox:1.37", | |
| 52 | 52 | Name: "gordon-vol-copy-" + destName, | |
| 53 | 53 | Cmd: []string{"cp", "-a", "/src/.", "/dst/"}, | |
| 54 | 54 | Volumes: map[string]string{ | |
| @@ -48,7 +48,7 @@ func CloneVolumes(ctx context.Context, cloner VolumeCloner, previewName string, | |||||
| 48 | } | 48 | } | ||
| 49 | 49 | ||||
| 50 | helperConfig := &domain.ContainerConfig{ | 50 | helperConfig := &domain.ContainerConfig{ | ||
| 51 | - | Image: "busybox:latest", | 51 | + | Image: "busybox:1.37", |
| 52 | Name: "gordon-vol-copy-" + destName, | 52 | Name: "gordon-vol-copy-" + destName, | ||
| 53 | Cmd: []string{"cp", "-a", "/src/.", "/dst/"}, | 53 | Cmd: []string{"cp", "-a", "/src/.", "/dst/"}, | ||
| 54 | Volumes: map[string]string{ | 54 | Volumes: map[string]string{ | ||