Repository navigation
Keep the dagql cache when a stop outlives the graceful-stop budget - #15052
Open
goingforstudying-ctrl wants to merge 2 commits into
Open
goingforstudying-ctrl wants to merge 2 commits into
goingforstudying-ctrl wants to merge 2 commits into
Conversation
The final persist in CloseWithShutdownError ran on the caller's stop budget, so an engine whose save outlived the budget, or whose budget was already spent by earlier shutdown phases, had its mirror transaction cancelled, never wrote clean_shutdown=1, and the next start wiped the whole store. A slow stop cost hours of cache. The wait for quiescence stays on the caller's budget, since in-flight calls are client behavior, but the save of the quiesced state and the clean-shutdown marker now run on a detached 15-minute context: their duration scales with the store's size, not with the stop budget. Signed-off-by: goingforstudying-ctrl <goingforstudying@gmail.com>
Signed-off-by: goingforstudying-ctrl <goingforstudying@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Was digging through #14956 (the measured one — engines g/q/d losing their whole cache on slow stops) and the close path turned out to be the culprit.
What's happening:
CloseWithShutdownErrorruns the finalpersistCurrentStateon the caller's stop-budget context. Once the budget expires — mid-persist for engine g, or long before the close phase even starts if earlier phases were slow — the mirror transaction dies withcontext deadline exceeded,clean_shutdown=1never gets written, and the next boot wipes the whole store. Since the persist is one SQLite transaction over already-quiesced state, cutting it doesn't even buy safety: the store rolls back to its previous consistent save, but the marker still says dirty, so the next start throws all of it away.The fix splits the close by what each half waits on. Waiting for quiescence stays on the caller's budget — in-flight calls are client behavior, and a stuck call shouldn't hold shutdown hostage. The save itself is deterministic local I/O whose duration scales with the store's size, not with the stop budget, so it now runs on a detached context (
context.WithoutCancel+ a 15-minuteclosePersistTimeout), and theclean_shutdown=1marker write moved onto that same context — previously a budget that expired during the persist would cancel the marker write too, wiping the store after a successful save.Verified with a regression test that closes with an already-cancelled context: before the fix the reopen reports
unclean_shutdownand the store is empty, after it the store survives with its contents. Ran the./dagqlpersistence/close/checkpoint suites (39 tests) plus a race-detector pass on the close tests — all green.Scope honesty: this doesn't bound
wait for garbage collection(#15017), and a SIGKILL before the close phase still loses the save — those need the phase-bounding work. It also doesn't do incremental checkpointing, which is the bigger redesign from the issue's asks. It just closes the hole where the close phase was reached and the budget cut the save, which is the failure the issue measured on engine g.