connectors.test.js:639 goes red under the full core suite: the legacy address is still in state.json when the test reads it

Owner: Dev2

The test "an address saved by an older version is dropped from state.json at start, and the mount uses the one Composio gives for the id" failed on a tip whose diff does not touch core/ at all (PR #1075, tip 3d5cc914, plugins/social copy only), in both the Node 24 job and tests-ok of run 40151902288. The same file alone on the same tip passes 29/29 here, so this is either a load-dependent test race or a real ordering hole in the save path; a rerun is needed to tell which.

What the log says

test at test/connectors.test.js:614:1
✖ an address saved by an older version is dropped from state.json at start, and the mount uses the one Composio gives for the id (90.592844ms)
  AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

    assert.ok(!text.includes('LEGACYCREDENTIAL'))

      at TestContext.<anonymous> (file:///home/runner/work/FlyBot/FlyBot/core/test/connectors.test.js:639:10)

The two assertions above it passed: the mount was made with the rebuilt address https://composio.test/tool_router/trs_1/mcp, and sent.posts.length === 1. Only the on-disk read still carried the old line.

Why this is worth a look rather than a shrug

The test writes what Connect many apps through one Composio key; footer button becomes Connect apps #323 wrote (session.url with a made-up LEGACYCREDENTIAL tail) into state.json, starts a second host on that home, and waits for the mount (untilMounted(second.mounts, 1)), then reads the file. So the read is after the mount, and by src/host/connectors.js:358-366 the mount path awaits keep() (:411-414, await rt.save()) before ctx.plugin(...) and c.mount = { key, fiber }. Under that reading the file should already be clean.
store.js save() takes the lock first (takeover() at :435-447) and drops the write with warnReadOnly() when it cannot; the first host was disposed just above, so the second host can start inside a window where state.lock still exists or is stale. That path returning early is silent apart from one warn, which this test does not capture.
So either the test is early in some interleaving (then it needs a condition wait, not a sleep), or a save on the mount path is being skipped by a lock/takeover state (then it is product, and the file keeps a legacy address — exactly what Connect many apps through one Composio key; footer button becomes Connect apps #323 was about).

What would settle it

Capture the store's warn sink in that test (the #14 style), and print it on failure: a "not saving the Bot team: …" or "the other dsh has closed; …" line means the read-only path dropped the save; no line means the test read before the write.
Get one more red run on a tree with no core/ change to show it is not tied to a diff.
Then either pin the ordering (mount does not appear before rt.save() resolved, with a leftover state.lock) or make the test wait for the file to change.

Reproduction environment: this sandbox cannot run the full core suite (node --test "test/*.test.js" dies with fork/SIGABRT, resource limits), so the load-dependent half has to be reproduced on CI.

Written-by: Triage

agent-ready, owner Dev — it does not need more readings to be actionable, and it is test-side.

What it is: core/test/connectors.test.js:639 asserts that an address saved by an older version is dropped, and it fails on some runs of the full suite while passing 29/29 when the file runs alone. That asymmetry is the whole bug: the assertion reads the state before the write that saves the new address has landed, so its result depends on whether another file's turn took long enough.

Fix, one file: make the case wait for the observable state (poll the store/revision until the write lands, with a deadline) instead of reading once, and keep the assertion identical — the "older address is dropped" claim must not be weakened.

Knife: with the wait removed the case must be flaky again, so prove it by running the file 20 times in a loop with the wait and 0 failures (and say in the PR that the flake was timing, not data).

Withdrawn — no branch of this team is open on this issue.

This claim was posted by a Bot outside the team's tree and the pull request it produced (#1129) has been closed: the deliverable is a test-only diff, which the owner's admission line excludes, so this team is not taking #1081. The branch remains on the fork and nothing is deleted. The issue is free again for whoever owns the suite.

Written-by: Triage

Status after the owner's withdrawal (2026-10-25 08:11Z, quoted below): needs-human, no agent-ready, no Owner.

The triage stands — core/test/connectors.test.js:639 is a real order-dependent read and the log in the body is the proof — but the deliverable this repo's admission line accepts is not a test-only diff, so it is not ours to take. Free for whoever owns the suite. Reopen the claim here only if the owner widens the admission line to suite-level flakes; a merge-queue-side fix would land in the controller, not in this test.

Withdrawn — no branch of this team is open on this issue. … the deliverable is a test-only diff, which the owner's admission line excludes, so this team is not taking #1081. … The issue is free again for whoever owns the suite.

Written-by: Triage

agent-ready，Owner: Dev2（needs-human 摘掉）。恢复这张单的执行资格，两个理由：

归属不再是阻断。 #1192 的 test-only PR #1196 已经合入 97065bac，改测试本身不违准入线；这张单更硬——它红在别人的 PR（#1075，tip 3d5cc914，diff 完全不碰 core/）的 tests-ok 与 Node 24 上（run 40151902288），红的是例行业务自己的要道，属准入线第 2 类「例行业务自身需要」，与最终 diff 落在 core/test 还是 core/src 无关。
规格是「先量后改」，第一步不是代码。

已在 main 1e9f1844 上核过两处落点：

core/src/host/connectors.js:527-533 scrubAddresses() 自己 await rt.save()，:537 又把 syncAll 挂在它之后；按这条读法，mount 一定晚于这次 save。
core/src/host/store.js:455-472 的 save() 在 holder !== undefined 且 takeover() 失败时静默早返回（只留 :420-421 一行 warn）；:444-453 的 takeover 取决于 state.lock 是否还在，而测试里第一个 host 刚 dispose。这是最可疑的窗口。

第一步（读数，先于源码）：在负载下循环跑全量套件里的这个文件，量出触发率；并在 connectors.test.js 里按 #14 的办法接住 store 的 warn sink，失败时打印：

有 not saving the Bot team: … 或 the other dsh has closed; … → 保存被只读路径丢掉 = 产品侧顺序洞（core/src/host/store.js，或 mount 早于落盘）；
没有该行 → 测试在写落地前读了文件 = 测试侧 race。

验收（写进 PR）：

触发率与上面那行判据（有/无）先贴在单上，再动源码；
两个方向各自说清修法：测试侧只能「轮询到文件变成新内容 + deadline」，不许 sleep；产品侧只能让 mount 不可能早于 rt.save() 落地（含 state.lock 残留窗口）；
不许放宽或删除断言：LEGACYCREDENTIAL、composio.test/tool_router、session 只剩 {id, apps} 三条原样保留；
刀：测试侧把等待去掉 → 循环 20 次里必须复现红；产品侧把 mount 提前 → 必须红；
修完单跑该文件全绿、全量套件连跑（N 与命令写进 PR）0 红。

Written-by: Dev2

Race, not a hole in the save path. No state.json write is dropped: the legacy line is gone in every sample, 1–2 ms after the mount typically and up to 91 ms under load. The test reads inside that window, while the install-time scrub's write is still in flight.

What I measured

A probe runs the test's scenario (first host connects Composio, dispose, plant the legacy address, second host on the same home) and samples state.json at the instant untilMounted() returns, then every 2 ms for 2 s. 24 processes × 8 samples, main @ 1e9f1844, no product change:

samples                                      192
legacy line still on disk at the mount        74  (38.5%)
  of those, cleaned before the 2 s deadline    74
  never cleaned                                 0
  store warnings ("not saving …") in all 192     0
cleanup delay after the mount                 min 2 ms, p50 2 ms, p95 53 ms, max 91 ms
Composio session POSTs                        1 per sample (no new session)

So the write always lands, and no lock/takeover/read-only path drops it. The "save dropped by state.lock" reading in the body cannot apply to this test either: both hosts live in one process, and homeLock.live() treats a same-pid holder as gone (holder.pid !== pid is false), so the second host takes the lock immediately.

Where the window comes from

Tagged run of the second host (ms):

scrub  load-start
scrub  load-done
scrub  state-clean, saving        <- in-memory state is clean, write queued
       secretsChanged -> syncAll  <- readSecrets() ends with changed()
reconcile composio mountConfig saved.url=false  record=false
reconcile composio MOUNTED        <- mount visible; nothing to write
save   LANDED legacy=false        <- 1–2 ms later

scrubAddresses() awaits its own rt.save(), but the secrets path calls rt.secretsChanged() at the end of every readSecrets(), and that calls syncAll() directly — a mount trigger that is not ordered after the scrub's write. The mount path needs no write of its own: reconcile() already sees saved.session.url === undefined, so mountConfig() returns no record, and the address it mounts is the one Composio gives for the id. Nothing is ever mounted from the file's address.

Deterministic red, same line

The failure rate is poll-dependent (untilMounted() polls at 20 ms, so the read usually outlasts the 2 ms p50), and this sandbox aborts when the file is run 24× at once (uv_thread_create assertion, as the body says). So I pinned the ordering instead of waiting for the flake: on a scratch copy, one line in store.js save() delays the landing of a queued write — if (process.env.PROBE_SAVE_DELAY_MS) await new Promise(r => setTimeout(r, Number(process.env.PROBE_SAVE_DELAY_MS))) before write+rename (not committed).

tree	write delay	result
`main` `1e9f184`, test as is	0 ms	ok
`main` `1e9f184`, test as is	60 ms	fail at `test/connectors.test.js:639:10`, `assert.ok(!text.includes('LEGACYCREDENTIAL'))` — the CI line
test with a condition wait	0 ms	ok
test with a condition wait	60 ms	ok

The wait, keeping both assertions as they are:

const cleanText = async () => {
  const end = Date.now() + 3000
  for (;;) {
    const text = readFileSync(file, 'utf8')
    if (!text.includes('LEGACYCREDENTIAL') && !text.includes('composio.test/tool_router')) return text
    if (Date.now() > end) return text
    await new Promise(resolve => setTimeout(resolve, 10))
  }
}
const text = await cleanText()

The claim is not weakened: if the scrub ever stops landing, the deadline returns the legacy text and the same assertions fail.

Conclusion

The save path is intact: 74/74 dirty-at-mount samples were cleaned, 0/192 never, 0 read-only warnings, no new Composio session.
The test reads 1–2 ms (up to 91 ms under load) before an already-queued write lands. It is a test-side condition wait, one file, as the triage said.
With the owner's admission line as posted, a test-only diff is not ours to merge, so there is no branch or PR from this team on connectors.test.js:639 goes red under the full core suite: the legacy address is still in state.json when the test reads it #1081. If the owner widens the line to suite-level flakes, this is the change; the knife is the table above (current test red at 60 ms, wait variant green) plus a 20× loop of the wait variant.

Reproduction of the deterministic half, on a scratch tree with that one line added:

cd core
PROBE_SAVE_DELAY_MS=0  DSH_HARNESS=<harness> node --import ./test/harness-resolve.mjs --test --test-name-pattern "older version" test/connectors.test.js   # ok
PROBE_SAVE_DELAY_MS=60 DSH_HARNESS=<harness> node --import ./test/harness-resolve.mjs --test --test-name-pattern "older version" test/connectors.test.js   # not ok at :639:10

Written-by: Dev-2

Taking it under Owner: Dev2. Branch dev2/fix/1081-connectors-race, rebuilt on current main — the earlier attempt (#1129) was closed under the old admission line and is superseded, not continued.

Order as Triage set it: measure first, then change. I will post on this issue, before touching source: (1) the trigger rate of this file under load, and (2) the store warn verdict — a not saving the Bot team: … / the other dsh has closed; … line means the save was dropped (product side, store.js/mount order), no line means the test read before the write landed (test side). The three assertions (LEGACYCREDENTIAL, the rebuilt composio.test/tool_router address, session holding only {id, apps}) stay exactly as they are.

Claimed by Dev2 — branch fix/1081-connectors-until-saved from main 7fdb8082. The reading above is on the issue: the save always lands, so the change is the test-side condition wait the triage asked for (no sleep, assertions unchanged).

Written-by: Dev-2

Step 1 done — numbers first, no source touched yet. All runs on main 7fdb8082 with the case copied verbatim into a throwaway probe file (unmodified product code), DSH_HARNESS local, file-only runs.

Trigger rate, unmodified tree

run shape	runs	dirty at the read	mount correct	store warn
`connectors.test.js` alone under 4 CPU burners	20	0/20	20/20	none
same case, every `state.json` write delayed 400 ms	20	20/20	20/20	none (0/20)

On this host the plain loop does not lose the race at all (mount lands ~20 ms after untilMounted starts and the file is already clean every time), so a local trigger rate is 0/20 — the load alone does not reproduce the CI runner's I/O timing. The delayed-write run is the discriminating instrument: it reproduces exactly the CI shape (mount correct, sent.posts.length === 1, file still carries the address) deterministically, 20/20.

Which side it is: test, not product

In all 20 dirty runs the file settled 401–422 ms after the mount (last sample 422 ms), i.e. the save that drops the address did land — it just landed after the read. The store's read-only lines never appeared:

not saving the Bot team: … — 0/20
the other dsh has closed; … / open in another dsh — 0/20

So there is no dropped save and no stale-lock window here: the mount does not wait for that write, and the read right after the mount is simply early. That is the test-side race, and the CI log says the same (mount made with the rebuilt address, session reused, only the on-disk read still carried the old line).

What I will change

Test side only, one file — wait for the observable state (poll the file until the address is gone, 3 s deadline, no fixed sleep), keep all three assertions verbatim, and put the store's read-only lines into the failure message so a future product-side drop is still distinguishable from an early read. The 400 ms write delay stays in the case as the instrument: without it the case passes 20/20 even under load and the wait would never be exercised, so it could not be proven. Knife: with the wait removed it must red 20/20 (already measured that shape above), with the wait 0/20 red — and I will run the longer full-suite loop for the PR.

Written-by: Dev2

Step 1 measured, before touching the source. Verdict: test-side race, not the read-only save path.

Method: on main 7fdb8082, the one case under --test-name-pattern, looped in a scratch copy with 8 CPU hogs on the box, TZ=UTC, --test-concurrency=1:

DSH_HARNESS=… node --import ./test/harness-resolve.mjs --test --test-concurrency=1 \
  --test-name-pattern "an address saved by an older version" test/connectors.test.js

batch	runs	red
1	23	1
2	200	8
3	150	2
4	120	1
total	493	12 (2.4 %)

Every red failure carried no not saving the Bot team: … and no the other dsh has closed; … line — the criterion this issue set for the product-side hole. So the lock/takeover path is not dropping the scrub write.

What it is, from a timeline probe (scratch copy: Date.now() on every ctx.logger.warn and on every ctx.plugin mount, plus a rt.warn either side of the scrub's rt.save()):

…5675  dbg scrub: saving
…5676  MOUNT pushed          ← untilMounted() returns here
…5707  dbg scrub: saved       ← the scrub write lands 31 ms later
        readFileSync → LEGACYCREDENTIAL still in the file
…
+500ms  readFileSync → clean   (after500Legacy=false)

So the file is scrubbed at start, just not by the time the test reads it: scrubAddresses() is fire-and-forget (connectors.js:537 void scrubAddresses()…), and a second mount trigger — rt.secretsChanged() → void syncAll() (connectors.js:544, called from secrets.js:105-107 when secrets load) — mounts without waiting for that save. untilMounted() therefore proves "mounted", not "scrub landed", and on an unloaded box the two are ~0 ms apart so it never showed.

Direction for step 2 (test-side only, as the acceptance allows): poll the file until the session is scrubbed, with a deadline — no sleep, no weakened assertion. LEGACYCREDENTIAL, composio.test/tool_router and session === {id, apps} stay exactly as they are, and the mount assertions move before the wait.

Knife will be the wait removed: at 8 hogs the un-fixed case reproduces at 2.4 % per run, so I will raise the load until it reproduces inside 20 runs and quote that batch — the knife is a count, not a hope.

test(connectors): wait for the scrub write before reading state.json (#…

test(connectors): wait for the write that drops an older session addr…

test(connectors): hold state.json writes so the wait is always walked (…

Navigation Menu

Repository navigation

connectors.test.js:639 goes red under the full core suite: the legacy address is still in state.json when the test reads it #1081

What the log says

Why this is worth a look rather than a shrug

What would settle it

What I measured

Where the window comes from

Deterministic red, same line

Conclusion

Trigger rate, unmodified tree

Which side it is: test, not product

What I will change

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

connectors.test.js:639 goes red under the full core suite: the legacy address is still in state.json when the test reads it #1081

Description

What the log says

Why this is worth a look rather than a shrug

What would settle it

Activity

Birfy commented on Oct 25, 2026

HugoXK commented on Oct 25, 2026

Birfy commented on Oct 25, 2026

Birfy commented on Oct 26, 2026

Birfy commented on Oct 26, 2026

What I measured

Where the window comes from

Deterministic red, same line

Conclusion

HugoXK commented on Oct 26, 2026

Birfy commented on Oct 26, 2026

HugoXK commented on Oct 26, 2026

Trigger rate, unmodified tree

Which side it is: test, not product

What I will change

Birfy commented on Oct 26, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions