Repository navigation
perf(onlion): no request may be killed, and an accept takes no compare-and-swap - #128
Merged
Merged
Conversation
…ys collectable stageSearchPart() called stagePart() without the reuse map, so every search retry uploaded another full partition snapshot. Worse than the wasted upload: without the map the key was never registered anywhere, so no cleanup could even name it after a later attempt committed - the object survived every path, including the failure branch that collects the map. The argument is now forwarded, and the test asserts both effects of the same call: identical content uploads once, and the key is registered so an abandoned attempt is still reachable. A six-argument call keeps uploading twice, which is spelled out so the cost of forgetting it shows up in a diff. The caller side (worker.mjs search callback) still has to pass the map; it does not destructure it today.
…es what it reused `stageSearchPart` accepts a reuse map as its seventh argument and `stagePart` returns the earlier key for byte-identical content, but the search operation never passed one: the snapshot was re-uploaded on every CAS retry, which for a single-partition user under same-user contention is every attempt. - the search operation destructures `reuse` from the attempt context and passes it - the committed-attempt cleanup retires `staged ∪ reuse.values()` minus what the committed manifest references. Filtering only `staged` left one orphan per reused snapshot under `parts/`, and the invariant that matters is "every key under parts/ is referenced by the manifest". Both are invisible at runtime - the signature defaults to null and the upload still succeeds - so the call site is pinned by an assertion in the add-log suite, next to the one that pins the log default. The storage behaviour itself already had a test (`stageSearchPart` twice with the same bytes uploads once, and the version that forgets the argument uploads twice). Verified: onlion npm run verify 37 PASS; eslint clean; complexity gate clean for these files.
removeStaged() sent every key in one delete call. R2 and S3 cap a multi-object delete at 1000 keys, and the set it now receives can pass that: a search attempt stages one snapshot per part, so staged ∪ reuse for a wide user is easily over the cap. An oversized request fails as a whole, and both callers wrap the cleanup in a catch, so the failure looked exactly like a cleanup that succeeded - leaving the same permanent orphans this cleanup exists to prevent. The test bucket now enforces the real limit so the batch size is asserted rather than assumed: 1500 keys must all be gone afterwards.
Seven scored attempts in one day produced no score, and none of them failed inside the retrieval logic. Measured on the deployed endpoint from a clean network: every request answers at concurrency 1 and 2, and at 4 the median Add goes from 2.6 s to 106 s with 7 of 24 dying at the 120 s ceiling; at 16, five of thirty-two connections never completed. The harness probes /health before it starts, so one connection failure voids the attempt - the two `PARTICIPANT_ENDPOINT_UNAVAILABLE` results. The earlier `SEARCH_TIMEOUT 504`s are the same mechanism one layer down. The record keeps the measured table, the seven attempts with their classes, the timeline of what was deployed and reverted during the day, and the three guardrails that came out of it (the soak script, the endpoint in /admin/status, the budgets and COS timeout). The operating decision recorded is concurrency 1: a voided attempt costs everything, a slow one costs minutes. Indexed in docs/postmortem/README.md and the Chinese mirror.
Cleanup runs after the manifest CAS commits, so three paths leave an orphan with no name: killed between commit and delete, a delete the caller swallows, and - since deletes are batched at the provider's 1000-key limit - a later batch failing after earlier ones succeeded. None of them is visible in the manifest, which is the only thing a later request reads. The proposal records doomed keys in the manifest before deleting them, so a failed delete has a name that survives the request, and carries it to the next commit at no extra round trip. Rules worth pinning, in the order they must happen: record then delete; never delete doomed keys before the CAS (a retry fingerprint can still point at one); bounded newest-first so the keys a partial delete just failed on are the ones kept; and a corrupt field is skipped on read and replaced on write, never sliced, with /add, /search and /capacity serving regardless. Two things the record deliberately states rather than hides: the residue is bounded and self-healing but lags one commit, so the invariant is "after the last commit"; and a rollback loses the names permanently because an older writer drops the field. Its own decision entry and commit boundary, not part of the append-log accept slice.
…it at the byte ceiling Two points from the review that would otherwise be lost: The three counters go into `userCapacity`, which already returns `pending_entries` and `manifest_bytes` per user and already raises `warnings` at 80% of a limit. A drop counter that is non-zero raises its warning there, on the channel an operator is already reading, instead of in a field nobody has a reason to look at. The ordering rule now says why the two similar-looking deletions have opposite conclusions: graveyard keys come from earlier requests and can never be in this request's `reuse`, while staged keys belong to this request - which is exactly why deleting one also has to remove it from `reuse`. Also recorded as a companion pit, not fixed: `commitManifest` throws on `MAX_RECEIPTS` and throws the same way on `MAX_MANIFEST_BYTES`, so both are users that cannot be written to at all and are repairable only by reset or an admin action. The graveyard cap has to stay small enough that it can never be the reason a manifest crosses that ceiling.
… names Two corrections to the graveyard proposal from review: Writing is not a blind overwrite. A partially corrupt value can still hold valid keys, so the write salvages the non-empty strings out of it, unions them with this commit's doomed set and the carried-forward remainder, filters the result against the new `parts`, dedupes, caps newest-first and only then writes. Overwriting would discard keys that were perfectly parseable, which is a leak we caused rather than one the corruption caused. Both drop counters are named. Reset was a hole. `resetManifestState` builds a fresh object literal, so any field it does not name is dropped, and `clearResetPendingKeys` only knows `parts` keys and `reset.pending_keys` - a graveyard key would survive a reset with nothing referring to it, permanently, since reclamation only sweeps other epochs. Reset therefore merges the graveyard into `reset.pending_keys` and drops the field; the deletion path is already batched, so no new limit is introduced.
…ribe it The three criteria the record had pinned read-side tolerance and ordering. None of them fails an implementation that forgets the `parts` filter or the cap - and the first entry under Risks (a wrong filter deletes live data) was a paragraph rather than a test. Added: feed a non-array, an array with non-string entries, one longer than the cap, and one containing a live `parts` key; after one successful write, assert the field is either absent or an array of non-empty strings, no longer than the cap, and disjoint from `parts`.
…et fail The write-side criterion covered the shape of the stored field: absent, or an array of non-empty strings, no longer than the cap, disjoint from `parts`. Two implementations pass that while being wrong: - delete first, then tidy the field: disjointness holds and the live object is gone. The live-key case now also asserts the referenced object still exists. - cap with a reversed comparison: the length is right and the wrong keys survive. The over-cap case now asserts which keys survive - the newest and the newly doomed kept, the oldest dropped. Both were Risks entries without a test; the second one already asked, in its own text, for the direction to be pinned rather than the length.
Preflight twice, a concurrent Add phase, a concurrent Search phase - the shape the evaluation uses, runnable from any network. Reports per-status counts, p50/max and connection errors, exits 1 when anything other than 200 appears so it can gate a run. It exists because three rounds of diagnosis were spent on numbers taken through a proxy, which produced failures the real client never saw and hid the ones it did. The measurements it produced are in docs/postmortem/0006.
A request that runs into the 120 s invocation limit is not slow, it is lost: the platform kills it, the client sees 504 or an unavailable endpoint, and a scored attempt is voided. Measured on the deployed build, the paths that can reach that limit were both unbounded. - **Add**: the fold runs after the entry is durable, and it is one partition rewrite (read up to 8 MiB, export, write). A contended user folded its way to 51-106 s. It now runs under a budget (`NMG_FOLD_BUDGET_MS`, default 20 s); past it the backlog waits for the next request, and the accept's 200 is unaffected because it was earned before the fold started. - **Search**: the partition loop reads every partition to be exhaustive, which is unbounded in the manifest. Past `NMG_SEARCH_DEADLINE_MS` (default 60 s) it stops and answers from the partitions already read, with `partial: true` in the response - fewer candidates for one question instead of no answer at all. Both are asserted at the source in the add-log suite, next to the reuse call-site assertions, because neither can be observed from a passing request: an unbounded fold looks exactly like a fast one until it is killed. Not addressed: the CAS retry loop still has only a count limit, so an accept whose counter reservation keeps losing cannot honestly answer 200. That needs a different accept shape and is written up separately. Verified: onlion npm run verify 38 PASS; eslint clean.
The write path serialised every concurrent Add for one user on that user's manifest and retried the loser up to twelve times, each retry redoing a megabyte of partition work. Measured on the deployed build that is a 0.5 s write at concurrency 1 and 51-106 s at concurrency 4, with requests dying at the platform ceiling. The counter was the only thing the accept wrote there, and it is not the truth: the log is. So: - **accept writes the entry and nothing else.** It reads the manifest once for the two things the log cannot answer - which generation this entry belongs to, and the receipt of a request that was already folded - then writes the entry under its own key, which no rival can collide with. - **the count becomes a hint.** `bumpPendingHint` takes one attempt and no retry; losing the compare-and-swap only makes the hint stale, and a stale hint costs a listing. - **the read path stops trusting a zero.** `pendingForSearch` always attempts its bounded listing instead of returning early on `pendingCount === 0`, because a hint that lags is exactly how accepted data becomes invisible. - **the fold trigger tolerates an unknown hint.** A missing hint is worth a look: the fold lists the log first, so looking at an empty log costs one listing, while skipping a real backlog hides data. Asserted at the source: the accept body must not contain a `mutateUser(` call, the hint bump must swallow its own failure, and the pending read must not short-circuit on the hint. Verified: onlion npm run verify 38 PASS; eslint clean; complexity gate clean for these files.
The accept stopped taking a compare-and-swap in the previous commit, but it still wrote the count on a best-effort basis. That write had one consumer - an operator reading `/capacity` - so it was paying a compare-and-swap per accept to keep a number fresh for someone who may never look. - the hint write and its helper are gone; an accept now performs exactly one write, its own log entry, under a key no rival can collide with - `/capacity` derives `pending_entries` from a bounded listing and reports `pending_capped` when the bound was reached, so the number is honest instead of maintained - the fold trigger cannot read a count that no longer exists, so it is scheduled: roughly one accept in `FOLD_TRIGGER` (overridable with `NMG_FOLD_EVERY`) attempts a fold. The fold lists the log first and folds what is there, so a wasted attempt costs one listing while a missed backlog would hide accepted data Two gate sites trusted a zero count and had to go with it: `pendingForSearch` returned early, and `fusePendingRecords` returned before listing. Both are the same class - the manifest said zero while the log held entries - and the add-log suite's "accepted but unfolded entry must be searchable" case is what caught the second one. Asserted at the source: the accept body must contain no `mutateUser(`, `commitManifest(` or `setPending(`, the fold must be scheduled and budgeted, and the pending read must not short-circuit on the hint. Verified: onlion npm run verify 38 PASS; eslint clean.
…alls the provider The document text for this path is `[day ]statement` - the service never supplies `previous_turn` - so it is fully determined by the message and can be computed before any partition is opened. That makes the inlining possible, and it matters because the fold is the only per-user serialisation left: a fold that already holds the vectors is a pure storage operation. - accept computes the text, embeds it under `NMG_ACCEPT_EMBED_MS` (8 s) and stores the vectors in the log entry with the index identity that produced them. Losing the race is not an error: the entry is written without them and the fold embeds as it does today. - the fold seeds its client from the entry's vectors, keyed by the same text function, so the provider is not called again for the same record. A partial or stale seed falls through to the provider, which is correct and only costs the call this avoids in the common case. - `embeddingClient` gained `embedDocumentsDirect`: the deferred form throws a preparation by design, which is right inside a SQLite session and wrong on the accept path. Also bounded the fold's backfill to one batch of 32 (`maxBatches: 1`, matching the search path). Without it one fold embedded a whole partition's backlog in a single request: unbounded spend and unbounded duration, in the one place that is meant to be short. Measured in the suite: 24 sequential Adds, each making **at most one** provider call even when it triggers a fold, with a fold proven to have run (partition written, backlog drained). Verified: onlion npm run verify 38 PASS; eslint clean; LSP reports no diagnostics.
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.
What
Four commits that make a request unable to be killed at the platform ceiling, and remove the write path's compare-and-swap.
7eba5a97partial: true)0b0009f767346911/capacitylistsd85b76efMeasured, from a clean network against the deployed build before these changes: Add p50 2.6 s at concurrency 1, 51-106 s at concurrency 4 with 7 of 24 requests dying at 120 s, and 5 of 32 connections never completing at 16. The write path serialised on one manifest and retried the loser up to twelve times, each retry redoing a megabyte of partition work.
Why each piece
/capacity, which now lists a bounded prefix and reportspending_capped.[day ]statementfor this path, so it is known before a partition is opened; the fold then applies storage only, which is the one place left that serialises per user.Verification
onlion npm run verify: 38 PASS, including a case that 24 sequential Adds each make at most one provider call, with a fold proven to have run.Not verified
No request has been issued against a build containing these changes. The numbers above are the before-state; the after-state needs one real run.