fix(#6052): treat unstable_cache as a function wrapper, not an un-emitted const by arthurgeron · Pull Request #6053 · cajasmota/grafel

arthurgeron · 2026-07-31T02:46:21Z

unstable_cache joins useCallback and useMemo in the isFunctionWrapperCall switch, so const getItems = unstable_cache(fn, keys, opts) is emitted as SCOPE.Operation instead of falling through every branch un-emitted.

Three shapes change, all of them the wrapper branch (extractor.go:1896-1930) taking a declaration that previously reached a later branch or none:

At module scope with no type annotation, nothing was emitted; now a SCOPE.Operation subtype function is.
With a TypeScript type annotation, const getItems: Loader = unstable_cache(...) emitted SCOPE.Component subtype const through the :1931 branch. The wrapper branch runs first, so it is now SCOPE.Operation subtype function, which is what useCallback already gets.
Inside a function body, nothing was emitted (the final else is gated on funcDepth == 0); now the entity is emitted and tagged local_scope=true by :1928-1930, the same as any other wrapper call in a body. The wrapper branch also runs extractCallRelationships at :1907, so calls made in the callback are attributed to the const name.

Other notes:

The switch matches on the callee leaf identifier with no import tracking, so the entry is only as safe as the name. unstable_cache is specific enough to next/cache for that to hold. I did not add React's cache(), which has the same "returns a memoized function" shape, because the bare name cache is common enough in ordinary code to misclassify it.
Declarations inside the callback are unchanged in every case. walkChildren at :1969 recurses into the value regardless of which branch emits.
Nothing else in the switch moves. The neighbouring next/cache names in internal/external/synth.go:10662-10670 (revalidatePath, revalidateTag, unstable_noStore, unstable_expireTag, unstable_expirePath, cacheTag, cacheLife) are called for their effect rather than bound to a name, so they stay out, and useEffect stays out for the reason the existing comment gives.
The extractor instance is registered for both javascript and typescript (extractor.go:71-75), so this applies to TS sources too.

TestConstExportNextUnstableCache sits beside TestConstExportReactWrappers and follows its shape. Reverting extractor.go alone and keeping the test gives extractor_test.go:779: entity "getItems" not found; got names: [test.go].

go test ./... green (229 packages, no failures), go vet ./internal/extractors/javascript/ and gofmt -l internal/extractors/javascript clean.

On the coverage matrix: the five hoc_wrapper_recognition cells are per-framework (expo, ionic, nativescript, react, react-native) and there is no Next.js one, so I have left docs/coverage/registry.json alone rather than invent a slot. The react cell does cite internal/extractors/javascript/extractor.go with no name list, so if you would rather this land as a verified_at bump and a note there, say so and I will add it. go run ./tools/coverage validate exits 0 and gen leaves docs/coverage byte-identical either way.

Closes #6052

…an un-emitted const

cajasmota · 2026-07-31T14:27:31Z

Merged — thank you, this is a genuinely well-made change.

The thing I want to call out first: you supplied a working revert-mutation, unprompted. Reverting extractor.go alone while keeping the test gives exactly what you said it gives —

--- FAIL: TestConstExportNextUnstableCache
    extractor_test.go:779: entity "getItems" not found; got names: [test.go]

Verbatim, same line, same message. A test that passes is easy; a test that you have demonstrated fails for the right reason when the fix is removed is a different quality of contribution, and it is not something I see often.

A few more things you got right that are worth naming:

Every line number you cite is exact — in the issue and the PR, against both main and your branch. :1896, :1907, :1931, :1944, :1955, :1969, and the dual registration at :71-75. Nothing rounded or approximated, which made this reviewable directly against the tree.

Your branch-order reasoning holds. You correctly predicted that the wrapper branch at :1896 running before the type-annotation branch at :1931 is what makes the TS-annotated case flip from SCOPE.Component/const to SCOPE.Operation/function rather than emit twice. Confirmed by probe on all three shapes.

The local_scope observation is precise and load-bearing. Noticing that :1928-1930 tags in-function wrapper calls is exactly what keeps the new entities out of grafel_find, via the filter in internal/mcp/denoise.go:167. That's a non-obvious downstream consequence and you found it without being pointed at it.

You picked the right equivalence class. I ran your four shapes with useCallback substituted for unstable_cache: the output is byte-identical modulo the callee name, down to the entity ID. That's the best possible result for an extractor change — no new code path, just one more name in a class the codebase already exercises everywhere.

Your next/cache sibling analysis is correct. revalidatePath, revalidateTag, unstable_noStore, unstable_expireTag, unstable_expirePath, cacheTag, cacheLife are effect calls rather than name-bound wrappers and belong out of the switch. I probed unstable_cacheLife to check, and it emits nothing, correctly.

And the restraint in the issue — explicitly declining to claim references break, because you'd read the :1873-1878 structural-ref mechanism and knew you couldn't assert it — is rare and appreciated.

Two things I'm taking off your plate

The coverage registry: you were right, and closer than you thought. There genuinely is no Next.js cell in the hoc_wrapper_recognition row — you were correct not to invent one. What you couldn't have known is that a Next.js Internals record does exist at records[415], carrying middleware_runtime_detection, server_actions and friends. Since hoc_wrapper_recognition is an established cell name used by five sibling frameworks, adding it there extends the matrix rather than inventing a slot. That's our call to make, so I'll do it — no action needed from you.

React's cache(): your instinct was good, and stricter than our own precedent. You excluded it because the bare name cache is common enough to misclassify, which is sound. Worth knowing for calibration: the switch already contains memo, css, compose, pipe, connect and styled — every one at least as collision-prone. So the bar you applied to yourself is higher than the one we hold ourselves to. const getUser = cache(...) still emits nothing today; that's now a known deliberate gap instead of an unknown one, which is a better position than before your PR, and deciding it is on us.

One follow-up your PR pointed at

Checking the neighbourhood you opened up: next/dynamic has the identical bug and is a complete blind spot — the strings dynamic and next/dynamic appear nowhere in internal/. So export const Chart = dynamic(() => import('./Chart'), { ssr: false }) emits nothing at all, even though React's lazy() is already in the switch and gets special handling (react_lazy=true plus a lazy_module stamp recovering the import specifier). Every Next.js code-split boundary is currently invisible. I'll file that separately.

I'll also backfill tests for the two shapes yours doesn't cover — the TS-annotated case and the in-function local_scope stamp. Your test does its job for the shape it targets; this is just us pinning the rest.

This lands in the upcoming 0.2.0. One incidental piece of good timing: the type-annotated case changes its stable entity ID (EntityID hashes Kind, and the kind flips), which would normally mean churn on re-index — but 0.2.0 already carries a graph-format bump that forces a full reindex, so it costs nothing.

Thanks again. Please do send more.

…precedent, warn when read-only is defeated, widen the guard F1 (blocker) — the budget was sized on the wrong axis. 8 retries with exponential backoff advertised a bigger wall-clock number while making the ATTEMPT COUNT worse, and attempt count is what has to exceed contention: the tests this fixes are 8-way concurrent (links 8x20=160 writes, atomicfile 8x40) with Defender and the indexer competing on top. Now 40 x 5ms ~200ms, matching internal/graph/groupalgo/atomicrename_windows.go, whose own comment reasons against a FOUR-reader stress test — strictly lighter load than 8 writers, so anything below it would be a regression in disguise. Vars not consts, as all five precedents are, so a test can shrink the delay. TestRenameOver_SurvivesLongContentionRun pins a 30-loss run (impossible at 8 retries, any delay); TestRenameBudget_MatchesInTreePrecedent pins the numbers. F3 (blocker) — clearing FILE_ATTRIBUTE_READONLY is defeating a human's "protect this file" checkbox. No production call site passes a read-only perm (every atomicfile.WriteFile in the tree is 0644 or 0600), so a read-only destination in the wild is ALWAYS user intent, and the replacement carries the caller's writable perm — the protection is permanently gone. Before #6053 grafel reported "Access is denied" and left it alone. The contract stands; the silence does not. slog.Warn at the point of clearing, pinned by TestRenameOver_WarnsWhenItDefeatsReadOnly plus a negative case so the transient path stays quiet. F2 (disclosure) — this is the SIXTH copy of the Windows retry loop, not the second: graph/{groupalgo,descriptions,flows}, statusfile, install. Comments in rename.go and candidates.go corrected. It now matches those five instead of inventing a dialect: golang.org/x/sys/windows constants (already a direct go.mod requirement, and internal/process — which the reaper fix now leans on — imports it), the errors.Is(err, fs.ErrPermission) first test all five have, and the same budget. rename.go states plainly that graph/{groupalgo,descriptions,flows}.atomicRename remain broken for a read-only destination and will now burn the full 200ms retrying something retrying cannot fix, and that ~50 sites still call bare os.Rename. The next Windows red on one of those is expected, not a regression from this change. Consolidating them is deliberately NOT done here — this branch is release-blocking. F5 — "invisible on unix" was false in one case, now named and tested. Old probe: Signal(0)==nil, so EPERM => DEAD. New process.IsAlive: EPERM => ALIVE. Only foreign-uid pids differ; zombies, pid<=0 and pid-reuse are identical. A foreign-uid watcher now reaches the ownership check instead of being silently dropped. The new behaviour is the defensible one and matches every other IsAlive site — reaper_eperm_unix_test.go pins it against pid 1 and asserts the fixture really produces EPERM before trusting the result. F6 — the guard walked only package daemon's directory. It now walks all of internal/ from the module root (exempting internal/process, the correct home) and matches signal-ZERO probes by regexp, including the Kill(pid, 0) spelling. Scoped to signal 0 deliberately: syscall.Kill with a real signal is legitimate and pre-existing (sched/nice_unix.go, supervise_unix.go SIGKILL a process group), and a broader rule would be three false positives that get deleted rather than obeyed. Comments are stripped before matching so pidfile.go's doc comment spelling out the idiom is not a hit. F7 — TestWriteFile_RoutesThroughRenameAtomic swaps a package global unsynchronised and is the ONLY test that can catch WriteFile reverting to a bare os.Rename, so it must not be weakened. TestNoParallelTestsInThisPackage refuses the first parallel opt-in in the package rather than merely documenting the hazard; its needle is assembled at run time so the guard cannot pass by exempting itself. F9 — renameOver now wraps the exhausted-budget error with the attempt count and elapsed budget. Without it a too-small budget and a missing retry loop produce byte-identical CI output.

@arthurgeron

…ct cache(), and a comment-node recall bug (#6055) * feat(#6054): complete the Next.js wrapper story — next/dynamic, React cache(), backfilled shapes Builds directly on #6053 (unstable_cache). Four items: 1. next/dynamic recognition (next_dynamic.go). Next.js's code-splitting primitive was a total blind spot — `dynamic` and `next/dynamic` appeared nowhere in internal/. Unlike unstable_cache, the bare name `dynamic` is a plausible ordinary identifier, so recognition is gated on argument shape: the first argument must be a function literal whose body performs a dynamic import(). Recognised entities reuse the React.lazy stamps — react_lazy=true (shared "this is a code-split boundary" semantics; Next implements dynamic on top of lazy + Suspense) and lazy_module via the same specifier-recovery rules — plus next_dynamic=true so the framework provenance is not lost. lazyImportModule's specifier walker is extracted as dynamicImportSpecifier so both primitives share one implementation. 2. React 19's cache(), gated on arity rather than accepted on the bare name. React's cache() takes exactly one argument; the collision shapes that actually occur in real code are the zero-arg factory and the multi-arg LRU constructor, and both are excluded. A "must be a function literal" gate was rejected: `cache(implFn)` is the majority real-world shape. 3. Backfilled the two #6053 shapes that were unpinned — TS type-annotated (the only shape whose classification changed, and the one a reordering of the wrapper/type-annotation branches would silently regress) and in-function-body including the local_scope stamp — for unstable_cache and for dynamic. Every fixture carries a same-file negative control proving it can exhibit the opposite outcome. The existing test now asserts Subtype as well as Kind. 4. hoc_wrapper_recognition added to the existing Next.js Internals record in docs/coverage/registry.json, the cell name five sibling framework records already use. Credit to @arthurgeron, whose #6052/#6053 made these gaps visible. Closes #6054 * fix(#6054): tighten the cache() gate, stop comments defeating both gates, pin the loader narrowing Addresses adversarial review of the #6054 branch. R1 (HIGH) — cache() accepted a broader shape-set than the corpus scan could measure. `leaf` reduces a member_expression callee to its property, and a `const X = cache(` scan sees only bare-identifier callees, so `store.cache(1000)`, `cache("config")`, `cache(1000)` and `cache(`+"`user:${id}`"+`)` all reached the rule and were emitted as SCOPE.Operation. `.cache(` outnumbers the bare form ~16:1 in the same corpus. The single argument must now not be a string, number or template literal — React's cache() never takes a key. All seven real corpus sites pass an identifier or a function literal, so recall is unchanged. Two residual FP shapes (`redisClient.cache(loaderFn)`, `cache(someKey)`) are accepted deliberately and documented in-source with their reasons. R2 (MEDIUM) — `comment` is a NAMED node in tree-sitter-javascript, so one argument-position comment defeated both new gates: `dynamic(/* lazy */ () => import("./Chart"))` and `cache(/* memoized */ getUser)` produced no entity at all, and `// eslint-disable-next-line` in argument position is common in real Next.js code. firstCallArg and callArgCount now skip comments. This also widens the pre-existing with<Capital> HOC rule, which is pinned deliberately. R3 (MEDIUM) — the loader narrowing IS observable and a mutation against it survived the previous battery. findAllNodes is a stack-based DFS yielding REVERSE document order, so a trailing `loading: () => import("./Spinner")` option is reached before the real split target. Added the fixture; the widening mutation now dies, including the sharper case where a computed specifier would gain a wrong lazy_module where the correct answer is none. R4 (LOW) — React 19's cache() had no coverage cell. Documented on the React record's hoc_wrapper_recognition cell with its gate, its residual FP shapes and its tests; the Next.js cell no longer implies it covers cache(). F4 — dynamicImportSpecifier's doc said "first" import; reverse DFS means last in document order. Corrected. F5 — the gate matches shape and callee leaf, never provenance, so next_dynamic is best-effort attribution: a user `function dynamic(f){}` or a `loaders.dynamic(() => import(...))` method call of the same shape is recognised too. Said so instead of claiming everything else is left alone. F6 — the narrowing stops at the loader boundary, not its return position, so a nested `registerPrefetch(() => import("./Prefetch"))` still stamps. Noted. Not touched, per review: the TestIncremental_Performance_SingleFileEdit budget and the CancelKillsGrandchildren flakes.

…hang was a go-winio deadlock, not a slowdown The windows-latest job died at `-timeout 15m` with internal/cli reported as 900.3s against 53.9s on main. That 17x is an artefact: the panic names a single running test (TestRunDaemonStop_RPCPath_ReportsFailureWhenStillRunning, 14m10s), so the package did 900.3 - 850 = 50.3s of actual work — 7% FASTER than main's 53.9s. Nothing got slower. One test deadlocked. Root cause, from the goroutine dump: the test goroutine is parked in t.Cleanup at go-winio@v0.6.2 pipe.go:578 (`<-l.doneCh` inside win32PipeListener.Close) while that same listener's listenerRoutine sits at pipe.go:462, the top-of-loop select, with closed == false. That state is reachable because makeConnectedServerPipe (pipe.go:429) also has a `case <-l.closeCh`. When an Accept is outstanding it consumes Close's one and only closeCh send, aborts the pending connect, and at pipe.go:452 maps the completion to ErrPipeListenerClosed — but only when it is nil or ErrFileClosed. A cancelled ConnectNamedPipe can instead complete with ERROR_OPERATION_ABORTED, which survives unchanged, so `closed = err == ErrPipeListenerClosed` at 479 is false, the loop returns to the top select, doneCh is never closed, and Close blocks forever. This is not only a test hazard. internal/daemon/server.go's graceful-shutdown path calls listener.Close() unbounded, so the same wedge means the Windows daemon never exits — the exact user-visible symptom of #6044. Fix: statsListener.Close (Windows only) goes through closeBounded. The stuck listenerRoutine is parked at 462 with a live `case <-l.closeCh`, so a second Close is precisely the wakeup it needs: it sets closed = true, closes the real handle and closes doneCh, releasing the original Close too. This produces a genuine, confirmed close, not an abandoned handle. 250ms per attempt sits two orders of magnitude above the slowest legitimate close (a cancelled kernel I/O completing), so a merely-slow close is never interrupted; 4 attempts caps the worst case at 1s and the helper reports errCloseNotConfirmed rather than hanging if it cannot converge. server.go now logs that instead of discarding it. Unix is untouched: net.UnixListener.Close is already bounded. Refuted en route, with evidence rather than assumption: - The 60s waitStopped/defaultReadiness asymmetry. `waitStopped`, `Probe(` and `defaultReadiness` appear nowhere in the 15-minute dump, and internal/daemon/service ran 4.591s on main vs 4.609s here. - The new source-guard walk and TestNoParallelTestsInThisPackage. No such frame in the dump, and every package other than internal/cli was same-or-faster (engine 106.4->80.7s, watch 47.6->41.0s, walk 25.2->19.9s, cmd/grafel 163.6->144.4s). - The rename retry sleeps. internal/atomicfile went FAIL 3.634s -> ok 3.003s. Tests model go-winio's state machine directly (it cannot be exercised off Windows) and pin both directions: the model provably deadlocks on a single Close when the abort is misclassified, and provably does not when it is classified correctly, so the recovery test is not vacuous. 8 mutations, all killed with test failures. Not verified locally: no Windows machine was available. The real go-winio behaviour, and the exact errno a cancelled ConnectNamedPipe reports, were read from the library source and the CI dump, not executed.

…root-cause attribution Three review findings, all confirmed against the on-disk go-winio source before acting. REQUIRED 1 (HIGH) — the bounded close did not bound shutdown; the hang moved one line down. server.go's `<-acceptDone` was a bare receive sitting BEFORE the watchdog select. When closeBounded returns errCloseNotConfirmed the listener is by definition still live, so listenerRoutine can still take the accept loop's next handoff and park for a client that never arrives: Accept never returns, acceptLoop never closes acceptDone, and Run wedges there instead. Wrapped in a select with watchdogCtx.Done(), which falls through to the existing force-exit path. The comments in server.go and closebounded.go asserted a bound the code did not deliver — the exact defect class this release is about — and are now accurate rather than aspirational. Pinned by two new tests behind a new listener seam (SetListenFuncForTest, mirroring SetShutdownExitFuncForTest). A real net.UnixListener cannot exhibit this — its Close always unblocks Accept — so the shape is injected with a listener that delegates Accept but whose Close reports failure without releasing it. Verified RED: with the bare receive restored, Run does not return within 8s. The paired anchor asserts a healthy shutdown still completes WITHOUT the force-exit firing, so the fix cannot degrade into force-exiting every shutdown. REQUIRED 2 (MEDIUM) — the documented root cause was ruled out by the module on disk. I named ERROR_OPERATION_ABORTED as the errno escaping pipe.go:452 and called it "not theoretical". It cannot occur in v0.6.2: file.go:194-197 remaps that errno to ErrFileClosed whenever f.closing is set, and closeHandle (file.go:117) sets closing before cancelIoEx and before the wg.Wait() that gates asyncIO's read of it, so the remap always fires. The reachable variant is a race rather than a classification gap — connectNamedPipe failing immediately returns an errno raw through asyncIO (file.go:175-177) and connectPipe (pipe.go:549-551), and the two-ready-case select at pipe.go:441 can pick the closeCh branch, so that raw errno escapes 452 verbatim. Plus the ERROR_NO_DATA re-entry at 470-477. No code change: the remedy was already errno-agnostic and covers every variant. The comment is rewritten to say what the dump actually proves — that SOME error escaped 452, not which one — and records why the obvious candidate is wrong, so a future maintainer bumping go-winio is not misled. The test sentinel is renamed errUnclassifiedModel and its doc now states the conditional the model really establishes: IF an error escapes 452, THEN Close deadlocks and a retry fixes it. REQUIRED 3 (LOW) — closeBoundedFor could report failure for a close that succeeded. It returned whichever attempt landed first, so a retry's "use of closed network connection" could mask attempt 0's success — and server.go now surfaces any non-nil as a warning, making that a false alarm. Results are now tagged with their attempt: attempt 0 is authoritative, any nil confirms the close, and a later attempt's error is discarded as uninformative. Also documented, per review: errCloseNotConfirmed leaks up to closeMaxAttempts goroutines blocked inside the wedged Close (unrecoverable by construction), and callers must not treat this call as their shutdown bound. Both now stated on the error itself, and the leak bound is pinned by a test. closeMaxAttempts stays 4. The review is right that 2 suffices for the parked-at-462 shape — closeCh is unbuffered, so a retry is never lost, it queues as a pending sender. The headroom is justified instead by the ERROR_NO_DATA re-entry variant, which can consume a closeCh value per iteration; the constant's comment now says that rather than the vaguer earlier claim. Mutation battery rebuilt from scratch: 13 mutations, 13 killed, all real test failures. M9/M10 pin both directions of the attribution rule, M11 the leak bound, M12/M13 both directions of the acceptDone watchdog. Correcting one attribution from my previous report: the guard against the fixture pair going vacuous is M1 and M3, not TestCloseBounded_ProductionConstantsCanRecover, which only checks the constants and never touches the model. Out of scope, pre-existing, reported not fixed: `go test ./internal/daemon/ -race` has a data race in supervise.go:98 vs :329 (SetEngineChildCommandForTest's deferred restore vs a live engineSupervisor.run), reproduced on a clean stashed tree with none of these changes applied. Still not verified: no Windows machine. The go-winio behaviour is read from source and the CI dump, never executed.

… reaper that doesn't wipe the registry, and a shutdown that terminates (#6058) * fix(#6053): make atomic writes survive Windows; stop the reaper reaping every live watcher there Windows CI has been red since #6018 landed. Three distinct defects, one of which loses user data. CLASS A — atomicfile loses writes on Windows. Go's os.Rename on Windows IS MoveFileEx(REPLACE_EXISTING), so reaching for MoveFileEx by hand buys nothing. What it cannot do is (1) replace a destination carrying FILE_ATTRIBUTE_READONLY — ERROR_ACCESS_DENIED, deterministically, forever — or (2) replace a destination another handle holds without FILE_SHARE_DELETE (a concurrent replacer's own in-flight MoveFileEx, Defender's scan of the just-created temp, the search indexer). internal/links' concurrency test caught the second: 18 of 160 writes dropped, i.e. a link pass's output silently lost. renameAtomic handles both, in that order: clear the read-only attribute without sleeping (retrying a file attribute is pointless), restoring it if we still fail to replace; then a bounded ~510ms exponential retry for the transient case. On unix the platform predicates are constant false, so WriteFile stays exactly one os.Rename. The decision logic lives in rename.go behind injectable primitives so it is unit-tested on every host, not only on windows-latest — no one here can run Windows on demand. Only the errno classification and the os.Chmod/FILE_ATTRIBUTE_READONLY mapping are Windows-only, and those have their own tests that run there. internal/links kept a private copy of the helper #6018 generalised, still calling os.Rename. It now delegates; a fourth copy is how this got back in. CLASS B — Windows has no Unix permission bits: os.Stat reports 0666 or 0444 and nothing else, so `want 0644` tests the platform, not the product. New testsupport.AssertPerm adapts rather than skips: on Windows it still asserts writability, and a 0444 row was added to each perm table because that is the one mode Windows CAN represent, so a deleted chmod still fails there. No security guarantee is weakened — the narrowest mode any call site passes is 0600 and no credential is written through this path. CLASS C — real Windows defect, not a test artifact. reaper.go carried its own pidAliveProbe (signal-0, doc comment: "portable on darwin/linux"). On Windows (*os.Process).Signal returns EWINDOWS for every pid, so it reported every live watcher DEAD. watchreg.Sweep checks !Alive before the ownership comparison, so #5933's fail-closed orphan contract was unreachable dead code there and the daemon dropped its entire watcher inventory each sweep. process.IsAlive has had a correct OpenProcess/GetExitCodeProcess implementation all along and pidfile.go already used it. A source-level guard now fails if a private signal-0 probe reappears — no behavioural unix test can catch that. * test(#6053): bind WriteFile to renameAtomic — the only unix-visible signal of a revert On unix renameAtomic compiles down to a single os.Rename, so swapping it back for os.Rename in WriteFile passed the entire suite here and would only have resurfaced as lost writes on windows-latest. It survived the mutation battery until this test intercepted defaultRenameOps.rename and counted the call. * fix(#6053): review round 2 — align the retry budget with the in-tree precedent, warn when read-only is defeated, widen the guard F1 (blocker) — the budget was sized on the wrong axis. 8 retries with exponential backoff advertised a bigger wall-clock number while making the ATTEMPT COUNT worse, and attempt count is what has to exceed contention: the tests this fixes are 8-way concurrent (links 8x20=160 writes, atomicfile 8x40) with Defender and the indexer competing on top. Now 40 x 5ms ~200ms, matching internal/graph/groupalgo/atomicrename_windows.go, whose own comment reasons against a FOUR-reader stress test — strictly lighter load than 8 writers, so anything below it would be a regression in disguise. Vars not consts, as all five precedents are, so a test can shrink the delay. TestRenameOver_SurvivesLongContentionRun pins a 30-loss run (impossible at 8 retries, any delay); TestRenameBudget_MatchesInTreePrecedent pins the numbers. F3 (blocker) — clearing FILE_ATTRIBUTE_READONLY is defeating a human's "protect this file" checkbox. No production call site passes a read-only perm (every atomicfile.WriteFile in the tree is 0644 or 0600), so a read-only destination in the wild is ALWAYS user intent, and the replacement carries the caller's writable perm — the protection is permanently gone. Before #6053 grafel reported "Access is denied" and left it alone. The contract stands; the silence does not. slog.Warn at the point of clearing, pinned by TestRenameOver_WarnsWhenItDefeatsReadOnly plus a negative case so the transient path stays quiet. F2 (disclosure) — this is the SIXTH copy of the Windows retry loop, not the second: graph/{groupalgo,descriptions,flows}, statusfile, install. Comments in rename.go and candidates.go corrected. It now matches those five instead of inventing a dialect: golang.org/x/sys/windows constants (already a direct go.mod requirement, and internal/process — which the reaper fix now leans on — imports it), the errors.Is(err, fs.ErrPermission) first test all five have, and the same budget. rename.go states plainly that graph/{groupalgo,descriptions,flows}.atomicRename remain broken for a read-only destination and will now burn the full 200ms retrying something retrying cannot fix, and that ~50 sites still call bare os.Rename. The next Windows red on one of those is expected, not a regression from this change. Consolidating them is deliberately NOT done here — this branch is release-blocking. F5 — "invisible on unix" was false in one case, now named and tested. Old probe: Signal(0)==nil, so EPERM => DEAD. New process.IsAlive: EPERM => ALIVE. Only foreign-uid pids differ; zombies, pid<=0 and pid-reuse are identical. A foreign-uid watcher now reaches the ownership check instead of being silently dropped. The new behaviour is the defensible one and matches every other IsAlive site — reaper_eperm_unix_test.go pins it against pid 1 and asserts the fixture really produces EPERM before trusting the result. F6 — the guard walked only package daemon's directory. It now walks all of internal/ from the module root (exempting internal/process, the correct home) and matches signal-ZERO probes by regexp, including the Kill(pid, 0) spelling. Scoped to signal 0 deliberately: syscall.Kill with a real signal is legitimate and pre-existing (sched/nice_unix.go, supervise_unix.go SIGKILL a process group), and a broader rule would be three false positives that get deleted rather than obeyed. Comments are stripped before matching so pidfile.go's doc comment spelling out the idiom is not a hit. F7 — TestWriteFile_RoutesThroughRenameAtomic swaps a package global unsynchronised and is the ONLY test that can catch WriteFile reverting to a bare os.Rename, so it must not be weakened. TestNoParallelTestsInThisPackage refuses the first parallel opt-in in the package rather than merely documenting the hazard; its needle is assembled at run time so the guard cannot pass by exempting itself. F9 — renameOver now wraps the exhausted-budget error with the attempt count and elapsed budget. Without it a too-small budget and a missing retry loop produce byte-identical CI output. * fix(#6053): bound the Windows named-pipe listener close — the 15m CI hang was a go-winio deadlock, not a slowdown The windows-latest job died at `-timeout 15m` with internal/cli reported as 900.3s against 53.9s on main. That 17x is an artefact: the panic names a single running test (TestRunDaemonStop_RPCPath_ReportsFailureWhenStillRunning, 14m10s), so the package did 900.3 - 850 = 50.3s of actual work — 7% FASTER than main's 53.9s. Nothing got slower. One test deadlocked. Root cause, from the goroutine dump: the test goroutine is parked in t.Cleanup at go-winio@v0.6.2 pipe.go:578 (`<-l.doneCh` inside win32PipeListener.Close) while that same listener's listenerRoutine sits at pipe.go:462, the top-of-loop select, with closed == false. That state is reachable because makeConnectedServerPipe (pipe.go:429) also has a `case <-l.closeCh`. When an Accept is outstanding it consumes Close's one and only closeCh send, aborts the pending connect, and at pipe.go:452 maps the completion to ErrPipeListenerClosed — but only when it is nil or ErrFileClosed. A cancelled ConnectNamedPipe can instead complete with ERROR_OPERATION_ABORTED, which survives unchanged, so `closed = err == ErrPipeListenerClosed` at 479 is false, the loop returns to the top select, doneCh is never closed, and Close blocks forever. This is not only a test hazard. internal/daemon/server.go's graceful-shutdown path calls listener.Close() unbounded, so the same wedge means the Windows daemon never exits — the exact user-visible symptom of #6044. Fix: statsListener.Close (Windows only) goes through closeBounded. The stuck listenerRoutine is parked at 462 with a live `case <-l.closeCh`, so a second Close is precisely the wakeup it needs: it sets closed = true, closes the real handle and closes doneCh, releasing the original Close too. This produces a genuine, confirmed close, not an abandoned handle. 250ms per attempt sits two orders of magnitude above the slowest legitimate close (a cancelled kernel I/O completing), so a merely-slow close is never interrupted; 4 attempts caps the worst case at 1s and the helper reports errCloseNotConfirmed rather than hanging if it cannot converge. server.go now logs that instead of discarding it. Unix is untouched: net.UnixListener.Close is already bounded. Refuted en route, with evidence rather than assumption: - The 60s waitStopped/defaultReadiness asymmetry. `waitStopped`, `Probe(` and `defaultReadiness` appear nowhere in the 15-minute dump, and internal/daemon/service ran 4.591s on main vs 4.609s here. - The new source-guard walk and TestNoParallelTestsInThisPackage. No such frame in the dump, and every package other than internal/cli was same-or-faster (engine 106.4->80.7s, watch 47.6->41.0s, walk 25.2->19.9s, cmd/grafel 163.6->144.4s). - The rename retry sleeps. internal/atomicfile went FAIL 3.634s -> ok 3.003s. Tests model go-winio's state machine directly (it cannot be exercised off Windows) and pin both directions: the model provably deadlocks on a single Close when the abort is misclassified, and provably does not when it is classified correctly, so the recovery test is not vacuous. 8 mutations, all killed with test failures. Not verified locally: no Windows machine was available. The real go-winio behaviour, and the exact errno a cancelled ConnectNamedPipe reports, were read from the library source and the CI dump, not executed. * fix(#6053): review round 2 — bound <-acceptDone too, and correct the root-cause attribution Three review findings, all confirmed against the on-disk go-winio source before acting. REQUIRED 1 (HIGH) — the bounded close did not bound shutdown; the hang moved one line down. server.go's `<-acceptDone` was a bare receive sitting BEFORE the watchdog select. When closeBounded returns errCloseNotConfirmed the listener is by definition still live, so listenerRoutine can still take the accept loop's next handoff and park for a client that never arrives: Accept never returns, acceptLoop never closes acceptDone, and Run wedges there instead. Wrapped in a select with watchdogCtx.Done(), which falls through to the existing force-exit path. The comments in server.go and closebounded.go asserted a bound the code did not deliver — the exact defect class this release is about — and are now accurate rather than aspirational. Pinned by two new tests behind a new listener seam (SetListenFuncForTest, mirroring SetShutdownExitFuncForTest). A real net.UnixListener cannot exhibit this — its Close always unblocks Accept — so the shape is injected with a listener that delegates Accept but whose Close reports failure without releasing it. Verified RED: with the bare receive restored, Run does not return within 8s. The paired anchor asserts a healthy shutdown still completes WITHOUT the force-exit firing, so the fix cannot degrade into force-exiting every shutdown. REQUIRED 2 (MEDIUM) — the documented root cause was ruled out by the module on disk. I named ERROR_OPERATION_ABORTED as the errno escaping pipe.go:452 and called it "not theoretical". It cannot occur in v0.6.2: file.go:194-197 remaps that errno to ErrFileClosed whenever f.closing is set, and closeHandle (file.go:117) sets closing before cancelIoEx and before the wg.Wait() that gates asyncIO's read of it, so the remap always fires. The reachable variant is a race rather than a classification gap — connectNamedPipe failing immediately returns an errno raw through asyncIO (file.go:175-177) and connectPipe (pipe.go:549-551), and the two-ready-case select at pipe.go:441 can pick the closeCh branch, so that raw errno escapes 452 verbatim. Plus the ERROR_NO_DATA re-entry at 470-477. No code change: the remedy was already errno-agnostic and covers every variant. The comment is rewritten to say what the dump actually proves — that SOME error escaped 452, not which one — and records why the obvious candidate is wrong, so a future maintainer bumping go-winio is not misled. The test sentinel is renamed errUnclassifiedModel and its doc now states the conditional the model really establishes: IF an error escapes 452, THEN Close deadlocks and a retry fixes it. REQUIRED 3 (LOW) — closeBoundedFor could report failure for a close that succeeded. It returned whichever attempt landed first, so a retry's "use of closed network connection" could mask attempt 0's success — and server.go now surfaces any non-nil as a warning, making that a false alarm. Results are now tagged with their attempt: attempt 0 is authoritative, any nil confirms the close, and a later attempt's error is discarded as uninformative. Also documented, per review: errCloseNotConfirmed leaks up to closeMaxAttempts goroutines blocked inside the wedged Close (unrecoverable by construction), and callers must not treat this call as their shutdown bound. Both now stated on the error itself, and the leak bound is pinned by a test. closeMaxAttempts stays 4. The review is right that 2 suffices for the parked-at-462 shape — closeCh is unbuffered, so a retry is never lost, it queues as a pending sender. The headroom is justified instead by the ERROR_NO_DATA re-entry variant, which can consume a closeCh value per iteration; the constant's comment now says that rather than the vaguer earlier claim. Mutation battery rebuilt from scratch: 13 mutations, 13 killed, all real test failures. M9/M10 pin both directions of the attribution rule, M11 the leak bound, M12/M13 both directions of the acceptDone watchdog. Correcting one attribution from my previous report: the guard against the fixture pair going vacuous is M1 and M3, not TestCloseBounded_ProductionConstantsCanRecover, which only checks the constants and never touches the model. Out of scope, pre-existing, reported not fixed: `go test ./internal/daemon/ -race` has a data race in supervise.go:98 vs :329 (SetEngineChildCommandForTest's deferred restore vs a live engineSupervisor.run), reproduced on a clean stashed tree with none of these changes applied. Still not verified: no Windows machine. The go-winio behaviour is read from source and the CI dump, never executed.

fix(cajasmota#6052): treat unstable_cache as a function wrapper, not …

055dcea

…an un-emitted const

cajasmota merged commit 4cde539 into cajasmota:main Jul 31, 2026
1 check passed

cajasmota mentioned this pull request Jul 31, 2026

Next.js wrapper recognition: next/dynamic is invisible, two #6053 shapes unpinned, coverage cell missing, React cache() undecided #6054

Closed

cajasmota mentioned this pull request Jul 31, 2026

feat(#6054): complete Next.js wrapper recognition — next/dynamic, React cache(), and a comment-node recall bug #6055

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#6052): treat unstable_cache as a function wrapper, not an un-emitted const - #6053

fix(#6052): treat unstable_cache as a function wrapper, not an un-emitted const#6053
cajasmota merged 1 commit into
cajasmota:mainfrom
Blueprint-Finance:fix/js-unstable-cache-wrapper

arthurgeron commented Jul 31, 2026

Uh oh!

Uh oh!

cajasmota commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

arthurgeron commented Jul 31, 2026

Uh oh!

Uh oh!

cajasmota commented Jul 31, 2026

Two things I'm taking off your plate

One follow-up your PR pointed at

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants