tasks: keep a card waiting for its rework out of the role pool

changelog.d/1804-rework-lease.fixed.md

@@ -0,0 +1 @@
+A card waiting for its rework keeps its holder when its lease expires instead of being returned to the role queue: the open rework child is the card in flight, so the original is not re-offered to the role that sent it back and cannot be taken again by its producer ([#1804](https://github.com/ThinkFlowLab/FlyBot/issues/1804)).

core/src/host/tasks.js

@@ -628,6 +628,11 @@ export function install(rt) {
     for (const task of Object.values(store())) {
       const lease = leaseOf(task)
       if (lease === undefined || !Number.isFinite(lease.expiresAt) || lease.expiresAt > now) continue
+      // A card waiting for its rework is not abandoned work (#1804): the open child card is the
+      // one in flight, and putting the original back in a pool would offer it to the role that
+      // sent it back — or let the producer take it again — while the child is still open. It
+      // keeps its holder and `working` until `closeRework` moves it on.
+      if (Object.values(store()).some(other => other.reworkOf === task.id && openTask(other))) continue
       // Read once and clear before the requeue: a second read in the same tick sees no lease, so
       // two expired reads return the card once, not twice (#1729). The recovery is recorded on the
       // card itself, and the dead worker is dropped so the next claim starts with no state of it.

core/test/task-handoff-route.test.js

@@ -176,3 +176,27 @@ test('a role returns the card to its producer as a rework card the producer can
   assert.match(await app.track('d1', { claim: true, id: childId }), new RegExp(`^Claimed ${childId} `))
   assert.equal(app.rt.state.tasks[childId].claimedBy, 'd1')
 })
+
+test('a card waiting for its rework is not re-offered when its lease expires (#1804)', async (t) => {
+  const app = await start(t)
+  const id = app.idOf(await app.track('a', { tasks: [{ title: 'Build the route', handoff: { from: 'Lead', to: 'Dev' } }] }))
+  await app.drive()
+  await app.track('d1', { claim: true, id })
+  await app.track('d1', { tasks: [{ id, handoff: { from: 'Dev', to: 'Reviewer', reason: 'diff ready' } }] })
+  await app.drive()
+  await app.track('r1', { claim: true, id })
+  await app.track('r1', { tasks: [{ id, column: 'in-progress' }] })
+  const childId = app.idOf(await app.track('r1', { rework: { task: id, reason: 'no test for the return path' } }))
+  const offers = () => (app.text('r1').match(new RegExp(`Take ${id} `, 'g')) ?? []).length
+  const before = offers()
+
+  // The original waits for the child: recovering it would offer it back to the role that sent it
+  // back and let the producer take it again while the rework is open — the same work twice.
+  assert.equal(await app.rt.taskExpireLeases(Date.now() + 31 * 60 * 1000), 0, 'the card held for rework is not recovered')
+  assert.equal(app.rt.state.tasks[id].status, 'working', 'the original keeps waiting for its rework')
+  await app.drive()
+  assert.equal(offers(), before, 'the Reviewer is not offered the card it sent back')
+  assert.match(app.text('d1'), new RegExp(`Take ${childId} `), 'the producer is offered the rework card')
+  assert.match(await app.track('d1', { claim: true, id }), /no card for Dev in the handoff pool/, 'the producer cannot take the card its rework covers')
+  assert.match(await app.track('d1', { claim: true, id: childId }), new RegExp(`^Claimed ${childId} `), 'the producer takes the card that is in flight')
+})

docs/subsystems/core-host.md

@@ -626,7 +626,7 @@ Stage 3.2 of the task pool. One queue per role, one worker per claim, one owner
 
 - **Per-role cap.** `ROLE_QUEUE_CAP` (8) is the default ceiling for one role's queue in one group. A graph states its own through `cap` on the enqueued item, kept on the card as `queueCap`; a re-offer keeps the card's id and is never counted against itself. An enqueue over the ceiling is refused `queue-full` with a sentence naming the role and the cap (`the Dev queue already holds 8 items, its cap is 8`), so the caller knows which queue is full rather than seeing a silent null.
 - **Isolated workers.** A claim creates a worker (`worker-<hex>`, kept in `tasks.js`'s own map, capped at 64, oldest dropped), and the card names it in `worker`. `rt.taskWorkerState(id)` is the only read: an unknown id answers `undefined`, so a second claim cannot see the first's `files` or `vars`. `requeue` and an expired lease drop the worker, so the next claim starts clean.
-- **One owner for an expired lease.** `expireLeases` reads the lease once, clears it, then returns a queued card to its role queue — a second read in the same tick finds no lease and answers 0. The recovery is written to the card's `evidence` (`lease expired (held by <name>)`) within the existing eight-entry cap, the dead worker is dropped, and a card that is not a queued item only loses the dead lease. A live lease is untouched, so a restart with a live lease is a no-op; a claim on a live lease is refused `lease-held` naming the holder. `rt.taskExpireLeases` exposes the same read, and `rt.taskLease(id)` gives `{ holder, takenAt, expiresAt, live, heldBy }`; the workflow plugin reads the lease through `fb.tasks.lease(id)` and never keeps a copy.
+- **One owner for an expired lease.** `expireLeases` reads the lease once, clears it, then returns a queued card to its role queue — a second read in the same tick finds no lease and answers 0. A card that has an open `rework` child is not a queue item its holder abandoned: the rework decides where the card goes, so an expired lease on it is cleared without returning it to the pool, and the card keeps its holder and its `working` status until `closeRework` lands the child and closes it (#1804). The recovery is written to the card's `evidence` (`lease expired (held by <name>)`) within the existing eight-entry cap, the dead worker is dropped, and a card that is not a queued item only loses the dead lease. A live lease is untouched, so a restart with a live lease is a no-op; a claim on a live lease is refused `lease-held` naming the holder. `rt.taskExpireLeases` exposes the same read, and `rt.taskLease(id)` gives `{ holder, takenAt, expiresAt, live, heldBy }`; the workflow plugin reads the lease through `fb.tasks.lease(id)` and never keeps a copy.
 - **Measured deliveries.** `rt.taskCounters({ role?, run? })` counts `queued`, `claimed`, `working`, `blocked`, `accepted`, `failed`, `cancelled` per role and per run id from the rows, plus `deliveries` and `workerSeconds` (accepted cards with a lease, measured from `takenAt` to the card's last update). A rework chain is one accepted delivery (#1730): an accepted `rework` child whose `reworkOf` parent is also accepted folds into the parent's delivery, so a rejection→rework→accept cycle is two records and one delivery; a child whose parent never landed stays a delivery of its own, and a replay writes no rows so the count cannot double. `fb.tasks.counters(query)` is the plugin seam. No metric store is added.
 
 **Audit projection (#1730).** `rt.taskAudit(id)` renders one text block from the typed fields already on the card — `handoff` from→to/reason/at, each `evidence` entry (sha, pr, check, at), the `reworkOf` chain oldest→newest, and the current lease holder with its `takenAt`→`expiresAt` window. It is a read: nothing is written, minted or cached, so the same rows render the same text twice. A record stored before the slice carries none of the fields and answers the empty string, not a throw. The Project board puts the same text on its row as `audit` (only when non-empty) and the client shows it inside the existing card detail — no new endpoint and no new page. Tests: `core/test/task-audit.test.js`.

patches/ds-bot/files/src/host/tasks.js

@@ -628,6 +628,11 @@ export function install(rt) {
     for (const task of Object.values(store())) {
       const lease = leaseOf(task)
       if (lease === undefined || !Number.isFinite(lease.expiresAt) || lease.expiresAt > now) continue
+      // A card waiting for its rework is not abandoned work (#1804): the open child card is the
+      // one in flight, and putting the original back in a pool would offer it to the role that
+      // sent it back — or let the producer take it again — while the child is still open. It
+      // keeps its holder and `working` until `closeRework` moves it on.
+      if (Object.values(store()).some(other => other.reworkOf === task.id && openTask(other))) continue
       // Read once and clear before the requeue: a second read in the same tick sees no lease, so
       // two expired reads return the card once, not twice (#1729). The recovery is recorded on the
       // card itself, and the dead worker is dropped so the next claim starts with no state of it.

patches/ds-bot/files/test/task-handoff-route.test.js

@@ -176,3 +176,27 @@ test('a role returns the card to its producer as a rework card the producer can
   assert.match(await app.track('d1', { claim: true, id: childId }), new RegExp(`^Claimed ${childId} `))
   assert.equal(app.rt.state.tasks[childId].claimedBy, 'd1')
 })
+
+test('a card waiting for its rework is not re-offered when its lease expires (#1804)', async (t) => {
+  const app = await start(t)
+  const id = app.idOf(await app.track('a', { tasks: [{ title: 'Build the route', handoff: { from: 'Lead', to: 'Dev' } }] }))
+  await app.drive()
+  await app.track('d1', { claim: true, id })
+  await app.track('d1', { tasks: [{ id, handoff: { from: 'Dev', to: 'Reviewer', reason: 'diff ready' } }] })
+  await app.drive()
+  await app.track('r1', { claim: true, id })
+  await app.track('r1', { tasks: [{ id, column: 'in-progress' }] })
+  const childId = app.idOf(await app.track('r1', { rework: { task: id, reason: 'no test for the return path' } }))
+  const offers = () => (app.text('r1').match(new RegExp(`Take ${id} `, 'g')) ?? []).length
+  const before = offers()
+
+  // The original waits for the child: recovering it would offer it back to the role that sent it
+  // back and let the producer take it again while the rework is open — the same work twice.
+  assert.equal(await app.rt.taskExpireLeases(Date.now() + 31 * 60 * 1000), 0, 'the card held for rework is not recovered')
+  assert.equal(app.rt.state.tasks[id].status, 'working', 'the original keeps waiting for its rework')
+  await app.drive()
+  assert.equal(offers(), before, 'the Reviewer is not offered the card it sent back')
+  assert.match(app.text('d1'), new RegExp(`Take ${childId} `), 'the producer is offered the rework card')
+  assert.match(await app.track('d1', { claim: true, id }), /no card for Dev in the handoff pool/, 'the producer cannot take the card its rework covers')
+  assert.match(await app.track('d1', { claim: true, id: childId }), new RegExp(`^Claimed ${childId} `), 'the producer takes the card that is in flight')
+})