Skip to content

fix(runtime): join PTY finalization after a racing persist - #3082

Merged
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait
Aug 16, 2026
Merged

fix(runtime): join PTY finalization after a racing persist#3082
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

joins finalization when a real PTY exits before a queued control cut failed on CI with expected completed, got running even though #3075 did not touch runtime. Re-running the same commit passed.

writeStdin asked persistObservation whether to join finalization at call time. A real PTY can exit while that persist is still in flight, so the control returned the running snapshot. After persist, if driverExit / finalizeOnce is set, join the terminal record.

The test no longer assumes that first snapshot is already terminal: if it is still running, it waits with waitForTerminalShellRun (15s) before asserting completed.

Fixes#3077

Verification

  • npm --workspace @maka/core run build
  • npm --workspace @maka/runtime run build
  • node --test --test-name-pattern 'joins finalization when a real PTY exits before a queued control cut' packages/runtime/dist/__tests__/shell-run-manager.test.js5/5 repeats
  • Nearby queued-control cases — 3/3
  • Full shell-run-manager.test.js57 pass, 3 skip (Windows PowerShell)
  • biome check on the two files — clean

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

Grok authored the implementation, tests, and this PR description.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7be63958-f9a8-4f80-9f40-2e13e1eeab49

📥 Commits

Reviewing files that changed from the base of the PR and between 63f2702 and c562830.

📒 Files selected for processing (1)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

Problem solved

writeStdin could return a stale running snapshot when the PTY exited while persistObservation was in flight. The change re-joins finalization when driverExit or finalizeOnce indicates that finalization has started. This allows writeStdin to return the terminal record.

The test now waits for a terminal shell-run snapshot before it asserts completed, output dimensions, and revision consistency. It also covers PTY finalization during persistence, including resize, driver disposal, and cleanup.

Design assessment

The change extends the existing PTY finalization and durable observation flow. It does not create a parallel source of truth or change a public contract.

The production fix is small and located at the race point. The test synchronization and added regression coverage are necessary to verify asynchronous finalization. No deletion or simplification is evident without weakening coverage.

Validation and risks

Reported validation includes successful builds, targeted and full test passes, and clean Biome checks. Direct evidence of the current required-check status is unavailable, so that status remains unverified.

The main behavioral risk is that writeStdin can now wait for terminal finalization instead of returning an intermediate running snapshot when PTY exit or finalization has begun. The added wait could affect timing under those conditions. The regression tests cover this race and related cleanup behavior.

Review-relevant risks

No protected-area effect was identified in the current diff. The person performing the merge reviews the final diff, and a maintainer makes the final determination.

Walkthrough

writeStdin now joins PTY finalization when exit or finalization begins after persistence. The tests wait for the terminal snapshot before checking completion, dimensions, durable revision, and cleanup.

Changes

PTY finalization synchronization

Layer / File(s)Summary
Join finalization and validate terminal state
packages/runtime/src/shell-run-manager.ts, packages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin now waits for live.finished, marks the terminal record observed, and returns it. The tests cover terminal snapshots, PTY disposal, resize, final dimensions, and durable revision.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to c5628

This localized runtime change ensures PTY runs reach their terminal record after a racing persist; no actionable merge-blocking risk remains after normal checks.

Suggested reviewers:m4n5ter, astro-han, jackwener

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly and concisely describes the PTY finalization race fix.
Description check✅ PassedThe description includes the problem, issue reference, implementation, verification results, AI disclosure, and completed checklist items.
Linked Issues check✅ PassedThe implementation and tests address issue #3077 by joining PTY finalization after persistence races and waiting for terminal state.
Out of Scope Changes check✅ PassedAll changes are limited to PTY finalization behavior and tests required by issue #3077.
Ai Use Disclosure✅ PassedThe PR selects generative tooling and names Grok with scope; both introduced commits have standalone Generated-by: Grok trailers.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d3a6126-a1e6-4364-bf74-587d6dd3c61b

📥 Commits

Reviewing files that changed from the base of the PR and between 62cded2 and 63f2702.

📒 Files selected for processing (2)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts
  • packages/runtime/src/shell-run-manager.ts

Comment threadpackages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin decided whether to join finalization when persistObservation
was first called. A real PTY can exit while that persist is still in
flight, so the control returned a running snapshot. Re-join if
finalization has started, and wait for a terminal observation in the
queued-cut test instead of assuming the first snapshot is already done.
Fixesapache#3077
Generated-by: Grok
Hold the writeStdin persist until PTY finalization has started, then
assert the control result itself is completed. The existing queued-cut
test can still wait for a later snapshot.
Generated-by: Grok
@1625567290
1625567290force-pushed the fix/shell-run-finalization-wait branch from 63f2702 to c562830CompareAugust 16, 2026 03:10

@M4n5terM4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@M4n5ter
M4n5ter merged commit 2666a57 into apache:mainAug 16, 2026
12 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flaky test: shell-run-manager "joins finalization when a real PTY exits before a queued control cut"

2 participants

@1625567290@M4n5ter
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix(runtime): join PTY finalization after a racing persist by 1625567290 · Pull Request #3082 · apache/maka · GitHub
Skip to content

fix(runtime): join PTY finalization after a racing persist - #3082

Merged
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait
Aug 16, 2026
Merged

fix(runtime): join PTY finalization after a racing persist#3082
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

joins finalization when a real PTY exits before a queued control cut failed on CI with expected completed, got running even though #3075 did not touch runtime. Re-running the same commit passed.

writeStdin asked persistObservation whether to join finalization at call time. A real PTY can exit while that persist is still in flight, so the control returned the running snapshot. After persist, if driverExit / finalizeOnce is set, join the terminal record.

The test no longer assumes that first snapshot is already terminal: if it is still running, it waits with waitForTerminalShellRun (15s) before asserting completed.

Fixes#3077

Verification

  • npm --workspace @maka/core run build
  • npm --workspace @maka/runtime run build
  • node --test --test-name-pattern 'joins finalization when a real PTY exits before a queued control cut' packages/runtime/dist/__tests__/shell-run-manager.test.js5/5 repeats
  • Nearby queued-control cases — 3/3
  • Full shell-run-manager.test.js57 pass, 3 skip (Windows PowerShell)
  • biome check on the two files — clean

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

Grok authored the implementation, tests, and this PR description.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7be63958-f9a8-4f80-9f40-2e13e1eeab49

📥 Commits

Reviewing files that changed from the base of the PR and between 63f2702 and c562830.

📒 Files selected for processing (1)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

Problem solved

writeStdin could return a stale running snapshot when the PTY exited while persistObservation was in flight. The change re-joins finalization when driverExit or finalizeOnce indicates that finalization has started. This allows writeStdin to return the terminal record.

The test now waits for a terminal shell-run snapshot before it asserts completed, output dimensions, and revision consistency. It also covers PTY finalization during persistence, including resize, driver disposal, and cleanup.

Design assessment

The change extends the existing PTY finalization and durable observation flow. It does not create a parallel source of truth or change a public contract.

The production fix is small and located at the race point. The test synchronization and added regression coverage are necessary to verify asynchronous finalization. No deletion or simplification is evident without weakening coverage.

Validation and risks

Reported validation includes successful builds, targeted and full test passes, and clean Biome checks. Direct evidence of the current required-check status is unavailable, so that status remains unverified.

The main behavioral risk is that writeStdin can now wait for terminal finalization instead of returning an intermediate running snapshot when PTY exit or finalization has begun. The added wait could affect timing under those conditions. The regression tests cover this race and related cleanup behavior.

Review-relevant risks

No protected-area effect was identified in the current diff. The person performing the merge reviews the final diff, and a maintainer makes the final determination.

Walkthrough

writeStdin now joins PTY finalization when exit or finalization begins after persistence. The tests wait for the terminal snapshot before checking completion, dimensions, durable revision, and cleanup.

Changes

PTY finalization synchronization

Layer / File(s)Summary
Join finalization and validate terminal state
packages/runtime/src/shell-run-manager.ts, packages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin now waits for live.finished, marks the terminal record observed, and returns it. The tests cover terminal snapshots, PTY disposal, resize, final dimensions, and durable revision.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to c5628

This localized runtime change ensures PTY runs reach their terminal record after a racing persist; no actionable merge-blocking risk remains after normal checks.

Suggested reviewers:m4n5ter, astro-han, jackwener

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly and concisely describes the PTY finalization race fix.
Description check✅ PassedThe description includes the problem, issue reference, implementation, verification results, AI disclosure, and completed checklist items.
Linked Issues check✅ PassedThe implementation and tests address issue #3077 by joining PTY finalization after persistence races and waiting for terminal state.
Out of Scope Changes check✅ PassedAll changes are limited to PTY finalization behavior and tests required by issue #3077.
Ai Use Disclosure✅ PassedThe PR selects generative tooling and names Grok with scope; both introduced commits have standalone Generated-by: Grok trailers.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d3a6126-a1e6-4364-bf74-587d6dd3c61b

📥 Commits

Reviewing files that changed from the base of the PR and between 62cded2 and 63f2702.

📒 Files selected for processing (2)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts
  • packages/runtime/src/shell-run-manager.ts

Comment threadpackages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin decided whether to join finalization when persistObservation
was first called. A real PTY can exit while that persist is still in
flight, so the control returned a running snapshot. Re-join if
finalization has started, and wait for a terminal observation in the
queued-cut test instead of assuming the first snapshot is already done.
Fixesapache#3077
Generated-by: Grok
Hold the writeStdin persist until PTY finalization has started, then
assert the control result itself is completed. The existing queued-cut
test can still wait for a later snapshot.
Generated-by: Grok
@1625567290
1625567290force-pushed the fix/shell-run-finalization-wait branch from 63f2702 to c562830CompareAugust 16, 2026 03:10

@M4n5terM4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@M4n5ter
M4n5ter merged commit 2666a57 into apache:mainAug 16, 2026
12 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flaky test: shell-run-manager "joins finalization when a real PTY exits before a queued control cut"

2 participants

@1625567290@M4n5ter
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(runtime): join PTY finalization after a racing persist by 1625567290 · Pull Request #3082 · apache/maka · GitHub
Skip to content

fix(runtime): join PTY finalization after a racing persist - #3082

Merged
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait
Aug 16, 2026
Merged

fix(runtime): join PTY finalization after a racing persist#3082
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

joins finalization when a real PTY exits before a queued control cut failed on CI with expected completed, got running even though #3075 did not touch runtime. Re-running the same commit passed.

writeStdin asked persistObservation whether to join finalization at call time. A real PTY can exit while that persist is still in flight, so the control returned the running snapshot. After persist, if driverExit / finalizeOnce is set, join the terminal record.

The test no longer assumes that first snapshot is already terminal: if it is still running, it waits with waitForTerminalShellRun (15s) before asserting completed.

Fixes#3077

Verification

  • npm --workspace @maka/core run build
  • npm --workspace @maka/runtime run build
  • node --test --test-name-pattern 'joins finalization when a real PTY exits before a queued control cut' packages/runtime/dist/__tests__/shell-run-manager.test.js5/5 repeats
  • Nearby queued-control cases — 3/3
  • Full shell-run-manager.test.js57 pass, 3 skip (Windows PowerShell)
  • biome check on the two files — clean

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

Grok authored the implementation, tests, and this PR description.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7be63958-f9a8-4f80-9f40-2e13e1eeab49

📥 Commits

Reviewing files that changed from the base of the PR and between 63f2702 and c562830.

📒 Files selected for processing (1)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

Problem solved

writeStdin could return a stale running snapshot when the PTY exited while persistObservation was in flight. The change re-joins finalization when driverExit or finalizeOnce indicates that finalization has started. This allows writeStdin to return the terminal record.

The test now waits for a terminal shell-run snapshot before it asserts completed, output dimensions, and revision consistency. It also covers PTY finalization during persistence, including resize, driver disposal, and cleanup.

Design assessment

The change extends the existing PTY finalization and durable observation flow. It does not create a parallel source of truth or change a public contract.

The production fix is small and located at the race point. The test synchronization and added regression coverage are necessary to verify asynchronous finalization. No deletion or simplification is evident without weakening coverage.

Validation and risks

Reported validation includes successful builds, targeted and full test passes, and clean Biome checks. Direct evidence of the current required-check status is unavailable, so that status remains unverified.

The main behavioral risk is that writeStdin can now wait for terminal finalization instead of returning an intermediate running snapshot when PTY exit or finalization has begun. The added wait could affect timing under those conditions. The regression tests cover this race and related cleanup behavior.

Review-relevant risks

No protected-area effect was identified in the current diff. The person performing the merge reviews the final diff, and a maintainer makes the final determination.

Walkthrough

writeStdin now joins PTY finalization when exit or finalization begins after persistence. The tests wait for the terminal snapshot before checking completion, dimensions, durable revision, and cleanup.

Changes

PTY finalization synchronization

Layer / File(s)Summary
Join finalization and validate terminal state
packages/runtime/src/shell-run-manager.ts, packages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin now waits for live.finished, marks the terminal record observed, and returns it. The tests cover terminal snapshots, PTY disposal, resize, final dimensions, and durable revision.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to c5628

This localized runtime change ensures PTY runs reach their terminal record after a racing persist; no actionable merge-blocking risk remains after normal checks.

Suggested reviewers:m4n5ter, astro-han, jackwener

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly and concisely describes the PTY finalization race fix.
Description check✅ PassedThe description includes the problem, issue reference, implementation, verification results, AI disclosure, and completed checklist items.
Linked Issues check✅ PassedThe implementation and tests address issue #3077 by joining PTY finalization after persistence races and waiting for terminal state.
Out of Scope Changes check✅ PassedAll changes are limited to PTY finalization behavior and tests required by issue #3077.
Ai Use Disclosure✅ PassedThe PR selects generative tooling and names Grok with scope; both introduced commits have standalone Generated-by: Grok trailers.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d3a6126-a1e6-4364-bf74-587d6dd3c61b

📥 Commits

Reviewing files that changed from the base of the PR and between 62cded2 and 63f2702.

📒 Files selected for processing (2)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts
  • packages/runtime/src/shell-run-manager.ts

Comment threadpackages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin decided whether to join finalization when persistObservation
was first called. A real PTY can exit while that persist is still in
flight, so the control returned a running snapshot. Re-join if
finalization has started, and wait for a terminal observation in the
queued-cut test instead of assuming the first snapshot is already done.
Fixesapache#3077
Generated-by: Grok
Hold the writeStdin persist until PTY finalization has started, then
assert the control result itself is completed. The existing queued-cut
test can still wait for a later snapshot.
Generated-by: Grok
@1625567290
1625567290force-pushed the fix/shell-run-finalization-wait branch from 63f2702 to c562830CompareAugust 16, 2026 03:10

@M4n5terM4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@M4n5ter
M4n5ter merged commit 2666a57 into apache:mainAug 16, 2026
12 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flaky test: shell-run-manager "joins finalization when a real PTY exits before a queued control cut"

2 participants

@1625567290@M4n5ter
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(runtime): join PTY finalization after a racing persist by 1625567290 · Pull Request #3082 · apache/maka · GitHub
Skip to content

fix(runtime): join PTY finalization after a racing persist - #3082

Merged
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait
Aug 16, 2026
Merged

fix(runtime): join PTY finalization after a racing persist#3082
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

joins finalization when a real PTY exits before a queued control cut failed on CI with expected completed, got running even though #3075 did not touch runtime. Re-running the same commit passed.

writeStdin asked persistObservation whether to join finalization at call time. A real PTY can exit while that persist is still in flight, so the control returned the running snapshot. After persist, if driverExit / finalizeOnce is set, join the terminal record.

The test no longer assumes that first snapshot is already terminal: if it is still running, it waits with waitForTerminalShellRun (15s) before asserting completed.

Fixes#3077

Verification

  • npm --workspace @maka/core run build
  • npm --workspace @maka/runtime run build
  • node --test --test-name-pattern 'joins finalization when a real PTY exits before a queued control cut' packages/runtime/dist/__tests__/shell-run-manager.test.js5/5 repeats
  • Nearby queued-control cases — 3/3
  • Full shell-run-manager.test.js57 pass, 3 skip (Windows PowerShell)
  • biome check on the two files — clean

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

Grok authored the implementation, tests, and this PR description.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7be63958-f9a8-4f80-9f40-2e13e1eeab49

📥 Commits

Reviewing files that changed from the base of the PR and between 63f2702 and c562830.

📒 Files selected for processing (1)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

Problem solved

writeStdin could return a stale running snapshot when the PTY exited while persistObservation was in flight. The change re-joins finalization when driverExit or finalizeOnce indicates that finalization has started. This allows writeStdin to return the terminal record.

The test now waits for a terminal shell-run snapshot before it asserts completed, output dimensions, and revision consistency. It also covers PTY finalization during persistence, including resize, driver disposal, and cleanup.

Design assessment

The change extends the existing PTY finalization and durable observation flow. It does not create a parallel source of truth or change a public contract.

The production fix is small and located at the race point. The test synchronization and added regression coverage are necessary to verify asynchronous finalization. No deletion or simplification is evident without weakening coverage.

Validation and risks

Reported validation includes successful builds, targeted and full test passes, and clean Biome checks. Direct evidence of the current required-check status is unavailable, so that status remains unverified.

The main behavioral risk is that writeStdin can now wait for terminal finalization instead of returning an intermediate running snapshot when PTY exit or finalization has begun. The added wait could affect timing under those conditions. The regression tests cover this race and related cleanup behavior.

Review-relevant risks

No protected-area effect was identified in the current diff. The person performing the merge reviews the final diff, and a maintainer makes the final determination.

Walkthrough

writeStdin now joins PTY finalization when exit or finalization begins after persistence. The tests wait for the terminal snapshot before checking completion, dimensions, durable revision, and cleanup.

Changes

PTY finalization synchronization

Layer / File(s)Summary
Join finalization and validate terminal state
packages/runtime/src/shell-run-manager.ts, packages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin now waits for live.finished, marks the terminal record observed, and returns it. The tests cover terminal snapshots, PTY disposal, resize, final dimensions, and durable revision.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to c5628

This localized runtime change ensures PTY runs reach their terminal record after a racing persist; no actionable merge-blocking risk remains after normal checks.

Suggested reviewers:m4n5ter, astro-han, jackwener

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly and concisely describes the PTY finalization race fix.
Description check✅ PassedThe description includes the problem, issue reference, implementation, verification results, AI disclosure, and completed checklist items.
Linked Issues check✅ PassedThe implementation and tests address issue #3077 by joining PTY finalization after persistence races and waiting for terminal state.
Out of Scope Changes check✅ PassedAll changes are limited to PTY finalization behavior and tests required by issue #3077.
Ai Use Disclosure✅ PassedThe PR selects generative tooling and names Grok with scope; both introduced commits have standalone Generated-by: Grok trailers.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d3a6126-a1e6-4364-bf74-587d6dd3c61b

📥 Commits

Reviewing files that changed from the base of the PR and between 62cded2 and 63f2702.

📒 Files selected for processing (2)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts
  • packages/runtime/src/shell-run-manager.ts

Comment threadpackages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin decided whether to join finalization when persistObservation
was first called. A real PTY can exit while that persist is still in
flight, so the control returned a running snapshot. Re-join if
finalization has started, and wait for a terminal observation in the
queued-cut test instead of assuming the first snapshot is already done.
Fixesapache#3077
Generated-by: Grok
Hold the writeStdin persist until PTY finalization has started, then
assert the control result itself is completed. The existing queued-cut
test can still wait for a later snapshot.
Generated-by: Grok
@1625567290
1625567290force-pushed the fix/shell-run-finalization-wait branch from 63f2702 to c562830CompareAugust 16, 2026 03:10

@M4n5terM4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@M4n5ter
M4n5ter merged commit 2666a57 into apache:mainAug 16, 2026
12 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flaky test: shell-run-manager "joins finalization when a real PTY exits before a queued control cut"

2 participants

@1625567290@M4n5ter
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix(runtime): join PTY finalization after a racing persist by 1625567290 · Pull Request #3082 · apache/maka · GitHub
Skip to content

fix(runtime): join PTY finalization after a racing persist - #3082

Merged
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait
Aug 16, 2026
Merged

fix(runtime): join PTY finalization after a racing persist#3082
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

joins finalization when a real PTY exits before a queued control cut failed on CI with expected completed, got running even though #3075 did not touch runtime. Re-running the same commit passed.

writeStdin asked persistObservation whether to join finalization at call time. A real PTY can exit while that persist is still in flight, so the control returned the running snapshot. After persist, if driverExit / finalizeOnce is set, join the terminal record.

The test no longer assumes that first snapshot is already terminal: if it is still running, it waits with waitForTerminalShellRun (15s) before asserting completed.

Fixes#3077

Verification

  • npm --workspace @maka/core run build
  • npm --workspace @maka/runtime run build
  • node --test --test-name-pattern 'joins finalization when a real PTY exits before a queued control cut' packages/runtime/dist/__tests__/shell-run-manager.test.js5/5 repeats
  • Nearby queued-control cases — 3/3
  • Full shell-run-manager.test.js57 pass, 3 skip (Windows PowerShell)
  • biome check on the two files — clean

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

Grok authored the implementation, tests, and this PR description.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7be63958-f9a8-4f80-9f40-2e13e1eeab49

📥 Commits

Reviewing files that changed from the base of the PR and between 63f2702 and c562830.

📒 Files selected for processing (1)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

Problem solved

writeStdin could return a stale running snapshot when the PTY exited while persistObservation was in flight. The change re-joins finalization when driverExit or finalizeOnce indicates that finalization has started. This allows writeStdin to return the terminal record.

The test now waits for a terminal shell-run snapshot before it asserts completed, output dimensions, and revision consistency. It also covers PTY finalization during persistence, including resize, driver disposal, and cleanup.

Design assessment

The change extends the existing PTY finalization and durable observation flow. It does not create a parallel source of truth or change a public contract.

The production fix is small and located at the race point. The test synchronization and added regression coverage are necessary to verify asynchronous finalization. No deletion or simplification is evident without weakening coverage.

Validation and risks

Reported validation includes successful builds, targeted and full test passes, and clean Biome checks. Direct evidence of the current required-check status is unavailable, so that status remains unverified.

The main behavioral risk is that writeStdin can now wait for terminal finalization instead of returning an intermediate running snapshot when PTY exit or finalization has begun. The added wait could affect timing under those conditions. The regression tests cover this race and related cleanup behavior.

Review-relevant risks

No protected-area effect was identified in the current diff. The person performing the merge reviews the final diff, and a maintainer makes the final determination.

Walkthrough

writeStdin now joins PTY finalization when exit or finalization begins after persistence. The tests wait for the terminal snapshot before checking completion, dimensions, durable revision, and cleanup.

Changes

PTY finalization synchronization

Layer / File(s)Summary
Join finalization and validate terminal state
packages/runtime/src/shell-run-manager.ts, packages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin now waits for live.finished, marks the terminal record observed, and returns it. The tests cover terminal snapshots, PTY disposal, resize, final dimensions, and durable revision.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to c5628

This localized runtime change ensures PTY runs reach their terminal record after a racing persist; no actionable merge-blocking risk remains after normal checks.

Suggested reviewers:m4n5ter, astro-han, jackwener

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly and concisely describes the PTY finalization race fix.
Description check✅ PassedThe description includes the problem, issue reference, implementation, verification results, AI disclosure, and completed checklist items.
Linked Issues check✅ PassedThe implementation and tests address issue #3077 by joining PTY finalization after persistence races and waiting for terminal state.
Out of Scope Changes check✅ PassedAll changes are limited to PTY finalization behavior and tests required by issue #3077.
Ai Use Disclosure✅ PassedThe PR selects generative tooling and names Grok with scope; both introduced commits have standalone Generated-by: Grok trailers.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d3a6126-a1e6-4364-bf74-587d6dd3c61b

📥 Commits

Reviewing files that changed from the base of the PR and between 62cded2 and 63f2702.

📒 Files selected for processing (2)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts
  • packages/runtime/src/shell-run-manager.ts

Comment threadpackages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin decided whether to join finalization when persistObservation
was first called. A real PTY can exit while that persist is still in
flight, so the control returned a running snapshot. Re-join if
finalization has started, and wait for a terminal observation in the
queued-cut test instead of assuming the first snapshot is already done.
Fixesapache#3077
Generated-by: Grok
Hold the writeStdin persist until PTY finalization has started, then
assert the control result itself is completed. The existing queued-cut
test can still wait for a later snapshot.
Generated-by: Grok
@1625567290
1625567290force-pushed the fix/shell-run-finalization-wait branch from 63f2702 to c562830CompareAugust 16, 2026 03:10

@M4n5terM4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@M4n5ter
M4n5ter merged commit 2666a57 into apache:mainAug 16, 2026
12 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flaky test: shell-run-manager "joins finalization when a real PTY exits before a queued control cut"

2 participants

@1625567290@M4n5ter
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(runtime): join PTY finalization after a racing persist by 1625567290 · Pull Request #3082 · apache/maka · GitHub
Skip to content

fix(runtime): join PTY finalization after a racing persist - #3082

Merged
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait
Aug 16, 2026
Merged

fix(runtime): join PTY finalization after a racing persist#3082
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

joins finalization when a real PTY exits before a queued control cut failed on CI with expected completed, got running even though #3075 did not touch runtime. Re-running the same commit passed.

writeStdin asked persistObservation whether to join finalization at call time. A real PTY can exit while that persist is still in flight, so the control returned the running snapshot. After persist, if driverExit / finalizeOnce is set, join the terminal record.

The test no longer assumes that first snapshot is already terminal: if it is still running, it waits with waitForTerminalShellRun (15s) before asserting completed.

Fixes#3077

Verification

  • npm --workspace @maka/core run build
  • npm --workspace @maka/runtime run build
  • node --test --test-name-pattern 'joins finalization when a real PTY exits before a queued control cut' packages/runtime/dist/__tests__/shell-run-manager.test.js5/5 repeats
  • Nearby queued-control cases — 3/3
  • Full shell-run-manager.test.js57 pass, 3 skip (Windows PowerShell)
  • biome check on the two files — clean

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

Grok authored the implementation, tests, and this PR description.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7be63958-f9a8-4f80-9f40-2e13e1eeab49

📥 Commits

Reviewing files that changed from the base of the PR and between 63f2702 and c562830.

📒 Files selected for processing (1)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

Problem solved

writeStdin could return a stale running snapshot when the PTY exited while persistObservation was in flight. The change re-joins finalization when driverExit or finalizeOnce indicates that finalization has started. This allows writeStdin to return the terminal record.

The test now waits for a terminal shell-run snapshot before it asserts completed, output dimensions, and revision consistency. It also covers PTY finalization during persistence, including resize, driver disposal, and cleanup.

Design assessment

The change extends the existing PTY finalization and durable observation flow. It does not create a parallel source of truth or change a public contract.

The production fix is small and located at the race point. The test synchronization and added regression coverage are necessary to verify asynchronous finalization. No deletion or simplification is evident without weakening coverage.

Validation and risks

Reported validation includes successful builds, targeted and full test passes, and clean Biome checks. Direct evidence of the current required-check status is unavailable, so that status remains unverified.

The main behavioral risk is that writeStdin can now wait for terminal finalization instead of returning an intermediate running snapshot when PTY exit or finalization has begun. The added wait could affect timing under those conditions. The regression tests cover this race and related cleanup behavior.

Review-relevant risks

No protected-area effect was identified in the current diff. The person performing the merge reviews the final diff, and a maintainer makes the final determination.

Walkthrough

writeStdin now joins PTY finalization when exit or finalization begins after persistence. The tests wait for the terminal snapshot before checking completion, dimensions, durable revision, and cleanup.

Changes

PTY finalization synchronization

Layer / File(s)Summary
Join finalization and validate terminal state
packages/runtime/src/shell-run-manager.ts, packages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin now waits for live.finished, marks the terminal record observed, and returns it. The tests cover terminal snapshots, PTY disposal, resize, final dimensions, and durable revision.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to c5628

This localized runtime change ensures PTY runs reach their terminal record after a racing persist; no actionable merge-blocking risk remains after normal checks.

Suggested reviewers:m4n5ter, astro-han, jackwener

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly and concisely describes the PTY finalization race fix.
Description check✅ PassedThe description includes the problem, issue reference, implementation, verification results, AI disclosure, and completed checklist items.
Linked Issues check✅ PassedThe implementation and tests address issue #3077 by joining PTY finalization after persistence races and waiting for terminal state.
Out of Scope Changes check✅ PassedAll changes are limited to PTY finalization behavior and tests required by issue #3077.
Ai Use Disclosure✅ PassedThe PR selects generative tooling and names Grok with scope; both introduced commits have standalone Generated-by: Grok trailers.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d3a6126-a1e6-4364-bf74-587d6dd3c61b

📥 Commits

Reviewing files that changed from the base of the PR and between 62cded2 and 63f2702.

📒 Files selected for processing (2)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts
  • packages/runtime/src/shell-run-manager.ts

Comment threadpackages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin decided whether to join finalization when persistObservation
was first called. A real PTY can exit while that persist is still in
flight, so the control returned a running snapshot. Re-join if
finalization has started, and wait for a terminal observation in the
queued-cut test instead of assuming the first snapshot is already done.
Fixesapache#3077
Generated-by: Grok
Hold the writeStdin persist until PTY finalization has started, then
assert the control result itself is completed. The existing queued-cut
test can still wait for a later snapshot.
Generated-by: Grok
@1625567290
1625567290force-pushed the fix/shell-run-finalization-wait branch from 63f2702 to c562830CompareAugust 16, 2026 03:10

@M4n5terM4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@M4n5ter
M4n5ter merged commit 2666a57 into apache:mainAug 16, 2026
12 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flaky test: shell-run-manager "joins finalization when a real PTY exits before a queued control cut"

2 participants

@1625567290@M4n5ter
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(runtime): join PTY finalization after a racing persist by 1625567290 · Pull Request #3082 · apache/maka · GitHub
Skip to content

fix(runtime): join PTY finalization after a racing persist - #3082

Merged
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait
Aug 16, 2026
Merged

fix(runtime): join PTY finalization after a racing persist#3082
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

joins finalization when a real PTY exits before a queued control cut failed on CI with expected completed, got running even though #3075 did not touch runtime. Re-running the same commit passed.

writeStdin asked persistObservation whether to join finalization at call time. A real PTY can exit while that persist is still in flight, so the control returned the running snapshot. After persist, if driverExit / finalizeOnce is set, join the terminal record.

The test no longer assumes that first snapshot is already terminal: if it is still running, it waits with waitForTerminalShellRun (15s) before asserting completed.

Fixes#3077

Verification

  • npm --workspace @maka/core run build
  • npm --workspace @maka/runtime run build
  • node --test --test-name-pattern 'joins finalization when a real PTY exits before a queued control cut' packages/runtime/dist/__tests__/shell-run-manager.test.js5/5 repeats
  • Nearby queued-control cases — 3/3
  • Full shell-run-manager.test.js57 pass, 3 skip (Windows PowerShell)
  • biome check on the two files — clean

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

Grok authored the implementation, tests, and this PR description.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7be63958-f9a8-4f80-9f40-2e13e1eeab49

📥 Commits

Reviewing files that changed from the base of the PR and between 63f2702 and c562830.

📒 Files selected for processing (1)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

Problem solved

writeStdin could return a stale running snapshot when the PTY exited while persistObservation was in flight. The change re-joins finalization when driverExit or finalizeOnce indicates that finalization has started. This allows writeStdin to return the terminal record.

The test now waits for a terminal shell-run snapshot before it asserts completed, output dimensions, and revision consistency. It also covers PTY finalization during persistence, including resize, driver disposal, and cleanup.

Design assessment

The change extends the existing PTY finalization and durable observation flow. It does not create a parallel source of truth or change a public contract.

The production fix is small and located at the race point. The test synchronization and added regression coverage are necessary to verify asynchronous finalization. No deletion or simplification is evident without weakening coverage.

Validation and risks

Reported validation includes successful builds, targeted and full test passes, and clean Biome checks. Direct evidence of the current required-check status is unavailable, so that status remains unverified.

The main behavioral risk is that writeStdin can now wait for terminal finalization instead of returning an intermediate running snapshot when PTY exit or finalization has begun. The added wait could affect timing under those conditions. The regression tests cover this race and related cleanup behavior.

Review-relevant risks

No protected-area effect was identified in the current diff. The person performing the merge reviews the final diff, and a maintainer makes the final determination.

Walkthrough

writeStdin now joins PTY finalization when exit or finalization begins after persistence. The tests wait for the terminal snapshot before checking completion, dimensions, durable revision, and cleanup.

Changes

PTY finalization synchronization

Layer / File(s)Summary
Join finalization and validate terminal state
packages/runtime/src/shell-run-manager.ts, packages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin now waits for live.finished, marks the terminal record observed, and returns it. The tests cover terminal snapshots, PTY disposal, resize, final dimensions, and durable revision.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to c5628

This localized runtime change ensures PTY runs reach their terminal record after a racing persist; no actionable merge-blocking risk remains after normal checks.

Suggested reviewers:m4n5ter, astro-han, jackwener

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly and concisely describes the PTY finalization race fix.
Description check✅ PassedThe description includes the problem, issue reference, implementation, verification results, AI disclosure, and completed checklist items.
Linked Issues check✅ PassedThe implementation and tests address issue #3077 by joining PTY finalization after persistence races and waiting for terminal state.
Out of Scope Changes check✅ PassedAll changes are limited to PTY finalization behavior and tests required by issue #3077.
Ai Use Disclosure✅ PassedThe PR selects generative tooling and names Grok with scope; both introduced commits have standalone Generated-by: Grok trailers.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d3a6126-a1e6-4364-bf74-587d6dd3c61b

📥 Commits

Reviewing files that changed from the base of the PR and between 62cded2 and 63f2702.

📒 Files selected for processing (2)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts
  • packages/runtime/src/shell-run-manager.ts

Comment threadpackages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin decided whether to join finalization when persistObservation
was first called. A real PTY can exit while that persist is still in
flight, so the control returned a running snapshot. Re-join if
finalization has started, and wait for a terminal observation in the
queued-cut test instead of assuming the first snapshot is already done.
Fixesapache#3077
Generated-by: Grok
Hold the writeStdin persist until PTY finalization has started, then
assert the control result itself is completed. The existing queued-cut
test can still wait for a later snapshot.
Generated-by: Grok
@1625567290
1625567290force-pushed the fix/shell-run-finalization-wait branch from 63f2702 to c562830CompareAugust 16, 2026 03:10

@M4n5terM4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@M4n5ter
M4n5ter merged commit 2666a57 into apache:mainAug 16, 2026
12 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flaky test: shell-run-manager "joins finalization when a real PTY exits before a queued control cut"

2 participants

@1625567290@M4n5ter
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix(runtime): join PTY finalization after a racing persist by 1625567290 · Pull Request #3082 · apache/maka · GitHub
Skip to content

fix(runtime): join PTY finalization after a racing persist - #3082

Merged
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait
Aug 16, 2026
Merged

fix(runtime): join PTY finalization after a racing persist#3082
M4n5ter merged 2 commits into
apache:mainfrom
1625567290:fix/shell-run-finalization-wait

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

joins finalization when a real PTY exits before a queued control cut failed on CI with expected completed, got running even though #3075 did not touch runtime. Re-running the same commit passed.

writeStdin asked persistObservation whether to join finalization at call time. A real PTY can exit while that persist is still in flight, so the control returned the running snapshot. After persist, if driverExit / finalizeOnce is set, join the terminal record.

The test no longer assumes that first snapshot is already terminal: if it is still running, it waits with waitForTerminalShellRun (15s) before asserting completed.

Fixes#3077

Verification

  • npm --workspace @maka/core run build
  • npm --workspace @maka/runtime run build
  • node --test --test-name-pattern 'joins finalization when a real PTY exits before a queued control cut' packages/runtime/dist/__tests__/shell-run-manager.test.js5/5 repeats
  • Nearby queued-control cases — 3/3
  • Full shell-run-manager.test.js57 pass, 3 skip (Windows PowerShell)
  • biome check on the two files — clean

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

Grok authored the implementation, tests, and this PR description.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7be63958-f9a8-4f80-9f40-2e13e1eeab49

📥 Commits

Reviewing files that changed from the base of the PR and between 63f2702 and c562830.

📒 Files selected for processing (1)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

Problem solved

writeStdin could return a stale running snapshot when the PTY exited while persistObservation was in flight. The change re-joins finalization when driverExit or finalizeOnce indicates that finalization has started. This allows writeStdin to return the terminal record.

The test now waits for a terminal shell-run snapshot before it asserts completed, output dimensions, and revision consistency. It also covers PTY finalization during persistence, including resize, driver disposal, and cleanup.

Design assessment

The change extends the existing PTY finalization and durable observation flow. It does not create a parallel source of truth or change a public contract.

The production fix is small and located at the race point. The test synchronization and added regression coverage are necessary to verify asynchronous finalization. No deletion or simplification is evident without weakening coverage.

Validation and risks

Reported validation includes successful builds, targeted and full test passes, and clean Biome checks. Direct evidence of the current required-check status is unavailable, so that status remains unverified.

The main behavioral risk is that writeStdin can now wait for terminal finalization instead of returning an intermediate running snapshot when PTY exit or finalization has begun. The added wait could affect timing under those conditions. The regression tests cover this race and related cleanup behavior.

Review-relevant risks

No protected-area effect was identified in the current diff. The person performing the merge reviews the final diff, and a maintainer makes the final determination.

Walkthrough

writeStdin now joins PTY finalization when exit or finalization begins after persistence. The tests wait for the terminal snapshot before checking completion, dimensions, durable revision, and cleanup.

Changes

PTY finalization synchronization

Layer / File(s)Summary
Join finalization and validate terminal state
packages/runtime/src/shell-run-manager.ts, packages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin now waits for live.finished, marks the terminal record observed, and returns it. The tests cover terminal snapshots, PTY disposal, resize, final dimensions, and durable revision.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to c5628

This localized runtime change ensures PTY runs reach their terminal record after a racing persist; no actionable merge-blocking risk remains after normal checks.

Suggested reviewers:m4n5ter, astro-han, jackwener

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly and concisely describes the PTY finalization race fix.
Description check✅ PassedThe description includes the problem, issue reference, implementation, verification results, AI disclosure, and completed checklist items.
Linked Issues check✅ PassedThe implementation and tests address issue #3077 by joining PTY finalization after persistence races and waiting for terminal state.
Out of Scope Changes check✅ PassedAll changes are limited to PTY finalization behavior and tests required by issue #3077.
Ai Use Disclosure✅ PassedThe PR selects generative tooling and names Grok with scope; both introduced commits have standalone Generated-by: Grok trailers.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d3a6126-a1e6-4364-bf74-587d6dd3c61b

📥 Commits

Reviewing files that changed from the base of the PR and between 62cded2 and 63f2702.

📒 Files selected for processing (2)
  • packages/runtime/src/__tests__/shell-run-manager.test.ts
  • packages/runtime/src/shell-run-manager.ts

Comment threadpackages/runtime/src/__tests__/shell-run-manager.test.ts
writeStdin decided whether to join finalization when persistObservation
was first called. A real PTY can exit while that persist is still in
flight, so the control returned a running snapshot. Re-join if
finalization has started, and wait for a terminal observation in the
queued-cut test instead of assuming the first snapshot is already done.
Fixesapache#3077
Generated-by: Grok
Hold the writeStdin persist until PTY finalization has started, then
assert the control result itself is completed. The existing queued-cut
test can still wait for a later snapshot.
Generated-by: Grok
@1625567290
1625567290force-pushed the fix/shell-run-finalization-wait branch from 63f2702 to c562830CompareAugust 16, 2026 03:10

@M4n5terM4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@M4n5ter
M4n5ter merged commit 2666a57 into apache:mainAug 16, 2026
12 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flaky test: shell-run-manager "joins finalization when a real PTY exits before a queued control cut"

2 participants

@1625567290@M4n5ter