Structured concurrency design #130

New issue

Open

opened 2026-07-17 14:06:51 +00:00 by eliribble · 3 comments

eliribble commented 2026-07-17 14:06:51 +00:00

Owner

Copy link

nidus-sync currently has a problem shutting down the backend. Shutdown on a SIGINT is taking 20 seconds or more even on a dev machine where the process should be totally idle and shutdown should be 10s of milliseconds tops. I have a suspicion that it's caused by waiting for timeouts on some contexts rather than gracefully shutting down goroutines. A recent shutdown of nidus-sync showed this in the log:

^C2:02PM INF Received shutdown signal, shutting down...
2:02PM ERR stderr sync error="sync /dev/stderr: invalid argument"
2:02PM DBG Exiting listen-and-serve goroutine
2:02PM DBG pushed event to client env-org=0 type=3
2:02PM DBG Client closed connection id=40d76fc4-81e7-11f1-83be-18c04d5488aa org=1 user=1
2:02PM INF incoming_request bytes_in= bytes_out=7440 latency_ms=376873.626075 method=GET remote_ip=2600:8800:2b00:4e9:8c1b:d454:187b:9d47 status=200 url=/api/events
2:02PM INF DB notification context err error="context canceled"
2:02PM DBG Exiting listenForJobs
2:02PM DBG Exited job listener goroutine
2:02PM DBG Exited email websocket
2:02PM INF Shutdown complete
2:02PM INF Final cleanup
2:02PM ERR sync stderr error="sync /dev/stderr: invalid argument"
panic: can't sync stderr

goroutine 1 [running]:
main.main()
        /home/eliribble/src/nidus-sync/main.go:283 +0x200a

I'd like you to research the various goroutine/concurrency patterns in the nidus-sync codebase. Try to group them by pattern with an eye toward any patterns that may be missing a canceleable context, leaking a goroutine, or failing to check the context when waiting for other signals. Any of these could contribute to slow shutdown.

Additionally, I'm considering using structured concurrency from a library like https://github.com/sourcegraph/conc. I'd be interested in an assesment of the feasibility of refactoring to use conc as part of the study.

nidus-sync currently has a problem shutting down the backend. Shutdown on a SIGINT is taking 20 seconds or more even on a dev machine where the process should be totally idle and shutdown should be 10s of milliseconds tops. I have a suspicion that it's caused by waiting for timeouts on some contexts rather than gracefully shutting down goroutines. A recent shutdown of nidus-sync showed this in the log: ``` ^C2:02PM INF Received shutdown signal, shutting down... 2:02PM ERR stderr sync error="sync /dev/stderr: invalid argument" 2:02PM DBG Exiting listen-and-serve goroutine 2:02PM DBG pushed event to client env-org=0 type=3 2:02PM DBG Client closed connection id=40d76fc4-81e7-11f1-83be-18c04d5488aa org=1 user=1 2:02PM INF incoming_request bytes_in= bytes_out=7440 latency_ms=376873.626075 method=GET remote_ip=2600:8800:2b00:4e9:8c1b:d454:187b:9d47 status=200 url=/api/events 2:02PM INF DB notification context err error="context canceled" 2:02PM DBG Exiting listenForJobs 2:02PM DBG Exited job listener goroutine 2:02PM DBG Exited email websocket 2:02PM INF Shutdown complete 2:02PM INF Final cleanup 2:02PM ERR sync stderr error="sync /dev/stderr: invalid argument" panic: can't sync stderr goroutine 1 [running]: main.main() /home/eliribble/src/nidus-sync/main.go:283 +0x200a ``` I'd like you to research the various goroutine/concurrency patterns in the nidus-sync codebase. Try to group them by pattern with an eye toward any patterns that may be missing a canceleable context, leaking a goroutine, or failing to check the context when waiting for other signals. Any of these could contribute to slow shutdown. Additionally, I'm considering using structured concurrency from a library like https://github.com/sourcegraph/conc. I'd be interested in an assesment of the feasibility of refactoring to use conc as part of the study.

ned was assigned by eliribble 2026-07-17 14:06:51 +00:00

ned commented 2026-07-17 14:09:48 +00:00

Member

Copy link

Goroutine/Concurrency Pattern Analysis

I've done a thorough audit of every goroutine and concurrency pattern in nidus-sync. Here is a catalog grouped by pattern, followed by shutdown delay root cause analysis and a conc feasibility assessment.

---

Pattern Catalog

Pattern 1: Global WaitGroup-Managed Background Loops

Files: platform/start.go, platform/planet.go

Goroutine	WaitGroup	Checks ctx.Done()	Blocking I/O	Shutdown-Safe?
listenForJobs	✅ waitGroup	✅ At top of outer loop + in select	WaitForNotification (ctx-aware)	✅ Exits promptly after cancel
commsemail.StartWebsocket	✅ waitGroup	✅ In select before read	conn.ReadMessage() (NOT ctx-aware)	⚠️ BLOCKS
StartPlanetAuthCheck	✅ waitGroup	✅ via select on ctx.Done() with time.After	HTTP calls (ctx-aware via client)	✅

Problems:

• StartWebsocket: The select { case <-ctx.Done(): return; default: conn.ReadMessage() } pattern appears cancellable but is not. The default branch is chosen immediately and conn.ReadMessage() blocks indefinitely. After cancel() in main.go, the goroutine cannot be interrupted — it stays blocked on the websocket read until the server closes or a TCP timeout fires. This is the primary cause of the 20+ second shutdown delay.
• Fix: Set a read deadline via conn.SetReadDeadline(time.Now().Add(5*time.Second)) before each read, or launch a goroutine that calls conn.Close() when ctx is cancelled.

Pattern 2: Unmanaged Fire-and-Forget Background Goroutines

File: main.go

Goroutine	WaitGroup	Cancelable?	Shutdown Behavior
planet.StartSync (line 163)	❌ None	✅ Uses ctx	Completes on its own; harmless
Pprof debug server (line 209)	❌ None	❌ No ctx check	Leaked — runs until http.ListenAndServe returns an error (never)
HTTP listen-and-serve (lines 236/244)	❌ None	N/A (handled by server.Shutdown)	Fine — server.Shutdown kills the listener
addWaitingJobs goroutine (start.go:90)	❌ None	Uses parent ctx	If handleJob hangs on an external API, this goroutine leaks

Findings:

• The pprof server on localhost:6060 has no shutdown mechanism. It runs http.ListenAndServe which only returns on a fatal error. Low-risk but technically a leak.
• addWaitingJobs (start.go:61) spawns a goroutine that processes any pending database jobs found at startup. This goroutine is NOT tracked in the WaitGroup. If a job handler hangs (e.g., external API timeout), the goroutine continues past WaitForExit(). Jobs should either be tracked in the WaitGroup or moved into the listenForJobs notification system.

Pattern 3: SSE Event Dispatcher Goroutine

File: api/event.go:68

func SetEventChannel(chan_envelopes <-chan platform.Envelope) {
    go func() {
        for envelope := range chan_envelopes {
            for conn := range connectionsSSE { ... }
        }
    }()
}

Problems:

1. Unbuffered channel sends to SSE clients. The dispatcher pushes events to conn.chanEvent which is an unbuffered channel. If the SSE client has disconnected but the entry hasn't been removed from connectionsSSE yet, the dispatcher blocks forever on that send. The SSE handler cleans up via defer delete(connectionsSSE, &connection) when r.Context().Done() fires, but there's a race.
2. Shutdown race. The shutdown order is EventShutdown() → server.Shutdown() → cancel() → close(chan_envelope) → WaitForExit(). If chan_envelope is full (buffer=10) because the dispatcher is blocked on SSE clients, EventShutdown's goroutine blocks. The close(chan_envelope) then causes the dispatcher's range loop to exit, but if an SSE client channel isn't being drained, it blocks iterating. Worse: a sender goroutine racing with the close will panic (send on closed channel).

Pattern 4: Event System Fire-and-Forget

File: platform/event.go, platform/event/event.go

Every Created(), Updated(), UpdatedUser(), and Shutdown() call fires go Send(...). These push onto a buffered channel (buffer=10). If the channel fills up, every event-sending goroutine blocks forever. HTTP handler call sites with bursts of events could get stuck.

Pattern 5: HTTP Handler Fire-and-Forget

Files: api/twilio.go:137, api/voipms.go:98

go func() {
    err := text.HandleTextMessage(context.Background(), src, dst, body)
    if err != nil { log.Error().Err(err)... }
}()

These use context.Background() instead of r.Context(). Acceptable since webhook work should continue after the response, but no timeout/cancellation propagates from the request.

Pattern 6: ArcGIS Fieldseeker (Commented Out)

File: platform/arcgis.go

refreshFieldseekerData has a notable bug: it creates workerCtx from context.Background() instead of background_ctx. If re-enabled, nested worker goroutines wouldn't be cancelled when the parent cancels. Also, defer cancel() in the infinite loop only runs on function return (last iteration only). All currently dormant since the function is commented out in StartAll.

Pattern 7: Worker Pools

Files: platform/municipal.go, platform/csv/geocode.go

Both use the semaphore + WaitGroup pattern. Issues:

• municipal.go: Workers use context.Background() for DB inserts, ignoring parent context. On shutdown, workers keep inserting until their work is done.
• csv/geocode.go: Currently //nolint:unused, so dormant.

Pattern 8: Vision Auto-Scaler + Worker

Files: platform/vision/scaler/scaler.go, cmd/nidus-vision-worker/main.go

These are the cleanest patterns. Engine properly uses context.WithCancel, Stop() waits via wg.Wait(), and the loop checks ctx.Done(). The worker uses signal.Notify + cancel correctly.

---

Root Cause: Why Shutdown Takes 20+ Seconds

Smoking gun: Forward Email websocket (comms/email/websocket.go:72-77)

select {
case <-ctx.Done():
    return          // selected only when ctx is already done before the select
case default:
    message_type, message, err := conn.ReadMessage()  // BLOCKS here indefinitely
}

Even after cancel() fires in main.go, ReadMessage() does not unblock. The gorilla/websocket library provides no mechanism to interrupt a read via context. The read only returns when:

• The server sends a message or close frame
• A network-level timeout fires (potentially minutes)

Since Forward Email's server doesn't immediately close the websocket on our shutdown, this goroutine blocks for 20+ seconds until some lower-level timeout fires. During that time, platform.WaitForExit() calling waitGroup.Wait() is stuck.

Secondary contributor: The SSE dispatcher can also block sending to unbuffered SSE client channels, adding further delay.

Recommended fix (priority 1):

// Option A: Read deadline
conn.SetReadDeadline(time.Now().Add(5 * time.Second))
message_type, message, err := conn.ReadMessage()
if err != nil {
    if netErr, ok := err.(net.Error); ok && netErr.Timeout() {
        continue  // timeout = no message, retry (also checks ctx.Done() next iteration)
    }
    // real error handling
}

// Option B: Conn-close goroutine
go func() {
    <-ctx.Done()
    conn.Close()  // unblocks ReadMessage
}()

Secondary fix: Make conn.chanEvent buffered (size 1) in api/event.go so the dispatcher doesn't deadlock on slow/disconnected SSE clients.

---

Feasibility: Refactoring to github.com/sourcegraph/conc

Verdict: Feasible, but the bigger win is fixing the blocking-I/O problems first.

What conc would improve

1. WaitGroup ergonomics: conc.WaitGroup auto-captures panics and returns errors, replacing sync.WaitGroup + manual error handling.
2. Structured concurrency: conc.ContextPool ties goroutines to a context, making cancellation propagation automatic — fixing the patterns where goroutines ignore ctx.Done().
3. Panic safety: conc catches panics in child goroutines and re-panics on Wait() — the current codebase has go func() calls with no recovery at all.
4. Worker pools: conc.Pool with configurable max goroutines could replace the channel-based semaphore in platform/municipal.go.

What conc would NOT fix

1. Blocking I/O. conc can't solve conn.ReadMessage() ignoring context. That requires read deadlines or connection-level cancellation.
2. Channel-based event system. The SSE dispatcher, event channel, and per-client channels are channel-driven. conc doesn't help with channel selection races or unbuffered channel deadlocks.
3. HTTP handler fire-and-forget. Those are intentionally detached by design.

Migration difficulty by area

Area	conc fit	Effort	Value
platform/start.go background loops	✅ Perfect — conc.ContextPool	Low	High
platform/municipal.go worker pool	✅ Good — conc.PoolWithResults	Low	Medium
platform/arcgis.go workers (dormant)	⚠️ Good fit, but dormant	Medium	Low
platform/vision/scaler	😐 Already clean; minimal benefit	Medium	Low
api/event.go SSE dispatcher	❌ Not applicable	N/A	N/A
api/twilio.go/api/voipms.go	😐 Possible but overengineered	Low	Low
platform/event.go event sends	❌ Not applicable (channel send)	N/A	N/A
platform/csv/geocode.go (unused)	✅ Good, if re-enabled	Low	Low

Recommendation

1. Fix the websocket blocking I/O first — this is the actual cause of the 20-second delay and conc won't help with it.
2. Then consider conc — but start small. Migrate just the three WaitGroup goroutines in platform/start.go to conc.WaitGroup for panic safety and cleaner cancellation. Leave the channel-based event system alone.
3. Don't use conc.Pool or iterators in the first pass. Start with basic conc.WaitGroup and conc.ContextPool for the clearest pain points.

Rough migration example

// Current:
var waitGroup sync.WaitGroup

func StartAll(ctx context.Context) error {
    waitGroup.Add(1)
    go func() {
        defer waitGroup.Done()
        listenForJobs(ctx)
    }()
}

func WaitForExit() {
    waitGroup.Wait()
}

// With conc:
var wg conc.WaitGroup

func StartAll(ctx context.Context) error {
    wg.Go(func() {
        listenForJobs(ctx)  // panic caught + propagated on Wait()
    })
}

func WaitForExit() {
    wg.Wait()  // also re-panics if any goroutine panicked
    // NOTE: wg still blocks if goroutine is stuck in blocking I/O
}

---

Summary Table

Issue	Location	Severity	Fix
Non-cancellable websocket read	comms/email/websocket.go	🔥 Critical — 20s+ shutdown delay	Read deadline or conn-close goroutine
Unbuffered SSE channels blocking dispatcher	api/event.go	🔥 High — can stall shutdown	Buffer channel (1) or send timeout
addWaitingJobs untracked	platform/start.go:90	⚠️ Medium	Add to WaitGroup
Pprof server no shutdown	main.go:209	⚠️ Low	Add ctx-aware shutdown (or accept as leak)
go Send() can block on full channel	platform/event/event.go	⚠️ Medium	Select with default or larger buffer
refreshFieldseekerData uses Background ctx	platform/arcgis.go:79	Info (dormant)	Use parent ctx when re-enabling
Municipal workers use Background ctx for DB	platform/municipal.go:1242	⚠️ Medium	Pass parent ctx

## Goroutine/Concurrency Pattern Analysis I've done a thorough audit of every goroutine and concurrency pattern in nidus-sync. Here is a catalog grouped by pattern, followed by shutdown delay root cause analysis and a `conc` feasibility assessment. --- ## Pattern Catalog ### Pattern 1: Global WaitGroup-Managed Background Loops **Files:** `platform/start.go`, `platform/planet.go` | Goroutine | WaitGroup | Checks ctx.Done() | Blocking I/O | Shutdown-Safe? | |-----------|-----------|-------------------|-------------|----------------| | `listenForJobs` | ✅ `waitGroup` | ✅ At top of outer loop + in select | `WaitForNotification` (ctx-aware) | ✅ Exits promptly after cancel | | `commsemail.StartWebsocket` | ✅ `waitGroup` | ✅ In select before read | `conn.ReadMessage()` (NOT ctx-aware) | **⚠️ BLOCKS** | | `StartPlanetAuthCheck` | ✅ `waitGroup` | ✅ via `select` on `ctx.Done()` with `time.After` | HTTP calls (ctx-aware via client) | ✅ | **Problems:** - **StartWebsocket**: The `select { case <-ctx.Done(): return; default: conn.ReadMessage() }` pattern *appears* cancellable but is not. The `default` branch is chosen immediately and `conn.ReadMessage()` blocks indefinitely. After `cancel()` in main.go, the goroutine cannot be interrupted — it stays blocked on the websocket read until the server closes or a TCP timeout fires. This is the **primary cause of the 20+ second shutdown delay**. - Fix: Set a read deadline via `conn.SetReadDeadline(time.Now().Add(5*time.Second))` before each read, or launch a goroutine that calls `conn.Close()` when ctx is cancelled. ### Pattern 2: Unmanaged Fire-and-Forget Background Goroutines **File:** `main.go` | Goroutine | WaitGroup | Cancelable? | Shutdown Behavior | |-----------|-----------|-------------|-------------------| | `planet.StartSync` (line 163) | ❌ None | ✅ Uses ctx | Completes on its own; harmless | | Pprof debug server (line 209) | ❌ None | ❌ No ctx check | **Leaked** — runs until `http.ListenAndServe` returns an error (never) | | HTTP listen-and-serve (lines 236/244) | ❌ None | N/A (handled by `server.Shutdown`) | Fine — `server.Shutdown` kills the listener | | `addWaitingJobs` goroutine (start.go:90) | ❌ None | Uses parent ctx | If `handleJob` hangs on an external API, this goroutine leaks | **Findings:** - The pprof server on localhost:6060 has no shutdown mechanism. It runs `http.ListenAndServe` which only returns on a fatal error. Low-risk but technically a leak. - `addWaitingJobs` (start.go:61) spawns a goroutine that processes any pending database jobs found at startup. This goroutine is NOT tracked in the WaitGroup. If a job handler hangs (e.g., external API timeout), the goroutine continues past `WaitForExit()`. Jobs should either be tracked in the WaitGroup or moved into the `listenForJobs` notification system. ### Pattern 3: SSE Event Dispatcher Goroutine **File:** `api/event.go:68` ```go func SetEventChannel(chan_envelopes <-chan platform.Envelope) { go func() { for envelope := range chan_envelopes { for conn := range connectionsSSE { ... } } }() } ``` **Problems:** 1. **Unbuffered channel sends to SSE clients.** The dispatcher pushes events to `conn.chanEvent` which is an **unbuffered** channel. If the SSE client has disconnected but the entry hasn't been removed from `connectionsSSE` yet, the dispatcher blocks forever on that send. The SSE handler cleans up via `defer delete(connectionsSSE, &connection)` when `r.Context().Done()` fires, but there's a race. 2. **Shutdown race.** The shutdown order is `EventShutdown()` → `server.Shutdown()` → `cancel()` → `close(chan_envelope)` → `WaitForExit()`. If `chan_envelope` is full (buffer=10) because the dispatcher is blocked on SSE clients, `EventShutdown`'s goroutine blocks. The `close(chan_envelope)` then causes the dispatcher's range loop to exit, but if an SSE client channel isn't being drained, it blocks iterating. Worse: a sender goroutine racing with the close will panic (`send on closed channel`). ### Pattern 4: Event System Fire-and-Forget **File:** `platform/event.go`, `platform/event/event.go` Every `Created()`, `Updated()`, `UpdatedUser()`, and `Shutdown()` call fires `go Send(...)`. These push onto a buffered channel (buffer=10). If the channel fills up, every event-sending goroutine blocks forever. HTTP handler call sites with bursts of events could get stuck. ### Pattern 5: HTTP Handler Fire-and-Forget **Files:** `api/twilio.go:137`, `api/voipms.go:98` ```go go func() { err := text.HandleTextMessage(context.Background(), src, dst, body) if err != nil { log.Error().Err(err)... } }() ``` These use `context.Background()` instead of `r.Context()`. Acceptable since webhook work should continue after the response, but no timeout/cancellation propagates from the request. ### Pattern 6: ArcGIS Fieldseeker (Commented Out) **File:** `platform/arcgis.go` `refreshFieldseekerData` has a notable bug: it creates `workerCtx` from `context.Background()` instead of `background_ctx`. If re-enabled, nested worker goroutines wouldn't be cancelled when the parent cancels. Also, `defer cancel()` in the infinite loop only runs on function return (last iteration only). All currently dormant since the function is commented out in `StartAll`. ### Pattern 7: Worker Pools **Files:** `platform/municipal.go`, `platform/csv/geocode.go` Both use the semaphore + WaitGroup pattern. Issues: - **municipal.go**: Workers use `context.Background()` for DB inserts, ignoring parent context. On shutdown, workers keep inserting until their work is done. - **csv/geocode.go**: Currently `//nolint:unused`, so dormant. ### Pattern 8: Vision Auto-Scaler + Worker **Files:** `platform/vision/scaler/scaler.go`, `cmd/nidus-vision-worker/main.go` These are the **cleanest** patterns. `Engine` properly uses `context.WithCancel`, `Stop()` waits via `wg.Wait()`, and the loop checks `ctx.Done()`. The worker uses `signal.Notify` + cancel correctly. --- ## Root Cause: Why Shutdown Takes 20+ Seconds **Smoking gun: Forward Email websocket** (`comms/email/websocket.go:72-77`) ```go select { case <-ctx.Done(): return // selected only when ctx is already done before the select case default: message_type, message, err := conn.ReadMessage() // BLOCKS here indefinitely } ``` Even after `cancel()` fires in main.go, `ReadMessage()` does not unblock. The gorilla/websocket library provides no mechanism to interrupt a read via context. The read only returns when: - The server sends a message or close frame - A network-level timeout fires (potentially minutes) Since Forward Email's server doesn't immediately close the websocket on our shutdown, this goroutine blocks for 20+ seconds until some lower-level timeout fires. During that time, `platform.WaitForExit()` calling `waitGroup.Wait()` is stuck. **Secondary contributor:** The SSE dispatcher can also block sending to unbuffered SSE client channels, adding further delay. **Recommended fix (priority 1):** ```go // Option A: Read deadline conn.SetReadDeadline(time.Now().Add(5 * time.Second)) message_type, message, err := conn.ReadMessage() if err != nil { if netErr, ok := err.(net.Error); ok && netErr.Timeout() { continue // timeout = no message, retry (also checks ctx.Done() next iteration) } // real error handling } // Option B: Conn-close goroutine go func() { <-ctx.Done() conn.Close() // unblocks ReadMessage }() ``` **Secondary fix:** Make `conn.chanEvent` buffered (size 1) in `api/event.go` so the dispatcher doesn't deadlock on slow/disconnected SSE clients. --- ## Feasibility: Refactoring to `github.com/sourcegraph/conc` **Verdict: Feasible, but the bigger win is fixing the blocking-I/O problems first.** ### What `conc` would improve 1. **WaitGroup ergonomics:** `conc.WaitGroup` auto-captures panics and returns errors, replacing `sync.WaitGroup` + manual error handling. 2. **Structured concurrency:** `conc.ContextPool` ties goroutines to a context, making cancellation propagation automatic — fixing the patterns where goroutines ignore `ctx.Done()`. 3. **Panic safety:** `conc` catches panics in child goroutines and re-panics on `Wait()` — the current codebase has `go func()` calls with no recovery at all. 4. **Worker pools:** `conc.Pool` with configurable max goroutines could replace the channel-based semaphore in `platform/municipal.go`. ### What `conc` would NOT fix 1. **Blocking I/O.** `conc` can't solve `conn.ReadMessage()` ignoring context. That requires read deadlines or connection-level cancellation. 2. **Channel-based event system.** The SSE dispatcher, event channel, and per-client channels are channel-driven. `conc` doesn't help with channel selection races or unbuffered channel deadlocks. 3. **HTTP handler fire-and-forget.** Those are intentionally detached by design. ### Migration difficulty by area | Area | `conc` fit | Effort | Value | |------|-----------|--------|-------| | `platform/start.go` background loops | ✅ Perfect — `conc.ContextPool` | Low | High | | `platform/municipal.go` worker pool | ✅ Good — `conc.PoolWithResults` | Low | Medium | | `platform/arcgis.go` workers (dormant) | ⚠️ Good fit, but dormant | Medium | Low | | `platform/vision/scaler` | 😐 Already clean; minimal benefit | Medium | Low | | `api/event.go` SSE dispatcher | ❌ Not applicable | N/A | N/A | | `api/twilio.go`/`api/voipms.go` | 😐 Possible but overengineered | Low | Low | | `platform/event.go` event sends | ❌ Not applicable (channel send) | N/A | N/A | | `platform/csv/geocode.go` (unused) | ✅ Good, if re-enabled | Low | Low | ### Recommendation 1. **Fix the websocket blocking I/O first** — this is the actual cause of the 20-second delay and `conc` won't help with it. 2. **Then consider `conc`** — but start small. Migrate just the three WaitGroup goroutines in `platform/start.go` to `conc.WaitGroup` for panic safety and cleaner cancellation. Leave the channel-based event system alone. 3. **Don't use `conc.Pool` or iterators** in the first pass. Start with basic `conc.WaitGroup` and `conc.ContextPool` for the clearest pain points. ### Rough migration example ```go // Current: var waitGroup sync.WaitGroup func StartAll(ctx context.Context) error { waitGroup.Add(1) go func() { defer waitGroup.Done() listenForJobs(ctx) }() } func WaitForExit() { waitGroup.Wait() } // With conc: var wg conc.WaitGroup func StartAll(ctx context.Context) error { wg.Go(func() { listenForJobs(ctx) // panic caught + propagated on Wait() }) } func WaitForExit() { wg.Wait() // also re-panics if any goroutine panicked // NOTE: wg still blocks if goroutine is stuck in blocking I/O } ``` --- ## Summary Table | Issue | Location | Severity | Fix | |-------|----------|----------|-----| | Non-cancellable websocket read | `comms/email/websocket.go` | **🔥 Critical** — 20s+ shutdown delay | Read deadline or conn-close goroutine | | Unbuffered SSE channels blocking dispatcher | `api/event.go` | **🔥 High** — can stall shutdown | Buffer channel (1) or send timeout | | `addWaitingJobs` untracked | `platform/start.go:90` | ⚠️ Medium | Add to WaitGroup | | Pprof server no shutdown | `main.go:209` | ⚠️ Low | Add ctx-aware shutdown (or accept as leak) | | `go Send()` can block on full channel | `platform/event/event.go` | ⚠️ Medium | Select with default or larger buffer | | `refreshFieldseekerData` uses Background ctx | `platform/arcgis.go:79` | Info (dormant) | Use parent ctx when re-enabling | | Municipal workers use Background ctx for DB | `platform/municipal.go:1242` | ⚠️ Medium | Pass parent ctx |

eliribble commented 2026-07-17 14:25:15 +00:00

Author

Owner

Copy link

Excellent writeup, thanks for that!

Would you please create a PR for addressing the Critical, high, and medium severity issues from your summary table?

For the comms/email/websocket.go issue let's go with launch a goroutine that calls conn.Close() when ctx is cancelled. That seems like it is a bit more complex, but will provide near-immediate shutdown capability, which is very valuable.

For the SSE channel going to a buffer channel (1) should be fine.

Adding a waitgroup for platform/start.go seems pretty straightforward as a fix to me. Your other suggested medium severity fixes seem fine too.

Please make a commit per fix area for ease of reviewing. I'd appreciate a detailed explanation of each fix with the commit as well so that I can go over the patterns with the engineering team to avoid producing more of these issues in the future.

Excellent writeup, thanks for that! Would you please create a PR for addressing the Critical, high, and medium severity issues from your summary table? For the `comms/email/websocket.go` issue let's go with launch a goroutine that calls conn.Close() when ctx is cancelled. That seems like it is a bit more complex, but will provide near-immediate shutdown capability, which is very valuable. For the SSE channel going to a buffer channel (1) should be fine. Adding a waitgroup for `platform/start.go` seems pretty straightforward as a fix to me. Your other suggested medium severity fixes seem fine too. Please make a commit per fix area for ease of reviewing. I'd appreciate a detailed explanation of each fix with the commit as well so that I can go over the patterns with the engineering team to avoid producing more of these issues in the future.

ned referenced this issue 2026-07-17 14:30:28 +00:00

fix: address critical, high, and medium concurrency shutdown issues #131

ned commented 2026-07-17 14:30:33 +00:00

Member

Copy link

PR is up: #131

Five commits as requested:

1. Websocket cancellable — goroutine calls conn.Close() on ctx cancellation to unblock ReadMessage()
2. SSE channel buffered — per-client event channel now has capacity 1
3. addWaitingJobs tracked — goroutine now registered in the global WaitGroup
4. Event Send non-blocking — select with default prevents goroutine leaks when the event channel fills up
5. Situs insert uses parent ctx — worker goroutines now pass the cancellable context instead of context.Background()

Each commit has a detailed explanation of the fix. I left a note in the PR about the pre-existing go-planet submodule issue (commit no longer on remote) which causes a build failure in platform/planet_tile.go unrelated to these changes.

PR is up: https://source.gleipnir.technology/Gleipnir/nidus-sync/pulls/131 Five commits as requested: 1. **Websocket cancellable** — goroutine calls `conn.Close()` on ctx cancellation to unblock `ReadMessage()` 2. **SSE channel buffered** — per-client event channel now has capacity 1 3. **addWaitingJobs tracked** — goroutine now registered in the global WaitGroup 4. **Event Send non-blocking** — `select` with `default` prevents goroutine leaks when the event channel fills up 5. **Situs insert uses parent ctx** — worker goroutines now pass the cancellable context instead of `context.Background()` Each commit has a detailed explanation of the fix. I left a note in the PR about the pre-existing go-planet submodule issue (commit no longer on remote) which causes a build failure in `platform/planet_tile.go` unrelated to these changes.

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

fix/signin-stale-impersonation

issue-164-overview-map

worried-pug

add-ts-ui-tests-action

implement-maintenance-mode

investigate-planet-mosaics-new-area-sync

upbeat-turtle-2

quiet-peacock

light-rat-2

light-rat

upbeat-turtle

reveal-ui-bundle

smart-baboon

snowy-gopher

bitter-moth

famous-mantis

evil-donkey-continued

elegant-mule

exciting-sheep

sleek-fireant

magical-armadillo

aquatic-monkey

issue-176-n-items

issue-176-reveal-ui-redesign

issue-184-nearmap-tiles

issue-183-crop-data

issue-13-train-data-api

issue-180-skip-trace-design-doc

issue-174-reveal-ui-mockup

issue-166-communication-search

issue-165-address-reporter-list-view

issue-160-h3cell-bad-data

issue-155-feature-observation-parser-v2

issue-155-feature-observation-parser

issue-152-land-use-soil-data

fix/issue-134-ci-ssh-auth

fix/golint-ci-submodule-auth

fix/golangci-lint-unstable

fix/incoming-text-missing-destination-phone

fix/csv-import-missing-srid-4326

79

78

77

76

75

74

73

72

70

50

48

46

45

_backup/pre-rewrite-tag44

44

42

41

40

39

38

37

35

34

33

v0.0.5

v0.0.4

32

31

30

29

28

27

26

25

24

v0.0.23

v0.0.22

v0.0.21

v0.0.20

v0.0.19

0.0.18

0.0.17

0.0.16

0.0.15

0.0.14

0.0.13

0.0.12

0.0.11

0.0.10

0.0.9

0.0.8

0.0.7

v0.0.6

v0.0.3

v0.0.2

v0.0.1

Labels

Clear labels

  

customer

Customers are aware of, and affected by the issue

  

prod

Issue has manifested in production

  

repro'd

Has been reproduced by engineering

  

retest

Issue is ready for retesting in staging

  

staging

Issue has manifested in staging

No labels

customer

prod

repro'd

retest

staging

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

ned

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Gleipnir/nidus-sync#130

No description provided.