Repository navigation
rows: the write worker charges a block in flight once, and a save that fits the staging permit waits instead of being skipped - #1548
Conversation
|
Thanks @wojo. We replayed this on main ( Losing the older of two waiting saves isn't counted. When a second save also has to wait, //! A queued save that already waited still counts as skipped when a newer waiting save takes the mark.
const std = @import("std");
const rows = @import("rows_store.zig");
const fixture = @import("rows_async_fixture.zig");
test "losing the older of two waiting saves counts it as skipped" {
const x = try fixture.Fixture.open("two-waiters", .off, false, 1 << 20);
defer x.close();
var hold: fixture.Gate = .{};
defer hold.release.store(true, .release);
x.r.file_io = hold.io();
const first = fixture.prompt(41);
try x.keep(&first, 600);
try fixture.Gate.wait(&hold.entered);
x.r.worker.?.limit = x.r.worker.?.bytes();
const second = fixture.prompt(42);
try x.keep(&second, 600);
const older = x.r.waiting.?;
const third = fixture.prompt(43);
try x.keep(&third, 600);
try std.testing.expectEqual(@as(usize, 2), x.r.queue.items.len);
try std.testing.expect(x.r.waiting.? != older);
for (x.s.entries.items, 0..) |e, i| if (e == older) {
x.s.remove(i);
break;
};
try std.testing.expectEqual(@as(usize, 1), x.r.queue.items.len);
try std.testing.expectEqual(@as(u64, 1), x.r.counts.skipped);
try std.testing.expectEqual(error.StagingFull, x.r.failures.records[0].?.err);
}One way to fix it is to mark each queued save that has waited, and have |
…one waits too ashhart found the gap on ashhart#1548: stage wrote Rows.waiting over the save that waited before, and dropped checked only that one pointer. With two saves waiting, losing the older one's entry removed its queued save without counting it as skipped or recording StagingFull. A queued save now carries its own mark: stage puts a save back with Queued.waited set, and dropped counts the save it removes as skipped when that save has waited (unless the store has stopped, as before). Main counted such a save the moment it was refused, so the totals match main's. Rows.waiting is only the newest save refused room, which keeps step from spinning until a reap; dropped clears it when its entry leaves. A save that waited and is then dropped after a reap cleared Rows.waiting, before step tries it again, now counts too, since it did wait and was lost unwritten. The skip that dropped records takes a fresh job id. It used to take r.next_job as it stood, which can be the id of a job staged after the save waited, so two failure records could share one id. Tests: ashhart's, with three more checks: the newer save is still Rows.waiting, its queued save is marked, and it is written once the job in flight finishes. ashhart's own assertions fail before this change with "expected 1, found 0" (the marked check needs the new field). A new test drops a save that waited after a reap and after a newer save was staged: it fails without the mark (not counted) and without the fresh id (the record takes the staged job's id). docs/api.md now says a waiting save whose state leaves RAM is skipped unless --spill-gib copies it then.
|
Thanks @ashhart, good catch. Fixed on the branch as you suggested, and rebased onto main (
Host (Zig 0.17.0, an M4 Mac), on
I've updated the description to match. |
…one waits too ashhart found the gap on ashhart#1548: stage wrote Rows.waiting over the save that waited before, and dropped checked only that one pointer. With two saves waiting, losing the older one's entry removed its queued save without counting it as skipped or recording StagingFull. A queued save now carries its own mark: stage puts a save back with Queued.waited set, and dropped counts the save it removes as skipped when that save has waited (unless the store has stopped, as before). Main counted such a save the moment it was refused, so the totals match main's. Rows.waiting is only the newest save refused room, which keeps step from spinning until a reap; dropped clears it when its entry leaves. A save that waited and is then dropped after a reap cleared Rows.waiting, before step tries it again, now counts too, since it did wait and was lost unwritten. The skip that dropped records takes a fresh job id. It used to take r.next_job as it stood, which can be the id of a job staged after the save waited, so two failure records could share one id. Tests: ashhart's, with three more checks: the newer save is still Rows.waiting, its queued save is marked, and it is written once the job in flight finishes. ashhart's own assertions fail before this change with "expected 1, found 0" (the marked check needs the new field). A new test drops a save that waited after a reap and after a newer save was staged: it fails without the mark (not counted) and without the fresh id (the record takes the staged job's id). docs/api.md now says a waiting save whose state leaves RAM is skipped unless --spill-gib copies it then.
…one waits too ashhart found the gap on ashhart#1548: stage wrote Rows.waiting over the save that waited before, and dropped checked only that one pointer. With two saves waiting, losing the older one's entry removed its queued save without counting it as skipped or recording StagingFull. A queued save now carries its own mark: stage puts a save back with Queued.waited set, and dropped counts the save it removes as skipped when that save has waited (unless the store has stopped, as before). Main counted such a save the moment it was refused, so the totals match main's. Rows.waiting is only the newest save refused room, which keeps step from spinning until a reap; dropped clears it when its entry leaves. A save that waited and is then dropped after a reap cleared Rows.waiting, before step tries it again, now counts too, since it did wait and was lost unwritten. The skip that dropped records takes a fresh job id. It used to take r.next_job as it stood, which can be the id of a job staged after the save waited, so two failure records could share one id. Tests: ashhart's, with three more checks: the newer save is still Rows.waiting, its queued save is marked, and it is written once the job in flight finishes. ashhart's own assertions fail before this change with "expected 1, found 0" (the marked check needs the new field). A new test drops a save that waited after a reap and after a newer save was staged: it fails without the mark (not counted) and without the fresh id (the record takes the staged job's id). docs/api.md now says a waiting save whose state leaves RAM is skipped unless --spill-gib copies it then.
|
@ashhart The one red check on This PR changes only |
…one waits too ashhart found the gap on ashhart#1548: stage wrote Rows.waiting over the save that waited before, and dropped checked only that one pointer. With two saves waiting, losing the older one's entry removed its queued save without counting it as skipped or recording StagingFull. A queued save now carries its own mark: stage puts a save back with Queued.waited set, and dropped counts the save it removes as skipped when that save has waited (unless the store has stopped, as before). Main counted such a save the moment it was refused, so the totals match main's. Rows.waiting is only the newest save refused room, which keeps step from spinning until a reap; dropped clears it when its entry leaves. A save that waited and is then dropped after a reap cleared Rows.waiting, before step tries it again, now counts too, since it did wait and was lost unwritten. The skip that dropped records takes a fresh job id. It used to take r.next_job as it stood, which can be the id of a job staged after the save waited, so two failure records could share one id. Tests: ashhart's, with three more checks: the newer save is still Rows.waiting, its queued save is marked, and it is written once the job in flight finishes. ashhart's own assertions fail before this change with "expected 1, found 0" (the marked check needs the new field). A new test drops a save that waited after a reap and after a newer save was staged: it fails without the mark (not counted) and without the fresh id (the record takes the staged job's id). docs/api.md now says a waiting save whose state leaves RAM is skipped unless --spill-gib copies it then.
newBytes counted every block a state names that r.blocks lacks, and r.blocks learns a job's blocks only when reap publishes the job. A second save sharing blocks with a job in flight was therefore charged for them, and when staged it gathered and wrote them a second time (ashhart#1538). stageJob now treats a block as its own to write only when the disk lacks it and no job that can still publish writes it. Worker.promised collects those blocks once per stage, under one lock: the block files each job writes, not the blocks its state names, so a job that skipped a block promises nothing for it, and neither does a job of an older generation (a fenced job publishes nothing) or one that finished with an error. The charge, the file count and the gather loop use that one set, so the permit, the daily budget and the cap see each block once. A state that skips a block relies on the job that writes it, so publish now checks that every block the state names is on disk or among the job's own files, and fails the job (MissingBlock) otherwise. If the earlier job fails, no state on disk names a block that never reached it. That failure does not back admission off again: the job that owed the block already did. The price is that a save which skipped a block is lost with the job that failed to write it, where it used to write its own copy; a save staged after that job is reaped writes the block itself. The I/O worker picked and returned jobs by slot, so a slot freed early went to a newer job that then ran and was taken before an older one. Both now go by job id, which rises as jobs are staged, so a job that skips a block is published after the one that writes it. Tests: two saves sharing blocks are charged what reaches disk, gather each block once and read back; a state whose earlier job failed is not published; a block whose writer failed is written by the next save while a job that skipped it is still in flight; the promised blocks are those of jobs that can still publish; jobs run and are taken in staging order. docs/api.md says a block being written is charged once.
…eing skipped step popped the newest queued save and stageJob answered StagingFull when its charge did not fit beside the jobs in flight, so stage noted "saving N tokens skipped" and the save was gone, though it would have fit once a write finished (ashhart#1538). The second save of a pair was lost this way every time both were new, and a conversation's next turn resumed from the older state. stageJob now answers StagingBusy when the charge fits an empty permit and a job is in flight (Worker.busy; the eight-job limit counts the same way). stage puts the save back at the end of the queue, where step took it from, with its chunk starts, and remembers it in Rows.waiting. step then reports no work, and ready treats the queue as idle while its newest save is that one, so the engine's idle turn does not spin on a save it cannot stage. Any reap that takes a job clears the mark; a newer save arriving, or the waiting one leaving the queue, changes the newest entry, so the next one is tried at once. A save whose charge exceeds the permit on its own is skipped as before, and so are refusals by the daily budget, the cap and the free-disk floor. An eviction cannot wait, as its entry leaves RAM right after the copy, so evicted keeps its behaviour: a full permit skips that copy. A close still drains in-flight writes first and then stages the saves that waited. A waiting save whose entry leaves RAM first (the cache evicts it with spill off) is counted as skipped (StagingFull) and clears the mark, so the counters still show the lost save and the mark never names a freed entry. Two existing tests asserted the old skip (a full byte permit; a ninth small save behind eight blocked writes); they now expect the save to wait and be written. New tests cover a save larger than the permit alone, an eviction against a full permit, a newer save that fits being staged ahead of one that waits, a close that writes the saves that waited, and a waiting save whose entry leaves RAM. docs/api.md describes the permit accordingly.
…one waits too ashhart found the gap on ashhart#1548: stage wrote Rows.waiting over the save that waited before, and dropped checked only that one pointer. With two saves waiting, losing the older one's entry removed its queued save without counting it as skipped or recording StagingFull. A queued save now carries its own mark: stage puts a save back with Queued.waited set, and dropped counts the save it removes as skipped when that save has waited (unless the store has stopped, as before). Main counted such a save the moment it was refused, so the totals match main's. Rows.waiting is only the newest save refused room, which keeps step from spinning until a reap; dropped clears it when its entry leaves. A save that waited and is then dropped after a reap cleared Rows.waiting, before step tries it again, now counts too, since it did wait and was lost unwritten. The skip that dropped records takes a fresh job id. It used to take r.next_job as it stood, which can be the id of a job staged after the save waited, so two failure records could share one id. Tests: ashhart's, with three more checks: the newer save is still Rows.waiting, its queued save is marked, and it is written once the job in flight finishes. ashhart's own assertions fail before this change with "expected 1, found 0" (the marked check needs the new field). A new test drops a save that waited after a reap and after a newer save was staged: it fails without the mark (not counted) and without the fresh id (the record takes the staged job's id). docs/api.md now says a waiting save whose state leaves RAM is skipped unless --spill-gib copies it then.
…one waits too ashhart found the gap on #1548: stage wrote Rows.waiting over the save that waited before, and dropped checked only that one pointer. With two saves waiting, losing the older one's entry removed its queued save without counting it as skipped or recording StagingFull. A queued save now carries its own mark: stage puts a save back with Queued.waited set, and dropped counts the save it removes as skipped when that save has waited (unless the store has stopped, as before). Main counted such a save the moment it was refused, so the totals match main's. Rows.waiting is only the newest save refused room, which keeps step from spinning until a reap; dropped clears it when its entry leaves. A save that waited and is then dropped after a reap cleared Rows.waiting, before step tries it again, now counts too, since it did wait and was lost unwritten. The skip that dropped records takes a fresh job id. It used to take r.next_job as it stood, which can be the id of a job staged after the save waited, so two failure records could share one id. Tests: ashhart's, with three more checks: the newer save is still Rows.waiting, its queued save is marked, and it is written once the job in flight finishes. ashhart's own assertions fail before this change with "expected 1, found 0" (the marked check needs the new field). A new test drops a save that waited after a reap and after a newer save was staged: it fails without the mark (not counted) and without the fresh id (the record takes the staged job's id). docs/api.md now says a waiting save whose state leaves RAM is skipped unless --spill-gib copies it then.
Fixes #1538.
The rows store's write worker no longer drops a save that fits the staging permit on its own, and no longer charges or
writes a block twice while it is still being written. ashhart asked on #1411 for both as their own pull request on
main.
Commits
newBytescounted every block a state names thatr.blockslacks, andr.blockslearns a job's blocks onlywhen reap publishes it. A second save sharing blocks with a job in flight was charged for them and, if staged,
gathered and wrote them again.
stageJobnow takes a block as its own to write only when the disk lacks it and no job that can still publishwrites it.
Worker.promisedcollects those blocks under one lock, before the charge and again after the cap check(a fence inside
roomcan cancel jobs). It holds the block files each job writes, so a job that skipped a blockpromises nothing for it, and neither does a fenced job or one that finished with an error. The charge, the file
count and the gather loop use that one set.
publishnow checks that every block a state names is on disk or among the job's own files, and fails the job(
MissingBlock) otherwise, so no state on disk names a block that never reached it. The price: a save thatskipped a block is lost with the job that failed to write it, where it used to write its own copy. A save staged
after that job has failed writes the block itself.
taken first. Both now go by job id, so a job that skips a block is published after the one that writes it.
stageJobnow answersStagingBusywhen the charge fits an empty permit and a job is in flight (the eight-joblimit counts the same way).
stageputs the save back wheresteptook it from and remembers it inRows.waiting.stepthen reports no work andreadytreats the queue as idle while that save is the newest, sothe idle turn does not spin. A reap that takes a job clears the mark, and a newer save is tried at once.
and the free-disk floor. An eviction cannot wait (its entry leaves RAM right after the copy), so a full permit
still skips that copy. A close drains the writes in flight, then stages the saves that waited.
StagingFull), so the counters still show it.Queued.waited):stageputs a save back with it set, anddroppedcountsthe save it removes as skipped when it has waited, unless the store has stopped.
Rows.waitingis only the newestsave refused room, which keeps
stepfrom spinning;droppedclears it when its entry leaves.steptries it again, counts too.droppedrecords takes a fresh job id; it used to takenext_jobas it stood, which can be a job's id.docs/api.md describes both, including which saves are still skipped.
Tests
Each new test of changed behaviour fails before its fix, checked by reverting that one piece; the larger-than-permit,
eviction and fence tests guard behaviour that is unchanged:
rows_stage_test.zig:36fails withexpected 30256, found 37360(the bytes chargedagainst the bytes that reached disk). The test also reads the second state back.
complete()removed frompublish: "a state whose earlier job failed to write its block is not published" fails atrows_stage_test.zig:57(expected 0, found 1).writer failed to write is written by the next save, though a job that skipped it is in flight" fails at
rows_stage_test.zig:93, and the promised test atrows_io.zig:372.rows_io.zig:355("jobs are run and taken in the order they were staged, whateverslot each holds").
permit holds a ninth small save behind eight blocked writes until one is written" (
rows_async_test.zig:365and:388; they used to assert the skip). Also: a newer save that fits is staged ahead of one that waits; a closewrites the saves that waited; a save larger than the permit and an eviction against a full permit are still
skipped; a waiting save whose entry leaves RAM is counted as skipped (
rows_stage_test.zig:236with the count indroppedremoved).rows_stage_test.zig:266,expected 1, found 0without the count in
dropped; the test as posted fails the same way on commit 2). A save that waited, dropped after a reap and after a newer save was staged: not countedwithout the mark (
:303), and recorded under the staged job's id without the fresh id (:305).Host (Zig 0.17.0, an M4 Mac), on the branch's head after the rebase onto main
36470cecc:test-rows-host,test-rows-prefetch-host,test-learn-recovery,test-cuda-host,test-snapshot-index-host,test-lane-api-hostand
test-goldenpass (288/288 steps).zig fmt --checkandgit diff --checkare clean, andlean_check.pyreports425 on main and on the branch.
GPU: two GB10s at TP=2
These runs predate the rebase and commit 3, which changes only how a lost waiting save is counted and recorded.
GLM-5.3-Flash on CUDA is the family whose saves showed this (#1538's evidence), and its disk store is #1411. So the run
used a local merge (657c0b43c, not pushed) of this branch onto #1411's head
fbea020a2, against #1411 alone. Mia'sEXL3 4bpw checkpoint, MTP drafts,
--context 131072 --parallel 8 --prompt-cache-gib 6 --learn-conversations --learn-gib 30 --learn-min-free-gib 15. The cells marked (#1411's run) are from #1411's own run on the same setup.StagingFull) (#1411's run)token_shaDecode while saves stage. A 1,024-token reply, decoded alone and decoded right after two new 20k-token prompts
finish, so their saves stage between its rounds. Three times each, alternating the order, every reply to
max_tokens:Medians, with the range in parentheses. Of the branch's 13 writes, 12 come from the cell's prompts; the other is the
second filler's 55,296-token save, which had waited and was written during the first alone rep. The three saves #1411
skipped, written here, leave decode where it was: -0.2% against +0.0%, inside the spread between runs. Rank 1's logs
have no errors.
By Robert Wojciechowski (wojo on GitHub, @rwojo on X)