Repository navigation
fix(runtime): refuse new widget dev work once closed, and never reject a close - #649
Merged
Merged
Conversation
…t a close - Set a closed flag in close(): a late start is refused with 503 WIDGET_DEV_UNAVAILABLE and a resume still going through the store stops, so nothing watches a folder after close. - Once closed, a prune starts no further snapshot removal, so close waits for the one removal in flight only; leftovers stay listed for the next prune. This keeps several held snapshots inside the close bound. - An engine that throws as it closes is logged, and the other sessions still end; main attaches its handler to the close promise as it is made. - Assert the total retry budget of the removal options actually passed to rm, so a tiny budget fails the held-snapshot test. - Document why a close during settle cannot prune the newest build's snapshot, with a test for the approval path that relies on it. Refs #647
Contributor
Author
|
Review attestation: ready to merge at A push to this PR makes this attestation stale; the new head needs its own review. |
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 #647. Follow-ups from the isolated review of #646 (#639).
What changed
Retry budget assert. The held-snapshot test now checks the total wait of the options actually passed to
rm. Node waitsretryDelayms longer on each retry, so the test computesretryDelay * maxRetries * (maxRetries + 1) / 2and requires at least 1000 ms. The current values give 1500 ms. WithmaxRetries: 1, retryDelay: 1the test fails withexpected 1 to be greater than or equal to 1000.Closed flag.
close()now setsclosed:startis refused with503 WIDGET_DEV_UNAVAILABLE, which already meant "this node is not running sessions"; the EN and VI docs now add "or is closing";resume()that is still going through the store stops, and the remaining sessions staylivein the store for the next boot.No unhandled rejection.
end()catches and logs an engine that throws onclose(), so the other sessions still end andclose()never rejects.main.tsalso attaches a handler to the close promise when it creates it, because it awaits that promise only after the other shutdown steps.Close cap with several held snapshots. Once the node is closed, a prune starts no further removal.
closewaits only for the removal already running, about 1.5 s at most, which fits inside the 2 s cap and the node's 5 s shutdown grace. The skipped snapshots stay on the session's list, and the next prune after an install removes them.Close during settle. This case cannot happen, and a code comment explains why:
A test covers the approval path, where only the engine keeps the newest snapshot: the node closes as the answer arrives, and the newest snapshot is still present.
Tests
The new tests fail against main's
widget-dev-sessions.ts:starts no further removal once closing...fails withexpected [ …(2) ] to have a length of 1 but got 2ends every session, and resolves, on a close where a watcher fails to let gofails withpromise rejected "Error: the watcher would not let go" instead of resolvingwatches nothing once closed...fails withexpected { ok: true, ... } to match object { ok: false, code: 'SESSION_STOPPED' }keeps the newest build's snapshot...is a confirmation test for item 5 and passes on main too, as expected for a case that cannot happen.The spec mocks
startDevEnginefrom@clarkcant/coreas a pass-through that counts closes and can be made to throw.Overlap
PR #644 (#638) also edits
widget-dev-sessions.spec.ts.git merge-treeagainst its head merges cleanly.