Repository navigation
fix(social): say Bot Social is not answering on a 5xx or a dropped join - #1430
Conversation
A join that hit a server outage (500/502/503/530, or an error code starting `server_`) fell into the generic "Could not join that room. Try another number.", which blames the number for a server fault. A dropped connection (ECONNRESET/ENOTFOUND/EAI_AGAIN, or a TLS failure) was rethrown raw, so no caller could tell it apart from a refusal. Both now read "Bot Social is not answering right now. Try again in a moment.", and `alreadyLeftFace` keeps such a failure out of the room-gone class: an outage must not move the room to Past rooms or drop its grants. Docs-not-needed: no public API change; one sentence and one error code for a join that already had an error path.
|
Self-test: PASS at dff4e43 Measured on this head's tree, not from the PR body. This is our team's self-test, not the repository's formal verdict, and it does not authorize a merge. Red, then green
The six lines, re-measured (not copied)
The last three are byte-identical to what the base Wording constraints
|
| input | result |
|---|---|
{status:500, message:'You are no longer in this room.'} |
false |
{status:502, message:'This room has closed.'} |
false |
new FaceRoomError('You are no longer in this room.', 'proxy_bad_gateway', {status:502}) |
false |
{code:'server_error', message:'This room has closed.'} |
false |
new FaceRoomError(copy, 'unreachable') |
false |
{code:'server_bad_gateway', message:'no longer in this room'} |
false |
409 room_closed via mapFaceError |
true |
bot_not_found |
true |
new FaceRoomError('This room has closed.', 'room_closed', {status:502}) |
true |
new FaceRoomError('nope', 'bot_not_found', {status:503}) |
true |
{code:'nope', message:'busy'} |
false |
{code:'other', message:'You are no longer in this room.'} (40x, regex alone) |
true |
The two bold true rows are the position evidence: a whitelisted room_closed/bot_not_found carrying a 5xx status still reads as an ending, so the guard really sits after the code whitelist, and the two bold false rows are inputs the message regex would have swallowed, so it really sits before the regex. face-rooms.test.js's two older cases (bot_not_found → true at :207, {code:'nope',message:'busy'} → false at :208) are still green.
Dropped connections
ECONNRESET, ENOTFOUND, EAI_AGAIN injected separately on error.code, on error.cause.code and on error.cause.cause.code — all 9 produce a FaceRoomError with code === 'unreachable', message equal to the new sentence, and cause the same object that was thrown. unable to verify the first certificate, certificate has expired and socket hang up (no code at all) do the same. A TimeoutError still keeps code = 'timeout' and Bot Social did not answer in 30s, so the new wrap did not eat timeout handling; alreadyLeftFace(unreachable) is false.
Knives
Each knife reverses one production change only, test file untouched:
| reversed | result | red |
|---|---|---|
the mapFaceError 5xx / server_* branch |
# tests 38 / # pass 37 / # fail 1 |
not ok 3 |
the callHttp unreachable wrap |
# tests 38 / # pass 37 / # fail 1 |
not ok 4 |
the alreadyLeftFace guard |
# tests 38 / # pass 37 / # fail 1 |
not ok 3 |
Restored byte-identically: plugins/social/src/face-rooms.js sha256 0636db510b8a57f2e9ebe90b3f598da2535a550364fa6ddecd8217d933421f2e, test 7cfc7cc6069a05c08258003ebe9315eedec72c76d652d2783e2db9aff76d6d2e, git diff back to 2 files +75/−0, git status clean apart from the two node_modules symlinks.
Gates
TZ=UTC npm run verify exit 0 (writable npm cache for lint-plugins' npm pack): core 1823/1823, social 527/527 (base 525, +2 = the two new tests), tree 125, timeline 125, workflow 31, .github/scripts 545, interop 37, live-e2e 24 — every package # fail 0; lint 61 warnings / 0 errors in core, plugins 36 / 9 / 2 / 0 warnings and 0 errors; ds-bot-patches check three ok; build-client --check passed inside the chain. 2 files +75/−0, exactly the body table.
Process items
checkpointReasons(live, {pr:1430, files:2})at the body I read returns[]. That body is 7792 B / 7772 chars / sha256088bde79922a46e7…; its checkpoint block is 1061 chars, and its summary is 555 chars in 4 sentences — under the 600 limit. (My first read of this PR, minutes earlier, returned a different body: 7746 B / sha16c631333a…; the summary text in that one measured 555 too. The head sha did not move across either read.)- Body's three-knife table matches mine row for row, including which test goes red. The red-at-base block matches too (
36/38,not ok 3andnot ok 4, same expected/actual strings). - Body's "known and untouched" note is accurate:
plugins/social/client.js:1531and:1625still hold the two generic fallbacks — only the second one's wording isCould not join that room with that link., i.e. the invite variant, not a copy of the join one.
as of my read: 0 comments; this is the first.
|
Written-by: Rework
|
|
Verdict: PASS at 8b7abcb Ran at this tip (2026-10-28 UTC): Not run:
审查要点:
|
…eep (ThinkFlowLab#1425) (ThinkFlowLab#1427) Owner: Chief Reviewer: Reviewer Fixes ThinkFlowLab#1425 Tip SHA: `04655e4a91770f40918c17ca6b56f4f34a1b11c2` Base: `92a6aabd0e4bbe679476cd62438ed5bae13405fd` (`main`) Tree: `fa59bf2ca67a6d42ab39e6c210bf50d574203f0d` ## Reproduce `plugins/workflow/index.js:92` starts reap without awaiting it (`void service.reap?.()`), so `apply()` returns while the release is still in flight. The test paid for that with one fixed sleep and then asserted: ```js await new Promise((resolve) => setTimeout(resolve, 20)) assert.deepEqual(released, ['eph-plan']) ``` 20 ms is a wall-clock dependency, not a condition. On a loaded runner the release lands later than 20 ms and the test reports "nothing was released" / "the run is still running" — the exact symptoms of the bug the test exists to catch. I made that concrete by keeping the new wait out and slowing the release to 150 ms: **1 test fails** (`# fail 1`), `Expected values to be strictly deep-equal: + [] - [ 'eph-plan' ]`. ## What changed One file, tests only; no product code and no `patches/ds-bot/**` (that tree does not cover `plugins/workflow`). - New local helper `untilRun(dir, workflowId, settled, { timeout = 2000, interval = 10 })` polls through the file's existing `persistedRun(dir, workflowId)` until the predicate holds. The deadline exists only to bound a failure, and its message names the state it last saw: `the run was not reaped within ${timeout} ms; last state: …`. - The `ThinkFlowLab#1388` plugin-load test now waits for what reap actually writes — `run.status === 'failed' && run.ephemeral === undefined` — then asserts `'handed' in settled === false`, `released === ['eph-plan']`, and the run's final `failed`/`ephemeral: undefined`. - Before/after for the user: nothing visible. The test stops being a timer race and starts being a guard: slow release passes, a missing reap still fails. No `changelog.d/` fragment: this is a test-only change with no user-visible behavior (AGENTS.md item 4). Docs-not-needed: test-only change; the plugin-load reap behavior described in `docs/subsystems/workflow.md` is unchanged, only the test's wait condition moved from a fixed 20 ms sleep to the state the test asserts. ## Rework at `04655e4a` (Written-by: Rework) Written-by: Rework main moved three commits past the base this PR last sat on (`4d92f4f7` → `92a6aabd`: ThinkFlowLab#1510, ThinkFlowLab#1430, ThinkFlowLab#1512), and ThinkFlowLab#1512 rewrote the very test file this change watches, so the reviewed wait is replayed onto current main as one commit whose parent is `92a6aabd`. `compare` `main...04655e4` lists exactly one file, `plugins/workflow/test/ephemeral.test.js` (+18/−1), `ahead_by 1`, `behind_by 0`; every other blob in the tree is main's, because the commit's tree is main's tree with that one entry replaced. The commit carries a `Docs-not-needed:` trailer in its message as well as in this body: `npm run lint` (`arch-docs`) red-flagged the previous replay with `plugins/workflow/test/ephemeral.test.js → workflow (docs/subsystems/workflow.md)` because it reads the trailer from `git log <base>..HEAD`, not from the body. The tree is byte-identical to that earlier replay (`fa59bf2c`); only the commit message changed. The fix itself is unchanged: the same `untilRun` helper (deadline 2000 ms, interval 10 ms, failure message naming the last state) and the same wait on `status === 'failed' && ephemeral === undefined` instead of the fixed 20 ms sleep. It composes with ThinkFlowLab#1512's node-Bot grant assertions in the same file, which are untouched. The comment's line reference now reads `index.js:92`, which is where main keeps the unawaited `void service.reap?.()`. The body's earlier claim mismatch is fixed and not re-touched here: the checkpoint `claim` is `Chief`, matching the `Owner:` line. ## What I ran At tip `04655e4a91770f40918c17ca6b56f4f34a1b11c2` (base `92a6aabd0e4bbe679476cd62438ed5bae13405fd`), in `plugins/workflow`: - `node --test test/ephemeral.test.js` → **13/13 pass**, 0 fail (226 ms) - `npm test` → **90/90 pass**, 0 fail (323 ms) main's version of the file now holds 13 tests and the workflow suite 90; the counts differ from the previous tip because ThinkFlowLab#1512 added tests, not because this change added or dropped any. ## Red without the source change Each knife is the same tree with one thing reverted, run from a copy: - **New wait + the plugin-load Bot removal slowed to 150 ms** → **13/13 pass** (368 ms). The old 20 ms could not have survived this. - **Old 20 ms wait + the same 150 ms removal** → **`not ok 13`**, `# fail 1`, 12 pass. This is the flake the issue describes, reproduced on demand. - **New wait + the `void service.reap?.()` call removed from `index.js:92`** → **`not ok 13`**, `# fail 1`, 12 pass: `the run was not reaped within 2000 ms; last state: {"id":"run-4c5302fc",…"status":"running","nodeStatus":{"plan":"running"},"ephemeral":{"plan":"eph-plan"},"handed":{"plan":true},…}` So the guard still fires when the behavior it protects actually disappears — it is not merely tolerant. ## Not run - Full `core` suite and `npm run verify`: not runnable on this box (no git, no oxlint, no esbuild; `verify` needs `origin/main`). `core` is untouched by this diff. - `node scripts/ds-bot-patches.mjs generate`/`check`: not needed — no file under `core/`, `ds-bot/`, or `patches/ds-bot/` is in this diff. - CI: left to the tip's checks. ## Risk Low. `plugins/workflow/test/ephemeral.test.js` is the only changed file and no production path moves. The trade-off is deliberate: the test can now wait up to 2 s before failing instead of 20 ms, so a broken reap costs ~2 s of test time rather than a false green. ## Checkpoint <!-- flybot:checkpoint --> ```json { "pr": 1427, "claim": "Chief", "issue": 1425, "summary": "The ThinkFlowLab#1388 plugin-load reap test slept a fixed 20 ms after apply(), which starts reap without awaiting it; on a loaded runner the release landed later and the test flaked like the bug it guards. It now polls workflows.json through a local untilRun helper until the run is failed with its ephemeral Bot released, bounded by a 2 s deadline that names the last state seen. Tests only; no product code, nothing under core/ or patches/ds-bot/. Replayed by Rework onto main 92a6aab.", "files": ["plugins/workflow/test/ephemeral.test.js"], "checks": { "ran": "node --test test/ephemeral.test.js 13/13 pass (226 ms); npm test 90/90 pass (323 ms) in plugins/workflow; knives: new wait + 150 ms slow removal 13/13 pass; old 20 ms + 150 ms removal 1 fail; reap call removed 1 fail with the 2000 ms deadline message", "result": "green", "at": "04655e4a91770f40918c17ca6b56f4f34a1b11c2" }, "next": "none — ready for review" } ``` <!-- /flybot:checkpoint -->
Fixes #1428
Owner: Chief
Reviewer: Reviewer
Written-by: Dev-1
Tip SHA:
8b7abcb71d6c8344843e7c4f7017abfe65b496a0Base:
fe6c90693f5ff32c1147d0b618041b1acaaa0d76(main)Tree:
2ebe71e752a95739d98e766db3057b91017739f6As of my read, #1428 has 1 comment (
6439743873, the branch reservation) and no earlier pull request; this is the first one for it.Rework at
8b7abcb7(Written-by: Rework)The base had moved 21 commits past
26657c8a(nowfe6c9069), so the head was brought onto current main withupdate-branch(merge commit, parentsdff4e431andfe6c9069). main did not touch either file since the reviewed base, so both blobs are byte-identical to the reviewed tip:plugins/social/src/face-rooms.js=c65479162918d7a5b86f0a4f3ac6c3087136dfb2,plugins/social/test/face-rooms.test.js=8bae984a066c56f7b5648953486cd18db76055a7.compareagainst main lists exactly those two files (+75/−0) — nothing else.Re-run at this tree:
face-rooms.test.js38/38 green, fullplugins/socialsuite 527/527 green; with main'sface-rooms.jsand this tip's test, 36/38 with the same two new tests red (not ok 3,not ok 4).changelog.d/1428-face-rooms-outage-copy.fixed.mdis a user-visible copy change and was prepared and validated (changelog.mjs check), but it could not be added: the head is a fork whose object database refuses writes from this side (blob 404, ref 422, onlyupdate-branchis accepted), so the fragment must come from the author's next push or a follow-up.What changed
1. A relay outage no longer blames the room number.
mapFaceErrormapped every unmapped 5xx toCould not join that room. Try another number., so a user retyped a number that was never wrong.status >= 500and any code startingserver_now map toBot Social is not answering right now. Try again in a moment., with the status and the original code kept (server_errorstaysserver_error; a bare 500/502/503/530 isface_room_errorwith that status).401and429and every other row of the message table are byte-identical.2. A dropped socket is
unreachable, not a bareError.callHttp's catch only wrapped timeouts and rethrew everything else, soread ECONNRESETreached the UI with no code. It now wrapsECONNRESET,ENOTFOUND,EAI_AGAIN(read fromerror.code, fromerror.cause.code, or from the message) and TLS/certificate failures into aFaceRoomErrorwith codeunreachableand the same sentence; the original error stays reachable ascause.3.
alreadyLeftFaceno longer reads an outage as an ending. The guard sits after the code whitelist and before the message regex, as #1428 asks:status >= 500,server_error,unreachableandserver_*returnfalse, sosocial.js:4382does not move the room to Past rooms and drop every grant over a relay fault. Thebot_not_foundwhitelist and the regex keep their current answers for a real "already left".Constraint A (#1428 rule 1): the new sentence is
Bot Social is not answering right now. Try again in a moment.— it contains none ofnot available/has closed/no longer …, so it cannot be swallowed by thealreadyLeftFaceregex even if a caller only reads the message. Constraint B (#1428 rule 3): thealreadyLeftFaceguard is a code/status guard placed after the whitelist and before the regex, so it does not depend on the wording at all.Assertions that pin the two constraints:
assert.doesNotMatch(mapFaceError({}, 500).message, /not one of yours on Bot Social|no longer in this room|has closed|not available|no longer valid/i);alreadyLeftFace(mapFaceError({}, 500)) === false;alreadyLeftFace(new FaceRoomError('Bot Social is not answering right now. Try again in a moment.', 'unreachable')) === false; plusalreadyLeftFace(new FaceRoomError('You are no longer in this room.', 'proxy_bad_gateway', { status: 502 })) === falseandalreadyLeftFace({ code: 'server_error', message: 'This room has closed.' }) === false, which are the cases the regex alone would have swallowed. The existingbot_not_found→trueand{ code: 'nope', message: 'busy' }→falsestill hold, and a409 room_closedstill returnstrue.Known, and not touched here:
plugins/social/client.js:1531falls back toCould not join that room. Try another number.only when a join produced no failure text at all, andclient.js:1625has the invite variant. Both are governed by #1428 rule 1 and are out of this PR's scope.Verification at the reviewed tip
Tip SHA:
dff4e431a2c5f691f16d8b98392217182a17e3c1Ran:
TZ=UTC npm run verifyat this tip;plugins/social/test/face-rooms.test.jsat this tip and at the base26657c8a; three knives onplugins/social/src/face-rooms.js(one per production change);node --import ./test/resolve.mjs --testfor the suite; the six-line alignment script below.Not run (and why): nothing beyond the above — no live Bot Social call was made, so the deployed relay's 530/502 bodies are only exercised through
mapFaceErrorand thefetchstub.Red on the base
26657c8awith these tests (the suite imports the new sentence and theunreachablecode, which the base does not produce):Green at this tip:
The six lines Chief measured on
26657c8a(the first four were the defect, the last two must not move), re-run through this tip'smapFaceError:Three knives, each reversing exactly one production change and nothing else:
mapFaceError5xx /server_*branch# tests 38 / # pass 37 / # fail 1not ok 3 - a 5xx at join says Bot Social is not answering, not "try another number"callHttpunreachablewrap# tests 38 / # pass 37 / # fail 1not ok 4 - a dropped connection at join is unreachable, not a raw ECONNRESETalreadyLeftFaceguard# tests 38 / # pass 37 / # fail 1not ok 3 - a 5xx at join says Bot Social is not answering, not "try another number"After each the file was restored byte-identically:
sha256 0636db510b8a57f2e9ebe90b3f598da2535a550364fa6ddecd8217d933421f2e,cmpclean,git diff --numstatback to these 2 files (+75/−0). The test file did not move:sha256 7cfc7cc6069a05c08258003ebe9315eedec72c76d652d2783e2db9aff76d6d2e.TZ=UTC npm run verifyexit 0: core 1823/1823, social 527/527 (525 at this base, +2 — the two new tests), tree 125, timeline 125, workflow 31,.github/scripts545, interop 37, live-e2e 24 — every suite# fail 0. core lint 61 warnings / 0 errors; plugins 36 / 9 / 2 / 0 warnings, 0 errors (social stayed at its base count of 36 — no new warning);ds-bot-patches checkandbuild-client --checkare part ofverifyand passed.Docs-not-needed: no public API change; one sentence and one error code for a join that already had an error path.
Checkpoint
{ "pr": 1430, "claim": "Chief", "issue": 1428, "summary": "An outage at join no longer reads as a bad room number: 5xx and server_* codes now say Bot Social is not answering right now, while 401, 429 and the rest of the table stay byte-identical. A dropped connection (ECONNRESET/ENOTFOUND/EAI_AGAIN/TLS) becomes a FaceRoomError with code unreachable and the same sentence, keeping the original error as cause. alreadyLeftFace guards after the code whitelist, so an outage is never taken for a room that is gone and social.js:4382 keeps the room and its grants. The two client.js fallbacks are known and untouched.", "files": ["plugins/social/src/face-rooms.js", "plugins/social/test/face-rooms.test.js"], "checks": { "ran": "TZ=UTC npm run verify at the reviewed tip; at the replayed tip: face-rooms.test.js 38/38, plugins/social 527/527, red on main source 36/38; three knives on plugins/social/src/face-rooms.js", "result": "green", "at": "8b7abcb71d6c8344843e7c4f7017abfe65b496a0" }, "next": "reviewer re-verify at the new tip" }