diff --git a/docs/implementation/merge-queue.md b/docs/implementation/merge-queue.md index 0117761d..4dfddd99 100644 --- a/docs/implementation/merge-queue.md +++ b/docs/implementation/merge-queue.md @@ -20,7 +20,7 @@ Every update compares the current attempt ID and legal source state. A delayed p The coordinator owns at most one enqueue and one shared queue inspection. Cancellation does not release either operation until its GitHub call settles. One fourteen-second deadline covers every sequential validation, queue-correlation, and submission stage rather than restarting for each remote call. Shutdown rejects every new API request at admission and rechecks irreversible work after partially received request bodies, then gives admitted requests a bounded fourteen-and-a-half-second drain. At the deadline it aborts request-scoped status inspections, destroys requests still blocked on partial bodies, and cancels coordinator work before waiting for server closure and closing SQLite, so an admitted request or queue poll cannot deadlock teardown. -`GET /api/merge` is the narrow polling path for queue-backed attempts. It reads only the persisted attempt and the K1 queue observation; it does not reload Git history or reconstruct the review. Ambiguous direct submissions remain disabled and reconcile through a full review refresh instead of repeatedly calling the queue endpoint. The browser patches only the merge control and status banner, so current selection, scroll, code attachment, and composer drafts remain unchanged. Queue polling starts at two seconds, doubles while the same active state persists, and caps at thirty seconds; a lifecycle transition or explicit user action resets the interval. A review action advances the merge-poll generation, preventing an older response from re-enabling retry against pre-action review state. The action stays disabled while submitting or queued and after confirmed merge. Removal or failure exposes the reason but keeps retry disabled until a full review refresh revalidates current GitHub blockers. Retry is then offered only for an unchanged reviewed context, and the full gate is revalidated twice before another enqueue. +`GET /api/merge` is the narrow polling path for queue-backed attempts. It reads only the persisted attempt and the K1 queue observation; it does not reload Git history or reconstruct the review. Ambiguous direct submissions remain disabled and reconcile through a full review refresh instead of repeatedly calling the queue endpoint. The browser patches only the merge control and status banner, so current selection, scroll, code attachment, and composer drafts remain unchanged. Queue polling starts at two seconds, doubles while the same active state persists, and caps at thirty seconds; a lifecycle transition or explicit user action resets the interval. A review action advances the merge-poll generation, preventing an older response from re-enabling retry against pre-action review state. One merge poll runs at a time (#48): a render while a poll is in flight starts no second chain, and only the latest poll's answer or error is applied, and only while its generation is current. An older generation's poll that returns late neither applies nor releases the newer generation's poll. The page sets no timeout of its own on a poll; the server bounds `/api/merge` with its own read deadlines (traced, not tested). The merge click also clears the poll timer, as every generation change does; that is defensive, because a click needs `ready`, which excludes a running job or an active attempt (traced, not tested). The action stays disabled while submitting or queued and after confirmed merge. Removal or failure exposes the reason but keeps retry disabled until a full review refresh revalidates current GitHub blockers. Retry is then offered only for an unchanged reviewed context, and the full gate is revalidated twice before another enqueue. Transient or incomplete GitHub observations leave the attempt active, surface an observation error, and continue polling rather than clearing the browser's active attempt. Submitting attempts use unknown-outcome wording until GitHub proves that the attempt is queued. Only a validated queued, merged, removed, or failed observation changes durable state. Queue rules count as the server-side current-base guard; adapters without queue inspection continue to fail closed. @@ -51,4 +51,4 @@ The status cache in `GhMergeGateway` is cleared before and after every merge com Controlled unit and browser regressions cover enqueue success, delayed merge, queue removal, unmergeable failure, head replacement, retry, stale observation publication, active-attempt retry refusal, shutdown settlement, durable restart recovery, and preservation of UI input while polling. Final evidence is recorded against the exact pushed PR head. -#48 regressions: `test/merge-stop-reasons.test.ts` (stop reasons and resendable timeouts; a saved success answered after the click's own abort with no further load or GitHub read; output past the limit while the pipes drain still fails the command; an outcome saved meanwhile answered after a timeout and this click's own abort; gh's answer kept when shutdown finds writes closed; one inspection stage's gh exit reported after its stopped siblings settle, including past the deadline, with no unhandled rejection; a caller abort answered in place of the already-fixed check's own earlier gh failure), `test/merge-queue-poll.test.ts` (a failed, timed-out or gh-answered queue read answers from the current attempt; GitHub's refusal survives a later shutdown, saved or not; the click's answer is inactive, also when a poll ends the attempt before the answer is built; an issue-trust timeout answers 503, alone or before the merge deadline, and a gh answer before the deadline is saved and replayed), `test/merge-status-cache.test.ts` (cache evidence; these passed before the fixes), `test/pre-merge.test.ts` and `test/runner-publishing.test.ts` (a preparation's or a publish's trust read that timed out or that gh answered before shutdown is saved or recorded as failed with that answer, not as the shutdown), and `test/runner-start.test.ts` (a runner action's timed-out trust read is not saved and answers 503, also when shutdown started while gh stopped; a saved outcome still wins). The browser's duplicate merge polls are fixed separately on top of F6b, which rewrites that polling. +#48 regressions: `test/merge-stop-reasons.test.ts` (stop reasons and resendable timeouts; a saved success answered after the click's own abort with no further load or GitHub read; output past the limit while the pipes drain still fails the command; an outcome saved meanwhile answered after a timeout and this click's own abort; gh's answer kept when shutdown finds writes closed; one inspection stage's gh exit reported after its stopped siblings settle, including past the deadline, with no unhandled rejection; a caller abort answered in place of the already-fixed check's own earlier gh failure), `test/merge-queue-poll.test.ts` (a failed, timed-out or gh-answered queue read answers from the current attempt; GitHub's refusal survives a later shutdown, saved or not; the click's answer is inactive, also when a poll ends the attempt before the answer is built; an issue-trust timeout answers 503, alone or before the merge deadline, and a gh answer before the deadline is saved and replayed), `test/merge-status-cache.test.ts` (cache evidence; these passed before the fixes), `test/pre-merge.test.ts` and `test/runner-publishing.test.ts` (a preparation's or a publish's trust read that timed out or that gh answered before shutdown is saved or recorded as failed with that answer, not as the shutdown), and `test/runner-start.test.ts` (a runner action's timed-out trust read is not saved and answers 503, also when shutdown started while gh stopped; a saved outcome still wins). The #48 browser regressions (`test/browser/issue-48-poll-ordering.spec.ts`, `test/browser/issue-48-stale-errors.spec.ts`) cover one poll chain under a re-render, a held error, an older generation's late poll, and an unusable current answer; a re-render during a merge-job poll takes the same guard (traced, not tested). diff --git a/test/browser/issue-48-poll-ordering.spec.ts b/test/browser/issue-48-poll-ordering.spec.ts new file mode 100644 index 00000000..c1b4265c --- /dev/null +++ b/test/browser/issue-48-poll-ordering.spec.ts @@ -0,0 +1,124 @@ +import { test, expect } from '@playwright/test'; +import { chmodSync, mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { createDemo } from '../../scripts/demo.ts'; +import { startServer } from '../../web/server.ts'; +import type { MergeGateway, MergeQueueGateway } from '../../github/merge.ts'; +import { fixtureGit } from '../fixtures/git.ts'; + +// Issue #48, poll ordering (browser): a merge-queue poll answer computed before a newer poll answer must not be +// applied after it. Before the fix, any render() (for example selecting another item) while a poll was in flight +// scheduled a second poll chain with the same mergeGeneration, so two polls could be outstanding and nothing ordered +// their answers. Now one poll chain runs at a time. +let root: string, app: Awaited>; +test.beforeEach(async () => { root=mkdtempSync(join(tmpdir(),'codeboost-browser-'));app=await startServer(createDemo(join(root,'demo')),0); }); +test.afterEach(async () => { await app.close();rmSync(root,{recursive:true,force:true}); }); + +test('a re-render while a merge-queue poll is held starts no second poll chain, so no older answer follows a newer one',async({page})=>{ + const config={...app.service.config,demo:false};await app.close(); + chmodSync(join(config.repository,'run.sh'),0o644);fixtureGit(config.repository,'commit','-am','Restore declared scope'); + let appRef:typeof app,phase:'queued'|'merged'='queued'; + const gateway:MergeGateway&MergeQueueGateway={ + inspect:async()=>{const snapshot=(await appRef.service.load()).snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};}, + queueWatermark:async()=>null, + merge:async()=>({url:'https://github.com/example/repo/pull/24'}), + inspectQueue:async head=>phase==='merged'?{state:'merged',reviewedHead:head,mergedAt:'2026-09-24T08:10:00Z'}:{state:'queued',reviewedHead:head,entryId:'MQE_1',phase:'QUEUED',position:1,enqueuedAt:'2026-09-24T08:00:00Z',queueHead:head}, + }; + app=appRef=await startServer(config,0,undefined,gateway);let view=await app.service.load(); + for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=await app.service.act({action:'accept',key:segment.key,token:view.token}); + for(const item of view.items)view=await app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); + + // Poll 1: the server answers (queued) at once, but delivery of that answer to the page is held. Later polls pass. + let release!:()=>void,arrived!:()=>void; + const held=new Promise(resolve=>release=resolve),started=new Promise(resolve=>arrived=resolve); + let polls=0,heldState:string|undefined; + await page.route('**/api/merge',async route=>{ + const n=++polls; + if(n===1){const response=await route.fetch();const body=await response.json();heldState=body.queue?.state;arrived();await held;await route.fulfill({response,json:body});return;} + await route.continue(); + }); + await page.goto(app.url);page.on('dialog',dialog=>dialog.accept()); + await page.getByRole('button',{name:'Merge PR',exact:true}).click(); + await expect(page.getByRole('button',{name:'Merge queued',exact:true})).toBeDisabled(); + try{ + await started; + expect(heldState).toBe('queued'); + // GitHub now confirms the merge, so any later answer is newer than the held one. + phase='merged'; + // Selecting another item re-renders while poll 1 is in flight. It must not start a second poll chain, whose newer + // answer the held older one could then overwrite (the first poll delay is 2 s; wait past it). + await page.getByRole('button',{name:/P2 Document retry behavior/}).click(); + await page.waitForTimeout(3000); + expect(polls).toBe(1); + // The held answer is still the latest one issued, so it applies; the next poll then reports the merge. + release(); + await expect(page.getByRole('button',{name:'Merged',exact:true})).toBeDisabled({timeout:10000}); + await expect(page.locator('#banner')).toContainText('GitHub confirmed the reviewed head was merged.'); + expect(polls).toBe(2); + // Nothing older can arrive later and move the control back. + await page.waitForTimeout(2500); + expect(await page.locator('#merge').textContent()).toBe('Merged'); + }finally{release();} +}); + +// The poll in flight belongs to its generation: after a refresh starts a new generation and its own poll, the old poll's +// late answer is discarded and must not release the new generation's single-chain guard (AGENTS.md: "Poll starts, then +// a user action completes, then the old poll returns"). +test('an older generation poll returning late neither applies nor lets a re-render start a second chain',async({page})=>{ + const config={...app.service.config,demo:false};await app.close(); + chmodSync(join(config.repository,'run.sh'),0o644);fixtureGit(config.repository,'commit','-am','Restore declared scope'); + let appRef:typeof app; + const gateway:MergeGateway&MergeQueueGateway={ + inspect:async()=>{const snapshot=(await appRef.service.load()).snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};}, + queueWatermark:async()=>null, + merge:async()=>({url:'https://github.com/example/repo/pull/24'}), + inspectQueue:async head=>({state:'queued',reviewedHead:head,entryId:'MQE_1',phase:'QUEUED',position:1,enqueuedAt:'2026-09-24T08:00:00Z',queueHead:head}), + }; + app=appRef=await startServer(config,0,undefined,gateway);let view=await app.service.load(); + for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=await app.service.act({action:'accept',key:segment.key,token:view.token}); + for(const item of view.items)view=await app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); + // Polls 1 (old generation) and 2 (new generation) are held; any later poll is counted and passes. + const releases:Array<()=>void>=[],started:Array<()=>void>=[]; + const starts=[0,1].map(index=>new Promise(resolve=>started[index]=resolve)); + const holds=[0,1].map(index=>new Promise(resolve=>releases[index]=resolve)); + let polls=0; + await page.route('**/api/merge',async route=>{ + const n=++polls; + if(n<=2){ + const response=await route.fetch();const body=await response.json(); + // The old generation's answer says "merged", so applying it would be visible; the current generation's stays queued. + if(n===1&&body.queue)body.queue={...body.queue,state:'merged',mergedAt:'2026-09-24T08:10:00Z'}; + started[n-1]!();await holds[n-1];await route.fulfill({response,json:body});return; + } + await route.continue(); + }); + await page.goto(app.url);page.on('dialog',dialog=>dialog.accept()); + await page.getByRole('button',{name:'Merge PR',exact:true}).click(); + await expect(page.getByRole('button',{name:'Merge queued',exact:true})).toBeDisabled(); + try{ + await starts[0]; + // A refresh starts a new merge generation; its own poll (2) starts after the poll delay and is held too. + await page.getByRole('button',{name:'Refresh',exact:true}).click(); + await starts[1]; + expect(polls).toBe(2); + // The old generation's poll now returns; it must not apply, nor clear the new generation's in-flight poll. + releases[0]!(); + await page.waitForTimeout(300); + // Not applied: the control still shows the current generation's queued state. + await expect(page.getByRole('button',{name:'Merge queued',exact:true})).toBeDisabled(); + await expect(page.locator('#banner')).not.toContainText('confirmed the reviewed head was merged'); + // A re-render now must not start a third poll while poll 2 is in flight (wait past the 2 s poll delay). + await page.getByRole('button',{name:/P2 Document retry behavior/}).click(); + await page.waitForTimeout(3000); + expect(polls).toBe(2); + // Still not applied, however late the old answer arrived. The re-render cleared the banner, so the control is the + // evidence: an applied "merged" answer would show "Merged". + await expect(page.getByRole('button',{name:'Merge queued',exact:true})).toBeDisabled(); + await expect(page.locator('#banner')).not.toContainText('confirmed the reviewed head was merged'); + releases[1]!(); + // The current generation's poll applies its answer and continues the one chain: a third poll follows. + await expect.poll(()=>polls,{timeout:15_000}).toBe(3); + await expect(page.getByRole('button',{name:'Merge queued',exact:true})).toBeDisabled(); + }finally{releases.forEach(release=>release());} +}); diff --git a/test/browser/issue-48-stale-errors.spec.ts b/test/browser/issue-48-stale-errors.spec.ts new file mode 100644 index 00000000..6fb71408 --- /dev/null +++ b/test/browser/issue-48-stale-errors.spec.ts @@ -0,0 +1,70 @@ +import { test, expect } from '@playwright/test'; +import { chmodSync, mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { createDemo } from '../../scripts/demo.ts'; +import { startServer } from '../../web/server.ts'; +import type { MergeGateway, MergeQueueGateway } from '../../github/merge.ts'; +import { fixtureGit } from '../fixtures/git.ts'; +let root: string, app: Awaited>; +function removeDemoOutOfScope(config: typeof app.service.config) { chmodSync(join(config.repository,'run.sh'),0o644);fixtureGit(config.repository,'commit','-am','Restore declared scope'); } +test.beforeEach(async () => { root=mkdtempSync(join(tmpdir(),'codeboost-issue48-')); app=await startServer(createDemo(join(root,'demo')),0); }); +test.afterEach(async () => { await app.close(); rmSync(root,{recursive:true,force:true}); }); + +// Issue #48: an error from an older merge-queue poll must not overwrite the state a newer poll showed. Before the fix, +// selecting an item while poll #1 was held started poll #2 in the same merge generation; poll #2 confirmed "merged", +// and poll #1's later 500 replaced it. Now one poll chain runs at a time, so no newer poll exists while #1 is held: +// the test asserts that, shows #1's error as the latest result, and checks that polling then recovers. +test('a held merge-poll error starts no second poll chain, and polling recovers after it',async({page})=>{ + const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let appRef:typeof app,phase:'queued'|'merged'='queued'; + const gateway:MergeGateway&MergeQueueGateway={ + inspect:async()=>{const snapshot=(await appRef.service.load()).snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};}, + queueWatermark:async()=>null, + merge:async()=>({url:'https://github.com/example/repo/pull/24'}), + inspectQueue:async head=>phase==='queued'?{state:'queued',reviewedHead:head,entryId:'MQE_1',phase:'AWAITING_CHECKS',position:2,enqueuedAt:'2026-09-24T08:00:00Z',queueHead:head}:{state:'merged',reviewedHead:head,mergedAt:'2026-09-24T08:10:00Z'}, + }; + app=appRef=await startServer(config,0,undefined,gateway);let view=await app.service.load();for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=await app.service.act({action:'accept',key:segment.key,token:view.token});for(const item of view.items)view=await app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); + let release!:()=>void,arrived!:()=>void;const held=new Promise(resolve=>release=resolve),firstPollStarted=new Promise(resolve=>arrived=resolve);let polls=0; + await page.route('**/api/merge',async route=>{polls++; + if(polls===1){arrived();await held;await route.fulfill({status:500,contentType:'application/json',body:JSON.stringify({error:'Stale poll failure.'})});return;} + await route.continue();}); + await page.goto(app.url);page.on('dialog',dialog=>dialog.accept()); + await page.getByRole('button',{name:'Merge PR',exact:true}).click(); + await expect(page.getByRole('button',{name:'Merge queued',exact:true})).toBeDisabled(); + try{ + await firstPollStarted; + // GitHub now reports the merge. A plain item selection (no network write) re-renders while poll 1 is in flight; it + // must not start a second chain whose newer success the held error could then overwrite (wait past the 2 s delay). + phase='merged'; + await page.getByRole('button',{name:/P2 Document retry behavior/}).click(); + await page.waitForTimeout(3000); + expect(polls).toBe(1); + // The held error is the latest result and is shown; polling then continues and the newer state replaces it. + release(); + await expect(page.locator('#banner')).toContainText('Could not refresh merge status. Stale poll failure.'); + await expect(page.getByRole('button',{name:'Merged',exact:true})).toBeDisabled({timeout:15000}); + await expect(page.locator('#banner')).toContainText('GitHub confirmed the reviewed head was merged.'); + expect(polls).toBe(2); + }finally{release();} +}); + +// A current poll whose answer cannot be applied still reports the failure and keeps polling (an error thrown after the +// poll's result was accepted must not be dropped as if it came from an older poll). +test('reports an unusable current merge-poll answer and keeps polling',async({page})=>{ + const config={...app.service.config,demo:false};await app.close();removeDemoOutOfScope(config);let appRef:typeof app; + const gateway:MergeGateway&MergeQueueGateway={ + inspect:async()=>{const snapshot=(await appRef.service.load()).snapshot;return {base:snapshot.base,head:snapshot.head,pullRequestState:'OPEN',mergeable:'MERGEABLE',rulesKnown:true,atomicBaseGuard:true,mergeQueue:true,requiredChecks:[],alreadyFixed:'clear',pullRequest:7,draft:false};}, + queueWatermark:async()=>null, + merge:async()=>({url:'https://github.com/example/repo/pull/24'}), + inspectQueue:async head=>({state:'merged',reviewedHead:head,mergedAt:'2026-09-24T08:10:00Z'}), + }; + app=appRef=await startServer(config,0,undefined,gateway);let view=await app.service.load();for(const segment of view.segments.filter(value=>value.row==='Unplanned'||value.row==='Ambiguous'))view=await app.service.act({action:'accept',key:segment.key,token:view.token});for(const item of view.items)view=await app.service.act({action:'approve',item:item.id,confirmNoChange:item.count===0,token:view.token}); + let polls=0; + // Poll 1 answers 200 with a null body, which the page cannot apply; later polls pass. + await page.route('**/api/merge',async route=>{if(++polls===1){await route.fulfill({status:200,contentType:'application/json',body:'null'});return;}await route.continue();}); + await page.goto(app.url);page.on('dialog',dialog=>dialog.accept()); + await page.getByRole('button',{name:'Merge PR',exact:true}).click(); + await expect(page.locator('#banner')).toContainText('Could not refresh merge status.',{timeout:10000}); + await expect(page.getByRole('button',{name:'Merged',exact:true})).toBeDisabled({timeout:15000}); + expect(polls).toBe(2); +}); diff --git a/web/public/app.js b/web/public/app.js index 5ed6783e..202fd4ef 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -28,7 +28,11 @@ let mergeGeneration = 0, mergeObservationGeneration = 0, mergePollTimer = null, mergePollState = null, - mergePollDelay = 2000; + mergePollDelay = 2000, + // One merge poll chain at a time (#48): the poll in flight, by generation and sequence. A render while it runs does not + // start another, and only the latest poll's answer or error is applied. + mergePollSequence = 0, + mergePollInFlight = null; const mergePollMaximumDelay = 30000; const drafts = new Map(); const attachments = new Map(); @@ -227,6 +231,8 @@ function renderMerge() { scheduleMergePoll(); } function scheduleMergePoll() { + // The poll in flight reschedules itself when it settles; a second chain could apply an older answer after a newer one. + if (mergePollInFlight?.generation === mergeGeneration) return; if (mergePollTimer) clearTimeout(mergePollTimer); mergePollTimer = null; const queue = data?.merge?.queue; @@ -248,9 +254,23 @@ function backOffMergePoll() { mergePollDelay = Math.min(mergePollDelay * 2, mergePollMaximumDelay); } async function pollMergeQueue(generation) { + mergePollTimer = null; + const sequence = ++mergePollSequence; + mergePollInFlight = { generation, sequence }; + // Settles this poll's ownership and says whether its result may still be applied: only the latest poll's, and only + // while its generation is current. + const settle = () => { + const latest = mergePollInFlight?.sequence === sequence; + if (latest) mergePollInFlight = null; + return latest && generation === mergeGeneration && !!data?.merge?.available; + }; + // Decided once, when the request settles: an error thrown while applying a current answer is still reported, and + // polling still continues. + let current; try { const update = await api("/api/merge"); - if (generation !== mergeGeneration || !data?.merge?.available) return; + current = settle(); + if (!current) return; if (data.merge.job?.state === "running") { // A full review response read before this observation must not restore an older record of this job. mergeObservationGeneration++; @@ -278,7 +298,8 @@ async function pollMergeQueue(generation) { if (queue?.state === mergePollState) backOffMergePoll(); renderMerge(); } catch (error) { - if (generation !== mergeGeneration || !data?.merge?.available) return; + current ??= settle(); + if (!current) return; $("banner").textContent = `Could not refresh merge status. ${error.message}`; backOffMergePoll(); scheduleMergePoll(); @@ -696,6 +717,10 @@ $("merge").onclick = async () => { const busyOwner = beginBusy("write"); reviewGeneration++; mergeGeneration++; + // Defensive, like every generation change: a click needs `ready`, which excludes a running job or an active attempt, + // so no poll timer is pending here today (traced, not tested). + if (mergePollTimer) clearTimeout(mergePollTimer); + mergePollTimer = null; mergePollState = null; mergePollDelay = 2000; // With merge jobs, the click returns at once and the focused control must keep focus: mark it, do not disable it.