Uh oh!
There was an error while loading. Please reload this page.
fix(harness): give the workflow registry's temp file a per-process name - #659
Open
memosr wants to merge 1 commit into
Open
fix(harness): give the workflow registry's temp file a per-process name#659memosr wants to merge 1 commit into
memosr wants to merge 1 commit into
Conversation
persist() wrote to a fixed `${registryPath}.tmp`, with a comment claiming it mirrors SessionManager.persist(). It does not: that one uses .tmp-${pid}-${seq}, and so do workspace-context, store-retention and record-archive. This was the only atomic writer left on a shared temp name.
writeQueue serializes writers within one instance, but the default registry path is machine-wide (~/.sapiom/harness/workflows.json), so a CLI and a desktop app, or two Studio servers, run two registries over the same file. Concurrent writes then land in the same temp: one renames it away and the other fails, or worse, publishes a half-written file that load() swallows via catch { this.workflows = [] } - silent loss of connected workflows.
Add a pid + uuid suffix and clean the temp up on failure. The new test fails on the old code with ENOENT on rename and passes on the new one. The existing atomicity test asserted against the fixed temp name, which would have passed regardless once the name changed, so it now matches on the prefix instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Primary change type
Problem and motivation
WorkflowRegistry.persist()wrote to a fixed temp path:The comment is not accurate. Every other atomic writer in the package uses a per-process name:
core/session-manager.ts:1217.tmp-${pid}-${seq}core/workspace-context.ts:145.tmp-${pid}-${uuid}core/collector/store-retention.ts:127-tmp-${pid}-${Date.now()}core/record-archive.ts:420-tmp-${pid}-${now}-${counter}This was the only one left on a shared name.
writeQueueserializes writers within one instance, but the default registry path is machine-wide:expandHome(HARNESS_PATHS.workflows), i.e.~/.sapiom/harness/workflows.json. A CLI and a desktop app, or two Studio servers for two projects, run two registries over that one file.When two
/api/workflows/connector/scancalls overlap, both write into the same temp. Either one renames it away and the other fails, or the rename publishes a half-written file.ensureLoaded()swallows broken JSON withcatch { this.workflows = [] }, so the visible symptom is connected workflows silently disappearing. The rename is atomic, but sharing the temp voids the guarantee the comment claims.Summary and scope
Add a
pid+uuidsuffix to the temp name, and remove the temp file if the write or rename throws.Out of scope: the other atomic writers are already correct and are not touched. The broader question of whether two harness instances should share one registry file at all is left alone; this only makes the existing atomic-write claim hold.
Related work
Related issue or discussion: N/A — direct PR, focused bug fix with a reproduction, under the direct-PR policy.
Validation
The new test fails on the unpatched code:
I verified this by reverting only the temp-name line and re-running the file.
Tests and documentation
Tests: added a case where two registries over one path scan concurrently, asserting the persisted file is intact and no temp artefact is orphaned.
The existing C4 atomicity test asserted
fs.access(${registryPath}.tmp)rejects. That would pass for free once the name changed, and would not catch a temp orphaned under a different name, so it now matches on the.tmpprefix across the directory instead.Documentation: N/A, internal behavior only. The inaccurate code comment is corrected in place.
Compatibility and release impact
@sapiom/harness).Security
AI assistance
I used Claude (Claude Code and the Claude app). Claude surveyed the package for atomic writers and reported the inconsistency; I checked each of the four call sites myself and confirmed the registry path is machine-wide before accepting the finding. I directed the fix and the regression test, and Claude wrote them. I validated the test by reverting only the temp-name line and confirming the test fails with the ENOENT above, so it is a real regression test rather than one that passes either way.
Checklist
CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.