Skip to content

fix(server): terminal subprocess polling no longer floods the PID space - #6377

Merged
shivamhwp merged 3 commits into
pingdotgg:mainfrom
SunkenInTime:fix/terminal-subprocess-poller-pid-churn
Aug 14, 2026
Merged

shivamhwp merged 3 commits into
pingdotgg:mainfrom
SunkenInTime:fix/terminal-subprocess-poller-pid-churn

refactor(server): model snapshot failure details as error attributes

f10cf45
Select commit
Loading
Failed to load commit list.
MacroscopeApp / Macroscope - Effect Service Conventions succeeded Aug 12, 2026 in 1m 23s

Effect Service Conventions: All clear

All clear

Details

Note

Your check run agent prompt is: .macroscope/check-run-agents/effect-service-conventions.md
More information about how Check Run Agents work can be found in our Help Center.


🔍 Agent activity — reasoning, no tool calls

💭 I'm checking the polling area and the warning log that includes the error object as the reason field. The issue is that logging the entire error object could reintroduce the serialized cause that was supposed to be removed — the convention requires keeping sanitized errors bounded without reintroducing legacy fields like cause alongside them.… I'm thinking through whether the pattern of providing a service instance via provideService aligns with the guidelines about not hiding dependencies in module globals or closures. The fetchProcessTableSnapshot function closes over processRunner through this mechanism, which is the same pattern that existed in the previous code, but I should verify this doesn't violate the dependency transparency principle. For the error type changes, TerminalSubprocessCheckError now has a command field with two literal values and optional diagnostic fields like exitCode and timedOut, which avoids encoding the same distinction twice. The cause field being optional makes sense since not all error constructions have a cause. The log warning pattern of including reason as the error object was already established in the codebase, so that's consistent.… I'm checking the TerminalSubprocessCheckError convention—it has both a command discriminator and diagnostic fields, which aligns with the guidelines. The cause field should be optional since not all constructions wrap a failure, though there's a subtlety about avoiding duplicate distinctions that I need to think through. For the Effect.catch at line 2016, the entire error channel is just this one tag, which makes it ambiguous whether to use catchTags instead. But since the pre-existing sibling at line 2035 also uses Effect.catch, I'll keep it consistent rather than flag it. The subprocessInspector option is a test seam injection that was already there, so that's fine. For provideService(ProcessRunner.ProcessRunner, processRunner), the pattern is pre-existing and necessary for type safety since Effect.context<never>() captures at makeWithOptions — the requirement gets satisfied from the environment rather than hidden in a closure, which is acceptable. resolvePosixPsCommand uses Effect.fn for a helper, which is fine. In posixProcessTableSnapshot, the error mapper... Mapping errors at failure boundaries... The test setup uses a Manager with layers, which isn't a convention issue. I'm also checking the `… Checking that ProcessRunError doesn't expose raw arguments or output, and verifying the test imports are clean. I should also confirm there's no type definition issue with TerminalSubprocessInspector and whether the renamed parseFirstChildPidFromPgrep function is still being referenced correctly.…


Agent Credits: 77 credits