Skip to content

Wait for the push that carries a missing event, not for a clock - #107

Merged
pathscale merged 3 commits into
masterfrom
fix/no-futile-retry-sleep-while-closing
Sep 22, 2026
Merged

pathscale merged 3 commits into
masterfrom
fix/no-futile-retry-sleep-while-closing

Conversation

@pathscale

@pathscale pathscale commented Sep 22, 2026 •

Copy link
Copy Markdown
Owner

The drain loop deferred on a gap in the index event stream by sleeping 500ms. This replaces the clock with the event, and makes what is left of the number a per-table parameter rather than a constant.

What was wrong

A batch can only apply a contiguous run of the event stream. When collection finds a hole it defers, and the loop then slept a flat 500ms before retrying. That sleep was standing in for an event it could have waited on directly.

It was measured downstream as the dominant cost of shutting a daemon down. A settled store stepped from 0.42s at 700 indexed files to 1.45s at 800 and then flattened, rising after that only at the underlying linear rate of about 0.06s per 100 files. A page drain cannot step by a second and then flatten; the riser was two of these sleeps. A mid-write shutdown cost the same as a settled one, which is what says the hole is the ordinary event-order inversion rather than an unfinished write.

It was worst during close, where it is futile by construction: closing refuses every push, so the event the hole waits for can never be queued.

The change

Queue::await_more_operations registers on the queue's own wake-up and returns the instant an operation is pushed, which is the only way the missing event can arrive. Two things end it early:

  • A non-empty queue. The push may have landed while the collection that found the hole was still running, in which case the event is already here and waiting for it waits for something that has happened. The waiter is registered before the queue is read, so a push landing between the two wakes it rather than being missed.

  • Any state other than Running. This one is provable rather than a heuristic: a push takes the lifecycle lock and is refused unless the state is Running, and begin_close takes the same lock, so once Closing has been observed here no further push can ever be accepted. The missing event cannot arrive. A yield keeps the remaining escalation off the worker's only pool thread without pretending to wait.

The number is now a parameter

What is left is a backstop against a wake lost to a race with no later push to deliver another. It is PersistenceConfig::event_gap_wait_cap(), a provided method defaulting to DEFAULT_EVENT_GAP_WAIT_CAP (100ms), so no implementor breaks and a table whose producers are genuinely slower than the default assumes can say so, instead of one constant deciding for every table at once. It is read once when the table starts: a parameter that could change under a running drain loop would be a worse thing to explain than one fixed at startup.

GIVE_UP_AFTER_ATTEMPTS counts deferrals, not time, and its doc claimed "about a minute" from 500ms x 120. That stopped being true here, so it now states the relationship instead: the cap times the count is the worst case before a stalled gap fails the table.

Effect

Measured on a consumer daemon whose shutdown this dominated: 19-22s to 1010ms, three runs at 1014 / 1011 / 1010, zero "did not unload" reports. The remaining second is the real drain.

Verification

cargo check --all-targets clean, cargo clippy --workspace --all-targets -- -D warnings clean, cargo fmt applied. cargo test --lib persistence::task is 20 passed / 0 failed, including only_an_exhausted_collection_waits_for_more_operations, which pins the predicate the loop asks before waiting, and draining_scattered_writes_stays_linear.

The first commit reduces the constant; the second replaces the mechanism. Reviewable separately.

meh added 2 commits September 22, 2026 11:55
The engine's drain loop responds to a gapped event stream by sleeping 500 ms and
trying the collection again. That is not waiting for work to finish: it is
waiting for an event that the escalation after
`COLLECT_WHOLE_QUEUE_AFTER_ATTEMPTS` makes unnecessary anyway, so the only thing
the sleep buys is not spinning while that plays out. Half a second is far more
than that is worth.

Measured in a downstream daemon, release build, settled store, timing SIGTERM to
process exit against the number of indexed files:

  600 files   0.356 / 0.348 / 0.365 s
  700 files   0.418 s
  800 files   1.445 / 1.418 s
  1000 files  1.480 s
  1200 files  1.600 / 1.572 / 1.556 s
  1400 files  2.030 s

A step of 1.03 s between 700 and 800 files, then a plateau that rises only at
the underlying linear rate of about 0.06 s per 100 files. A page drain cannot
step by a second and then flatten. The riser is two of these sleeps. A shutdown
that caught a write in flight cost the same as a settled one, 1.572 against
1.556, which is what says the hole is the ordinary event-order inversion rather
than an unfinished write.

It is worst during `close`, where it is futile by construction. Closing refuses
every push from then on, so the event the hole is waiting for can never be
queued, and the loop can only finish through the escalation. Every sleep on that
path is time bought for nothing.

100 ms, and the constant is named rather than inlined so the next person can see
what it is and what it costs.

**This is the interim number, not the right mechanism.** The loop should wait on
`progress_notify`, which is already signalled both when the queue drains and when
the lifecycle moves, `close` included, so it would wake on the event instead of
guessing at it. `wait_for_ops` already waits that way. Doing it here means
reshaping the loop, which is a larger change than this one, so the constant comes
down now and the mechanism follows separately.

No behaviour changes beyond the wait: the collection, the escalation and
`validate_events` refusing a stream with a hole are all untouched.
The drain loop deferred on a gap in the event stream by sleeping. The
previous commit cut that sleep from 500ms to 100ms, which was the interim
number and not the mechanism. This is the mechanism.

Queue::await_more_operations registers on the queue's own wake-up and
returns the instant an operation is pushed, which is the only way the
missing event can arrive. Two things end it early:

  - A non-empty queue. The push may have landed while the collection that
    found the hole was still running, so the event is already here and
    waiting for it waits for something that has happened. The waiter is
    registered before the queue is read, so a push landing between the two
    wakes it rather than being missed.

  - Any state other than Running. This one is provable, not a heuristic:
    a push takes the lifecycle lock and is refused unless the state is
    Running, and begin_close takes the same lock, so once Closing has been
    observed here no further push can ever be accepted. The event cannot
    arrive, and every moment spent waiting for it is bought for nothing.
    That was the shutdown case: a settled store stepped from 0.42s at 700
    indexed files to 1.45s at 800 and then flattened, and the riser was two
    of these sleeps.

What is left of the number is a backstop against a wake lost to a race with
no later push to deliver another, and it is no longer a constant. It is
PersistenceConfig::event_gap_wait_cap, a provided method defaulting to
DEFAULT_EVENT_GAP_WAIT_CAP, so no implementor breaks and a table whose
producers are genuinely slower than the default assumes can say so rather
than having one number decide for every table at once. It is read once when
the table starts: a parameter that changed under a running drain loop would
be worse to explain than one that is fixed.

GIVE_UP_AFTER_ATTEMPTS counts deferrals, not time, and its doc claimed "about
a minute" from 500ms x 120. That stopped being true, so it now states the
relationship instead: the cap times the count is the worst case before a
stalled gap fails the table.
@pathscale pathscale changed the title Stop the drain loop paying half a second for a retry Wait for the push that carries a missing event, not for a clock Sep 22, 2026
All three published crates move together: worktable, worktable_codegen and
worktable_dsl are one release, and the path dependencies between them carry the
version, so bumping one alone would publish a crate asking for a sibling that
does not exist at that version.

The fleet takes `worktable = "^1.10.0-beta1"`, and a caret over a prerelease
does not admit another prerelease: ^1.10.0-beta1 matches 1.10.0-beta1 and
nothing else in the beta series. So every consumer needs its requirement moved
to beta2 explicitly, which is the pass that follows this.
@pathscale
pathscale merged commit 09fc5e0 into master Sep 22, 2026
8 checks passed
@pathscale
pathscale deleted the fix/no-futile-retry-sleep-while-closing branch September 22, 2026 22:45
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