Visitar URL original
Keep the dagql cache when a stop outlives the graceful-stop budget by goingforstudying-ctrl · Pull Request #15052 · dagger/dagger · GitHub
Skip to content

Keep the dagql cache when a stop outlives the graceful-stop budget - #15052

Open
goingforstudying-ctrl wants to merge 2 commits into
dagger:mainfrom
goingforstudying-ctrl:dagql-close-persist-past-stop-budget
Open

goingforstudying-ctrl wants to merge 2 commits into
dagger:mainfrom
goingforstudying-ctrl:dagql-close-persist-past-stop-budget

Conversation

@goingforstudying-ctrl

Copy link
Copy Markdown
Contributor

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: CloseWithShutdownError runs the final persistCurrentState on 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 with context deadline exceeded, clean_shutdown=1 never 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-minute closePersistTimeout), and the clean_shutdown=1 marker 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_shutdown and the store is empty, after it the store survives with its contents. Ran the ./dagql persistence/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.

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>
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