fix(checkpoint): report save failures instead of dropping or crashing on them - #85
Open
Agnik47 wants to merge 1 commit into
Open
fix(checkpoint): report save failures instead of dropping or crashing on them#85Agnik47 wants to merge 1 commit into
Agnik47 wants to merge 1 commit into
Conversation
… on them `save()` is declared `void` but does asynchronous work, and nobody observed the resulting promise. `_performSave` ends with `throw lastError` when all retries fail or the error is non-retriable — ENOSPC, EACCES, a read-only volume, a bad path — and that rejection had no handler: `nextQueue` was never caught, and `nextQueue.finally(...)` created a second unhandled rejected promise. Under Bun/Node that surfaces as an `unhandledRejection`, which by default terminates the process mid-run with no indication that a checkpoint write, rather than a provider, was the cause. It also meant any `save()` issued while a rejected promise sat in `saveLock` chained onto it with `.then()` and never called `_performSave` at all, silently dropping those updates. Terminate the chain with a `.catch` that records the failure and logs it. The promise stored in `saveLock` therefore never rejects, which fixes both halves at once: no unhandled rejection, and a failed write no longer stops the saves queued behind it. `flush()` previously rethrew whatever the queue happened to be holding, so a checkpoint write failure arrived at `Orchestrator.run` as an opaque rejection after every phase had finished, and `runBenchmark` wrote `status: "failed"` over a run whose work was complete. It now reports accumulated failures deliberately, naming checkpoint persistence and the affected run, and consumes them so a later flush reports new failures rather than repeating one already surfaced. Throwing remains correct: if the checkpoint cannot be written then neither can `updateStatus`, so the on-disk record is stale either way — the fix is that the reason is legible. Finally, serialise the checkpoint in `save()` rather than in `_performSave`. The object is mutated concurrently by in-flight tasks (`updatePhase` does `Object.assign`), so a queued write used to persist whatever state existed when it eventually ran. The queue now provides the snapshot guarantee its shape implies. Fixes supermemoryai#71
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.
Fixes #71
The bug
save()is declaredvoidbut does asynchronous work, and nobody observed the resulting promise:_performSaveends withthrow lastErrorwhen the retries are exhausted or the error is non-retriable — ENOSPC, EACCES, a read-only volume, a bad path. That rejection had no handler:nextQueuewas never caught, andnextQueue.finally(...)created a second unhandled rejected promise. Under Bun/Node that becomes anunhandledRejection, which by default terminates the process mid-run, with nothing indicating a checkpoint write rather than a provider was the cause.Three consequences, all reproduced in the tests:
save()issued while a rejected promise sat insaveLockchained onto it with.then(), so_performSavewas never called — those updates were silently lost.flush()inherited the rejection, rethrowing intoOrchestrator.run(index.ts:311) after every phase had completed, sorunBenchmarkwrotestatus: "failed"over a run whose work was entirely done.Plus the queue promised a snapshot it never delivered:
JSON.stringifyran inside_performSave, i.e. at write time, while the checkpoint object is mutated concurrently by in-flight tasks (updatePhasedoesObject.assign).The fix
Terminate the chain with a
.catchthat records the failure and logs it. The promise stored insaveLocktherefore never rejects — which fixes points 1 and 2 together: there is no unhandled rejection, and a failed write no longer prevents the saves queued behind it from running..finallyis likewise safe, since the promise it observes cannot reject.flush()now surfaces failures deliberately rather than by accident: it reports accumulated errors naming checkpoint persistence and the affected run, then consumes them so a later flush reports new failures instead of repeating one already surfaced. AddedhasSaveError(runId)for a non-throwing check, anddelete(runId)clears any pending error so a removed run cannot fail a later flush.On whether
flush()should still throw — I kept it throwing. If the checkpoint cannot be written thenupdateStatus(checkpoint, "completed")cannot be written either, so the on-disk record is stale no matter what; claiming completion would be false. The defect in point 3 was the opacity, not the failure itself, and the message now names the cause. Both call sites (orchestrator/index.ts:311,routes/runs.ts:269) already sit inside acatch, so this is the same control flow with a legible reason.Serialise in
save()rather than_performSave, so a queued write persists the state as of thesave()call instead of whatever concurrent tasks have since mutated it into.updatedAtmoves with it, which is the more accurate timestamp anyway.Verification
bun test— 11 new tests insrc/orchestrator/checkpoint.test.tspass.main, 9 of the 11 fail, including the unhandled-rejection test, which catches the real rejection via aprocess.on("unhandledRejection")listener.mkdirand the write both fail with ENOTDIR — the same code path as ENOSPC/EACCES. Each test gets its ownmkdtempdirectory, cleaned up inafterEach.deleteclearing pending errors, and write ordering.tsc --noEmitclean.src/orchestrator/checkpoint.tsalready failsprettier --checkonmain; I left that alone and confirmed every line I added is within the configuredprintWidth.