Skip to content

fix(routing,session): real session-affinity fix \u2014 D1 + D2 + D3 (Oracle two-stage verified) - #28

Merged
i1hwan merged 4 commits into
mainfrom
feat/session-affinity-real-fix
May 9, 2026
Merged

i1hwan merged 4 commits into
mainfrom
feat/session-affinity-real-fix

Conversation

@i1hwan

@i1hwan i1hwan commented May 8, 2026 •

Copy link
Copy Markdown
Owner

What

3.8.5/3.8.6 production observation: a Claude OAuth session bound to Account A switched to Account B only 4 seconds after the last successful Account A request, on the same opencode-supplied x-session-affinity header, losing 147k tokens of cache_write to a from-scratch re-cache on the new connection. PR #26's smart-affinity (v8) was supposed to prevent exactly this. Oracle hard-debug (ses_1fa7165c0ffeFFU8rjU82y0ItO) identified three independent design defects, all fixed here.

Root causes (Oracle-confirmed)

D1 — x-session-affinity was being silently ignored

extractExternalSessionId previously only matched x-session-id / x_session_id / x-omniroute-session / session-id. The opencode CLI sends a stable x-session-affinity: ses_... continuity signal, but ApexRoute fell through to a fingerprint hash of the request body (system prompt + first user message + tools). Any sub-agent invocation, dynamic context injection, or system-prompt drift produced a NEW fingerprint sessionId with no bound connection — selectByEarliestResetFirst then made a fresh score-based selection, which under tight scores can prefer the alt account. Cache lost.

D2 — touchSession was silently overwriting connectionId on every call

The handler chain calls touchSession(sessionId) early (chat.ts:188) and then touchSession(sessionId, credentials.connectionId) post-credential (chat.ts:548); the strategy also calls touchSession(sessionId, selected.id) in its fall-through (earliestResetFirst.ts:649). Each of those silently rewrote the bound connection. There was no observable signal when affinity broke — the previous binding evidence was always erased by the latest write. /api/sessions showed all 7 sessions on Account B even though the oldest was created 6 seconds before the first Account A request.

D3 — Cache-sensitive break path

affinity_break_p1_too_urgent can fire even at high quota when reset-time / backoff / degraded penalty pushes one account's score 3× + 250 above the bound's. For Anthropic Claude OAuth — where prompt cache write is 1.25× and read is 0.1×, so a 147k-token cache rewrite is roughly 12× more expensive than a single read — that break is actively harmful: the urgent-alt would have to amortise over 12+ reads in 5 minutes to pay for itself, which a typical chat workload cannot.

What this PR changes

D1 fix: x-session-affinity is a first-class external session header

extractExternalSessionId now matches it, between the generic forms and the OmniRoute-specific alias. Each header source gets a hyphenated namespace prefix (ext-xsa: for x-session-affinity, ext-omr: for x-omniroute-session, legacy ext: for the generic forms) so a legacy generic value of e.g. xsa:foo cannot collide with a newer x-session-affinity: foo request. opencode's continuity signal now flows through to bound-conn lookup; fingerprint fallback only kicks in when no header is present.

D2 fix: touchSession and bindSessionConnection are split

touchSession(sessionId) is now a pure activity update (lastActive, requestCount). All connection binding mutations go through bindSessionConnection(sessionId, connectionId, { source }) which returns a BindResult with five reasons:

no_session_id | first_bind | no_change | rebind_within_window | rebind_after_window

A within-window rebind emits a structured log.warn("SESSION/affinity", "session rebound within affinity window", { ... }) — this is the diagnostic alarm signal that lets us answer "did affinity break?" deterministically going forward. The alarm is suppressed for source: "emergency_fallback" because the chat.ts retry path is a legitimate within-window rebind. Critically, null oldConnectionId (touchSession created the entry, then bindSessionConnection makes the first real binding) is classified as first_bind, NOT rebind_within_window, so normal session creation does not raise the alarm.

The deprecated 2-arg touchSession(sessionId, conn) remains as a one-shot-warned compat shim for tests/legacy callers.

D3 fix: cache-sensitive provider gate on the urgent-alt break

selectByEarliestResetFirst(candidates, sessionId, provider?) and isAffinityValid(conn, sessionId, scoredAlternatives?, provider?) now plumb the in-scope provider from auth.ts. For provider === "claude", the urgent-alt heuristic break (affinity_break_p1_too_urgent) is suppressed unless boundScore <= 0 (genuinely unusable via accumulated backoff/error penalty — in which case the existing boundScore <= 0 branch lets any positive alt escape). Other break paths (terminal, rate_limited, quota_exhausted, low-quota < 15%, session_expired, affinity_window_passed) are unaffected. Other providers retain the original break behaviour.

Trace logging

Every routing decision now emits a structured log:

  • log.debug("AUTH/affinity", "kept", { sessionId, provider, boundId }) — affinity preserved
  • log.warn("AUTH/affinity", "broken", { sessionId, provider, oldBound, decision, reason, boundScore, bestAltScore }) — affinity broken via heuristic
  • log.debug("AUTH/affinity", "urgent-alt break suppressed by cache-sensitive gate", { gate: "claude_urgent_alt_suppressed", ... }) — Claude gate fired
  • log.debug("AUTH/affinity", "fall-through selected", { sessionId, provider, selected }) — no session bound

Combined with the bindSessionConnection rebind warning, the previously invisible silent overwrites are now first-class observable events.

Tests

Total 2874/2874 PASS (was 2852; +22 net).

File Coverage
tests/unit/session-manager-affinity-headers.test.mjs (NEW, 9 tests) namespace per source, cross-header non-collision (incl. legacy values that look-namespaced like xsa:foo), header chain priority, edge cases (empty / whitespace / 64+ chars / null headers).
tests/unit/session-manager-binding.test.mjs (NEW, 10 tests) touchSession activity-only contract, deprecated 2-arg compat shim, all five BindResult.reason states, null-oldConnectionId-is-first_bind (Oracle audit defect B), emergency_fallback rebind succeeds without raising the alarm (Oracle audit defect D).
tests/unit/auth-strategy-earliest-reset-first.test.mjs (extended) claudeConn extended with default provider: "claude". New githubConn helper. New seedSingleSessionAccount seed helper. SA-2 rewritten to use githubConn + selector arg "github" (gate inactive, original 4× urgent-alt break still fires for non-cache-sensitive providers). SA-12 NEW: Claude bound + 4× alt → break BLOCKED by gate. SA-13 NEW: Claude bound score=0 + positive alt → break ALLOWED (gate's escape clause for genuinely unusable bound). SA-14 NEW: GitHub bound + 4× alt → break ALLOWED (gate is Claude-only). 69 SA tests total, all PASS.

Verification

  • prettier ✅
  • eslint (errors 0, warnings 75 baseline unchanged) ✅
  • typecheck:core ✅
  • typecheck:noimplicit:core ✅
  • test:unit 2874/2874 PASS ✅
  • check:docs-sync ✅ (3.8.6 → 3.8.7)

Plan + Oracle two-stage verification (per user policy)

  • Plan: .sisyphus/plans/session-affinity-real-fix.md
  • Stage 1 plan v1 (ses_1f85345c8ffebTdtpTaQ5aeN8J): NEEDS_REVISION on 7 items (A defect, B nit, C defect, D defect, E defect, F nit, G nit). All applied in plan v2.
  • Stage 1 plan v2 (ses_1f84ae893ffeTXGq7QBjwfTgzX): NEEDS_REVISION (narrow) on 5 wording-mismatch items. All applied in plan v3.
  • Stage 2 implementation (ses_1f83ab6edffecggMOuz5EmDhT1): NEEDS_REVISION on 4 real defects + 1 nit. All five applied in this commit:
    • Defect A: namespace collision (ext:xsa:foo vs legacy ext: + raw xsa:foo) → moved to hyphenated ext-xsa: / ext-omr:.
    • Defect B (critical): bindSessionConnection after touchSession() (which creates entry with connectionId: null) was classifying as rebind_within_window and emitting the alarm on every new session's normal first bind → null oldConnectionId is now first_bind, no warning.
    • Defect D: emergency-fallback retry's within-window rebind would raise the alarm for legitimate behavior → new BindSource = "emergency_fallback" plumbed from chat.ts retry path, alarm suppressed for that source.
    • Defect H: lazy require("@/sse/utils/logger") could silently swallow logs in ESM/tsx → static import * as log from "@/sse/utils/logger" (verified no cycle).
    • Nit E: gate-suppressed urgent-alt now emits a debug trace with gate: "claude_urgent_alt_suppressed" for observability.

Branch / Commit

Out of scope

  • API-key-scoped external session IDs (current trust model is process-global namespace; future PR if isolation needed)
  • unbindSessionConnection API (no current caller needs it)
  • Cache-sensitive gate for non-Claude providers (codex, GitHub Copilot, etc. — separate cache economics; future per-provider study)
  • Sharing SESSION_AFFINITY_WINDOW_MS between sessionManager.ts and earliestResetFirst.ts via a neutral constant module (current duplication is documented and intentional to avoid circular imports)

…Oracle two-stage verified)

Production observation on 3.8.5/3.8.6: a Claude OAuth session bound to
APEXATGNU switched to GNUMAX only 4 seconds after the last successful
APEXA request, on the same opencode-supplied x-session-affinity header,
losing 147k tokens of cache_write. Oracle hard-debug (ses_1fa7165c0...)
identified three independent design defects, all fixed here.

D1: extractExternalSessionId now recognises x-session-affinity (between
the generic forms and the omniroute-specific alias). Each header source
gets a hyphenated namespace prefix (ext-xsa: / ext-omr:) so that a legacy
generic value of e.g. 'xsa:foo' cannot collide with a newer
x-session-affinity: 'foo' request. The opencode CLI's stable session
continuity signal now flows through to bound-conn lookup; fingerprint
fallback only kicks in when no header is present.

D2: touchSession is split. touchSession(sessionId) is now a pure
activity update (lastActive, requestCount). All connection binding
mutations go through bindSessionConnection(sessionId, connectionId,
{ source }) which returns a BindResult with reasons no_session_id /
first_bind / no_change / rebind_within_window / rebind_after_window.
A within-window rebind (the diagnostic anomaly) emits a structured
log.warn unless source is 'emergency_fallback' (legitimate retry path
in chat.ts). Critically, null oldConnectionId is treated as first_bind
(NOT rebind_within_window) so the production flow of touchSession()
followed by bindSessionConnection() does not raise the alarm on every
new session's normal first request. The deprecated 2-arg touchSession
remains as a one-shot-warned compat shim for tests/legacy callers.

D3: selectByEarliestResetFirst and isAffinityValid accept an optional
provider argument plumbed from auth.ts. For provider 'claude' (Anthropic
OAuth lane), the urgent-alt heuristic break (PR #26 v8) is suppressed
unless boundScore <= 0 (genuinely unusable via accumulated backoff).
Other break paths (terminal, rate_limited, quota_exhausted, low-quota
< 15%, session_expired, affinity_window_passed) are unaffected. Other
providers retain the original break behaviour.

Trace logs added at every routing decision: log.debug 'kept' on affinity
preserved, log.warn 'broken' with bound/altscore on heuristic break, and
log.debug 'urgent-alt break suppressed by cache-sensitive gate' with
gate='claude_urgent_alt_suppressed' when the Claude gate fires. A single
log line during production traffic now answers 'did affinity break, and
why?' deterministically.

Oracle two-stage verification:
- Stage 1 plan v1 (ses_1f85345c8...): NEEDS_REVISION on 7 items.
- Stage 1 plan v2 (ses_1f84ae893...): NEEDS_REVISION (narrow) on 5
  wording-mismatch items.
- Stage 2 implementation (ses_1f83ab6ed...): NEEDS_REVISION on 4 real
  defects + 1 nit. Defect A namespace collision, defect B critical
  null-first-bind misclassification, defect D emergency-fallback alarm
  noise, defect H lazy-require silent swallow, nit E gate trace log
  reason. ALL FIVE FIXED in this commit.

New tests: SA-12 / SA-13 / SA-14 (Claude block / score=0 allow / GitHub
allow), session-manager-affinity-headers (9 tests incl. collision-proof
namespace), session-manager-binding (10 tests incl. null-first-bind and
emergency_fallback). Test count: 2874/2874 PASS (was 2852; +22 net).

Version 3.8.6 -> 3.8.7.
Copilot AI review requested due to automatic review settings May 8, 2026 13:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a production session-affinity regression by (1) recognizing x-session-affinity as an external session header with collision-proof namespacing, (2) separating session activity tracking from connection binding to prevent silent affinity overwrites, and (3) gating the urgent-alt heuristic break for Claude to avoid costly prompt-cache rewrites.

Changes:

  • Add namespaced external session ID extraction for x-session-affinity and related headers (extractExternalSessionId).
  • Split session tracking into touchSession() (activity-only) and bindSessionConnection() (single binding mutation site), and update routing/handlers to use binding explicitly.
  • Plumb provider into selectByEarliestResetFirst / isAffinityValid and suppress the urgent-alt break for provider === "claude" unless boundScore <= 0, plus add structured affinity logs.
  • Add/extend unit tests for header extraction, binding semantics, and the Claude-only gate; bump version to 3.8.7 and update docs/changelog.

Commands / Tests / Coverage (from PR description; not executed in this review environment):

  • Reported: npm run test:unit → 2874/2874 PASS
  • Coverage gate: npm run test:coverage (≥60% c8 thresholds) not verified here.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
open-sse/services/sessionManager.ts Introduces bindSessionConnection, makes touchSession activity-only, and adds namespaced extractExternalSessionId (incl. x-session-affinity).
src/sse/services/strategies/earliestResetFirst.ts Threads provider through affinity validation, adds Claude cache-sensitive gate + structured logs, and binds sessions via bindSessionConnection on fall-through.
src/sse/services/auth.ts Passes provider into selectByEarliestResetFirst so provider-scoped affinity logic can apply.
src/sse/handlers/chat.ts Switches post-credential binding from deprecated touchSession(sessionId, conn) to bindSessionConnection(..., {source}) (incl. emergency fallback source).
tests/unit/session-manager-affinity-headers.test.mjs Adds unit coverage for x-session-affinity extraction, namespacing, collision avoidance, and header precedence/edge cases.
tests/unit/session-manager-binding.test.mjs Adds unit coverage for activity-only touch, binding reasons, null-old first bind, and emergency-fallback behavior.
tests/unit/auth-strategy-earliest-reset-first.test.mjs Updates SA-2 to non-Claude provider and adds SA-12/13/14 to validate Claude-only urgent-alt suppression + escape clause.
CHANGELOG.md Documents 3.8.7 changes and verification notes.
package.json Bumps version to 3.8.7.
docs/openapi.yaml Bumps API version to 3.8.7.

Comment thread CHANGELOG.md Outdated
Comment on lines +31 to +32
- New unit tests in `tests/unit/session-manager-affinity-headers.test.mjs` (8 tests) cover the new namespace-per-source extraction, header collision avoidance, fallback chain order, and edge cases (empty / whitespace / 64+ char values).
- New unit tests in `tests/unit/session-manager-binding.test.mjs` (8 tests) cover the `touchSession` activity-only contract, the deprecated 2-arg compat shim, and all five `BindResult.reason` states.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 592ccb9. Reconciled the test counts in [3.8.7]: line 31 now says (9 tests) for affinity-headers and line 32 says (10 tests) for binding, matching the +22 net total on line 34 and the actual test files (9 + 10 + 3 SA-12/13/14).

Comment on lines +126 to +150
test("bindSessionConnection: rebind_after_window when conn changes after the affinity window", async () => {
bindSessionConnection("s7", "conn-A", { source: "fall_through" });

// Force the existing session's lastActive into the past so the change is
// classified as 'after window' without sleeping for 5 real minutes.
const info = getSessionInfo("s7");
assert.ok(info);
// Mutate the underlying entry by re-creating with a stale lastActive. The
// module-private map is not exported, so we rely on internal API: clear then
// re-bind via test helper. Simulate by clearing and installing manually
// through the bind path with a forwarded clock — easier: replace by clearing
// sessions and seeding a fresh entry with stale activity using touchSession
// is not possible. Skip the strict sub-assertion of "after_window" reason
// and instead exercise the post-window path by waiting if running outside
// CI. As a cheap signal: we assert the function does NOT crash and still
// mutates the binding when called twice with different conns.
// (Production behaviour is exercised by selectByEarliestResetFirst trace
// logs in auth-strategy-earliest-reset-first.test.mjs.)
const r = bindSessionConnection("s7", "conn-C", { source: "fall_through" });
assert.equal(r.ok, true);
assert.equal(getSessionConnection("s7"), "conn-C");
// Same-tick rebind is still classified within window; this is acceptable
// for the unit-level contract because the reason classification is wall-
// clock dependent and integration coverage exercises both branches.
assert.ok(["rebind_within_window", "rebind_after_window"].includes(r.reason));

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 592ccb9 by renaming. The test no longer claims to validate the after-window classification deterministically; it now asserts the weaker (but truthful) contract: a rebind to a different conn (a) succeeds, (b) actually mutates the bound conn, and (c) classifies as one of the two rebind variants. The deterministic within-window emission path is covered by the previous test (rebind_within_window when conn changes inside affinity window), which I left unchanged. Stubbing Date.now() / node:test mock timers is deferred to a future PR if/when integration coverage is needed for the strict after-window emit path.

// ratio=100×, delta=1980 → triggers both factor and absolute delta gates.
// Uses GitHub provider so the Claude cache-sensitive gate (PR #28 D3)
// does NOT suppress the urgent-alt break. Claude-blocked variant lives in SA-12.
// Bound seeded as github so quotaCache lookup matches the conn.provider field.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 592ccb9. The SA-2 comment now correctly states that setQuotaCache stores a provider tag alongside the per-connectionId entry but the lookup key for scoreSessionTrack/scoreWeeklyTrack is the connectionId only — the github string is descriptive metadata, not part of the cache key. The Claude gate decision is driven entirely by the selectByEarliestResetFirst third argument; the seedSingleSessionAccount(..., "github", ...) call is unchanged but its purpose is now described accurately.

R1-1: CHANGELOG line 31-32 said '8 tests' for both new session-manager
test files, but line 34 (and the actual files) say 9 + 10. Reconciled to
9 (affinity-headers) and 10 (binding); aligned with the +22 net total.

R1-2: the 'rebind_after_window' test was named as if it deterministically
asserted the after-window classification, but it only asserts the weaker
'one of the two rebind variants' contract because it does not advance
real time. Test renamed to 'rebind to a different conn always succeeds
and updates the bound conn (window classification is wall-clock
dependent)' so the assertion matches the name. The deterministic
within-window branch is exercised by the previous test in the same file.

R1-3: SA-2 comment claimed the quotaCache lookup keys on conn.provider,
but the cache is keyed by connectionId only — the provider string is
descriptive metadata, not part of the lookup key. Comment corrected to
state that the Claude gate decision is driven entirely by the
selectByEarliestResetFirst third argument, not by any cache-key
provider field.

No production code change. 2874/2874 unit tests still pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Comment on lines +671 to +684
if (bound && affinity && affinity.valid === false) {
const boundScored = scored.find((sc) => sc.conn.id === boundId);
const usableAlts = scored.filter((sc) => !sc.excluded && sc.conn.id !== boundId);
const bestAltScore =
usableAlts.length > 0 ? Math.max(...usableAlts.map((sc) => sc.score ?? 0)) : null;
log.warn("AUTH/affinity", "broken", {
sessionId,
provider: provider ?? null,
oldBound: boundId,
decision: "fall_through_break",
reason: affinity.reason,
boundScore: boundScored?.score ?? null,
bestAltScore,
});

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 57d512c. The warn-level log is now gated by affinity.reason: only the two heuristic break reasons (affinity_break_low_quota and affinity_break_p1_too_urgent) emit at log.warn — those are the diagnostic alarm cases where smart-affinity actively chose to break a healthy binding. All other invalid reasons (affinity_window_passed, rate_limited, terminal, quota_exhausted_unknown_reset, inactive, track-excluded variants) drop to log.debug since they are expected affinity invalidations during normal routing.

Comment thread CHANGELOG.md Outdated

#### Trace logging

`selectByEarliestResetFirst` now emits a structured log on every routing decision: `log.debug("AUTH/affinity", "kept", ...)` when affinity is preserved, `log.warn("AUTH/affinity", "broken", ...)` when affinity is broken via heuristic with the bound and best-alt scores attached, and `log.debug("AUTH/affinity", "fall-through selected", ...)` when no session was bound. A single `log.warn` line during production traffic is now the diagnostic signal for "did affinity break?" — combined with the `bindSessionConnection` rebind-within-window warning, the previously invisible silent overwrites are now first-class observable events.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 57d512c. The [3.8.7] wording now matches the gated emission: warn ONLY when reason is one of the heuristic break codes; non-heuristic reasons are explicitly listed as dropping to debug.

Comment on lines +33 to +34
// as bindSessionConnection. The deprecated warning is best-effort and not
// asserted here (logger may be unavailable in this test environment).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 57d512c. The comment now describes the real reason: keeping the unit test decoupled from logger side effects, with explicit acknowledgement that sessionManager statically imports the logger so the deprecation warning IS emitted in production. Verifying the emission would be an integration-test responsibility (logger stub).

Comment on lines +33 to +35
// with the new x-session-affinity: "foo" namespace. The hyphenated prefix
// form (ext-xsa) prevents this because no legacy generic raw value can
// begin with the hyphen-prefix marker.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 57d512c. Reworded the rationale: collision-proofing comes from the prefixes themselves differing (ext-xsa: for x-session-affinity vs ext: for the legacy generic chain). Legacy generic value xsa:foo becomes ext:xsa:foo while the new affinity value foo becomes ext-xsa:foo — the character right after ext differs (- vs :), making the resulting internal ids structurally distinct. The proof has nothing to do with what raw values can or cannot begin with.

R2-1 (defect): selectByEarliestResetFirst was logging at warn level for
EVERY isAffinityValid invalid reason — including expected non-heuristic
ones like affinity_window_passed / rate_limited / terminal /
quota_exhausted_unknown_reset / inactive / track-excluded. That polluted
production warn-level logs with normal routing events. Now only the two
HEURISTIC break reasons (affinity_break_low_quota /
affinity_break_p1_too_urgent) emit at warn level — those are the
diagnostic alarm cases where the smart-affinity logic actively chose to
break a healthy binding. All other invalid reasons drop to debug.

R2-2 (changelog): aligned the [3.8.7] wording with the actual emission
("ONLY when affinity is broken via the heuristic rules" + explicit list
of expected reasons that drop to debug).

R2-3 (test comment): the binding test note 'logger may be unavailable
in this test environment' was stale after the PR's switch to a static
logger import. Updated to the real reason: keeping the unit test
decoupled from logger side effects, with verification of the warning
emission deferred to an integration test.

R2-4 (test comment): the affinity-headers test rationale was slightly
off: collision-proofing comes from the prefixes themselves differing
('ext-xsa:' vs 'ext:'), not from any restriction on what legacy raw
values can begin with. Reworded to make the proof source explicit.

No production behavior change beyond R2-1's log-level gating. 2874/2874
unit tests still pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comment on lines +165 to +169
bindSessionConnection(sessionId, deprecatedConnectionId, {
source: "explicit_post_credential",
});
}
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b0f581c. The compat shim now binds FIRST (so bindSessionConnection sees the pre-touch lastActive and classifies the rebind correctly), then preserves the original 1-touch == 1-request semantics by only incrementing requestCount when an entry already existed (since the first_bind branch in bindSessionConnection already sets it to 1). A fresh session via the compat shim now sees requestCount == 1 exactly as before; a stale (>5 min idle) session that gets a new conn no longer mis-fires the within-window alarm.

Comment on lines +707 to +709
if (sessionId) {
bindSessionConnection(sessionId, selected.id, { source: "fall_through" });
log.debug("AUTH/affinity", "fall-through selected", {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b0f581c by adopting the cleaner option (a): the strategy no longer binds. selectByEarliestResetFirst returns the chosen connection but does NOT mutate the session binding. chat.ts:548 is now the single binding site for the post-credential decision and selects source: "emergency_fallback" vs "explicit_post_credential" from runtimeOptions.emergencyFallbackTried. The emergency_fallback alarm suppression now works as designed because the strategy no longer beats the post-credential bind with a hard-coded fall_through source. SA tests (69) all still pass — they exercise the strategy decision logic and explicitly setup binding via bindSessionConnection in their fixtures, so the strategy-level bind removal is invisible to them. The unused bindSessionConnection import was removed from the strategy.

…redact PII from CHANGELOG

R3-1 (defect): touchSession(sessionId, connectionId) compat shim was
updating lastActive BEFORE forwarding to bindSessionConnection. That
made the forwarded call always see 'within affinity window' (because
lastActive had just been refreshed to Date.now()), so a session idle
>5 minutes would mis-fire the within-window alarm on its first
compat-shim rebind. The shim now binds first (so the binding decision
sees the pre-touch lastActive). Only when an entry pre-existed is
requestCount incremented — first_bind already sets it to 1, so a fresh
session via the compat shim sees requestCount == 1 (preserves the
original 1-touch == 1-request semantics that session-manager.test.mjs
relies on).

R3-2 (defect): selectByEarliestResetFirst was binding fall-through
selections itself with a hard-coded source: 'fall_through'. Since the
strategy ran BEFORE chat.ts:548's post-credential bind, the actual
within-window rebind always carried the wrong source — the
emergency_fallback alarm suppression added in stage-2 audit could
never engage in the emergency retry path. The strategy no longer binds.
chat.ts:548 is now the single binding site (single source of truth) and
selects source: 'emergency_fallback' vs 'explicit_post_credential' from
runtimeOptions.emergencyFallbackTried, so the suppression works as
intended. The unused bindSessionConnection import was removed from
the strategy.

Test count: 2875/2875 PASS (was 2874; +1 new compat-shim first-bind
contract test).

PII REDACTION (urgent user demand): CHANGELOG.md previously contained
real account names (APEXATGNU, GNUMAX) in production observation
narratives across multiple [3.8.x] sections. All occurrences have been
replaced with generic placeholders (Account A / Account B). The same
substitution was applied to .sisyphus/plans/*.md (workspace-only,
not git-tracked, but kept consistent for future readers). PR #28 body
on GitHub has also been edited to use the generic names. Merged commit
messages on main (PR #25 / #26 / #27) cannot be redacted without a
force-push to main, which is forbidden by AGENTS.md (incident cheaper-inference#1386
on 2026-04-18); their messages remain as-is on origin/main.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

@i1hwan
i1hwan merged commit d831925 into main May 9, 2026
50 of 51 checks passed
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.

2 participants