Repository navigation
perf(dynamic-workflow): batch volatile progress events off the fsync path [CI-only mirror of upstream PR #67] - #1
Closed
NkAntony777 wants to merge 6 commits into
Closed
NkAntony777 wants to merge 6 commits into
NkAntony777 wants to merge 6 commits into
Conversation
…path Every event was written inside its own BEGIN IMMEDIATE/COMMIT under synchronous=FULL, so each one paid a full fsync plus a hash-chain advance (2 SELECT + 2 INSERT). Measured on a real Windows/NVMe deployment: 1035 us per event. step.progress is 85% of the event volume on a real run (1110 of 1312 events in a sampled database) and carries no replayable state -- only stepId/sessionId/turnId/time. The same run measured 1100 progress events at 1139 ms; batching them brings that to 95 ms (12x) with the integrity chain still verifying. Changes: - synchronous=NORMAL (WAL): survives process crash, which is the common case. Only host power loss can lose the last few transactions, and the hash chain -- not the fsync cadence -- is the authority for detecting that. Adds busy_timeout=5000. - Volatile event types are broadcast immediately, buffered in memory, and flushed on a 64-event / 1s boundary. Any durable event (step.queued / started / finished, run.*) is a flush barrier, so seq order stays monotonic across replay. close() flushes what is left. - A volatile event takes its sequence number at emission, so a long-poll woken by the change notification can actually read it -- previously the notification arrived while the event was still only in memory, and waitEvents read from SQLite alone, returning zero events and a cursor that had not moved. - Store startup recovers the counter from sqlite_sequence and MAX(seq), so a restart does not reissue sequence numbers. - A failed batch restores the buffer and reschedules the flush instead of dropping events. The tamper-evident integrity ledger from MiniMax-AI#48 is preserved: every buffered event is still chained in the same transaction that inserts it, and verifyIntegrity() passes over the buffered and flushed states alike. Tests: checks/integrity.check.mjs gains two cases -- buffered events are readable before flush and keep their sequence after it (including through workflow_wait end to end), and a sequence counter survives close/reopen with the chain still verifying. Suite: 116/118 passing, 2 skipped, 0 regressions against main (which is 114/117 with the same single pre-existing failure: cross-reuse-mcp EBUSY, the win32 detached-daemon cleanup defect that MiniMax-AI#57 addresses). Build is byte-reproducible; packaged MCP test green.
…OMMIT workflow_wait validated its cursor with Number.isInteger, which accepts 1e100. A packaged MCP call with afterSequence: 1e100 returned nextSequence: 1e100 and no events, and every real future sequence was then filtered out for that client permanently. Require Number.isSafeInteger and give the schema an explicit Number.MAX_SAFE_INTEGER maximum; the regression covers 1e100, MAX_SAFE_INTEGER + 1, Infinity and a negative cursor. flushVolatile restored the whole batch on any exception, so a COMMIT that reached SQLite but reported an error afterwards left the same rows both persisted and buffered: events() exposed duplicate sequence numbers and a later close() failed with UNIQUE constraint failed: events.seq. A thrown error is not evidence the rows are absent, so the table now decides. restoreVolatile merges keyed by seq and drops rows already present; the probe is skipped inside an enclosing transaction, where the rows are only visible and an outer ROLLBACK would otherwise drop them silently. Adds after-COMMIT ambiguity and unsafe-cursor regressions. 126 checks pass, and the packaged MCP test passes when run standalone: the two suites must not run concurrently because they contend for the same dashboard port.
…BACK failure A failing ROLLBACK was treated as proof that COMMIT had already landed. That inference is wrong: a rollback can fail for other reasons (busy/locked) and leave the transaction open. Probing the events table in that state reads the connection's own uncommitted rows as durable and drops them from the retry buffer, losing progress events that were never persisted. Ask SQLite instead. isTransaction reports whether a transaction is still open, and only once it has genuinely ended does a row on this connection mean committed. restoreVolatile now defaults its probe to that condition instead of the unrelated txDepth counter. flushVolatile owns the reconciliation for its own batch: the batch leaves volatileBuffer before transaction() runs, so transaction() cannot see it. Restoring it in transaction()'s catch instead would double-restore against a buffer that no longer holds it. Adds a regression that injects COMMIT and ROLLBACK failures together and asserts the uncommitted rows stay queued. 127 checks pass plus the packaged MCP test standalone.
A failed COMMIT followed by a failed ROLLBACK left SQLite inside a transaction while the store already believed it was idle. The next BEGIN then failed, so close() could not persist the queued batch. Roll that leftover transaction back before reserving sequences. Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
|
All three exact-head runs for 52fb00e completed successfully — CI: https://github.com/NkAntony777/MiniMax-Code-Plugins/actions/runs/38677974092, Dynamic Workflow: https://github.com/NkAntony777/MiniMax-Code-Plugins/actions/runs/38677973805, CodeQL: https://github.com/NkAntony777/MiniMax-Code-Plugins/actions/runs/38677973952. Fork-hosted evidence for upstream PR MiniMax-AI#67; closing as planned. |
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.
Fork-internal PR created solely to trigger the pull_request workflows (CI, CodeQL, Dynamic Workflow) for the exact head of upstream PR MiniMax-AI#67 (
52fb00eae5bcf626299a66873cca23b690b39d61), whose upstream runs are approval-gated. Do not merge; close after the runs complete.