Replies: 13 comments
|
Two more from the same audit — the vendored line is a separate track that has not taken several upstream fixes. Both below are reproduced the same way: one probe, run against the shipped package and against upstream 1.
|
update() returns |
dropped call | |
|---|---|---|
| as shipped | undefined — no promise to await or catch |
unhandledRejection: 'boom' |
upstream 4.0.0-rc.10 |
rejects with boom |
nothing leaked |
Unhandled rejections terminate the process by default, so a plugin that fails while reloading takes the host down with it. Entry.init() in the same vendored package already carries the matching .catch(() => {}) from that commit, so only the Fiber.update half is missing.
2. waterfall runs its continuations twice — 5b195b3 (guard waterfall continuations, #44)
cordis/src/events.ts has no next() called multiple times guard. A listener that calls next() twice runs the rest of the chain again instead of failing.
Probe: two listeners, the first calling next() twice, plus a terminal.
| downstream calls | thrown | |
|---|---|---|
| as shipped | 3 | none |
upstream 4.0.0-rc.10 |
2 | next() called multiple times |
Silent double execution is the worse half: a listener that reaches next() on two paths (an early return and a fallthrough, say) doubles whatever the rest of the chain does rather than reporting it.
Where this leaves the audit
Confirmed missing so far, all verified by probe:
| upstream | what it fixes |
|---|---|
1b7d0f2 |
await nested reconciliation during config update |
2ceea23 (#109) |
dropped Fiber.update failure becomes an unhandled rejection |
5b195b3 (#44) |
waterfall continuation run twice |
Still source-level only, not reproduced: c594d1a (#121, include journal + the EntryGroup.remove order and guard) and c2835d8 (#123, bare specifier resolution).
I have the probes for the three above as standalone scripts and can paste any of them if that helps.
|
Three more, same method — one probe per item, run against the shipped package and upstream 3. Concurrent
|
| waiters | as shipped | upstream |
|---|---|---|
| 2 | times out | both resolve |
| 3 | times out | all resolve |
for await over ctx.interval(delay) in two places at once is the shape that stalls — one of them never sees a tick.
4. An event named after an Object.prototype member throws — 1c1a10e (dispatch symbol and prototype-named events, #51)
cordis/src/events.ts initialises the hook table with {} where upstream uses Object.create(null):
_hooks: Record<keyof any, Hook[]> = {}
...
const hooks = this._hooks[name] ||= []this._hooks['toString'] finds the inherited Object.prototype.toString instead of undefined, so the ||= never fires and hooks is a function:
ctx.on('toString', () => {}) → TypeError: hooks[method] is not a function
Upstream registers it normally. Same for constructor, valueOf, hasOwnProperty.
5. The logger's bounded buffer is replaced, not truncated — fd96b0a (logger exporter dispose, #36)
self.buffer.push(message)
if (self.buffer.length > self.bufferSize) {
self.buffer = self.buffer.slice(-self.bufferSize)
}Assigning a new array leaves every existing holder of ctx.logger.buffer pointing at the old one, which keeps growing without bound. Upstream truncates in place; the test it added is named "keeps the bounded buffer in place and chronological".
const held = ctx.logger.buffer // what a log panel would hold
ctx.logger.bufferSize = 2
for (let i = 0; i < 6; i++) ctx.logger.info('msg' + i)
held.length // 3 and climbing, vs 2 upstream
ctx.logger.buffer.length // 2 either wayThe same commit's exporter-dispose half is present here already, in a ctx.effect form, so only the buffer half is missing.
Running list
| upstream | what it fixes | verified |
|---|---|---|
1b7d0f2 |
await nested reconciliation during config update | probe |
2ceea23 (#109) |
dropped Fiber.update failure becomes an unhandled rejection |
probe |
5b195b3 (#44) |
waterfall continuation run twice |
probe |
cb77029 (#53) |
concurrent interval() reads never settle |
probe |
1c1a10e (#51) |
Object.prototype-named events throw on registration |
probe |
fd96b0a (#36) |
logger buffer replaced instead of truncated | probe |
Source-level only, not reproduced: c594d1a (#121) and c2835d8 (#123). Still checking 8abd903 (#35), be7d36e (#37), 4cfd19a and 752dbee (#40) — those four look like the same features implemented differently rather than omissions, and I would rather confirm that than report them as missing.
|
Closing out the four I said I was still checking — the outcome is better than the raw diff suggested. Present, just implemented differently — the shadow/traceable machinery is all here under its own shape (
Genuinely absent — the identifier does not occur anywhere in
Final tally for this thread
Every one of the six carries a probe that runs against the shipped packages and against upstream |
|
补丁:https://github.com/173787247/dsh-cordis-backport —— README、三个补丁、六条探针及其运行说明。仓库同时提供中文说明(补丁部分、探针部分)。 The patches are in that repository too — README, the three patches, and the six probes with run instructions. A Chinese translation of the README is available there. Backports for the six are written and verified. Since this repository does not take pull requests, here is what they contain and what they do. Three patches:
Each row is one probe run before and after. All three patches apply cleanly to untouched 4.0.4 sources. One thing worth flaggingIn - ctx.on('internal/update', (config) => {
- this.update(config)
- })
+ ctx.on('internal/update', config => this.update(config))It is not. The first returns Not included, deliberately
|
|
One more, on a different subject: a look at the part of this tree with no upstream counterpart — the Result: it holds up. Everything is in One missing guard did turn up, and the reachability argument is the interesting part: function isSchemastery(schema: Plugin.Runtime['Config']): schema is Schema {
return schema?.['~standard'].vendor === 'schemastery'
}A non-nullish if (!runtime.Config) return config
const result = runtime.Config['~standard'].validate(config)Two missing guards mask each other. Fixing either one alone makes the other live. That is why this is written up rather than sent as a patch — a fix has to cover both, and it is a small robustness question rather than something anyone is hitting. Same class as upstream's open #141 / issue #102, incidentally: upstream's For completeness, one behaviour recorded rather than asserted, because nothing relies on it and whether it should is a design call:
|
|
Different subject again: I have been fuzzing the core state machine rather than diffing versions, and it found something upstream that reading never would have. Upstream bug, PR #175: a fiber disposed before it ever activates keeps its disposables forever. Two open PRs already touch this (#133, #140). Both add The part that is relevant to you: the vendored line in this repo already handles this correctly, and its comment is where the reasoning came from —
So this is a case of the vendored line being ahead, not behind. Nothing to do here. Four seeded invariant fuzzers are in One caveat I want to state rather than bury. Run against Two of my invariants were also wrong to begin with — effect counters incremented beside |
|
A correction to my earlier summary, and a seventh item for the list. Earlier in this thread I closed out What it does
const fiber = new Fiber(...)
const wrapped = Object.create(fiber) as Fiber & PromiseLike<Fiber>
Upstream dereferences at the top of both methods instead, which is the whole of that commit: const fiber = this.ctx.fiber
fiber.assertActive()What it costsA ten-line reduction, deterministic: const db = root.provide('b', 1)
const fiber = root.inject(['b'], async () => { await sleep(1); return () => {} })
await sleep(10)
fiber.update({ v: 1 }) // <- without this line both lines agree
await sleep(20)
db() // withdraw bThe cached field disagrees with the epoch, nothing writes to It needs the Why the first pass missed itThe audit checked whether the identifiers a commit introduces are present. Every name That generalisation is the useful part for anyone reviewing this line: on a codebase with a wrapper/prototype indirection, "does this identifier exist" tells you nothing about whether a fix is present. StatusAdded to the backport set — Updated: https://github.com/173787247/dsh-cordis-backport — the correction is written up in |
|
The blind spot from the last correction was worth chasing. Re-checking the commits I had marked present with a behavioural test instead of a name check turned up two more — one a fix, one a confirmed divergence I am deliberately not patching. Eighth fix:
|
| upstream | verdict |
|---|---|
10194de (#98) |
missing — now patched |
be7d36e (#37) |
missing — confirmed by upstream's own test, not patched |
8618a49 (#29) |
present — has execute: function () and runner.execute.call(this) |
8487fc5, 2df12b5, 542728e |
present |
4cfd19a |
same machinery, still unresolved |
So of the items I had cleared, two were wrong. Both for the same reason: I had been checking whether the identifiers a commit introduces exist here, and both of these change control flow or the receiver of an assignment instead. That check cannot see either. The list in the top-level README is corrected, and the patch set is at eight.
|
Last item on the list, and it closes the audit window. I had left Upstream's test for it, ported verbatim: class FooBar extends Service { constructor(ctx) { super(ctx, 'foo.bar') } hello() { return 'bar' } }
class Baz extends Service {
static inject = ['foo', 'foo.bar']
probe() { return this.ctx.foo.bar.hello() }
}
class Qux extends Service {
static inject = ['foo'] // injects foo, not foo.bar
probe() { return this.ctx.foo.bar.hello() }
}The first two lines are the part that matters. Both of the remaining unpatched items — this and The window is now fully adjudicatedAll sixteen upstream
Nine is the number in the patch set; the READMEs carry the corrected list, and the two unpatched items are written up with their upstream tests rather than as prose. |
|
The last two items are done. I said earlier that I also owe a correction on the framing. I wrote that porting it "is a refactor of the shadow machinery, and half of it is worse than none". The second half was right — shipping an unverified half would have been worse. The first half was me judging the size by reading the diff instead of counting it. It is 54 lines in What it fixesHow it was validatedNot with my own probes only. The same 87 core tests from upstream, run twice — once with the vendored Five fixed, and no test that passed before fails after. That second half is the part I cared about, so I compared the failing test names rather than the counts: The five that still fail are all
|
|
A twelfth fix, and this one is the most user-visible of the set. I found it by reading your own
|
|
Going back over That file states the rule: "Keep this log exhaustive — every divergence from upstream must be listed." The twelve fixes have been sitting in a patch set and a probe suite, which is the code, but the thing your sync procedure actually consumes is the numbered log. So I have written the twelve as entries 23–34 in the log's own format: https://github.com/173787247/dsh-cordis-backport/blob/main/for-dsh/vendor-log-entries.md Each entry follows Three things worth flagging about the content: Two entries need files the log does not otherwise track. Entry 31 (the Entry 30 is complementary to your entry 20, not an alternative. Your entry 20 fixes the wrapped-fiber defect at the loader's Entry 33 (the shadow rework) is the one to review most carefully. It is a ~65-line port across The patch set stays as the code half: |
|
A closing note, because the most useful thing I found this round was not another defect — it was one of your own documents.
|
Uh oh!
There was an error while loading. Please reload this page.
Summary
A config update returns before the nested entries it created have been reconciled, so
await loader.update(...)resolves while the new child is still being imported.Upstream cordis fixed exactly this in
1b7d0f2— fix(loader): await nested reconciliation during config update (2026-09-03), which also added a test for it inpackages/loader/tests/group.spec.ts. The vendored line in@deepseek-ai/cordis-plugin-loaderdoes not have that change.Where
cordis-plugin-loader/src/config/entry.ts,Entry._patchContext:The method is not
asyncand neither the waterfall norfiber.updateis awaited, so all three call sites proceed immediately. Upstream made the method async and awaits both:Reproduction
The 20 ms import is only there to make the gap observable; the ordering is wrong regardless of how slow the import is.
barenabled"2"fiber@deepseek-ai/cordis-plugin-loader(as shipped)falsecordis@4.0.0-rc.10trueSame probe against both, unchanged.
Why it matters here
The upstream test spells out the consequence: "without the awaited chain the update resolves while the new child is still being imported." Anything that awaits a config change and then uses the entries it just declared is racing — enabling or disabling plugins, and reloading a config file, both go through this path.
Two more upstream changes the vendored line does not have
Not part of the same defect, but they come from the same window and touch the same files, so I checked them while I was in there:
#121—c594d1afix(include): reconcile file and runtime edits through a journal.cordis-plugin-includehere has no journal at all, and the same commit also reorderedEntryGroup.removeso the entry is unregistered before the fiber is disposed — with a comment explaining that theinternal/pluginhandler tells "removed by the loader" from "disposed itself" by checking whether the entry is still in the store. The vendoredremovestill disposes first and omits theentry.parent !== thisguard the same commit added.#123—c2835d8fix(loader): resolve bare specifiers from the project. There is noresolve.tsin the vendored loader, so bare specifiers in a config file resolve against the loader rather than the project.I have not reproduced the second one, so I am reporting it as a source-level difference rather than a confirmed defect.
Related
I have been sending fixes for some of the same files upstream — #164–#172 and #173, #174 — several with regression tests, if any of this is easier to act on with a patch attached.
All reactions