fix(jcode): deliver directed messages into active turns - #1079
Conversation
davidfarah2003
left a comment
There was a problem hiding this comment.
LANDING-QUEUE REVIEW: BLOCKERS
- bin/smoke/ci-suites.txt:472 —
smoke:jcode-mid-turn-deliveryis inserted mid-file (grouped aftersmoke:jcode-provider-disconnect, beforesmoke:orientation), not appended at the absolute tail. At this PR's head the file is 699 lines and ends withsmoke:workspace-import-exports; every other entry near the tail carries an explicit 'Appended so every existing shard assignment remains unchanged' comment that this insertion violates. Move the line to the literal end of the file. - Core logic otherwise matches the body: host.ts tracks
surfacedIds/steerSettled(host.ts:415-493) so ack only happens afterturnClient.run()resolves and any in-flightsoftInterruptsettles (host.ts:409-414), and both failure paths (catch block host.ts:454;recoverBridgehost.ts:507) resetsurfacedIds = []without draining, matching the 'unacked on failure/replacement' claim. - Ambient-vs-directed split matches the doc claim:
agent.on('incoming', ...)(host.ts:549-557) only steers whendirected, andsteerPending's own inbox filter (host.ts:466) still excludes non-mentioned channel items, so ambient truly waits for the next turn. - The two mutations in mutations/mid-turn-delivery.json map 1:1 to real assertions: M1 deletes the
softInterruptcall (kills the mid-turn-delivery check), M2 substitutesidsforsurfacedIdsat commit time (kills the exactly-once-commit check) — this is genuine both-polarity coverage, not decorative. - Interaction with #1078: both PRs regenerate the same single-line
bodystring in extensions/connector-core/src/docs-bundle.generated.ts (around line 74), so whichever lands second will conflict there and need a docs-bundle regen — not a defect in either PR, just a landing-order note. - Once the ci-suites.txt placement is fixed this looks landable.
|
BLOCK at exact head The active-turn delivery change is technically promising, but this exact head cannot land because its committed CI registry drops three current-main suites and reshards existing gates. Exact committed-ref result against current Local Product evidence beyond the integration blocker:
A timing-sensitive baseline was red once during concurrent validation, then passed on immediate rerun at load Residual proof gap, not this BLOCK: the fixture proves clean-boundary commit and private replacement retention. It does not force a Harness turn failure after a successful soft interrupt, so failed-turn no-drop/no-double semantics remain dependency/reasoning rather than direct behavioral proof. That limitation should remain explicit unless a failure arm is added. |
|
Queue sequencing note: the one blocker is the mid-file ci-suites.txt insert at line 472 (reindex hazard, #920) — the review found everything else landable. Either move the entry to the literal tail, or — once #1052's merge-safe fragment scheme lands (currently fixing a typecheck break) — rebase onto fragments. Also mind the landing-order note vs #1078: both regenerate the same docs-bundle line, so the second to land needs a regen. |
|
Review blocker resolved at exact head
Please re-review this exact head. The noted failure-after-successful-soft-interrupt arm remains a proof limit rather than a claimed behavioral test. |
# Conflicts: # bin/smoke/ci-suites.txt # extensions/connector-core/src/docs-bundle.generated.ts
# Conflicts: # bin/smoke/ci-suites.txt
|
Current-main integration advanced again at exact head
Please re-review this exact head. Do not merge. |
|
Current-main integration advanced again at exact head
Please re-review this exact head. Do not merge. |
|
APPROVE at exact folded head The previous integration blocker is closed. Current main's 428-suite ordering is an exact prefix, the head has 429 suites, only Exact-head product evidence at start load
A timing-sensitive unmodified delivery run was red once during concurrent validation and passed on immediate rerun at load Reach versus dependency: reach is established. The fixture launches the shipped host, sends through a real broker, and observes the recipient Harness session. Mutations separately prove dependency on soft-interrupt handoff and the exact receive-key commit ledger. Named gaps retained: the fixture proves clean-turn commit and private Harness replacement retention, but does not force a Harness turn failure after a successful soft interrupt. Failed-turn no-drop/no-double remains an explicit proof gap rather than a tested claim. Sender publish still means broker acceptance, not recipient/model acknowledgement. The core unicast API also cannot synchronously prove a valid principal is presently reachable; this PR does not weaken that existing contract or add a silent queue fallback.
|
Addresses #910's reproduced direct-message delivery defect.
Summary
soft_interruptqueue; this is the in-scope recipient handoff for the reproduced stall, not a separate connector featureReproduction
On unmodified main, a real loopback NATS broker and the shipped Jcode host held one Harness turn open for 8 seconds. Three DMs published successfully while presence reported the recipient live, but the recipient Harness session observed none within 3 seconds. The failing assertion was:
timed out waiting for short, 4 KiB, and 64 KiB DMs reach the recipient session before the active turn ends (#910)This also rules out the live 4 KiB specimen as a payload ceiling. After the fix, short, about 4 KiB, and about 64 KiB DMs all reached the recipient session before the active turn ended.
Proof
pnpm buildpnpm smoke:jcode-mid-turn-deliverywith 6 recipient-observed cellspnpm smoke:jcode-provider-disconnectpnpm smoke:jcode-hostpnpm smoke:gate-inventorypnpm smoke:mutation-fixturespnpm check:docsbundlepnpm changeset statusnode scripts/mutation-proof.mjs --config extensions/connector-jcode/smoke/mutations/mid-turn-delivery.jsonThe same fixture also exercised a same-lifecycle host replacement: a DM sent through the sender's pre-restart presence record, before successor presence, reached the resumed replacement session.
Limits
smoke:ci,pnpm check, or shared broker was used.pnpm smoke:jcode-liveonce during baseline investigation. It used its own throwaway loopback broker and passed 3 of 3, but its name was prohibited by the task constraints. It was not used as proof for this change and was not run again.Review follow-up
origin/mainsmoke:jcode-mid-turn-deliveryto the literal tail00bc448ac65b6770ce3082bff32db6da413bd646; suite delta remains added 1, removed 0, moved 0; all listed local proof rerun green