use commitTS for workpool - #220
Draft
ianmacartney wants to merge 8 commits into
Draft
Conversation
`v.commitTs()` and `db.vars.commitTs` landed in convex 1.43, and the pending queues are about to be ordered by them, so raise the peer dependency to match. Also bumps convex-test to 0.0.55-alpha.0, the first version that resolves the commitTs placeholder at commit time instead of storing it verbatim.
The `segment` fields on pendingStart/pendingCompletion/pendingCancelation held a 100ms bucket derived from `Date.now()`. That is read when a mutation *starts*, so a slow transaction could commit an entry behind a cursor the main loop had already read past. 0.4.7 compensated by rewinding each cursor 15 seconds on every scan and rescanning each table from the beginning once a minute, which at load meant re-reading thousands of tombstones per iteration. Those fields are now `v.commitTs()`, written as `db.vars.commitTs` and resolved at commit, so an entry cannot appear behind a timestamp already read. The rewind buffer and the periodic rescan are gone; the recovery iteration still runs, but only to find stuck running jobs. On a 5000-task saturation benchmark (parallelism 200, 20ms tasks) throughput went from 121 to 144 tasks/s and p99 latency from ~32s to ~25s. The field and index keep their names so existing documents still validate. The values are now nanoseconds rather than 100ms buckets; an older version's much smaller ones sort first and get processed promptly on upgrade. Work that shouldn't start yet stores its start time in the same field, which sorts after everything committed before then, so one index covers both cases: - A retry backoff is written directly, however short. `rescheduleJob` runs inside the transaction that writes the cursor, so a time past now is certain to sort ahead of it. - A caller's `runAt` is written directly only when it is more than SAFE_FUTURE_MS out, far enough that no commit latency could place it in the past. Nearer than that it is ordered by the commit timestamp with `runAt` stored alongside, and the loop moves it forward itself (`promoteScheduled`) rather than starting it. Eligibility is bounded by `endOfMs(now)` rather than `toTimestamp(now)`: `Date.now()` is whole milliseconds but a commit timestamp is not, so a bound at the start of the current millisecond would skip everything that just committed. Two paths that returned without deleting a pendingStart — work already in `running`, and a cancelation whose work document is gone — now delete it. They used to be repaired by the periodic rescan; with the cursor no longer rewinding, an entry left behind would never be read again.
The throughput scenarios only ever enqueue work that is ready immediately, so nothing exercised the two paths that write a wall-clock time into the ordering field instead of the commit-timestamp placeholder. Those are also the paths convex-test could be lenient about, since it does not enforce what the backend accepts for a `v.commitTs()` field. `test/scheduling:default` enqueues one delayed mutation and one action that fails twice, then reports when each ran. Against the dev deployment: a `runAfter: 8000` ran 59ms after its runAt and never early, retries backed off 722ms then 939ms over 3 attempts, and a `runAfter: 360000` was stored at its start time with no promotion pass and ran 196ms late.
Notes the new ordering, the benchmark result, what happens to entries an older version wrote, and the convex 1.43 requirement.
commit: |
Member
Author
|
One thing to note here is that reverting to an older version would require a migration to convert segments into their old format, - which could be hard to get right, but could make some assumptions about the maximum segment a user would have previously used, or migrate based on creation time of the documents or something..? |
reeceyang
reviewed
Aug 4, 2026
reeceyang
left a comment
reeceyang
left a comment
Contributor
There was a problem hiding this comment.
When users migrate to this version, would work that was originally scheduled to run in the future get executed immediately?
| * work until then; the default runs it as soon as the loop sees it. `segment` | ||
| * pins the ordering value, e.g. to give several entries the same one the way | ||
| * a single batch enqueue does. | ||
| * Bypasses the public enqueue API to keep tests focused on the loop. |
Contributor
There was a problem hiding this comment.
would it be possible to use the enqueueHandler here? or extract out a common helper function so the segment logic stays in sync between the test and the actual implementation
| expect(o.running).toHaveLength(0); // moved, not started | ||
| expect(o.pendingStart[0].segment).toBe(toTimestamp(runAt)); | ||
| // The cursor moved past where it used to sit, so it won't come back. | ||
| expect(o.segmentCursors!.incoming).toBeLessThan(toTimestamp(runAt)); |
Contributor
There was a problem hiding this comment.
does this actually check that the cursor moved forward? The cursor would also be less than runAt before the loop runs for the first time, right?
| // Entries whose `runAt` hasn't arrived were only visible because we | ||
| // couldn't safely write that time into `segment` at enqueue. We can here: | ||
| // this transaction also writes the cursor, so a value past `now` is | ||
| // guaranteed to land ahead of it. Move them and they stop coming back. |
Contributor
There was a problem hiding this comment.
If I understand correctly, this is ok because we never advance the cursor past eligibleBefore (which is endOfMs(Date.now()))?
A pendingStart written by an older version stores a 100ms bucket in `segment` and has no `runAt`, so the bucket is the only record of when the work should start. Read as a nanosecond timestamp that bucket is ~1.8e10 against ~1.8e18 for a real one, which put every such entry below the eligibility bound: work scheduled minutes or years out was started immediately on upgrade, and its reported start lag was nonsense. Entries below a threshold — nanoseconds for the year 2000, orders of magnitude away from either format — are now read back as a scheduled time and handed to `run` as `runAt`. From there the existing path applies: due work starts, and anything still in the future is rewritten as a timestamp and left alone until then. Verified against a deployment by seeding a 0.4.9-format entry 90s out alongside 0.4.9-format cursors; it was rewritten to its exact bucketed time rather than started, then ran when due.
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
2 participants
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.
use segment field to capture commit TS, and new runAt sidecar when it needs to be in the future, but not so far in the future that we can safely assign it directly