37cfa863eeb6e2053c3df906ea72c55d9f436d6c
17 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
A.Shakhmatov
|
37cfa863ee |
commands: the aggregation expression evaluator
The whole corpus is green: 46 pass, 0 fail, byte-identical to mongod 8.3.7 on
every case including all 27 expressions and the compound `_id` that was the
last accumulator failure.
Expressions are *compiled once per pipeline and evaluated per document*, and
that split is the point rather than an optimisation: it keeps the property M2's
refusals bought, which is that a pipeline that cannot be answered is refused
before a single document is read instead of half way through with part of the
work already reported. `Expr` is the compiled tree, `compile_expr` reports,
`eval_expr` cannot.
Nineteen operators: `$literal`, the five arithmetic ones, seven comparisons,
`$and`/`$or`/`$not`, `$cond` in both its forms, `$ifNull` and `$switch`. Plus
the two shapes that are not operators at all -- a compound document, which is
what a `$group` `_id` usually is, and an array.
Everything the corpus recorded, and none of it guessable:
- absent and a present null are *different* internally, because `$ifNull`
treats them alike and `$push` does not. Hence `?bson.Value` throughout,
where the obvious shortcut is to fold absent into `.null` at the boundary
and lose the distinction for good.
- arithmetic over absent or null is `null` -- not an error, not zero -- and
over a string is an error, 7157723.
- `$divide` by zero is 4848401, `$switch` with no branch and no default is
40069, and both are failures only a document can produce, so `EvalError`
exists and `report_eval_error` maps it.
- two operators in one expression document is 15983 and *not* `$group`'s
40238: mongod distinguishes an expression from an accumulator there.
- truthiness is MongoDB's, so `-5` is true and `0.0` is false.
- `$mod` follows the dividend's sign, so -5 mod 4 is -1.
- `$not` takes a bare argument as readily as a one-element array.
`compile_expr` and `compile_operator` call each other, so their error set is
written out rather than inferred -- Zig cannot infer a cycle, and the failure
mode is a "dependency loop" message that says nothing about expressions.
Three cases left the Tier 0 refusal test, because a compound `_id`, `$literal`
and a `$multiply` argument all work now. What is refused should be what is
missing, so an unknown operator and a wrong operand count took their place.
191/191 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e2
concurrent, e2e3 16, e2e4 17, e2e6 72, e2e7 86, crud corpus unchanged at
201/90/196.
|
||
|
A.Shakhmatov
|
1c72aa8938 |
tests/spec: record the expression evaluator's spec from mongod
27 cases, recorded before a line of the evaluator is written, which is the
whole point of having built the recorder first: the expectations come from
mongod 8.3.7 rather than from what the implementation is about to do.
The corpus reads 1 pass / 26 fail, and the one pass is the unknown-operator
refusal M2 already answers correctly. Expressions are exercised through
`$group` because `_id` and the accumulator arguments are the only expression
positions that exist until `$addFields` and `$project`'s computed fields land;
testing them anywhere else would be testing a stage that is not there.
Ten things the recording settled that guessing would have got wrong:
$add over a missing field or null null -- not an error, and not 0
$add over a string error 7157723
$divide by zero error 4848401
$mod of -5 by 4 -1, the dividend's sign
$lt of a number and a string true, canonical type order
$and over -5 truthy
$not of a missing field true
$switch, no branch and no default error 40069
two operators in one expression error 15983, *not* $group's 40238
$subtract with one operand error 16020
The six error codes are in `ErrorCode` already, so the evaluator's refusals and
its runtime failures have somewhere measured to land. The evaluator itself is
the next commit and is not started.
191/191 unit tests, crud corpus unchanged at 201/90/196.
|
||
|
A.Shakhmatov
|
76afa75efe |
commands: the $group accumulators
Nine of them -- `$sum`, `$avg`, `$min`, `$max`, `$first`, `$last`, `$push`,
`$addToSet`, `$count` -- and the corpus goes 9 pass / 10 fail to 18 pass /
1 fail. The one left is the compound `_id`, which needs the expression
evaluator.
Landed before that evaluator, against the tier order the design review set
out, and the corpus is why: every one of these takes a single value per
document, a path or a constant, so nine of its ten failures turned out to be
reachable without one. `classify_expr` already produced exactly that value.
What the recording caught, which is the argument for measuring expectations
rather than writing them:
- `$avg` over a group with no numeric value is **null**, not `0`. A divisor
that counted documents rather than numbers would pass every test anybody
would think to write by hand, and be wrong on the one group that matters.
- `$min`/`$max` compare across types in canonical BSON order, so the maximum
of `30`, `7` and `"not a number"` is the string.
- `$push` skips an absent field but would push an explicit null, so "resolved
to nothing" and "resolved to null" cannot be the same value internally --
which is why the accumulators take `?bson.Value` and not `.null`.
- `$first`/`$last` follow input order, including when the value is absent:
`$last` of a missing field is null, not the last present one.
`AccState` is one struct rather than a union: the fields are small and every
site already switches on the kind, so a union would add a tag test where a
switch was going to be anyway. Its arrays are the gpa's, the values inside them
the reply arena's -- they outlive the group and travel with the documents.
`numeric_value` is the int32-or-double narrowing MongoDB reports, shared now
between the accumulators and `cmd_aggregate`'s count fast path. It was written
twice before; a divergence between them would make `countDocuments` disagree
with the pipeline it is a shortcut for.
`$avg` and `$push` came out of the Tier 0 refusal test, replaced by
`$stdDevPop` and `$mergeObjects`. The refusal is a property of what is missing
rather than of a list, and the test should read that way.
191/191 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e3 16,
e2e4 17, e2e6 72, e2e7 86, crud corpus unchanged at 201/90/196.
|
||
|
A.Shakhmatov
|
3044a38d1c |
tests/spec: an aggregation corpus, recorded from mongod
M2.5's gate, built before the milestone it gates -- the same order that put
`expectEvents` before the free list in M1 and Tier 0 before everything in M2.
`mongodb/specifications` has no aggregation suite, which is amendment A6's
central finding, so this milestone has to bring its own. The hazard in a corpus
we author is obvious and fatal: it can encode our own bugs as expectations and
then agree with us forever. So the split is enforced by the tooling.
`sources/*.json` holds documents and pipelines and nothing else; `record.js`
asks a real mongod 8.3.7 what each pipeline answers and writes the unified-
format file from the reply. Inputs authored, expectations measured -- the
discipline that corrected three assumptions in M1's session work and every
error code in M2, where the alternative would have shipped both times.
No second runner. `run.js --suite-dir` points the existing one somewhere else,
so the entity model, the matchers, the skip accounting and `expectEvents` come
for free; a second runner would drift from the first exactly where it mattered.
`--scorecard` is refused with `--suite-dir`, because `scorecard.txt` is the crud
corpus's record and the milestones are compared against it -- writing it from an
unrelated run would replace that record silently.
Errors record the code and not the message: message text is mongod's to change
between releases. Group pipelines end in a `$sort`, because group output order
is unspecified and a case depending on it would fail for the wrong reason on
either server.
The first source covers `$group`: nine accumulators including the edge cases
that decide an implementation -- `$avg` over a group whose values are not
numbers, `$min` of a field no document has, `$push` skipping a missing field,
`$first`/`$last` against input order, grouping on an array, a compound `_id`.
Where it starts, run against the M2 tip:
group-accumulators.json 9 pass 10 fail 0 skip
The nine include the four refusals M2 added, which answer with mongod's own
codes -- so the corpus already confirms that half. The ten are the milestone.
The crud corpus is unchanged at 201/90/196.
|
||
|
A.Shakhmatov
|
8ebeb9d4ec |
commands: $out and $merge, written by the dispatch epilogue
The seven reachable failures of M2, and the first commit of the milestone to move the scorecard: 194/97/196 -> 201/90/196, with `aggregate-*.json` going 9 pass / 13 fail to 16 pass / 6 fail. The seven that moved are exactly the seven priced as reachable, and the six that remain are exactly the six attributed to M2.5 ($addFields, the expression engine), M4 ($listLocalSessions) and M8 (collation). Both stages write to a collection the pipeline is not reading, and three things stood against doing that in the handler: `aggregate` is a `.read` command, dispatch takes locks from a static table keyed on the command name before the handler runs, and `Collection.lock` allows exactly one collection lock at a time. So the handler computes the output under the locks it has and leaves it in `Context.pending_write`; the epilogue applies it with nothing held, beside the commit and the checkpoint already there. The `.read`/`.write` contract is amended in its own comment rather than quietly broken. `pending_write` is cleared at the top of every dispatch, so a handler that errors before setting one cannot leave the previous command's write to fire. A failed write replaces the pipeline's `ok: 1` with the failure, because a client told the aggregation succeeded would believe the collection had been written. What the stages do not implement is refused, not ignored: `$merge`'s `whenMatched`, `whenNotMatched`, `on` and `let` all select behaviour this server does not have, and a `whenMatched: "fail"` that silently merged would be the same lie Tier 0 spent three commits removing. Codes measured against mongod 8.3.7. `$out` and `$merge` answer byte-identically to it on both the replace and the upsert case. NOT ATOMIC, and said out loud in the code rather than left to be discovered. mongod replaces an `$out` target atomically; this engine has no cross-collection atomicity and no rename to build one from, so a crash between the drop and the last insert leaves the target holding part of the new output where MongoDB would leave the whole of the old. The fix is write-to-temp-and-rename and rename is a command that does not exist here. The test's mutation is the argument for the epilogue in one line: apply the write inside the `$out` branch and it deadlocks rather than fails. 191/191 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e2 concurrent, e2e3 16, e2e4 17, e2e6 72, e2e7 86. |
||
|
A.Shakhmatov
|
6d5c860e11 |
tests/spec: an event's command is compared in the shape it was sent
A command-monitoring event hands over the command as the driver holds it in
memory, and that is not always the shape it puts on the wire: a sort is a JS
`Map` (driver lib/sort.js). `Object.keys` on a Map is empty, so the matcher
reported every key of an expected sort as missing from a command that in fact
carried it -- five cases, all of them the runner's fault and none the
engine's.
This one is worth the paragraph because of how well it hides. EJSON serializes
a Map exactly like a document, so `MFDB_DUMP_EVENTS` prints
`"sort":{"_id":1}` next to a failure that says `sort._id` is missing, and the
dump -- the tool built for exactly this triage in the commit that added the
buffers -- reads as evidence that the matcher is wrong about something else.
It took `Object.keys(formatSort({_id: 1}))` returning `[]` to see it.
Converted for the comparison only, and at every depth, since a sort also
appears inside `updates[i]`. `match` stays a plain reading of the spec's
Evaluating Matches with no driver knowledge in it.
Mutation-checked: pass the event's own value through and
findOne.json "FindOne with filter, sort, and skip" goes red again with the
original message.
189/102/196 becomes 194/97/196.
|
||
|
A.Shakhmatov
|
548c882d47 |
commands: the wire version says what the version string says
`buildInfo` has always reported 4.4.0 and the handshake has always reported maxWireVersion 8, which is 4.2. A driver believes the wire version: it refused client-side to send `hint` on an unacknowledged delete or findAndModify (the error is "only supported on MongoDB 4.4+", raised without a round trip), and withheld `comment` from getMore, listCollections and listDatabases (lib/operations/get_more.js:43 and its neighbours). Both are things this engine handles -- the acknowledged hint suites pass, and getMore ignores fields it does not know -- so the effect was purely the number disagreeing with itself. The 8 was not arbitrary. The comment above it tied it to omitting `topologyVersion`, which is what keeps a driver off the streaming hello protocol we do not implement -- a real bug, once visible as Compass reconnecting every heartbeat. Checked before touching it, in the driver rather than from memory: `useStreamingProtocol` (lib/sdam/monitor.js:154) returns false whenever `topologyVersion` is absent and never looks at the wire version at all. The omission is the whole mechanism; the wire version was a second line of defence that never existed. The comment now says so. A test asserts the two agree, so they cannot drift apart again silently, which is the actual defect here -- not the value. Spec suites 173/118/196 -> 189/102/196: 16 cases fixed, none broken. Ten are the unacknowledged-hint cases the previous commit uncovered, six are `comment` forwarding. 166/166 unit tests in ReleaseFast and ReleaseSafe, 82/82 fuzz, e2e 49, e2e2 concurrent 2 + crash pair, e2e3 16, e2e4 17, e2e6 72, e2e7 86, crash-fuzz 60 cycles. |
||
|
A.Shakhmatov
|
7f426cdd33 |
tests/spec: a collection entity gets the options it was declared with
`buildEntities` built every collection as `db.collection(name)` and every
database as `client.db(name)`, dropping `collectionOptions` and
`databaseOptions` on the floor. 15 collection entities declare a
`writeConcern`, 7 a `readConcern`, one a `readPreference` -- and the 15 are all
`{w: 0}`, so every "unacknowledged write" case in this corpus has been running
an acknowledged write against a driver that was never told otherwise.
They passed anyway, because an acknowledged and an unacknowledged write of the
same document produce results a `$$unsetOrMatches` expectation accepts either
way. Only the command on the wire distinguished them, and nothing was reading
the command until the previous commits. This is the first thing the event
assertions found, and it is a fair answer to what they cost.
Option documents are unwrapped from their BSON types on the way to the driver.
The suites are parsed with `relaxed: false`, so `{w: 0}` arrives as an Int32
and the driver gates `writeConcern.w` on `typeof w === 'number'` -- the same
trap NUMERIC_OPTIONS already documents for operation options, and a silent one:
the option would simply not apply. Wholesale unwrapping is safe here in a way
it is not there, since these are settings the driver consumes rather than
values an assertion compares. An option key outside the spec's
`collectionOrDatabaseOptions` set is reported unsupported rather than ignored,
which is the lesson of the bug itself.
159/132/196 becomes 173/118/196. 14 cases fixed, none broken.
The other 10 unacknowledged cases now fail differently, and that is progress
of a sort: with `w: 0` actually applied, the driver refuses client-side to send
`hint` on a delete or findAndModify to a server older than 4.4. This engine
reports itself as 4.4.0 with maxWireVersion 8, and 4.4 is wire 9. That
inconsistency is ours, it is the same one behind the `comment`-on-getMore
failures, and it gets the next commit.
|
||
|
A.Shakhmatov
|
bb8cdd964b |
tests/spec: the scorecard no longer disclaims expectEvents
The disclaimer was accurate for as long as it stood -- events were not read,
so `pass` was an upper bound and saying otherwise would have been a lie about
the number. It is now a lie in the other direction, so it goes, replaced by
what is actually true: events are compared exactly, in number and in order,
which is what makes a pass mean the engine answered correctly *and* was asked
the right question. The header says plainly that scorecards recorded before
this are not comparable, and enumerates what is still skipped inside events
rather than leaving "asserted" to be read as "asserted completely".
Two facts in the docs had gone stale and are corrected here because this is
the commit that rereads them:
- README said `--op-timeout-ms` defaults to 3 s. It has been 10 s since the
commit that explains, at length and directly above the constant, why 3 s
was wrong. A stale number in exactly the place that warns against
tightening it is worse than no number.
- `MAX_SCHEMA` is [1, 24]; the comment above it still claimed 1.0-1.9.
Totals unchanged at 159/132/196 -- this commit only rewrites prose, and the
scorecard is re-recorded so its header matches the runner that produced it.
|
||
|
A.Shakhmatov
|
54ad124c18 |
tests/spec: a CSOT-rewritten maxTimeMS cannot be asserted
Every client entity is built with CSOT `timeoutMS` (OP_TIMEOUT_MS, 10 s), and CSOT overwrites `maxTimeMS` on each command with what is left of that budget. An expectation of `maxTimeMS: 6000` therefore meets the harness's 10000, and no amount of engine correctness would change it. Reported unsupported rather than failed: a FAIL is a claim about the engine, and this is a claim about the runner. Refused unconditionally when an expected command mentions `maxTimeMS`, not only when the two values differ, so it can never become a pass by coincidence. Exactly one case in the corpus asserts it -- estimatedDocumentCount.json, "estimatedDocumentCount with maxTimeMS" -- so the whole cost of the hatch is one case, which is why it is worth taking instead of dropping `timeoutMS`. That option is not open anyway: `timeoutMS` is what replaced the outer race that once turned ~190 good cases into phantom timeout FAILs. This is the only escape hatch in the runner. Everything else is either an honest FAIL or an enumerated unsupported feature. 159/133/195 becomes 159/132/196: one case, fail to skip, and nothing else moves. |
||
|
A.Shakhmatov
|
97e3e3a556 |
tests/spec: assert expectEvents
The headline is not the delta, it is that `pass` changed meaning. 354 of the
487 cases declare `expectEvents` and until now the runner read none of them,
so a case could send the wrong command entirely and still be counted a pass
as long as the *result* came back right. The old column was an upper bound by
construction. 193/99/195 becomes 159/133/195, and the two numbers are not
comparable.
Two rules decide how far the assertion reaches, both taken from the spec
rather than from what would be convenient:
- `command` and `reply` match as *root* documents
(unified-test-format.md:1020-1022, :1037-1039). The driver hangs `lsid`,
`$db` and `maxTimeMS` off nearly everything it sends; as nested documents
essentially the whole corpus would fail on keys no expectation was ever
written to mention, and the number would say nothing.
- the event list is exact in number and order, not a prefix
(unified-test-format.md:3088-3091). 23 cases expect an empty list and a
prefix rule would pass every one of them without looking.
The assertion runs after the operations, so a wrong result is still reported
as a wrong result rather than being masked, and after the listeners are
disabled, so the teardown's own commands cannot reach the buffer.
`cmap` and `sdam` event types, `ignoreExtraEvents`, and any event field beyond
`command`/`reply`/`commandName`/`databaseName` are reported unsupported at the
point of assertion. None occurs in this corpus -- all 354 blocks are
`eventType: command`, carrying 349 `commandStartedEvent` and 6
`commandSucceededEvent` -- so nothing is being quietly waived.
All 34 newly-failing cases, triaged. Not one is a wrong answer from the
engine; every one is a command the driver never sent:
- 22x `command.writeConcern: missing` -- runner gap, and the sharpest thing
this commit found. `buildEntities` drops `collectionOptions` on the floor,
so `writeConcern: {w: 0}` never reached the driver and every "unacknowledged
write" case in the corpus has been running an acknowledged write. They
passed because the results of the two agree. This is precisely the class of
error the instrument was built to find, and it was invisible to the result
column.
- 5x `command.sort.<key>: missing` -- runner gap. The driver holds a sort as
a JS `Map` (lib/sort.js), so `Object.keys` on it is empty and the matcher
reports every expected key as absent. Measured, not guessed: EJSON prints a
`Map` exactly like a document, which is why the dump looks correct.
- 4x `command.bypassDocumentValidation: missing` -- unclassified. The option
is absent from the wire for the `false` cases; the driver only forwards it
when true on some paths (lib/operations/find_and_modify.js:19), and whether
the runner also drops it has not been established.
- 2x `command.comment: missing` on getMore -- server gap, most likely. The
driver gates it on `maxWireVersion >= 9` (lib/operations/get_more.js:43)
and this engine advertises 8 while reporting itself as 4.4.0, which is
wire 9. The inconsistency is ours.
- 1x `command.maxTimeMS: expected 6000, got 10000` -- the CSOT rewrite, dealt
with in the next commit.
Each of those gets its own commit, and none of them is fixed here: a check and
the fix for what the check caught do not belong in one change.
|
||
|
A.Shakhmatov
|
6560aec915 |
tests/spec: buffer command-monitoring events per client entity
Plumbing only: a client entity that declares `observeEvents` now gets `monitorCommands` and a buffer, and nothing reads the buffer. That is the point of splitting it out -- the totals not moving *is* this commit's test. Command monitoring changes how the driver builds every command it sends, and if that alone shifted a result there would be no way to tell it apart from the assertions landing in the next commit. 193/99/195 before, 193/99/195 after, 175/175 files, 0 errored. The rules the buffer already enforces, so that the next commit is only about comparing: `ignoreCommandMonitoringEvents` by command name; sensitive commands dropped unless `observeSensitiveCommands` says otherwise, with `hello` and legacy hello inferred sensitive from the driver having redacted them to empty documents (unified-test-format.md:3070-3075). Neither fires on this corpus -- 136 client entities observe `commandStartedEvent`, 6 also `commandSucceededEvent`, and not one sets either field -- but a rule that only exists where it is exercised is a rule that will be missing when M7 brings auth. `cmap` and `sdam` observations are collected by nobody; a test that goes on to assert them is reported unsupported where it asserts, not where it declares. Two things about placement, both load-bearing. Listeners are attached after `connect()`, so a client's own handshake is not in its own buffer -- measured rather than assumed: with the buffers dumped, find.json's five cases show exactly `find`, `getMore`, `getMore` and nothing else. And they are disabled after the operations and before the outcome check (unified-test-format.md:3081), plus again unconditionally in the teardown `finally`, because the outcome check and the teardown both issue commands and a buffer still growing through them would make the assertion a function of the harness rather than of the engine. `MFDB_DUMP_EVENTS=1` prints each case's buffer. That is how the handshake question above was settled and how a failing event assertion will be triaged. |
||
|
A.Shakhmatov
|
afa5c6ef9d |
tests/spec: $$unsetOrMatches does not change root-ness
`special()` passed a hard `false` for `root` into its recursion, so a value
standing behind `$$unsetOrMatches` was matched as a nested document even when
it sat at the top of an `expectResult`. The spec says the opposite in so many
words -- "This operator does not influence whether or not an actual document
value is considered a root-level document" (unified-test-format.md:2873, and
:2821 for `$$matchesEntity`) -- and that distinction is the whole of the
extra-key rule: only a root document may carry keys the expectation does not
mention.
`root` now threads through `match` -> `special` -> the recursion. From under a
key it is always false, which is what it already was; from the top level it is
whatever the caller had.
25 cases go from FAIL to pass and none moves the other way. Every one is the
same shape -- an `expectResult` of `{$$unsetOrMatches: {acknowledged: false}}`
against a driver write result that also carries its counts, or the
`insertedId`/`insertedIds` forms of the same thing -- and every one was the
runner failing a result the engine had got right. 168/124/195 becomes
193/99/195; the scorecard is rewritten here so the delta belongs to this
change alone.
Mutation-checked: put the `false` back in the `$$unsetOrMatches` arm and
bulkWrite-deleteMany-hint-unacknowledged.json returns to 0 pass, 2 fail. The
`$$matchesEntity` arm is the same one-word change on the same sentence of the
spec, but the crud corpus does not use that operator once, so it rests on the
spec text rather than on a red test.
Two consecutive full runs, both 193/99/195, 175/175 files, 0 errored, no
lingering timers.
|
||
| f2844e7894 |
cursors: server-side cursors for find, aggregate and the listing commands
Every reply came back in a single batch with `cursor.id = 0`, `getMore` was a
stub answering an empty `nextBatch` on the literal namespace `test.$cmd`, and
nothing read `batchSize`. That caps the useful collection size at what fits in
one 48 MiB message, which is the opposite of the tens-of-GB target and the
reason M0 made whole-index scans stream: the streaming candidate generator
existed with no consumer that could suspend.
## What a cursor is allowed to remember
A cursor holds no lock between requests, so everything it saves has to survive
arbitrary concurrent mutation. Nothing here is a pointer, and the two things
that look like stable addresses are not: `reset_tree` re-creates node ids 0 and
1 as different nodes, and `rebuild_collection` moves every document. Three
sources, chosen by query shape, each with a different memory contract:
- **stream** -- an index-ordered walk resumed from a `(key, off)` anchor plus a
`(leaf, slot)` hint. O(key) state, so this is what lets a cursor walk a
collection larger than memory. Survives a rebuild, because a repack changes
no key.
- **offsets** -- the matched slab offsets a narrowed plan already materialized,
8 bytes each. Killed by a rebuild with `QueryPlanKilled`, because those
offsets now name unrelated bytes.
- **buffered** -- canonical BSON copies, for a sort no index provides and for
aggregate/listing output. Depends on nothing, which is what lets a listing
hold a cursor over a `$cmd.*` namespace no collection backs.
`Collection.layout_epoch` and `Index.epoch` are the invalidation tokens, both
checked as error returns rather than assertions since a client reaches them by
keeping a cursor open across maintenance.
## Resume
`resume_forward`/`resume_reverse` are O(1) while the hint holds and fall back to
an exact-order band walk bounded by `resume_walk_max`. Without the hint, `seek`
lands at the *start* of an equal-key band, so `sort({status: 1})` over three
distinct values across 10M documents would cost ~5e10 comparisons to drain.
Two hazards found by draining a collection while writing to it, neither
predictable from reading the code:
- A deleted anchor must resume at its *band position*, or the rest of an
equal-key band is silently dropped -- most of the collection on a
low-cardinality index. Hence `band_index`.
- On a **unique** index a same-key entry can only be the anchor rewritten, so
resuming at it returned updated documents twice. Observed as duplicate `_id`s
while updating underneath a drain.
## Protocol
Measured against mongod 8.3.7 rather than recalled, which corrected three
assumptions: a bare `getMore` does *not* inherit the find's `batchSize` (4998 of
5000 documents come back), a namespace mismatch is `Unauthorized` (13) not
`CursorNotFound`, and `CursorInUse` is 143 not 12051.
`internalQueryFindCommandBatchSize` is 101, `cursorTimeoutMillis` 600000,
`clientCursorMonitorFrequencySecs` 4.
The rule everything follows is **never look ahead**: a batch that met its target
leaves the cursor open even when the source is in fact exhausted, so four
documents at `batchSize: 2` take three commands. `limit` acts as an EOF source,
which is what makes `batchSize == limit` close in one round trip. `skip` is
consumed once. `batchSize: 0` returns an empty batch with a live cursor.
Cursor ids are `(nonce << 20) | slot`, always positive. The nonce is not
decoration: without it a recycled slot serves one client another's documents.
Cursors are not connection-pinned, since the driver spec allows a `getMore` on
any connection to the same server; they end at exhaustion, `killCursors`, or the
idle sweep (a second monitor fiber, separate from the TTL one because the
cadences differ by an order of magnitude and a TTL failure must not stop
reclamation). The registry is fixed-capacity and evicts the least recently used
cursor, whose client sees the same 43 an idle timeout gives.
Fixed alongside, because cursors are what expose them:
- `listCollections` reported `"<db>."` with an *empty* collection part, which
makes the driver throw client-side -- so it would have broken the moment its
cursor stopped being id 0. Now `<db>.$cmd.listCollections`, as mongod uses.
- `count` ignored `skip` and `limit` entirely.
- `wire.end_message` now bounds a reply by the 48 MiB we advertise rather than
by `maxInt(u32)`; a reply past what we told the client to expect is not a large
reply, it is a desynchronized connection.
- Two `codeName` strings were wrong: 72 is `InvalidOptions` (MongoDB has no
`InvalidArgument`), and 40324 reports as `Location40324`.
## Verification
Unit 160/160 in ReleaseFast and ReleaseSafe; `tests/e2e/e2e7.js` adds 86 cursor
checks across five phases (batching/lifecycle/errors, streaming across churn,
aggregate+listings+count, expiry+capacity, restart) and is self-contained
because cursor behaviour is only observable with non-default flags. No
regressions: e2e 49, e2e3 16, e2e4 17, e2e2 2, e2e6 72. Spec 168 pass / 124
fail, +5 against the previous scorecard.
Mutation-checked, per the repo's second ground rule: `hint_slot + 1`, the
`band_index` off-by-one, both epoch bumps, the id nonce, the at-least-one-
document rule, and `stream_shape` returning null each turn the intended test
red. One claim was withdrawn rather than kept -- swapping `std.mem.order` for
`cmp_prefix` in the band walk changes nothing observable, so the comment now
says so instead of asserting a check that does not hold.
|
|||
| 21489723a9 |
db: a write that changes nothing is not a write
`nModified` counted every write, so an update that altered nothing was reported
as a modification. MongoDB counts a document as modified only if applying the
update changed it, and writes no oplog entry when it did not: `$set: {x: 11}`
on a document already holding `x: 11` is matched and not modified. The spec
suite says it plainly -- `bulkWrite` with four updateOne operations expects
matchedCount 2 and modifiedCount 1.
Decided in the engine rather than the command, because that is where the
document is already serialized: the comparison is against the bytes that would
actually be stored, and it lands before the log append, so a no-op costs no log
record, no fsync, no slab bytes and no garbage. `Engine.replace` returns
`Written.modified` or `.unchanged` and `cmd_update` counts the first.
That exposed a second difference. A replacement keeps `_id` at the front, so
replacing a document with itself was a byte-level change whenever `_id` was not
stored first -- and it usually was not: the Node driver fills a missing `_id` by
assigning the property, which in JavaScript appends it, so `insertOne({name,
age})` reaches the server as `{name, age, _id}` and we stored it that way.
MongoDB moves `_id` to the front whatever order it arrives in. Now so does
`serialize_with_id`, for every document rather than only the ones whose `_id` it
generates. Visible to clients as `_id` coming back first, as it does from
MongoDB.
spec scorecard 161 pass / 131 fail -> 163 pass / 129 fail
bulkWrite.json 8 pass / 2 fail -> 10 pass / 0 fail
e2e.js 45 checks -> 49
No spec file regressed. Mutation: delete the byte comparison in `upsert`'s
`.replace` arm -- red on the log growing, on the garbage counters moving, and on
`replace` claiming `.modified`.
|
|||
| 53f88e6d3b |
update: replacement-style writes
`replaceOne`, `findOneAndReplace` and `bulkWrite`'s `replaceOne` all failed
with "bad update". `update.apply` rejected any update document whose first key
was not `$`-prefixed, so a replacement document -- which by definition has no
operators -- could not get through at all.
MongoDB decides on the first field and nothing else: `$`-prefixed means
operators, anything else means the document *is* the new content. An empty
document is a replacement too, and a legal one. `is_replacement` says which,
`apply_replacement` does the work, and because all three call sites already go
through `apply`, that one branch covers the update command, findAndModify and
the upsert builder.
What a replacement means, precisely:
- every field is replaced except `_id`, which is immutable and keeps its
position at the front, where it is stored and where the `_id_` index
descends on it;
- a replacement may restate the same `_id` but not a different one -- that
is `ImmutableId`, because otherwise a rewrite would silently change a
document's identity while the index entry kept the old key;
- when the target has no `_id` yet, the replacement supplies it. That is the
upsert path: `build_upsert_doc` seeds a document from the filter's
equalities, so `replaceOne({_id: 99}, {u: 1}, {upsert: true})` inserts
`{_id: 99, u: 1}` and not a generated ObjectId;
- a mixed document is refused from either side, rather than guessed at.
Two options on update specs are refused rather than ignored:
- `multi` with a replacement (FailedToParse). A replacement describes one
document; applying it to many would leave every match identical apart from
its `_id`.
- `sort`, a MongoDB 8.0 addition this server does not implement. Ignoring it
is the worst of the three answers -- `sort` chooses *which* match to write,
so the client would silently get a different document than it asked for.
spec scorecard 131 pass / 161 fail -> 161 pass / 131 fail
e2e.js 35 checks -> 45
Sixteen spec files improved and none regressed. The two `-sort` files briefly
did: they had been passing on their "server-side error" case, which our
"bad update" failure satisfied by accident, and passing for the wrong reason is
how a gap survives a scorecard.
Five mutations, each verified red: seeding the replacement from the old pairs,
dropping the `_id` comparison, dropping the `_id` a replacement supplies, and
removing the mixed-document guard from either loop.
Note `nModified` is still wrong for a write that changes nothing -- MongoDB
counts a document as modified only if applying the update altered it. That is
the remaining bulkWrite failure and is fixed next, separately.
|
|||
| 90de7820da |
tests/spec: MongoDB spec-test runner and the M0 scorecard
PLAN D2 makes the official specification suites the gate for command semantics; D7.6 asks for the harness to exist at M0 with a recorded baseline. This is that harness, pinned on both sides -- mongodb/specifications @ 615e0f9 and mongodb@7.5.0 -- because a scorecard is only comparable across milestones if a delta cannot be an upstream test change. It implements the unified format's Evaluating Matches algorithm as written, including the two rules that decide whether a pass is earned: extra keys are tolerated only in a root document, and numeric types compare flexibly. Anything unimplemented is a SKIP with a reason, never a pass, and the one assertion class not yet checked -- expectEvents, i.e. command monitoring -- is disclosed at the top of the scorecard so `pass` reads as an upper bound. First honest run: 131 pass, 161 fail, 195 skip over 175 files, zero timeouts. Getting there took four attempts, and the failures are documented in the README because each would have shipped a scorecard claiming a compatibility gap that did not exist. Two were genuine leaks in this runner (clients left open when a case timed out; clients registered for cleanup only after `await connect()`, plus abandoned cases still creating more). The third I misdiagnosed as machine load. The fourth attempt found the real cause: a leaked catalog lock in the engine, fixed separately, which alone accounts for the jump from 45 passes to 131. So the runner carries its own guards: per-operation CSOT timeouts so work is never abandoned, an active-handle census per file, an end-of-run tripwire for stray timers, a hard stop if the server dies rather than emitting hundreds of misleading ECONNREFUSED failures, and --skip/--limit for bisecting a run whose failures depend on position. The README states the rule plainly -- a long unbroken tail of timeouts is a harness bug until proven otherwise -- and the two commands that settle it. Also fixes bench-run.sh, which copied its report over bench-latest.txt unconditionally, including after a run that only warned -- so a degraded run could silently replace the baseline that PLAN D7.5 makes a milestone gate. |
