Conversation
…l clock The lock TTL serves two opposed roles: it is both the contender's give-up deadline and the holder's staleness threshold. Raising it cannot make these tests deterministic: a starved holder can still become evictable, while the longer contender wait can overrun Bun's 5000ms watchdog. Use injected clocks and explicit barriers instead. A synthetic fresh owner lets the lock_busy test advance from fresh-owner inspection to deadline exhaustion without elapsed time. Startup migration tests suppress only the test-observed 100ms warmup escape, and concurrent migration waits on entered/release/rename barriers rather than sleeps. Production behavior is untouched. Under 16 CPU hogs, the unmodified tests were 0/10 and included semantic failures such as 'Expected promise that rejects / Received promise that resolved'. After the change, no lock assertion failed; remaining red runs were exclusively Bun watchdog kills followed by temp-directory cleanup cascades. Green runs clustered below 100ms (the concurrent case occasionally took longer when descheduled), while watchdog failures began at 5.4s. The direct mkdir/write/read lock_busy test was once reported at 7588ms under two-core oversubscription, proving that extreme-load gate measured scheduler starvation rather than lock semantics.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 files
Confidence score: 2/5
- In
packages/opencode/src/index.ts, a second desktop notice can be inserted without being marked as plugin-generated when the previous notice is the latest user message, which risks incorrect notice classification or placement; preserve the requiredmessageIDin this case. - In
packages/opencode/src/index.ts, repeated updates for the user message that triggered the current idle event can advancedesktopNoticeUserRevisionseven though no new prompt occurred, potentially causing incorrect revision tracking; increment only when the user ID changes. - In
packages/opencode/src/tests/index.test.ts, the mock guard becomes false after the firstpromptAsynccall, so the second call does not wait onswitchNoticeCompletionand the restoration notice resolves immediately; keep the guard active long enough to exercise the intended blocking behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/tests/index.test.ts">
<violation number="1" location="packages/opencode/src/tests/index.test.ts:19026">
P2: The second promptAsync call cannot block on switchNoticeCompletion because the mock's guard turned false after the first call. The restoration-notice call resolves immediately. If a genuine user message or any other insertion were to revoke the lease mid-flight (the exact race this PR must reproduce and keep protected), the promptAsync spy here would still report two calls and the test would pass — the assertion does not prove the notice was not pre-empted. To make the test actually cover the in-flight race, hold the restoration notice too (e.g. block until a separate deferred, resolved after checking delivery), or verify queue state rather than only call count.</violation>
</file>
<file name="packages/opencode/src/index.ts">
<violation number="1" location="packages/opencode/src/index.ts:657">
P1: When a second desktop notice is inserted before another assistant message exists, this callback does not mark it as plugin-generated. The placement helper omits `messageID` once the previous notice is the latest user message, so OpenCode assigns an untracked latest-user ID; that can revoke the lease and re-enter the provider for a duplicate billed turn. Generate a unique ordered ID for every notice, or correlate the host-assigned ID before applying the genuine-user filter.</violation>
<violation number="2" location="packages/opencode/src/index.ts:5308">
P2: A repeated update for the user message that produced the current idle event now changes `desktopNoticeUserRevisions` even though it is not a new prompt. Increment the revision only when the user ID differs from `desktopNoticeIdleUserMessages`, otherwise a queued notice can remain stranded without a new idle event.</violation>
</file>
Architecture diagram
sequenceDiagram
participant OC as OpenCode Client
participant Plugin as Anthropic Auth Plugin
participant NM as Notice Manager
participant LE as Lease Logic
participant SR as Session Router
participant TB as Test Backend
Note over OC,TB: Fable/Opus Recovery Window Notice Delivery Flow
OC->>Plugin: session.idle event
Plugin->>Plugin: Recovery window complete notice queued
Plugin->>LE: scheduleDesktopNoticeProbe(sessionId)
Note over Plugin,LE: Switch notice still in flight
OC->>Plugin: message.updated (switch notice as user msg)
Plugin->>Plugin: isDesktopNotice? Check recorded message IDs
Plugin->>LE: CHANGED: Skip lease revocation (plugin notice)
OC->>Plugin: session.status busy
Plugin->>LE: CHANGED: No lease revocation for plugin notice
alt Genuine user message arrives
OC->>Plugin: message.updated (real user msg)
Plugin->>LE: Revoke lease immediately
Plugin->>NM: Abort delivery attempt
else No genuine user message
Plugin->>LE: Probe idle status
LE->>Plugin: Session still idle
Plugin->>OC: sendIgnoredMessage (with onMessageId)
OC->>Plugin: messageID assigned
Plugin->>Plugin: CHANGED: Record notice message ID
Plugin->>NM: Insert ignored notice
OC->>Plugin: message.updated (notice as user msg)
Plugin->>Plugin: CHANGED: Recognize as plugin notice, not genuine
Plugin->>LE: Keep lease valid
end
Note over Plugin,OC: Flush decision re-check
Plugin->>LE: canSend check before insertion
alt Lease revoked by genuine user
LE-->>Plugin: false - abort send
Plugin-->>OC: Notice not delivered
else Lease still valid
LE-->>Plugin: true - proceed
Plugin->>OC: Insert notice successfully
Plugin->>Plugin: CHANGED: Check for more pending notices
alt More notices pending and revision unchanged
Plugin->>LE: Re-grant lease
Plugin->>Plugin: Schedule next probe
end
end
Note over Plugin,TB: Test isolation (pi package)
Plugin->>TB: Run test suite
TB->>TB: CHANGED: Isolated config directory created
TB->>TB: CHANGED: Host env vars cleared
TB->>TB: Run tests with clean config
TB-->>Plugin: Results
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // A new user prompt can start while that request is in flight, so re-check the | ||
| // caller's delivery lease immediately before inserting the ignored message. | ||
| if (options.canSend && !options.canSend()) return false | ||
| if (request.body.messageID) options.onMessageId?.(request.body.messageID) |
There was a problem hiding this comment.
P1: When a second desktop notice is inserted before another assistant message exists, this callback does not mark it as plugin-generated. The placement helper omits messageID once the previous notice is the latest user message, so OpenCode assigns an untracked latest-user ID; that can revoke the lease and re-enter the provider for a duplicate billed turn. Generate a unique ordered ID for every notice, or correlate the host-assigned ID before applying the genuine-user filter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/index.ts, line 657:
<comment>When a second desktop notice is inserted before another assistant message exists, this callback does not mark it as plugin-generated. The placement helper omits `messageID` once the previous notice is the latest user message, so OpenCode assigns an untracked latest-user ID; that can revoke the lease and re-enter the provider for a duplicate billed turn. Generate a unique ordered ID for every notice, or correlate the host-assigned ID before applying the genuine-user filter.</comment>
<file context>
@@ -653,6 +654,7 @@ async function sendIgnoredMessage(
// A new user prompt can start while that request is in flight, so re-check the
// caller's delivery lease immediately before inserting the ignored message.
if (options.canSend && !options.canSend()) return false
+ if (request.body.messageID) options.onMessageId?.(request.body.messageID)
if (typeof session?.promptAsync === 'function') {
</file context>
| ) | ||
| const switchNoticeCompletion = deferred() | ||
| let holdSwitchNotice = true | ||
| mockClient.session.promptAsync = mock(async () => { |
There was a problem hiding this comment.
P2: The second promptAsync call cannot block on switchNoticeCompletion because the mock's guard turned false after the first call. The restoration-notice call resolves immediately. If a genuine user message or any other insertion were to revoke the lease mid-flight (the exact race this PR must reproduce and keep protected), the promptAsync spy here would still report two calls and the test would pass — the assertion does not prove the notice was not pre-empted. To make the test actually cover the in-flight race, hold the restoration notice too (e.g. block until a separate deferred, resolved after checking delivery), or verify queue state rather than only call count.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/index.test.ts, line 19026:
<comment>The second promptAsync call cannot block on switchNoticeCompletion because the mock's guard turned false after the first call. The restoration-notice call resolves immediately. If a genuine user message or any other insertion were to revoke the lease mid-flight (the exact race this PR must reproduce and keep protected), the promptAsync spy here would still report two calls and the test would pass — the assertion does not prove the notice was not pre-empted. To make the test actually cover the in-flight race, hold the restoration notice too (e.g. block until a separate deferred, resolved after checking delivery), or verify queue state rather than only call count.</comment>
<file context>
@@ -19021,6 +19021,13 @@ describe('auth.loader', () => {
)
+ const switchNoticeCompletion = deferred()
+ let holdSwitchNotice = true
+ mockClient.session.promptAsync = mock(async () => {
+ if (!holdSwitchNotice) return
+ holdSwitchNotice = false
</file context>
| if (!isDesktopNotice) { | ||
| desktopNoticeUserRevisions.set( | ||
| sessionId, | ||
| (desktopNoticeUserRevisions.get(sessionId) ?? 0) + 1, |
There was a problem hiding this comment.
P2: A repeated update for the user message that produced the current idle event now changes desktopNoticeUserRevisions even though it is not a new prompt. Increment the revision only when the user ID differs from desktopNoticeIdleUserMessages, otherwise a queued notice can remain stranded without a new idle event.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/index.ts, line 5308:
<comment>A repeated update for the user message that produced the current idle event now changes `desktopNoticeUserRevisions` even though it is not a new prompt. Increment the revision only when the user ID differs from `desktopNoticeIdleUserMessages`, otherwise a queued notice can remain stranded without a new idle event.</comment>
<file context>
@@ -5237,21 +5297,32 @@ const anthropicAuthPlugin = async (
+ if (!isDesktopNotice) {
+ desktopNoticeUserRevisions.set(
+ sessionId,
+ (desktopNoticeUserRevisions.get(sessionId) ?? 0) + 1,
+ )
+ if (typeof info.id === 'string') {
</file context>
|
Stack, for merge order. Four PRs came out of chasing three separate CI failures to root cause; each fix is in its own PR rather than folded into the custody change. Suggested order: #219 → #228 → #229 → #218. Each rebases cleanly on the previous. This PR: stacked on #219. Green. Fixes the restoration-notice bug that was failing the e2e in #219. The three CI failures were unrelated to each other: a Pi suite reading the operator's live config (#219), manifest-lock tests racing a wall-clock TTL (#228), and a genuine product bug where the model-restored notice never reaches a session with no TUI attached (#229). |
…y turn The switch notice was still inside promptAsync when the final cache warm queued the restoration notice. OpenCode published that ignored notice as a user message and a busy status, revoking the idle-delivery lease; the queued restoration then had no later idle event to release it. Track plugin-generated notice IDs separately from genuine user messages, then re-enter the existing bounded status probe after a successful noReply insertion only when no genuine user message arrived. The live status check and final lease check still gate insertion, so an active prompt cannot adopt the ignored message and duplicate a billed provider turn.
cbdbf02 to
e8c31f3
Compare
|
Re-stacked into a single linear chain so every PR can go green independently: Merge in that order. The earlier shape had #228/#229/#218 as siblings on #219, which left #218 red on the e2e notice bug that #229 fixes — a failure that was never its own. Local gates on the chain tip: core 199/0 · opencode 1875/0 · pi 114/0 · e2e 36/0 · typecheck clean. |
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Confidence score: 3/5
- In
packages/opencode/src/tests/index.test.ts, a hanging startup credential fetch can replace the 100 ms warmup timeout with a timer that never fires, leavinggetPluginwithout a deadline and potentially hanging the test suite—retain an independent timeout around the credential path. - In
packages/opencode/src/tests/index.test.ts, assertinglogs.some(...)immediately aftergetPluginmay race background warmup and make the test flaky, causing intermittent failures—restore bounded polling or await the warmup completion signal before checking logs.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/tests/index.test.ts">
<violation number="1" location="packages/opencode/src/tests/index.test.ts:745">
P2: When a startup credential fetch hangs, this branch replaces the plugin's 100 ms warmup timeout with a timer that never fires. The changed tests await `getPlugin` inside this helper without another deadline, so a regression now hangs the test runner instead of exercising the timeout path; retain an independent deadlock guard while suppressing the warmup timeout.</violation>
<violation number="2" location="packages/opencode/src/tests/index.test.ts:2887">
P2: The one-shot `logs.some(...).toBe(true)` right after `getPlugin` resolves can race the background warmup. The previous version polled logs for up to ~1 s before asserting, and sibling tests await `plugin.__fallbackRefreshReady` before checking migration effects — so the 'manifest write failed' log can land after `getPlugin` returns. Await `plugin.__fallbackRefreshReady` (or keep a short poll) before the `expect`, otherwise the test flakes. Note this also drops the bounded-wait assertion (elapsed 120–1000 ms) that verified the lock respects its deadline.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ...arguments_: Parameters<typeof globalThis.setTimeout> | ||
| ) => | ||
| arguments_[1] === 100 | ||
| ? ({ unref() {} } as ReturnType<typeof globalThis.setTimeout>) |
There was a problem hiding this comment.
P2: When a startup credential fetch hangs, this branch replaces the plugin's 100 ms warmup timeout with a timer that never fires. The changed tests await getPlugin inside this helper without another deadline, so a regression now hangs the test runner instead of exercising the timeout path; retain an independent deadlock guard while suppressing the warmup timeout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/index.test.ts, line 745:
<comment>When a startup credential fetch hangs, this branch replaces the plugin's 100 ms warmup timeout with a timer that never fires. The changed tests await `getPlugin` inside this helper without another deadline, so a regression now hangs the test runner instead of exercising the timeout path; retain an independent deadlock guard while suppressing the warmup timeout.</comment>
<file context>
@@ -734,6 +734,26 @@ async function getPlugin(
+ ...arguments_: Parameters<typeof globalThis.setTimeout>
+ ) =>
+ arguments_[1] === 100
+ ? ({ unref() {} } as ReturnType<typeof globalThis.setTimeout>)
+ : originalSetTimeout(...arguments_)) as typeof globalThis.setTimeout
+ const setTimeoutSpy = spyOn(globalThis, 'setTimeout').mockImplementation(
</file context>
| break | ||
| await Bun.sleep(10) | ||
| ), | ||
| ).toBe(true) |
There was a problem hiding this comment.
P2: The one-shot logs.some(...).toBe(true) right after getPlugin resolves can race the background warmup. The previous version polled logs for up to ~1 s before asserting, and sibling tests await plugin.__fallbackRefreshReady before checking migration effects — so the 'manifest write failed' log can land after getPlugin returns. Await plugin.__fallbackRefreshReady (or keep a short poll) before the expect, otherwise the test flakes. Note this also drops the bounded-wait assertion (elapsed 120–1000 ms) that verified the lock respects its deadline.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/index.test.ts, line 2887:
<comment>The one-shot `logs.some(...).toBe(true)` right after `getPlugin` resolves can race the background warmup. The previous version polled logs for up to ~1 s before asserting, and sibling tests await `plugin.__fallbackRefreshReady` before checking migration effects — so the 'manifest write failed' log can land after `getPlugin` returns. Await `plugin.__fallbackRefreshReady` (or keep a short poll) before the `expect`, otherwise the test flakes. Note this also drops the bounded-wait assertion (elapsed 120–1000 ms) that verified the lock respects its deadline.</comment>
<file context>
@@ -2820,72 +2851,59 @@ describe('fallback Claustrum credential resolution', () => {
- break
- await Bun.sleep(10)
+ ),
+ ).toBe(true)
+ expect(
+ await readFile(
</file context>
|
Closing as superseded by the narrower fix already on |
2 participants
After a Fable/Opus recovery window closes, the
Fable recovery window complete. Returning to Fable 5.notice never reaches a session with no TUI attached. The switch notice arrives; only the restoration one is lost. In production that means the user is never told the model was restored unless they happen to send another message — a human sends another turn and sees the state resolve anyway, which is why only the e2e ever noticed.Not a slow test: at a 120s wait the notice still never arrived.
Mechanism (this refuted my first hypothesis, which was simply "no later idle event to release it"). The restoration notice is queued with a valid lease and its probe does run. The decisive part is overlap with the still-in-flight switch notice: OpenCode publishes our own ignored insertion as a user-message update plus a busy status. The lease logic could not tell that apart from a genuine user prompt, so notice A's delivery revoked notice B's lease — and with no further idle event coming, nothing ever released it.
Fix. Plugin-generated notice IDs are recorded and excluded from the genuine-user revision counter, so our own insertions no longer revoke the lease. Genuine user messages still bump the revision and still revoke it immediately.
Why a duplicate billed provider turn remains impossible — the hazard this path was reworked for in #162, where an ignored notice becoming the latest user message makes the run loop invoke the provider again on the same turn:
session.status()idle probe with its bounded re-arm,canSendstill re-checks the lease immediately before insertion, so a user message landing mid-flight aborts the send.Verified by mutation rather than by reading:
isDesktopNotice = false(pre-fix behaviour) reddens the e2e withsession notification not found: Returning to Fable 5— the fix is load-bearing,canSendfence reddensdowngrades a filtered Fable session for ten successful Opus turns and warms Fable after each— the duplicate-turn guard is covered by a test, not merely present.The e2e went from failing deterministically on this host (~36s) to passing in 13.8s.
Also adds debug logging at queue and flush-decision. That path had none, which is why diagnosing it required instrumenting the harness by hand.
Stacked on #219, which fixes an unrelated pre-existing Pi failure this branch would otherwise hit.
core 188/0 · opencode 1869/0 · pi 114/0 · e2e 36/0 · typecheck, format, biome clean.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the
Fable recovery window complete. Returning to Fable 5.notice being silently dropped for headless sessions after a recovery, and makes the manifest-lock contention tests deterministic under CPU load.Bug Fixes
Tests
Written for commit e8c31f3. Summary will update on new commits.