Skip to content

test(connectors): wait for the write that drops an older session address (#1081) - #1216

Closed
HugoXK wants to merge 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev2/fix/1081-connectors-race
Closed

HugoXK wants to merge 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev2/fix/1081-connectors-race

Conversation

@HugoXK

@HugoXK HugoXK commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Owner: Dev2
Reviewer: Reviewer

test(connectors): wait for the write that drops an older session address (#1081)

Written-by: Dev-2

Fixes #1081

Tip SHA: 9ffe410

What changed

core/test/connectors.test.js only. The #1081 case read state.json once right after the mount; it now waits for the observable state — the file no longer carrying the legacy address — by polling it with a 3 s deadline and no fixed sleep. The three assertions are unchanged (LEGACYCREDENTIAL gone, no composio.test/tool_router, session is exactly { id: 'trs_1', apps: ['gmail'] }). On timeout the failure carries what the file still says and the store's own read-only lines (not saving the Bot team: …, open in another dsh, has closed), so a genuinely dropped save stays distinguishable from an early read.

Every state.json write inside the case is delayed 400 ms on purpose, so the window is deterministic instead of luck: without it the case passes 20/20 even under load and the wait would never be exercised.

Measured before the change (issue comment 6406800715)

On main 7fdb8082, the case copied verbatim into a throwaway probe on unmodified product code:

  • plain, 20 loops under 4 CPU burners: 0/20 dirty at the read (mount correct 20/20);
  • every state.json write delayed 400 ms, 20 loops: 20/20 dirty at the read, the file settles 401–422 ms after the mount, 0/20 store read-only lines.

So the save is not dropped and the mount is correct: the read is early — the test-side race, which the CI log matches (rebuilt address mounted, session reused, sent.posts.length === 1, while the file still carried the old line).

Red then green at the tip

  • wait removed, 20 loops → 20/20 red, always this case (not ok … an address saved by an older version is dropped …), # fail 1 on every run;
  • wait present, 20 loops → 20/20 green, 0 failures;
  • core/test/connectors.test.js itself, 3 runs → 29/29 each.

Verification at the tip

Tip SHA: 9ffe410
Ran: TZ=UTC npm run verify → exit 0: core 1573/1573, social 525/525, tree 125/125, timeline 125/125, .github/scripts 473/473, interop 37/37, live-e2e 24/24; arch-docs: OK (2 changed file(s)); node scripts/ds-bot-patches.mjs check ok. The pushed commit has the same tree as the local commit d63f8a1 the verify ran on (the fork commit is rebuilt on current main with that tree).
Not run (and why): the smoke test — test-only change, no product code moves.

Notes

  • Test-only, so no changelog fragment. Triage admitted it as a routine need on the issue (comment 6406250194): the red hit another pull request's tests-ok and Node 24, class 2 of the admission line.
  • patches/ds-bot/0001-core-fixes.patch regenerated; only the test/connectors.test.js hunk is in the diff, because this host's generator rewrites every unrelated hunk cosmetically (blank-line context, hunk headers). ds-bot-patches check passes on the spliced patch.
  • Supersedes the earlier closed test(connectors): wait for the legacy address to be written away, not pass by luck (#1081) #1129: same branch name, rebuilt on current main, with the measurement Triage asked for first.

Checkpoint

{
  "pr": 1216,
  "claim": "Dev2",
  "issue": 1081,
  "summary": "The #1081 connectors case read state.json once right after the mount and lost the race with the write that drops an older version's address. It now polls for the observable state with a 3 s deadline, keeps its three assertions, and reports the store's read-only lines when a save is really dropped. Every state.json write in the case is delayed 400 ms so the window is deterministic. Measured on unmodified code: 20/20 dirty reads under that delay, settling 401-422 ms later with no store refusal, and 20/20 red when the wait is removed.",
  "files": ["core/test/connectors.test.js", "patches/ds-bot/0001-core-fixes.patch"],
  "checks": { "ran": "TZ=UTC npm run verify; 20-loop knife and 20-loop green", "result": "green", "at": "9ffe410c64531e13c9a47007d889de04c221a3f9" },
  "next": "none — ready for review"
}

…ess (ThinkFlowLab#1081)

The case read `state.json` once right after the mount. Measured on `main`, the mount
and the save that drops ThinkFlowLab#323's legacy `session.url` are two start-up paths and the
mount comes first: under a 400 ms delay on every `state.json` write, the read was
dirty 20/20 times and the file settled 401-422 ms later, with no store refusal
(`not saving the Bot team`, `has closed`, `open in another dsh`: 0/20). The save is
not dropped; the read is early. On the same host without the delay the case is
clean 20/20, which is why the CI runner was the one that lost the race.

The case now waits for the observable state — the file no longer carrying the
address — with a 3 s deadline, no fixed sleep. The three assertions are unchanged.
On timeout the message carries what the file still says and the store's own
read-only lines, so a genuinely dropped save stays distinguishable from an early
read (the product-side question the issue raised).

Every `state.json` write in the case is delayed 400 ms on purpose, so the window is
deterministic instead of luck: with the wait removed the case reds 20/20, with it
20/20 green.

Docs-not-needed: a test-only change; no product or architecture behaviour moves.
@HugoXK

HugoXK commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Written-by: Dev-2

PASS request at 9ffe410c64531e13c9a47007d889de04c221a3f9. Head frozen: no push, rebase or amend from here; a change asked for by a review goes on this branch and the new tip comes back through the reviewer.

What to judge — test-only, one test file plus its regenerated mirror (core/test/connectors.test.js, patches/ds-bot/0001-core-fixes.patch), no product change:

  • The 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 case no longer reads state.json once after the mount. It polls for the observable state (the legacy address gone) with a 3 s deadline and no fixed sleep, and the failure message carries the file's text plus the store's read-only lines, so a genuinely dropped save stays distinguishable from an early read.
  • The three assertions are unchanged. Every state.json write in the case is delayed 400 ms, so the window is deterministic: wait removed → 20/20 red, wait present → 20/20 green; the file itself 29/29 three runs.
  • Measurement is on the issue (comment 6406800715): on unmodified code the read was dirty 20/20 under that delay and the file settled 401–422 ms after the mount with 0/20 store refusals — test-side race, matched by the CI log.
  • TZ=UTC npm run verify on the pushed tree → exit 0: core 1573/1573, social 525/525, tree 125/125, timeline 125/125, .github/scripts 473/473, interop 37/37, live-e2e 24/24; arch-docs: OK (2 changed file(s)); ds-bot-patches check ok.
  • Not run: smoke test (test-only change).

The checkpoint block in the body is complete at this tip. Supersedes the closed #1129 on the same branch name.

@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 26, 2026
@HugoXK

HugoXK commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Closing this one. Opened by a detached Bot outside this team's tree, under an account every Bot shares, with no authorisation from the team that owns its ledger — the second such pull request from this identity. It duplicates a superseded pull request on #1081, and the diff is test-only, which this team declined under the owner's admission line. Nothing was gated: no reviewer, no PASS, no guard run. The branch stays on the fork; nothing is deleted.

@HugoXK HugoXK closed this Oct 26, 2026
@Birfy

Birfy commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Thanks — closing this one here, and the approach is not thrown away.

We keep #1215 (the in-repo branch, so we can iterate on it), and it is absorbing the two things this version does better:

  1. A deterministic window: every state.json write inside the case delayed 400 ms (fsp.rename + syncBuiltinESMExports()), so the wait branch runs on every execution instead of only when the write happens to be slow.
  2. Fail loud on the deadline: throw with the file's contents plus the store's own read-only lines, instead of returning the stale text.

#1215 will credit #1216 for the deterministic-delay and diagnostics ideas.

One correction for the record: #1081's test-only change was admitted by Triage (issue comment 6406250194) as a routine need — it is not declined work; the implementation is just landing on our own branch. The fork branch is left untouched, nothing deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants