4c6e36a fix(download): harden engine download setup and terminal state · dumber

M internal/infrastructure/cef/download_handler.go +33 -22


@@ -3,8 +3,6 @@ package cef
3	3		import (
4	4		"context"
5	5		"fmt"
6		-	"net/http"
7		-	"strings"
8	6		"sync"
9	7
10	8		purecef "github.com/bnema/purego-cef/cef"

@@ -15,16 +13,22 @@ import (
15	13		"github.com/bnema/dumber/internal/logging"
16	14		)
17	15
	16	+	// maxFinishedEntries caps the finished-ID set. When the cap is reached the set
	17	+	// is cleared, accepting a tiny risk of a duplicate terminal event for very old
	18	+	// downloads. In practice download IDs are monotonically increasing uint32s and
	19	+	// no user session will approach this limit.
	20	+	const maxFinishedEntries = 1024
	21	+
18	22		type downloadHandler struct {
19		-	runtime *downloadruntime.Runtime
20		-	mu sync.Mutex
21		-	active map[uint32]cefDownloadState
	23	+	runtime *downloadruntime.Runtime
	24	+	mu sync.Mutex
	25	+	active map[uint32]cefDownloadState
	26	+	finished map[uint32]struct{} // IDs that already emitted a terminal event
22	27		}
23	28
24	29		type cefDownloadState struct {
25	30		filename string
26	31		destination string
27		-	finished bool
28	32		}
29	33
30	34		type cefDownloadResponseAdapter struct {

@@ -37,13 +41,14 @@ func newDownloadHandler(
37	41		preparer port.DownloadPreparer,
38	42		) *downloadHandler {
39	43		return &downloadHandler{
40		-	runtime: downloadruntime.New(downloadPath, eventHandler, preparer),
41		-	active: make(map[uint32]cefDownloadState),
	44	+	runtime: downloadruntime.New(downloadPath, eventHandler, preparer),
	45	+	active: make(map[uint32]cefDownloadState),
	46	+	finished: make(map[uint32]struct{}),
42	47		}
43	48		}
44	49
45		-	func (h *downloadHandler) canDownload(_ purecef.Browser, _, requestMethod string) bool {
46		-	return requestMethod == "" \|\| strings.EqualFold(requestMethod, http.MethodGet)
	50	+	func (h *downloadHandler) canDownload(_ purecef.Browser, _, _ string) bool {
	51	+	return true
47	52		}
48	53
49	54		func (h *downloadHandler) onBeforeDownload(

@@ -89,16 +94,25 @@ func (h *downloadHandler) onDownloadUpdated(
89	94		}
90	95
91	96		id := downloadItem.GetID()
	97	+
	98	+	// Fast path: skip already-finished downloads to avoid re-populating active.
	99	+	h.mu.Lock()
	100	+	_, done := h.finished[id]
	101	+	h.mu.Unlock()
	102	+	if done {
	103	+	return
	104	+	}
	105	+
92	106		state := h.currentState(id, downloadItem)
93	107
94	108		switch {
95	109		case downloadItem.IsComplete():
96		-	if !h.markFinished(id, state) {
	110	+	if !h.markFinished(id) {
97	111		return
98	112		}
99	113		h.runtime.EmitFinished(ctx, state.filename, state.destination)
100	114		case downloadItem.IsCanceled() \|\| downloadItem.IsInterrupted():
101		-	if !h.markFinished(id, state) {
	115	+	if !h.markFinished(id) {
102	116		return
103	117		}
104	118		var err error

@@ -129,21 +143,18 @@ func (h *downloadHandler) currentState(id uint32, item purecef.DownloadItem) cef
129	143		return state
130	144		}
131	145
132		-	func (h *downloadHandler) markFinished(id uint32, state cefDownloadState) bool {
	146	+	func (h *downloadHandler) markFinished(id uint32) bool {
133	147		h.mu.Lock()
134	148		defer h.mu.Unlock()
135	149
136		-	current, ok := h.active[id]
137		-	if !ok {
138		-	state.finished = true
139		-	h.active[id] = state
140		-	return true
141		-	}
142		-	if current.finished {
	150	+	if _, seen := h.finished[id]; seen {
143	151		return false
144	152		}
145		-	state.finished = true
146		-	h.active[id] = state
	153	+	delete(h.active, id)
	154	+	if len(h.finished) >= maxFinishedEntries {
	155	+	clear(h.finished)
	156	+	}
	157	+	h.finished[id] = struct{}{}
147	158		return true
148	159		}
149	160

 import (
 	"context"
 	"fmt"
-	"net/http"
-	"strings"
 	"sync"
 	purecef "github.com/bnema/purego-cef/cef"
 	"github.com/bnema/dumber/internal/logging"
 )
+// maxFinishedEntries caps the finished-ID set. When the cap is reached the set
+// is cleared, accepting a tiny risk of a duplicate terminal event for very old
+// downloads. In practice download IDs are monotonically increasing uint32s and
+// no user session will approach this limit.
+const maxFinishedEntries = 1024
 type downloadHandler struct {
-	runtime *downloadruntime.Runtime
+	runtime  *downloadruntime.Runtime
-	mu      sync.Mutex
+	mu       sync.Mutex
-	active  map[uint32]cefDownloadState
+	active   map[uint32]cefDownloadState
+	finished map[uint32]struct{} // IDs that already emitted a terminal event
 }
 type cefDownloadState struct {
 	filename    string
 	destination string
-	finished    bool
 }
 type cefDownloadResponseAdapter struct {
 	preparer port.DownloadPreparer,
 ) *downloadHandler {
 	return &downloadHandler{
-		runtime: downloadruntime.New(downloadPath, eventHandler, preparer),
+		runtime:  downloadruntime.New(downloadPath, eventHandler, preparer),
-		active:  make(map[uint32]cefDownloadState),
+		active:   make(map[uint32]cefDownloadState),
+		finished: make(map[uint32]struct{}),
 	}
 }
-func (h *downloadHandler) canDownload(_ purecef.Browser, _, requestMethod string) bool {
+func (h *downloadHandler) canDownload(_ purecef.Browser, _, _ string) bool {
-	return requestMethod == "" || strings.EqualFold(requestMethod, http.MethodGet)
+	return true
 }
 func (h *downloadHandler) onBeforeDownload(
 	}
 	id := downloadItem.GetID()
+	// Fast path: skip already-finished downloads to avoid re-populating active.
+	h.mu.Lock()
+	_, done := h.finished[id]
+	h.mu.Unlock()
+	if done {
+		return
+	}
 	state := h.currentState(id, downloadItem)
 	switch {
 	case downloadItem.IsComplete():
-		if !h.markFinished(id, state) {
+		if !h.markFinished(id) {
 			return
 		}
 		h.runtime.EmitFinished(ctx, state.filename, state.destination)
 	case downloadItem.IsCanceled() || downloadItem.IsInterrupted():
-		if !h.markFinished(id, state) {
+		if !h.markFinished(id) {
 			return
 		}
 		var err error
 	return state
 }
-func (h *downloadHandler) markFinished(id uint32, state cefDownloadState) bool {
+func (h *downloadHandler) markFinished(id uint32) bool {
 	h.mu.Lock()
 	defer h.mu.Unlock()
-	current, ok := h.active[id]
+	if _, seen := h.finished[id]; seen {
-	if !ok {
-		state.finished = true
-		h.active[id] = state
-		return true
-	}
-	if current.finished {
 		return false
 	}
-	state.finished = true
+	delete(h.active, id)
-	h.active[id] = state
+	if len(h.finished) >= maxFinishedEntries {
+		clear(h.finished)
+	}
+	h.finished[id] = struct{}{}
 	return true
 }

M internal/infrastructure/cef/download_handler_test.go +37 -0


@@ -92,4 +92,41 @@ func TestCEFDownloadHandlerLifecycle(t *testing.T) {
92	92		require.Len(t, events.events, 2)
93	93		require.Equal(t, port.DownloadEventFinished, events.events[1].Type)
94	94		require.Equal(t, "image.iso", events.events[1].Filename)
	95	+
	96	+	// Active map should be cleaned up after terminal event.
	97	+	handler.mu.Lock()
	98	+	_, inActive := handler.active[7]
	99	+	_, inFinished := handler.finished[7]
	100	+	handler.mu.Unlock()
	101	+	require.False(t, inActive, "completed download should be removed from active map")
	102	+	require.True(t, inFinished, "completed download should be in finished set")
	103	+	}
	104	+
	105	+	func TestCanDownload_AllowsAllMethods(t *testing.T) {
	106	+	preparer := usecase.NewPrepareDownloadUseCase(nil)
	107	+	handler := newDownloadHandler("/tmp/downloads", nil, preparer)
	108	+
	109	+	require.True(t, handler.canDownload(nil, "https://example.com/file.zip", "GET"))
	110	+	require.True(t, handler.canDownload(nil, "https://example.com/file.zip", "POST"))
	111	+	require.True(t, handler.canDownload(nil, "https://example.com/file.zip", "PUT"))
	112	+	require.True(t, handler.canDownload(nil, "https://example.com/file.zip", ""))
	113	+	}
	114	+
	115	+	func TestMarkFinished_SuppressesDuplicatesAfterCleanup(t *testing.T) {
	116	+	preparer := usecase.NewPrepareDownloadUseCase(nil)
	117	+	handler := newDownloadHandler("/tmp/downloads", nil, preparer)
	118	+
	119	+	// Simulate a download that was tracked through onBeforeDownload.
	120	+	handler.mu.Lock()
	121	+	handler.active[42] = cefDownloadState{filename: "test.bin", destination: "/tmp/test.bin"}
	122	+	handler.mu.Unlock()
	123	+
	124	+	require.True(t, handler.markFinished(42), "first terminal event should succeed")
	125	+	require.False(t, handler.markFinished(42), "second terminal event should be suppressed")
	126	+
	127	+	// Active map should not contain the entry.
	128	+	handler.mu.Lock()
	129	+	_, inActive := handler.active[42]
	130	+	handler.mu.Unlock()
	131	+	require.False(t, inActive)
95	132		}


@@ -92,4 +92,41 @@ func TestCEFDownloadHandlerLifecycle(t *testing.T) {
92	require.Len(t, events.events, 2)	92		require.Len(t, events.events, 2)
93	require.Equal(t, port.DownloadEventFinished, events.events[1].Type)	93		require.Equal(t, port.DownloadEventFinished, events.events[1].Type)
94	require.Equal(t, "image.iso", events.events[1].Filename)	94		require.Equal(t, "image.iso", events.events[1].Filename)
		95	+
		96	+	// Active map should be cleaned up after terminal event.
		97	+	handler.mu.Lock()
		98	+	_, inActive := handler.active[7]
		99	+	_, inFinished := handler.finished[7]
		100	+	handler.mu.Unlock()
		101	+	require.False(t, inActive, "completed download should be removed from active map")
		102	+	require.True(t, inFinished, "completed download should be in finished set")
		103	+	}
		104	+
		105	+	func TestCanDownload_AllowsAllMethods(t *testing.T) {
		106	+	preparer := usecase.NewPrepareDownloadUseCase(nil)
		107	+	handler := newDownloadHandler("/tmp/downloads", nil, preparer)
		108	+
		109	+	require.True(t, handler.canDownload(nil, "https://example.com/file.zip", "GET"))
		110	+	require.True(t, handler.canDownload(nil, "https://example.com/file.zip", "POST"))
		111	+	require.True(t, handler.canDownload(nil, "https://example.com/file.zip", "PUT"))
		112	+	require.True(t, handler.canDownload(nil, "https://example.com/file.zip", ""))
		113	+	}
		114	+
		115	+	func TestMarkFinished_SuppressesDuplicatesAfterCleanup(t *testing.T) {
		116	+	preparer := usecase.NewPrepareDownloadUseCase(nil)
		117	+	handler := newDownloadHandler("/tmp/downloads", nil, preparer)
		118	+
		119	+	// Simulate a download that was tracked through onBeforeDownload.
		120	+	handler.mu.Lock()
		121	+	handler.active[42] = cefDownloadState{filename: "test.bin", destination: "/tmp/test.bin"}
		122	+	handler.mu.Unlock()
		123	+
		124	+	require.True(t, handler.markFinished(42), "first terminal event should succeed")
		125	+	require.False(t, handler.markFinished(42), "second terminal event should be suppressed")
		126	+
		127	+	// Active map should not contain the entry.
		128	+	handler.mu.Lock()
		129	+	_, inActive := handler.active[42]
		130	+	handler.mu.Unlock()
		131	+	require.False(t, inActive)
95	}	132		}

M internal/infrastructure/cef/engine.go +4 -0


@@ -3,6 +3,7 @@ package cef
3	3		import (
4	4		"context"
5	5		"crypto/subtle"
	6	+	"fmt"
6	7		"sync"
7	8		"sync/atomic"
8	9		"time"

@@ -265,6 +266,9 @@ func (e *Engine) ConfigureDownloads(
265	266		eventHandler port.DownloadEventHandler,
266	267		preparer port.DownloadPreparer,
267	268		) error {
	269	+	if preparer == nil {
	270	+	return fmt.Errorf("cef: download preparer is required")
	271	+	}
268	272		e.downloadMu.Lock()
269	273		e.downloadHandler = newDownloadHandler(downloadPath, eventHandler, preparer)
270	274		e.downloadMu.Unlock()


@@ -3,6 +3,7 @@ package cef
3	import (	3		import (
4	"context"	4		"context"
5	"crypto/subtle"	5		"crypto/subtle"
		6	+	"fmt"
6	"sync"	7		"sync"
7	"sync/atomic"	8		"sync/atomic"
8	"time"	9		"time"

@@ -265,6 +266,9 @@ func (e *Engine) ConfigureDownloads(
265	eventHandler port.DownloadEventHandler,	266		eventHandler port.DownloadEventHandler,
266	preparer port.DownloadPreparer,	267		preparer port.DownloadPreparer,
267	) error {	268		) error {
		269	+	if preparer == nil {
		270	+	return fmt.Errorf("cef: download preparer is required")
		271	+	}
268	e.downloadMu.Lock()	272		e.downloadMu.Lock()
269	e.downloadHandler = newDownloadHandler(downloadPath, eventHandler, preparer)	273		e.downloadHandler = newDownloadHandler(downloadPath, eventHandler, preparer)
270	e.downloadMu.Unlock()	274		e.downloadMu.Unlock()

M internal/infrastructure/cef/engine_contract_test.go +10 -0


@@ -19,6 +19,16 @@ func TestEngineConfigureDownloads_StoresHandler(t *testing.T) {
19	19		require.NotNil(t, eng.currentDownloadHandler())
20	20		}
21	21
	22	+	func TestEngineConfigureDownloads_NilPreparer(t *testing.T) {
	23	+	eng := &Engine{}
	24	+
	25	+	err := eng.ConfigureDownloads(context.Background(), "/tmp", nil, nil)
	26	+
	27	+	require.Error(t, err)
	28	+	require.Contains(t, err.Error(), "preparer is required")
	29	+	require.Nil(t, eng.currentDownloadHandler())
	30	+	}
	31	+
22	32		func TestWebViewFactoryCreateRelated_ReturnsUnsupported(t *testing.T) {
23	33		factory := &WebViewFactory{}
24	34		wv, err := factory.CreateRelated(context.Background(), port.WebViewID(42))


@@ -19,6 +19,16 @@ func TestEngineConfigureDownloads_StoresHandler(t *testing.T) {
19	require.NotNil(t, eng.currentDownloadHandler())	19		require.NotNil(t, eng.currentDownloadHandler())
20	}	20		}
21		21
		22	+	func TestEngineConfigureDownloads_NilPreparer(t *testing.T) {
		23	+	eng := &Engine{}
		24	+
		25	+	err := eng.ConfigureDownloads(context.Background(), "/tmp", nil, nil)
		26	+
		27	+	require.Error(t, err)
		28	+	require.Contains(t, err.Error(), "preparer is required")
		29	+	require.Nil(t, eng.currentDownloadHandler())
		30	+	}
		31	+
22	func TestWebViewFactoryCreateRelated_ReturnsUnsupported(t *testing.T) {	32		func TestWebViewFactoryCreateRelated_ReturnsUnsupported(t *testing.T) {
23	factory := &WebViewFactory{}	33		factory := &WebViewFactory{}
24	wv, err := factory.CreateRelated(context.Background(), port.WebViewID(42))	34		wv, err := factory.CreateRelated(context.Background(), port.WebViewID(42))

M internal/infrastructure/downloadruntime/runtime.go +2 -4


@@ -20,11 +20,9 @@ type Runtime struct {
20	20		preparer port.DownloadPreparer
21	21		}
22	22
	23	+	// New creates a Runtime. The caller must ensure preparer is non-nil;
	24	+	// ConfigureDownloads validates this before reaching here.
23	25		func New(downloadPath string, eventHandler port.DownloadEventHandler, preparer port.DownloadPreparer) *Runtime {
24		-	if preparer == nil {
25		-	panic("preparer is required")
26		-	}
27		-
28	26		return &Runtime{
29	27		downloadPath: downloadPath,
30	28		eventHandler: eventHandler,

 	preparer     port.DownloadPreparer
 }
+// New creates a Runtime. The caller must ensure preparer is non-nil;
+// ConfigureDownloads validates this before reaching here.
 func New(downloadPath string, eventHandler port.DownloadEventHandler, preparer port.DownloadPreparer) *Runtime {
-	if preparer == nil {
-		panic("preparer is required")
-	}
 	return &Runtime{
 		downloadPath: downloadPath,
 		eventHandler: eventHandler,

M internal/infrastructure/downloadruntime/runtime_test.go +6 -0


@@ -43,3 +43,9 @@ func TestRuntimeResolveDestinationAndEvents(t *testing.T) {
43	43		require.Equal(t, port.DownloadEventStarted, events.events[0].Type)
44	44		require.Equal(t, port.DownloadEventFinished, events.events[1].Type)
45	45		}
	46	+
	47	+	func TestNewRuntime_NilPreparer_NoPanic(t *testing.T) {
	48	+	require.NotPanics(t, func() {
	49	+	_ = New("/tmp/downloads", nil, nil)
	50	+	})
	51	+	}


@@ -43,3 +43,9 @@ func TestRuntimeResolveDestinationAndEvents(t *testing.T) {
43	require.Equal(t, port.DownloadEventStarted, events.events[0].Type)	43		require.Equal(t, port.DownloadEventStarted, events.events[0].Type)
44	require.Equal(t, port.DownloadEventFinished, events.events[1].Type)	44		require.Equal(t, port.DownloadEventFinished, events.events[1].Type)
45	}	45		}
		46	+
		47	+	func TestNewRuntime_NilPreparer_NoPanic(t *testing.T) {
		48	+	require.NotPanics(t, func() {
		49	+	_ = New("/tmp/downloads", nil, nil)
		50	+	})
		51	+	}

M internal/infrastructure/webkit/download_handler.go +1 -4


@@ -20,15 +20,12 @@ type DownloadHandler struct {
20	20		}
21	21
22	22		// NewDownloadHandler creates a new download handler.
23		-	// Panics if preparer is nil (fail fast on misconfiguration).
	23	+	// The caller (ConfigureDownloads) must validate that preparer is non-nil.
24	24		func NewDownloadHandler(
25	25		downloadPath string,
26	26		handler port.DownloadEventHandler,
27	27		preparer port.DownloadPreparer,
28	28		) *DownloadHandler {
29		-	if preparer == nil {
30		-	panic("preparer is required")
31		-	}
32	29		return &DownloadHandler{
33	30		runtime: downloadruntime.New(downloadPath, handler, preparer),
34	31		}

 }
 // NewDownloadHandler creates a new download handler.
-// Panics if preparer is nil (fail fast on misconfiguration).
+// The caller (ConfigureDownloads) must validate that preparer is non-nil.
 func NewDownloadHandler(
 	downloadPath string,
 	handler port.DownloadEventHandler,
 	preparer port.DownloadPreparer,
 ) *DownloadHandler {
-	if preparer == nil {
-		panic("preparer is required")
-	}
 	return &DownloadHandler{
 		runtime: downloadruntime.New(downloadPath, handler, preparer),
 	}

M internal/infrastructure/webkit/engine.go +3 -0


@@ -120,6 +120,9 @@ func (e *Engine) ConfigureDownloads(
120	120		if e.wkCtx == nil {
121	121		return fmt.Errorf("webkit context not initialized")
122	122		}
	123	+	if preparer == nil {
	124	+	return fmt.Errorf("download preparer is required")
	125	+	}
123	126		handler := NewDownloadHandler(downloadPath, eventHandler, preparer)
124	127		e.wkCtx.SetDownloadHandler(ctx, handler)
125	128		e.downloadPath = downloadPath


@@ -120,6 +120,9 @@ func (e *Engine) ConfigureDownloads(
120	if e.wkCtx == nil {	120		if e.wkCtx == nil {
121	return fmt.Errorf("webkit context not initialized")	121		return fmt.Errorf("webkit context not initialized")
122	}	122		}
		123	+	if preparer == nil {
		124	+	return fmt.Errorf("download preparer is required")
		125	+	}
123	handler := NewDownloadHandler(downloadPath, eventHandler, preparer)	126		handler := NewDownloadHandler(downloadPath, eventHandler, preparer)
124	e.wkCtx.SetDownloadHandler(ctx, handler)	127		e.wkCtx.SetDownloadHandler(ctx, handler)
125	e.downloadPath = downloadPath	128		e.downloadPath = downloadPath

M internal/infrastructure/webkit/engine_runtime_test.go +8 -0


@@ -46,6 +46,14 @@ func TestEngine_ConfigureDownloads_NilContext(t *testing.T) {
46	46		require.Contains(t, err.Error(), "webkit context not initialized")
47	47		}
48	48
	49	+	func TestNewDownloadHandler_NilPreparer_NoPanic(t *testing.T) {
	50	+	// After removing the panic, NewDownloadHandler with nil preparer should
	51	+	// not panic. The caller (ConfigureDownloads) is responsible for validation.
	52	+	require.NotPanics(t, func() {
	53	+	_ = NewDownloadHandler("/tmp", nil, nil)
	54	+	})
	55	+	}
	56	+
49	57		func TestEngine_Close_NilPool(t *testing.T) {
50	58		e := &Engine{}
51	59		err := e.Close()


@@ -46,6 +46,14 @@ func TestEngine_ConfigureDownloads_NilContext(t *testing.T) {
46	require.Contains(t, err.Error(), "webkit context not initialized")	46		require.Contains(t, err.Error(), "webkit context not initialized")
47	}	47		}
48		48
		49	+	func TestNewDownloadHandler_NilPreparer_NoPanic(t *testing.T) {
		50	+	// After removing the panic, NewDownloadHandler with nil preparer should
		51	+	// not panic. The caller (ConfigureDownloads) is responsible for validation.
		52	+	require.NotPanics(t, func() {
		53	+	_ = NewDownloadHandler("/tmp", nil, nil)
		54	+	})
		55	+	}
		56	+
49	func TestEngine_Close_NilPool(t *testing.T) {	57		func TestEngine_Close_NilPool(t *testing.T) {
50	e := &Engine{}	58		e := &Engine{}
51	err := e.Close()	59		err := e.Close()