Skip to content

fix: join detached cache work before embedded storage closes - #256

Open
perfloop[bot] wants to merge 3 commits into
mainfrom
perfloop-pr-open-rvg0nzpqgf
Open

perfloop[bot] wants to merge 3 commits into
mainfrom
perfloop-pr-open-rvg0nzpqgf

Conversation

@perfloop

@perfloop perfloop Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

proxy.Service.Stop now seals detached-work admission, cancels the Service lifetime, and joins admitted cache users after HTTP handlers drain but before main closes embedded storage. The Service lifetime is independent of an individual request, so a cache warm can continue after a client disconnect; shutdown cancels and joins it. Full-object writes have their own registered owner, so a fetch-supervisor timeout cannot hide a still-running PutStream or release its reservation early. An admitted tiered cleanup retains its timeout through the conditional S3 delete and marker repair. Shutdown still joins it because cancellation after dispatch can leave the remote result ambiguous.

TestShutdownWaitsForBackgroundWriterBeforeEmbeddedClose runs TAG as a child process with embedded storage and a controlled local S3 origin. It holds an admitted writer through SIGTERM and records admission, cancellation, writer exit, and backend close. A retained result-mapping driver also runs the cold-range process after the fetch supervisor times out and the active-fetch count returns to zero, plus a separate active-writer control. TestShutdownDrainsNormalRequestWithinGrace checks an ordinary request completing within the existing HTTP grace. TestStopWaitsForWriterAfterFetchSupervisorTimesOut observes the reservation until the writer exits, and TestTieredCleanupFinishesMarkerRepairDuringServiceStop covers the joined conditional-delete path. The existing TestColdOwner_RangeWarmsCacheOnClientCancel integration test remains in the suite.

The same process-and-reservation assertion is violated on test-only comparison 84ae52583b59e4299e6bf23bb354c59be853f5c3 and satisfied on the final source.

make build, make test, make test-integration, go test -race -count=1 ./proxy, and make lint-ci passed. The process and result-mapping drivers are retained verification support, not part of this PR.

No production shutdown incidence or overall shutdown duration was measured. The HTTP drain keeps its 30-second deadline; the worker join has none, and an uncooperative cache operation can outlast the bundled StatefulSet's 30-second termination grace. No fixed total completion time is promised. This ordering applies after successful HTTP draining, not to handlers that survive its deadline. See graceful shutdown.


Generated by Perfloop.


Note

Medium Risk
Changes shutdown ordering and how long-running cache goroutines are canceled and joined; incorrect behavior could prolong shutdown or close RocksDB while writers still run, but behavior is heavily tested and scoped to lifecycle boundaries.

Overview
Graceful shutdown now drains HTTP first, then calls proxy.Service.Stop() to cancel service-scoped work and block until admitted background cache tasks finish—before embedded storage closes in main.

A new serviceWorkers lifecycle (startBackgroundTask / beginBackgroundTask) replaces ad-hoc goroutines across populate, block fetch, parquet prefetch, tiered cleanup, and upstream streaming. Detached work uses a service lifetime context (not only the request), so shutdown can cancel and join it. Populate and block-fetch ownership is tightened: cache slots and byte budget stay held until the real PutStream/PutBlock exits; abandoned pipe readers are closed when writers return; full-object background writes register a separate supervised task so supervisor timeout cannot release reservations early. Tiered cleanup keeps its own timeout through conditional DELETE and marker repair while still being joined on stop.

Docs add graceful-shutdown notes in README and docs/deploy.md. Tests include subprocess shutdown ordering probes, integration Service.Stop() in teardown, and unit tests for stop vs. held writers and tiered repair during stop.

Reviewed by Cursor Bugbot for commit af7e64b. Bugbot is set up for automated code reviews on this repo. Configure here.

Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
@greptile-apps

greptile-apps Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[High impact] The PR appears safe to merge; the previously reported shutdown hang is fixed.

Summary

TAG now joins admitted background cache work before closing embedded storage after HTTP requests drain.

  • Service shutdown waits for detached cache work before storage closes.
  • Cache writers keep their reservations until the writes actually end.
  • Shutdown waits for tiered cleanup's delete and marker repair.

Reviews (2) · Last reviewed commit: "fix: unblock canceled cache relay writer..." · Reviewed by Greptile

Comment thread proxy/caching.go
// Use a detached context for cache writes to avoid cancellation when HTTP request completes.
// Scale timeout by content length so large objects have enough time to write.
cacheCtx, cacheCancel := context.WithTimeout(context.Background(), cacheWriteTimeoutForSize(meta.ContentLength))
cacheCtx, cacheCancel := context.WithTimeout(s.workers.ctx, cacheWriteTimeoutForSize(meta.ContentLength))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Shutdown hangs on abandoned pipe

When a full GET has finished but its whole-body populate still has queued bytes for a remote cache owner, Stop now cancels cacheCtx. ocache’s remote write can return on cancellation without draining sigReader. The relay then stays blocked in pipeWriter.Write, while the outer task waits for pipeErrCh before closing the pipe. Stop never returns even though the cache call has returned.

Close pipeReader when the cache writer exits, or drain it on error, and cover this full-GET shutdown path in a test.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Response to this feedback.

This issue is valid. If the remote cache write returns on cancellation before consuming the pipe, the relay can remain blocked in its write while the task waits for that relay, preventing Stop from returning. I will close the pipe reader when the cache writer exits and add coverage for full-GET shutdown with queued data.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in af7e64bf56a6f30b15c6700ed9475e85d1998b63. Response to this feedback.

The cache writer now closes its pipe reader when the cache operation returns, passing the cache error through to the relay. That releases the relay when a canceled remote write stops consuming queued data. I added a full-GET test that leaves bytes unread, cancels the Service, and verifies Stop joins without test-side draining.

Signed-off-by: Perfloop Agent <agent@perfloop.ai>
@perfloop

perfloop Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

On SIGINT or SIGTERM, a cache-enabled TAG process now seals detached-work admission, cancels the Service lifetime, and joins admitted cache users after HTTP handlers drain but before main closes embedded storage. The Service lifetime stays independent of an individual request, so cache warming can continue after a client disconnect; shutdown still cancels and joins it. For foreground full GETs, the cache writer closes its pipe reader on return and passes its error through to the relay. This lets a relay blocked on queued bytes finish when a canceled remote write stops reading. Range-triggered full-object writes retain their own registered owner after fetch-supervisor timeout, keeping their slot and byte reservation until the actual writer exits. Tiered cleanup keeps its timeout through conditional S3 delete and marker repair; Stop still joins it because cancellation after dispatch can leave the remote outcome ambiguous.

TestShutdownWaitsForBackgroundWriterBeforeEmbeddedClose runs TAG as a child process with embedded storage and a controlled local S3 origin. It holds a writer through SIGTERM and records admission, cancellation, writer exit, and backend close. TestServiceStopUnblocksFullGetWhenCacheWriterStopsReading uses a controlled cache client that reads one byte and returns on service cancellation; it verifies the completed 200 response is preserved and Stop joins without a test-side pipe drain. TestShutdownDrainsNormalRequestWithinGrace checks the ordinary drain. TestStopWaitsForWriterAfterFetchSupervisorTimesOut checks that new detached work is rejected while Stop waits on a held writer and that its reservation remains held until exit. TestTieredCleanupFinishesMarkerRepairDuringServiceStop covers the conditional-delete path. The existing client-cancel warming integration test remains. Process and result-mapping drivers are retained verification support, not part of this PR.

The same process, admission, and reservation assertion is violated on test-only comparison 84ae52583b59e4299e6bf23bb354c59be853f5c3 and satisfied on the final source.

The repository checks make build, make test, make test-integration, go test -race -count=1 ./proxy, and make lint-ci passed.

No production shutdown incidence or overall shutdown duration was measured. HTTP draining keeps its existing 30-second deadline; the worker join has no deadline and an uncooperative cache operation can outlast the bundled StatefulSet's 30-second grace. No fixed total completion time is promised. This ordering applies after successful HTTP draining, not to handlers that survive the deadline. See graceful shutdown.


Generated by Perfloop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant