mirror of
https://github.com/stablyai/orca.git
synced 2026-10-27 16:02:42 +00:00
fix(worktrees): only count stuck retirement listings, and fence the backoff on a monotonic clock
Two more review findings on the outstanding-listing cap:
- The cap counted every in-flight listing, not just the stuck ones, so it fired on healthy
machines. Nested workspaces give each repo its own scan key, so a few first-time backfills are
routinely in flight together; the third was rejected and its create then picked a name against
an unseeded registry. Only listings that have outlived their deadline occupy a slot now, which
is what the cap was always meant to bound — healthy scans finish in milliseconds.
- The backoff fence compared wall-clock times. The WSL gate this scan runs under deliberately
avoids wall time for exactly this ('would misjudge stuckness across laptop sleep or NTP steps');
a backward step pinned a namespace in its failure memo for the size of the step. Now monotonic.
A third finding — that a late listing writing into an entry a retry has replaced loses the answer —
was investigated and refuted. An entry is only ever read back through the map, and callers hold the
promise rather than the entry, so a write to a replaced entry cannot be observed. No guard added:
the test for it passed with and without one.
This commit is contained in:
@@ -88,6 +88,24 @@ describe('runRetirementBackfillScan', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it('never defers a healthy machine, however many namespaces seed at once', async () => {
|
||||
// Nested workspaces give every repo its own scan key, so several first-time backfills are
|
||||
// routinely in flight together. Counting merely-in-flight listings would reject the third and
|
||||
// let its create pick a name against an unseeded registry.
|
||||
const scans = Array.from({ length: RETIREMENT_BACKFILL_MAX_OUTSTANDING_SCANS + 2 }, () =>
|
||||
stallingScan()
|
||||
)
|
||||
const pending = scans.map((scan, index) =>
|
||||
runRetirementBackfillScan({}, `ns-${index}`, scan.run)
|
||||
)
|
||||
expect(getOutstandingRetirementBackfillScanCount()).toBe(0)
|
||||
|
||||
scans.forEach((scan, index) => scan.finish([`name-${index}`]))
|
||||
await expect(Promise.all(pending)).resolves.toEqual(
|
||||
scans.map((_, index) => new Set([`name-${index}`]))
|
||||
)
|
||||
})
|
||||
|
||||
it('keeps a listing that lands after the deadline instead of discarding the answer', async () => {
|
||||
// The WSL gate admits one scan at a time and allows it 60s — four times this deadline — so a
|
||||
// late-but-correct listing is the normal case there, not an edge case.
|
||||
|
||||
@@ -5,19 +5,20 @@ export const RETIREMENT_BACKFILL_SCAN_TIMEOUT_MS = 15_000
|
||||
/** How long a failed scan stays failed, so a stalled mount is not re-probed on every create. */
|
||||
export const RETIREMENT_BACKFILL_RETRY_AFTER_FAILURE_MS = 60_000
|
||||
|
||||
/** Listings this module may leave outstanding at once, process-wide.
|
||||
/** Listings that have outlived their deadline and are still holding a thread, process-wide.
|
||||
*
|
||||
* The deadline above abandons a scan, it cannot cancel one: an unabortable `readdir` keeps its
|
||||
* libuv threadpool thread until the OS releases it, and that pool defaults to four. So the
|
||||
* backoff alone only paces the leak — each lapse still stacks another stuck thread. This bounds
|
||||
* what retirement discovery can hold; it is not a guarantee the pool stays healthy, since the WSL
|
||||
* gate and its close lane hold threads of their own. Two leaves headroom for both. */
|
||||
* The deadline abandons a scan, it cannot cancel one: an unabortable `readdir` keeps its libuv
|
||||
* threadpool thread until the OS releases it, and that pool defaults to four. So the backoff alone
|
||||
* only paces the leak — each lapse still stacks another stuck thread. Only listings past their
|
||||
* deadline count, so healthy scans (which finish in milliseconds) never deny each other a slot.
|
||||
* This bounds what retirement discovery can hold; it is not a guarantee the pool stays healthy,
|
||||
* since the WSL gate and its close lane hold threads of their own. Two leaves headroom for both. */
|
||||
export const RETIREMENT_BACKFILL_MAX_OUTSTANDING_SCANS = 2
|
||||
|
||||
type BackfillScan = {
|
||||
/** Resolves with the discovered names, or rejects once the deadline lapses. */
|
||||
names: Promise<Set<string>>
|
||||
/** Epoch ms this scan may be retried after; 0 while it is in flight or has succeeded. */
|
||||
/** Monotonic ms this scan may be retried after; 0 while it is in flight or has succeeded. */
|
||||
retryAfter: number
|
||||
}
|
||||
|
||||
@@ -26,6 +27,12 @@ const scansByStore = new WeakMap<object, Map<string, BackfillScan>>()
|
||||
/** Process-wide on purpose: the threadpool these listings occupy is process-wide too. */
|
||||
let outstandingScans = 0
|
||||
|
||||
/** Monotonic, like the WSL gate's own stuck timer: wall time misjudges a backoff across laptop
|
||||
* sleep or an NTP step, either pinning a namespace in its failure memo or ending it early. */
|
||||
function monotonicNow(): number {
|
||||
return performance.now()
|
||||
}
|
||||
|
||||
export function getOutstandingRetirementBackfillScanCount(): number {
|
||||
return outstandingScans
|
||||
}
|
||||
@@ -54,8 +61,8 @@ function withScanDeadline(scan: Promise<Set<string>>): Promise<Set<string>> {
|
||||
* `readdir` had no deadline, so one stalled listing wedged this create and every later
|
||||
* generated-name create for the namespace until Orca restarted.
|
||||
*
|
||||
* Retrying a failure is therefore allowed, but only up to
|
||||
* `RETIREMENT_BACKFILL_MAX_OUTSTANDING_SCANS` listings still stuck in the kernel at once. */
|
||||
* Retrying a failure is therefore allowed, but only while fewer than
|
||||
* `RETIREMENT_BACKFILL_MAX_OUTSTANDING_SCANS` listings are still stuck past their deadline. */
|
||||
export function runRetirementBackfillScan(
|
||||
store: object,
|
||||
scanKey: string,
|
||||
@@ -67,7 +74,7 @@ export function runRetirementBackfillScan(
|
||||
scansByStore.set(store, storeScans)
|
||||
}
|
||||
const cached = storeScans.get(scanKey)
|
||||
if (cached && (cached.retryAfter === 0 || Date.now() < cached.retryAfter)) {
|
||||
if (cached && (cached.retryAfter === 0 || monotonicNow() < cached.retryAfter)) {
|
||||
return cached.names
|
||||
}
|
||||
if (outstandingScans >= RETIREMENT_BACKFILL_MAX_OUTSTANDING_SCANS) {
|
||||
@@ -82,25 +89,35 @@ export function runRetirementBackfillScan(
|
||||
}
|
||||
const entry: BackfillScan = { names: Promise.resolve(new Set()), retryAfter: 0 }
|
||||
const started = scan()
|
||||
outstandingScans += 1
|
||||
void started.then(
|
||||
(names) => {
|
||||
outstandingScans -= 1
|
||||
// A listing that lands after the deadline still holds the right answer, and under WSL gate
|
||||
// contention that is the common case rather than the edge one — the gate admits a single
|
||||
// scan at a time and gives it 60s, four times this deadline. Dropping it would leave the
|
||||
// namespace unseeded on exactly the mounts this feature exists to cover.
|
||||
if (entry.retryAfter !== 0) {
|
||||
entry.names = Promise.resolve(names)
|
||||
entry.retryAfter = 0
|
||||
}
|
||||
},
|
||||
() => {
|
||||
let settled = false
|
||||
let holdsSlot = false
|
||||
const releaseSlot = (): void => {
|
||||
settled = true
|
||||
if (holdsSlot) {
|
||||
outstandingScans -= 1
|
||||
holdsSlot = false
|
||||
}
|
||||
)
|
||||
}
|
||||
// Registered before the deadline wrapper so `settled` is already true when its catch runs, which
|
||||
// is how a scan that failed fast is told apart from one that outlived its deadline.
|
||||
void started.then((names) => {
|
||||
releaseSlot()
|
||||
// A listing that lands after the deadline still holds the right answer, and under WSL gate
|
||||
// contention that is the common case rather than the edge one — the gate admits a single scan
|
||||
// at a time and gives it 60s, four times this deadline. Dropping it would leave the namespace
|
||||
// unseeded on exactly the mounts this feature exists to cover. No liveness check is needed:
|
||||
// once a retry replaces this entry in the map nothing can read it again, so the write is inert.
|
||||
if (entry.retryAfter !== 0) {
|
||||
entry.names = Promise.resolve(names)
|
||||
entry.retryAfter = 0
|
||||
}
|
||||
}, releaseSlot)
|
||||
entry.names = withScanDeadline(started).catch((error: unknown) => {
|
||||
entry.retryAfter = Date.now() + RETIREMENT_BACKFILL_RETRY_AFTER_FAILURE_MS
|
||||
if (!settled) {
|
||||
outstandingScans += 1
|
||||
holdsSlot = true
|
||||
}
|
||||
entry.retryAfter = monotonicNow() + RETIREMENT_BACKFILL_RETRY_AFTER_FAILURE_MS
|
||||
throw error
|
||||
})
|
||||
storeScans.set(scanKey, entry)
|
||||
|
||||
Reference in New Issue
Block a user