sneak/pixa
1
0
Fork 0
Code Issues 51 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Eviction loop is not context-cancellable; shutdown ignores its deadline #102

New Issue
Open
opened 2026-08-09 15:37:46 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 15:37:46 +02:00
Collaborator
Copy Link

Surfaced by the contextcheck linter while bringing #55's code under the canonical .golangci.yml in PR #54. Filing rather than silencing: it is a real design choice, not a style nit.

What the linter reports

internal/handlers/handlers.go:55:29: Function `initImageService->StartEviction->evictionLoop`
should pass the context parameter (contextcheck)

What is actually going on

internal/handlers/handlers.go registers the eviction lifecycle on fx:

OnStart: func(_ context.Context) error {
    return s.initImageService()
},
OnStop: func(_ context.Context) error {
    if s.imgCache != nil {
        s.imgCache.StopEviction()
    }

    return nil
},

Both hooks discard the context.Context fx hands them. Cache.evictionLoop (internal/imgcache/eviction.go) then makes its own:

func (c *Cache) evictionLoop(interval time.Duration) {
    defer close(c.evictionDone)

    ctx := context.Background()
    ...
}

Every database call in the eviction and reconciliation passes descends from that context.Background(), so nothing in the loop is cancellable. Shutdown relies entirely on the evictionStop channel:

func (c *Cache) StopEviction() {
    if !c.evictionStarted {
        return
    }

    c.evictionStopOnce.Do(func() {
        close(c.evictionStop)
        <-c.evictionDone
    })
}

StopEviction blocks on <-c.evictionDone, and the loop only observes evictionStop between passes. So an eviction or reconciliation pass that is already in flight when OnStop fires runs to completion, however long that takes, and the shutdown deadline fx passes to OnStop has no effect on it. A reconciliation pass walks the whole variant and source cache directory tree, so on a large cache this is not a negligible amount of work to be unable to interrupt.

Discarding the OnStart context is correct and must stay that way: the loop has to outlive OnStart, so it cannot inherit that context. The gap is that nothing replaces it.

Suggested fix

Give Cache its own cancellable context for the eviction goroutine:

  • StartEviction derives ctx, cancel := context.WithCancel(context.Background()) and stores cancel on the Cache.
  • evictionLoop uses that ctx instead of making its own, and selects on ctx.Done() alongside evictionStop.
  • StopEviction calls cancel() before closing evictionStop, so an in-flight pass unwinds via context cancellation instead of running to completion. The <-c.evictionDone wait then returns promptly.

Worth deciding as part of this: whether StopEviction should take a context.Context so OnStop can pass fx's shutdown deadline straight through, and whether an interrupted eviction pass needs anything beyond the transaction rollback it already gets (it should not: evictSourceBlob deletes rows in one transaction before unlinking, so a cancelled pass leaves accounting consistent and the next reconciliation pass picks up any orphaned file).

Why not in PR #54

That PR replaces the linter config and fixes the findings mechanically, explicitly without behavior changes. This one changes shutdown semantics of concurrency-sensitive code that had just passed adversarial review on #55, so it belongs in its own change with its own tests. PR #54 carries a //nolint:contextcheck on the OnStart hook that points at this issue.

Surfaced by the `contextcheck` linter while bringing #55's code under the canonical `.golangci.yml` in PR #54. Filing rather than silencing: it is a real design choice, not a style nit. ## What the linter reports ``` internal/handlers/handlers.go:55:29: Function `initImageService->StartEviction->evictionLoop` should pass the context parameter (contextcheck) ``` ## What is actually going on `internal/handlers/handlers.go` registers the eviction lifecycle on fx: ```go OnStart: func(_ context.Context) error { return s.initImageService() }, OnStop: func(_ context.Context) error { if s.imgCache != nil { s.imgCache.StopEviction() } return nil }, ``` Both hooks discard the `context.Context` fx hands them. `Cache.evictionLoop` (`internal/imgcache/eviction.go`) then makes its own: ```go func (c *Cache) evictionLoop(interval time.Duration) { defer close(c.evictionDone) ctx := context.Background() ... } ``` Every database call in the eviction and reconciliation passes descends from that `context.Background()`, so nothing in the loop is cancellable. Shutdown relies entirely on the `evictionStop` channel: ```go func (c *Cache) StopEviction() { if !c.evictionStarted { return } c.evictionStopOnce.Do(func() { close(c.evictionStop) <-c.evictionDone }) } ``` `StopEviction` blocks on `<-c.evictionDone`, and the loop only observes `evictionStop` between passes. So an eviction or reconciliation pass that is already in flight when `OnStop` fires **runs to completion**, however long that takes, and the shutdown deadline fx passes to `OnStop` has no effect on it. A reconciliation pass walks the whole variant and source cache directory tree, so on a large cache this is not a negligible amount of work to be unable to interrupt. Discarding the `OnStart` context is correct and must stay that way: the loop has to outlive `OnStart`, so it cannot inherit that context. The gap is that nothing replaces it. ## Suggested fix Give `Cache` its own cancellable context for the eviction goroutine: - `StartEviction` derives `ctx, cancel := context.WithCancel(context.Background())` and stores `cancel` on the `Cache`. - `evictionLoop` uses that `ctx` instead of making its own, and selects on `ctx.Done()` alongside `evictionStop`. - `StopEviction` calls `cancel()` before closing `evictionStop`, so an in-flight pass unwinds via context cancellation instead of running to completion. The `<-c.evictionDone` wait then returns promptly. Worth deciding as part of this: whether `StopEviction` should take a `context.Context` so `OnStop` can pass fx's shutdown deadline straight through, and whether an interrupted eviction pass needs anything beyond the transaction rollback it already gets (it should not: `evictSourceBlob` deletes rows in one transaction before unlinking, so a cancelled pass leaves accounting consistent and the next reconciliation pass picks up any orphaned file). ## Why not in PR #54 That PR replaces the linter config and fixes the findings mechanically, explicitly without behavior changes. This one changes shutdown semantics of concurrency-sensitive code that had just passed adversarial review on #55, so it belongs in its own change with its own tests. PR #54 carries a `//nolint:contextcheck` on the `OnStart` hook that points at this issue.
clawbot added this to the 1.0.0 milestone 2026-08-09 15:37:46 +02:00
clawbot commented 2026-08-09 16:18:28 +02:00
Author
Collaborator
Copy Link

Correction to this issue's body, which is now out of date in one respect.

The "Why not in PR #54" section says that PR replaces the linter config and fixes findings "explicitly without behavior changes". That framing was accurate when this issue was filed, but PR #54's round-4 review established that the PR is not a pure no-op, and its description has since been rewritten to disclose three behavior deltas:

  1. Cache.StoreVariant now takes a context.Context and uses ExecContext — on a cancelled request the accounting row is skipped where main committed it.
  2. MetadataStorage.Store no longer leaks .tmp-*.json files — its cleanup defer was dead on main (unnamed result parameter, so the closure read an outer err left nil by the successful os.CreateTemp, while all three failure paths shadowed it). A genuine latent bug fix.
  3. The signing_key validation error text gained value too short: .

The reasoning for deferring this issue is unaffected, and if anything is stronger now. The argument was never that PR #54 contains literally zero behavior change — it is that changing the shutdown semantics of concurrency-sensitive code does not belong in a lint-conformance pass. The three deltas above are each local and individually reviewable; making the eviction loop cancellable is a design change to code that had just cleared adversarial review on #55, and it deserves its own change with its own tests.

Leaving the body as-is rather than rewriting it, so the record of what was believed when stays intact — but treat this comment as the correction.

Correction to this issue's body, which is now out of date in one respect. The "Why not in PR #54" section says that PR replaces the linter config and fixes findings "explicitly without behavior changes". That framing was accurate when this issue was filed, but PR #54's round-4 review established that the PR is **not** a pure no-op, and its description has since been rewritten to disclose three behavior deltas: 1. `Cache.StoreVariant` now takes a `context.Context` and uses `ExecContext` — on a cancelled request the accounting row is skipped where `main` committed it. 2. `MetadataStorage.Store` no longer leaks `.tmp-*.json` files — its cleanup defer was dead on `main` (unnamed result parameter, so the closure read an outer `err` left nil by the successful `os.CreateTemp`, while all three failure paths shadowed it). A genuine latent bug fix. 3. The `signing_key` validation error text gained `value too short: `. **The reasoning for deferring this issue is unaffected**, and if anything is stronger now. The argument was never that PR #54 contains literally zero behavior change — it is that changing the **shutdown semantics of concurrency-sensitive code** does not belong in a lint-conformance pass. The three deltas above are each local and individually reviewable; making the eviction loop cancellable is a design change to code that had just cleared adversarial review on #55, and it deserves its own change with its own tests. Leaving the body as-is rather than rewriting it, so the record of what was believed when stays intact — but treat this comment as the correction.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#102
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?